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

[Havard Skinnemoen <hskinnemoen@google.com>]

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.

Havard