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

[Roger Quadros <rogerq@kernel.org>]

On 19/09/2022 16: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. 
> 
> I'm not sure how to proceed here? Does it make sense to add a check for different 
> waitpin polarities?
> 

Yes I think it is better to check and fail if different polarity is requested than the first time.
Else it will be a potential debugging nightmare. ;)

> 
>>
>>> +	}
>>> +
>>>  	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.
> 
>>
>>> +				pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n",
>>
>> dev_err, not pr_err
> 
> Ok. I didn't want to introduce dev_* functions. Currently pr_* functions where used all over the place.
> 
>>
>>> +				       __func__, p->wait_pin, p->wait_pin_polarity);
>>
>> Skip __func__
>>
> Ok.
> 
>>> +				p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT;
>>> +			}
>>> +		} else {
>>> +			pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__);
>>
>> Ditto.
> Ok.
> 
>>
>>> +		}
>>> +
>>>  		p->wait_on_read = of_property_read_bool(np,
>>>  							"gpmc,wait-on-read");
>>>  		p->wait_on_write = of_property_read_bool(np,
>>> diff --git a/include/linux/platform_data/gpmc-omap.h b/include/linux/platform_data/gpmc-omap.h
>>> index c9cc4e32435d..c46c28069c31 100644
>>> --- a/include/linux/platform_data/gpmc-omap.h
>>> +++ b/include/linux/platform_data/gpmc-omap.h
>>> @@ -136,6 +136,11 @@ struct gpmc_device_timings {
>>>  #define GPMC_MUX_AAD			1	/* Addr-Addr-Data multiplex */
>>>  #define GPMC_MUX_AD			2	/* Addr-Data multiplex */
>>>  
>>> +/* Wait pin polarity values */
>>> +#define WAITPINPOLARITY_DEFAULT -1
>>
>> Missing prefix. This is a global header.
> Ok. Makes sense.
>>
>>> +#define WAITPINPOLARITY_ACTIVE_LOW 0
>>> +#define WAITPINPOLARITY_ACTIVE_HIGH 1
>>> +
>>>  struct gpmc_settings {
>>>  	bool burst_wrap;	/* enables wrap bursting */
>>>  	bool burst_read;	/* enables read page/burst mode */
>>> @@ -149,6 +154,7 @@ struct gpmc_settings {
>>>  	u32 device_width;	/* device bus width (8 or 16 bit) */
>>>  	u32 mux_add_data;	/* multiplex address & data */
>>>  	u32 wait_pin;		/* wait-pin to be used */
>>> +	u32 wait_pin_polarity;	/* wait-pin polarity */
>>
>> Skip the comment. You just copied the name of variable. Such comments
>> are useless.
>>
>> We do not have KPIs in kernel to achieve some comment-ratio...
>>
> Ok, makes sense.
> 
>> Best regards,
>> Krzysztof
> 

cheers,
-roger