am335x fixes for 3.7-rc2

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Quoted 8 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

Quoted 7 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

I'm applying this one into omap-for-v3.7-rc1/fixes-part2.

Next time, please also cc linux-omap at vger.kernel.org for series like this. I'm sure the people reading the omap list are interested in these.

From 753a4928bf6f7baa4c001bdca3d15a85e999db4c Mon Sep 17 00:00:00 2001

Hi Tony,

Quoted 17 lines · Tony Lindgren

Quoted 15 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Quoted 5 lines

I'm applying this one into omap-for-v3.7-rc1/fixes-part2.

Next time, please also cc linux-omap at vger.kernel.org for series like this. I'm sure the people reading the omap list are interested in these.

This patch appears to be masking an underlying issue. How about something like the following ...

Quoted 26 lines · Jon Hunter

Hi Tony,

Quoted 22 lines · Tony Lindgren

Quoted 8 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

Quoted 7 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

I'm applying this one into omap-for-v3.7-rc1/fixes-part2.

Next time, please also cc linux-omap at vger.kernel.org for series like this. I'm sure the people reading the omap list are interested in these.

Quoted 2 lines

This patch appears to be masking an underlying issue. How about something like the following ...

OK that looks good to me. I'll drop the earlier fix and use yours instead.

From 91f5234d567c07ce1579b50e52de1a1e06ce5c68 Mon Sep 17 00:00:00 2001

Quoted 30 lines · Tony Lindgren

Quoted 28 lines · Jon Hunter

Hi Tony,

Quoted 17 lines · Tony Lindgren

Quoted 8 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

Quoted 7 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Quoted 5 lines

I'm applying this one into omap-for-v3.7-rc1/fixes-part2.

Next time, please also cc linux-omap at vger.kernel.org for series like this. I'm sure the people reading the omap list are interested in these.

This patch appears to be masking an underlying issue. How about something like the following ...

Quoted 2 lines

OK that looks good to me. I'll drop the earlier fix and use yours instead.

Hi Tony, sorry but I realised now that in my patch that I need to take care of releasing and memory and clocks that were acquired during the probe. Here is a V2. If you prefer I can create a delta patch also with the previous.

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Quoted 7 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

I am not sure I completely follow the logic here.

Won't this problem occur if the bootloader does not configure the gpmc cs space AND we are not using DT?

Is the csvalid bit set because we are booting from the internal ROM?

I guess I don't see why csvalid bit being set and a base-address of 0x0 should not be allowed. That should be a valid combination.

One problem I see with gpmc_mem_init() is that it assumes that BOOT_ROM_SPACE is 1MB for all devices starting at 0x0 apart from the apollon board. For newer devices such as OMAP4, there is only 48KB of internal ROM and only mapped to 0x0 when booting from internal ROM. So this needs to be fixed.

How much internal ROM does the AM335x have and where is it mapped?

Quoted 4 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

Quoted 3 lines

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Yes, but we do actually still allow some platform devices to be probed (such as dmtimers) when booting with DT that don't support DT yet. So this change prevents us from using the gpmc on boards when booting with DT.

I am not convinced that this is addressing the underlying problem with gpmc_mem_init().

Cheers
Jon

Quoted 10 lines · Jon

Quoted 7 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

Quoted 5 lines

I am not sure I completely follow the logic here.

Won't this problem occur if the bootloader does not configure the gpmc cs space AND we are not using DT?

That's what exactly the above comment describes.

Quoted 2 lines

Is the csvalid bit set because we are booting from the internal ROM?

As per TRM, the reset value of the CS0_valis bit is set to 0. I have pasted TRM statement below -

"Chip-select enable (reset value is 1 for CS[0] and 0 for CS[1-5])."

And same applies to OMAP3 family of devices.

Quoted 3 lines

I guess I don't see why csvalid bit being set and a base-address of 0x0 should not be allowed. That should be a valid combination.

Yes, agreed.

Quoted 8 lines

One problem I see with gpmc_mem_init() is that it assumes that BOOT_ROM_SPACE is 1MB for all devices starting at 0x0 apart from the apollon board. For newer devices such as OMAP4, there is only 48KB of internal ROM and only mapped to 0x0 when booting from internal ROM. So this needs to be fixed.

How much internal ROM does the AM335x have and where is it mapped?

AM33xx memory map is something like,

Boot ROM 0x4000_0000 0x4001_FFFF 128KB

          0x4002_0000 0x4002_BFFF 48KB    32-bit Ex/R(1) - Public
Reserved  0x4002_C000 0x400F_FFFF 848KB   Reserved
Reserved  0x4010_0000 0x401F_FFFF 1MB     Reserved
Reserved  0x4020_0000 0x402E_FFFF 960KB   Reserved
Reserved  0x402f_0000 0x4020_03FF 64KB    Reserved

SRAM internal 0x402F_0400 0x402F_FFFF 32-bit Ex/R/W(1)

Quoted 4 lines

Quoted 4 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

Quoted 8 lines

Quoted 3 lines

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Yes, but we do actually still allow some platform devices to be probed (such as dmtimers) when booting with DT that don't support DT yet. So this change prevents us from using the gpmc on boards when booting with DT.

The idea here was,

In order to use GPMC in meaningful way, where some peripheral is connected to the GPMC, you must create platform_device for the probe to happen properly. Now all the devices I know so far, we have gpmc_smsc911x_init(), omap_nand_flash_init(), etc...
These api's are getting called only through machine_desc.init_xxx callbacks, And in case of DT, we have centralized machine_desc definition for all platforms (board-generic.c). So even though you want to use GPMC for DT boot mode, you can not make use of peripheral without changing board-files to change to create platform_device.

Does it make sense?

Quoted 3 lines

I am not convinced that this is addressing the underlying problem with gpmc_mem_init().

The patch you submitted is cleanup patch and is required irrespective of this patch. I believe this patch is just makes sure that, if you are booting from DT and you do not have meaningful DT node for GPMC and peripheral interfaced to it, no point in probing it.

Does it make any sense???

Quoted 13 lines · Vaibhav

Quoted 12 lines · Jon

Quoted 7 lines · Richard Cochran

From: hvaibhav at ti.com <[email protected]>

With recent changes in omap gpmc driver code, in case of DT boot mode, where bootloader does not configure gpmc cs space will result into kernel BUG() inside gpmc_mem_init() function, as gpmc cs0 gpmc_config7[0].csvalid bit is set to '1' and gpmc_config7[0].baseaddress is set to '0' on reset.

I am not sure I completely follow the logic here.

Quoted 5 lines

Quoted 3 lines

Won't this problem occur if the bootloader does not configure the gpmc cs space AND we are not using DT?

That's what exactly the above comment describes.

Hmm ... you said "in the case of DT", but I am saying even "in the case WITHOUT DT" this can happen. So I think the subject is mis-leading.

Quoted 7 lines

Quoted 2 lines

Is the csvalid bit set because we are booting from the internal ROM?

As per TRM, the reset value of the CS0_valis bit is set to 0. I have pasted TRM statement below -

"Chip-select enable (reset value is 1 for CS[0] and 0 for CS[1-5])."

The above two sentences don't see to agree ...

Quoted 2 lines

And same applies to OMAP3 family of devices.

For which boot-modes? All or just the gpmc boot-modes?

My omap3430 beagle has been booting with DT fine for some time and I have not encountered this problem even on the latest kernel with the gpmc driver present.

Quoted 17 lines

Quoted 3 lines

I guess I don't see why csvalid bit being set and a base-address of 0x0 should not be allowed. That should be a valid combination.

Yes, agreed.

Quoted 8 lines

One problem I see with gpmc_mem_init() is that it assumes that BOOT_ROM_SPACE is 1MB for all devices starting at 0x0 apart from the apollon board. For newer devices such as OMAP4, there is only 48KB of internal ROM and only mapped to 0x0 when booting from internal ROM. So this needs to be fixed.

How much internal ROM does the AM335x have and where is it mapped?

AM33xx memory map is something like,

Quoted 7 lines

Boot ROM 0x4000_0000 0x4001_FFFF 128KB

          0x4002_0000 0x4002_BFFF 48KB    32-bit Ex/R(1) - Public
Reserved  0x4002_C000 0x400F_FFFF 848KB   Reserved
Reserved  0x4010_0000 0x401F_FFFF 1MB     Reserved
Reserved  0x4020_0000 0x402E_FFFF 960KB   Reserved
Reserved  0x402f_0000 0x4020_03FF 64KB    Reserved

SRAM internal 0x402F_0400 0x402F_FFFF 32-bit Ex/R/W(1)

Does the boot ROM get mapped to 0x0, when booting from ROM?

Quoted 25 lines

Quoted 4 lines

Quoted 4 lines

This use-case is applicable for any board/EVM which doesn't have any peripheral connected to gpmc cs0, for example BeagleXM and BeagleBone, so DT boot mode fails.

Quoted 8 lines

Quoted 3 lines

This patch adds of_have_populated_dt() check before creating device, so that for DT boot mode, gpmc probe will not be called which is expected behavior, as gpmc is not supported yet from DT.

Yes, but we do actually still allow some platform devices to be probed (such as dmtimers) when booting with DT that don't support DT yet. So this change prevents us from using the gpmc on boards when booting with DT.

The idea here was,

In order to use GPMC in meaningful way, where some peripheral is connected to the GPMC, you must create platform_device for the probe to happen properly. Now all the devices I know so far, we have gpmc_smsc911x_init(), omap_nand_flash_init(), etc...
These api's are getting called only through machine_desc.init_xxx callbacks, And in case of DT, we have centralized machine_desc definition for all platforms (board-generic.c). So even though you want to use GPMC for DT boot mode, you can not make use of peripheral without changing board-files to change to create platform_device.

Quoted 1 line

Does it make sense?

Sure, if you are using one of the generic machine configurations for DT. However, while this migration happens people may create their own custom machine configurations for DT for testing things like smsc911x.

Quoted 9 lines

Quoted 3 lines

I am not convinced that this is addressing the underlying problem with gpmc_mem_init().

The patch you submitted is cleanup patch and is required irrespective of this patch. I believe this patch is just makes sure that, if you are booting from DT and you do not have meaningful DT node for GPMC and peripheral interfaced to it, no point in probing it.

Quoted 1 line

Does it make any sense???

Yes, but do you also see the bug that is hiding in gpmc_mem_init()?

My point is to highlight this and not hide it, so that we can fix it now. Otherwise if we wait until we enable the gpmc driver with DT and this could hinder the DT migration later.

Jon

On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:

Quoted 6 lines

Yes, but do you also see the bug that is hiding in gpmc_mem_init()?

My point is to highlight this and not hide it, so that we can fix it now. Otherwise if we wait until we enable the gpmc driver with DT and this could hinder the DT migration later.

As I already mentioned in my previous response, your patch is required irrespective of this patch. I would consider your patch as a cleanup patch.

Both the patches are independent, your patch is handling the error path properly, whereas, my patch makes sure that you don't unnecessarily probe GPMC if you are booting from DT and GPMC node is not present, as described above.

Quoted 1 line · Vaibhav

On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:

...

Quoted 11 lines

Quoted 6 lines

Yes, but do you also see the bug that is hiding in gpmc_mem_init()?

My point is to highlight this and not hide it, so that we can fix it now. Otherwise if we wait until we enable the gpmc driver with DT and this could hinder the DT migration later.

As I already mentioned in my previous response, your patch is required irrespective of this patch. I would consider your patch as a cleanup patch.

Quoted 4 lines

Both the patches are independent, your patch is handling the error path properly, whereas, my patch makes sure that you don't unnecessarily probe GPMC if you are booting from DT and GPMC node is not present, as described above.

Your patch hides a bug. That's my point. How do you expect am335x ever to support gpmc devices if this bug is not addressed?

So I think that you are over-simplifying it when you say that my patch is just a clean-up patch. I agree that it is adding appropriate error handling, but it also highlights the presence of a bug by allowing the probe to fail.

Anyway, I don't care to debate this any further,

we just need to fix
gpmc_mem_init().

Jon

Quoted 22 lines · Jon

Quoted 1 line · Vaibhav

On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:

...

Quoted 15 lines

Quoted 6 lines

Yes, but do you also see the bug that is hiding in gpmc_mem_init()?

My point is to highlight this and not hide it, so that we can fix it now. Otherwise if we wait until we enable the gpmc driver with DT and this could hinder the DT migration later.

As I already mentioned in my previous response, your patch is required irrespective of this patch. I would consider your patch as a cleanup patch.

Both the patches are independent, your patch is handling the error path properly, whereas, my patch makes sure that you don't unnecessarily probe GPMC if you are booting from DT and GPMC node is not present, as described above.

Quoted 3 lines

Your patch hides a bug. That's my point. How do you expect am335x ever to support gpmc devices if this bug is not addressed?

Jon,
May be my commit description was mis-leading to you.
I am not commenting anything on your bug-fix, but do not agree that it is anything to do with hiding a bug.

I only agree with you on one point, if someone wants to change the board- file to use GPMC with DT boot mode, then he will not be able to use it.

Quoted 6 lines

So I think that you are over-simplifying it when you say that my patch is just a clean-up patch. I agree that it is adding appropriate error handling, but it also highlights the presence of a bug by allowing the probe to fail.

Anyway, I don't care to debate this any further,

Me neither...

Quoted 3 lines

we just need to fix
gpmc_mem_init().

Agreed, and that's what your patch rightly doing it.

From: Mugunthan V N <[email protected]>

This patch adds minimal hwmod support for davinci mdio driver. This patch requires rework on parent child relation between cpsw and davinci mdio hwmod data to support runtime PM.

From: Vaibhav Hiremath <[email protected]>

By mistake (most likely a copy-paste), instead of pm_runtime_get_sync() api, driver is calling pm_runtime_put_sync() api in resume callback function. The bug was introduced by commit id (ae2c07aaf74:
davinci_mdio: runtime PM support).

Now, the reason why it didn't impact functionality is, the patch has been tested on AM335x-EVM and BeagleBone platform while submitting; and in case of AM335x the MDIO driver doesn't control the module enable/disable part, which is handled by CPSW driver.

Signed-off-by Vaibhav Hiremath <[email protected]>

Cc Mugunthan V N <[email protected]>

---
 drivers/net/ethernet/ti/davinci_mdio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

drivers/net/ethernet/ti/davinci_mdio.c +1 -1

index 51a96db..ae74280 100644
@@ -465,7 +465,7 @@ static int davinci_mdio_resume(struct device *dev)
 	u32 ctrl;
 
 	spin_lock(&data->lock);
-	pm_runtime_put_sync(data->dev);
+	pm_runtime_get_sync(data->dev);
 
 	/* restart the scan state machine */
 	ctrl = __raw_readl(&data->regs->control);

Signature · 1 line

1.7.2.5

From: Vaibhav Hiremath <[email protected]>

CPGMAC SubSystem consist of various sub-modules, like, mdio, cpdma, cpsw, etc... These sub-modules are also used in some of Davinci family of devices. Now based on requirement, use-case and available technology nodes the integration of these sub-modules varies across devices.

So coming back to Linux net driver, currently separate and independent platform devices & drivers for CPSW and MDIO is implemented. In case of Davinci they both has separate control, from resources perspective, like clock.

In case of AM33XX, the resources are shared and only one register bit-field is provided to control module/clock enable/disable, makes it difficult to handle common resource.

So the solution here implemented in this patch is,

Create parent<->child relationship between both the drivers, making CPSW as a parent and MDIO as its child and enumerate all the child nodes under cpsw module.
Both the drivers will function exactly the way it was operating before, including runtime-pm functionality. No change is required in MDIO driver (for that matter to any child driver).

As this is only supported during DT boot, the parent<->child relationship is created and populated in DT execution flow. The only required change is inside DTS file, making MDIO as a child to CPSW node.

Signed-off-by Vaibhav Hiremath <[email protected]>

Cc Mugunthan V N <[email protected]>

---
 drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

drivers/net/ethernet/ti/cpsw.c +14 -2

index df55e24..fb1a692 100644
@@ -827,7 +827,7 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 	}
 	data->mac_control = prop;
 
-	for_each_child_of_node(node, slave_node) {
+	for_each_node_by_name(slave_node, "slave") {
 		struct cpsw_slave_data *slave_data = data->slave_data + i;
 		const char *phy_id = NULL;
 		const void *mac_addr = NULL;

@@ -862,6 +862,14 @@ static int cpsw_probe_dt(struct cpsw_platform_data *data,
 		i++;
 	}
 
+	/*
+	 * Populate all the child nodes here...
+	 */
+	ret = of_platform_populate(node, NULL, NULL, &pdev->dev);
+	/* We do not want to force this, as in some cases may not have child */
+	if (ret)
+		pr_warn("Doesn't have any child node\n");
+
 	return 0;
 
 error_ret:

@@ -895,6 +903,11 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 	priv->msg_enable = netif_msg_init(debug_level, CPSW_DEBUG);
 	priv->rx_packet_max = max(rx_packet_max, 128);
 
+	/*
+	 * This may be required here for child devices.
+	 */
+	pm_runtime_enable(&pdev->dev);
+
 	if (cpsw_probe_dt(&priv->data, pdev)) {
 		pr_err("cpsw: platform data missing\n");
 		ret = -ENODEV;

@@ -921,7 +934,6 @@ static int __devinit cpsw_probe(struct platform_device *pdev)
 	for (i = 0; i < data->slaves; i++)
 		priv->slaves[i].slave_num = i;
 
-	pm_runtime_enable(&pdev->dev);
 	priv->clk = clk_get(&pdev->dev, "fck");
 	if (IS_ERR(priv->clk)) {
 		dev_err(&pdev->dev, "fck is not found\n");

Signature · 1 line

1.7.2.5

From: Mugunthan V N <[email protected]>

Add CPSW and MDIO related device tree data for AM33XX. Also enable them into board/evm dts files by providing respective phy-id.