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
