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@xxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxx>
>>> ---
>>> 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.
>
> 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
}
so how this helps in case no configuration from DT is done?
Best regards,
Krzysztof
