RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt

[Leo Li]

> -----Original Message-----
> From: Rasmus Villemoes <linux@xxxxxxxxxxxxxxxxxx>
> Sent: Tuesday, November 3, 2020 2:03 AM
> To: Leo Li <leoyang.li@xxxxxxx>; Biwen Li (OSS) <biwen.li@xxxxxxxxxxx>;
> shawnguo@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; Z.q.
> Hou <zhiqiang.hou@xxxxxxx>; tglx@xxxxxxxxxxxxx; jason@xxxxxxxxxxxxxx;
> maz@xxxxxxxxxx
> Cc: devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Jiafei Pan
> <jiafei.pan@xxxxxxx>; Xiaobo Xie <xiaobo.xie@xxxxxxx>; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A
> external interrupt
> 
> On 02/11/2020 22.22, Leo Li wrote:
> >>>
> >>> Where did you get this information that the register on LS1043 and
> >>> LS1046 is bit reversed?  I cannot find such information in the RM.
> >>> And does this mean all other SCFG registers are also bit reversed?
> >>> If this is some information that is not covered by the RM, we
> >>> probably should clarify it in the code and the commit message.
> >> Hi Leo,
> >>
> >> I directly use the same logic to write the bit(field IRQ0~11INTP) of
> >> the register SCFG_INTPCR in LS1043A and LS1046A.
> >> Such as,
> >> if I want to control the polarity of IRQ0(field IRQ0INTP, IRQ0 is
> >> active low) of LS1043A/LS1046A, then I just need write a value 1 << (31 - 0)
> to it.
> >> The logic depends on register's definition in LS1043A/LS1046A's RM.
> >
> > Ok.  The SCFG_SCFGREVCR seems to be a one-off fixup only existed on
> LS1021.  And it is mandatory to be bit_reversed according to the RM which is
> already taken care of in the RCW.  So the bit reversed case should be the only
> case supported otherwise a lot of other places for SCFG access should be
> failed.
> >
> > I think we should remove the bit_reverse thing all together from the driver
> for good.  This will prevent future confusion.  Rasmus, what do you think?
> 
> Yes, all the ls1021a-derived boards I know of do have something like
> 
> # Initialize bit reverse of SCFG registers
> 09570200 ffffffff
> 
> in their pre-boot-loader config file. And yes, the RM does say
> 
>   This register must be written 0xFFFF_FFFF as a part of
>   initialization sequence before writing to any other SCFG
>   register.
> 
> but nowhere does it say "or else...", nor a little honest addendum "because
> we accidentally released broken silicon with this misfeature _and_ wrong
> POR value".

Yeah.  I do think they messed up at the beginning when trying to integrate the big endian registers on little endian core.  It is good that we are doing it correctly in later SoCs.

> 
> Can we have an official statement from NXP stating that SCFGREVCR is a
> hardware design bug? And can you send it through a time-machine so I had it
> three years ago avoiding the whole "fsl,bit-reverse device-tree-property, no,
> read the register if you're on a ls1021a and decide" hullabaloo.

I'm not sure if it is possible to update the related documents right now for this.  But definitely it was not your fault to have introduced this in the driver due to the confusion from document.  My suggestion to remove it is just to prevent this from causing more confusions in the future as this driver is used on more SoCs.

Regards,
Leo