GPMC driver conversion

gpmc driver platform definitions

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

---
 arch/arm/plat-omap/include/plat/gpmc.h |   49 ++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

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

index 802fb22..21a8cce 100644
@@ -139,6 +139,55 @@ struct gpmc_timings {
 	u16 wr_data_mux_bus;	/* WRDATAONADMUXBUS */
 };
 
+/* bool type time settings */
+struct gpmc_misc_timings {
+	bool cycle2cyclediffcsen;
+	bool cycle2cyclesamecsen;
+	bool we_extra_delay;
+	bool oe_extra_delay;
+	bool adv_extra_delay;
+	bool cs_extra_delay;
+	bool time_para_granularity;
+};
+
+enum {

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

+struct gpmc_cs_data {
+	unsigned		cs;
+	unsigned long		mem_size;
+	unsigned long		mem_offset;
+	bool			have_config;
+	unsigned		config;
+	struct gpmc_time_ctrl	time_ctrl;
+	unsigned		irq_config;
+};
+
+struct gpmc_device_pdata {
+	char			*name;
+	int			id;
+	void			*pdata;
+	unsigned		pdata_size;
+	struct resource		*per_res;
+	unsigned		per_res_cnt;
+	struct gpmc_cs_data	*cs_data;
+	unsigned		num_cs;
+};
+
+struct gpmc_pdata {
+	unsigned			num_device;
+	struct gpmc_device_pdata	**device_pdata;
+};
+
 struct gpmc_nand_regs {
 	void __iomem	*gpmc_status;
 	void __iomem	*gpmc_nand_command;

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

Quoted 11 lines · Afzal Mohammed

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

+enum {
+	has_none,
+	has_period,
+	has_clock
+};
+
+struct gpmc_time_ctrl {
+	int type;
+	struct gpmc_timings timings;
+	struct gpmc_misc_timings bool_timings;
+};

Why not combine misc_timings and the original timings? I don't see why these need to be kept separate. Even if combined it can still be

backward compatible for legacy boards as they will not specify the misc timing fields. I am also not convinced we need this type member.

Create API for platforms to adapt gpmc to HWMOD

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

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

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

index 517953f..b471c2f 100644
@@ -27,6 +27,7 @@
 
 #include <asm/mach-types.h>
 #include <plat/gpmc.h>
+#include <plat/omap_device.h>
 
 #include <plat/sdrc.h>
 

@@ -898,6 +899,36 @@ static int __init gpmc_init(void)
 }
 postcore_initcall(gpmc_init);
 
+__init int omap_gpmc_init(struct gpmc_pdata *pdata)
+{
+	struct omap_hwmod *oh;
+	struct platform_device *pdev;
+	char *name = "omap-gpmc";
+	char *oh_name = "gpmc";
+
+	oh = omap_hwmod_lookup(oh_name);
+	if (!oh) {
+		pr_err("Could not look up %s\n", oh_name);
+		return -ENODEV;
+	}
+
+	pdev = omap_device_build(name, -1, oh, pdata,
+					sizeof(*pdata), NULL, 0, 0);
+	if (IS_ERR(pdev)) {

Quoted 9 lines

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

+	pdev = omap_device_build(name, -1, oh, pdata,
+					sizeof(*pdata), NULL, 0, 0);
+	if (IS_ERR(pdev)) {
+		WARN(1, "Can't build omap_device for %s:%s.\n",
+						name, oh->name);
+		return PTR_ERR(pdev);
+	}
+
+	gpmc_l3_clk = clk_get(NULL, oh->main_clk);

arch/arm/mach-omap2/gpmc.c static int __init gpmc_init(void) · line 927

Quoted 4 lines

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

+	if (IS_ERR(gpmc_l3_clk)) {
+		pr_err("Could not get GPMC clock\n");
+		return PTR_ERR(gpmc_l3_clk);
+	}

My preference would be to store gpmc_l3_clk in the pdata and pass to probe via the pdata. The aim would be to remove the global gpmc_l3_clk altogether.

Also we should attempt to get the clk before calling omap_device_build which is registering the driver.

Hi Jon,

Quoted 9 lines · Jon

Quoted 9 lines

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

+	pdev = omap_device_build(name, -1, oh, pdata,
+					sizeof(*pdata), NULL, 0, 0);
+	if (IS_ERR(pdev)) {
+		WARN(1, "Can't build omap_device for %s:%s.\n",
+						name, oh->name);
+		return PTR_ERR(pdev);
+	}
+
+	gpmc_l3_clk = clk_get(NULL, oh->main_clk);

Quoted 8 lines

arch/arm/mach-omap2/gpmc.c static int __init gpmc_init(void) · line 927

Quoted 4 lines

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

+	if (IS_ERR(gpmc_l3_clk)) {
+		pr_err("Could not get GPMC clock\n");
+		return PTR_ERR(gpmc_l3_clk);
+	}

My preference would be to store gpmc_l3_clk in the pdata and pass to probe via the pdata. The aim would be to remove the global gpmc_l3_clk altogether.

For timing calculation by platform outside of driver, we need clk rate

Quoted 2 lines

Also we should attempt to get the clk before calling omap_device_build which is registering the driver.

omap_device_build registers only device, but I agree that clk should be obtained before it as driver can get probed at that point

Quoted 7 lines · Afzal

Quoted 3 lines · Jon

My preference would be to store gpmc_l3_clk in the pdata and pass to probe via the pdata. The aim would be to remove the global gpmc_l3_clk altogether.

For timing calculation by platform outside of driver, we need clk rate

Right but potentially, this could be done by the driver.

Hi Jon,

Quoted 10 lines · Jon

Quoted 7 lines · Afzal

Quoted 3 lines · Jon

My preference would be to store gpmc_l3_clk in the pdata and pass to probe via the pdata. The aim would be to remove the global gpmc_l3_clk altogether.

For timing calculation by platform outside of driver, we need clk rate

Right but potentially, this could be done by the driver.

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Quoted 11 lines · Mohammed, Afzal

Hi Jon,

Quoted 1 line · Jon

Right but potentially, this could be done by the driver.

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

Quoted 3 lines

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Yeah I also think the GPMC code should handle the L3 timings, and dynamically calculate them for DVFS when L3 frequency changes. Does the GPMC have enough information now to do that?

Additionally GPMC should add constraints to DVFS if not enough data is know to scale L3.

Tony

Hi Tony,

Quoted 12 lines · Tony Lindgren

Quoted 11 lines · Mohammed, Afzal

Hi Jon,

Quoted 1 line · Jon

Right but potentially, this could be done by the driver.

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

Quoted 7 lines

Quoted 3 lines

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Yeah I also think the GPMC code should handle the L3 timings, and dynamically calculate them for DVFS when L3 frequency changes. Does the GPMC have enough information now to do that?

Do you mean that gpmc driver should have the capability to calculate peripheral timings at runtime based on frequency ?, I am not sure how this can be handled by gpmc driver as calculation for different peripherals are done in different way, requiring gpmc driver to know about connected peripheral, that would imply that gpmc driver would not be peripheral agnostic.

Or else some sort of a callback to be used ?

Out of the 20,14 are depending on bootloader, both omap3evm & beagle has been converted to utilize runtime calculation, but for other 12 boards, first we need to get values used by bootloader (those include peripherals that doesn't have gpmc-* helpers), then derive it based on expression, after that only, we will have information to achieve it and those are the ones that I do not have access to.

Quoted 8 lines · Mohammed, Afzal

On Wed, Jun 13, 2012 at 17:32:09, Tony Lindgren wrote:

Do you mean that gpmc driver should have the capability to calculate peripheral timings at runtime based on frequency ?, I am not sure how this can be handled by gpmc driver as calculation for different peripherals are done in different way, requiring gpmc driver to know about connected peripheral, that would imply that gpmc driver would not be peripheral agnostic.

Quoted 1 line

Or else some sort of a callback to be used ?

Oops yeah right, we have some platform_retime functions that are in the driver platform init code. It would be best that the drivers can do the recalculation with no platform init code callbacks needed.

Quoted 7 lines

Out of the 20,14 are depending on bootloader, both omap3evm & beagle has been converted to utilize runtime calculation, but for other 12 boards, first we need to get values used by bootloader (those include peripherals that doesn't have gpmc-* helpers), then derive it based on expression, after that only, we will have information to achieve it and those are the ones that I do not have access to.

It's OK to use fixed timings as long as we disable L3 scaling. Some of these values we'll probably never be able to calculate dynamically.

Hi Tony,

Quoted 12 lines · Tony Lindgren

Quoted 11 lines · Mohammed, Afzal

Hi Jon,

Quoted 1 line · Jon

Right but potentially, this could be done by the driver.

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

Quoted 7 lines

Quoted 3 lines

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Yeah I also think the GPMC code should handle the L3 timings, and dynamically calculate them for DVFS when L3 frequency changes. Does the GPMC have enough information now to do that?

Do you mean that gpmc driver should have the capability to calculate peripheral timings at runtime based on frequency ?, I am not sure how this can be handled by gpmc driver as calculation for different peripherals are done in different way, requiring gpmc driver to know about connected peripheral, that would imply that gpmc driver would not be peripheral agnostic.

Quoted 5 lines · Afzal

Do you mean that gpmc driver should have the capability to calculate peripheral timings at runtime based on frequency ?, I am not sure how this can be handled by gpmc driver as calculation for different peripherals are done in different way, requiring gpmc driver to know about connected peripheral, that would imply that gpmc driver would not be peripheral agnostic.

I guess I still don't agree with this. From the gpmc timing point of view it should not care what device is connected, it just needs to know the associated timings. Therefore, the gpmc driver just needs the time associated with the different timings fields in the various GPMC_CONFIGx registers and then convert to clocks based upon the gpmc fclk. The only item that is device specific here is the actual timing values and these can be passed to the driver.

Furthermore, gpmc_cs_set_timings function is completely device agnostic. Why can we not call this from within the driver? Why does it need to be called outside the driver?

Hi Jon,

Quoted 10 lines · Jon

Quoted 7 lines · Afzal

Quoted 3 lines · Jon

My preference would be to store gpmc_l3_clk in the pdata and pass to probe via the pdata. The aim would be to remove the global gpmc_l3_clk altogether.

For timing calculation by platform outside of driver, we need clk rate

Right but potentially, this could be done by the driver.

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Quoted 8 lines

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

So I see that the various gpmc-*.c files have some form of _retime() function. However, at the end of the day they all call gpmc_cs_set_timings() to convert time into gpmc clocks. Converting time to gpmc clocks is completely independent of the actual device and so this can be performed by the driver. We just need to populate the gpmc_timings struct and pass to the driver to convert to clocks and program into the registers.

gpmc_cs_set_timings() does currently convert time to clock cycles required, and this gpmc driver have the capability to do it.

What I was saying is a different issue, input to gpmc_cs_set_timings, which is time sometimes in turn is a function of time or to be exact depends on gpmc clock period also. So timings provided to gpmc_cs_set_timings for a particular frequency may not hold good for another frequency, unless we change the input time to gpmc_cs_set_timings based on gpmc clock.

If you see gpmc-* files, many a times, they need to know value of gpmc fclk, to calculate the input time to be fed for gpmc_cs_set_timings

Hi Jon,

Quoted 9 lines · Jon

Quoted 8 lines

I do not think it is practically possible. Please see timing calculations in arch/arm/mach-omap2/gpmc-*, the way it is done for different peripherals are different, and we cannot expect gpmc driver to do those as that would require gpmc driver being aware of type of peripheral connected.

And all those gpmc-* timing calculation needs to be done before driver is ready, they rely on functions like gpmc_get_fclk_rate(), which in turn requires the clk rate to be available before driver is probed.

Quoted 7 lines

So I see that the various gpmc-*.c files have some form of _retime() function. However, at the end of the day they all call gpmc_cs_set_timings() to convert time into gpmc clocks. Converting time to gpmc clocks is completely independent of the actual device and so this can be performed by the driver. We just need to populate the gpmc_timings struct and pass to the driver to convert to clocks and program into the registers.

gpmc_cs_set_timings() does currently convert time to clock cycles required, and this gpmc driver have the capability to do it.

What I was saying is a different issue, input to gpmc_cs_set_timings, which is time sometimes in turn is a function of time or to be exact depends on gpmc clock period also. So timings provided to gpmc_cs_set_timings for a particular frequency may not hold good for another frequency, unless we change the input time to gpmc_cs_set_timings based on gpmc clock.

If you see gpmc-* files, many a times, they need to know value of gpmc fclk, to calculate the input time to be fed for gpmc_cs_set_timings

What does make this a bit more difficult is the function gpmc_round_ns_to_ticks(). It appears to convert nanoseconds to ticks and back to nanoseconds. I am guessing to account for some rounding error. I am curious what impact this function is having on the timing. Ideally this should be handle in gpmc_cs_set_timings().

Quoted 5 lines · Jon Hunter

What does make this a bit more difficult is the function gpmc_round_ns_to_ticks(). It appears to convert nanoseconds to ticks and back to nanoseconds. I am guessing to account for some rounding error. I am curious what impact this function is having on the timing. Ideally this should be handle in gpmc_cs_set_timings().

I don't have the code in front of me at the moment, nor my old GPMC timing spreadsheet, so this comment may be superfluous. But since kernel arithmetic needs to be in integers, it might be necessary to do some of the timing calculations in picosecond units to avoid roundoff issues.

Quoted 8 lines · Afzal

gpmc_cs_set_timings() does currently convert time to clock cycles required, and this gpmc driver have the capability to do it.

What I was saying is a different issue, input to gpmc_cs_set_timings, which is time sometimes in turn is a function of time or to be exact depends on gpmc clock period also. So timings provided to gpmc_cs_set_timings for a particular frequency may not hold good for another frequency, unless we change the input time to gpmc_cs_set_timings based on gpmc clock.

Absolutely! No argument there.

Quoted 2 lines

If you see gpmc-* files, many a times, they need to know value of gpmc fclk, to calculate the input time to be fed for gpmc_cs_set_timings

I understand, but that is not my point. My point is that timings should be provided in nanoseconds for all devices. Then the gpmc driver can query the gpmc fclk and convert to gpmc cycles. This way the gpmc driver will not care what device is connected in terms of the timings and can convert them.

If the clk handle for the gpmc is passed to the gpmc driver, then there is no reason why the driver cannot do this.

Hi Jon,

Quoted 2 lines · Jon

If the clk handle for the gpmc is passed to the gpmc driver, then there is no reason why the driver cannot do this.

I believe passing clk details through platform data is an abuse of clock framework.

Quoted 5 lines · Afzal

Quoted 2 lines · Jon

If the clk handle for the gpmc is passed to the gpmc driver, then there is no reason why the driver cannot do this.

Quoted 2 lines

I believe passing clk details through platform data is an abuse of clock framework.

Why? You currently have a global variable storing the clock handle. It can be quite common for drivers to know the clock frequencies of their functional clocks. How else can drivers calculate timings?

Hi Jon,

Quoted 6 lines · Jon

Quoted 5 lines · Afzal

Quoted 2 lines · Jon

If the clk handle for the gpmc is passed to the gpmc driver, then there is no reason why the driver cannot do this.

Quoted 6 lines

Quoted 2 lines

I believe passing clk details through platform data is an abuse of clock framework.

Why? You currently have a global variable storing the clock handle. It can be quite common for drivers to know the clock frequencies of their functional clocks. How else can drivers calculate timings?

Please see Russell King's comments,

[1] http://permalink.gmane.org/gmane.linux.ports.ppc.embedded/27634
[2] http://www.mail-archive.com/[email protected]/msg05365.html

Quoted 11 lines · Afzal

Quoted 3 lines · Jon

Why? You currently have a global variable storing the clock handle. It can be quite common for drivers to know the clock frequencies of their functional clocks. How else can drivers calculate timings?

Please see Russell King's comments,

[1] http://permalink.gmane.org/gmane.linux.ports.ppc.embedded/27634
[2] http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg05365.html

Thanks. So I still think you need to get rid of the global variable for storing the gpmc fclk, that is really my point.

So if you look at commit [1] mentioned by Russell in the above thread, the appropriate thing to do would be to create a gpmc clock alias for all OMAP2+ devices and then you could simply call the following from the gpmc probe ...

	gpmc_fck = clk_get(&pdev->dev, "fck");

You could then store somewhere in one of the gpmc structures.

Hi Jon,

Quoted 21 lines · Jon

Quoted 11 lines · Afzal

Quoted 3 lines · Jon

Why? You currently have a global variable storing the clock handle. It can be quite common for drivers to know the clock frequencies of their functional clocks. How else can drivers calculate timings?

Please see Russell King's comments,

[1] http://permalink.gmane.org/gmane.linux.ports.ppc.embedded/27634
[2] http://www.mail-archive.com/linux-samsung-soc at vger.kernel.org/msg05365.html

Thanks. So I still think you need to get rid of the global variable for storing the gpmc fclk, that is really my point.

So if you look at commit [1] mentioned by Russell in the above thread, the appropriate thing to do would be to create a gpmc clock alias for all OMAP2+ devices and then you could simply call the following from the gpmc probe ...

Quoted 3 lines

	gpmc_fck = clk_get(&pdev->dev, "fck");

You could then store somewhere in one of the gpmc structures.

Here clock is required even before driver is probed, i.e for platform to calculate timings, that has to be passed through platform data.

I understand the necessity for clk rate information in driver, but seems unless we have a generic way to scale timings for all the kinds of peripheral, having it may not be of much help.

Quoted 2 lines · Mohammed, Afzal

Here clock is required even before driver is probed, i.e for platform to calculate timings, that has to be passed through platform data.

Eventually we should be able to move the gpmc registration to the driver probe, especially with device tree. There's no need to set up gpmc before the driver probe runs for the device using gpmc. Just how the gpmc init gets called from the driver probe is still a bit open though..

It may require some bus level hooks, or wrapper drivers for the generic device drivers like smsc911x.

Quoted 4 lines

I understand the necessity for clk rate information in driver, but seems unless we have a generic way to scale timings for all the kinds of peripheral, having it may not be of much help.

We should not need to pass clock handles around. It's better to export some helper functions in the gpmc code for the calculation.

Hi Tony,

Quoted 5 lines · Tony Lindgren

Quoted 2 lines · Mohammed, Afzal

Here clock is required even before driver is probed, i.e for platform to calculate timings, that has to be passed through platform data.

Quoted 4 lines

Eventually we should be able to move the gpmc registration to the driver probe, especially with device tree. There's no need to set up gpmc before the driver probe runs for the device using gpmc. Just how the gpmc init gets called from the driver probe is still a bit open though..

Sorry, I did not get you, can you please clarify

By gpmc registration, if you meant registering platform device for gpmc peripherals, for a board that uses the new gpmc driver interface*, it will be done in probe only.

All the responsibilities of old gpmc init is now taken care by probe; probe in addition does setting up gpmc for each peripheral, registering platform device (if the board makes use of new driver interface). If a board uses new gpmc driver interface, gpmc for that device is not setup before gpmc probe.

Quoted 2 lines

It may require some bus level hooks, or wrapper drivers for the generic device drivers like smsc911x.

This too, not sure whether I follow you

Quoted 6 lines

Quoted 3 lines

I understand the necessity for clk rate information in driver, but seems unless we have a generic way to scale timings for all the kinds of peripheral, having it may not be of much help.

We should not need to pass clock handles around. It's better to export some helper functions in the gpmc code for the calculation.

Currently we have helper function in gpmc.c for the same, were you referring those ?

*: [1] converts omap3evm & beagle board to use new interface, in addition
to [1], similar to it, one more series would be posted to convert remaining boards also to use the new gpmc driver interface. As these cannot be tested by me and as it depends on final shape of this basic driver conversion series (or the present series), they have not yet been converted

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

Quoted 5 lines · Mohammed, Afzal

On Fri, Jun 15, 2012 at 18:15:20, Tony Lindgren wrote:

By gpmc registration, if you meant registering platform device for gpmc peripherals, for a board that uses the new gpmc driver interface*, it will be done in probe only.

I was thinking when the gpmc needs to be initialized, and there should not be any need to do it earlier than at the gpmc using driver probe time. With device tree that is, as there's no need to stuff the gpmc timings into a board-*.c file.

Quoted 4 lines

Quoted 2 lines

It may require some bus level hooks, or wrapper drivers for the generic device drivers like smsc911x.

This too, not sure whether I follow you

Well smsc911x has device tree binding, and is a generic driver. How do we trigger the gpmc initialization from a generic driver probe?

Quoted 5 lines

Quoted 2 lines

We should not need to pass clock handles around. It's better to export some helper functions in the gpmc code for the calculation.

Currently we have helper function in gpmc.c for the same, were you referring those ?

Yes something that let's the driver call gpmc code to do the calculation. The other option would be to just add gpmc clock as a clock fwk node, and then the driver could clk_get() it as ick.

Hi Tony,

Quoted 11 lines · Tony Lindgren

Quoted 5 lines · Mohammed, Afzal

On Fri, Jun 15, 2012 at 18:15:20, Tony Lindgren wrote:

By gpmc registration, if you meant registering platform device for gpmc peripherals, for a board that uses the new gpmc driver interface*, it will be done in probe only.

I was thinking when the gpmc needs to be initialized, and there should not be any need to do it earlier than at the gpmc using driver probe time. With device tree that is, as there's no need to stuff the gpmc timings into a board-*.c file.

I believe by "gpmc needs to be initialized", you meant calculating gpmc timings, determining configuration, the things that are done in functions like gpmc_smsc911x_update etc. as in [1] and not initializing gpmc at hardware level. With the above assumption, I feel we need to have a way first to generalize gpmc timing calculation for different peripherals as suggested by Jon as well as have logic to handle timings that depends on cycles too.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

It may require some bus level hooks, or wrapper drivers for the generic device drivers like smsc911x.

This too, not sure whether I follow you

Quoted 2 lines

Well smsc911x has device tree binding, and is a generic driver. How do we trigger the gpmc initialization from a generic driver probe?

Not sure whether device tree have capability to represent something like child devices, if non bus devices can have child devices, then we can have peripherals connected to gpmc as childs, but may be this will remain only as a dream; I need to get into DT to find things out

Quoted 6 lines

Quoted 5 lines

Quoted 2 lines

We should not need to pass clock handles around. It's better to export some helper functions in the gpmc code for the calculation.

Currently we have helper function in gpmc.c for the same, were you referring those ?

Quoted 3 lines

Yes something that let's the driver call gpmc code to do the calculation. The other option would be to just add gpmc clock as a clock fwk node, and then the driver could clk_get() it as ick.

For gpmc driver to calculate timings rather than platform code doing it, we first need to have a generalized way to calculate gpmc timings for all peripherals as well as have a logic to calculate timings based on time & cycles, correct ? (to make sure we are talking the same thing)

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

Quoted 15 lines · Mohammed, Afzal

Hi Tony,

Quoted 11 lines · Tony Lindgren

Quoted 5 lines · Mohammed, Afzal

On Fri, Jun 15, 2012 at 18:15:20, Tony Lindgren wrote:

By gpmc registration, if you meant registering platform device for gpmc peripherals, for a board that uses the new gpmc driver interface*, it will be done in probe only.

I was thinking when the gpmc needs to be initialized, and there should not be any need to do it earlier than at the gpmc using driver probe time. With device tree that is, as there's no need to stuff the gpmc timings into a board-*.c file.

Quoted 7 lines

I believe by "gpmc needs to be initialized", you meant calculating gpmc timings, determining configuration, the things that are done in functions like gpmc_smsc911x_update etc. as in [1] and not initializing gpmc at hardware level. With the above assumption, I feel we need to have a way first to generalize gpmc timing calculation for different peripherals as suggested by Jon as well as have logic to handle timings that depends on cycles too.

Yup. We'll be only getting the timings from device tree. Ideally the gpmc code would just do all the calculations, but it sounds like device timings may require some calculation on the gpmc consumer driver side too.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

It may require some bus level hooks, or wrapper drivers for the generic device drivers like smsc911x.

This too, not sure whether I follow you

Quoted 7 lines

Quoted 2 lines

Well smsc911x has device tree binding, and is a generic driver. How do we trigger the gpmc initialization from a generic driver probe?

Not sure whether device tree have capability to represent something like child devices, if non bus devices can have child devices, then we can have peripherals connected to gpmc as childs, but may be this will remain only as a dream; I need to get into DT to find things out

Yes the that's there set up automatically. But for the timings we could have something like this to associate the following invented gpmc timings with smsc911x:

/* in omap[234].dtsi */

gpmc: gpmc@480121234 {
	compatible = "ti,gpmc";
	reg = <0x480121234 0x1234>;
	#address-cells = <0>;
	#size-cells = <0>;
};

...

/* in some board *.dts file */

&gpmc {
	gpmc_cs1: gpmc_cs1_timings {
		...
	};
};

net@48001234 {
	compatible = "smsc,lan91c111";
	reg = <0x48001234 0x10000>;
	interrupts = <12>;
	gpmc_timings = <&gpmc_cs1>;
};

Quoted 10 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

We should not need to pass clock handles around. It's better to export some helper functions in the gpmc code for the calculation.

Currently we have helper function in gpmc.c for the same, were you referring those ?

Yes something that let's the driver call gpmc code to do the calculation. The other option would be to just add gpmc clock as a clock fwk node, and then the driver could clk_get() it as ick.

Quoted 4 lines

For gpmc driver to calculate timings rather than platform code doing it, we first need to have a generalized way to calculate gpmc timings for all peripherals as well as have a logic to calculate timings based on time & cycles, correct ? (to make sure we are talking the same thing)

Yes that might be tricky..

Quoted 1 line · Tony Lindgren

* Mohammed, Afzal <[email protected]> [120620 07:57]:

Therefore, I am wondering if Afzal's driver needs to register the gpmc devices outside of the gpmc_probe() and add the devices as children. Or maybe we tackle that when we migrate it to DT.

Quoted 6 lines

Quoted 4 lines

For gpmc driver to calculate timings rather than platform code doing it, we first need to have a generalized way to calculate gpmc timings for all peripherals as well as have a logic to calculate timings based on time & cycles, correct ? (to make sure we are talking the same thing)

Yes that might be tricky..

I am wondering more and more if we need to do a clean-up of the timing calculations prior to driver migration ...

A driver is being created out of GPMC code. This is being attempted to acheive by not breaking existing interface, necessitating requirement of GPMC peripherals being able to work with as well as without the help of driver. To not break existing, initcall is required as in old interface GPMC is configured at arch initcall and GPMC should be ready to handle old interface by that time

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

---
 arch/arm/mach-omap2/gpmc.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

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

index b471c2f..6dbddb9 100644
@@ -902,7 +902,7 @@ postcore_initcall(gpmc_init);
 __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 {
 	struct omap_hwmod *oh;
-	struct platform_device *pdev;
+	static struct platform_device *pdev;
 	char *name = "omap-gpmc";
 	char *oh_name = "gpmc";
 

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

 	pdev = omap_device_build(name, -1, oh, pdata,
 					sizeof(*pdata), NULL, 0, 0);
 	if (IS_ERR(pdev)) {

@@ -929,6 +935,17 @@ __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 	return 0;
 }
 
+static int __init gpmc_pre_init(void)

Quoted 12 lines

__init int omap_gpmc_init(struct gpmc_pdata *pdata) {

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

 	struct omap_hwmod *oh;
-	struct platform_device *pdev;
+	static struct platform_device *pdev;
 	char *name = "omap-gpmc";
 	char *oh_name = "gpmc";

@@ -912,6 +912,12 @@ __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 		return -ENODEV;
 	}
 

arch/arm/mach-omap2/gpmc.c __init int omap_gpmc_init(struct gpmc_pdata *pdata) · line 920

Quoted 6 lines

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

+	if (pdev != NULL) {
+		clk_put(gpmc_l3_clk);
+		omap_device_delete(pdev->archdata.od);
+		platform_device_unregister(pdev);
+	}
+

I am not sure if I am missing something, but it appears that pdev will always be NULL here as it is a local uninitialised variable.

+static int __init gpmc_pre_init(void)

arch/arm/mach-omap2/gpmc.c __init int omap_gpmc_init(struct gpmc_pdata *pdata) · line 948

Quoted 10 lines

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

+{
+	static struct gpmc_device_pdata *gpmc_device_data[1];
+	struct gpmc_pdata gpmc_data = {
+		.device_pdata	= gpmc_device_data,
+	};
+
+	return omap_gpmc_init(&gpmc_data);
+}
+postcore_initcall(gpmc_pre_init);
+

Not sure I see the point in the above function. Why not declare the gpmc_device_data struct as static in the file and access it directly instead of passing it in omap_gpmc_init(). The postcore_init can then call omap_gpmc_init() directly.

Shouldn't the post_initcall be added in patch #4, when the driver is created?

Hi Jon,

This change is required only till driver migration of all platforms are done, after it, this hackish patch has to be reverted. This has been done so that existing interface will work for each patch of this series as well as till all boards are migrated.

Quoted 12 lines · Jon

Quoted 12 lines

__init int omap_gpmc_init(struct gpmc_pdata *pdata) {

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

 	struct omap_hwmod *oh;
-	struct platform_device *pdev;
+	static struct platform_device *pdev;
 	char *name = "omap-gpmc";
 	char *oh_name = "gpmc";

@@ -912,6 +912,12 @@ __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 		return -ENODEV;
 	}
 

Quoted 9 lines

arch/arm/mach-omap2/gpmc.c __init int omap_gpmc_init(struct gpmc_pdata *pdata) · line 920

Quoted 6 lines

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

+	if (pdev != NULL) {
+		clk_put(gpmc_l3_clk);
+		omap_device_delete(pdev->archdata.od);
+		platform_device_unregister(pdev);
+	}
+

I am not sure if I am missing something, but it appears that pdev will always be NULL here as it is a local uninitialised variable.

omap_gpmc_init will be called by board files once again, at that time, existing omap device will be destroyed (resulting in driver remove being executed, and inverse of omap_device_build is not available, hence doing circus as above). Again omap device will be created, this time with details about gpmc peripherals (first time, there were no peripheral details provided, and this was done for old interface to work with same driver), once this happens new interface will starting it's job.

This kind of change was required as old interface works at arch_init

Quoted 16 lines

+static int __init gpmc_pre_init(void)

arch/arm/mach-omap2/gpmc.c __init int omap_gpmc_init(struct gpmc_pdata *pdata) · line 948

Quoted 10 lines

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

+{
+	static struct gpmc_device_pdata *gpmc_device_data[1];
+	struct gpmc_pdata gpmc_data = {
+		.device_pdata	= gpmc_device_data,
+	};
+
+	return omap_gpmc_init(&gpmc_data);
+}
+postcore_initcall(gpmc_pre_init);
+

Not sure I see the point in the above function. Why not declare the gpmc_device_data struct as static in the file and access it directly instead of passing it in omap_gpmc_init(). The postcore_init can then call omap_gpmc_init() directly.

Can be done, but it is not necessary to make it available outside the function

Ok, I see that this is necessary until all boards are migrated. However, please label with a FIXME to make it clear that this is to be removed.

Hi Afzal,

Quoted 30 lines · Afzal Mohammed

A driver is being created out of GPMC code. This is being attempted to acheive by not breaking existing interface, necessitating requirement of GPMC peripherals being able to work with as well as without the help of driver. To not break existing, initcall is required as in old interface GPMC is configured at arch initcall and GPMC should be ready to handle old interface by that time

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

---
 arch/arm/mach-omap2/gpmc.c |   19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

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

index b471c2f..6dbddb9 100644
@@ -902,7 +902,7 @@ postcore_initcall(gpmc_init);
 __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 {
 	struct omap_hwmod *oh;
-	struct platform_device *pdev;
+	static struct platform_device *pdev;
 	char *name = "omap-gpmc";
 	char *oh_name = "gpmc";
 

@@ -912,6 +912,12 @@ __init int omap_gpmc_init(struct gpmc_pdata *pdata)
 		return -ENODEV;
 	}
 

arch/arm/mach-omap2/gpmc.c __init int omap_gpmc_init(struct gpmc_pdata *pdata) · line 920

Quoted 6 lines

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

+	if (pdev != NULL) {
+		clk_put(gpmc_l3_clk);
+		omap_device_delete(pdev->archdata.od);
+		platform_device_unregister(pdev);
+	}
+

I am not sure if I am missing something, but it appears that pdev will always be NULL here as it is a local uninitialised variable.

Not sure I see the point in the above function. Why not declare the gpmc_device_data struct as static in the file and access it directly instead of passing it in omap_gpmc_init(). The postcore_init can then call omap_gpmc_init() directly.

Shouldn't the post_initcall be added in patch #4, when the driver is created?

Quoted 2 lines

I am not sure if I am missing something, but it appears that pdev will always be NULL here as it is a local uninitialised variable.

This also creates a new warning:

arch/arm/mach-omap2/gpmc.c: In function ?gpmc_create_device?:
arch/arm/mach-omap2/gpmc.c:1434: warning: ?pdev? may be used uninitialized in this function

Quoted 8 lines

Not sure I see the point in the above function. Why not declare the gpmc_device_data struct as static in the file and access it directly instead of passing it in omap_gpmc_init(). The postcore_init can then call omap_gpmc_init() directly.

Shouldn't the post_initcall be added in patch #4, when the driver is created?

There should no longer be any need to initialize GPMC early. It should behave like any other device driver, and also work as a loadable module.

Quoted 2 lines · Tony Lindgren

There should no longer be any need to initialize GPMC early. It should behave like any other device driver, and also work as a loadable module.

Once driver migration is complete for all the boards, we can remove the initcall and make it loadable.

But till that time as we need to support both old & new interface, I feel this is necessary as old interface would happen in arch initcall, and old gpmc_init is now gpmc_probe, and hence it needs to get executed before arch initcall, necessitating this.

Hi Tony,

Quoted 8 lines · Afzal

Quoted 2 lines · Tony Lindgren

There should no longer be any need to initialize GPMC early. It should behave like any other device driver, and also work as a loadable module.

Once driver migration is complete for all the boards, we can remove the initcall and make it loadable.

Quoted 4 lines

But till that time as we need to support both old & new interface, I feel this is necessary as old interface would happen in arch initcall, and old gpmc_init is now gpmc_probe, and hence it needs to get executed before arch initcall, necessitating this.

Seems you missed this in the flood of mails, please let me know your comments on this. This is a commit that has to be reverted once driver migration is complete for all boards.

Create a minimal driver out of gpmc code.
Responsibilities handled by earlier gpmc
initialization is now achieved in probe.

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

---
 arch/arm/mach-omap2/gpmc.c |  170 ++++++++++++++++++++++++++++++++------------
 1 file changed, 123 insertions(+), 47 deletions(-)

arch/arm/mach-omap2/gpmc.c +85 -36

index 6dbddb9..a91f40f 100644
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
 
 #include <asm/mach-types.h>
 #include <plat/gpmc.h>

@@ -31,6 +32,8 @@
 
 #include <plat/sdrc.h>
 
+#define	DRIVER_NAME		"omap-gpmc"
+
 /* GPMC register offsets */
 #define GPMC_REVISION		0x00
 #define GPMC_SYSCONFIG		0x10

@@ -115,6 +118,21 @@ struct omap3_gpmc_regs {
 	struct gpmc_cs_config cs_context[GPMC_CS_NUM];
 };
 
+struct gpmc_peripheral {
+	char			*name;
+	int			id;
+	void			*pdata;
+	unsigned		pdata_size;
+	struct resource		*per_res;
+	unsigned		per_res_cnt;
+	struct resource		*gpmc_res;
+	unsigned		gpmc_res_cnt;
+	bool			have_waitpin;
+	bool			waitpin_polarity;
+	unsigned		waitpin;
+	struct platform_device	*pdev;
+};
+
 static struct gpmc_client_irq gpmc_client_irq[GPMC_NR_IRQ];
 static struct irq_chip gpmc_irq_chip;
 static unsigned gpmc_irq_start;

@@ -124,6 +142,10 @@ static struct resource	gpmc_cs_mem[GPMC_CS_NUM];
 static DEFINE_SPINLOCK(gpmc_mem_lock);
 static unsigned int gpmc_cs_map;	/* flag for cs which are initialized */
 static int gpmc_ecc_used = -EINVAL;	/* cs using ecc engine */
+static struct device *gpmc_dev;
+static u32 gpmc_revision;
+static int gpmc_irq;
+static resource_size_t phys_base, mem_size;
 
 static void __iomem *gpmc_base;
 

@@ -433,6 +455,19 @@ static int gpmc_cs_insert_mem(int cs, unsigned long base, unsigned long size)
 	return r;
 }
 
+static int gpmc_cs_delete_mem(int cs)
+{
+	struct resource	*res = &gpmc_cs_mem[cs];
+	int r;
+
+	spin_lock(&gpmc_mem_lock);
+	r = release_resource(&gpmc_cs_mem[cs]);
+	res->start = res->end = 0;
+	spin_unlock(&gpmc_mem_lock);
+
+	return r;
+}
+
 int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
 {
 	struct resource *res = &gpmc_cs_mem[cs];

@@ -769,7 +804,7 @@ static void gpmc_irq_noop(struct irq_data *data) { }
 
 static unsigned int gpmc_irq_noop_ret(struct irq_data *data) { return 0; }
 
-static int gpmc_setup_irq(int gpmc_irq)
+static int gpmc_setup_irq(void)
 {
 	int i;
 	u32 regval;

@@ -813,7 +848,37 @@ static int gpmc_setup_irq(int gpmc_irq)
 	return request_irq(gpmc_irq, gpmc_handle_irq, 0, "gpmc", NULL);
 }
 
-static void __init gpmc_mem_init(void)
+static __exit int gpmc_free_irq(void)
+{
+	int i;
+
+	if (gpmc_irq)
+		free_irq(gpmc_irq, NULL);
+
+	for (i = 0; i < GPMC_NR_IRQ; i++) {
+		irq_set_handler(gpmc_client_irq[i].irq, NULL);
+		irq_set_chip(gpmc_client_irq[i].irq, &no_irq_chip);
+		irq_modify_status(gpmc_client_irq[i].irq, 0, 0);
+	}
+
+	irq_free_descs(gpmc_irq_start, GPMC_NR_IRQ);
+
+	return 0;
+}
+
+static void __devexit gpmc_mem_exit(void)
+{
+	int cs;
+
+	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
+		if (!gpmc_cs_mem_enabled(cs))
+			continue;
+		gpmc_cs_delete_mem(cs);
+	}
+
+}
+
+static void __devinit gpmc_mem_init(void)
 {
 	int cs;
 	unsigned long boot_rom_space = 0;

@@ -840,64 +905,75 @@ static void __init gpmc_mem_init(void)
 	}
 }
 
-static int __init gpmc_init(void)
+static __devinit int gpmc_probe(struct platform_device *pdev)
 {
 	u32 l;
-	int ret = -EINVAL;
-	int gpmc_irq;
-	char *ck = NULL;
-
-	if (cpu_is_omap24xx()) {
-		ck = "core_l3_ck";
-		if (cpu_is_omap2420())
-			l = OMAP2420_GPMC_BASE;
-		else
-			l = OMAP34XX_GPMC_BASE;
-		gpmc_irq = INT_34XX_GPMC_IRQ;
-	} else if (cpu_is_omap34xx()) {
-		ck = "gpmc_fck";
-		l = OMAP34XX_GPMC_BASE;
-		gpmc_irq = INT_34XX_GPMC_IRQ;
-	} else if (cpu_is_omap44xx()) {
-		ck = "gpmc_ck";
-		l = OMAP44XX_GPMC_BASE;
-		gpmc_irq = OMAP44XX_IRQ_GPMC;
-	}
+	struct resource *res;
 
-	if (WARN_ON(!ck))
-		return ret;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL)
+		return -ENOENT;
 
-	gpmc_l3_clk = clk_get(NULL, ck);
-	if (IS_ERR(gpmc_l3_clk)) {
-		printk(KERN_ERR "Could not get GPMC clock %s\n", ck);
-		BUG();
-	}
+	phys_base = res->start;
+	mem_size = resource_size(res);
 
-	gpmc_base = ioremap(l, SZ_4K);
-	if (!gpmc_base) {
-		clk_put(gpmc_l3_clk);
-		printk(KERN_ERR "Could not get GPMC register memory\n");
-		BUG();
-	}
+	gpmc_base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!gpmc_base)
+		return -EADDRNOTAVAIL;
+
+	gpmc_dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+	if (res == NULL)
+		dev_warn(gpmc_dev, "Failed to get resource: irq\n");
+	else
+		gpmc_irq = res->start;
 

arch/arm/mach-omap2/gpmc.c static void __init gpmc_mem_init(void) · line 935

Quoted 1 line

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

+	gpmc_revision = (l >> 4) & 0xf;

Why are you only storing the major part of the rev? Why not keep both parts?

arch/arm/mach-omap2/gpmc.c static void __init gpmc_mem_init(void) · line 936

Quoted 2 lines

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

+	dev_info(gpmc_dev, "GPMC revision %d.%d\n", gpmc_revision, l & 0x0f);

Nit-pick, please add a mask and shift definition for the revision register major and minor fields and use these above.

Hi Jon,

Quoted 3 lines · Jon

arch/arm/mach-omap2/gpmc.c static void __init gpmc_mem_init(void) · line 935

Quoted 1 line

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

+	gpmc_revision = (l >> 4) & 0xf;

Why are you only storing the major part of the rev? Why not keep both parts?

Does having minor revision add any value ?, at least as of now, I do not see any.

Quoted 4 lines

arch/arm/mach-omap2/gpmc.c static void __init gpmc_mem_init(void) · line 936

Quoted 1 line

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

+	dev_info(gpmc_dev, "GPMC revision %d.%d\n", gpmc_revision, l & 0x0f);

Nit-pick, please add a mask and shift definition for the revision register major and minor fields and use these above.

Yes, right

Quoted 4 lines · Afzal

Hi Jon,

On Tue, Jun 12, 2012 at 02:13:22, Hunter, Jon wrote:

Quoted 6 lines

Quoted 3 lines

arch/arm/mach-omap2/gpmc.c static void __init gpmc_mem_init(void) · line 935

Quoted 1 line

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

+	gpmc_revision = (l >> 4) & 0xf;

Why are you only storing the major part of the rev? Why not keep both parts?

Does having minor revision add any value ?, at least as of now, I do not see any.

May be not but it does not hurt either.

Quoted 7 lines · Jon Hunter

Quoted 2 lines · Afzal

Does having minor revision add any value ?, at least as of now, I do not see any.

May be not but it does not hurt either.

Yeah might as well show the full revision.

Hi Tony,

Quoted 7 lines · Tony Lindgren

Quoted 6 lines · Jon Hunter

Quoted 2 lines · Afzal

Does having minor revision add any value ?, at least as of now, I do not see any.

Quoted 3 lines

Quoted 1 line

May be not but it does not hurt either.

Yeah might as well show the full revision.

We are already showing full version, but only storing major, it seems you want to store minor version along with major version in the same variable, right ?

Quoted 12 lines · Mohammed, Afzal

Hi Tony,

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Jon Hunter

Quoted 2 lines · Afzal

Does having minor revision add any value ?, at least as of now, I do not see any.

May be not but it does not hurt either.

Quoted 5 lines

Quoted 1 line

Yeah might as well show the full revision.

We are already showing full version, but only storing major, it seems you want to store minor version along with major version in the same variable, right ?

Ah OK, yeah might as well just store the minor version.

Actually why do you even need to store the revision? You can just read it from the hardware as needed.

Hi Tony,

Quoted 2 lines · Tony Lindgren

Actually why do you even need to store the revision? You can just read it from the hardware as needed.

Earlier for wr_access & wr_data_mux_bus, we were checking, cpu_is_34xx, but I feel it should instead be based on IP revision as it is the IP revision that decides the availability of those register bits.

Hi Tony,

Quoted 7 lines · Afzal

Hi Tony,

Quoted 2 lines · Tony Lindgren

Actually why do you even need to store the revision? You can just read it from the hardware as needed.

Quoted 4 lines

Earlier for wr_access & wr_data_mux_bus, we were checking, cpu_is_34xx, but I feel it should instead be based on IP revision as it is the IP revision that decides the availability of those register bits.

Or you meant, even there read revision register ?

Quoted 1 line · Mohammed, Afzal

Or you meant, even there read revision register ?

Yeah that should be fine as this really should only happen during init and whatever revision specific features can be initialized for GPMC.

Hi Tony,

Quoted 5 lines · Tony Lindgren

Quoted 1 line · Mohammed, Afzal

Or you meant, even there read revision register ?

Quoted 3 lines

Yeah that should be fine as this really should only happen during init and whatever revision specific features can be initialized for GPMC.

Sorry, I got confused, we need revision to be available while setting time also, so you meant to store it as a variable or read revision at probe as well as during setting time ?

Quoted 3 lines · Mohammed, Afzal

Sorry, I got confused, we need revision to be available while setting time also, so you meant to store it as a variable or read revision at probe as well as during setting time ?

You can set up GPMC specific flags during init, something like OMAP_GPMC_HAS_XYZ, or you can set up function pointers if only some features are available on some revisions.

Quoted 12 lines · Mohammed, Afzal

Hi Tony,

Quoted 9 lines · Tony Lindgren

Quoted 7 lines · Jon Hunter

Quoted 2 lines · Afzal

Does having minor revision add any value ?, at least as of now, I do not see any.

May be not but it does not hurt either.

Quoted 5 lines

Quoted 1 line

Yeah might as well show the full revision.

We are already showing full version, but only storing major, it seems you want to store minor version along with major version in the same variable, right ?

Ah OK, yeah might as well just store the minor version.

Create a minimal driver out of gpmc code.
Responsibilities handled by earlier gpmc
initialization is now achieved in probe.

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

---
 arch/arm/mach-omap2/gpmc.c |  170 ++++++++++++++++++++++++++++++++------------
 1 file changed, 123 insertions(+), 47 deletions(-)

arch/arm/mach-omap2/gpmc.c +77 -36

index 6dbddb9..a91f40f 100644
@@ -24,6 +24,7 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/interrupt.h>
+#include <linux/platform_device.h>
 
 #include <asm/mach-types.h>
 #include <plat/gpmc.h>

@@ -31,6 +32,8 @@
 
 #include <plat/sdrc.h>
 
+#define	DRIVER_NAME		"omap-gpmc"
+
 /* GPMC register offsets */
 #define GPMC_REVISION		0x00
 #define GPMC_SYSCONFIG		0x10

@@ -115,6 +118,21 @@ struct omap3_gpmc_regs {
 	struct gpmc_cs_config cs_context[GPMC_CS_NUM];
 };
 
+struct gpmc_peripheral {
+	char			*name;
+	int			id;
+	void			*pdata;
+	unsigned		pdata_size;
+	struct resource		*per_res;
+	unsigned		per_res_cnt;
+	struct resource		*gpmc_res;
+	unsigned		gpmc_res_cnt;
+	bool			have_waitpin;
+	bool			waitpin_polarity;
+	unsigned		waitpin;
+	struct platform_device	*pdev;
+};
+
 static struct gpmc_client_irq gpmc_client_irq[GPMC_NR_IRQ];
 static struct irq_chip gpmc_irq_chip;
 static unsigned gpmc_irq_start;

@@ -124,6 +142,10 @@ static struct resource	gpmc_cs_mem[GPMC_CS_NUM];
 static DEFINE_SPINLOCK(gpmc_mem_lock);
 static unsigned int gpmc_cs_map;	/* flag for cs which are initialized */
 static int gpmc_ecc_used = -EINVAL;	/* cs using ecc engine */
+static struct device *gpmc_dev;
+static u32 gpmc_revision;
+static int gpmc_irq;
+static resource_size_t phys_base, mem_size;
 
 static void __iomem *gpmc_base;
 

@@ -433,6 +455,19 @@ static int gpmc_cs_insert_mem(int cs, unsigned long base, unsigned long size)
 	return r;
 }
 
+static int gpmc_cs_delete_mem(int cs)
+{
+	struct resource	*res = &gpmc_cs_mem[cs];
+	int r;
+
+	spin_lock(&gpmc_mem_lock);
+	r = release_resource(&gpmc_cs_mem[cs]);
+	res->start = res->end = 0;
+	spin_unlock(&gpmc_mem_lock);
+
+	return r;
+}
+
 int gpmc_cs_request(int cs, unsigned long size, unsigned long *base)
 {
 	struct resource *res = &gpmc_cs_mem[cs];

@@ -769,7 +804,7 @@ static void gpmc_irq_noop(struct irq_data *data) { }
 
 static unsigned int gpmc_irq_noop_ret(struct irq_data *data) { return 0; }
 
-static int gpmc_setup_irq(int gpmc_irq)
+static int gpmc_setup_irq(void)
 {
 	int i;
 	u32 regval;

@@ -813,7 +848,37 @@ static int gpmc_setup_irq(int gpmc_irq)
 	return request_irq(gpmc_irq, gpmc_handle_irq, 0, "gpmc", NULL);
 }
 
-static void __init gpmc_mem_init(void)
+static __exit int gpmc_free_irq(void)
+{
+	int i;
+
+	if (gpmc_irq)
+		free_irq(gpmc_irq, NULL);
+
+	for (i = 0; i < GPMC_NR_IRQ; i++) {
+		irq_set_handler(gpmc_client_irq[i].irq, NULL);
+		irq_set_chip(gpmc_client_irq[i].irq, &no_irq_chip);
+		irq_modify_status(gpmc_client_irq[i].irq, 0, 0);
+	}
+
+	irq_free_descs(gpmc_irq_start, GPMC_NR_IRQ);
+
+	return 0;
+}
+
+static void __devexit gpmc_mem_exit(void)
+{
+	int cs;
+
+	for (cs = 0; cs < GPMC_CS_NUM; cs++) {
+		if (!gpmc_cs_mem_enabled(cs))
+			continue;
+		gpmc_cs_delete_mem(cs);
+	}
+
+}
+
+static void __devinit gpmc_mem_init(void)
 {
 	int cs;
 	unsigned long boot_rom_space = 0;

@@ -840,64 +905,75 @@ static void __init gpmc_mem_init(void)
 	}
 }
 
-static int __init gpmc_init(void)
+static __devinit int gpmc_probe(struct platform_device *pdev)
 {
 	u32 l;
-	int ret = -EINVAL;
-	int gpmc_irq;
-	char *ck = NULL;
-
-	if (cpu_is_omap24xx()) {
-		ck = "core_l3_ck";
-		if (cpu_is_omap2420())
-			l = OMAP2420_GPMC_BASE;
-		else
-			l = OMAP34XX_GPMC_BASE;
-		gpmc_irq = INT_34XX_GPMC_IRQ;
-	} else if (cpu_is_omap34xx()) {
-		ck = "gpmc_fck";
-		l = OMAP34XX_GPMC_BASE;
-		gpmc_irq = INT_34XX_GPMC_IRQ;
-	} else if (cpu_is_omap44xx()) {
-		ck = "gpmc_ck";
-		l = OMAP44XX_GPMC_BASE;
-		gpmc_irq = OMAP44XX_IRQ_GPMC;
-	}
+	struct resource *res;
 
-	if (WARN_ON(!ck))
-		return ret;
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (res == NULL)
+		return -ENOENT;
 
-	gpmc_l3_clk = clk_get(NULL, ck);
-	if (IS_ERR(gpmc_l3_clk)) {
-		printk(KERN_ERR "Could not get GPMC clock %s\n", ck);
-		BUG();
-	}
+	phys_base = res->start;
+	mem_size = resource_size(res);
 
-	gpmc_base = ioremap(l, SZ_4K);
-	if (!gpmc_base) {
-		clk_put(gpmc_l3_clk);
-		printk(KERN_ERR "Could not get GPMC register memory\n");
-		BUG();
-	}
+	gpmc_base = devm_request_and_ioremap(&pdev->dev, res);
+	if (!gpmc_base)
+		return -EADDRNOTAVAIL;

Quoted 1 line · Afzal Mohammed

clk_enable(gpmc_l3_clk);

We should be able to get rid of the clk_enable() here and use ...

	pm_runtime_enable(&pdev->dev);
	pm_runtime_get(&pdev->dev);

The clk_disable should also be replaced with a pm_runtime_put(). You will also need to include pm_runtime.h.

Helpers for propulating given resource structure
with memory & interrupt information.

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

---
 arch/arm/mach-omap2/gpmc.c |   45 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

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

index a91f40f..652b245 100644
@@ -905,6 +905,51 @@ static void __devinit gpmc_mem_init(void)
 	}
 }
 
+static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs,
+						struct resource *res)
+{
+	unsigned long start;
+	int ret;
+
+	ret = gpmc_cs_request(cs->cs, cs->mem_size, &start);
+	if (IS_ERR_VALUE(ret)) {
+		dev_err(gpmc_dev, "error: gpmc request on CS: %d\n", cs->cs);
+		return ret;
+	}

arch/arm/mach-omap2/gpmc.c static void __devinit gpmc_mem_init(void) · line 928

Quoted 5 lines

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

+static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs,
+						struct resource *res)
 
+	return 1;
+}

Nit-pick, CodingStyle chapter 16 states that 0 should be used for success when returning from a function.

arch/arm/mach-omap2/gpmc.c static void __devinit gpmc_mem_init(void) · line 933

Quoted 4 lines

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

+static inline unsigned gpmc_bit_to_irq(unsigned bitmask)
+{
+	return bitmask;
+}

I have to ask what is the value in this helper function? ;-)

Hi Jon,

Quoted 8 lines · Jon

arch/arm/mach-omap2/gpmc.c static void __devinit gpmc_mem_init(void) · line 928

Quoted 5 lines

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

+static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs,
+						struct resource *res)
 
+	return 1;
+}

Nit-pick, CodingStyle chapter 16 states that 0 should be used for success when returning from a function.

I doubt whether this falls to the category you are referring. This function returns number of resources setup, please see similar for irq too, there it can be 2 also, success or failure is not sufficient for the other, and with this we have both functions returning values similar way.

Quoted 6 lines

arch/arm/mach-omap2/gpmc.c static void __devinit gpmc_mem_init(void) · line 933

Quoted 4 lines

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

+static inline unsigned gpmc_bit_to_irq(unsigned bitmask)
+{
+	return bitmask;
+}

I have to ask what is the value in this helper function? ;-)

It is to stress the fact that flag user is passing need not be same as irq bit (here both have same value), this kind of an arrangement would be required later if waitpin interrupt also has to be handled, do you still prefer removing it now ?

Well looking at the function it seems that you either return an error code or 1. So if you are never going to return anything other than 1 on success it may as well be 0.

Hi Jon,

Quoted 3 lines · Jon

Well looking at the function it seems that you either return an error code or 1. So if you are never going to return anything other than 1 on success it may as well be 0.

Irq & memory resource creation functions returns number of resources, if memory function only is modified, that will cause loss of uniformity w.r.t irq function, even though both does similar things.

Quoted 3 lines · Afzal

Irq & memory resource creation functions returns number of resources, if memory function only is modified, that will cause loss of uniformity w.r.t irq function, even though both does similar things.

Ok, I see what you mean but from a readability standpoint it looked odd. The other function is returning "n" where as this is just returning 1 on success but it is not clear that this actually means 1 resource.

Helper for configuring given CS based on flags.

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

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

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

index 652b245..4e19010 100644

arch/arm/mach-omap2/gpmc.c static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs, · line 932

Quoted 3 lines

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

+static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
+{
+	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);

Why is it necessary to read the register first? I thought you wanted to get away from relying on bootloader settings?

arch/arm/mach-omap2/gpmc.c static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs, · line 945

Quoted 13 lines

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

+	l &= ~(GPMC_CONFIG1_MUXADDDATA |
+		GPMC_CONFIG1_WRITETYPE_SYNC |
+		GPMC_CONFIG1_WRITEMULTIPLE_SUPP |
+		GPMC_CONFIG1_READTYPE_SYNC |
+		GPMC_CONFIG1_READMULTIPLE_SUPP |
+		GPMC_CONFIG1_WRAPBURST_SUPP |
+		GPMC_CONFIG1_DEVICETYPE(~0) |
+		GPMC_CONFIG1_DEVICESIZE(~0) |
+		GPMC_CONFIG1_PAGE_LEN(~0));
+
+	l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
+	l |= conf & GPMC_CONFIG1_DEVICESIZE_16;

I can see that it works to use the above definitions as masks because of the possible values that can be programmed into these fields. However, from a read-ability standpoint this is unclear and requires people to review the documentation to understand what you are doing here. Furthermore, if any new device-types or sizes were added in the future this could lead to bugs. Hence, it would be better to define a mask for these fields.

Quoted 10 lines

arch/arm/mach-omap2/gpmc.c static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs, · line 945

Quoted 2 lines

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

+	l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
+	l |= conf & GPMC_CONFIG1_DEVICESIZE_16;

I can see that it works to use the above definitions as masks because of the possible values that can be programmed into these fields. However, from a read-ability standpoint this is unclear and requires people to review the documentation to understand what you are doing here. Furthermore, if any new device-types or sizes were added in the future this could lead to bugs. Hence, it would be better to define a mask for these fields.

I had thought about it initially, but then it was felt it will lead to a less simple code, that path was not taken, let me revisit this.

Hi Jon,

Quoted 11 lines · Afzal

Quoted 10 lines

arch/arm/mach-omap2/gpmc.c static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs, · line 945

Quoted 2 lines

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

+	l |= conf & GPMC_CONFIG1_DEVICETYPE_NAND;
+	l |= conf & GPMC_CONFIG1_DEVICESIZE_16;

I can see that it works to use the above definitions as masks because of the possible values that can be programmed into these fields. However, from a read-ability standpoint this is unclear and requires people to review the documentation to understand what you are doing here. Furthermore, if any new device-types or sizes were added in the future this could lead to bugs. Hence, it would be better to define a mask for these fields.

Quoted 3 lines

I had thought about it initially, but then it was felt it will lead to a less simple code, that path was not taken, let me revisit this.

Thinking again over it, I am feeling above is sufficient, reason same as said earlier, to keep code simple & currently this is sufficient to handle GPMC bit patterns for IPs presently available. What you are suggesting is to take care of the imaginary case when new GPMC IP happens with new device type or size, I think that should be handled when such a scenario happens. Probably, it is better here to add a comment to make intention clear.

Quoted 7 lines · Afzal

Thinking again over it, I am feeling above is sufficient, reason same as said earlier, to keep code simple & currently this is sufficient to handle GPMC bit patterns for IPs presently available. What you are suggesting is to take care of the imaginary case when new GPMC IP happens with new device type or size, I think that should be handled when such a scenario happens. Probably, it is better here to add a comment to make intention clear.

That is one possibility but I think that more important reasons are ...

1. Readability, at first it appears that we are always configuring the CS for NAND. However, this is not the case. Maybe a comment here can help as you mentioned.

2. A bad setting in the configuration passed. Hopefully, people will stick to the flags but we know that we expect the device type to be a 0 or 2 and so should we check?

Hi Jon,

Quoted 10 lines · Jon

Quoted 7 lines · Afzal

Thinking again over it, I am feeling above is sufficient, reason same as said earlier, to keep code simple & currently this is sufficient to handle GPMC bit patterns for IPs presently available. What you are suggesting is to take care of the imaginary case when new GPMC IP happens with new device type or size, I think that should be handled when such a scenario happens. Probably, it is better here to add a comment to make intention clear.

Quoted 5 lines

That is one possibility but I think that more important reasons are ...

1. Readability, at first it appears that we are always configuring the CS for NAND. However, this is not the case. Maybe a comment here can help as you mentioned.

As far as the users of GPMC is considered there is no confusion. Eg. For device type, they have been provided with two macros,

GPMC_CONFIG1_DEVICETYPE_NOR, GPMC_CONFIG1_DEVICETYPE_NAND

So for NOR, user can feel satisfied by giving NOR flag, little does he know that driver doesn't do anything with the flag, but he still gets what he want NOR flag is defined as zero. Yes even if he doesn't specify any type, device type will be set as NOR, but then that is the default - the only other option

Quoted 4 lines

2. A bad setting in the configuration passed. Hopefully, people will stick to the flags but we know that we expect the device type to be a 0 or 2 and so should we check?

Value of device type is something driver has to worry about, not its users, they have been provided with two flags, one for NAND & other for NOR.

Sure, but reviewing the function it still looks odd from a readability standpoint. At least it made me think "what is going on here ...". So a comment is definitely needed.

Quoted 5 lines

Quoted 4 lines

2. A bad setting in the configuration passed. Hopefully, people will stick to the flags but we know that we expect the device type to be a 0 or 2 and so should we check?

Quoted 2 lines

Value of device type is something driver has to worry about, not its users, they have been provided with two flags, one for NAND & other for NOR.

Yes, but the driver does not seem to worry about it. In other words, there is no error checking. Ok so not a big deal a comment should suffice.

Hi Jon,

Quoted 6 lines · Jon

arch/arm/mach-omap2/gpmc.c static __devinit int gpmc_setup_cs_mem(struct gpmc_cs_data *cs, · line 932

Quoted 3 lines

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

+static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
+{
+	u32 l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);

Why is it necessary to read the register first? I thought you wanted to get away from relying on bootloader settings?

This is not trying to depend on bootloader, it is to alter bits that are only meant for configuration. There are other bits in the same register configured as part of time setting.

Well it is unclear what the code flow is for using this helper as this change simply adds the helper. If someone other function is writing to the CONFIG1 register before this function, then you may wish to highlight the programming sequence in the changelog or at least describe why this function performs a read-modify-write and not just a write.

Helper for setting GPMC timing by taking input as register values.

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

---
 arch/arm/mach-omap2/gpmc.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

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

index 4e19010..e60076e3 100644
@@ -960,6 +960,49 @@ static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 }
 
+static inline void gpmc_set_one_timing(int cs, int reg, int start,
+							int end, u32 val)
+{
+	u32 l;
+	unsigned mask;
+
+	mask = (1 << (end - start + 1)) - 1;
+	l = gpmc_cs_read_reg(cs, reg);
+	l &= ~(mask << start);
+	l |= val << start;
+	gpmc_cs_write_reg(cs, reg, l);
+}
+
+static void gpmc_cs_set_register_timings(int cs, const struct gpmc_timings *t)
+{
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG1, 0, 1, t->sync_clk);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG1, 18, 19, t->wait_monitoring);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG1, 25, 26, t->clk_activation);
+
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG2, 0, 3, t->cs_on);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG2, 8, 12, t->cs_rd_off);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG2, 16, 20, t->cs_wr_off);
+
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG3, 0, 3, t->adv_on);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG3, 8, 12, t->adv_rd_off);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG3, 16, 20, t->adv_wr_off);
+
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG4, 0, 3, t->oe_on);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG4, 8, 12, t->oe_off);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG4, 16, 19, t->we_on);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG4, 24, 28, t->we_off);
+
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG5, 24, 27, t->page_burst_access);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG6, 0, 3, t->bus_turnaround);
+	gpmc_set_one_timing(cs, GPMC_CS_CONFIG6, 8, 11,	t->cycle2cycle_delay);
+
+	if (gpmc_revision >= 4) {
+		gpmc_set_one_timing(cs, GPMC_CS_CONFIG6, 16, 19,
+						t->wr_data_mux_bus);
+		gpmc_set_one_timing(cs, GPMC_CS_CONFIG6, 24, 28, t->wr_access);
+	}
+}
+
 static inline unsigned gpmc_bit_to_irq(unsigned bitmask)
 {
 	return bitmask;

When are the timings calculated? Why not just use the existing gpmc_cs_set_timings()?

I guess I am not convinced that we need to have multiple formats to pass timings such as clock, period, etc.

I think that it is sufficient to pass the timings as time (period) as is currently done and have the driver convert to clocks based upon the gpmc fclk rate.

Some of the timing configuration like extra delay
has bool type configurations. Provide a helper so
that these too can be configured in Kernel.

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

---
 arch/arm/mach-omap2/gpmc.c |   55 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

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

index e60076e3..65052f8 100644
@@ -68,6 +68,13 @@
 #define GPMC_ECC_CTRL_ECCREG8		0x008
 #define GPMC_ECC_CTRL_ECCREG9		0x009
 
+#define	GPMC_CONFIG2_CSEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG3_ADVEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG4_OEEXTRADELAY		BIT(7)
+#define	GPMC_CONFIG4_WEEXTRADELAY		BIT(23)
+#define	GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN	BIT(6)
+#define	GPMC_CONFIG6_CYCLE2CYCLESAMECSEN	BIT(7)
+
 #define GPMC_CS0_OFFSET		0x60
 #define GPMC_CS_SIZE		0x30
 

@@ -960,6 +967,54 @@ static void gpmc_setup_cs_config(unsigned cs, unsigned conf)
 	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
 }
 
+static void gpmc_cs_misc_timings(int cs, const struct gpmc_misc_timings *p)
+{
+	u32 l;
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
+	if (p->time_para_granularity)
+		l |= GPMC_CONFIG1_TIME_PARA_GRAN;
+	else
+		l &= ~GPMC_CONFIG1_TIME_PARA_GRAN;
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG2);
+	if (p->cs_extra_delay)
+		l |= GPMC_CONFIG2_CSEXTRADELAY;
+	else
+		l &= ~GPMC_CONFIG2_CSEXTRADELAY;
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG2, l);
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG3);
+	if (p->adv_extra_delay)
+		l |= GPMC_CONFIG3_ADVEXTRADELAY;
+	else
+		l &= ~GPMC_CONFIG3_ADVEXTRADELAY;
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG3, l);
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG4);
+	if (p->oe_extra_delay)
+		l |= GPMC_CONFIG4_OEEXTRADELAY;gpmc_cs_misc_timings
+	else
+		l &= ~GPMC_CONFIG4_OEEXTRADELAY;
+	if (p->we_extra_delay)
+		l |= GPMC_CONFIG4_WEEXTRADELAY;
+	else
+		l &= ~GPMC_CONFIG4_WEEXTRADELAY;
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG4, l);
+
+	l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG6);

Quoted 4 lines

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

+	if (p->cycle2cyclesamecsen)
+		l |= GPMC_CONFIG6_CYCLE2CYCLESAMECSEN;
+	else
+		l &= ~GPMC_CONFIG6_CYCLE2CYCLESAMECSEN;

arch/arm/mach-omap2/gpmc.c static void gpmc_setup_cs_config(unsigned cs, unsigned conf) · line 1016

Quoted 6 lines

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

+	if (p->cycle2cyclediffcsen)
+		l |= GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN;
+	else
+		l &= ~GPMC_CONFIG6_CYCLE2CYCLEDIFFCSEN;
+	gpmc_cs_write_reg(cs, GPMC_CS_CONFIG6, l);
+}

How about adding a sub-function like ...