Re: [PATCH v5 2/3] memory: omap-gpmc: add support for wait pin polarity

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

On 20/09/2022 11:13, Niedermayr, BENEDIKT wrote:
> Hi Krzysztof,
> 
> On Tue, 2022-09-20 at 09:39 +0200, Krzysztof Kozlowski wrote:
>> On 19/09/2022 15:25, Niedermayr, BENEDIKT wrote:
>>> Hi Krzysztof,
>>>
>>> On Mon, 2022-09-19 at 11:38 +0200, Krzysztof Kozlowski wrote:
>>>> On 16/09/2022 14:07, B. Niedermayr wrote:
>>>>> From: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>>
>>>>> The waitpin polarity can be configured via the WAITPIN<X>POLARITY bits
>>>>> in the GPMC_CONFIG register. This is currently not supported by the
>>>>> driver. This patch adds support for setting the required register bits
>>>>> with the "gpmc,wait-pin-polarity" dt-property.
>>>>>
>>>>> Signed-off-by: Benedikt Niedermayr <benedikt.niedermayr@siemens.com>
>>>>> ---
>>>>>  drivers/memory/omap-gpmc.c              | 27 +++++++++++++++++++++++++
>>>>>  include/linux/platform_data/gpmc-omap.h |  6 ++++++
>>>>>  2 files changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
>>>>> index ea495e93766b..2853fc28bccc 100644
>>>>> --- a/drivers/memory/omap-gpmc.c
>>>>> +++ b/drivers/memory/omap-gpmc.c
>>>>> @@ -132,6 +132,7 @@
>>>>>  #define GPMC_CONFIG_DEV_SIZE	0x00000002
>>>>>  #define GPMC_CONFIG_DEV_TYPE	0x00000003
>>>>>  
>>>>> +#define GPMC_CONFIG_WAITPINPOLARITY(pin)	(BIT(pin) << 8)
>>>>>  #define GPMC_CONFIG1_WRAPBURST_SUPP     (1 << 31)
>>>>>  #define GPMC_CONFIG1_READMULTIPLE_SUPP  (1 << 30)
>>>>>  #define GPMC_CONFIG1_READTYPE_ASYNC     (0 << 29)
>>>>> @@ -1882,6 +1883,17 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p)
>>>>>  
>>>>>  	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1);
>>>>>  
>>>>> +	if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>>>> +		config1 = gpmc_read_reg(GPMC_CONFIG);
>>>>> +
>>>>> +		if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_LOW)
>>>>> +			config1 &= ~GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>>>> +		else if (p->wait_pin_polarity == WAITPINPOLARITY_ACTIVE_HIGH)
>>>>> +			config1 |= GPMC_CONFIG_WAITPINPOLARITY(p->wait_pin);
>>>>> +
>>>>> +		gpmc_write_reg(GPMC_CONFIG, config1);
>>>>
>>>> What happens if wait pin is shared and you have different polarities in
>>>> both of devices?
>>> In this case the second one wins and will overwrite the polarity of the first one.
>>> But that would be the result of a misconfiguration in the DT.
>>
>> In many cases drivers do not accept blindly a DT, but perform some basic
>> sanity on it, especially if mistake is easy to make (e.g. with
>> overlays). Such design of DT is just fragile. Schema cannot validate it,
>> driver does not care, mistake is quite possible.
> 
> Ok, that makes sense. I'm going to implement this in v6.
>>
>>> I'm not sure how to proceed here? Does it make sense to add a check for different 
>>> waitpin polarities?
>>
>> I don't know. I would just disallow such sharing entirely or disallow
>> sharing if DT is misconfigured.
>>
>>
>>>
>>>>> +	}
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>  
>>>>> @@ -1981,7 +1993,22 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p)
>>>>>  				__func__);
>>>>>  	}
>>>>>  
>>>>> +	p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>>>> +
>>>>>  	if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) {
>>>>> +		if (!of_property_read_u32(np, "gpmc,wait-pin-polarity",
>>>>> +					  &p->wait_pin_polarity)) {
>>>>> +			if (p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH &&
>>>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW &&
>>>>> +			    p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) {
>>>>
>>>> WAITPINPOLARITY_DEFAULT is not allowed in DT, so you can skip it.
>>> This value is not assigned from the DT. It is only assigned within the GPMC and serves as a init
>>> value (right before the if clause). This helps in case no configuration from DT is done where the 
>>> GPMC registers should stay untouched.
>>
>> I don't see it. Your code is:
>>
>> p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>> # and DT has WAITPINPOLARITY_DEFAULT
>> if (....) {
>>   pr_err
>>   p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>> } else {
>>   pr_err
>> }
>>
> Maybe I dont't get what you mean with DT in this context.
> 
> What I meant is that the value WAITPINPOLARITY_DEFAULT is not directly extracted from the DT but is assigned in case
> "gpmc,wait-pin-polarity" is not set or has an invalid value. In any case the p->wait_pin_polarity should have
> at least the init value assigned so we can make proper decisions in gpmc_cs_program_settings().
> 
> Maybe I need some clarification what exatly is forbidden here.

I commented exactly below the line which I question. I don't question
other lines. So let me be a bit more specific:

Why do you need
"p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT"
? Can you write a scenario where this is useful?

Best regards,
Krzysztof