| Message ID | 1306247094-25372-3-git-send-email-tarun.kanti@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: > From: Charulatha V <charu@ti.com> > > Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But > the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx > (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. Can you cite the documentation you're using for the OMAP3 non-wakeup GPIOs? This is a change of functionality from current code, where all OMAP3 GPIOs are considered wakeup capable. I'd like this to be tackled in two patches. One for the cleanup/consolidation, and one for change in behavior. For this cleanup/consolidation (this series), please keep existing functionality and focus on the cleanup. Then, in an additional patch (on top of the cleanup/conslidation), change the functionality with a detailed changelog. Thanks, Kevin > Fix the above by providing non-wakeup GPIO information through pdata > specific to the SoC. > > The GPIO rev id provided in the hwmod database is the same for OMAP2420 > and OMAP2430. Change the GPIO rev ids in hwmod database as given below > so that it can be used to identify OMAP2420 and OMAP2430. > OMAP2420 - 0 > OMAP2430 - 1 > OMAP3 - 2 > OMAP4 - 3 > > Signed-off-by: Charulatha V <charu@ti.com> > Cc: Cousson, Benoit <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > --- > arch/arm/mach-omap2/gpio.c | 26 ++++++++++++++++++++++++-- > arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- > arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- > arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- > arch/arm/plat-omap/include/plat/gpio.h | 1 + > drivers/gpio/gpio_omap.c | 11 +++-------- > 6 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c > index 0446bd1..6cd26b4 100644 > --- a/arch/arm/mach-omap2/gpio.c > +++ b/arch/arm/mach-omap2/gpio.c > @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) > return -ENOMEM; > } > > + switch (oh->class->rev) { > + case 0: > + if (id == 1) > + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ > + pdata->non_wakeup_gpios = 0xe203ffc0; > + else if (id == 2) > + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ > + pdata->non_wakeup_gpios = 0x08700040; > + break; > + case 2: > + if (id == 2) > + /* non-wakeup GPIO pins for OMAP3 Bank2 */ > + pdata->non_wakeup_gpios = 0x00000001; > + else if (id == 6) > + /* non-wakeup GPIO pins for OMAP3 Bank6 */ > + pdata->non_wakeup_gpios = 0x08000000; > + break; > + default: > + /* No non-wakeup GPIO pins for other SoCs */ > + break; > + } > + > dev_attr = (struct omap_gpio_dev_attr *)oh->dev_attr; > pdata->bank_width = dev_attr->bank_width; > pdata->dbck_flag = dev_attr->dbck_flag; > @@ -70,6 +92,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) > switch (oh->class->rev) { > case 0: > case 1: > + case 2: > pdata->bank_type = METHOD_GPIO_24XX; > pdata->regs->revision = OMAP24XX_GPIO_REVISION; > pdata->regs->direction = OMAP24XX_GPIO_OE; > @@ -86,7 +109,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) > pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN; > pdata->regs->ctrl = OMAP24XX_GPIO_CTRL; > break; > - case 2: > + case 3: > pdata->bank_type = METHOD_GPIO_44XX; > pdata->regs->revision = OMAP4_GPIO_REVISION; > pdata->regs->direction = OMAP4_GPIO_OE; > @@ -108,7 +131,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) > kfree(pdata); > return -EINVAL; > } > - > od = omap_device_build(name, id - 1, oh, pdata, > sizeof(*pdata), omap_gpio_latency, > ARRAY_SIZE(omap_gpio_latency), > diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c > index 9682dd5..ae702b5 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c > @@ -1728,7 +1728,7 @@ static struct omap_hwmod_class_sysconfig omap243x_gpio_sysc = { > static struct omap_hwmod_class omap243x_gpio_hwmod_class = { > .name = "gpio", > .sysc = &omap243x_gpio_sysc, > - .rev = 0, > + .rev = 1, > }; > > /* gpio1 */ > diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c > index 909a84d..05e7005 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c > @@ -2117,7 +2117,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_gpio_sysc = { > static struct omap_hwmod_class omap3xxx_gpio_hwmod_class = { > .name = "gpio", > .sysc = &omap3xxx_gpio_sysc, > - .rev = 1, > + .rev = 2, > }; > > /* gpio_dev_attr*/ > diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c > index abc548a..ea30752 100644 > --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c > +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c > @@ -1703,7 +1703,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = { > static struct omap_hwmod_class omap44xx_gpio_hwmod_class = { > .name = "gpio", > .sysc = &omap44xx_gpio_sysc, > - .rev = 2, > + .rev = 3, > }; > > /* gpio dev_attr */ > diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h > index caf432c..8014a8a 100644 > --- a/arch/arm/plat-omap/include/plat/gpio.h > +++ b/arch/arm/plat-omap/include/plat/gpio.h > @@ -199,6 +199,7 @@ struct omap_gpio_platform_data { > int bank_width; /* GPIO bank width */ > int bank_stride; /* Only needed for omap1 MPUIO */ > bool dbck_flag; /* dbck required or not - True for OMAP3&4 */ > + u32 non_wakeup_gpios; > > struct omap_gpio_reg_offs *regs; > }; > diff --git a/drivers/gpio/gpio_omap.c b/drivers/gpio/gpio_omap.c > index dfdc45e..0ba4cdb 100644 > --- a/drivers/gpio/gpio_omap.c > +++ b/drivers/gpio/gpio_omap.c > @@ -1008,7 +1008,7 @@ static inline int init_gpio_info(struct platform_device *pdev) > } > > /* TODO: Cleanup cpu_is_* checks */ > -static void omap_gpio_mod_init(struct gpio_bank *bank, int id) > +static void omap_gpio_mod_init(struct gpio_bank *bank) > { > if (cpu_class_is_omap2()) { > if (cpu_is_omap44xx()) { > @@ -1028,12 +1028,6 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, int id) > > /* Initialize interface clk ungated, module enabled */ > __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL); > - } else if (cpu_is_omap24xx()) { > - static const u32 non_wakeup_gpios[] = { > - 0xe203ffc0, 0x08700040 > - }; > - if (id < ARRAY_SIZE(non_wakeup_gpios)) > - bank->non_wakeup_gpios = non_wakeup_gpios[id]; > } > } else if (cpu_class_is_omap1()) { > if (bank_is_mpuio(bank)) > @@ -1179,6 +1173,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > bank->dbck_flag = pdata->dbck_flag; > bank->stride = pdata->bank_stride; > bank->width = pdata->bank_width; > + bank->non_wakeup_gpios = pdata->non_wakeup_gpios; > > bank->regs = pdata->regs; > > @@ -1205,7 +1200,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) > pm_runtime_enable(bank->dev); > pm_runtime_get_sync(bank->dev); > > - omap_gpio_mod_init(bank, id); > + omap_gpio_mod_init(bank); > omap_gpio_chip_init(bank); > omap_gpio_show_rev(bank);
> -----Original Message----- > From: linux-omap-owner@vger.kernel.org > [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of > DebBarma, Tarun Kanti > Sent: Tuesday, May 24, 2011 7:55 PM > To: linux-omap@vger.kernel.org > Cc: Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > linux-arm-kernel@lists.infradead.org; Varadarajan, > Charulatha; Cousson, Benoit; Paul Walmsley > Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO > and rev_ids > > From: Charulatha V <charu@ti.com> > > Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But > the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx > (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. > > Fix the above by providing non-wakeup GPIO information through pdata > specific to the SoC. > > The GPIO rev id provided in the hwmod database is the same > for OMAP2420 > and OMAP2430. Change the GPIO rev ids in hwmod database as given below > so that it can be used to identify OMAP2420 and OMAP2430. > OMAP2420 - 0 > OMAP2430 - 1 > OMAP3 - 2 > OMAP4 - 3 [sp] Magic numbers should be avoided. Suggest using something like: #define GPIO_REV_2420 0 #define GPIO_REV_2430 1 #define GPIO_REV_34XX 2 #define GPIO_REV_44xx 3 We don't have to refer back to this comment while reading the code. I also believed that HWMODs were auto generated. Can the changes to structures in this patch recreated using current scripts? ~sanjeev > > Signed-off-by: Charulatha V <charu@ti.com> > Cc: Cousson, Benoit <b-cousson@ti.com> > Cc: Paul Walmsley <paul@pwsan.com> > --- > arch/arm/mach-omap2/gpio.c | 26 > ++++++++++++++++++++++++-- > arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- > arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- > arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- > arch/arm/plat-omap/include/plat/gpio.h | 1 + > drivers/gpio/gpio_omap.c | 11 +++-------- > 6 files changed, 31 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c > index 0446bd1..6cd26b4 100644 > --- a/arch/arm/mach-omap2/gpio.c > +++ b/arch/arm/mach-omap2/gpio.c > @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct > omap_hwmod *oh, void *unused) > return -ENOMEM; > } > > + switch (oh->class->rev) { > + case 0: > + if (id == 1) > + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ > + pdata->non_wakeup_gpios = 0xe203ffc0; > + else if (id == 2) > + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ > + pdata->non_wakeup_gpios = 0x08700040; > + break; > + case 2: > + if (id == 2) > + /* non-wakeup GPIO pins for OMAP3 Bank2 */ > + pdata->non_wakeup_gpios = 0x00000001; > + else if (id == 6) > + /* non-wakeup GPIO pins for OMAP3 Bank6 */ > + pdata->non_wakeup_gpios = 0x08000000; > + break; [sp] Where is the description on non-wakeup GPIOs in OMAP3? Here is text from AM37x TRM: [quote ...only relevant text] Each GPIO module provides 32 dedicated general-purpose pins with input and output capabilities; .... These pins can be configured for the following applications: - Data input (capture)/output (drive) - Keyboard interface with a debounce cell - Interrupt generation in .... - Wake-up request generation in idle mode [/quote] Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and GPIO6_WAKEUPENABLE (0x4905 8020) meant for? > + default: > + /* No non-wakeup GPIO pins for other SoCs */ > + break; > + } > + ~sanjeev [snip]...[snip]
On Thu, May 26, 2011 at 03:04, Kevin Hilman <khilman@ti.com> wrote: > Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: > >> From: Charulatha V <charu@ti.com> >> >> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >> (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. > > Can you cite the documentation you're using for the OMAP3 non-wakeup > GPIOs? I am referring to Table 25-5. GPIO Channel Description at page no 3354 OMAP3430 public TRM Version P. > > This is a change of functionality from current code, where all OMAP3 > GPIOs are considered wakeup capable. > > I'd like this to be tackled in two patches. One for the > cleanup/consolidation, and one for change in behavior. Okay. > > For this cleanup/consolidation (this series), please keep existing > functionality and focus on the cleanup. Then, in an additional patch > (on top of the cleanup/conslidation), change the functionality with a > detailed changelog. Okay. > > Thanks, > > Kevin > >> Fix the above by providing non-wakeup GPIO information through pdata >> specific to the SoC. >> >> The GPIO rev id provided in the hwmod database is the same for OMAP2420 >> and OMAP2430. Change the GPIO rev ids in hwmod database as given below >> so that it can be used to identify OMAP2420 and OMAP2430. >> OMAP2420 - 0 >> OMAP2430 - 1 >> OMAP3 - 2 >> OMAP4 - 3 >> >> Signed-off-by: Charulatha V <charu@ti.com> >> Cc: Cousson, Benoit <b-cousson@ti.com> >> Cc: Paul Walmsley <paul@pwsan.com> >> --- >> arch/arm/mach-omap2/gpio.c | 26 ++++++++++++++++++++++++-- >> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- >> arch/arm/plat-omap/include/plat/gpio.h | 1 + >> drivers/gpio/gpio_omap.c | 11 +++-------- >> 6 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c >> index 0446bd1..6cd26b4 100644 >> --- a/arch/arm/mach-omap2/gpio.c >> +++ b/arch/arm/mach-omap2/gpio.c >> @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) >> return -ENOMEM; >> } >> >> + switch (oh->class->rev) { >> + case 0: >> + if (id == 1) >> + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ >> + pdata->non_wakeup_gpios = 0xe203ffc0; >> + else if (id == 2) >> + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ >> + pdata->non_wakeup_gpios = 0x08700040; >> + break; >> + case 2: >> + if (id == 2) >> + /* non-wakeup GPIO pins for OMAP3 Bank2 */ >> + pdata->non_wakeup_gpios = 0x00000001; >> + else if (id == 6) >> + /* non-wakeup GPIO pins for OMAP3 Bank6 */ >> + pdata->non_wakeup_gpios = 0x08000000; >> + break; >> + default: >> + /* No non-wakeup GPIO pins for other SoCs */ >> + break; >> + } >> + >> dev_attr = (struct omap_gpio_dev_attr *)oh->dev_attr; >> pdata->bank_width = dev_attr->bank_width; >> pdata->dbck_flag = dev_attr->dbck_flag; >> @@ -70,6 +92,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) >> switch (oh->class->rev) { >> case 0: >> case 1: >> + case 2: >> pdata->bank_type = METHOD_GPIO_24XX; >> pdata->regs->revision = OMAP24XX_GPIO_REVISION; >> pdata->regs->direction = OMAP24XX_GPIO_OE; >> @@ -86,7 +109,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) >> pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN; >> pdata->regs->ctrl = OMAP24XX_GPIO_CTRL; >> break; >> - case 2: >> + case 3: >> pdata->bank_type = METHOD_GPIO_44XX; >> pdata->regs->revision = OMAP4_GPIO_REVISION; >> pdata->regs->direction = OMAP4_GPIO_OE; >> @@ -108,7 +131,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) >> kfree(pdata); >> return -EINVAL; >> } >> - >> od = omap_device_build(name, id - 1, oh, pdata, >> sizeof(*pdata), omap_gpio_latency, >> ARRAY_SIZE(omap_gpio_latency), >> diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c >> index 9682dd5..ae702b5 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c >> +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c >> @@ -1728,7 +1728,7 @@ static struct omap_hwmod_class_sysconfig omap243x_gpio_sysc = { >> static struct omap_hwmod_class omap243x_gpio_hwmod_class = { >> .name = "gpio", >> .sysc = &omap243x_gpio_sysc, >> - .rev = 0, >> + .rev = 1, >> }; >> >> /* gpio1 */ >> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c >> index 909a84d..05e7005 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c >> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c >> @@ -2117,7 +2117,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_gpio_sysc = { >> static struct omap_hwmod_class omap3xxx_gpio_hwmod_class = { >> .name = "gpio", >> .sysc = &omap3xxx_gpio_sysc, >> - .rev = 1, >> + .rev = 2, >> }; >> >> /* gpio_dev_attr*/ >> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c >> index abc548a..ea30752 100644 >> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c >> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c >> @@ -1703,7 +1703,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = { >> static struct omap_hwmod_class omap44xx_gpio_hwmod_class = { >> .name = "gpio", >> .sysc = &omap44xx_gpio_sysc, >> - .rev = 2, >> + .rev = 3, >> }; >> >> /* gpio dev_attr */ >> diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h >> index caf432c..8014a8a 100644 >> --- a/arch/arm/plat-omap/include/plat/gpio.h >> +++ b/arch/arm/plat-omap/include/plat/gpio.h >> @@ -199,6 +199,7 @@ struct omap_gpio_platform_data { >> int bank_width; /* GPIO bank width */ >> int bank_stride; /* Only needed for omap1 MPUIO */ >> bool dbck_flag; /* dbck required or not - True for OMAP3&4 */ >> + u32 non_wakeup_gpios; >> >> struct omap_gpio_reg_offs *regs; >> }; >> diff --git a/drivers/gpio/gpio_omap.c b/drivers/gpio/gpio_omap.c >> index dfdc45e..0ba4cdb 100644 >> --- a/drivers/gpio/gpio_omap.c >> +++ b/drivers/gpio/gpio_omap.c >> @@ -1008,7 +1008,7 @@ static inline int init_gpio_info(struct platform_device *pdev) >> } >> >> /* TODO: Cleanup cpu_is_* checks */ >> -static void omap_gpio_mod_init(struct gpio_bank *bank, int id) >> +static void omap_gpio_mod_init(struct gpio_bank *bank) >> { >> if (cpu_class_is_omap2()) { >> if (cpu_is_omap44xx()) { >> @@ -1028,12 +1028,6 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, int id) >> >> /* Initialize interface clk ungated, module enabled */ >> __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL); >> - } else if (cpu_is_omap24xx()) { >> - static const u32 non_wakeup_gpios[] = { >> - 0xe203ffc0, 0x08700040 >> - }; >> - if (id < ARRAY_SIZE(non_wakeup_gpios)) >> - bank->non_wakeup_gpios = non_wakeup_gpios[id]; >> } >> } else if (cpu_class_is_omap1()) { >> if (bank_is_mpuio(bank)) >> @@ -1179,6 +1173,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) >> bank->dbck_flag = pdata->dbck_flag; >> bank->stride = pdata->bank_stride; >> bank->width = pdata->bank_width; >> + bank->non_wakeup_gpios = pdata->non_wakeup_gpios; >> >> bank->regs = pdata->regs; >> >> @@ -1205,7 +1200,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) >> pm_runtime_enable(bank->dev); >> pm_runtime_get_sync(bank->dev); >> >> - omap_gpio_mod_init(bank, id); >> + omap_gpio_mod_init(bank); >> omap_gpio_chip_init(bank); >> omap_gpio_show_rev(bank); >
Sanjeev, Thanks for the comments. On Thu, May 26, 2011 at 14:53, Premi, Sanjeev <premi@ti.com> wrote: > > >> -----Original Message----- >> From: linux-omap-owner@vger.kernel.org >> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of >> DebBarma, Tarun Kanti >> Sent: Tuesday, May 24, 2011 7:55 PM >> To: linux-omap@vger.kernel.org >> Cc: Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; >> linux-arm-kernel@lists.infradead.org; Varadarajan, >> Charulatha; Cousson, Benoit; Paul Walmsley >> Subject: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup GPIO >> and rev_ids >> >> From: Charulatha V <charu@ti.com> >> >> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >> (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. >> >> Fix the above by providing non-wakeup GPIO information through pdata >> specific to the SoC. >> >> The GPIO rev id provided in the hwmod database is the same >> for OMAP2420 >> and OMAP2430. Change the GPIO rev ids in hwmod database as given below >> so that it can be used to identify OMAP2420 and OMAP2430. >> OMAP2420 - 0 >> OMAP2430 - 1 >> OMAP3 - 2 >> OMAP4 - 3 > > [sp] Magic numbers should be avoided. > Suggest using something like: > #define GPIO_REV_2420 0 > #define GPIO_REV_2430 1 > #define GPIO_REV_34XX 2 > #define GPIO_REV_44xx 3 > > We don't have to refer back to this comment while reading the code. Okay. > I also believed that HWMODs were auto generated. > Can the changes to structures in this patch recreated using current > scripts? Benoit's and Paul's ack is required for this change. > > ~sanjeev > >> >> Signed-off-by: Charulatha V <charu@ti.com> >> Cc: Cousson, Benoit <b-cousson@ti.com> >> Cc: Paul Walmsley <paul@pwsan.com> >> --- >> arch/arm/mach-omap2/gpio.c | 26 >> ++++++++++++++++++++++++-- >> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- >> arch/arm/plat-omap/include/plat/gpio.h | 1 + >> drivers/gpio/gpio_omap.c | 11 +++-------- >> 6 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c >> index 0446bd1..6cd26b4 100644 >> --- a/arch/arm/mach-omap2/gpio.c >> +++ b/arch/arm/mach-omap2/gpio.c >> @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct >> omap_hwmod *oh, void *unused) >> return -ENOMEM; >> } >> >> + switch (oh->class->rev) { >> + case 0: >> + if (id == 1) >> + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ >> + pdata->non_wakeup_gpios = 0xe203ffc0; >> + else if (id == 2) >> + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ >> + pdata->non_wakeup_gpios = 0x08700040; >> + break; >> + case 2: >> + if (id == 2) >> + /* non-wakeup GPIO pins for OMAP3 Bank2 */ >> + pdata->non_wakeup_gpios = 0x00000001; >> + else if (id == 6) >> + /* non-wakeup GPIO pins for OMAP3 Bank6 */ >> + pdata->non_wakeup_gpios = 0x08000000; >> + break; > > [sp] Where is the description on non-wakeup GPIOs in OMAP3? As mentioned in other reply, I am referring to Table 25-5. GPIO Channel Description at page no 3354 OMAP3430 public TRM Version P. Please correct me if I am wrong. > Here is text from AM37x TRM: > [quote ...only relevant text] > Each GPIO module provides 32 dedicated general-purpose pins with input > and output capabilities; .... These pins can be configured for the > following applications: > - Data input (capture)/output (drive) > - Keyboard interface with a debounce cell > - Interrupt generation in .... > - Wake-up request generation in idle mode > [/quote] > Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and > GPIO6_WAKEUPENABLE (0x4905 8020) meant for? > >> + default: >> + /* No non-wakeup GPIO pins for other SoCs */ >> + break; >> + } >> + > > ~sanjeev > > [snip]...[snip] >
On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: >> From: linux-omap-owner@vger.kernel.org >> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of >> DebBarma, Tarun Kanti >> Sent: Tuesday, May 24, 2011 7:55 PM >> >> From: Charulatha V<charu@ti.com> >> >> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect. >> >> Fix the above by providing non-wakeup GPIO information through pdata >> specific to the SoC. >> >> The GPIO rev id provided in the hwmod database is the same >> for OMAP2420 >> and OMAP2430. Change the GPIO rev ids in hwmod database as given below >> so that it can be used to identify OMAP2420 and OMAP2430. >> OMAP2420 - 0 >> OMAP2430 - 1 >> OMAP3 - 2 >> OMAP4 - 3 > > [sp] Magic numbers should be avoided. > Suggest using something like: > #define GPIO_REV_2420 0 > #define GPIO_REV_2430 1 > #define GPIO_REV_34XX 2 > #define GPIO_REV_44xx 3 Well, it is not a magic number, it is a revision id that is incremented each time there is a difference in the IP. The OMAP version -> IP version link is done at hwmod level. The driver does not have to know it is an OMAP3 or OMAP4. In that case we did change the IP version for every revisions, but OMAP5 will use the OMAP4 revision. I'm not even considering all the Davinci variants that are not named OMAP but do use OMAP IPs... as you already know... That can provide a rather confusing information for my point of view. Whereas a comment like that will give you the exhaustive information. 0: OMAP2420 1: OMAP2430 2: OMAP3, DMxxx 3: OMAP4, OMAP5, DM816x That being said, some drivers already did that, so I'm not opposed to such change. I just think it is not the best approach. At least it gives a pointer to the TRM that contains the IP doc. > We don't have to refer back to this comment while reading the code. > I also believed that HWMODs were auto generated. > Can the changes to structures in this patch recreated using current > scripts? Only OMAP4 & 5 are generated today, and in anycase this information is some custom data added by driver owner. So we just have to update that information. I cannot really use the real HW IP version because it is irrelevant most of the time. Maybe in the future we will be able to force the HW team to provide relevant data in that field :-) Regards, Benoit > > ~sanjeev > >> >> Signed-off-by: Charulatha V<charu@ti.com> >> Cc: Cousson, Benoit<b-cousson@ti.com> >> Cc: Paul Walmsley<paul@pwsan.com> >> --- >> arch/arm/mach-omap2/gpio.c | 26 >> ++++++++++++++++++++++++-- >> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- >> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- >> arch/arm/plat-omap/include/plat/gpio.h | 1 + >> drivers/gpio/gpio_omap.c | 11 +++-------- >> 6 files changed, 31 insertions(+), 13 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c >> index 0446bd1..6cd26b4 100644 >> --- a/arch/arm/mach-omap2/gpio.c >> +++ b/arch/arm/mach-omap2/gpio.c >> @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct >> omap_hwmod *oh, void *unused) >> return -ENOMEM; >> } >> >> + switch (oh->class->rev) { >> + case 0: >> + if (id == 1) >> + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ >> + pdata->non_wakeup_gpios = 0xe203ffc0; >> + else if (id == 2) >> + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ >> + pdata->non_wakeup_gpios = 0x08700040; >> + break; >> + case 2: >> + if (id == 2) >> + /* non-wakeup GPIO pins for OMAP3 Bank2 */ >> + pdata->non_wakeup_gpios = 0x00000001; >> + else if (id == 6) >> + /* non-wakeup GPIO pins for OMAP3 Bank6 */ >> + pdata->non_wakeup_gpios = 0x08000000; >> + break; > > [sp] Where is the description on non-wakeup GPIOs in OMAP3? > Here is text from AM37x TRM: > [quote ...only relevant text] > Each GPIO module provides 32 dedicated general-purpose pins with input > and output capabilities; .... These pins can be configured for the > following applications: > - Data input (capture)/output (drive) > - Keyboard interface with a debounce cell > - Interrupt generation in .... > - Wake-up request generation in idle mode > [/quote] > Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and > GPIO6_WAKEUPENABLE (0x4905 8020) meant for? > >> + default: >> + /* No non-wakeup GPIO pins for other SoCs */ >> + break; >> + } >> + > > ~sanjeev > > [snip]...[snip]
> -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 3:41 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > linux-arm-kernel@lists.infradead.org; Varadarajan, > Charulatha; Paul Walmsley > Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > GPIO and rev_ids > > On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: > >> From: linux-omap-owner@vger.kernel.org > >> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of > >> DebBarma, Tarun Kanti > >> Sent: Tuesday, May 24, 2011 7:55 PM > >> > >> From: Charulatha V<charu@ti.com> > >> > >> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But > >> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx > >> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect. > >> > >> Fix the above by providing non-wakeup GPIO information > through pdata > >> specific to the SoC. > >> > >> The GPIO rev id provided in the hwmod database is the same > >> for OMAP2420 > >> and OMAP2430. Change the GPIO rev ids in hwmod database as > given below > >> so that it can be used to identify OMAP2420 and OMAP2430. > >> OMAP2420 - 0 > >> OMAP2430 - 1 > >> OMAP3 - 2 > >> OMAP4 - 3 > > > > [sp] Magic numbers should be avoided. > > Suggest using something like: > > #define GPIO_REV_2420 0 > > #define GPIO_REV_2430 1 > > #define GPIO_REV_34XX 2 > > #define GPIO_REV_44xx 3 > > Well, it is not a magic number, it is a revision id that is > incremented > each time there is a difference in the IP. > The OMAP version -> IP version link is done at hwmod level. > The driver > does not have to know it is an OMAP3 or OMAP4. In that case we did > change the IP version for every revisions, but OMAP5 will use > the OMAP4 > revision. > I'm not even considering all the Davinci variants that are not named > OMAP but do use OMAP IPs... as you already know... > That can provide a rather confusing information for my point of view. > > Whereas a comment like that will give you the exhaustive information. > > 0: OMAP2420 > 1: OMAP2430 > 2: OMAP3, DMxxx > 3: OMAP4, OMAP5, DM816x > > That being said, some drivers already did that, so I'm not opposed to > such change. I just think it is not the best approach. > At least it gives a pointer to the TRM that contains the IP doc. [sp] Benoit, your are right from generation perspectove where the it is easier to increment numbers but when we use comparison code - as in this patch, comparison against a number isn't readable. As example: + switch (oh->class->rev) { ## This is auto-generated. + case 0: ## But this is our code. I am recommending this to read as: + switch (oh->class->rev) { ## This is auto-generated. + case GPIO_REV_2420: ## More readable. ~sanjeev > > > We don't have to refer back to this comment while > reading the code. > > I also believed that HWMODs were auto generated. > > Can the changes to structures in this patch recreated > using current > > scripts? > > Only OMAP4 & 5 are generated today, and in anycase this > information is > some custom data added by driver owner. So we just have to > update that > information. > I cannot really use the real HW IP version because it is > irrelevant most > of the time. Maybe in the future we will be able to force the > HW team to > provide relevant data in that field :-) > > Regards, > Benoit > > > > > > ~sanjeev > > > >> > >> Signed-off-by: Charulatha V<charu@ti.com> > >> Cc: Cousson, Benoit<b-cousson@ti.com> > >> Cc: Paul Walmsley<paul@pwsan.com> > >> --- > >> arch/arm/mach-omap2/gpio.c | 26 > >> ++++++++++++++++++++++++-- > >> arch/arm/mach-omap2/omap_hwmod_2430_data.c | 2 +- > >> arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 2 +- > >> arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 2 +- > >> arch/arm/plat-omap/include/plat/gpio.h | 1 + > >> drivers/gpio/gpio_omap.c | 11 +++-------- > >> 6 files changed, 31 insertions(+), 13 deletions(-) > >> > >> diff --git a/arch/arm/mach-omap2/gpio.c > b/arch/arm/mach-omap2/gpio.c > >> index 0446bd1..6cd26b4 100644 > >> --- a/arch/arm/mach-omap2/gpio.c > >> +++ b/arch/arm/mach-omap2/gpio.c > >> @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct > >> omap_hwmod *oh, void *unused) > >> return -ENOMEM; > >> } > >> > >> + switch (oh->class->rev) { > >> + case 0: > >> + if (id == 1) > >> + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ > >> + pdata->non_wakeup_gpios = 0xe203ffc0; > >> + else if (id == 2) > >> + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ > >> + pdata->non_wakeup_gpios = 0x08700040; > >> + break; > >> + case 2: > >> + if (id == 2) > >> + /* non-wakeup GPIO pins for OMAP3 Bank2 */ > >> + pdata->non_wakeup_gpios = 0x00000001; > >> + else if (id == 6) > >> + /* non-wakeup GPIO pins for OMAP3 Bank6 */ > >> + pdata->non_wakeup_gpios = 0x08000000; > >> + break; > > > > [sp] Where is the description on non-wakeup GPIOs in OMAP3? > > Here is text from AM37x TRM: > > [quote ...only relevant text] > > Each GPIO module provides 32 dedicated > general-purpose pins with input > > and output capabilities; .... These pins can be > configured for the > > following applications: > > - Data input (capture)/output (drive) > > - Keyboard interface with a debounce cell > > - Interrupt generation in .... > > - Wake-up request generation in idle mode > > [/quote] > > Otherwise, what are the GPIO2_WAKEUPENABLE (0x4905 0020) and > > GPIO6_WAKEUPENABLE (0x4905 8020) meant for? > > > >> + default: > >> + /* No non-wakeup GPIO pins for other SoCs */ > >> + break; > >> + } > >> + > > > > ~sanjeev > > > > [snip]...[snip] > >
On 5/26/2011 1:47 PM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Cousson, Benoit >> Sent: Thursday, May 26, 2011 3:41 PM >> To: Premi, Sanjeev >> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; >> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; >> linux-arm-kernel@lists.infradead.org; Varadarajan, >> Charulatha; Paul Walmsley >> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup >> GPIO and rev_ids >> >> On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: >>>> From: linux-omap-owner@vger.kernel.org >>>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of >>>> DebBarma, Tarun Kanti >>>> Sent: Tuesday, May 24, 2011 7:55 PM >>>> >>>> From: Charulatha V<charu@ti.com> >>>> >>>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >>>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >>>> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect. >>>> >>>> Fix the above by providing non-wakeup GPIO information >> through pdata >>>> specific to the SoC. >>>> >>>> The GPIO rev id provided in the hwmod database is the same >>>> for OMAP2420 >>>> and OMAP2430. Change the GPIO rev ids in hwmod database as >> given below >>>> so that it can be used to identify OMAP2420 and OMAP2430. >>>> OMAP2420 - 0 >>>> OMAP2430 - 1 >>>> OMAP3 - 2 >>>> OMAP4 - 3 >>> >>> [sp] Magic numbers should be avoided. >>> Suggest using something like: >>> #define GPIO_REV_2420 0 >>> #define GPIO_REV_2430 1 >>> #define GPIO_REV_34XX 2 >>> #define GPIO_REV_44xx 3 >> >> Well, it is not a magic number, it is a revision id that is >> incremented >> each time there is a difference in the IP. >> The OMAP version -> IP version link is done at hwmod level. >> The driver >> does not have to know it is an OMAP3 or OMAP4. In that case we did >> change the IP version for every revisions, but OMAP5 will use >> the OMAP4 >> revision. >> I'm not even considering all the Davinci variants that are not named >> OMAP but do use OMAP IPs... as you already know... >> That can provide a rather confusing information for my point of view. >> >> Whereas a comment like that will give you the exhaustive information. >> >> 0: OMAP2420 >> 1: OMAP2430 >> 2: OMAP3, DMxxx >> 3: OMAP4, OMAP5, DM816x >> >> That being said, some drivers already did that, so I'm not opposed to >> such change. I just think it is not the best approach. >> At least it gives a pointer to the TRM that contains the IP doc. > > [sp] Benoit, your are right from generation perspectove where the > it is easier to increment numbers but when we use comparison This is not even related to generation. A version number is a number. If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP version is relevant for the driver. For the documentation point of view, since we do not have a per IP TRM, then it can make sense to know where that IP is documented. But as I said a comment is enough and will allow a much more relevant level of details information than a define. > code - as in this patch, comparison against a number isn't > readable. > > As example: > > + switch (oh->class->rev) { ## This is auto-generated. > + case 0: ## But this is our code. > > I am recommending this to read as: > > + switch (oh->class->rev) { ## This is auto-generated. > + case GPIO_REV_2420: ## More readable. More readable, maybe, but not necessarily relevant nor accurate if the same IP version is used in another chip or revision. What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? Benoit
> -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 5:41 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > linux-arm-kernel@lists.infradead.org; Varadarajan, > Charulatha; Paul Walmsley > Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > GPIO and rev_ids > > On 5/26/2011 1:47 PM, Premi, Sanjeev wrote: > >> -----Original Message----- > >> From: Cousson, Benoit > >> Sent: Thursday, May 26, 2011 3:41 PM > >> To: Premi, Sanjeev > >> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > >> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > >> linux-arm-kernel@lists.infradead.org; Varadarajan, > >> Charulatha; Paul Walmsley > >> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > >> GPIO and rev_ids > >> > >> On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: > >>>> From: linux-omap-owner@vger.kernel.org > >>>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of > >>>> DebBarma, Tarun Kanti > >>>> Sent: Tuesday, May 24, 2011 7:55 PM > >>>> > >>>> From: Charulatha V<charu@ti.com> > >>>> > >>>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But > >>>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx > >>>> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect. > >>>> > >>>> Fix the above by providing non-wakeup GPIO information > >> through pdata > >>>> specific to the SoC. > >>>> > >>>> The GPIO rev id provided in the hwmod database is the same > >>>> for OMAP2420 > >>>> and OMAP2430. Change the GPIO rev ids in hwmod database as > >> given below > >>>> so that it can be used to identify OMAP2420 and OMAP2430. > >>>> OMAP2420 - 0 > >>>> OMAP2430 - 1 > >>>> OMAP3 - 2 > >>>> OMAP4 - 3 > >>> > >>> [sp] Magic numbers should be avoided. > >>> Suggest using something like: > >>> #define GPIO_REV_2420 0 > >>> #define GPIO_REV_2430 1 > >>> #define GPIO_REV_34XX 2 > >>> #define GPIO_REV_44xx 3 > >> > >> Well, it is not a magic number, it is a revision id that is > >> incremented > >> each time there is a difference in the IP. > >> The OMAP version -> IP version link is done at hwmod level. > >> The driver > >> does not have to know it is an OMAP3 or OMAP4. In that case we did > >> change the IP version for every revisions, but OMAP5 will use > >> the OMAP4 > >> revision. > >> I'm not even considering all the Davinci variants that are > not named > >> OMAP but do use OMAP IPs... as you already know... > >> That can provide a rather confusing information for my > point of view. > >> > >> Whereas a comment like that will give you the exhaustive > information. > >> > >> 0: OMAP2420 > >> 1: OMAP2430 > >> 2: OMAP3, DMxxx > >> 3: OMAP4, OMAP5, DM816x > >> > >> That being said, some drivers already did that, so I'm not > opposed to > >> such change. I just think it is not the best approach. > >> At least it gives a pointer to the TRM that contains the IP doc. > > > > [sp] Benoit, your are right from generation perspectove where the > > it is easier to increment numbers but when we use comparison > > This is not even related to generation. A version number is a number. > If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP > version is relevant for the driver. > For the documentation point of view, since we do not have a > per IP TRM, > then it can make sense to know where that IP is documented. But as I > said a comment is enough and will allow a much more relevant level of > details information than a define. > > > code - as in this patch, comparison against a number isn't > > readable. > > > > As example: > > > > + switch (oh->class->rev) { ## This is auto-generated. > > + case 0: ## But this is our code. > > > > I am recommending this to read as: > > > > + switch (oh->class->rev) { ## This is auto-generated. > > + case GPIO_REV_2420: ## More readable. > > More readable, maybe, but not necessarily relevant nor > accurate if the > same IP version is used in another chip or revision. > > What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? How is value 0 accurate? > > Benoit >
On 5/26/2011 2:38 PM, Premi, Sanjeev wrote: >> -----Original Message----- >> From: Cousson, Benoit >> Sent: Thursday, May 26, 2011 5:41 PM >> To: Premi, Sanjeev >> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; >> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; >> linux-arm-kernel@lists.infradead.org; Varadarajan, >> Charulatha; Paul Walmsley >> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup >> GPIO and rev_ids >> >> On 5/26/2011 1:47 PM, Premi, Sanjeev wrote: >>>> -----Original Message----- >>>> From: Cousson, Benoit >>>> Sent: Thursday, May 26, 2011 3:41 PM >>>> To: Premi, Sanjeev >>>> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; >>>> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; >>>> linux-arm-kernel@lists.infradead.org; Varadarajan, >>>> Charulatha; Paul Walmsley >>>> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup >>>> GPIO and rev_ids >>>> >>>> On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: >>>>>> From: linux-omap-owner@vger.kernel.org >>>>>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of >>>>>> DebBarma, Tarun Kanti >>>>>> Sent: Tuesday, May 24, 2011 7:55 PM >>>>>> >>>>>> From: Charulatha V<charu@ti.com> >>>>>> >>>>>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >>>>>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >>>>>> (bothe OMAP 2420 and 2430)& not for OMAP3 which is incorrect. >>>>>> >>>>>> Fix the above by providing non-wakeup GPIO information >>>> through pdata >>>>>> specific to the SoC. >>>>>> >>>>>> The GPIO rev id provided in the hwmod database is the same >>>>>> for OMAP2420 >>>>>> and OMAP2430. Change the GPIO rev ids in hwmod database as >>>> given below >>>>>> so that it can be used to identify OMAP2420 and OMAP2430. >>>>>> OMAP2420 - 0 >>>>>> OMAP2430 - 1 >>>>>> OMAP3 - 2 >>>>>> OMAP4 - 3 >>>>> >>>>> [sp] Magic numbers should be avoided. >>>>> Suggest using something like: >>>>> #define GPIO_REV_2420 0 >>>>> #define GPIO_REV_2430 1 >>>>> #define GPIO_REV_34XX 2 >>>>> #define GPIO_REV_44xx 3 >>>> >>>> Well, it is not a magic number, it is a revision id that is >>>> incremented >>>> each time there is a difference in the IP. >>>> The OMAP version -> IP version link is done at hwmod level. >>>> The driver >>>> does not have to know it is an OMAP3 or OMAP4. In that case we did >>>> change the IP version for every revisions, but OMAP5 will use >>>> the OMAP4 >>>> revision. >>>> I'm not even considering all the Davinci variants that are >> not named >>>> OMAP but do use OMAP IPs... as you already know... >>>> That can provide a rather confusing information for my >> point of view. >>>> >>>> Whereas a comment like that will give you the exhaustive >> information. >>>> >>>> 0: OMAP2420 >>>> 1: OMAP2430 >>>> 2: OMAP3, DMxxx >>>> 3: OMAP4, OMAP5, DM816x >>>> >>>> That being said, some drivers already did that, so I'm not >> opposed to >>>> such change. I just think it is not the best approach. >>>> At least it gives a pointer to the TRM that contains the IP doc. >>> >>> [sp] Benoit, your are right from generation perspectove where the >>> it is easier to increment numbers but when we use comparison >> >> This is not even related to generation. A version number is a number. >> If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, only that IP >> version is relevant for the driver. >> For the documentation point of view, since we do not have a >> per IP TRM, >> then it can make sense to know where that IP is documented. But as I >> said a comment is enough and will allow a much more relevant level of >> details information than a define. >> >>> code - as in this patch, comparison against a number isn't >>> readable. >>> >>> As example: >>> >>> + switch (oh->class->rev) { ## This is auto-generated. >>> + case 0: ## But this is our code. >>> >>> I am recommending this to read as: >>> >>> + switch (oh->class->rev) { ## This is auto-generated. >>> + case GPIO_REV_2420: ## More readable. >> >> More readable, maybe, but not necessarily relevant nor >> accurate if the >> same IP version is used in another chip or revision. >> >> What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? > > How is value 0 accurate? 0 is just the revision number that can give you the exact list of Soc that does use that IP. Thanks to an exhaustive comment. 0: OMAP2420 1: OMAP2430 2: OMAP3, DMxxx 3: OMAP4, OMAP5, DM816x BTW, I'm still waiting the answer for previous question... Regards, Benoit
> -----Original Message----- > From: Cousson, Benoit > Sent: Thursday, May 26, 2011 6:17 PM > To: Premi, Sanjeev > Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > linux-arm-kernel@lists.infradead.org; Varadarajan, > Charulatha; Paul Walmsley > Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > GPIO and rev_ids > > On 5/26/2011 2:38 PM, Premi, Sanjeev wrote: > >> -----Original Message----- > >> From: Cousson, Benoit > >> Sent: Thursday, May 26, 2011 5:41 PM > >> To: Premi, Sanjeev > >> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > >> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > >> linux-arm-kernel@lists.infradead.org; Varadarajan, > >> Charulatha; Paul Walmsley > >> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > >> GPIO and rev_ids > >> > >> On 5/26/2011 1:47 PM, Premi, Sanjeev wrote: > >>>> -----Original Message----- > >>>> From: Cousson, Benoit > >>>> Sent: Thursday, May 26, 2011 3:41 PM > >>>> To: Premi, Sanjeev > >>>> Cc: DebBarma, Tarun Kanti; linux-omap@vger.kernel.org; > >>>> Hilman, Kevin; Shilimkar, Santosh; tony@atomide.com; > >>>> linux-arm-kernel@lists.infradead.org; Varadarajan, > >>>> Charulatha; Paul Walmsley > >>>> Subject: Re: [PATCH 02/15] OMAP2PLUS: GPIO: Fix non-wakeup > >>>> GPIO and rev_ids > >>>> > >>>> On 5/26/2011 11:23 AM, Premi, Sanjeev wrote: > >>>>>> From: linux-omap-owner@vger.kernel.org > >>>>>> [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of > >>>>>> DebBarma, Tarun Kanti > >>>>>> Sent: Tuesday, May 24, 2011 7:55 PM > >>>>>> > >>>>>> From: Charulatha V<charu@ti.com> > >>>>>> > >>>>>> Non-wakeup GPIOs are available only in OMAP2420 and > OMAP3430. But > >>>>>> the GPIO driver initializes the non-wakeup GPIO bits > for OMAP24xx > >>>>>> (bothe OMAP 2420 and 2430)& not for OMAP3 which is > incorrect. > >>>>>> > >>>>>> Fix the above by providing non-wakeup GPIO information > >>>> through pdata > >>>>>> specific to the SoC. > >>>>>> > >>>>>> The GPIO rev id provided in the hwmod database is the same > >>>>>> for OMAP2420 > >>>>>> and OMAP2430. Change the GPIO rev ids in hwmod database as > >>>> given below > >>>>>> so that it can be used to identify OMAP2420 and OMAP2430. > >>>>>> OMAP2420 - 0 > >>>>>> OMAP2430 - 1 > >>>>>> OMAP3 - 2 > >>>>>> OMAP4 - 3 > >>>>> > >>>>> [sp] Magic numbers should be avoided. > >>>>> Suggest using something like: > >>>>> #define GPIO_REV_2420 0 > >>>>> #define GPIO_REV_2430 1 > >>>>> #define GPIO_REV_34XX 2 > >>>>> #define GPIO_REV_44xx 3 > >>>> > >>>> Well, it is not a magic number, it is a revision id that is > >>>> incremented > >>>> each time there is a difference in the IP. [sp] Just to quote a definition: [quote]The term magic number or magic constant also refers to the programming practice of using numbers directly in source code. [/quote] It has no relation to whether/how it can signify something important. > >>>> The OMAP version -> IP version link is done at hwmod level. > >>>> The driver > >>>> does not have to know it is an OMAP3 or OMAP4. In that > case we did > >>>> change the IP version for every revisions, but OMAP5 will use > >>>> the OMAP4 > >>>> revision. [sp] I don't understand the confusion because IPs are anyways going to be used across. Many macros originally created for OMAP34XX are true/valid for OMAP36XX. And same holds good (probably) for OMAP24XX as well. But in each context, we are aware of the intended usage and can relate to the use and reuse better because it is quite evident. Use of OMAP2_CONTROL_GENERAL in OMAP34xx specific function is not accurate by your example below, but we all know the meaning and need for the same. > >>>> I'm not even considering all the Davinci variants that are > >> not named > >>>> OMAP but do use OMAP IPs... as you already know... > >>>> That can provide a rather confusing information for my > >> point of view. [sp] So now what happens when existing IP gets changed to fix an issue in existing OMAP3 design. Going by same logic will we add another id for the same? > >>>> > >>>> Whereas a comment like that will give you the exhaustive > >> information. > >>>> > >>>> 0: OMAP2420 > >>>> 1: OMAP2430 > >>>> 2: OMAP3, DMxxx > >>>> 3: OMAP4, OMAP5, DM816x > >>>> > >>>> That being said, some drivers already did that, so I'm not > >> opposed to > >>>> such change. I just think it is not the best approach. [sp] Taking same point further, if the IP from OMAP4 above gets used to create OMAP3699 (for example). How does this case get handled? OR we expect users to know about exceptions each time they read code? Leaving aside the fact the situation is hypothetical; design should be generic to handle such situations. > >>>> At least it gives a pointer to the TRM that contains the IP doc. > >>> > >>> [sp] Benoit, your are right from generation perspectove where the > >>> it is easier to increment numbers but when we use > comparison > >> > >> This is not even related to generation. A version number > is a number. > >> If OMAP2 is using GPIO v1.6 and OMAP3 the version v2.2, > only that IP > >> version is relevant for the driver. > >> For the documentation point of view, since we do not have a > >> per IP TRM, > >> then it can make sense to know where that IP is > documented. But as I > >> said a comment is enough and will allow a much more > relevant level of > >> details information than a define. [sp] Based on what I have read, it makes more sense is to have the IP version used in this field verbatim. Then there is no pollution with processor containing the IP. > >> > >>> code - as in this patch, comparison against a number isn't > >>> readable. > >>> > >>> As example: > >>> > >>> + switch (oh->class->rev) { ## This is auto-generated. > >>> + case 0: ## But this is our code. > >>> > >>> I am recommending this to read as: > >>> > >>> + switch (oh->class->rev) { ## This is auto-generated. > >>> + case GPIO_REV_2420: ## More readable. > >> > >> More readable, maybe, but not necessarily relevant nor > >> accurate if the > >> same IP version is used in another chip or revision. > >> > >> What IP version is used in a DM816x? GPIO_REV_3430, GPIO_REV_4430? > > > > How is value 0 accurate? > > 0 is just the revision number that can give you the exact list of Soc > that does use that IP. Thanks to an exhaustive comment. [sp] The exhaustive comment will not accompany every place the field "rev" is used in the code. So someone needs to find this comment and then get the meaning of a revision. Otherwise why do we define so many macros in files like control.h? Exhaustive comments would have served well! > > 0: OMAP2420 > 1: OMAP2430 > 2: OMAP3, DMxxx > 3: OMAP4, OMAP5, DM816x > > > BTW, I'm still waiting the answer for previous question... [sp] I didn't find a question - but responsded to what I appeared to me as question(s). I will be offline for few days. Futher responses will be delayed. ~sanjeev > > Regards, > Benoit >
Why not do the following:
#define OMAP_GPIO_REV_0 0
#define OMAP_GPIO_REV_1 1
#define OMAP_GPIO_REV_2 2
#define OMAP_GPIO_REV_3 3
/*
OMAP_GPIO_REV_0: OMAP2420
OMAP_GPIO_REV_1: OMAP2430
OMAP_GPIO_REV_2: OMAP3, DMxxx
OMAP_GPIO_REV_3: OMAP4, OMAP5, DM816x
*/
+ switch (oh->class->rev) { ## This is auto-generated.
+ case 0: ## But this is our code.
I am recommending this to read as:
+ switch (oh->class->rev) { ## This is auto-generated.
+ case OMAP_GPIO_REV_0: ## More readable.
That approach solves both issues -- Revision -> Chip mapping in comment, no magic numbers in the code, and no implied restriction of the rev number to a specific SoC. Using the defines makes it easier to search the code for a specific revision, since you would no longer get false positives for all the other '0' and '1' constants that appear in the code. It also makes it indexible by tools like LXR.
B.J. Buchalter
Metric Halo
http://www.mhlabs.com
On 5/26/2011 3:38 PM, B.J. Buchalter wrote: > Why not do the following: > > #define OMAP_GPIO_REV_0 0 > #define OMAP_GPIO_REV_1 1 > #define OMAP_GPIO_REV_2 2 > #define OMAP_GPIO_REV_3 3 > /* > OMAP_GPIO_REV_0: OMAP2420 > OMAP_GPIO_REV_1: OMAP2430 > OMAP_GPIO_REV_2: OMAP3, DMxxx > OMAP_GPIO_REV_3: OMAP4, OMAP5, DM816x > */ > > + switch (oh->class->rev) { ## This is auto-generated. > + case 0: ## But this is our code. > > I am recommending this to read as: > > + switch (oh->class->rev) { ## This is auto-generated. > + case OMAP_GPIO_REV_0: ## More readable. > > That approach solves both issues -- Revision -> Chip mapping in comment, no magic numbers in the code, and no implied restriction of the rev number to a specific SoC. Using the defines makes it easier to search the code for a specific revision, since you would no longer get false positives for all the other '0' and '1' constants that appear in the code. It also makes it indexible by tools like LXR. This is indeed what was done for I2C recently. The point is that this kind of define does not bring a lot of semantic. But on the other hand it will definitively help the search aspect. Thanks, Benoit
"Varadarajan, Charulatha" <charu@ti.com> writes: > On Thu, May 26, 2011 at 03:04, Kevin Hilman <khilman@ti.com> wrote: >> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: >> >>> From: Charulatha V <charu@ti.com> >>> >>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >>> (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. >> >> Can you cite the documentation you're using for the OMAP3 non-wakeup >> GPIOs? > > I am referring to Table 25-5. GPIO Channel Description at page no 3354 > OMAP3430 public TRM Version P. > Based on how I understand that, those non-wakeup GPIOs are non-wakeup because they're not actually mapped anywhere, and not available on external pins. Therefore, they should not need any special case handling for this triggering workaround. Kevin
Kevin, On Thu, May 26, 2011 at 12:15, Kevin Hilman <khilman@ti.com> wrote: > "Varadarajan, Charulatha" <charu@ti.com> writes: > >> On Thu, May 26, 2011 at 03:04, Kevin Hilman <khilman@ti.com> wrote: >>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: >>> >>>> From: Charulatha V <charu@ti.com> >>>> >>>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >>>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >>>> (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. >>> >>> Can you cite the documentation you're using for the OMAP3 non-wakeup >>> GPIOs? >> >> I am referring to Table 25-5. GPIO Channel Description at page no 3354 >> OMAP3430 public TRM Version P. >> > > Based on how I understand that, those non-wakeup GPIOs are non-wakeup > because they're not actually mapped anywhere, and not available on > external pins. Therefore, they should not need any special case > handling for this triggering workaround. Okay. Thanks for clarifying. I will not do this for OMAP3. The special case would be only for OMAP2420 against the current code which is handling for OMAP24xx. -V Charulatha > > Kevin > >
"Varadarajan, Charulatha" <charu@ti.com> writes: > Kevin, > > On Thu, May 26, 2011 at 12:15, Kevin Hilman <khilman@ti.com> wrote: >> "Varadarajan, Charulatha" <charu@ti.com> writes: >> >>> On Thu, May 26, 2011 at 03:04, Kevin Hilman <khilman@ti.com> wrote: >>>> Tarun Kanti DebBarma <tarun.kanti@ti.com> writes: >>>> >>>>> From: Charulatha V <charu@ti.com> >>>>> >>>>> Non-wakeup GPIOs are available only in OMAP2420 and OMAP3430. But >>>>> the GPIO driver initializes the non-wakeup GPIO bits for OMAP24xx >>>>> (bothe OMAP 2420 and 2430) & not for OMAP3 which is incorrect. >>>> >>>> Can you cite the documentation you're using for the OMAP3 non-wakeup >>>> GPIOs? >>> >>> I am referring to Table 25-5. GPIO Channel Description at page no 3354 >>> OMAP3430 public TRM Version P. >>> >> >> Based on how I understand that, those non-wakeup GPIOs are non-wakeup >> because they're not actually mapped anywhere, and not available on >> external pins. Therefore, they should not need any special case >> handling for this triggering workaround. > > Okay. Thanks for clarifying. I will not do this for OMAP3. The special case > would be only for OMAP2420 against the current code which is handling > for OMAP24xx. > Great, thanks. Kevin
diff --git a/arch/arm/mach-omap2/gpio.c b/arch/arm/mach-omap2/gpio.c index 0446bd1..6cd26b4 100644 --- a/arch/arm/mach-omap2/gpio.c +++ b/arch/arm/mach-omap2/gpio.c @@ -56,6 +56,28 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) return -ENOMEM; } + switch (oh->class->rev) { + case 0: + if (id == 1) + /* non-wakeup GPIO pins for OMAP2420 Bank1 */ + pdata->non_wakeup_gpios = 0xe203ffc0; + else if (id == 2) + /* non-wakeup GPIO pins for OMAP2420 Bank2 */ + pdata->non_wakeup_gpios = 0x08700040; + break; + case 2: + if (id == 2) + /* non-wakeup GPIO pins for OMAP3 Bank2 */ + pdata->non_wakeup_gpios = 0x00000001; + else if (id == 6) + /* non-wakeup GPIO pins for OMAP3 Bank6 */ + pdata->non_wakeup_gpios = 0x08000000; + break; + default: + /* No non-wakeup GPIO pins for other SoCs */ + break; + } + dev_attr = (struct omap_gpio_dev_attr *)oh->dev_attr; pdata->bank_width = dev_attr->bank_width; pdata->dbck_flag = dev_attr->dbck_flag; @@ -70,6 +92,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) switch (oh->class->rev) { case 0: case 1: + case 2: pdata->bank_type = METHOD_GPIO_24XX; pdata->regs->revision = OMAP24XX_GPIO_REVISION; pdata->regs->direction = OMAP24XX_GPIO_OE; @@ -86,7 +109,7 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) pdata->regs->debounce_en = OMAP24XX_GPIO_DEBOUNCE_EN; pdata->regs->ctrl = OMAP24XX_GPIO_CTRL; break; - case 2: + case 3: pdata->bank_type = METHOD_GPIO_44XX; pdata->regs->revision = OMAP4_GPIO_REVISION; pdata->regs->direction = OMAP4_GPIO_OE; @@ -108,7 +131,6 @@ static int omap2_gpio_dev_init(struct omap_hwmod *oh, void *unused) kfree(pdata); return -EINVAL; } - od = omap_device_build(name, id - 1, oh, pdata, sizeof(*pdata), omap_gpio_latency, ARRAY_SIZE(omap_gpio_latency), diff --git a/arch/arm/mach-omap2/omap_hwmod_2430_data.c b/arch/arm/mach-omap2/omap_hwmod_2430_data.c index 9682dd5..ae702b5 100644 --- a/arch/arm/mach-omap2/omap_hwmod_2430_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_2430_data.c @@ -1728,7 +1728,7 @@ static struct omap_hwmod_class_sysconfig omap243x_gpio_sysc = { static struct omap_hwmod_class omap243x_gpio_hwmod_class = { .name = "gpio", .sysc = &omap243x_gpio_sysc, - .rev = 0, + .rev = 1, }; /* gpio1 */ diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c index 909a84d..05e7005 100644 --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c @@ -2117,7 +2117,7 @@ static struct omap_hwmod_class_sysconfig omap3xxx_gpio_sysc = { static struct omap_hwmod_class omap3xxx_gpio_hwmod_class = { .name = "gpio", .sysc = &omap3xxx_gpio_sysc, - .rev = 1, + .rev = 2, }; /* gpio_dev_attr*/ diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c index abc548a..ea30752 100644 --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c @@ -1703,7 +1703,7 @@ static struct omap_hwmod_class_sysconfig omap44xx_gpio_sysc = { static struct omap_hwmod_class omap44xx_gpio_hwmod_class = { .name = "gpio", .sysc = &omap44xx_gpio_sysc, - .rev = 2, + .rev = 3, }; /* gpio dev_attr */ diff --git a/arch/arm/plat-omap/include/plat/gpio.h b/arch/arm/plat-omap/include/plat/gpio.h index caf432c..8014a8a 100644 --- a/arch/arm/plat-omap/include/plat/gpio.h +++ b/arch/arm/plat-omap/include/plat/gpio.h @@ -199,6 +199,7 @@ struct omap_gpio_platform_data { int bank_width; /* GPIO bank width */ int bank_stride; /* Only needed for omap1 MPUIO */ bool dbck_flag; /* dbck required or not - True for OMAP3&4 */ + u32 non_wakeup_gpios; struct omap_gpio_reg_offs *regs; }; diff --git a/drivers/gpio/gpio_omap.c b/drivers/gpio/gpio_omap.c index dfdc45e..0ba4cdb 100644 --- a/drivers/gpio/gpio_omap.c +++ b/drivers/gpio/gpio_omap.c @@ -1008,7 +1008,7 @@ static inline int init_gpio_info(struct platform_device *pdev) } /* TODO: Cleanup cpu_is_* checks */ -static void omap_gpio_mod_init(struct gpio_bank *bank, int id) +static void omap_gpio_mod_init(struct gpio_bank *bank) { if (cpu_class_is_omap2()) { if (cpu_is_omap44xx()) { @@ -1028,12 +1028,6 @@ static void omap_gpio_mod_init(struct gpio_bank *bank, int id) /* Initialize interface clk ungated, module enabled */ __raw_writel(0, bank->base + OMAP24XX_GPIO_CTRL); - } else if (cpu_is_omap24xx()) { - static const u32 non_wakeup_gpios[] = { - 0xe203ffc0, 0x08700040 - }; - if (id < ARRAY_SIZE(non_wakeup_gpios)) - bank->non_wakeup_gpios = non_wakeup_gpios[id]; } } else if (cpu_class_is_omap1()) { if (bank_is_mpuio(bank)) @@ -1179,6 +1173,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) bank->dbck_flag = pdata->dbck_flag; bank->stride = pdata->bank_stride; bank->width = pdata->bank_width; + bank->non_wakeup_gpios = pdata->non_wakeup_gpios; bank->regs = pdata->regs; @@ -1205,7 +1200,7 @@ static int __devinit omap_gpio_probe(struct platform_device *pdev) pm_runtime_enable(bank->dev); pm_runtime_get_sync(bank->dev); - omap_gpio_mod_init(bank, id); + omap_gpio_mod_init(bank); omap_gpio_chip_init(bank); omap_gpio_show_rev(bank);