| Message ID | 20201027044619.41879-1-biwen.li@oss.nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt | expand |
On 27/10/2020 05.46, Biwen Li wrote: > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > 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." > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > Signed-off-by: Biwen Li <biwen.li@nxp.com> > --- > 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 <linux@rasmusvillemoes.dk> 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. Rasmus
> > Caution: EXT Email > > On 27/10/2020 05.46, Biwen Li wrote: > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > 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 <Zhiqiang.Hou@nxp.com> > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > --- > > 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 <linux@rasmusvillemoes.dk> > > 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
On 2020-10-27 04:46, Biwen Li wrote: > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > 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) > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > Signed-off-by: Biwen Li <biwen.li@nxp.com> You clearly couldn't be bothered to read what I wrote in my earlier replies. I'm thus ignoring this series... > --- > 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 <linux@rasmusvillemoes.dk> > + * Copyright 2020 NXP ... specially when you keep attributing someone else's copyright to NXP. M. -- Jazz is not dead. It just smells funny...
> > On 2020-10-27 04:46, Biwen Li wrote: > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > 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) > > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > > > > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > You clearly couldn't be bothered to read what I wrote in my earlier replies. I'm > thus ignoring this series... Okay, got it. > > > --- > > 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 <linux@rasmusvillemoes.dk> > > + * Copyright 2020 NXP > > ... specially when you keep attributing someone else's copyright to NXP. Then I don't know how to add the copyright, any suggestions? > > M. > -- > Jazz is not dead. It just smells funny...
On 2020-10-27 10:35, Biwen Li (OSS) wrote: >> >> On 2020-10-27 04:46, Biwen Li wrote: >> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> >> > >> > 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) >> > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA >> > >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> >> > Signed-off-by: Biwen Li <biwen.li@nxp.com> >> >> You clearly couldn't be bothered to read what I wrote in my earlier >> replies. I'm >> thus ignoring this series... > Okay, got it. >> >> > --- >> > 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 <linux@rasmusvillemoes.dk> >> > + * Copyright 2020 NXP >> >> ... specially when you keep attributing someone else's copyright to >> NXP. > Then I don't know how to add the copyright, any suggestions? Simple. You don't add anything. NXP's copyright doesn't apply to this file before this patch, and your changes are so trivial that they don't really warrant a mention. Furthermore, the git history already keeps track of who did what. M. -- Jazz is not dead. It just smells funny...
> >> On 2020-10-27 04:46, Biwen Li wrote: > >> > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > >> > > >> > 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) > >> > - compatible "fsl,ls1088a-extirq" for LS1088A, LS208xA, LX216xA > >> > > >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > >> > Signed-off-by: Biwen Li <biwen.li@nxp.com> > >> > >> You clearly couldn't be bothered to read what I wrote in my earlier > >> replies. I'm thus ignoring this series... > > Okay, got it. > >> > >> > --- > >> > 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 <linux@rasmusvillemoes.dk> > >> > + * Copyright 2020 NXP > >> > >> ... specially when you keep attributing someone else's copyright to > >> NXP. > > Then I don't know how to add the copyright, any suggestions? > > Simple. You don't add anything. NXP's copyright doesn't apply to this file > before this patch, and your changes are so trivial that they don't really warrant > a mention. Furthermore, the git history already keeps track of who did what. Okay, got it. Thanks. > > M. > -- > Jazz is not dead. It just smells funny...
> -----Original Message----- > From: Biwen Li <biwen.li@nxp.com> > Sent: Tuesday, October 27, 2020 2:48 AM > To: Rasmus Villemoes <linux@rasmusvillemoes.dk>; Biwen Li (OSS) > <biwen.li@oss.nxp.com>; shawnguo@kernel.org; robh+dt@kernel.org; > mark.rutland@arm.com; Leo Li <leoyang.li@nxp.com>; Z.q. Hou > <zhiqiang.hou@nxp.com>; tglx@linutronix.de; jason@lakedaemon.net; > maz@kernel.org > Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > <jiafei.pan@nxp.com>; Xiaobo Xie <xiaobo.xie@nxp.com>; linux-arm- > kernel@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 <Zhiqiang.Hou@nxp.com> > > > > > > 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 <Zhiqiang.Hou@nxp.com> > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > > --- > > > 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 <linux@rasmusvillemoes.dk> > > > > 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
> > > > > > Caution: EXT Email > > > > > > On 27/10/2020 05.46, Biwen Li wrote: > > > > From: Hou Zhiqiang <Zhiqiang.Hou@nxp.com> > > > > > > > > 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 <Zhiqiang.Hou@nxp.com> > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > > > --- > > > > 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 <linux@rasmusvillemoes.dk> > > > > > > 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
> -----Original Message----- > From: Biwen Li (OSS) <biwen.li@oss.nxp.com> > Sent: Monday, November 2, 2020 12:15 AM > To: Leo Li <leoyang.li@nxp.com>; Rasmus Villemoes > <linux@rasmusvillemoes.dk>; Biwen Li (OSS) <biwen.li@oss.nxp.com>; > shawnguo@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Z.q. > Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; jason@lakedaemon.net; > maz@kernel.org > Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > <jiafei.pan@nxp.com>; Xiaobo Xie <xiaobo.xie@nxp.com>; linux-arm- > kernel@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 <Zhiqiang.Hou@nxp.com> > > > > > > > > > > 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 <Zhiqiang.Hou@nxp.com> > > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com> > > > > > --- > > > > > 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 <linux@rasmusvillemoes.dk> > > > > > > > > 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
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". 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
> -----Original Message----- > From: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Sent: Tuesday, November 3, 2020 2:03 AM > To: Leo Li <leoyang.li@nxp.com>; Biwen Li (OSS) <biwen.li@oss.nxp.com>; > shawnguo@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Z.q. > Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; jason@lakedaemon.net; > maz@kernel.org > Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > <jiafei.pan@nxp.com>; Xiaobo Xie <xiaobo.xie@nxp.com>; linux-arm- > kernel@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. Regards, Leo
On Thu, Nov 5, 2020 at 5:04 PM Leo Li <leoyang.li@nxp.com> wrote: > > > > > -----Original Message----- > > From: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > Sent: Tuesday, November 3, 2020 2:03 AM > > To: Leo Li <leoyang.li@nxp.com>; Biwen Li (OSS) <biwen.li@oss.nxp.com>; > > shawnguo@kernel.org; robh+dt@kernel.org; mark.rutland@arm.com; Z.q. > > Hou <zhiqiang.hou@nxp.com>; tglx@linutronix.de; jason@lakedaemon.net; > > maz@kernel.org > > Cc: devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Jiafei Pan > > <jiafei.pan@nxp.com>; Xiaobo Xie <xiaobo.xie@nxp.com>; linux-arm- > > kernel@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
> > > >>> 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. Hi Leo, sure, np. > > Regards, > Leo
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 <linux@rasmusvillemoes.dk> + * Copyright 2020 NXP + */ #define pr_fmt(fmt) "irq-ls-extirq: " fmt #include <linux/irq.h> @@ -183,6 +186,9 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent) priv->bit_reverse = (revcr != 0); } + if (of_device_is_compatible(node, "fsl,ls1043a-extirq")) + priv->bit_reverse = true; + domain = irq_domain_add_hierarchy(parent_domain, 0, priv->nirq, node, &extirq_domain_ops, priv); if (!domain) @@ -195,3 +201,5 @@ ls_extirq_of_init(struct device_node *node, struct device_node *parent) } IRQCHIP_DECLARE(ls1021a_extirq, "fsl,ls1021a-extirq", ls_extirq_of_init); +IRQCHIP_DECLARE(ls1043a_extirq, "fsl,ls1043a-extirq", ls_extirq_of_init); +IRQCHIP_DECLARE(ls1088a_extirq, "fsl,ls1088a-extirq", ls_extirq_of_init);