Re: [PATCH v2 3/3] dt-bindings: memory-controllers: gpmc-child: Add binding for wait-pin-polarity

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 05/09/2022 13:48, Niedermayr, BENEDIKT wrote:
> On Mon, 2022-09-05 at 11:54 +0200, Krzysztof Kozlowski wrote:
>> On 05/09/2022 11:21, Roger Quadros wrote:
>>>
>>> On 05/09/2022 12:14, Niedermayr, BENEDIKT wrote:
>>>> On Mon, 2022-09-05 at 11:56 +0300, Roger Quadros wrote:
>>>>> Hi Benedikt,
>>>>>
>>>>> On 05/09/2022 10:17, B. Niedermayr wrote:
>>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>>>
>>>>>> Add a new dt-binding for the wait-pin-polarity property
>>>>>>
>>>>>> Signed-off-by: Benedikt Niedermayr <
>>>>>> benedikt.niedermayr@siemens.com
>>>>>> ---
>>>>>>  .../bindings/memory-controllers/ti,gpmc-child.yaml         | 
>>>>>> 7
>>>>>> +++++++
>>>>>>  1 file changed, 7 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..7c721206f10b 100644
>>>>>> --- a/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-
>>>>>> child.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/memory-
>>>>>> controllers/ti,gpmc-
>>>>>> child.yaml
>>>>>> @@ -230,6 +230,13 @@ properties:
>>>>>>        Wait-pin used by client. Must be less than "gpmc,num-
>>>>>> waitpins".
>>>>>>      $ref: /schemas/types.yaml#/definitions/uint32
>>>>>>  
>>>>>> +  gpmc,wait-pin-polarity:
>>>>>> +    description: |
>>>>>> +      Wait-pin polarity used by the clien. It relates to the
>>>>>> pin
>>>>>> defined
>>>>>
>>>>> did you mean "client?"
>>>>> Can you please specify what value is for Active Low vs Active
>>>>> High?
>>>>
>>>> Yes, that makes sense. And yes I meant "client". My typo.....
>>>>>> +      with "gpmc,wait-pin".
>>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>>
>>>>> Why can't type be boolean?
>>>>
>>>> Of course we can use the boolean there. In that case I should
>>>> give the
>>>> property a more meaningful name e.g. wait-pin-active-high or
>>>> wait-pin-
>>>> active-low. 
>>>> Since the default behavour of this pin is Active High,
>>>> a bool property "gpmc,wait-pin-active-low" would make more sense
>>>> for
>>>> backwards compatibility. 
>>>> If the property is missing, than the polarity stays on Active
>>>> High like
>>>> before.
>>>>
>>>
>>> OK, in that case you don't have to clarify the polarity in
>>> description.
>>
>> I don't understand (and it is not explained in commit msg), why do
>> you
>> need such property instead of using standard GPIO flags.
>>
>> The driver should use standard GPIO descriptor and standard bindings.
>> If
>> it cannot, this has to be explained.
>>
>> Best regards,
>> Krzysztof
> 
> I think this is beacause the GPMC controller itself is not respecting
> the GPIO flags. Instead the GPMC is reading the Line Level directly
> (high,low) and then evaluates the logic depending how
> the WAIT<x>PINPOLARITY bit is set in the GPMPC_CONFIG register.
> 
> Until now gpiochip_request_own_desc() was hardcorded
> to GPIO_ACTIVE_HIGH. An the GPMC_CONFIG register configuration has no
> relation to the GPIO setting (in the current implementation).
> My first approach was to make this part configurable via a new device
> tree property (wait-pin-polarity).
> 
> IMHO (correct me if I'm wrong) the current implementation also does not
> make ues of standart GPIO bindings and defines the wait pin via a
> separate "gpmc,waitpin" binding.
> 
> E.g. gpmc,watipin = <0> or gpmc,waitpin=<1>
> 
> The best solution would should be when setting the binding this way for
> example: gpmc,wait-pin = <&gpiox y ACTIVE_X>

Yes and I am afraid this will grow instead of adding proper GPIO usage.
Any reason why it cannot be a standard GPIO pin desc?

> 
> But I think the current omap-gpmc.c implementation does not offer such
> a usecase and as roger already mentioned: 
> "GPMC wait_pin polarity logic is hard-wired and doesn't depend on GPIO
> subsystem for its polarity"

This part I don't get. You mean hard-wired in the driver or hard-wired
in the hardware? If the first, please un-wire it. If the latter, your
property makes no sense, right?

Best regards,
Krzysztof