| Message ID | 20220916120749.2517727-3-benedikt.niedermayr@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpmc wait-pin additions | expand |
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? > + } > + > 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. > + pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n", dev_err, not pr_err > + __func__, p->wait_pin, p->wait_pin_polarity); Skip __func__ > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; > + } > + } else { > + pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__); Ditto. > + } > + > 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. > +#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... Best regards, Krzysztof
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? > > > + } > > + > > 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
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
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. > > 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
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. > so how this helps in case no configuration from DT is done? > > Best regards, > Krzysztof Cheers, benedikt
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
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@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? > 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
On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote: >> 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". Exactly this will happen. As expected. This value cannot appear in DTS, therefore I would expect error message. Now you allow such value in DTS which is not the same as your bindings. > 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
Hi Krzysztof, On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote: > On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote: > > > 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". > > Exactly this will happen. As expected. This value cannot appear in DTS, > therefore I would expect error message. > > Now you allow such value in DTS which is not the same as your bindings. > And now I completely got it... With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT... Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional property anymore. If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed. Is this an acceptable solution for you? > > > 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
Hi, On 20/09/2022 15:17, Niedermayr, BENEDIKT wrote: > Hi Krzysztof, > > On Tue, 2022-09-20 at 13:23 +0200, Krzysztof Kozlowski wrote: >> On 20/09/2022 12:12, Niedermayr, BENEDIKT wrote: >>>> 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". >> >> Exactly this will happen. As expected. This value cannot appear in DTS, >> therefore I would expect error message. >> >> Now you allow such value in DTS which is not the same as your bindings. >> > And now I completely got it... > With this implementation it's even possible to set WAITPINPOLARITY_DEFAULT in the DT... > > Ok, changing this will lead to an error message if the "gpmc,wait-pin-polarity" is not set in DT. Means the DT property is more orless not an optional > property anymore. > If one defines the wait-pin without defining the polarity the driver probes successfully but and error message is printed. > Is this an acceptable solution for you? > No this is not acceptable. As current implementations don't define polarity and rely on reset defaults. You can check return value of "of_property_read_u32(np, "gpmc,wait-pin-polarity", &p->wait_pin_polarity))" " * Return: 0 on success, -EINVAL if the property does not exist, * -ENODATA if property does not have a value, and -EOVERFLOW if the * property data isn't large enough." If property is present you don't need to check for WAITPINPOLARITY_DEFAULT as that is not valid value for this property. If property is not present you force WAITPINPOLARITY_DEFAULT and don't print error message. > >> >>> 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 > cheers, -roger
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); + } + 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) { + pr_err("%s: Invalid wait-pin-polarity (pin: %d, pol: %d)\n", + __func__, p->wait_pin, p->wait_pin_polarity); + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; + } + } else { + pr_err("%s: Failed to read gpmc,wait-pin-polarity\n", __func__); + } + 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 +#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 */ }; /* Data for each chip select */