Re: [PATCH] target/nios2: Shadow register set, EIC and VIC

[Peter Maydell]

On Sun, 20 Feb 2022 at 15:37, Amir Gonnen <amir.gonnen@neuroblade.ai> wrote:
> I can split the VIC from the core+EIC changes, although the core+EIC changes 
> make little sense without a VIC to interact with them.
> However, I'm not sure how to split the changes to the nios2 core into 
> multiple patches.
> The shadow register set, together with the EIC interface and interrupt 
> handling are very much tied together.
>
> Regarding the "fixed eret" - perhaps I didn't phrase it right. What I meant 
> is that eret was changed to work correctly in the presence of a shadow 
> register set.
> So, the changes to eret are part of the shadow register set support on the 
> core and cannot exist on their own.

> > https://www.qemu.org/docs/master/devel/submitting-a-patch.html
> > is our guidelines on patch formatting.
>
> In fact, I tried to follow them. I've also run checkpatch.pl, etc.
> Could you please point out where I failed to follow them or what I'm missing?

Mostly it was the combination of two things:
 (1) a large patch that touches both device and cpu code
 (2) a commit message that lists half a dozen things with very
     low level of detail

This triggers my "probably needs to be split" heuristic, and
also "probably somebody's first patch". So I mentioned the URL in
case you hadn't seen it yet.

> I tested this on Neuroblade board with JUART. I did not wire it to an 
> existing demo board.

I think we'd like an upstream board that uses this. Otherwise it's
dead code, from our point of view.

That said, my suggested split would be something like:
 * VIC device model
 * CPU changes, in digestible chunks
    -- these should all be conditional on the "behave the same as
    the old code" default value of intc_present, I assume, so it
    doesn't matter if they don't all arrive in the codebase in the
    same commit. If there are any changes which *do* change behaviour
    even for intc_present=true, they definitely need to be split
    out, anyway.
 * board changes to use the new device

thanks
-- PMM