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 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@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?
> 
> 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



[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux