RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> > > >>> 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 > > > > > > in their pre-boot-loader config file. And yes, the RM does say > > > > > > This register must be written 0x_ 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. Hi Leo, sure, np. > > Regards, > Leo
Re: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
On Thu, Nov 5, 2020 at 5:04 PM Leo Li wrote: > > > > > -Original Message- > > From: Rasmus Villemoes > > Sent: Tuesday, November 3, 2020 2:03 AM > > To: Leo Li ; Biwen Li (OSS) ; > > shawn...@kernel.org; robh...@kernel.org; mark.rutl...@arm.com; Z.q. > > Hou ; t...@linutronix.de; ja...@lakedaemon.net; > > m...@kernel.org > > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > > ; Xiaobo Xie ; 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 > > > > in their pre-boot-loader config file. And yes, the RM does say > > > > This register must be written 0x_ 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
RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> -Original Message- > From: Rasmus Villemoes > Sent: Tuesday, November 3, 2020 2:03 AM > To: Leo Li ; Biwen Li (OSS) ; > shawn...@kernel.org; robh...@kernel.org; mark.rutl...@arm.com; Z.q. > Hou ; t...@linutronix.de; ja...@lakedaemon.net; > m...@kernel.org > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > ; Xiaobo Xie ; 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 > > in their pre-boot-loader config file. And yes, the RM does say > > This register must be written 0x_ 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
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 in their pre-boot-loader config file. And yes, the RM does say This register must be written 0x_ 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". 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. Rasmus
RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> -Original Message- > From: Biwen Li (OSS) > Sent: Monday, November 2, 2020 12:15 AM > To: Leo Li ; Rasmus Villemoes > ; Biwen Li (OSS) ; > shawn...@kernel.org; robh...@kernel.org; mark.rutl...@arm.com; Z.q. > Hou ; t...@linutronix.de; ja...@lakedaemon.net; > m...@kernel.org > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > ; Xiaobo Xie ; linux-arm- > ker...@lists.infradead.org > Subject: RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A > external interrupt > > > > > > > > > Caution: EXT Email > > > > > > > > On 27/10/2020 05.46, Biwen Li wrote: > > > > > From: Hou Zhiqiang > > > > > > > > > > Add an new IRQ chip declaration for LS1043A and LS1088A > > > > > - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A. > > SCFG_INTPCR[31:0] > > > > > of these SoCs is stored/read as SCFG_INTPCR[0:31] defaultly(bit > > > > > reverse) > > > > > > > > s/defaultly/by default/ I suppose. But what does that mean? Is it > > > > still configurable, just now through some undocumented register? > > > > If that register still exists, does it now have a reset value of > > > > all-ones as opposed to the ls1021 case? If it's not configurable, > > > > then describing the situation as "by default" is confusing and > > > > wrong, it should just say "On LS1043A, LS1046A, SCFG_INTPCR is > > > > stored/read bit- > > > reversed." > > > Okay, got it. Will update it in v3. Thanks. > > > > Hi Biwen, > > > > 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? Regards, Leo > > Regards, > Biwen > > > > > Regards, > > Leo > > > > > > > > > > > > > > > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > > > > > > > Signed-off-by: Hou Zhiqiang > > > > > Signed-off-by: Biwen Li > > > > > --- > > > > > Change in v2: > > > > > - add despcription of bit reverse > > > > > - update copyright > > > > > > > > > > drivers/irqchip/irq-ls-extirq.c | 10 +- > > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/irqchip/irq-ls-extirq.c > > > > > b/drivers/irqchip/irq-ls-extirq.c index > > > > > 4d1179fed77c..9587bc2607fc > > > > > 100644 > > > > > --- a/drivers/irqchip/irq-ls-extirq.c > > > > > +++ b/drivers/irqchip/irq-ls-extirq.c > > > > > @@ -1,5 +1,8 @@ > > > > > // SPDX-License-Identifier: GPL-2.0 > > > > > - > > > > > +/* > > > > > + * Author: Rasmus Villemoes > > > > > > > > If I wanted my name splattered all over the files I touch or add, > > > > I'd add it myself, TYVM. The git history is plenty fine for > > > > recording authorship as far as I'm concerned, and I absolutely > > > > abhor having to skip over any kind of legalese boilerplate when opening > a file. > > > Okay, got it. Will drop it in v3. Thanks. > > > > > > > > Rasmus
RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> > > > > > Caution: EXT Email > > > > > > On 27/10/2020 05.46, Biwen Li wrote: > > > > From: Hou Zhiqiang > > > > > > > > Add an new IRQ chip declaration for LS1043A and LS1088A > > > > - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A. > SCFG_INTPCR[31:0] > > > > of these SoCs is stored/read as SCFG_INTPCR[0:31] defaultly(bit > > > > reverse) > > > > > > s/defaultly/by default/ I suppose. But what does that mean? Is it > > > still configurable, just now through some undocumented register? If > > > that register still exists, does it now have a reset value of > > > all-ones as opposed to the ls1021 case? If it's not configurable, > > > then describing the situation as "by default" is confusing and > > > wrong, it should just say "On LS1043A, LS1046A, SCFG_INTPCR is > > > stored/read bit- > > reversed." > > Okay, got it. Will update it in v3. Thanks. > > Hi Biwen, > > 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. Regards, Biwen > > Regards, > Leo > > > > > > > > > > > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > > > > > Signed-off-by: Hou Zhiqiang > > > > Signed-off-by: Biwen Li > > > > --- > > > > Change in v2: > > > > - add despcription of bit reverse > > > > - update copyright > > > > > > > > drivers/irqchip/irq-ls-extirq.c | 10 +- > > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/irqchip/irq-ls-extirq.c > > > > b/drivers/irqchip/irq-ls-extirq.c index 4d1179fed77c..9587bc2607fc > > > > 100644 > > > > --- a/drivers/irqchip/irq-ls-extirq.c > > > > +++ b/drivers/irqchip/irq-ls-extirq.c > > > > @@ -1,5 +1,8 @@ > > > > // SPDX-License-Identifier: GPL-2.0 > > > > - > > > > +/* > > > > + * Author: Rasmus Villemoes > > > > > > If I wanted my name splattered all over the files I touch or add, > > > I'd add it myself, TYVM. The git history is plenty fine for > > > recording authorship as far as I'm concerned, and I absolutely abhor > > > having to skip over any kind of legalese boilerplate when opening a file. > > Okay, got it. Will drop it in v3. Thanks. > > > > > > Rasmus
RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> -Original Message- > From: Biwen Li > Sent: Tuesday, October 27, 2020 2:48 AM > To: Rasmus Villemoes ; Biwen Li (OSS) > ; shawn...@kernel.org; robh...@kernel.org; > mark.rutl...@arm.com; Leo Li ; Z.q. Hou > ; t...@linutronix.de; ja...@lakedaemon.net; > m...@kernel.org > Cc: devicet...@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > ; Xiaobo Xie ; linux-arm- > ker...@lists.infradead.org > Subject: RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A > external interrupt > > > > > Caution: EXT Email > > > > On 27/10/2020 05.46, Biwen Li wrote: > > > From: Hou Zhiqiang > > > > > > Add an new IRQ chip declaration for LS1043A and LS1088A > > > - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A. SCFG_INTPCR[31:0] > > > of these SoCs is stored/read as SCFG_INTPCR[0:31] defaultly(bit > > > reverse) > > > > s/defaultly/by default/ I suppose. But what does that mean? Is it > > still configurable, just now through some undocumented register? If > > that register still exists, does it now have a reset value of all-ones > > as opposed to the ls1021 case? If it's not configurable, then > > describing the situation as "by default" is confusing and wrong, it > > should just say "On LS1043A, LS1046A, SCFG_INTPCR is stored/read bit- > reversed." > Okay, got it. Will update it in v3. Thanks. Hi Biwen, 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. Regards, Leo > > > > > > > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > > > Signed-off-by: Hou Zhiqiang > > > Signed-off-by: Biwen Li > > > --- > > > Change in v2: > > > - add despcription of bit reverse > > > - update copyright > > > > > > drivers/irqchip/irq-ls-extirq.c | 10 +- > > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/irqchip/irq-ls-extirq.c > > > b/drivers/irqchip/irq-ls-extirq.c index 4d1179fed77c..9587bc2607fc > > > 100644 > > > --- a/drivers/irqchip/irq-ls-extirq.c > > > +++ b/drivers/irqchip/irq-ls-extirq.c > > > @@ -1,5 +1,8 @@ > > > // SPDX-License-Identifier: GPL-2.0 > > > - > > > +/* > > > + * Author: Rasmus Villemoes > > > > If I wanted my name splattered all over the files I touch or add, I'd > > add it myself, TYVM. The git history is plenty fine for recording > > authorship as far as I'm concerned, and I absolutely abhor having to > > skip over any kind of legalese boilerplate when opening a file. > Okay, got it. Will drop it in v3. Thanks. > > > > Rasmus
RE: [EXT] Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt
> > Caution: EXT Email > > On 27/10/2020 05.46, Biwen Li wrote: > > From: Hou Zhiqiang > > > > Add an new IRQ chip declaration for LS1043A and LS1088A > > - compatible "fsl,ls1043a-extirq" for LS1043A, LS1046A. SCFG_INTPCR[31:0] > > of these SoCs is stored/read as SCFG_INTPCR[0:31] defaultly(bit > > reverse) > > s/defaultly/by default/ I suppose. But what does that mean? Is it still > configurable, just now through some undocumented register? If that register > still exists, does it now have a reset value of all-ones as opposed to the > ls1021 > case? If it's not configurable, then describing the situation as "by default" > is > confusing and wrong, it should just say "On LS1043A, LS1046A, SCFG_INTPCR > is stored/read bit-reversed." Okay, got it. Will update it in v3. Thanks. > > > > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > Signed-off-by: Hou Zhiqiang > > Signed-off-by: Biwen Li > > --- > > Change in v2: > > - add despcription of bit reverse > > - update copyright > > > > drivers/irqchip/irq-ls-extirq.c | 10 +- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/irqchip/irq-ls-extirq.c > > b/drivers/irqchip/irq-ls-extirq.c index 4d1179fed77c..9587bc2607fc > > 100644 > > --- a/drivers/irqchip/irq-ls-extirq.c > > +++ b/drivers/irqchip/irq-ls-extirq.c > > @@ -1,5 +1,8 @@ > > // SPDX-License-Identifier: GPL-2.0 > > - > > +/* > > + * Author: Rasmus Villemoes > > If I wanted my name splattered all over the files I touch or add, I'd add it > myself, > TYVM. The git history is plenty fine for recording authorship as far as I'm > concerned, and I absolutely abhor having to skip over any kind of legalese > boilerplate when opening a file. Okay, got it. Will drop it in v3. Thanks. > > Rasmus
