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

[Philippe Mathieu-Daudé]

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.

>> 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

> 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

> 
>> 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).

Regards,

Phil.