Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

[Tony Lindgren]

[-- Attachment #1: Type: text/plain, Size: 730 bytes --]

* Tony Lindgren <tony@atomide.com> [090430 11:56]:
> * Tony Lindgren <tony@atomide.com> [090430 07:06]:
> > * vimal singh <vimalsingh@ti.com> [090429 23:33]:
> > > 'gpmc-onenand.c' is still confusing name. This is not going to used in
> > > all boards anyway.
> > 
> > Why do you think this cannot be used for all boards?
> > 
> > The GPMC timings are totally based on the onenand chip features.
> 
> And these two patches make omap3430sdp to work with the gpmc-onenand
> code. Sync mode does not work, but it seems like it was never enabled
> for sdp anyways.
> 
> Similar patch should work for other boards too.

Setting the sync_write depends on flags and processor, not just flags.
Here's a fixed version of this patch.

Tony

[-- Attachment #2: onenand-add-flags-v2.patch --]
[-- Type: text/x-diff, Size: 5183 bytes --]

>From 7c9f55ca7dcf9768fee7128fab5f5cf3cacc1e21 Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Thu, 30 Apr 2009 11:43:19 -0700
Subject: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2

Allow disabling sync read and write based on flags

Signed-off-by: Tony Lindgren <tony@atomide.com>

diff --git a/arch/arm/mach-omap2/gpmc-onenand.c b/arch/arm/mach-omap2/gpmc-onenand.c
index fc79183..4583c79 100644
--- a/arch/arm/mach-omap2/gpmc-onenand.c
+++ b/arch/arm/mach-omap2/gpmc-onenand.c
@@ -86,15 +86,18 @@ static void omap2_onenand_writew(unsigned short value, void __iomem *addr)
 }
 
 static void set_onenand_cfg(void __iomem *onenand_base, int latency,
-			    int sync_write, int hf)
+				int sync_read, int sync_write, int hf)
 {
 	u32 reg;
 
 	reg = omap2_onenand_readw(onenand_base + ONENAND_REG_SYS_CFG1);
 	reg &= ~((0x7 << ONENAND_SYS_CFG1_BRL_SHIFT) | (0x7 << 9));
 	reg |=	(latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
-		ONENAND_SYS_CFG1_SYNC_READ |
 		ONENAND_SYS_CFG1_BL_16;
+	if (sync_read)
+		reg |= ONENAND_SYS_CFG1_SYNC_READ;
+	else
+		reg &= ~ONENAND_SYS_CFG1_SYNC_READ;
 	if (sync_write)
 		reg |= ONENAND_SYS_CFG1_SYNC_WRITE;
 	else
@@ -106,8 +109,9 @@ static void set_onenand_cfg(void __iomem *onenand_base, int latency,
 	omap2_onenand_writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
 }
 
-static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
-				       int freq)
+static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
+					void __iomem *onenand_base,
+					int freq)
 {
 	struct gpmc_timings t;
 	const int t_cer  = 15;
@@ -118,9 +122,19 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 	const int t_wph  = 30;
 	int min_gpmc_clk_period, t_ces, t_avds, t_avdh, t_ach, t_aavdh, t_rdyo;
 	int tick_ns, div, fclk_offset_ns, fclk_offset, gpmc_clk_ns, latency;
-	int err, ticks_cez, sync_write = 0, first_time = 0, hf = 0;
+	int err, ticks_cez, sync_read, sync_write, first_time = 0, hf = 0;
+	int cs = cfg->cs;
 	u32 reg;
 
+	if (cfg->flags & ONENAND_DIS_SYNC_READ)
+		sync_read = 0;
+	else
+		sync_read = 1;
+	if (cpu_is_omap24xx() || (cfg->flags & ONENAND_DIS_SYNC_WRITE))
+		sync_write = 0;
+	else
+		sync_write = 1;
+
 	if (!freq) {
 		/* Very first call freq is not known */
 		err = omap2_onenand_set_async_mode(cs, onenand_base);
@@ -160,8 +174,6 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 		t_ach   = 6;
 		t_aavdh = 6;
 		t_rdyo  = 9;
-		if (cpu_is_omap34xx())
-			sync_write = 1;
 		break;
 	case 66:
 		min_gpmc_clk_period = 15; /* 66 MHz */
@@ -171,8 +183,6 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 		t_ach   = 6;
 		t_aavdh = 6;
 		t_rdyo  = 11;
-		if (cpu_is_omap34xx())
-			sync_write = 1;
 		break;
 	default:
 		min_gpmc_clk_period = 18; /* 54 MHz */
@@ -182,6 +192,7 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 		t_ach   = 9;
 		t_aavdh = 7;
 		t_rdyo  = 15;
+		sync_write = 0;
 		break;
 	}
 
@@ -198,7 +209,8 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 		latency = 4;
 
 	if (first_time)
-		set_onenand_cfg(onenand_base, latency, sync_write, hf);
+		set_onenand_cfg(onenand_base, latency,
+					sync_read, sync_write, hf);
 
 	if (div == 1) {
 		reg = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG2);
@@ -272,7 +284,7 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1,
 			  GPMC_CONFIG1_WRAPBURST_SUPP |
 			  GPMC_CONFIG1_READMULTIPLE_SUPP |
-			  GPMC_CONFIG1_READTYPE_SYNC |
+			  (sync_read ? GPMC_CONFIG1_READTYPE_SYNC : 0) |
 			  (sync_write ? GPMC_CONFIG1_WRITEMULTIPLE_SUPP : 0) |
 			  (sync_write ? GPMC_CONFIG1_WRITETYPE_SYNC : 0) |
 			  GPMC_CONFIG1_CLKACTIVATIONTIME(fclk_offset) |
@@ -288,7 +300,7 @@ static int omap2_onenand_set_sync_mode(int cs, void __iomem *onenand_base,
 	if (err)
 		return err;
 
-	set_onenand_cfg(onenand_base, latency, sync_write, hf);
+	set_onenand_cfg(onenand_base, latency, sync_read, sync_write, hf);
 
 	return 0;
 }
@@ -298,7 +310,7 @@ static int gpmc_onenand_setup(void __iomem *onenand_base, int freq)
 	struct device *dev = &gpmc_onenand_device.dev;
 
 	/* Set sync timings in GPMC */
-	if (omap2_onenand_set_sync_mode(gpmc_onenand_data->cs, onenand_base,
+	if (omap2_onenand_set_sync_mode(gpmc_onenand_data, onenand_base,
 			freq) < 0) {
 		dev_err(dev, "Unable to set synchronous mode\n");
 		return -EINVAL;
diff --git a/arch/arm/plat-omap/include/mach/onenand.h b/arch/arm/plat-omap/include/mach/onenand.h
index 2a391fa..562b2a9 100644
--- a/arch/arm/plat-omap/include/mach/onenand.h
+++ b/arch/arm/plat-omap/include/mach/onenand.h
@@ -14,6 +14,9 @@
 
 #ifndef __ASM_ARCH_OMAP_ONENAND_H__
 
+#define ONENAND_DIS_SYNC_READ	(1 << 0)
+#define ONENAND_DIS_SYNC_WRITE	(1 << 1)
+
 struct omap_onenand_platform_data {
 	int			cs;
 	int			gpio_irq;
@@ -21,6 +24,7 @@ struct omap_onenand_platform_data {
 	int			nr_parts;
 	int                     (*onenand_setup)(void __iomem *, int freq);
 	int			dma_channel;
+	u8			flags;
 };
 
 int omap2_onenand_rephase(void);

Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

[vimal singh]

On Fri, May 1, 2009 at 11:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Tony Lindgren <tony@atomide.com> [090430 11:56]:
>> * Tony Lindgren <tony@atomide.com> [090430 07:06]:
>> > * vimal singh <vimalsingh@ti.com> [090429 23:33]:
>> > > 'gpmc-onenand.c' is still confusing name. This is not going to used in
>> > > all boards anyway.
>> >
>> > Why do you think this cannot be used for all boards?
>> >
>> > The GPMC timings are totally based on the onenand chip features.
>>
>> And these two patches make omap3430sdp to work with the gpmc-onenand
>> code. Sync mode does not work, but it seems like it was never enabled
>> for sdp anyways.
>>
>> Similar patch should work for other boards too.
>
> Setting the sync_write depends on flags and processor, not just flags.
> Here's a fixed version of this patch.
OK, these both patches seems good to me...
Earlier I was in impression that this patch series is basically to remove
board-*-flash.c files. Since in 3430sdp boards we find out 'CS' number for
flash devices dynamically in different versions of boards. So, I was confused.

> Tony
>


---
Regards,
\/ | |\/| /-\ |_

Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

[Tony Lindgren]

* vimal singh <vimalsingh@ti.com> [090503 22:36]:
> 
> 
> On Fri, May 1, 2009 at 11:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * Tony Lindgren <tony@atomide.com> [090430 11:56]:
> >> * Tony Lindgren <tony@atomide.com> [090430 07:06]:
> >> > * vimal singh <vimalsingh@ti.com> [090429 23:33]:
> >> > > 'gpmc-onenand.c' is still confusing name. This is not going to used in
> >> > > all boards anyway.
> >> >
> >> > Why do you think this cannot be used for all boards?
> >> >
> >> > The GPMC timings are totally based on the onenand chip features.
> >>
> >> And these two patches make omap3430sdp to work with the gpmc-onenand
> >> code. Sync mode does not work, but it seems like it was never enabled
> >> for sdp anyways.
> >>
> >> Similar patch should work for other boards too.
> >
> > Setting the sync_write depends on flags and processor, not just flags.
> > Here's a fixed version of this patch.
> OK, these both patches seems good to me...

OK, thanks for looking.

> Earlier I was in impression that this patch series is basically to remove
> board-*-flash.c files. Since in 3430sdp boards we find out 'CS' number for
> flash devices dynamically in different versions of boards. So, I was confused.

Well looks like those functions are used for at least few boards, so we could
have functions like gpmc_probe_onenand() and gpmc_probe_nor() functions that
could be called from board-*.c files.

That way we could have generic gpmc-onenand.c and gpmc-nor.c, and still do
the necessary probe logic in the board-*.c files.

Regards,

Tony

Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

[vimal singh]

On Mon, May 4, 2009 at 9:29 PM, Tony Lindgren <tony@atomide.com> wrote:
> * vimal singh <vimalsingh@ti.com> [090503 22:36]:
>>
>>
>> On Fri, May 1, 2009 at 11:08 PM, Tony Lindgren <tony@atomide.com> wrote:
>> > * Tony Lindgren <tony@atomide.com> [090430 11:56]:
>> >> * Tony Lindgren <tony@atomide.com> [090430 07:06]:
>> >> > * vimal singh <vimalsingh@ti.com> [090429 23:33]:
>> >> > > 'gpmc-onenand.c' is still confusing name. This is not going to used in
>> >> > > all boards anyway.
>> >> >
>> >> > Why do you think this cannot be used for all boards?
>> >> >
>> >> > The GPMC timings are totally based on the onenand chip features.
>> >>
>> >> And these two patches make omap3430sdp to work with the gpmc-onenand
>> >> code. Sync mode does not work, but it seems like it was never enabled
>> >> for sdp anyways.
>> >>
>> >> Similar patch should work for other boards too.
>> >
>> > Setting the sync_write depends on flags and processor, not just flags.
>> > Here's a fixed version of this patch.
>> OK, these both patches seems good to me...
>
> OK, thanks for looking.
>
>> Earlier I was in impression that this patch series is basically to remove
>> board-*-flash.c files. Since in 3430sdp boards we find out 'CS' number for
>> flash devices dynamically in different versions of boards. So, I was confused.
>
> Well looks like those functions are used for at least few boards, so we could
> have functions like gpmc_probe_onenand() and gpmc_probe_nor() functions that
> could be called from board-*.c files.
>
> That way we could have generic gpmc-onenand.c and gpmc-nor.c, and still do
> the necessary probe logic in the board-*.c files.
But then how we'll be taking care of timing parameter configuration, for
different chips (part numbers), as some of these parts may vary in timing
specifications, and also for different working frequencies.
And if we are going to put those information in board-*.c, then rather I will
prefer separate board-*-flash.c file to handle all this.

---
Regards,
\/ | |\/| /-\ |_

Re: [PATCH] onenand_init: Allow disabling sync read and write based on flags, v2 (Re: [PATCH 1/4] onenand init: Rename board-n800-flash.c to gpmc-onenand.c)

[Tony Lindgren]

* vimal singh <vimalsingh@ti.com> [090504 22:52]:
> 
> 
> On Mon, May 4, 2009 at 9:29 PM, Tony Lindgren <tony@atomide.com> wrote:
> > * vimal singh <vimalsingh@ti.com> [090503 22:36]:
> >>
> >>
> >> On Fri, May 1, 2009 at 11:08 PM, Tony Lindgren <tony@atomide.com> wrote:
> >> > * Tony Lindgren <tony@atomide.com> [090430 11:56]:
> >> >> * Tony Lindgren <tony@atomide.com> [090430 07:06]:
> >> >> > * vimal singh <vimalsingh@ti.com> [090429 23:33]:
> >> >> > > 'gpmc-onenand.c' is still confusing name. This is not going to used in
> >> >> > > all boards anyway.
> >> >> >
> >> >> > Why do you think this cannot be used for all boards?
> >> >> >
> >> >> > The GPMC timings are totally based on the onenand chip features.
> >> >>
> >> >> And these two patches make omap3430sdp to work with the gpmc-onenand
> >> >> code. Sync mode does not work, but it seems like it was never enabled
> >> >> for sdp anyways.
> >> >>
> >> >> Similar patch should work for other boards too.
> >> >
> >> > Setting the sync_write depends on flags and processor, not just flags.
> >> > Here's a fixed version of this patch.
> >> OK, these both patches seems good to me...
> >
> > OK, thanks for looking.
> >
> >> Earlier I was in impression that this patch series is basically to remove
> >> board-*-flash.c files. Since in 3430sdp boards we find out 'CS' number for
> >> flash devices dynamically in different versions of boards. So, I was confused.
> >
> > Well looks like those functions are used for at least few boards, so we could
> > have functions like gpmc_probe_onenand() and gpmc_probe_nor() functions that
> > could be called from board-*.c files.
> >
> > That way we could have generic gpmc-onenand.c and gpmc-nor.c, and still do
> > the necessary probe logic in the board-*.c files.
> But then how we'll be taking care of timing parameter configuration, for
> different chips (part numbers), as some of these parts may vary in timing
> specifications, and also for different working frequencies.

Well at least gpmc-onenand.c already configures things based on the onenand
revision, see ONENAND_REG_VERSION_ID in gpmc_onenand.c.

We should be able to calculate the timings for the connected chip rather than
using hardcoded values for each board. This will save lots of time bringing
up new boards.

> And if we are going to put those information in board-*.c, then rather I will
> prefer separate board-*-flash.c file to handle all this.

Sure we can have it that way if there's a need for that. It depends how much
code is left to initialize there in the end.

To me it sounds like it would contain just the partition tables and few lines
to query the GPMC for connected flash chips. The rest we should be able to
initialize in a generic way.

Regards,

Tony