| Message ID | 20220906124747.1767318-5-benedikt.niedermayr@siemens.com |
|---|---|
| State | New |
| Headers | show |
| Series | omap-gpmc wait pin additions | expand |
Benedikt, On 06/09/2022 15:47, B. Niedermayr wrote: > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > The GPMC controller has the ability to configure the polarity for the > wait pin. The current properties do not allow this configuration. > This binding directly configures the WAITPIN<X>POLARITY bit > in the GPMC_CONFIG register. > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > --- > .../bindings/memory-controllers/ti,gpmc-child.yaml | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > index 6e3995bb1630..a115b544a407 100644 > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml > @@ -230,6 +230,12 @@ properties: > Wait-pin used by client. Must be less than "gpmc,num-waitpins". > $ref: /schemas/types.yaml#/definitions/uint32 > > + gpmc,wait-pin-active-low: > + description: | > + Set the polarity for the selected wait pin to active low. > + Defaults to active high if this is not set. > + type: boolean > + I just checked that the default behaviour is active low. Reset value of the polarity register field is 0, which means active low. We will need to use the property "gpmc,wait-pin-active-high" instead. Sorry for not catching this earlier. > gpmc,wait-on-read: > description: Enables wait monitoring on reads. > type: boolean cheers, -roger
On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: > Benedikt, > > > On 06/09/2022 15:47, B. Niedermayr wrote: > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > > The GPMC controller has the ability to configure the polarity for > > the > > wait pin. The current properties do not allow this configuration. > > This binding directly configures the WAITPIN<X>POLARITY bit > > in the GPMC_CONFIG register. > > > > Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com > > > > > --- > > .../bindings/memory-controllers/ti,gpmc-child.yaml | 6 > > ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/memory- > > controllers/ti,gpmc-child.yaml > > b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > child.yaml > > index 6e3995bb1630..a115b544a407 100644 > > --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > child.yaml > > +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > child.yaml > > @@ -230,6 +230,12 @@ properties: > > Wait-pin used by client. Must be less than "gpmc,num- > > waitpins". > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > + gpmc,wait-pin-active-low: > > + description: | > > + Set the polarity for the selected wait pin to active low. > > + Defaults to active high if this is not set. > > + type: boolean > > + > > I just checked that the default behaviour is active low. > Reset value of the polarity register field is 0, which means active > low. > > We will need to use the property "gpmc,wait-pin-active-high" instead. > > Sorry for not catching this earlier. It's ok. No worries. Well, the Datasheets are telling me different reset values here. The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY as 0x0, whereas the am64x TRM (Rev. C) defines the reset value of WAIT1PIN POLARITY as 0x1. The am64x TRM also defines different reset values for WAIT0PINPOLARITY and WAIT1PINPOLARITY. The interesting thing is that I'm currently working on an am335x platform and I dumped the GPMC_CONFIG register and got 0x00000a00 (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM specifies. Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases accordingly. 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case "gpmc,wait-pin-active-low" is not set. So the reset value is always overwritten. Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-active-low " is also ok for me, but it feels more like a cosmetic thing at this point. > > > gpmc,wait-on-read: > > description: Enables wait monitoring on reads. > > type: boolean > > cheers, > -roger cheers, benedikt
Benedikt, On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote: > On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: >> Benedikt, >> >> >> On 06/09/2022 15:47, B. Niedermayr wrote: >>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >>> >>> The GPMC controller has the ability to configure the polarity for >>> the >>> wait pin. The current properties do not allow this configuration. >>> This binding directly configures the WAITPIN<X>POLARITY bit >>> in the GPMC_CONFIG register. >>> >>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com >>>> >>> --- >>> .../bindings/memory-controllers/ti,gpmc-child.yaml | 6 >>> ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/memory- >>> controllers/ti,gpmc-child.yaml >>> b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>> child.yaml >>> index 6e3995bb1630..a115b544a407 100644 >>> --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>> child.yaml >>> +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>> child.yaml >>> @@ -230,6 +230,12 @@ properties: >>> Wait-pin used by client. Must be less than "gpmc,num- >>> waitpins". >>> $ref: /schemas/types.yaml#/definitions/uint32 >>> >>> + gpmc,wait-pin-active-low: >>> + description: | >>> + Set the polarity for the selected wait pin to active low. >>> + Defaults to active high if this is not set. >>> + type: boolean >>> + >> >> I just checked that the default behaviour is active low. >> Reset value of the polarity register field is 0, which means active >> low. >> >> We will need to use the property "gpmc,wait-pin-active-high" instead. >> >> Sorry for not catching this earlier. > > It's ok. No worries. > > Well, the Datasheets are telling me different reset values here. > The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY as > 0x0, whereas the am64x TRM (Rev. C) defines the reset value of WAIT1PIN > POLARITY as 0x1. The am64x TRM also defines different reset values for > WAIT0PINPOLARITY and WAIT1PINPOLARITY. > > The interesting thing is that I'm currently working on an am335x > platform and I dumped the GPMC_CONFIG register and got 0x00000a00 > (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM specifies. I can confirm the same behaviour on am642 EVM as well. I get 0xa00 on reading GPMC_CONFIG. > > > Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases > accordingly. > 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case > "gpmc,wait-pin-active-low" is not set. So the reset value is always > overwritten. > > > Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin-active-low > " is also ok for me, but it feels more like a cosmetic thing at this > point. My main concern is for legacy platforms not specifying the property in DT. Earlier we were not touching the WAITPINPOLARITY config and now we are so we might break some legacy platforms that don't specify the polarity and we flip it here. Fortunately, there are only few boards using gpmc wait-pin and mostly wait-pin 0 for which there is no discrepancy as far as wait-pin reset value is concerned. logicpd-torpedo-baseboard.dtsi: gpmc,wait-pin = <0>; omap3-devkit8000-common.dtsi: gpmc,wait-pin = <0>; Binary file omap3-devkit8000.dtb matches Binary file omap3-devkit8000-lcd43.dtb matches Binary file omap3-devkit8000-lcd70.dtb matches omap3-lilly-a83x.dtsi: gpmc,wait-pin = <0>; Binary file omap3-lilly-dbb056.dtb matches Binary file omap3-zoom3.dtb matches Only 1 board is using wait-pin 1 omap-zoom-common.dtsi: gpmc,wait-pin = <1>; from OMP36xx TRM, here are the reset values WAIT3PINPOLARITY 0x1 WAIT2PINPOLARITY 0x0 WAIT1PINPOLARITY 0x1 WAIT0PINPOLARITY 0x0 cheers, -roger
Roger, On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote: > Benedikt, > > On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote: > > On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: > > > Benedikt, > > > > > > > > > On 06/09/2022 15:47, B. Niedermayr wrote: > > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > > > > > > The GPMC controller has the ability to configure the polarity > > > > for > > > > the > > > > wait pin. The current properties do not allow this > > > > configuration. > > > > This binding directly configures the WAITPIN<X>POLARITY bit > > > > in the GPMC_CONFIG register. > > > > > > > > Signed-off-by: Benedikt Niedermayr < > > > > benedikt.niedermayr@siemens.com > > > > --- > > > > .../bindings/memory-controllers/ti,gpmc-child.yaml | > > > > 6 > > > > ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/Documentation/devicetree/bindings/memory- > > > > controllers/ti,gpmc-child.yaml > > > > b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- > > > > child.yaml > > > > index 6e3995bb1630..a115b544a407 100644 > > > > --- a/Documentation/devicetree/bindings/memory- > > > > controllers/ti,gpmc- > > > > child.yaml > > > > +++ b/Documentation/devicetree/bindings/memory- > > > > controllers/ti,gpmc- > > > > child.yaml > > > > @@ -230,6 +230,12 @@ properties: > > > > Wait-pin used by client. Must be less than "gpmc,num- > > > > waitpins". > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > > + gpmc,wait-pin-active-low: > > > > + description: | > > > > + Set the polarity for the selected wait pin to active > > > > low. > > > > + Defaults to active high if this is not set. > > > > + type: boolean > > > > + > > > > > > I just checked that the default behaviour is active low. > > > Reset value of the polarity register field is 0, which means > > > active > > > low. > > > > > > We will need to use the property "gpmc,wait-pin-active-high" > > > instead. > > > > > > Sorry for not catching this earlier. > > > > It's ok. No worries. > > > > Well, the Datasheets are telling me different reset values here. > > The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY > > as > > 0x0, whereas the am64x TRM (Rev. C) defines the reset value of > > WAIT1PIN > > POLARITY as 0x1. The am64x TRM also defines different reset values > > for > > WAIT0PINPOLARITY and WAIT1PINPOLARITY. > > > > The interesting thing is that I'm currently working on an am335x > > platform and I dumped the GPMC_CONFIG register and got 0x00000a00 > > (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM > > specifies. > > I can confirm the same behaviour on am642 EVM as well. > I get 0xa00 on reading GPMC_CONFIG. > > > > > Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases > > accordingly. > > 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case > > "gpmc,wait-pin-active-low" is not set. So the reset value is always > > overwritten. > > > > > > Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin- > > active-low > > " is also ok for me, but it feels more like a cosmetic thing at > > this > > point. > > My main concern is for legacy platforms not specifying the property > in DT. > Earlier we were not touching the WAITPINPOLARITY config and now we > are > so we might break some legacy platforms that don't specify > the polarity and we flip it here. > > Fortunately, there are only few boards using gpmc wait-pin and mostly > wait-pin 0 > for which there is no discrepancy as far as wait-pin reset value is > concerned. > > logicpd-torpedo-baseboard.dtsi: gpmc,wait-pin = <0>; > omap3-devkit8000-common.dtsi: gpmc,wait-pin = <0>; > Binary file omap3-devkit8000.dtb matches > Binary file omap3-devkit8000-lcd43.dtb matches > Binary file omap3-devkit8000-lcd70.dtb matches > omap3-lilly-a83x.dtsi: gpmc,wait-pin = <0>; > Binary file omap3-lilly-dbb056.dtb matches > Binary file omap3-zoom3.dtb matches > > Only 1 board is using wait-pin 1 > omap-zoom-common.dtsi: gpmc,wait-pin = <1>; > > from OMP36xx TRM, here are the reset values > WAIT3PINPOLARITY 0x1 > WAIT2PINPOLARITY 0x0 > WAIT1PINPOLARITY 0x1 > WAIT0PINPOLARITY 0x0 Ah ok. The picture is getting clearer. Does it make sense then not to use a boolean property in that case? With a boolean property we are only able to change the polarity bits into one direction (0 -> 1 or 1 -> 0) but we have different reset values for each bit. This part of my patch may then break the mentioned legacy platforms because it even overwrites the register in case the property is not set: + if (p->wait_pin_active_low) + config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); + else + config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); + + gpmc_write_reg(GPMC_CONFIG, config1); So in order to preserve compatibility as well as the possibility to change the polarity bits into the desired value I would propose to use an uint32 value for the desired value and in case the dt-property is not set we should not touch the register at all. > > cheers, > -roger cheers, benedikt
Benedikt, On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote: > Roger, > > On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote: >> Benedikt, >> >> On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote: >>> On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: >>>> Benedikt, >>>> >>>> >>>> On 06/09/2022 15:47, B. Niedermayr wrote: >>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >>>>> >>>>> The GPMC controller has the ability to configure the polarity >>>>> for >>>>> the >>>>> wait pin. The current properties do not allow this >>>>> configuration. >>>>> This binding directly configures the WAITPIN<X>POLARITY bit >>>>> in the GPMC_CONFIG register. >>>>> >>>>> Signed-off-by: Benedikt Niedermayr < >>>>> benedikt.niedermayr@siemens.com >>>>> --- >>>>> .../bindings/memory-controllers/ti,gpmc-child.yaml | >>>>> 6 >>>>> ++++++ >>>>> 1 file changed, 6 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/memory- >>>>> controllers/ti,gpmc-child.yaml >>>>> b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc- >>>>> child.yaml >>>>> index 6e3995bb1630..a115b544a407 100644 >>>>> --- a/Documentation/devicetree/bindings/memory- >>>>> controllers/ti,gpmc- >>>>> child.yaml >>>>> +++ b/Documentation/devicetree/bindings/memory- >>>>> controllers/ti,gpmc- >>>>> child.yaml >>>>> @@ -230,6 +230,12 @@ properties: >>>>> Wait-pin used by client. Must be less than "gpmc,num- >>>>> waitpins". >>>>> $ref: /schemas/types.yaml#/definitions/uint32 >>>>> >>>>> + gpmc,wait-pin-active-low: >>>>> + description: | >>>>> + Set the polarity for the selected wait pin to active >>>>> low. >>>>> + Defaults to active high if this is not set. >>>>> + type: boolean >>>>> + >>>> >>>> I just checked that the default behaviour is active low. >>>> Reset value of the polarity register field is 0, which means >>>> active >>>> low. >>>> >>>> We will need to use the property "gpmc,wait-pin-active-high" >>>> instead. >>>> >>>> Sorry for not catching this earlier. >>> >>> It's ok. No worries. >>> >>> Well, the Datasheets are telling me different reset values here. >>> The am335x TRM (Rev. Q) defines the reset value of WAIT1PINPOLARITY >>> as >>> 0x0, whereas the am64x TRM (Rev. C) defines the reset value of >>> WAIT1PIN >>> POLARITY as 0x1. The am64x TRM also defines different reset values >>> for >>> WAIT0PINPOLARITY and WAIT1PINPOLARITY. >>> >>> The interesting thing is that I'm currently working on an am335x >>> platform and I dumped the GPMC_CONFIG register and got 0x00000a00 >>> (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM >>> specifies. >> >> I can confirm the same behaviour on am642 EVM as well. >> I get 0xa00 on reading GPMC_CONFIG. >> >>> >>> Nevertheless, I'm setting the WAITXPINPOLARITY bits in both cases >>> accordingly. >>> 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case >>> "gpmc,wait-pin-active-low" is not set. So the reset value is always >>> overwritten. >>> >>> >>> Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin- >>> active-low >>> " is also ok for me, but it feels more like a cosmetic thing at >>> this >>> point. >> >> My main concern is for legacy platforms not specifying the property >> in DT. >> Earlier we were not touching the WAITPINPOLARITY config and now we >> are >> so we might break some legacy platforms that don't specify >> the polarity and we flip it here. >> >> Fortunately, there are only few boards using gpmc wait-pin and mostly >> wait-pin 0 >> for which there is no discrepancy as far as wait-pin reset value is >> concerned. >> >> logicpd-torpedo-baseboard.dtsi: gpmc,wait-pin = <0>; >> omap3-devkit8000-common.dtsi: gpmc,wait-pin = <0>; >> Binary file omap3-devkit8000.dtb matches >> Binary file omap3-devkit8000-lcd43.dtb matches >> Binary file omap3-devkit8000-lcd70.dtb matches >> omap3-lilly-a83x.dtsi: gpmc,wait-pin = <0>; >> Binary file omap3-lilly-dbb056.dtb matches >> Binary file omap3-zoom3.dtb matches >> >> Only 1 board is using wait-pin 1 >> omap-zoom-common.dtsi: gpmc,wait-pin = <1>; >> >> from OMP36xx TRM, here are the reset values >> WAIT3PINPOLARITY 0x1 >> WAIT2PINPOLARITY 0x0 >> WAIT1PINPOLARITY 0x1 >> WAIT0PINPOLARITY 0x0 > > Ah ok. The picture is getting clearer. > > Does it make sense then not to use a boolean property in that case? > With a boolean property we are only able to change the polarity bits > into one direction (0 -> 1 or 1 -> 0) but we have different reset > values for each bit. > > This part of my patch may then break the mentioned legacy platforms > because it even overwrites the register in case the property is not > set: > > > + if (p->wait_pin_active_low) > + config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > + else > + config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > + > + gpmc_write_reg(GPMC_CONFIG, config1); > > > So in order to preserve compatibility as well as the possibility to > change the polarity bits into the desired value I would propose to use > an uint32 value for the desired value and in case the dt-property is > not set we should not touch the register at all. I'm sorry I didn't understand how uint32 value solves this issue. Could you please explain with a DT example? cheers, -roger
Roger, On Tue, 2022-09-13 at 16:18 +0300, Roger Quadros wrote: > Benedikt, > > On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote: > > Roger, > > > > On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote: > > > Benedikt, > > > > > > On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote: > > > > On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: > > > > > Benedikt, > > > > > > > > > > > > > > > On 06/09/2022 15:47, B. Niedermayr wrote: > > > > > > From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> > > > > > > > > > > > > The GPMC controller has the ability to configure the > > > > > > polarity > > > > > > for > > > > > > the > > > > > > wait pin. The current properties do not allow this > > > > > > configuration. > > > > > > This binding directly configures the WAITPIN<X>POLARITY bit > > > > > > in the GPMC_CONFIG register. > > > > > > > > > > > > Signed-off-by: Benedikt Niedermayr < > > > > > > benedikt.niedermayr@siemens.com > > > > > > --- > > > > > > .../bindings/memory-controllers/ti,gpmc- > > > > > > child.yaml | > > > > > > 6 > > > > > > ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/Documentation/devicetree/bindings/memory- > > > > > > controllers/ti,gpmc-child.yaml > > > > > > b/Documentation/devicetree/bindings/memory- > > > > > > controllers/ti,gpmc- > > > > > > child.yaml > > > > > > index 6e3995bb1630..a115b544a407 100644 > > > > > > --- a/Documentation/devicetree/bindings/memory- > > > > > > controllers/ti,gpmc- > > > > > > child.yaml > > > > > > +++ b/Documentation/devicetree/bindings/memory- > > > > > > controllers/ti,gpmc- > > > > > > child.yaml > > > > > > @@ -230,6 +230,12 @@ properties: > > > > > > Wait-pin used by client. Must be less than > > > > > > "gpmc,num- > > > > > > waitpins". > > > > > > $ref: /schemas/types.yaml#/definitions/uint32 > > > > > > > > > > > > + gpmc,wait-pin-active-low: > > > > > > + description: | > > > > > > + Set the polarity for the selected wait pin to active > > > > > > low. > > > > > > + Defaults to active high if this is not set. > > > > > > + type: boolean > > > > > > + > > > > > > > > > > I just checked that the default behaviour is active low. > > > > > Reset value of the polarity register field is 0, which means > > > > > active > > > > > low. > > > > > > > > > > We will need to use the property "gpmc,wait-pin-active-high" > > > > > instead. > > > > > > > > > > Sorry for not catching this earlier. > > > > > > > > It's ok. No worries. > > > > > > > > Well, the Datasheets are telling me different reset values > > > > here. > > > > The am335x TRM (Rev. Q) defines the reset value of > > > > WAIT1PINPOLARITY > > > > as > > > > 0x0, whereas the am64x TRM (Rev. C) defines the reset value of > > > > WAIT1PIN > > > > POLARITY as 0x1. The am64x TRM also defines different reset > > > > values > > > > for > > > > WAIT0PINPOLARITY and WAIT1PINPOLARITY. > > > > > > > > The interesting thing is that I'm currently working on an > > > > am335x > > > > platform and I dumped the GPMC_CONFIG register and got > > > > 0x00000a00 > > > > (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM > > > > specifies. > > > > > > I can confirm the same behaviour on am642 EVM as well. > > > I get 0xa00 on reading GPMC_CONFIG. > > > > > > > Nevertheless, I'm setting the WAITXPINPOLARITY bits in both > > > > cases > > > > accordingly. > > > > 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case > > > > "gpmc,wait-pin-active-low" is not set. So the reset value is > > > > always > > > > overwritten. > > > > > > > > > > > > Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin- > > > > active-low > > > > " is also ok for me, but it feels more like a cosmetic thing at > > > > this > > > > point. > > > > > > My main concern is for legacy platforms not specifying the > > > property > > > in DT. > > > Earlier we were not touching the WAITPINPOLARITY config and now > > > we > > > are > > > so we might break some legacy platforms that don't specify > > > the polarity and we flip it here. > > > > > > Fortunately, there are only few boards using gpmc wait-pin and > > > mostly > > > wait-pin 0 > > > for which there is no discrepancy as far as wait-pin reset value > > > is > > > concerned. > > > > > > logicpd-torpedo-baseboard.dtsi: gpmc,wait-pin = <0>; > > > omap3-devkit8000-common.dtsi: gpmc,wait-pin = <0>; > > > Binary file omap3-devkit8000.dtb matches > > > Binary file omap3-devkit8000-lcd43.dtb matches > > > Binary file omap3-devkit8000-lcd70.dtb matches > > > omap3-lilly-a83x.dtsi: gpmc,wait-pin = <0>; > > > Binary file omap3-lilly-dbb056.dtb matches > > > Binary file omap3-zoom3.dtb matches > > > > > > Only 1 board is using wait-pin 1 > > > omap-zoom-common.dtsi: gpmc,wait-pin = <1>; > > > > > > from OMP36xx TRM, here are the reset values > > > WAIT3PINPOLARITY 0x1 > > > WAIT2PINPOLARITY 0x0 > > > WAIT1PINPOLARITY 0x1 > > > WAIT0PINPOLARITY 0x0 > > > > Ah ok. The picture is getting clearer. > > > > Does it make sense then not to use a boolean property in that case? > > With a boolean property we are only able to change the polarity > > bits > > into one direction (0 -> 1 or 1 -> 0) but we have different reset > > values for each bit. > > > > This part of my patch may then break the mentioned legacy platforms > > because it even overwrites the register in case the property is not > > set: > > > > > > + if (p->wait_pin_active_low) > > + config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > > + else > > + config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > > + > > + gpmc_write_reg(GPMC_CONFIG, config1); > > > > > > So in order to preserve compatibility as well as the possibility to > > change the polarity bits into the desired value I would propose to > > use > > an uint32 value for the desired value and in case the dt-property > > is > > not set we should not touch the register at all. > > I'm sorry I didn't understand how uint32 value solves this issue. > Could you please explain with a DT example? I meant a similar implementation like in my first patchseries. Just a example: dts: gpmc { foo0@0 { gpmc,wait-pin = <0>; gpmc,wait-pin-polarity = <0>; /* active low */ }; bar0@1 { gpmc,wait-pin = <1>; gpmc,wait-pin-polarity = <1>; /* active high */ }; foobar0@2 { gpmc,wait-pin = <2>; /* don't touch wait pin polarity here */ }; }; omap-gpmc: gpmc_read_settings_dt() { p->wait-pin_polarity = -1; /* some init value required here */ if (!of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait-pin_polarity) { .... } } gpmc_cs_program_settings() { if (p->wait_pin_polarity == 0) config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); if (p->wait_pin_polarity == 1) config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); } This should met all requirements. If "gpmc,wait-pin-polarity" is not set in the device tree, then the registers stay untouched. If it is set, then the WAIT<X>PINPOLARITY bit is set accordingly. On the OMP36xx platform for example we have want to set all wait pin polarities to active low (0) and have following register reset values: WAIT3PINPOLARITY 0x1 WAIT2PINPOLARITY 0x0 WAIT1PINPOLARITY 0x1 WAIT0PINPOLARITY 0x0 With an boolean "gpmc,wait-pin-active-high" property we're not able to set WAIT3PINPOLARITY and WAIT1PINPOLARITY to 0. And vice versa with WAIT2PINPOLARITY and WAIT0PINPOLARITY if we want to set them to active high (1) and only would have a "gpmc,wait-pin- active-low" property. I hope this clarifies my proposal. > cheers, -roger cheers, benedikt
Benedikt, On 13/09/2022 23:56, Niedermayr, BENEDIKT wrote: > Roger, > > On Tue, 2022-09-13 at 16:18 +0300, Roger Quadros wrote: >> Benedikt, >> >> On 13/09/2022 11:23, Niedermayr, BENEDIKT wrote: >>> Roger, >>> >>> On Mon, 2022-09-12 at 14:04 +0300, Roger Quadros wrote: >>>> Benedikt, >>>> >>>> On 12/09/2022 10:43, Niedermayr, BENEDIKT wrote: >>>>> On Thu, 2022-09-08 at 15:09 +0300, Roger Quadros wrote: >>>>>> Benedikt, >>>>>> >>>>>> >>>>>> On 06/09/2022 15:47, B. Niedermayr wrote: >>>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com> >>>>>>> >>>>>>> The GPMC controller has the ability to configure the >>>>>>> polarity >>>>>>> for >>>>>>> the >>>>>>> wait pin. The current properties do not allow this >>>>>>> configuration. >>>>>>> This binding directly configures the WAITPIN<X>POLARITY bit >>>>>>> in the GPMC_CONFIG register. >>>>>>> >>>>>>> Signed-off-by: Benedikt Niedermayr < >>>>>>> benedikt.niedermayr@siemens.com >>>>>>> --- >>>>>>> .../bindings/memory-controllers/ti,gpmc- >>>>>>> child.yaml | >>>>>>> 6 >>>>>>> ++++++ >>>>>>> 1 file changed, 6 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/memory- >>>>>>> controllers/ti,gpmc-child.yaml >>>>>>> b/Documentation/devicetree/bindings/memory- >>>>>>> controllers/ti,gpmc- >>>>>>> child.yaml >>>>>>> index 6e3995bb1630..a115b544a407 100644 >>>>>>> --- a/Documentation/devicetree/bindings/memory- >>>>>>> controllers/ti,gpmc- >>>>>>> child.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/memory- >>>>>>> controllers/ti,gpmc- >>>>>>> child.yaml >>>>>>> @@ -230,6 +230,12 @@ properties: >>>>>>> Wait-pin used by client. Must be less than >>>>>>> "gpmc,num- >>>>>>> waitpins". >>>>>>> $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>> >>>>>>> + gpmc,wait-pin-active-low: >>>>>>> + description: | >>>>>>> + Set the polarity for the selected wait pin to active >>>>>>> low. >>>>>>> + Defaults to active high if this is not set. >>>>>>> + type: boolean >>>>>>> + >>>>>> >>>>>> I just checked that the default behaviour is active low. >>>>>> Reset value of the polarity register field is 0, which means >>>>>> active >>>>>> low. >>>>>> >>>>>> We will need to use the property "gpmc,wait-pin-active-high" >>>>>> instead. >>>>>> >>>>>> Sorry for not catching this earlier. >>>>> >>>>> It's ok. No worries. >>>>> >>>>> Well, the Datasheets are telling me different reset values >>>>> here. >>>>> The am335x TRM (Rev. Q) defines the reset value of >>>>> WAIT1PINPOLARITY >>>>> as >>>>> 0x0, whereas the am64x TRM (Rev. C) defines the reset value of >>>>> WAIT1PIN >>>>> POLARITY as 0x1. The am64x TRM also defines different reset >>>>> values >>>>> for >>>>> WAIT0PINPOLARITY and WAIT1PINPOLARITY. >>>>> >>>>> The interesting thing is that I'm currently working on an >>>>> am335x >>>>> platform and I dumped the GPMC_CONFIG register and got >>>>> 0x00000a00 >>>>> (WAIT1PINPOLARITY == 0x1). So It doesn't behave like the TRM >>>>> specifies. >>>> >>>> I can confirm the same behaviour on am642 EVM as well. >>>> I get 0xa00 on reading GPMC_CONFIG. >>>> >>>>> Nevertheless, I'm setting the WAITXPINPOLARITY bits in both >>>>> cases >>>>> accordingly. >>>>> 0x0 in case "gpmc,wait-pin-active-low" is set and 0x1 in case >>>>> "gpmc,wait-pin-active-low" is not set. So the reset value is >>>>> always >>>>> overwritten. >>>>> >>>>> >>>>> Using "gpmc,wait-pin-active-high" rather than "gpmc,wait-pin- >>>>> active-low >>>>> " is also ok for me, but it feels more like a cosmetic thing at >>>>> this >>>>> point. >>>> >>>> My main concern is for legacy platforms not specifying the >>>> property >>>> in DT. >>>> Earlier we were not touching the WAITPINPOLARITY config and now >>>> we >>>> are >>>> so we might break some legacy platforms that don't specify >>>> the polarity and we flip it here. >>>> >>>> Fortunately, there are only few boards using gpmc wait-pin and >>>> mostly >>>> wait-pin 0 >>>> for which there is no discrepancy as far as wait-pin reset value >>>> is >>>> concerned. >>>> >>>> logicpd-torpedo-baseboard.dtsi: gpmc,wait-pin = <0>; >>>> omap3-devkit8000-common.dtsi: gpmc,wait-pin = <0>; >>>> Binary file omap3-devkit8000.dtb matches >>>> Binary file omap3-devkit8000-lcd43.dtb matches >>>> Binary file omap3-devkit8000-lcd70.dtb matches >>>> omap3-lilly-a83x.dtsi: gpmc,wait-pin = <0>; >>>> Binary file omap3-lilly-dbb056.dtb matches >>>> Binary file omap3-zoom3.dtb matches >>>> >>>> Only 1 board is using wait-pin 1 >>>> omap-zoom-common.dtsi: gpmc,wait-pin = <1>; >>>> >>>> from OMP36xx TRM, here are the reset values >>>> WAIT3PINPOLARITY 0x1 >>>> WAIT2PINPOLARITY 0x0 >>>> WAIT1PINPOLARITY 0x1 >>>> WAIT0PINPOLARITY 0x0 >>> >>> Ah ok. The picture is getting clearer. >>> >>> Does it make sense then not to use a boolean property in that case? >>> With a boolean property we are only able to change the polarity >>> bits >>> into one direction (0 -> 1 or 1 -> 0) but we have different reset >>> values for each bit. >>> >>> This part of my patch may then break the mentioned legacy platforms >>> because it even overwrites the register in case the property is not >>> set: >>> >>> >>> + if (p->wait_pin_active_low) >>> + config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); >>> + else >>> + config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); >>> + >>> + gpmc_write_reg(GPMC_CONFIG, config1); >>> >>> >>> So in order to preserve compatibility as well as the possibility to >>> change the polarity bits into the desired value I would propose to >>> use >>> an uint32 value for the desired value and in case the dt-property >>> is >>> not set we should not touch the register at all. >> >> I'm sorry I didn't understand how uint32 value solves this issue. >> Could you please explain with a DT example? > > I meant a similar implementation like in my first patchseries. > > Just a example: > > dts: > > gpmc { > > foo0@0 { > gpmc,wait-pin = <0>; > gpmc,wait-pin-polarity = <0>; /* active low */ > }; > > bar0@1 { > gpmc,wait-pin = <1>; > gpmc,wait-pin-polarity = <1>; /* active high */ > }; > > foobar0@2 { > gpmc,wait-pin = <2>; > /* don't touch wait pin polarity here */ > }; > }; > > omap-gpmc: > > gpmc_read_settings_dt() > { > p->wait-pin_polarity = -1; /* some init value required here */ > if (!of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait-pin_polarity) { > > .... > } > } > > gpmc_cs_program_settings() > { > if (p->wait_pin_polarity == 0) > config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > if (p->wait_pin_polarity == 1) > config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin); > } > > This should met all requirements. > > If "gpmc,wait-pin-polarity" is not set in the device tree, then the > registers stay untouched. > > If it is set, then the WAIT<X>PINPOLARITY bit is set accordingly. > > > On the OMP36xx platform for example we have want to set all wait pin > polarities to active low (0) and have following register reset values: > > WAIT3PINPOLARITY 0x1 > WAIT2PINPOLARITY 0x0 > WAIT1PINPOLARITY 0x1 > WAIT0PINPOLARITY 0x0 > > With an boolean "gpmc,wait-pin-active-high" property we're not able to > set WAIT3PINPOLARITY and WAIT1PINPOLARITY to 0. > And vice versa with WAIT2PINPOLARITY and WAIT0PINPOLARITY if we want > to > set them to active high (1) and only would have a "gpmc,wait-pin- > active-low" property. > > I hope this clarifies my proposal. > Yes now it is clear. I think it is a good proposal and solves all our current issues. cheers, -roger
diff --git a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml index 6e3995bb1630..a115b544a407 100644 --- a/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml +++ b/Documentation/devicetree/bindings/memory-controllers/ti,gpmc-child.yaml @@ -230,6 +230,12 @@ properties: Wait-pin used by client. Must be less than "gpmc,num-waitpins". $ref: /schemas/types.yaml#/definitions/uint32 + gpmc,wait-pin-active-low: + description: | + Set the polarity for the selected wait pin to active low. + Defaults to active high if this is not set. + type: boolean + gpmc,wait-on-read: description: Enables wait monitoring on reads. type: boolean