On Thu, Nov 5, 2020 at 5:04 PM Leo Li <leoyang...@nxp.com> wrote: > > > > > -----Original Message----- > > From: Rasmus Villemoes <li...@rasmusvillemoes.dk> > > Sent: Tuesday, November 3, 2020 2:03 AM > > To: Leo Li <leoyang...@nxp.com>; Biwen Li (OSS) <biwen...@oss.nxp.com>; > > shawn...@kernel.org; robh...@kernel.org; mark.rutl...@arm.com; Z.q. > > Hou <zhiqiang....@nxp.com>; t...@linutronix.de; ja...@lakedaemon.net; > > m...@kernel.org > > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > > <jiafei....@nxp.com>; Xiaobo Xie <xiaobo....@nxp.com>; linux-arm- > > ker...@lists.infradead.org > > 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.
Hi Biwen, Would you send a new version of this patch? Thanks. Regards, Leo
