2020-10-27 14:14:00

by Rasmus Villemoes

[permalink] [raw]
Subject: Re: [v2 01/11] irqchip: ls-extirq: Add LS1043A, LS1088A external interrupt

On 27/10/2020 05.46, Biwen Li wrote:
> From: Hou Zhiqiang <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Biwen Li <[email protected]>
> ---
> 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 <[email protected]>

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


2020-10-28 01:42:27

by Biwen Li

[permalink] [raw]
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 <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Biwen Li <[email protected]>
> > ---
> > 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 <[email protected]>
>
> 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

2020-10-28 21:40:09

by Leo Li

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



> -----Original Message-----
> From: Biwen Li <[email protected]>
> Sent: Tuesday, October 27, 2020 2:48 AM
> To: Rasmus Villemoes <[email protected]>; Biwen Li (OSS)
> <[email protected]>; [email protected]; [email protected];
> [email protected]; Leo Li <[email protected]>; Z.q. Hou
> <[email protected]>; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Jiafei Pan
> <[email protected]>; Xiaobo Xie <[email protected]>; linux-arm-
> [email protected]
> 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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Signed-off-by: Biwen Li <[email protected]>
> > > ---
> > > 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 <[email protected]>
> >
> > 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

2020-11-02 06:19:38

by Biwen Li (OSS)

[permalink] [raw]
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 <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > Signed-off-by: Biwen Li <[email protected]>
> > > > ---
> > > > 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 <[email protected]>
> > >
> > > 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

2020-11-02 21:24:30

by Leo Li

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



> -----Original Message-----
> From: Biwen Li (OSS) <[email protected]>
> Sent: Monday, November 2, 2020 12:15 AM
> To: Leo Li <[email protected]>; Rasmus Villemoes
> <[email protected]>; Biwen Li (OSS) <[email protected]>;
> [email protected]; [email protected]; [email protected]; Z.q.
> Hou <[email protected]>; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Jiafei Pan
> <[email protected]>; Xiaobo Xie <[email protected]>; linux-arm-
> [email protected]
> 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 <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > Signed-off-by: Biwen Li <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
> > > >
> > > > 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

2020-11-03 08:04:24

by Rasmus Villemoes

[permalink] [raw]
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".

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

2020-11-05 23:05:43

by Leo Li

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



> -----Original Message-----
> From: Rasmus Villemoes <[email protected]>
> Sent: Tuesday, November 3, 2020 2:03 AM
> To: Leo Li <[email protected]>; Biwen Li (OSS) <[email protected]>;
> [email protected]; [email protected]; [email protected]; Z.q.
> Hou <[email protected]>; [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; Jiafei Pan
> <[email protected]>; Xiaobo Xie <[email protected]>; linux-arm-
> [email protected]
> 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

2020-11-24 01:36:56

by Leo Li

[permalink] [raw]
Subject: 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 <[email protected]> wrote:
>
>
>
> > -----Original Message-----
> > From: Rasmus Villemoes <[email protected]>
> > Sent: Tuesday, November 3, 2020 2:03 AM
> > To: Leo Li <[email protected]>; Biwen Li (OSS) <[email protected]>;
> > [email protected]; [email protected]; [email protected]; Z.q.
> > Hou <[email protected]>; [email protected]; [email protected];
> > [email protected]
> > Cc: [email protected]; [email protected]; Jiafei Pan
> > <[email protected]>; Xiaobo Xie <[email protected]>; linux-arm-
> > [email protected]
> > 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

2020-11-30 01:41:00

by Biwen Li (OSS)

[permalink] [raw]
Subject: 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 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