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@xxxxxxxxxxx> >>>>> >>>>> 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@xxxxxxxxxxx >>>>> --- >>>>> .../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
