From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 110018 invoked by alias); 4 Sep 2017 21:14:30 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 110008 invoked by uid 89); 4 Sep 2017 21:14:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS,SPF_PASS autolearn=ham version=3.3.2 spammy=icp, cup X-HELO: smtp.polymtl.ca Received: from smtp.polymtl.ca (HELO smtp.polymtl.ca) (132.207.4.11) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 04 Sep 2017 21:14:25 +0000 Received: from simark.ca (simark.ca [158.69.221.121]) (authenticated bits=0) by smtp.polymtl.ca (8.14.7/8.14.7) with ESMTP id v84LEFJj016938 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT) for ; Mon, 4 Sep 2017 17:14:21 -0400 Received: by simark.ca (Postfix, from userid 112) id 176CE1EA2F; Mon, 4 Sep 2017 17:14:15 -0400 (EDT) Received: from simark.ca (localhost [127.0.0.1]) by simark.ca (Postfix) with ESMTP id 497C11E974; Mon, 4 Sep 2017 17:14:13 -0400 (EDT) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Mon, 04 Sep 2017 21:14:00 -0000 From: Simon Marchi To: Stafford Horne Cc: GDB patches , Openrisc , Mike Frysinger Subject: Re: [PATCH v4 3/5] sim: or1k: add or1k target to sim In-Reply-To: References: Message-ID: <617acff0490f4ae9ffcf961fb55b2ef1@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.3.0 X-Poly-FromMTA: (simark.ca [158.69.221.121]) at Mon, 4 Sep 2017 21:14:15 +0000 X-IsSubscribed: yes X-SW-Source: 2017-09/txt/msg00070.txt.bz2 Hi Stafford, I started to look at this patch, and I have to say that it's quite difficult for an outsider like me to follow. I can see what the code is doing, and can correlate some parts with the openrisc spec. But since there are no comments at all, I have no idea what the intents are, so I can't really tell whether the code is right or wrong wrt the intent. Maybe it's just because of my lack of experience in the area, and somebody familiar with the sim code would find it obvious. Still, I think adding some comments would help future contributors. For example, in or1k_cpu_init: > +void > +or1k_cpu_init (SIM_DESC sd, sim_cpu * current_cpu, const USI or1k_vr, > + const USI or1k_upr, const USI or1k_cpucfgr) > +{ > + SET_H_SYS_VR (or1k_vr); > + SET_H_SYS_UPR (or1k_upr); > + SET_H_SYS_CPUCFGR (or1k_cpucfgr); > + > +#define CHECK_SPR_FIELD(GROUP, INDEX, FIELD, test) > \ > + do { > \ > + USI field = GET_H_##SYS##_##INDEX##_##FIELD (); > \ > + if (!(test)) { > \ > + sim_io_eprintf(sd, "WARNING: unsupported %s field in %s > register: 0x%x\n", \ > + #FIELD, #INDEX, field); > \ > + } > \ > + } while (0) > + > + current_cpu->next_delay_slot = 0; > + current_cpu->delay_slot = 0; > + > + CHECK_SPR_FIELD (SYS, UPR, UP, field == 1); > + CHECK_SPR_FIELD (SYS, UPR, DCP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, ICP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, DMP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, MP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, IMP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, DUP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, PCUP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, PICP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, PMP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, TTP, field == 0); > + CHECK_SPR_FIELD (SYS, UPR, CUP, field == 0); > + > + CHECK_SPR_FIELD (SYS, CPUCFGR, NSGR, field == 0); > + CHECK_SPR_FIELD (SYS, CPUCFGR, CGF, field == 0); > + CHECK_SPR_FIELD (SYS, CPUCFGR, OB32S, field == 1); > + CHECK_SPR_FIELD (SYS, CPUCFGR, OB64S, field == 0); > + CHECK_SPR_FIELD (SYS, CPUCFGR, OF64S, field == 0); > + CHECK_SPR_FIELD (SYS, CPUCFGR, OV64S, field == 0); Here, I think a comment should explain what these checks are about. I understand they are checking for CPU features, making sure they are or aren't there, but why? > + > +#undef CHECK_SPR_FIELD > + > + SET_H_SYS_UPR_UP (1); This, looks fishy to me, but maybe there's a good reason for it to be there? In the checks above we made sure that the UP bit of the UPR register was set. Why do we need to set it here? I understand that there can't be (and we don't want) a comment for each source line, but at least some high level ones. Thanks, Simon