| Message ID | 20220915091333.2425306-3-benedikt.niedermayr@siemens.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | omap-gpmc wait pin additions | expand |
Hi, On 15/09/2022 12:13, 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 | 22 ++++++++++++++++++++++ > include/linux/platform_data/gpmc-omap.h | 6 ++++++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index e3674a15b934..66dd7dd80653 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) > @@ -1881,6 +1882,21 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p) > > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1); > > + if (p->wait_on_read || p->wait_on_write) { > + 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); > + else if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) > + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", > + __func__, p->wait_pin, p->wait_pin_polarity); We could avoid the print here. Instead the check for correct polarity value and printing an error message on invalid values needs to be done when we read the DT property in gpmc_read_settings_dt() > + > + gpmc_write_reg(GPMC_CONFIG, config1); How about avoiding the read/write altogether if p->wait_pin_polarity == WAITPINPOLARITY_DEFAULT? > + } > + > + > return 0; > } > > @@ -1981,6 +1997,12 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p) > } > > if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { > + New line not required. > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; This should be moved before this 'if' block. Else we will have cases where WAITPINPOLARITY_DEFAULT is not set when there was no "gpmc,wait-pin" property in the DT node. > + of_property_read_u32(np, > + "gpmc,wait-pin-polarity", > + &p->wait_pin_polarity); > + Please sanity check p->wait_pin_polarity here. If invalid value, print error and set to WAITPINPOLARITY_DEFAULT. > 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 */ cheers, -roger
On 15/09/2022 12:13, 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 | 22 ++++++++++++++++++++++ > include/linux/platform_data/gpmc-omap.h | 6 ++++++ > 2 files changed, 28 insertions(+) ./scripts/checkpatch.pl --strict ------------------------------------------------------------- 0002-memory-omap-gpmc-add-support-for-wait-pin-polarity.patch ------------------------------------------------------------- CHECK: Alignment should match open parenthesis #42: FILE: drivers/memory/omap-gpmc.c:1899: + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", + __func__, p->wait_pin, p->wait_pin_polarity); CHECK: Please don't use multiple blank lines #47: FILE: drivers/memory/omap-gpmc.c:1904: + + CHECK: Blank lines aren't necessary after an open brace '{' #55: FILE: drivers/memory/omap-gpmc.c:1995: if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { + CHECK: Alignment should match open parenthesis #58: FILE: drivers/memory/omap-gpmc.c:1998: + of_property_read_u32(np, + "gpmc,wait-pin-polarity", total: 0 errors, 0 warnings, 4 checks, 58 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index e3674a15b934..66dd7dd80653 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) > @@ -1881,6 +1882,21 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p) > > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1); > > + if (p->wait_on_read || p->wait_on_write) { > + 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); > + else if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) > + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", > + __func__, p->wait_pin, p->wait_pin_polarity); > + > + gpmc_write_reg(GPMC_CONFIG, config1); > + } > + > + > return 0; > } > > @@ -1981,6 +1997,12 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p) > } > > if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { > + > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; > + of_property_read_u32(np, > + "gpmc,wait-pin-polarity", > + &p->wait_pin_polarity); > + > 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 */ cheers, -roger
On Fri, 2022-09-16 at 11:55 +0300, Roger Quadros wrote: > > On 15/09/2022 12:13, 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 | 22 ++++++++++++++++++++++ > > include/linux/platform_data/gpmc-omap.h | 6 ++++++ > > 2 files changed, 28 insertions(+) > > ./scripts/checkpatch.pl --strict Sorry, I wasn't aware of that option. It's my first patchseries I try to get upstream. But thanks for the review and feedback! > > ------------------------------------------------------------- > 0002-memory-omap-gpmc-add-support-for-wait-pin-polarity.patch > ------------------------------------------------------------- > CHECK: Alignment should match open parenthesis > #42: FILE: drivers/memory/omap-gpmc.c:1899: > + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", > + __func__, p->wait_pin, p->wait_pin_polarity); > > CHECK: Please don't use multiple blank lines > #47: FILE: drivers/memory/omap-gpmc.c:1904: > + > + > > CHECK: Blank lines aren't necessary after an open brace '{' > #55: FILE: drivers/memory/omap-gpmc.c:1995: > if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { > + > > CHECK: Alignment should match open parenthesis > #58: FILE: drivers/memory/omap-gpmc.c:1998: > + of_property_read_u32(np, > + "gpmc,wait-pin-polarity", > > total: 0 errors, 0 warnings, 4 checks, 58 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix-inplace. > > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > > index e3674a15b934..66dd7dd80653 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) > > @@ -1881,6 +1882,21 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p) > > > > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1); > > > > + if (p->wait_on_read || p->wait_on_write) { > > + 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); > > + else if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) > > + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", > > + __func__, p->wait_pin, p->wait_pin_polarity); > > + > > + gpmc_write_reg(GPMC_CONFIG, config1); > > + } > > + > > + > > return 0; > > } > > > > @@ -1981,6 +1997,12 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p) > > } > > > > if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { > > + > > + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; > > + of_property_read_u32(np, > > + "gpmc,wait-pin-polarity", > > + &p->wait_pin_polarity); > > + > > 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 */ > > cheers, > -roger cheers, benedikt
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index e3674a15b934..66dd7dd80653 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) @@ -1881,6 +1882,21 @@ int gpmc_cs_program_settings(int cs, struct gpmc_settings *p) gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, config1); + if (p->wait_on_read || p->wait_on_write) { + 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); + else if (p->wait_pin_polarity != WAITPINPOLARITY_DEFAULT) + pr_err("%s: invalid wait-pin-polarity (pin: %d, polarity: %d)\n", + __func__, p->wait_pin, p->wait_pin_polarity); + + gpmc_write_reg(GPMC_CONFIG, config1); + } + + return 0; } @@ -1981,6 +1997,12 @@ void gpmc_read_settings_dt(struct device_node *np, struct gpmc_settings *p) } if (!of_property_read_u32(np, "gpmc,wait-pin", &p->wait_pin)) { + + p->wait_pin_polarity = WAITPINPOLARITY_DEFAULT; + of_property_read_u32(np, + "gpmc,wait-pin-polarity", + &p->wait_pin_polarity); + 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 */