Re: [PATCH v3 3/3] dt-bindings: memory-controllers: gpmc-child: add wait-pin polarity

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux