Re: [PATCH v2 6/8] irqchip/al-fic: support error and fatal outputs and FIC v2

[Radu Rendec <radu@rendec.net>]

On Mon, 2026-10-05 at 11:19 +0000, Farber, Eliav wrote:
> On Sun, 2026-10-05 at 01:01 +0000, Radu Rendec wrote:
> > On Sun, 2026-09-27 at 08:06 +0000, Eliav Farber wrote:
> > 
> > > +   /* make sure the controller works in non msi_x mode */
> > > +   control |= CONTROL_MASK_MSI_X;
> > 
> > The side effect of this is that all the other bits previously set in
> > the AL_FIC_CONTROL register are preserved, whereas before this patch
> > they were reset by initializing "control" to CONTROL_MASK_MSI_X. Is
> > this intentional? If it is, then perhaps it deserves a comment because
> > it looks like a behavior change.
> 
> It is intentional, but it is not actually a behaviour change. Every
> writable bit in this register resets to 0; the only non-zero reset field
> is the revision, which is read-only. So at probe the read-modify-write
> produces the same value as building it from CONTROL_MASK_MSI_X alone.

I see, so you're relying on all bits being 0 out of hardware reset
(except for the revision bits). The driver can only be compiled as
built-in, so it will never be loaded multiple times during the lifetime
of the kernel, which means the hardware will always be in the 
after-reset state when the driver initializes. I know nothing about
this particular hardware, so the question may be silly - is the
hardware guaranteed to also reset in the case of a "soft" reboot 
(e.g. running the "reboot" command)?

> I kept the read-modify-write because it is the better practice - it costs
> nothing and does not rely on the reset value staying 0 - and because the
> version field now has to be read from this register anyway. I did not add
> a code comment since there is no surprising behaviour to flag, but I added
> a paragraph to the commit message explaining the equivalence. Let me know
> if you would still rather see a comment at the write site.

No, I think the commit message explains it very clearly, so a comment
at the call site isn't necessary.

I agree that read-modify-write is generally good practice. However, as
part of their initialization, drivers should make sure that the
hardware is in a known state - either by resetting it or by setting
registers to fixed values. In this case, you're assuming it's already
in a specific state - which is fine if that's guaranteed to always be
the hardware reset state. In other words, I just want to make sure
you're not missing a case when the hardware can be in a different state
than after-reset when the driver initializes.

-- 
Best regards,
Radu