* [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf
@ 2015-01-24 20:28 Semen Protsenko
2015-01-24 20:28 ` [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static Semen Protsenko
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Semen Protsenko @ 2015-01-24 20:28 UTC (permalink / raw)
To: Roger Quadros, Tony Lindgren; +Cc: linux-omap, linux-kernel
Some GPMC_CONFIG7 register bits marked as "RESERVED", means they
shouldn't be overwritten. A typical approach to handle such bits called
"Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf()
utilizes RMW technique, but implemented incorrectly. Due to obvious typo
in code read register value is being rewritten by another value, which
leads to loss of read RESERVED bits. This patch fixes this.
While at it, replace magic numbers with named constants to improve code
readability.
Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com>
---
drivers/memory/omap-gpmc.c | 20 ++++++++++++++++----
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 24696f5..10eb4ac 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -153,6 +153,15 @@
#define GPMC_CONFIG1_FCLK_DIV4 (GPMC_CONFIG1_FCLK_DIV(3))
#define GPMC_CONFIG7_CSVALID (1 << 6)
+#define GPMC_CONFIG7_BASEADDRESS_MASK 0x3f
+#define GPMC_CONFIG7_CSVALID_MASK BIT(6)
+#define GPMC_CONFIG7_MASKADDRESS_OFFSET 8
+#define GPMC_CONFIG7_MASKADDRESS_MASK (0xf << GPMC_CONFIG7_MASKADDRESS_OFFSET)
+/* All CONFIG7 bits except reserved bits */
+#define GPMC_CONFIG7_MASK (GPMC_CONFIG7_BASEADDRESS_MASK | \
+ GPMC_CONFIG7_CSVALID_MASK | \
+ GPMC_CONFIG7_MASKADDRESS_MASK)
+
#define GPMC_DEVICETYPE_NOR 0
#define GPMC_DEVICETYPE_NAND 2
#define GPMC_CONFIG_WRITEPROTECT 0x00000010
@@ -586,12 +595,15 @@ static int gpmc_cs_set_memconf(int cs, u32 base, u32 size)
if (base & (size - 1))
return -EINVAL;
+ base >>= GPMC_CHUNK_SHIFT;
mask = (1 << GPMC_SECTION_SHIFT) - size;
+ mask >>= GPMC_CHUNK_SHIFT;
+ mask <<= GPMC_CONFIG7_MASKADDRESS_OFFSET;
+
l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG7);
- l &= ~0x3f;
- l = (base >> GPMC_CHUNK_SHIFT) & 0x3f;
- l &= ~(0x0f << 8);
- l |= ((mask >> GPMC_CHUNK_SHIFT) & 0x0f) << 8;
+ l &= ~GPMC_CONFIG7_MASK;
+ l |= base & GPMC_CONFIG7_BASEADDRESS_MASK;
+ l |= mask & GPMC_CONFIG7_MASKADDRESS_MASK;
l |= GPMC_CONFIG7_CSVALID;
gpmc_cs_write_reg(cs, GPMC_CS_CONFIG7, l);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static 2015-01-24 20:28 [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf Semen Protsenko @ 2015-01-24 20:28 ` Semen Protsenko 2015-01-26 9:20 ` Roger Quadros 2015-01-24 20:28 ` [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures Semen Protsenko 2015-01-26 9:34 ` Roger Quadros 2 siblings, 1 reply; 16+ messages in thread From: Semen Protsenko @ 2015-01-24 20:28 UTC (permalink / raw) To: Roger Quadros, Tony Lindgren; +Cc: linux-omap, linux-kernel Fix sparse warning: warning: symbol 'gpmc_cs_get_name' was not declared. Should it be static? Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> --- drivers/memory/omap-gpmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index 10eb4ac..db77adb 100644 --- a/drivers/memory/omap-gpmc.c +++ b/drivers/memory/omap-gpmc.c @@ -668,7 +668,7 @@ static void gpmc_cs_set_name(int cs, const char *name) gpmc->name = name; } -const char *gpmc_cs_get_name(int cs) +static const char *gpmc_cs_get_name(int cs) { struct gpmc_cs_data *gpmc = &gpmc_cs[cs]; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static 2015-01-24 20:28 ` [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static Semen Protsenko @ 2015-01-26 9:20 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:20 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel On 24/01/15 22:28, Semen Protsenko wrote: > Fix sparse warning: > warning: symbol 'gpmc_cs_get_name' was not declared. Should it be static? > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> Acked-by: Roger Quadros <rogerq@ti.com> cheers, -roger > --- > drivers/memory/omap-gpmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index 10eb4ac..db77adb 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -668,7 +668,7 @@ static void gpmc_cs_set_name(int cs, const char *name) > gpmc->name = name; > } > > -const char *gpmc_cs_get_name(int cs) > +static const char *gpmc_cs_get_name(int cs) > { > struct gpmc_cs_data *gpmc = &gpmc_cs[cs]; > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static @ 2015-01-26 9:20 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:20 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel On 24/01/15 22:28, Semen Protsenko wrote: > Fix sparse warning: > warning: symbol 'gpmc_cs_get_name' was not declared. Should it be static? > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> Acked-by: Roger Quadros <rogerq@ti.com> cheers, -roger > --- > drivers/memory/omap-gpmc.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index 10eb4ac..db77adb 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -668,7 +668,7 @@ static void gpmc_cs_set_name(int cs, const char *name) > gpmc->name = name; > } > > -const char *gpmc_cs_get_name(int cs) > +static const char *gpmc_cs_get_name(int cs) > { > struct gpmc_cs_data *gpmc = &gpmc_cs[cs]; > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static 2015-01-26 9:20 ` Roger Quadros (?) @ 2015-02-02 17:07 ` Tony Lindgren -1 siblings, 0 replies; 16+ messages in thread From: Tony Lindgren @ 2015-02-02 17:07 UTC (permalink / raw) To: Roger Quadros; +Cc: Semen Protsenko, linux-omap, linux-kernel * Roger Quadros <rogerq@ti.com> [150126 01:23]: > On 24/01/15 22:28, Semen Protsenko wrote: > > Fix sparse warning: > > warning: symbol 'gpmc_cs_get_name' was not declared. Should it be static? > > > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> > > Acked-by: Roger Quadros <rogerq@ti.com> Roger is going to queue this, so: Acked-by: Tony Lindgren <tony@atomide.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures 2015-01-24 20:28 [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf Semen Protsenko 2015-01-24 20:28 ` [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static Semen Protsenko @ 2015-01-24 20:28 ` Semen Protsenko 2015-01-26 9:50 ` Roger Quadros 2015-01-26 9:34 ` Roger Quadros 2 siblings, 1 reply; 16+ messages in thread From: Semen Protsenko @ 2015-01-24 20:28 UTC (permalink / raw) To: Roger Quadros, Tony Lindgren; +Cc: linux-omap, linux-kernel New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as RESERVED). Seems like these SoCs have new revision of GPMC IP-core (despite of same GPMC_REVISION value as for earlier SoCs). According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are supported by the GPMC, but their use is highly limited. Because only 10 address pins are available on the GPMC interface, the maximum device size supported is 2KiB." >From [1]: - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both limited and full address mode. - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. OMAP5 supports only limited address mode for nonmultiplexed flashes In this mode only A1-A10 lines are being modified by GPMC, which leaves us (on flash devices with 1 word = 2 bytes) only 2^10 * 2 = 2KiB memory that we can access. - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. DRA7XX supports only full address mode for nonmultiplexed flashes (current TRM says contrary, but according to [1] it's typo and gonna be fixed in new DRA7XX TRMs). In full address mode all A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS bit on new OMAP-based devices, because such an action could possibly lead to undefined behavior in GPMC state-machine (this bit is marked as RESERVED now). [1] http://e2e.ti.com/support/omap/f/885/t/396939 Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> --- drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c index db77adb..477d0ba 100644 --- a/drivers/memory/omap-gpmc.c +++ b/drivers/memory/omap-gpmc.c @@ -108,6 +108,7 @@ #define GPMC_HAS_WR_ACCESS 0x1 #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 #define GPMC_HAS_MUX_AAD 0x4 +#define GPMC_HAS_LIMITEDADDRESS 0x8 #define GPMC_NR_WAITPINS 4 @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, } #endif +static void gpmc_disable_limited(void) +{ + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { + u32 val; + + /* Clear limited address i.e. enable A26-A11 */ + val = gpmc_read_reg(GPMC_CONFIG); + val &= ~GPMC_CONFIG_LIMITEDADDRESS; + gpmc_write_reg(GPMC_CONFIG, val); + } +} + /** * gpmc_probe_generic_child - configures the gpmc for a child device * @pdev: pointer to gpmc platform device @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, unsigned long base; const char *name; int ret, cs; - u32 val; if (of_property_read_u32(child, "reg", &cs) < 0) { dev_err(&pdev->dev, "%s has no 'reg' property\n", @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, goto err; } - /* Clear limited address i.e. enable A26-A11 */ - val = gpmc_read_reg(GPMC_CONFIG); - val &= ~GPMC_CONFIG_LIMITEDADDRESS; - gpmc_write_reg(GPMC_CONFIG, val); + gpmc_disable_limited(); /* Enable CS region */ gpmc_cs_enable_mem(cs); @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) * devices support the addr-addr-data multiplex protocol. * * GPMC IP revisions: - * - OMAP24xx = 2.0 - * - OMAP3xxx = 5.0 - * - OMAP44xx/54xx/AM335x = 6.0 + * - OMAP24xx = 2.0 + * - OMAP3xxx = 5.0 + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 */ if (GPMC_REVISION_MAJOR(l) > 0x4) gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; if (GPMC_REVISION_MAJOR(l) > 0x5) gpmc_capability |= GPMC_HAS_MUX_AAD; + if (GPMC_REVISION_MAJOR(l) < 0x6) + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) + if (GPMC_REVISION_MAJOR(l) == 0x6) + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; +#endif + dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l), GPMC_REVISION_MINOR(l)); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures 2015-01-24 20:28 ` [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures Semen Protsenko @ 2015-01-26 9:50 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:50 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel Hi, On 24/01/15 22:28, Semen Protsenko wrote: > New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have > LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as > RESERVED). Seems like these SoCs have new revision of GPMC IP-core > (despite of same GPMC_REVISION value as for earlier SoCs). > > According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are > supported by the GPMC, but their use is highly limited. Because only > 10 address pins are available on the GPMC interface, the maximum device > size supported is 2KiB." > > From [1]: > - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both > limited and full address mode. > - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. According to OMAP5432 TRM SWPU249AC, I can see LIMITEDADDRESS bit in GPMC_CONFIG. Which TRM version are you referring to? > OMAP5 supports only limited address mode for nonmultiplexed flashes > In this mode only A1-A10 lines are being modified by GPMC, which > leaves us (on flash devices with 1 word = 2 bytes) only > 2^10 * 2 = 2KiB memory that we can access. > - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. > DRA7XX supports only full address mode for nonmultiplexed flashes > (current TRM says contrary, but according to [1] it's typo and > gonna be fixed in new DRA7XX TRMs). In full address mode all > A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. > > This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS > bit on new OMAP-based devices, because such an action could possibly > lead to undefined behavior in GPMC state-machine (this bit is marked as > RESERVED now). > > [1] http://e2e.ti.com/support/omap/f/885/t/396939 > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> > --- > drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index db77adb..477d0ba 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -108,6 +108,7 @@ > #define GPMC_HAS_WR_ACCESS 0x1 > #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 > #define GPMC_HAS_MUX_AAD 0x4 > +#define GPMC_HAS_LIMITEDADDRESS 0x8 > > #define GPMC_NR_WAITPINS 4 > > @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, > } > #endif > > +static void gpmc_disable_limited(void) > +{ > + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { > + u32 val; > + > + /* Clear limited address i.e. enable A26-A11 */ > + val = gpmc_read_reg(GPMC_CONFIG); > + val &= ~GPMC_CONFIG_LIMITEDADDRESS; > + gpmc_write_reg(GPMC_CONFIG, val); > + } > +} > + > /** > * gpmc_probe_generic_child - configures the gpmc for a child device > * @pdev: pointer to gpmc platform device > @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > unsigned long base; > const char *name; > int ret, cs; > - u32 val; > > if (of_property_read_u32(child, "reg", &cs) < 0) { > dev_err(&pdev->dev, "%s has no 'reg' property\n", > @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > goto err; > } > > - /* Clear limited address i.e. enable A26-A11 */ > - val = gpmc_read_reg(GPMC_CONFIG); > - val &= ~GPMC_CONFIG_LIMITEDADDRESS; > - gpmc_write_reg(GPMC_CONFIG, val); > + gpmc_disable_limited(); > > /* Enable CS region */ > gpmc_cs_enable_mem(cs); > @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) > * devices support the addr-addr-data multiplex protocol. > * > * GPMC IP revisions: > - * - OMAP24xx = 2.0 > - * - OMAP3xxx = 5.0 > - * - OMAP44xx/54xx/AM335x = 6.0 > + * - OMAP24xx = 2.0 > + * - OMAP3xxx = 5.0 > + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 > */ > if (GPMC_REVISION_MAJOR(l) > 0x4) > gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; > if (GPMC_REVISION_MAJOR(l) > 0x5) > gpmc_capability |= GPMC_HAS_MUX_AAD; > + if (GPMC_REVISION_MAJOR(l) < 0x6) > + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; > +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) > + if (GPMC_REVISION_MAJOR(l) == 0x6) > + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; > +#endif > + This logic will prevent GPMC_HAS_LIMITEDADDRESS to be set on OMAP4 if we have either SOC_OMAP5 or SOC_DRA7XX enabled. So it is not good. I think this patch is unnecessary as we are only setting the reserved bit to 0. It is safe to set RESERVED bit to 0. We are not modifying it as RESERVED bit is meant to be 0. If bootloader is setting the reserved bit to 1, it is a bug in the bootloader. > dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l), > GPMC_REVISION_MINOR(l)); > > cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures @ 2015-01-26 9:50 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:50 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel Hi, On 24/01/15 22:28, Semen Protsenko wrote: > New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have > LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as > RESERVED). Seems like these SoCs have new revision of GPMC IP-core > (despite of same GPMC_REVISION value as for earlier SoCs). > > According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are > supported by the GPMC, but their use is highly limited. Because only > 10 address pins are available on the GPMC interface, the maximum device > size supported is 2KiB." > > From [1]: > - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both > limited and full address mode. > - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. According to OMAP5432 TRM SWPU249AC, I can see LIMITEDADDRESS bit in GPMC_CONFIG. Which TRM version are you referring to? > OMAP5 supports only limited address mode for nonmultiplexed flashes > In this mode only A1-A10 lines are being modified by GPMC, which > leaves us (on flash devices with 1 word = 2 bytes) only > 2^10 * 2 = 2KiB memory that we can access. > - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. > DRA7XX supports only full address mode for nonmultiplexed flashes > (current TRM says contrary, but according to [1] it's typo and > gonna be fixed in new DRA7XX TRMs). In full address mode all > A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. > > This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS > bit on new OMAP-based devices, because such an action could possibly > lead to undefined behavior in GPMC state-machine (this bit is marked as > RESERVED now). > > [1] http://e2e.ti.com/support/omap/f/885/t/396939 > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> > --- > drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index db77adb..477d0ba 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -108,6 +108,7 @@ > #define GPMC_HAS_WR_ACCESS 0x1 > #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 > #define GPMC_HAS_MUX_AAD 0x4 > +#define GPMC_HAS_LIMITEDADDRESS 0x8 > > #define GPMC_NR_WAITPINS 4 > > @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, > } > #endif > > +static void gpmc_disable_limited(void) > +{ > + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { > + u32 val; > + > + /* Clear limited address i.e. enable A26-A11 */ > + val = gpmc_read_reg(GPMC_CONFIG); > + val &= ~GPMC_CONFIG_LIMITEDADDRESS; > + gpmc_write_reg(GPMC_CONFIG, val); > + } > +} > + > /** > * gpmc_probe_generic_child - configures the gpmc for a child device > * @pdev: pointer to gpmc platform device > @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > unsigned long base; > const char *name; > int ret, cs; > - u32 val; > > if (of_property_read_u32(child, "reg", &cs) < 0) { > dev_err(&pdev->dev, "%s has no 'reg' property\n", > @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, > goto err; > } > > - /* Clear limited address i.e. enable A26-A11 */ > - val = gpmc_read_reg(GPMC_CONFIG); > - val &= ~GPMC_CONFIG_LIMITEDADDRESS; > - gpmc_write_reg(GPMC_CONFIG, val); > + gpmc_disable_limited(); > > /* Enable CS region */ > gpmc_cs_enable_mem(cs); > @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) > * devices support the addr-addr-data multiplex protocol. > * > * GPMC IP revisions: > - * - OMAP24xx = 2.0 > - * - OMAP3xxx = 5.0 > - * - OMAP44xx/54xx/AM335x = 6.0 > + * - OMAP24xx = 2.0 > + * - OMAP3xxx = 5.0 > + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 > */ > if (GPMC_REVISION_MAJOR(l) > 0x4) > gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; > if (GPMC_REVISION_MAJOR(l) > 0x5) > gpmc_capability |= GPMC_HAS_MUX_AAD; > + if (GPMC_REVISION_MAJOR(l) < 0x6) > + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; > +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) > + if (GPMC_REVISION_MAJOR(l) == 0x6) > + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; > +#endif > + This logic will prevent GPMC_HAS_LIMITEDADDRESS to be set on OMAP4 if we have either SOC_OMAP5 or SOC_DRA7XX enabled. So it is not good. I think this patch is unnecessary as we are only setting the reserved bit to 0. It is safe to set RESERVED bit to 0. We are not modifying it as RESERVED bit is meant to be 0. If bootloader is setting the reserved bit to 1, it is a bug in the bootloader. > dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l), > GPMC_REVISION_MINOR(l)); > > cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures 2015-01-26 9:50 ` Roger Quadros (?) @ 2015-01-26 13:07 ` Sam Protsenko 2015-01-26 15:46 ` Roger Quadros -1 siblings, 1 reply; 16+ messages in thread From: Sam Protsenko @ 2015-01-26 13:07 UTC (permalink / raw) To: Roger Quadros; +Cc: Tony Lindgren, linux-omap, linux-kernel On Mon, Jan 26, 2015 at 11:50 AM, Roger Quadros <rogerq@ti.com> wrote: > Hi, > > On 24/01/15 22:28, Semen Protsenko wrote: >> New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have >> LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as >> RESERVED). Seems like these SoCs have new revision of GPMC IP-core >> (despite of same GPMC_REVISION value as for earlier SoCs). >> >> According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are >> supported by the GPMC, but their use is highly limited. Because only >> 10 address pins are available on the GPMC interface, the maximum device >> size supported is 2KiB." >> >> From [1]: >> - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both >> limited and full address mode. >> - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. > > According to OMAP5432 TRM SWPU249AC, I can see LIMITEDADDRESS bit in GPMC_CONFIG. > Which TRM version are you referring to? > Wow, it seems I mixed it with OMAP57xx TRM. Good catch! >> OMAP5 supports only limited address mode for nonmultiplexed flashes >> In this mode only A1-A10 lines are being modified by GPMC, which >> leaves us (on flash devices with 1 word = 2 bytes) only >> 2^10 * 2 = 2KiB memory that we can access. >> - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. >> DRA7XX supports only full address mode for nonmultiplexed flashes >> (current TRM says contrary, but according to [1] it's typo and >> gonna be fixed in new DRA7XX TRMs). In full address mode all >> A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. >> >> This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS >> bit on new OMAP-based devices, because such an action could possibly >> lead to undefined behavior in GPMC state-machine (this bit is marked as >> RESERVED now). >> >> [1] http://e2e.ti.com/support/omap/f/885/t/396939 >> >> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> >> --- >> drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >> index db77adb..477d0ba 100644 >> --- a/drivers/memory/omap-gpmc.c >> +++ b/drivers/memory/omap-gpmc.c >> @@ -108,6 +108,7 @@ >> #define GPMC_HAS_WR_ACCESS 0x1 >> #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 >> #define GPMC_HAS_MUX_AAD 0x4 >> +#define GPMC_HAS_LIMITEDADDRESS 0x8 >> >> #define GPMC_NR_WAITPINS 4 >> >> @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, >> } >> #endif >> >> +static void gpmc_disable_limited(void) >> +{ >> + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { >> + u32 val; >> + >> + /* Clear limited address i.e. enable A26-A11 */ >> + val = gpmc_read_reg(GPMC_CONFIG); >> + val &= ~GPMC_CONFIG_LIMITEDADDRESS; >> + gpmc_write_reg(GPMC_CONFIG, val); >> + } >> +} >> + >> /** >> * gpmc_probe_generic_child - configures the gpmc for a child device >> * @pdev: pointer to gpmc platform device >> @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >> unsigned long base; >> const char *name; >> int ret, cs; >> - u32 val; >> >> if (of_property_read_u32(child, "reg", &cs) < 0) { >> dev_err(&pdev->dev, "%s has no 'reg' property\n", >> @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >> goto err; >> } >> >> - /* Clear limited address i.e. enable A26-A11 */ >> - val = gpmc_read_reg(GPMC_CONFIG); >> - val &= ~GPMC_CONFIG_LIMITEDADDRESS; >> - gpmc_write_reg(GPMC_CONFIG, val); >> + gpmc_disable_limited(); >> >> /* Enable CS region */ >> gpmc_cs_enable_mem(cs); >> @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) >> * devices support the addr-addr-data multiplex protocol. >> * >> * GPMC IP revisions: >> - * - OMAP24xx = 2.0 >> - * - OMAP3xxx = 5.0 >> - * - OMAP44xx/54xx/AM335x = 6.0 >> + * - OMAP24xx = 2.0 >> + * - OMAP3xxx = 5.0 >> + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 >> */ >> if (GPMC_REVISION_MAJOR(l) > 0x4) >> gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; >> if (GPMC_REVISION_MAJOR(l) > 0x5) >> gpmc_capability |= GPMC_HAS_MUX_AAD; >> + if (GPMC_REVISION_MAJOR(l) < 0x6) >> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >> +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) >> + if (GPMC_REVISION_MAJOR(l) == 0x6) >> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >> +#endif >> + > > This logic will prevent GPMC_HAS_LIMITEDADDRESS to be set on OMAP4 if we have > either SOC_OMAP5 or SOC_DRA7XX enabled. So it is not good. > Why? OMAP4 devices have 0x6 revision of GPMC, and as I remember they don't define SOC_OMAP5. So this logic enables limited address capability for OMAP4. Am I missing something? > I think this patch is unnecessary as we are only setting the reserved bit to 0. > It is safe to set RESERVED bit to 0. We are not modifying it as RESERVED bit is > meant to be 0. If bootloader is setting the reserved bit to 1, it is a bug in > the bootloader. > Ok, let's drop this patch then. Anyway, I'm gonna reuse it next week for another patch (I want to make it possible to enable/disable limited address mode via dts bool option). Also, can you point me out where I can read about RESERVED bit should be always 0? >> dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l), >> GPMC_REVISION_MINOR(l)); >> >> > > cheers, > -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures 2015-01-26 13:07 ` Sam Protsenko @ 2015-01-26 15:46 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 15:46 UTC (permalink / raw) To: semen.protsenko; +Cc: Tony Lindgren, linux-omap, linux-kernel On 26/01/15 15:07, Sam Protsenko wrote: > On Mon, Jan 26, 2015 at 11:50 AM, Roger Quadros <rogerq@ti.com> wrote: >> Hi, >> >> On 24/01/15 22:28, Semen Protsenko wrote: >>> New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have >>> LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as >>> RESERVED). Seems like these SoCs have new revision of GPMC IP-core >>> (despite of same GPMC_REVISION value as for earlier SoCs). >>> >>> According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are >>> supported by the GPMC, but their use is highly limited. Because only >>> 10 address pins are available on the GPMC interface, the maximum device >>> size supported is 2KiB." >>> >>> From [1]: >>> - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both >>> limited and full address mode. >>> - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. >> >> According to OMAP5432 TRM SWPU249AC, I can see LIMITEDADDRESS bit in GPMC_CONFIG. >> Which TRM version are you referring to? >> > > Wow, it seems I mixed it with OMAP57xx TRM. Good catch! > >>> OMAP5 supports only limited address mode for nonmultiplexed flashes >>> In this mode only A1-A10 lines are being modified by GPMC, which >>> leaves us (on flash devices with 1 word = 2 bytes) only >>> 2^10 * 2 = 2KiB memory that we can access. >>> - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. >>> DRA7XX supports only full address mode for nonmultiplexed flashes >>> (current TRM says contrary, but according to [1] it's typo and >>> gonna be fixed in new DRA7XX TRMs). In full address mode all >>> A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. >>> >>> This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS >>> bit on new OMAP-based devices, because such an action could possibly >>> lead to undefined behavior in GPMC state-machine (this bit is marked as >>> RESERVED now). >>> >>> [1] http://e2e.ti.com/support/omap/f/885/t/396939 >>> >>> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> >>> --- >>> drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- >>> 1 file changed, 24 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >>> index db77adb..477d0ba 100644 >>> --- a/drivers/memory/omap-gpmc.c >>> +++ b/drivers/memory/omap-gpmc.c >>> @@ -108,6 +108,7 @@ >>> #define GPMC_HAS_WR_ACCESS 0x1 >>> #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 >>> #define GPMC_HAS_MUX_AAD 0x4 >>> +#define GPMC_HAS_LIMITEDADDRESS 0x8 >>> >>> #define GPMC_NR_WAITPINS 4 >>> >>> @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, >>> } >>> #endif >>> >>> +static void gpmc_disable_limited(void) >>> +{ >>> + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { >>> + u32 val; >>> + >>> + /* Clear limited address i.e. enable A26-A11 */ >>> + val = gpmc_read_reg(GPMC_CONFIG); >>> + val &= ~GPMC_CONFIG_LIMITEDADDRESS; >>> + gpmc_write_reg(GPMC_CONFIG, val); >>> + } >>> +} >>> + >>> /** >>> * gpmc_probe_generic_child - configures the gpmc for a child device >>> * @pdev: pointer to gpmc platform device >>> @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >>> unsigned long base; >>> const char *name; >>> int ret, cs; >>> - u32 val; >>> >>> if (of_property_read_u32(child, "reg", &cs) < 0) { >>> dev_err(&pdev->dev, "%s has no 'reg' property\n", >>> @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >>> goto err; >>> } >>> >>> - /* Clear limited address i.e. enable A26-A11 */ >>> - val = gpmc_read_reg(GPMC_CONFIG); >>> - val &= ~GPMC_CONFIG_LIMITEDADDRESS; >>> - gpmc_write_reg(GPMC_CONFIG, val); >>> + gpmc_disable_limited(); >>> >>> /* Enable CS region */ >>> gpmc_cs_enable_mem(cs); >>> @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) >>> * devices support the addr-addr-data multiplex protocol. >>> * >>> * GPMC IP revisions: >>> - * - OMAP24xx = 2.0 >>> - * - OMAP3xxx = 5.0 >>> - * - OMAP44xx/54xx/AM335x = 6.0 >>> + * - OMAP24xx = 2.0 >>> + * - OMAP3xxx = 5.0 >>> + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 >>> */ >>> if (GPMC_REVISION_MAJOR(l) > 0x4) >>> gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; >>> if (GPMC_REVISION_MAJOR(l) > 0x5) >>> gpmc_capability |= GPMC_HAS_MUX_AAD; >>> + if (GPMC_REVISION_MAJOR(l) < 0x6) >>> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >>> +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) >>> + if (GPMC_REVISION_MAJOR(l) == 0x6) >>> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >>> +#endif >>> + >> >> This logic will prevent GPMC_HAS_LIMITEDADDRESS to be set on OMAP4 if we have >> either SOC_OMAP5 or SOC_DRA7XX enabled. So it is not good. >> > > Why? OMAP4 devices have 0x6 revision of GPMC, and as I remember they don't > define SOC_OMAP5. So this logic enables limited address capability for OMAP4. > Am I missing something? OMAP4 devices define ARCH_OMAP4. Consider this case ARCH_OMAP4=y, SOC_OMAP5=y, SOC_DRA7=y. In this case LIMITEDADDRESS flag won't be set. > >> I think this patch is unnecessary as we are only setting the reserved bit to 0. >> It is safe to set RESERVED bit to 0. We are not modifying it as RESERVED bit is >> meant to be 0. If bootloader is setting the reserved bit to 1, it is a bug in >> the bootloader. >> > > Ok, let's drop this patch then. Anyway, I'm gonna reuse it next week > for another patch > (I want to make it possible to enable/disable limited address mode via > dts bool option). > Also, can you point me out where I can read about RESERVED bit should > be always 0? bit definition of RESERVED bit of GPMC_CONFIG register in DRA7 TRM states "Write 0 for future compatibility. Read returns 0." cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures @ 2015-01-26 15:46 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 15:46 UTC (permalink / raw) To: semen.protsenko; +Cc: Tony Lindgren, linux-omap, linux-kernel On 26/01/15 15:07, Sam Protsenko wrote: > On Mon, Jan 26, 2015 at 11:50 AM, Roger Quadros <rogerq@ti.com> wrote: >> Hi, >> >> On 24/01/15 22:28, Semen Protsenko wrote: >>> New OMAP-based architectures (like OMAP5, DRA7XX, AM572X) don't have >>> LIMITEDADDRESS bit in GPMC_CONFIG register (this bit marked as >>> RESERVED). Seems like these SoCs have new revision of GPMC IP-core >>> (despite of same GPMC_REVISION value as for earlier SoCs). >>> >>> According to OMAP5 TRM: "Nonmultiplexed NOR flash devices are >>> supported by the GPMC, but their use is highly limited. Because only >>> 10 address pins are available on the GPMC interface, the maximum device >>> size supported is 2KiB." >>> >>> From [1]: >>> - OMAP4 GPMC_CONFIG has LIMITEDADDRESS bit, hence supports both >>> limited and full address mode. >>> - OMAP5 GPMC_CONFIG doesn't have LIMITEDDADDRESS bit. >> >> According to OMAP5432 TRM SWPU249AC, I can see LIMITEDADDRESS bit in GPMC_CONFIG. >> Which TRM version are you referring to? >> > > Wow, it seems I mixed it with OMAP57xx TRM. Good catch! > >>> OMAP5 supports only limited address mode for nonmultiplexed flashes >>> In this mode only A1-A10 lines are being modified by GPMC, which >>> leaves us (on flash devices with 1 word = 2 bytes) only >>> 2^10 * 2 = 2KiB memory that we can access. >>> - DRA7XX GPMC_CONFIG doesn't have LIMITEDADDRESS bit. >>> DRA7XX supports only full address mode for nonmultiplexed flashes >>> (current TRM says contrary, but according to [1] it's typo and >>> gonna be fixed in new DRA7XX TRMs). In full address mode all >>> A1-A26 lines are modified by GPMC, so one can address up to 128 MiB. >>> >>> This patch ensures that GPMC driver doesn't try to modify LIMITEDADDRESS >>> bit on new OMAP-based devices, because such an action could possibly >>> lead to undefined behavior in GPMC state-machine (this bit is marked as >>> RESERVED now). >>> >>> [1] http://e2e.ti.com/support/omap/f/885/t/396939 >>> >>> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> >>> --- >>> drivers/memory/omap-gpmc.c | 32 ++++++++++++++++++++++++-------- >>> 1 file changed, 24 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c >>> index db77adb..477d0ba 100644 >>> --- a/drivers/memory/omap-gpmc.c >>> +++ b/drivers/memory/omap-gpmc.c >>> @@ -108,6 +108,7 @@ >>> #define GPMC_HAS_WR_ACCESS 0x1 >>> #define GPMC_HAS_WR_DATA_MUX_BUS 0x2 >>> #define GPMC_HAS_MUX_AAD 0x4 >>> +#define GPMC_HAS_LIMITEDADDRESS 0x8 >>> >>> #define GPMC_NR_WAITPINS 4 >>> >>> @@ -1709,6 +1710,18 @@ static int gpmc_probe_onenand_child(struct platform_device *pdev, >>> } >>> #endif >>> >>> +static void gpmc_disable_limited(void) >>> +{ >>> + if (gpmc_capability & GPMC_HAS_LIMITEDADDRESS) { >>> + u32 val; >>> + >>> + /* Clear limited address i.e. enable A26-A11 */ >>> + val = gpmc_read_reg(GPMC_CONFIG); >>> + val &= ~GPMC_CONFIG_LIMITEDADDRESS; >>> + gpmc_write_reg(GPMC_CONFIG, val); >>> + } >>> +} >>> + >>> /** >>> * gpmc_probe_generic_child - configures the gpmc for a child device >>> * @pdev: pointer to gpmc platform device >>> @@ -1726,7 +1739,6 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >>> unsigned long base; >>> const char *name; >>> int ret, cs; >>> - u32 val; >>> >>> if (of_property_read_u32(child, "reg", &cs) < 0) { >>> dev_err(&pdev->dev, "%s has no 'reg' property\n", >>> @@ -1805,10 +1817,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev, >>> goto err; >>> } >>> >>> - /* Clear limited address i.e. enable A26-A11 */ >>> - val = gpmc_read_reg(GPMC_CONFIG); >>> - val &= ~GPMC_CONFIG_LIMITEDADDRESS; >>> - gpmc_write_reg(GPMC_CONFIG, val); >>> + gpmc_disable_limited(); >>> >>> /* Enable CS region */ >>> gpmc_cs_enable_mem(cs); >>> @@ -1935,14 +1944,21 @@ static int gpmc_probe(struct platform_device *pdev) >>> * devices support the addr-addr-data multiplex protocol. >>> * >>> * GPMC IP revisions: >>> - * - OMAP24xx = 2.0 >>> - * - OMAP3xxx = 5.0 >>> - * - OMAP44xx/54xx/AM335x = 6.0 >>> + * - OMAP24xx = 2.0 >>> + * - OMAP3xxx = 5.0 >>> + * - OMAP44xx/54xx/AM335x/DRA7XX = 6.0 >>> */ >>> if (GPMC_REVISION_MAJOR(l) > 0x4) >>> gpmc_capability = GPMC_HAS_WR_ACCESS | GPMC_HAS_WR_DATA_MUX_BUS; >>> if (GPMC_REVISION_MAJOR(l) > 0x5) >>> gpmc_capability |= GPMC_HAS_MUX_AAD; >>> + if (GPMC_REVISION_MAJOR(l) < 0x6) >>> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >>> +#if !defined(CONFIG_SOC_OMAP5) && !defined(CONFIG_SOC_DRA7XX) >>> + if (GPMC_REVISION_MAJOR(l) == 0x6) >>> + gpmc_capability |= GPMC_HAS_LIMITEDADDRESS; >>> +#endif >>> + >> >> This logic will prevent GPMC_HAS_LIMITEDADDRESS to be set on OMAP4 if we have >> either SOC_OMAP5 or SOC_DRA7XX enabled. So it is not good. >> > > Why? OMAP4 devices have 0x6 revision of GPMC, and as I remember they don't > define SOC_OMAP5. So this logic enables limited address capability for OMAP4. > Am I missing something? OMAP4 devices define ARCH_OMAP4. Consider this case ARCH_OMAP4=y, SOC_OMAP5=y, SOC_DRA7=y. In this case LIMITEDADDRESS flag won't be set. > >> I think this patch is unnecessary as we are only setting the reserved bit to 0. >> It is safe to set RESERVED bit to 0. We are not modifying it as RESERVED bit is >> meant to be 0. If bootloader is setting the reserved bit to 1, it is a bug in >> the bootloader. >> > > Ok, let's drop this patch then. Anyway, I'm gonna reuse it next week > for another patch > (I want to make it possible to enable/disable limited address mode via > dts bool option). > Also, can you point me out where I can read about RESERVED bit should > be always 0? bit definition of RESERVED bit of GPMC_CONFIG register in DRA7 TRM states "Write 0 for future compatibility. Read returns 0." cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf 2015-01-24 20:28 [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf Semen Protsenko @ 2015-01-26 9:34 ` Roger Quadros 2015-01-24 20:28 ` [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures Semen Protsenko 2015-01-26 9:34 ` Roger Quadros 2 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:34 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel On 24/01/15 22:28, Semen Protsenko wrote: > Some GPMC_CONFIG7 register bits marked as "RESERVED", means they > shouldn't be overwritten. A typical approach to handle such bits called > "Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf() > utilizes RMW technique, but implemented incorrectly. Due to obvious typo > in code read register value is being rewritten by another value, which > leads to loss of read RESERVED bits. This patch fixes this. > > While at it, replace magic numbers with named constants to improve code > readability. > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> This is much nicer. Acked-by: Roger Quadros <rogerq@ti.com> cheers, -roger > --- > drivers/memory/omap-gpmc.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index 24696f5..10eb4ac 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -153,6 +153,15 @@ > #define GPMC_CONFIG1_FCLK_DIV4 (GPMC_CONFIG1_FCLK_DIV(3)) > #define GPMC_CONFIG7_CSVALID (1 << 6) > > +#define GPMC_CONFIG7_BASEADDRESS_MASK 0x3f > +#define GPMC_CONFIG7_CSVALID_MASK BIT(6) > +#define GPMC_CONFIG7_MASKADDRESS_OFFSET 8 > +#define GPMC_CONFIG7_MASKADDRESS_MASK (0xf << GPMC_CONFIG7_MASKADDRESS_OFFSET) > +/* All CONFIG7 bits except reserved bits */ > +#define GPMC_CONFIG7_MASK (GPMC_CONFIG7_BASEADDRESS_MASK | \ > + GPMC_CONFIG7_CSVALID_MASK | \ > + GPMC_CONFIG7_MASKADDRESS_MASK) > + > #define GPMC_DEVICETYPE_NOR 0 > #define GPMC_DEVICETYPE_NAND 2 > #define GPMC_CONFIG_WRITEPROTECT 0x00000010 > @@ -586,12 +595,15 @@ static int gpmc_cs_set_memconf(int cs, u32 base, u32 size) > if (base & (size - 1)) > return -EINVAL; > > + base >>= GPMC_CHUNK_SHIFT; > mask = (1 << GPMC_SECTION_SHIFT) - size; > + mask >>= GPMC_CHUNK_SHIFT; > + mask <<= GPMC_CONFIG7_MASKADDRESS_OFFSET; > + > l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG7); > - l &= ~0x3f; > - l = (base >> GPMC_CHUNK_SHIFT) & 0x3f; > - l &= ~(0x0f << 8); > - l |= ((mask >> GPMC_CHUNK_SHIFT) & 0x0f) << 8; > + l &= ~GPMC_CONFIG7_MASK; > + l |= base & GPMC_CONFIG7_BASEADDRESS_MASK; > + l |= mask & GPMC_CONFIG7_MASKADDRESS_MASK; > l |= GPMC_CONFIG7_CSVALID; > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG7, l); > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf @ 2015-01-26 9:34 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-01-26 9:34 UTC (permalink / raw) To: Semen Protsenko, Tony Lindgren; +Cc: linux-omap, linux-kernel On 24/01/15 22:28, Semen Protsenko wrote: > Some GPMC_CONFIG7 register bits marked as "RESERVED", means they > shouldn't be overwritten. A typical approach to handle such bits called > "Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf() > utilizes RMW technique, but implemented incorrectly. Due to obvious typo > in code read register value is being rewritten by another value, which > leads to loss of read RESERVED bits. This patch fixes this. > > While at it, replace magic numbers with named constants to improve code > readability. > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> This is much nicer. Acked-by: Roger Quadros <rogerq@ti.com> cheers, -roger > --- > drivers/memory/omap-gpmc.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c > index 24696f5..10eb4ac 100644 > --- a/drivers/memory/omap-gpmc.c > +++ b/drivers/memory/omap-gpmc.c > @@ -153,6 +153,15 @@ > #define GPMC_CONFIG1_FCLK_DIV4 (GPMC_CONFIG1_FCLK_DIV(3)) > #define GPMC_CONFIG7_CSVALID (1 << 6) > > +#define GPMC_CONFIG7_BASEADDRESS_MASK 0x3f > +#define GPMC_CONFIG7_CSVALID_MASK BIT(6) > +#define GPMC_CONFIG7_MASKADDRESS_OFFSET 8 > +#define GPMC_CONFIG7_MASKADDRESS_MASK (0xf << GPMC_CONFIG7_MASKADDRESS_OFFSET) > +/* All CONFIG7 bits except reserved bits */ > +#define GPMC_CONFIG7_MASK (GPMC_CONFIG7_BASEADDRESS_MASK | \ > + GPMC_CONFIG7_CSVALID_MASK | \ > + GPMC_CONFIG7_MASKADDRESS_MASK) > + > #define GPMC_DEVICETYPE_NOR 0 > #define GPMC_DEVICETYPE_NAND 2 > #define GPMC_CONFIG_WRITEPROTECT 0x00000010 > @@ -586,12 +595,15 @@ static int gpmc_cs_set_memconf(int cs, u32 base, u32 size) > if (base & (size - 1)) > return -EINVAL; > > + base >>= GPMC_CHUNK_SHIFT; > mask = (1 << GPMC_SECTION_SHIFT) - size; > + mask >>= GPMC_CHUNK_SHIFT; > + mask <<= GPMC_CONFIG7_MASKADDRESS_OFFSET; > + > l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG7); > - l &= ~0x3f; > - l = (base >> GPMC_CHUNK_SHIFT) & 0x3f; > - l &= ~(0x0f << 8); > - l |= ((mask >> GPMC_CHUNK_SHIFT) & 0x0f) << 8; > + l &= ~GPMC_CONFIG7_MASK; > + l |= base & GPMC_CONFIG7_BASEADDRESS_MASK; > + l |= mask & GPMC_CONFIG7_MASKADDRESS_MASK; > l |= GPMC_CONFIG7_CSVALID; > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG7, l); > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf 2015-01-26 9:34 ` Roger Quadros (?) @ 2015-02-02 17:08 ` Tony Lindgren 2015-02-03 9:30 ` Roger Quadros -1 siblings, 1 reply; 16+ messages in thread From: Tony Lindgren @ 2015-02-02 17:08 UTC (permalink / raw) To: Roger Quadros; +Cc: Semen Protsenko, linux-omap, linux-kernel * Roger Quadros <rogerq@ti.com> [150126 01:38]: > On 24/01/15 22:28, Semen Protsenko wrote: > > Some GPMC_CONFIG7 register bits marked as "RESERVED", means they > > shouldn't be overwritten. A typical approach to handle such bits called > > "Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf() > > utilizes RMW technique, but implemented incorrectly. Due to obvious typo > > in code read register value is being rewritten by another value, which > > leads to loss of read RESERVED bits. This patch fixes this. > > > > While at it, replace magic numbers with named constants to improve code > > readability. > > > > Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> > > This is much nicer. > > Acked-by: Roger Quadros <rogerq@ti.com> Roger will queue this so: Acked-by: Tony Lindgren <tony@atomide.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf 2015-02-02 17:08 ` Tony Lindgren @ 2015-02-03 9:30 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-02-03 9:30 UTC (permalink / raw) To: Tony Lindgren, Semen Protsenko; +Cc: linux-omap, linux-kernel On 02/02/15 19:08, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [150126 01:38]: >> On 24/01/15 22:28, Semen Protsenko wrote: >>> Some GPMC_CONFIG7 register bits marked as "RESERVED", means they >>> shouldn't be overwritten. A typical approach to handle such bits called >>> "Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf() >>> utilizes RMW technique, but implemented incorrectly. Due to obvious typo >>> in code read register value is being rewritten by another value, which >>> leads to loss of read RESERVED bits. This patch fixes this. >>> >>> While at it, replace magic numbers with named constants to improve code >>> readability. >>> >>> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> >> >> This is much nicer. >> >> Acked-by: Roger Quadros <rogerq@ti.com> > > Roger will queue this so: > > Acked-by: Tony Lindgren <tony@atomide.com> > Thanks. Patches 1 and 2 queued for v3.21. https://github.com/rogerq/linux/tree/for-v3.21/gpmc-omap cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf @ 2015-02-03 9:30 ` Roger Quadros 0 siblings, 0 replies; 16+ messages in thread From: Roger Quadros @ 2015-02-03 9:30 UTC (permalink / raw) To: Tony Lindgren, Semen Protsenko; +Cc: linux-omap, linux-kernel On 02/02/15 19:08, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [150126 01:38]: >> On 24/01/15 22:28, Semen Protsenko wrote: >>> Some GPMC_CONFIG7 register bits marked as "RESERVED", means they >>> shouldn't be overwritten. A typical approach to handle such bits called >>> "Read-Modify-Write". Writing procedure used in gpmc_cs_set_memconf() >>> utilizes RMW technique, but implemented incorrectly. Due to obvious typo >>> in code read register value is being rewritten by another value, which >>> leads to loss of read RESERVED bits. This patch fixes this. >>> >>> While at it, replace magic numbers with named constants to improve code >>> readability. >>> >>> Signed-off-by: Semen Protsenko <semen.protsenko@globallogic.com> >> >> This is much nicer. >> >> Acked-by: Roger Quadros <rogerq@ti.com> > > Roger will queue this so: > > Acked-by: Tony Lindgren <tony@atomide.com> > Thanks. Patches 1 and 2 queued for v3.21. https://github.com/rogerq/linux/tree/for-v3.21/gpmc-omap cheers, -roger ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2015-02-03 9:30 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-24 20:28 [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf Semen Protsenko 2015-01-24 20:28 ` [PATCH 2/3] ARM: OMAP2+: gpmc: make gpmc_cs_get_name() static Semen Protsenko 2015-01-26 9:20 ` Roger Quadros 2015-01-26 9:20 ` Roger Quadros 2015-02-02 17:07 ` Tony Lindgren 2015-01-24 20:28 ` [PATCH 3/3] ARM: OMAP2+: gpmc: Do not modify LIMITEDADDRESS on new architectures Semen Protsenko 2015-01-26 9:50 ` Roger Quadros 2015-01-26 9:50 ` Roger Quadros 2015-01-26 13:07 ` Sam Protsenko 2015-01-26 15:46 ` Roger Quadros 2015-01-26 15:46 ` Roger Quadros 2015-01-26 9:34 ` [PATCH 1/3] ARM: OMAP2+: gpmc: Fix writing in gpmc_cs_set_memconf Roger Quadros 2015-01-26 9:34 ` Roger Quadros 2015-02-02 17:08 ` Tony Lindgren 2015-02-03 9:30 ` Roger Quadros 2015-02-03 9:30 ` Roger Quadros
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.