Hi Krzysztof,
On Tue, 2022-09-20 at 11:47 +0200, Krzysztof Kozlowski wrote:
> 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@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.
> >
> > 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?
>
Ok. I think I got you now. Sorry I'm relatively new to OSS contributions, so please be patient with me...
If I remove that part of the if clause, then an error message would be printed in case "p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT".
But this is a not an error case. WAITPINPOLARITY_DEFAULT is a valid value, is assigned right before the if clause as an init value(not extracted from DT),
and leads to not touching the GPMC_CONFIG register in gpmc_cs_program_settings().
So in gpmc_cs_program_settings() if:
p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_HIGH -> Issue a write to the GPMC_CONFIG register
p->wait_pin_polarity != WAITPINPOLARITY_ACTIVE_LOW -> Issua a write to the GPMC_CONFIG register
p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT -> Do not touch the GPMC_CONFIG register
We want to preserve the reset value of the GPMC_CONFIG register in case the DT does not use the "gpmc,wait-pin-polarity" property. Otherwise
we might break platforms which rely on these reset values.
> Best regards,
> Krzysztof
Cheers,
benedikt
