Prepare for GPMC driver conversion

Helper function for updating nand platform data has been added the capability to take timing structure arguement. Usage of omap_nand_flash_init() has been replaced by modifed one, omap_nand_flash_init was doing things similar to board_nand_init except that NAND CS# were being acquired based on bootloader setting. As CS# is hardwired for a given board, acquiring gpmc CS# has been removed, and updated with the value on board.

NAND CS# used in beagle board was found to be CS0. Thomas Weber <[email protected]> reported that value of devkit8000 to be CS0. Overo board was found to be using CS0 based on u-boot, while google grep says omap3touchbook too has CS0.

Signed-off-by Afzal Mohammed <[email protected]>

Which boards have been tested with this change?

Reviewed-by Jon Hunter <[email protected]>

Reorganize gpmc-onenand initialization so that changes required for gpmc driver migration can be made smooth.

Ensuring sync read/write are disabled in onenand cannot be expect to work properly unless GPMC is setup, this has been removed.

Two instances of doing the same has been clubbed into one as onenand driver always calls indirectly "omap2_onenand_set_sync_mode", and has placed such that configuring onenand for async read/write would happen once GPMC is setup for async mode (which happens even for sync mode, before configuring to sync mode).

Signed-off-by Afzal Mohammed <[email protected]>

---
 arch/arm/mach-omap2/gpmc-onenand.c |   24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

arch/arm/mach-omap2/gpmc-onenand.c +1 -7

index 71d7c07..fd4c48d 100644
@@ -38,10 +38,9 @@ static struct platform_device gpmc_onenand_device = {
 	.resource	= &gpmc_onenand_resource,
 };
 
-static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
+static int omap2_onenand_set_async_mode(int cs)
 {
 	struct gpmc_timings t;
-	u32 reg;
 	int err;
 
 	const int t_cer = 15;

@@ -55,11 +54,6 @@ static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
 	const int t_wpl = 40;
 	const int t_wph = 30;
 
-	/* Ensure sync read and sync write are disabled */
-	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
-	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
-	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
-
 	memset(&t, 0, sizeof(t));
 	t.sync_clk = 0;
 	t.cs_on = 0;

arch/arm/mach-omap2/gpmc-onenand.c +1 -4

 	if (cfg->flags & ONENAND_SYNC_READ) {
 		sync_read = 1;
 	} else if (cfg->flags & ONENAND_SYNC_READWRITE) {
 		sync_read = 1;
 		sync_write = 1;
 	} else
-		return omap2_onenand_set_async_mode(cs, onenand_base);
+		return 0;
 
 	if (!freq) {
 		/* Very first call freq is not known */
-		err = omap2_onenand_set_async_mode(cs, onenand_base);
-		if (err)
-			return err;
 		freq = omap2_onenand_get_freq(cfg, onenand_base, &clk_dep);
 		first_time = 1;
 	}

I agree with getting rid of the first instance at the beginning of _set_async_mode, but why get rid of the above one? Are you assuming that by default it is in async mode? Could be nice to keep it to be explicit.

Quoted 4 lines

arch/arm/mach-omap2/gpmc-onenand.c +0 -0

@@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	u32 reg;
 	bool clk_dep = false;
 

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 188

Quoted 5 lines

arch/arm/mach-omap2/gpmc-onenand.c +5 -0

+	/* Ensure sync read and sync write are disabled */
+	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
+	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
+	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
+

Should we only set these after we have checked the flags and depending on which flags are set?

arch/arm/mach-omap2/gpmc-onenand.c void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data) · line 417

Quoted 6 lines

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

@@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
 							ONENAND_IO_SIZE - 1;
 
+	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
+

What about putting this in the gpmc_onenand_setup() function before the call to _set_sync_mode? Seems nice to have these together.

Hi Jon,

Quoted 3 lines · Jon

I agree with getting rid of the first instance at the beginning of _set_async_mode, but why get rid of the above one? Are you assuming that by default it is in async mode? Could be nice to keep it to be explicit.

Second one is achieved below

Quoted 4 lines

Quoted 4 lines

arch/arm/mach-omap2/gpmc-onenand.c +0 -0

@@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg,
 	u32 reg;
 	bool clk_dep = false;
 

Quoted 8 lines

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 188

Quoted 5 lines

arch/arm/mach-omap2/gpmc-onenand.c +5 -0

+	/* Ensure sync read and sync write are disabled */
+	reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
+	reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
+	writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
+

Should we only set these after we have checked the flags and depending on which flags are set?

Even for sync mode, setting async mode initially is required

Quoted 9 lines

arch/arm/mach-omap2/gpmc-onenand.c void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data) · line 417

Quoted 6 lines

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

@@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct omap_onenand_platform_data *_onenand_data)
 	gpmc_onenand_resource.end = gpmc_onenand_resource.start +
 							ONENAND_IO_SIZE - 1;
 
+	omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
+

What about putting this in the gpmc_onenand_setup() function before the call to _set_sync_mode? Seems nice to have these together.

Intention of this patch is to setup GPMC async mode before driver starts its job so that reconfiguring GPMC needs to be to be done only once (this helps in avoiding issues when it has to work with gpmc driver).

With the existing code, set_async was done as part of set_sync, hence requiring GPMC to be configured twice after driver takes control, with your suggestion too, GPMC would have to be configured twice.

Quoted 3 lines · Afzal

With the existing code, set_async was done as part of set_sync, hence requiring GPMC to be configured twice after driver takes control, with your suggestion too, GPMC would have to be configured twice.

I am just suggesting that you place the call to set_async_mode in the gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the calls from set_sync (like you have done). So I don't see that these would configure the GPMC twice.

Hi Jon,

Quoted 5 lines · Jon

Quoted 3 lines · Afzal

With the existing code, set_async was done as part of set_sync, hence requiring GPMC to be configured twice after driver takes control, with your suggestion too, GPMC would have to be configured twice.

Quoted 4 lines

I am just suggesting that you place the call to set_async_mode in the gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the calls from set_sync (like you have done). So I don't see that these would configure the GPMC twice.

As gpmc_onenand_setup is a callback by onenand driver, we would have lost the opportunity to configure onenand before driver is probed.

This would cause requirement of double GPMC configuring and we lost the opportunity to configure GPMC before driver is probed.

And the first step for onenand configuration is always to set it to async mode (with the way it is done now), so it seems reasonable to rely on normal GPMC configuration for async & then do reconfigure for sync.

Quoted 2 lines · Afzal

As gpmc_onenand_setup is a callback by onenand driver, we would have lost the opportunity to configure onenand before driver is probed.

Is that a problem? Looks like it is called early in the probe and so I would hope no one is attempting to access the onenand itself before the probe has completed.

Quoted 3 lines

This would cause requirement of double GPMC configuring and we lost the opportunity to configure GPMC before driver is probed.

I am not convinced we need to. Furthermore with your change you do not actually set async mode in the onenand until _set_sync() is called.

Quoted 5 lines

And the first step for onenand configuration is always to set it to async mode (with the way it is done now), so it seems reasonable to rely on normal GPMC configuration for async & then do reconfigure for sync.

Yes but as far as I can see, it seems that this is the intent of the onenand_setup() function to perform the necessary initialisation.

Have you tested onenand? Do you have a board with onenand?

Hi Jon,

Quoted 5 lines · Jon

Quoted 2 lines · Afzal

As gpmc_onenand_setup is a callback by onenand driver, we would have lost the opportunity to configure onenand before driver is probed.

Quoted 3 lines

Is that a problem? Looks like it is called early in the probe and so I would hope no one is attempting to access the onenand itself before the probe has completed.

During gpmc driver probe, it will configure all the connected peripherals, if configuration details are not present at that point of time, gpmc driver will cry out saying that configuration & timings has not been configured, (please see holler if no configuration patch).

And I do not see any reason
why gpmc driver should not be fed with async mode configuration initially, as it has to be done always.

Quoted 6 lines

Quoted 3 lines

This would cause requirement of double GPMC configuring and we lost the opportunity to configure GPMC before driver is probed.

I am not convinced we need to. Furthermore with your change you do not actually set async mode in the onenand until _set_sync() is called.

Yes, setting async mode in onenand is done in set_sync function, and it is always called by onenand driver indirectly.

Seems if setting async mode in onenand is taken out of set_sync & placed it before set_sync invocation in gpmc_onenand_setup, intention will be clear, right ? (even though sequence wise same thing is happening now)

Quoted 6 lines

Quoted 5 lines

And the first step for onenand configuration is always to set it to async mode (with the way it is done now), so it seems reasonable to rely on normal GPMC configuration for async & then do reconfigure for sync.

Quoted 2 lines

Yes but as far as I can see, it seems that this is the intent of the onenand_setup() function to perform the necessary initialisation.

I believe doing it in gpmc_onenand_init is better, due to the reasons mentioned above as well as because function name will correspond to what it is doing, i.e. initialization

Quoted 1 line

Have you tested onenand? Do you have a board with onenand?

Yes, on omap3evm, with help of local patches (as mainline doesn't have those). It was mentioned in cover letter of gpmc driver conversion series

Quoted 4 lines · Afzal

During gpmc driver probe, it will configure all the connected peripherals, if configuration details are not present at that point of time, gpmc driver will cry out saying that configuration & timings has not been configured, (please see holler if no configuration patch).

Sorry, I am not sure if I am missing something here, but isn't the chip-select requested during the gpmc probe? If so then we should not be programming the gpmc registers at all until the chip-select has been allocated. Hence, after the probe seems more appropriate.

Quoted 9 lines

Quoted 2 lines

I am not convinced we need to. Furthermore with your change you do not actually set async mode in the onenand until _set_sync() is called.

Yes, setting async mode in onenand is done in set_sync function, and it is always called by onenand driver indirectly.

Seems if setting async mode in onenand is taken out of set_sync & placed it before set_sync invocation in gpmc_onenand_setup, intention will be clear, right ? (even though sequence wise same thing is happening now)

Exactly.

Quoted 6 lines

Quoted 2 lines

Yes but as far as I can see, it seems that this is the intent of the onenand_setup() function to perform the necessary initialisation.

I believe doing it in gpmc_onenand_init is better, due to the reasons mentioned above as well as because function name will correspond to what it is doing, i.e. initialization

If the chip-select is allocating during the probe, then I don't agree.

Quoted 2 lines

Yes, on omap3evm, with help of local patches (as mainline doesn't have those). It was mentioned in cover letter of gpmc driver conversion series

Great. Sorry but with 20+ patch spread across 3 series it is not always easy to find the details. So ideally it should be mentioned in this cover letter too.

Configure busturnaround, cycle2cycledelay, waitmonitoringtime, clkactivationtime in gpmc_cs_set_timings(). This is done so that boards can configure these parameters of gpmc in Kernel instead of relying on bootloader.

Signed-off-by Afzal Mohammed <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c             |    6 ++++++
 arch/arm/plat-omap/include/plat/gpmc.h |    6 ++++++
 2 files changed, 12 insertions(+)

arch/arm/mach-omap2/gpmc.c +6 -0

index 578fd4c..517953f 100644
@@ -313,6 +313,12 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
 
 	GPMC_SET_ONE(GPMC_CS_CONFIG5, 24, 27, page_burst_access);
 
+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
+
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
+
 	if (cpu_is_omap34xx()) {
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
 		GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);

arch/arm/plat-omap/include/plat/gpmc.h +0 -0

index 2e6e259..802fb22 100644
@@ -128,6 +128,12 @@ struct gpmc_timings {
 	u16 rd_cycle;		/* Total read cycle time */
 	u16 wr_cycle;		/* Total write cycle time */

What boards have been tested with this change?

arch/arm/plat-omap/include/plat/gpmc.h struct gpmc_timings { · line 139

Quoted 9 lines

arch/arm/plat-omap/include/plat/gpmc.h +6 -0

+	u16 bus_turnaround;
+	u16 cycle2cycle_delay;
+
+	u16 wait_monitoring;
+	u16 clk_activation;
+
 	/* The following are only on OMAP3430 */
 	u16 wr_access;		/* WRACCESSTIME */
 	u16 wr_data_mux_bus;	/* WRDATAONADMUXBUS */

In general, I agree with this, but if we apply this today, it seems that we *may* be clearing these fields if they have been configured by the bootloader and hence this could introduce a regression (potentially).

So we ever need to test boards that this change impacts or at least verify that this change would not impact these boards because these fields have not been configured.

Hi Jon,

Quoted 1 line · Jon

What boards have been tested with this change?

Beagle board, after applying all 5 series of patches, without all patch series it can't be tested for beagle board as it depended on bootloader, not this API

Quoted 14 lines

arch/arm/plat-omap/include/plat/gpmc.h struct gpmc_timings { · line 139

Quoted 9 lines

arch/arm/plat-omap/include/plat/gpmc.h +6 -0

+	u16 bus_turnaround;
+	u16 cycle2cycle_delay;
+
+	u16 wait_monitoring;
+	u16 clk_activation;
+
 	/* The following are only on OMAP3430 */
 	u16 wr_access;		/* WRACCESSTIME */
 	u16 wr_data_mux_bus;	/* WRDATAONADMUXBUS */

In general, I agree with this, but if we apply this today, it seems that we *may* be clearing these fields if they have been configured by the bootloader and hence this could introduce a regression (potentially).

Quoted 3 lines

So we ever need to test boards that this change impacts or at least verify that this change would not impact these boards because these fields have not been configured.

Yes, it is the exactly the reason this patch has been splitted out of gpmc driver conversion series. To find out whether enhancing existing API cause any regression and if so, then it can be easily root caused and would not be confused with driver conversion.

This was the only change required w.r.t old interface, need to make sure that this won't causes breakage on any of the boards (the boards which were unknowingly depending on bootloader for these settings). I have only beagle board & omap3evm (both would not get affected by this change with existing code) and others can't be validated. Once this is in lo tree or mainline, this change can be validated for different boards.

Should you at least warn, if you are going to over-write a value?

Hi Jon,

Quoted 1 line · Jon

Should you at least warn, if you are going to over-write a value?

Yes, that would be better except for too much logging, if Tony prefers that way I will add.

Quoted 6 lines · Mohammed, Afzal

Hi Jon,

Quoted 1 line · Jon

Should you at least warn, if you are going to over-write a value?

Quoted 2 lines

Yes, that would be better except for too much logging, if Tony prefers that way I will add.

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

Quoted 4 lines

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

Actually this patch breaks at least DMA on tusb6010 on n8x0. That's a MUSB hardware in a wrapper connected to GPMC that's very picky with the timings.

Got any hints what should be done with the cycle2cycle stuff for tusb6010?

Quoted 5 lines · Tony Lindgren

Quoted 4 lines

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

Quoted 6 lines

Actually this patch breaks at least DMA on tusb6010 on n8x0. That's a MUSB hardware in a wrapper connected to GPMC that's very picky with the timings.

Got any hints what should be done with the cycle2cycle stuff for tusb6010?

Not as of now, let me try to find out.

Hi Tony,

Quoted 11 lines · Afzal

Quoted 9 lines · Tony Lindgren

Quoted 4 lines

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

Actually this patch breaks at least DMA on tusb6010 on n8x0. That's a MUSB hardware in a wrapper connected to GPMC that's very picky with the timings.

Quoted 4 lines

Quoted 2 lines

Got any hints what should be done with the cycle2cycle stuff for tusb6010?

Not as of now, let me try to find out.

Probably with the below patch [1], we can get values set by bootloader & calculate back value to entered in Kernel, of course that may not work if tusb6010 works with different gpmc i/p frequency.

Well I took a look at the values, and it seems the only difference is the static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.

Hi Tony,

Quoted 2 lines · Tony Lindgren

Well I took a look at the values, and it seems the only difference is the static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.

It seems change below should be part of $subject.

Please let me know your comments

Quoted 1 line · Mohammed, Afzal

It seems change below should be part of $subject.

For onenand I'm getting the following error:

omap2-onenand omap2-onenand: Cannot request GPMC CS

* Mohammed, Afzal <[email protected]> [120614 03:43]:

For onenand I'm getting the following error:

omap2-onenand omap2-onenand: Cannot request GPMC CS

Quoted 3 lines · Tony Lindgren

For onenand I'm getting the following error:

omap2-onenand omap2-onenand: Cannot request GPMC CS

I am a bit confused with this message, for onenand, we only have,

pr_err("%s: Cannot request GPMC CS\n", __func__);

dev_err is used only for nand

Hi Tony,

Quoted 11 lines · Afzal

Quoted 3 lines · Tony Lindgren

For onenand I'm getting the following error:

omap2-onenand omap2-onenand: Cannot request GPMC CS

I am a bit confused with this message, for onenand, we only have,

pr_err("%s: Cannot request GPMC CS\n", __func__);

dev_err is used only for nand

This error can happen if my patches are not present,

dev_err for onenand driver was removed by,

[mtd: onenand: omap2: obtain memory from resource],

seems, mtd patches are not in your present branch

Hi Tony,

Quoted 2 lines · Tony Lindgren

Well I took a look at the values, and it seems the only difference is the static GPMC_CONFIG1_CLKACTIVATIONTIME(1) that your patch now overwrites 0.

It seems change below should be part of $subject.

Please let me know your comments

Regards
Afzal

arch/arm/mach-omap2/gpmc-onenand.c +0 -0

index fd4c48d..938896c 100644

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 325

Quoted 2 lines · Mohammed, Afzal

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

+       t.clk_activation = fclk_offset_ns;
+

This too should be fclk_offset, not fclk_offset_ns.

Hi Tony,

Quoted 5 lines · Tony Lindgren

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 325

Quoted 2 lines · Mohammed, Afzal

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

+       t.clk_activation = fclk_offset_ns;
+

This too should be fclk_offset, not fclk_offset_ns.

As gpmc_cs_set_timing convert it to ticks from ns, shouldn't we put it in ns ?

Quoted 9 lines · Mohammed, Afzal

Quoted 5 lines · Tony Lindgren

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 325

Quoted 2 lines · Mohammed, Afzal

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

+       t.clk_activation = fclk_offset_ns;
+

This too should be fclk_offset, not fclk_offset_ns.

As gpmc_cs_set_timing convert it to ticks from ns, shouldn't we put it in ns ?

Hmm I see, something's wrong though.. Some of these we really want to specify as ticks instead of ns. Your patch was using 1 tick value as 1 ns value, which won't work. When I changed it back to what I thought was ticks, it just happened to work probably because of rounding. That's probably the reason why some of these have been set directly with gpmc_cs_write_reg as that sets tick values directly.

Hi Tony,

Quoted 11 lines · Tony Lindgren

Quoted 9 lines · Mohammed, Afzal

Quoted 5 lines · Tony Lindgren

arch/arm/mach-omap2/gpmc-onenand.c static int omap2_onenand_set_sync_mode(struct omap_onenand_platform_data *cfg, · line 325

Quoted 2 lines · Mohammed, Afzal

arch/arm/mach-omap2/gpmc-onenand.c +2 -0

+       t.clk_activation = fclk_offset_ns;
+

This too should be fclk_offset, not fclk_offset_ns.

As gpmc_cs_set_timing convert it to ticks from ns, shouldn't we put it in ns ?

Quoted 7 lines

Hmm I see, something's wrong though.. Some of these we really want to specify as ticks instead of ns. Your patch was using 1 tick value as 1 ns value, which won't work. When I changed it back to what I thought was ticks, it just happened to work probably because of rounding. That's probably the reason why some of these have been set directly with gpmc_cs_write_reg as that sets tick values directly.

For onenand,

fclk_offset = gpmc_ns_to_ticks(fclk_offset_ns);

By using directly fclk_offset_ns, we transfer job of doing the same to gpmc_cs_set_timings (which does gpmc_ns_to_ticks over fclk_offset_ns & then do << 25)

Hence passing fclk_offset_ns in effect should be same as,

GPMC_CONFIG1_CLKACTIVATIONTIME(fclk_offset)

If onenand is not working with it, then there is something that is invisible here.

Were you able to get it working with the changed value for usb or onenand ?

In case possible, can you check enabling DEBUG and see what timings are logged

Sounds like the tusb6010 non-ns tick value is the only remaining issue.

But I am unable to find reason for failure upon using gpmc_ticks_to_ns(1), which seems to me right thing to be used. Let me try to invoke tusb6010 functions in beagle board, observe timings so that at least I will get an idea what is going on here (even though it is guaranteed to crash)

Hi Tony,

Quoted 5 lines · Afzal

But I am unable to find reason for failure upon using gpmc_ticks_to_ns(1), which seems to me right thing to be used. Let me try to invoke tusb6010 functions in beagle board, observe timings so that at least I will get an idea what is going on here (even though it is guaranteed to crash)

Checked simulating on beagle board, I am at total loss to understand why using gpmc_ticks_to_ns(1) has failed for n8x0

clk_activation timings with both values as follows,

[1] With t.clk_activation = gpmc_ticks_to_ns(1);

GPMC CS4: clk_activation: 1 ticks, 6 ns (was 0 ticks) 6 ns

[2] With t.clk_activation = 1;

GPMC CS4: clk_activation: 1 ticks, 6 ns (was 0 ticks) 1 ns

Last field show in ns the time we are trying to set, and for both cases, 1 ticks are being programmed in register.

something yesterday when manually patching the clk_activation, maybe I put the clk_activation value into async timings instead as I was seeing the tick value set to 0 for the sync mode.

Quoted 2 lines · Mohammed, Afzal

Yes, that would be better except for too much logging, if Tony prefers that way I will add.

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

Hi Tony,

Quoted 4 lines · Tony Lindgren

Quoted 2 lines · Mohammed, Afzal

Yes, that would be better except for too much logging, if Tony prefers that way I will add.

Quoted 4 lines

If there's a chance it causing file system corruption, we should test it carefully on the boards before applying. If that's done, then there's probably no need for warnings. It's safer to disable NAND for untested boards if there's a chance of breaking the timings.

By disabling NAND, I understand that you are suggesting to remove nand initialization done in board file, right ?

And boards that can be tested here are omap3evm & beagleboard, both of which can't be tested for this change.

Or should additional timings be achieved without affecting old interface, but that it seems would necessitate more code duplication.

Quoted 3 lines

Or should additional timings be achieved without affecting old interface, but that it seems would necessitate more code duplication.

We should just keep the same timings as before, with values added for the newly added registers.

Hi Tony,

Quoted 4 lines · Tony Lindgren

Quoted 3 lines

Or should additional timings be achieved without affecting old interface, but that it seems would necessitate more code duplication.

Quoted 2 lines

We should just keep the same timings as before, with values added for the newly added registers.

What I understood from above is to get values bootloader used to set for those new timings & put in Kernel, right ?

Configure busturnaround, cycle2cycledelay, waitmonitoringtime, clkactivationtime in gpmc_cs_set_timings(). This is done so that boards can configure these parameters of gpmc in Kernel instead of relying on bootloader.

Signed-off-by Afzal Mohammed <[email protected]>

---
 arch/arm/mach-omap2/gpmc.c             |    6 ++++++
 arch/arm/plat-omap/include/plat/gpmc.h |    6 ++++++
 2 files changed, 12 insertions(+)

arch/arm/mach-omap2/gpmc.c +0 -0

index 578fd4c..517953f 100644

arch/arm/mach-omap2/gpmc.c int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t) · line 321

Quoted 6 lines · Afzal Mohammed

arch/arm/mach-omap2/gpmc.c +6 -0

+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
+
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
+

Thinking about this, the CONFIG1 bits have been set with gpmc_cs_write_reg because these are part of the static configuration and do not need to be dynamically calculated as they are tick based. For example, tusb6010 sets GPMC_CONFIG1_CLKACTIVATIONTIME(1) during init.

Writing these over and over again during DVFS does not make sense, they should be only initialized once.

Regards,

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Tony,

Quoted 13 lines · Tony Lindgren

arch/arm/mach-omap2/gpmc.c int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t) · line 321

Quoted 6 lines · Afzal Mohammed

arch/arm/mach-omap2/gpmc.c +6 -0

+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
+	GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
+
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
+	GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
+

Thinking about this, the CONFIG1 bits have been set with gpmc_cs_write_reg because these are part of the static configuration and do not need to be dynamically calculated as they are tick based. For example, tusb6010 sets GPMC_CONFIG1_CLKACTIVATIONTIME(1) during init.

But aren't we deciding number of ticks based on clock period ? If we take case of onenand, based on the knowledge of clock period, number of ticks are calculated.

And similarly to decide cycle2cycledelay, busturnaround, we decide number of ticks based on peripheral datasheet timings & gpmc clock, hence shouldn't it also be dynamically calculated similar to timings that were existing earlier.

Objective of this series is to make things easy for GPMC driver conversion series by separating out more things from driver conversion series.

This series,
1. Unifies NAND platform initialization functions
2. Cleans up OneNAND platform code a little
3. Handles additional timings in Kernel

This series is based on 3.5-rc1 & made on top of
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69501.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html

Hi Tony,

Quoted 9 lines · Afzal

Objective of this series is to make things easy for GPMC driver conversion series by separating out more things from driver conversion series.

This series,
1. Unifies NAND platform initialization functions
2. Cleans up OneNAND platform code a little
3. Handles additional timings in Kernel

Quoted 3 lines

This series is based on 3.5-rc1 & made on top of
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69501.html
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69881.html

Please let me know your comments on this series.

gpmc driver conversion series [1] is dependent on this.

Regards
Afzal

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg69897.html

NAND for untested boards if timings change. Are Jon's comments all handled?

Hi Tony,

Quoted 1 line · Tony Lindgren

NAND for untested boards if timings change. Are Jon's comments all handled?

I have explained the justification, why those changes were done so, waiting for his comments.

Afzal, let me know if you have been able to test. I have a omap2430 with onenand I could try.