Re: [PATCH v5 01/11] hw/misc: Add NPCM7xx System Global Control Registers device model

[Havard Skinnemoen]

On Fri, Jul 10, 2020 at 11:46 PM Havard Skinnemoen
<hskinnemoen@google.com> wrote:
>
> On Fri, Jul 10, 2020 at 2:31 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> wrote:
> >
> > On 7/9/20 7:42 PM, Havard Skinnemoen wrote:
> > > On Thu, Jul 9, 2020 at 10:24 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > > wrote:
> > >> On 7/9/20 7:09 PM, Havard Skinnemoen wrote:
> > >>> On Thu, Jul 9, 2020 at 9:23 AM Philippe Mathieu-Daudé <f4bug@amsat.org> 
> > >>> wrote:
> > >>>> On 7/9/20 8:43 AM, Havard Skinnemoen wrote:
> > >>>>> On Wed, Jul 8, 2020 at 11:04 PM Philippe Mathieu-Daudé 
> > >>>>> <f4bug@amsat.org> wrote:
> > >>>>>> On 7/9/20 2:35 AM, Havard Skinnemoen wrote:
> > >>>>>>> Implement a device model for the System Global Control Registers in 
> > >>>>>>> the
> > >>>>>>> NPCM730 and NPCM750 BMC SoCs.
> > >>>>>>>
> > >>>>>>> This is primarily used to enable SMP boot (the boot ROM spins 
> > >>>>>>> reading
> > >>>>>>> the SCRPAD register) and DDR memory initialization; other registers 
> > >>>>>>> are
> > >>>>>>> best effort for now.
> > >>>>>>>
> > >>>>>>> The reset values of the MDLR and PWRON registers are determined by 
> > >>>>>>> the
> > >>>>>>> SoC variant (730 vs 750) and board straps respectively.
> > >>>>>>>
> > >>>>>>> Reviewed-by: Joel Stanley <joel@jms.id.au>
> > >>>>>>> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> > >>>>>>> Signed-off-by: Havard Skinnemoen <hskinnemoen@google.com>
> > >>>>>>> ---
> > [...]
> > >>>>>> Maybe use HWADDR_PRIx instead of casting to int?
> > >>>>>
> > >>>>> Will do, thanks!
> > >>>>
> > >>>> Glad you are not annoyed by my review comments.
> > >>>>
> > >>>> FYI your series quality is in my top-4 "adding new machine to QEMU".
> > >>>> I'd like to use it as example to follow.
> > >>>>
> > >>>> I am slowly reviewing because this is not part of my work duties,
> > >>>> so I do that in my free time before/after work, which is why I can
> > >>>> barely do more that 2 per day, as I have to learn the SoC and
> > >>>> cross check documentation (or in this case, existing firmware code
> > >>>> since there is no public doc).
> > >>>
> > >>> Your feedback is super valuable, thanks a lot. I really appreciate it.
> > >>
> > >> OK I'll continue, but might not have time until next week.
> > >> After I plan to test too.
> > >
> > > OK, I'll try to post a v6 before the weekend, unless you prefer that I
> > > hold off until you've reviewed the whole series.
> >
> > I don't mind, if it is not too much overhead on your side.
> >
> > What I noticed on the QEMU community is:
> >
> > - If a series reviewed the same day and is almost done except
> >   a single bugfix, it is OK to repost the same day, so the
> >   maintainer is likely to queue it directly.
> >
> > - If there are various reviews, and you do gradual improvements,
> >   it is OK to repost every 3 days. Else reviewers tend to skip/
> >   postpone your series for later.
> >
> > - Pinging for review before 1 week passed is stressing reviewers,
> >   except in case of critical security bug (CVE) or during freeze.
> >
> > - Series with integrated test provided are usually reviewed first.
>
> This is very helpful, thanks.
>
> > >> What would be useful is an acceptance test (see examples in
> > >> tests/acceptance/), using the artefacts from the link you shared
> > >> elsewhere:
> > >> https://openpower.xyz/job/run-meta-ci/distro=ubuntu,label=builder,target=gsj/lastSuccessfulBuild/artifact/openbmc/build/tmp/deploy/images/gsj/
> > >
> > > Yes, tests are definitely on my radar. I'm working on SPI flash
> > > qtests. I haven't had much success with avocado yet, but I'll keep
> > > trying (perhaps using docker to control the environment more tightly).
> >
> > This might help:
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg704907.html
>
> Thanks, I got it working now.
>
> > > Since the 5.1 release is frozen now, I might post some followup
> > > patches before this series is merged, e.g. tests, bootrom
> > > submodule+blob, more peripherals, etc. Is it preferable to keep this
> > > series frozen (modulo API updates) since you spent a lot of time
> > > reviewing it, and post the new stuff separately, or is it better to
> > > add new patches to the end of the series and resend the whole thing?
> >
> > If you rework a peripheral, you need to reset the Reviewed-by/Tested-by
> > tags. If you add new peripherals, you only need to reset these tags on
> > the SoC patch. I'm fine either way, I use git-backport-diff to see the
> > SoC changes easily:
> >
> > https://github.com/codyprime/git-scripts/blob/master/git-backport-diff
>
> I've been adding new peripherals incrementally after the basic SoC
> support patch. Is that OK to do without resetting the tags?
>
> But it's more likely that I'll add other things than peripherals next,
> i.e. bootrom and tests.
>
> > >
> > >> But these are apparently not stable links (expire after 30 days?).
> > >
> > > Sorry, I'm too ignorant about Jenkins to know. I'll see if I can
> > > figure something out.
> >
> > What I do in that case is take the binary used for the test,
> > write a comment and push it in a stable branch to my own repo:
> > https://github.com/philmd/qemu-testing-blob/ and use the stable
> > url in the test.
> >
> > We know QEMU emulation worked with this particular binary at some
> > point. We want to avoid regressions in QEMU, so let's keep testing
> > what we know worked. We don't want to track 2 bugs at a time (one
> > in the updated guest and one in QEMU).
>
> Good point. I'll see if I can upload images to github. I might fork
> the openbmc repository and attach binaries to a github release, to
> make it clear where the binaries came from.
>
> I accidentally broke my test image and had some trouble recreating it,
> so I ended up reworking my various hacks a bit. The good news is that
> I got most of them turned into proper bug fixes that I can send
> upstream.
>
> It might take a little longer than I said previously, but I'll try to
> include acceptance tests in the next series.

I uploaded the images here:

https://github.com/hskinnemoen/openbmc/releases/tag/20200711-gsj-qemu-0

I used them to implement an acceptance test that I'll include in v6.