[PATCH 0/5] am335x fixes for 3.7-rc2

[PATCH 0/5] am335x fixes for 3.7-rc2

[Richard Cochran]

This series contains patches required to let the kernel run on a TI
am335x, like the popular BeagleBone. None of these patches are from
me, yet all of them are essential fixes. For net-next I have prepared
a cpsw driver extension that enables time stamping and a PTP clock,
but in order to be useful, the kernel must boot and the MAC driver
must be working.

The first patch was supposed to have made -rc1, but it seems to have
been forgotten. The other four patches are for the cpsw MAC driver,
which has not worked since the migration to device tree. Actually, I
am not sure if the cpsw ever did work, but in any case these patches
let the driver work (again), rather than being a lifeless derelict.

Thanks,
Richard


Mugunthan V N (2):
  ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio
  arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

Vaibhav Hiremath (2):
  net: davinci_mdio: Fix type mistake in calling runtime-pm api
  net: cpsw: Add parent<->child relation support between cpsw and mdio

hvaibhav@ti.com (1):
  ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

 arch/arm/boot/dts/am335x-bone.dts          |    8 ++++
 arch/arm/boot/dts/am335x-evm.dts           |    8 ++++
 arch/arm/boot/dts/am33xx.dtsi              |   50 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/gpmc.c                 |    4 ++
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |   34 ++++++++++++++++++-
 drivers/net/ethernet/ti/cpsw.c             |   16 ++++++++-
 drivers/net/ethernet/ti/davinci_mdio.c     |    2 +-
 7 files changed, 117 insertions(+), 5 deletions(-)

-- 
1.7.2.5

[PATCH 0/5] am335x fixes for 3.7-rc2

[Richard Cochran]

This series contains patches required to let the kernel run on a TI
am335x, like the popular BeagleBone. None of these patches are from
me, yet all of them are essential fixes. For net-next I have prepared
a cpsw driver extension that enables time stamping and a PTP clock,
but in order to be useful, the kernel must boot and the MAC driver
must be working.

The first patch was supposed to have made -rc1, but it seems to have
been forgotten. The other four patches are for the cpsw MAC driver,
which has not worked since the migration to device tree. Actually, I
am not sure if the cpsw ever did work, but in any case these patches
let the driver work (again), rather than being a lifeless derelict.

Thanks,
Richard


Mugunthan V N (2):
  ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio
  arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

Vaibhav Hiremath (2):
  net: davinci_mdio: Fix type mistake in calling runtime-pm api
  net: cpsw: Add parent<->child relation support between cpsw and mdio

hvaibhav at ti.com (1):
  ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

 arch/arm/boot/dts/am335x-bone.dts          |    8 ++++
 arch/arm/boot/dts/am335x-evm.dts           |    8 ++++
 arch/arm/boot/dts/am33xx.dtsi              |   50 ++++++++++++++++++++++++++++
 arch/arm/mach-omap2/gpmc.c                 |    4 ++
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |   34 ++++++++++++++++++-
 drivers/net/ethernet/ti/cpsw.c             |   16 ++++++++-
 drivers/net/ethernet/ti/davinci_mdio.c     |    2 +-
 7 files changed, 117 insertions(+), 5 deletions(-)

-- 
1.7.2.5

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Richard Cochran]

From: hvaibhav@ti.com <hvaibhav@ti.com>

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.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Afzal Mohammed <afzal@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/gpmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..90b033e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -981,6 +981,10 @@ static int __init omap_gpmc_init(void)
 	struct platform_device *pdev;
 	char *oh_name = "gpmc";
 
+	/* If dtb is there, the devices will be created dynamically */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
 	oh = omap_hwmod_lookup(oh_name);
 	if (!oh) {
 		pr_err("Could not look up %s\n", oh_name);
-- 
1.7.2.5

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Richard Cochran]

From: hvaibhav@ti.com <hvaibhav@ti.com>

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.

Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
Cc: Afzal Mohammed <afzal@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc Paul Walmsley <paul@pwsan.com>
---
 arch/arm/mach-omap2/gpmc.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..90b033e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -981,6 +981,10 @@ static int __init omap_gpmc_init(void)
 	struct platform_device *pdev;
 	char *oh_name = "gpmc";
 
+	/* If dtb is there, the devices will be created dynamically */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
 	oh = omap_hwmod_lookup(oh_name);
 	if (!oh) {
 		pr_err("Could not look up %s\n", oh_name);
-- 
1.7.2.5

[PATCH 2/5] ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio

[Richard Cochran]

From: Mugunthan V N <mugunthanvnm@ti.com>

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.

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |   34 ++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index 59d5c1c..f96bbc0 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -650,8 +650,7 @@ static struct omap_hwmod_class_sysconfig am33xx_cpgmac_sysc = {
 	.rev_offs	= 0x0,
 	.sysc_offs	= 0x8,
 	.syss_offs	= 0x4,
-	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_MIDLEMODE |
-			   SYSS_HAS_RESET_STATUS),
+	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_MIDLEMODE),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | MSTANDBY_FORCE |
 			   MSTANDBY_NO),
 	.sysc_fields	= &omap_hwmod_sysc_type3,
@@ -682,6 +681,8 @@ static struct omap_hwmod am33xx_cpgmac0_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= (HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY |
+			   HWMOD_INIT_NO_RESET | HWMOD_INIT_NO_IDLE),
 };
 
 /*
@@ -2510,6 +2511,34 @@ static struct omap_hwmod_addr_space am33xx_elm_addr_space[] = {
 	{ }
 };
 
+/* mdio class */
+static struct omap_hwmod_class am33xx_mdio_hwmod_class = {
+	.name		= "davinci_mdio",
+};
+
+struct omap_hwmod_addr_space am33xx_mdio_addr_space[] = {
+	{
+		.pa_start	= 0x4A101000,
+		.pa_end		= 0x4A101000 + SZ_256 - 1,
+		.flags		= ADDR_MAP_ON_INIT,
+	},
+	{ }
+};
+
+static struct omap_hwmod am33xx_mdio_hwmod = {
+	.name		= "davinci_mdio",
+	.class		= &am33xx_mdio_hwmod_class,
+	.clkdm_name	= "cpsw_125mhz_clkdm",
+	.main_clk	= "cpsw_125mhz_gclk",
+};
+
+struct omap_hwmod_ocp_if am33xx_cpgmac0__mdio = {
+	.master		= &am33xx_cpgmac0_hwmod,
+	.slave		= &am33xx_mdio_hwmod,
+	.addr		= am33xx_mdio_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
 static struct omap_hwmod_ocp_if am33xx_l4_ls__elm = {
 	.master		= &am33xx_l4_ls_hwmod,
 	.slave		= &am33xx_elm_hwmod,
@@ -3371,6 +3400,7 @@ static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[] __initdata = {
 	&am33xx_l3_main__tptc2,
 	&am33xx_l3_s__usbss,
 	&am33xx_l4_hs__cpgmac0,
+	&am33xx_cpgmac0__mdio,
 	NULL,
 };
 
-- 
1.7.2.5

[PATCH 2/5] ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio

[Richard Cochran]

From: Mugunthan V N <mugunthanvnm@ti.com>

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.

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c |   34 ++++++++++++++++++++++++++-
 1 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index 59d5c1c..f96bbc0 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -650,8 +650,7 @@ static struct omap_hwmod_class_sysconfig am33xx_cpgmac_sysc = {
 	.rev_offs	= 0x0,
 	.sysc_offs	= 0x8,
 	.syss_offs	= 0x4,
-	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_MIDLEMODE |
-			   SYSS_HAS_RESET_STATUS),
+	.sysc_flags	= (SYSC_HAS_SIDLEMODE | SYSC_HAS_MIDLEMODE),
 	.idlemodes	= (SIDLE_FORCE | SIDLE_NO | MSTANDBY_FORCE |
 			   MSTANDBY_NO),
 	.sysc_fields	= &omap_hwmod_sysc_type3,
@@ -682,6 +681,8 @@ static struct omap_hwmod am33xx_cpgmac0_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= (HWMOD_SWSUP_SIDLE | HWMOD_SWSUP_MSTANDBY |
+			   HWMOD_INIT_NO_RESET | HWMOD_INIT_NO_IDLE),
 };
 
 /*
@@ -2510,6 +2511,34 @@ static struct omap_hwmod_addr_space am33xx_elm_addr_space[] = {
 	{ }
 };
 
+/* mdio class */
+static struct omap_hwmod_class am33xx_mdio_hwmod_class = {
+	.name		= "davinci_mdio",
+};
+
+struct omap_hwmod_addr_space am33xx_mdio_addr_space[] = {
+	{
+		.pa_start	= 0x4A101000,
+		.pa_end		= 0x4A101000 + SZ_256 - 1,
+		.flags		= ADDR_MAP_ON_INIT,
+	},
+	{ }
+};
+
+static struct omap_hwmod am33xx_mdio_hwmod = {
+	.name		= "davinci_mdio",
+	.class		= &am33xx_mdio_hwmod_class,
+	.clkdm_name	= "cpsw_125mhz_clkdm",
+	.main_clk	= "cpsw_125mhz_gclk",
+};
+
+struct omap_hwmod_ocp_if am33xx_cpgmac0__mdio = {
+	.master		= &am33xx_cpgmac0_hwmod,
+	.slave		= &am33xx_mdio_hwmod,
+	.addr		= am33xx_mdio_addr_space,
+	.user		= OCP_USER_MPU,
+};
+
 static struct omap_hwmod_ocp_if am33xx_l4_ls__elm = {
 	.master		= &am33xx_l4_ls_hwmod,
 	.slave		= &am33xx_elm_hwmod,
@@ -3371,6 +3400,7 @@ static struct omap_hwmod_ocp_if *am33xx_hwmod_ocp_ifs[] __initdata = {
 	&am33xx_l3_main__tptc2,
 	&am33xx_l3_s__usbss,
 	&am33xx_l4_hs__cpgmac0,
+	&am33xx_cpgmac0__mdio,
 	NULL,
 };
 
-- 
1.7.2.5

[PATCH 3/5] net: davinci_mdio: Fix type mistake in calling runtime-pm api

[Richard Cochran]

From: Vaibhav Hiremath <hvaibhav@ti.com>

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 <hvaibhav@ti.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/davinci_mdio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 51a96db..ae74280 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -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);
-- 
1.7.2.5

[PATCH 3/5] net: davinci_mdio: Fix type mistake in calling runtime-pm api

[Richard Cochran]

From: Vaibhav Hiremath <hvaibhav@ti.com>

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 <hvaibhav@ti.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/davinci_mdio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
index 51a96db..ae74280 100644
--- a/drivers/net/ethernet/ti/davinci_mdio.c
+++ b/drivers/net/ethernet/ti/davinci_mdio.c
@@ -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);
-- 
1.7.2.5

[PATCH 4/5] net: cpsw: Add parent<->child relation support between cpsw and mdio

[Richard Cochran]

From: Vaibhav Hiremath <hvaibhav@ti.com>

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 <hvaibhav@ti.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index df55e24..fb1a692 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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");
-- 
1.7.2.5

[PATCH 4/5] net: cpsw: Add parent<->child relation support between cpsw and mdio

[Richard Cochran]

From: Vaibhav Hiremath <hvaibhav@ti.com>

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 <hvaibhav@ti.com>
Cc: Mugunthan V N <mugunthanvnm@ti.com>
---
 drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++++--
 1 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index df55e24..fb1a692 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -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");
-- 
1.7.2.5

[PATCH 5/5] arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

[Richard Cochran]

From: Mugunthan V N <mugunthanvnm@ti.com>

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

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
---
 arch/arm/boot/dts/am335x-bone.dts |    8 ++++++
 arch/arm/boot/dts/am335x-evm.dts  |    8 ++++++
 arch/arm/boot/dts/am33xx.dtsi     |   50 +++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts
index c634f87..e233cfa 100644
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -78,3 +78,11 @@
 		};
 	};
 };
+
+&cpsw_emac0 {
+	phy_id = "4a101000.mdio:00";
+};
+
+&cpsw_emac1 {
+	phy_id = "4a101000.mdio:01";
+};
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 185d632..415c3b3 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -118,3 +118,11 @@
 		};
 	};
 };
+
+&cpsw_emac0 {
+	phy_id = "4a101000.mdio:00";
+};
+
+&cpsw_emac1 {
+	phy_id = "4a101000.mdio:01";
+};
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index bb31bff..f6bea04 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -210,5 +210,55 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		mac: ethernet@4A100000 {
+			compatible = "ti,cpsw";
+			ti,hwmods = "cpgmac0";
+			cpdma_channels = <8>;
+			host_port_no = <0>;
+			cpdma_reg_ofs = <0x800>;
+			cpdma_sram_ofs = <0xa00>;
+			ale_reg_ofs = <0xd00>;
+			ale_entries = <1024>;
+			host_port_reg_ofs = <0x108>;
+			hw_stats_reg_ofs = <0x900>;
+			bd_ram_ofs = <0x2000>;
+			bd_ram_size = <0x2000>;
+			no_bd_ram = <0>;
+			rx_descs = <64>;
+			mac_control = <0x20>;
+			slaves = <2>;
+			reg = <0x4a100000 0x800
+				0x4a101200 0x100
+				0x4a101000 0x100>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			interrupt-parent = <&intc>;
+			/* c0_rx_thresh_pend c0_rx_pend c0_tx_pend c0_misc_pend*/
+			interrupts = <40 41 42 43>;
+			ranges;
+			cpsw_emac0: slave@0 {
+				slave_reg_ofs = <0x208>;
+				sliver_reg_ofs = <0xd80>;
+				/* Filled in by U-Boot */
+				mac-address = [ 00 00 00 00 00 00 ];
+			};
+			cpsw_emac1: slave@1 {
+				slave_reg_ofs = <0x308>;
+				sliver_reg_ofs = <0xdc0>;
+				/* Filled in by U-Boot */
+				mac-address = [ 00 00 00 00 00 00 ];
+			};
+
+			davinci_mdio: mdio@4a101000 {
+				compatible = "ti,davinci_mdio";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				ti,hwmods = "davinci_mdio";
+				bus_freq = <1000000>;
+				reg = <0x4a101000 0x100>;
+			};
+
+		};
 	};
 };
-- 
1.7.2.5

[PATCH 5/5] arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

[Richard Cochran]

From: Mugunthan V N <mugunthanvnm@ti.com>

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

Signed-off-by: Mugunthan V N <mugunthanvnm@ti.com>
Signed-off-by: Vaibhav Hiremath <hvaibhav@ti.com>
---
 arch/arm/boot/dts/am335x-bone.dts |    8 ++++++
 arch/arm/boot/dts/am335x-evm.dts  |    8 ++++++
 arch/arm/boot/dts/am33xx.dtsi     |   50 +++++++++++++++++++++++++++++++++++++
 3 files changed, 66 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/am335x-bone.dts b/arch/arm/boot/dts/am335x-bone.dts
index c634f87..e233cfa 100644
--- a/arch/arm/boot/dts/am335x-bone.dts
+++ b/arch/arm/boot/dts/am335x-bone.dts
@@ -78,3 +78,11 @@
 		};
 	};
 };
+
+&cpsw_emac0 {
+	phy_id = "4a101000.mdio:00";
+};
+
+&cpsw_emac1 {
+	phy_id = "4a101000.mdio:01";
+};
diff --git a/arch/arm/boot/dts/am335x-evm.dts b/arch/arm/boot/dts/am335x-evm.dts
index 185d632..415c3b3 100644
--- a/arch/arm/boot/dts/am335x-evm.dts
+++ b/arch/arm/boot/dts/am335x-evm.dts
@@ -118,3 +118,11 @@
 		};
 	};
 };
+
+&cpsw_emac0 {
+	phy_id = "4a101000.mdio:00";
+};
+
+&cpsw_emac1 {
+	phy_id = "4a101000.mdio:01";
+};
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index bb31bff..f6bea04 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -210,5 +210,55 @@
 			interrupt-parent = <&intc>;
 			interrupts = <91>;
 		};
+
+		mac: ethernet at 4A100000 {
+			compatible = "ti,cpsw";
+			ti,hwmods = "cpgmac0";
+			cpdma_channels = <8>;
+			host_port_no = <0>;
+			cpdma_reg_ofs = <0x800>;
+			cpdma_sram_ofs = <0xa00>;
+			ale_reg_ofs = <0xd00>;
+			ale_entries = <1024>;
+			host_port_reg_ofs = <0x108>;
+			hw_stats_reg_ofs = <0x900>;
+			bd_ram_ofs = <0x2000>;
+			bd_ram_size = <0x2000>;
+			no_bd_ram = <0>;
+			rx_descs = <64>;
+			mac_control = <0x20>;
+			slaves = <2>;
+			reg = <0x4a100000 0x800
+				0x4a101200 0x100
+				0x4a101000 0x100>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+			interrupt-parent = <&intc>;
+			/* c0_rx_thresh_pend c0_rx_pend c0_tx_pend c0_misc_pend*/
+			interrupts = <40 41 42 43>;
+			ranges;
+			cpsw_emac0: slave at 0 {
+				slave_reg_ofs = <0x208>;
+				sliver_reg_ofs = <0xd80>;
+				/* Filled in by U-Boot */
+				mac-address = [ 00 00 00 00 00 00 ];
+			};
+			cpsw_emac1: slave at 1 {
+				slave_reg_ofs = <0x308>;
+				sliver_reg_ofs = <0xdc0>;
+				/* Filled in by U-Boot */
+				mac-address = [ 00 00 00 00 00 00 ];
+			};
+
+			davinci_mdio: mdio at 4a101000 {
+				compatible = "ti,davinci_mdio";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				ti,hwmods = "davinci_mdio";
+				bus_freq = <1000000>;
+				reg = <0x4a101000 0x100>;
+			};
+
+		};
 	};
 };
-- 
1.7.2.5

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> From: hvaibhav@ti.com <hvaibhav@ti.com>
> 
> 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.

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

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

Regards,

Tony

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> From: hvaibhav at ti.com <hvaibhav@ti.com>
> 
> 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.

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.

Regards,

Tony

Re: [PATCH 2/5] ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:23]:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> 
> 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.

Looks like Paul Walmsley may have missed this one, maybe please resend
with him and linux-omap cc:d.

Regards,

Tony

[PATCH 2/5] ARM: OMAP3+: hwmod: Add AM33XX HWMOD data for davinci_mdio

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:23]:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> 
> 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.

Looks like Paul Walmsley may have missed this one, maybe please resend
with him and linux-omap cc:d.

Regards,

Tony

Re: [PATCH 5/5] arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:23]:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> 
> Add CPSW and MDIO related device tree data for AM33XX.
> Also enable them into board/evm dts files by providing
> respective phy-id.

These omap specific .dts changes  should be queued by Benoit Cousson,
please cc him and linux-omap.

Regards,

Tony

[PATCH 5/5] arm/dts: am33xx: Add cpsw and mdio module nodes for AM33XX

[Tony Lindgren]

* Richard Cochran <richardcochran@gmail.com> [121015 12:23]:
> From: Mugunthan V N <mugunthanvnm@ti.com>
> 
> Add CPSW and MDIO related device tree data for AM33XX.
> Also enable them into board/evm dts files by providing
> respective phy-id.

These omap specific .dts changes  should be queued by Benoit Cousson,
please cc him and linux-omap.

Regards,

Tony

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/15/2012 02:16 PM, Richard Cochran wrote:
> From: hvaibhav@ti.com <hvaibhav@ti.com>
> 
> 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?

> 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.

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

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/15/2012 02:16 PM, Richard Cochran wrote:
> From: hvaibhav@ti.com <hvaibhav@ti.com>
> 
> 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?

> 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.

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

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/15/2012 02:16 PM, Richard Cochran wrote:
> From: hvaibhav at ti.com <hvaibhav@ti.com>
> 
> 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?

> 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.

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

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

Hi Tony,

On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>> From: hvaibhav@ti.com <hvaibhav@ti.com>
>>
>> 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.
> 
> I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
> 
> Next time, please also cc linux-omap@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 ...

Cheers
Jon

>From 753a4928bf6f7baa4c001bdca3d15a85e999db4c Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Tested on AM335x beagle bone board.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..8f0d3c8 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,17 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc))
+			return rc;
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +940,11 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

Hi Tony,

On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>> From: hvaibhav@ti.com <hvaibhav@ti.com>
>>
>> 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.
> 
> I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
> 
> Next time, please also cc linux-omap@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 ...

Cheers
Jon

>From 753a4928bf6f7baa4c001bdca3d15a85e999db4c Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Tested on AM335x beagle bone board.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..8f0d3c8 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,17 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc))
+			return rc;
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +940,11 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

Hi Tony,

On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>> From: hvaibhav at ti.com <hvaibhav@ti.com>
>>
>> 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.
> 
> 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 ...

Cheers
Jon

>From 753a4928bf6f7baa4c001bdca3d15a85e999db4c Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Tested on AM335x beagle bone board.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..8f0d3c8 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,17 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc))
+			return rc;
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +940,11 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
> Hi Tony,
> 
> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> > * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> >> From: hvaibhav@ti.com <hvaibhav@ti.com>
> >>
> >> 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.
> > 
> > I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
> > 
> > Next time, please also cc linux-omap@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 ...

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

Regards,

Tony

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
> Hi Tony,
> 
> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> > * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> >> From: hvaibhav at ti.com <hvaibhav@ti.com>
> >>
> >> 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.
> > 
> > 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 ...

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

Regards,

Tony

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/16/2012 04:26 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
>> Hi Tony,
>>
>> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
>>> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>>>> From: hvaibhav@ti.com <hvaibhav@ti.com>
>>>>
>>>> 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.
>>>
>>> I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
>>>
>>> Next time, please also cc linux-omap@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 ...
> 
> 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. 

Cheers
Jon

>From 91f5234d567c07ce1579b50e52de1a1e06ce5c68 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH V2] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Boot tested on AM335x beagle bone board and OMAP4430 Panda board.

V2 changes:
- Ensure that clock and memory resources are released on error.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..92b5718 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,21 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc)) {
+			while (--cs >= 0)
+				if (gpmc_cs_mem_enabled(cs))
+					gpmc_cs_delete_mem(cs);
+			return rc;
+		}
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +944,13 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		clk_disable_unprepare(gpmc_l3_clk);
+		clk_put(gpmc_l3_clk);
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5
 

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/16/2012 04:26 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
>> Hi Tony,
>>
>> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
>>> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>>>> From: hvaibhav@ti.com <hvaibhav@ti.com>
>>>>
>>>> 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.
>>>
>>> I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
>>>
>>> Next time, please also cc linux-omap@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 ...
> 
> 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. 

Cheers
Jon

>From 91f5234d567c07ce1579b50e52de1a1e06ce5c68 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH V2] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Boot tested on AM335x beagle bone board and OMAP4430 Panda board.

V2 changes:
- Ensure that clock and memory resources are released on error.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..92b5718 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,21 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc)) {
+			while (--cs >= 0)
+				if (gpmc_cs_mem_enabled(cs))
+					gpmc_cs_delete_mem(cs);
+			return rc;
+		}
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +944,13 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		clk_disable_unprepare(gpmc_l3_clk);
+		clk_put(gpmc_l3_clk);
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5
 

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/16/2012 04:26 PM, Tony Lindgren wrote:
> * Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
>> Hi Tony,
>>
>> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
>>> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
>>>> From: hvaibhav at ti.com <hvaibhav@ti.com>
>>>>
>>>> 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.
>>>
>>> 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 ...
> 
> 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. 

Cheers
Jon

>From 91f5234d567c07ce1579b50e52de1a1e06ce5c68 Mon Sep 17 00:00:00 2001
From: Jon Hunter <jon-hunter@ti.com>
Date: Tue, 16 Oct 2012 15:22:58 -0500
Subject: [PATCH V2] ARM: OMAP2+: Allow kernel to boot even if GPMC fails to
 reserve memory

Currently, if the GPMC driver fails to reserve memory when probed we will
call BUG() and the kernel will not boot. Instead of calling BUG(), return
an error from probe and allow kernel to boot.

Boot tested on AM335x beagle bone board and OMAP4430 Panda board.

V2 changes:
- Ensure that clock and memory resources are released on error.

Signed-off-by: Jon Hunter <jon-hunter@ti.com>
---
 arch/arm/mach-omap2/gpmc.c |   24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 5ac5cf3..92b5718 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -868,9 +868,9 @@ static void __devexit gpmc_mem_exit(void)
 
 }
 
-static void __devinit gpmc_mem_init(void)
+static int __devinit gpmc_mem_init(void)
 {
-	int cs;
+	int cs, rc;
 	unsigned long boot_rom_space = 0;
 
 	/* never allocate the first page, to facilitate bug detection;
@@ -890,13 +890,21 @@ static void __devinit gpmc_mem_init(void)
 		if (!gpmc_cs_mem_enabled(cs))
 			continue;
 		gpmc_cs_get_memconf(cs, &base, &size);
-		if (gpmc_cs_insert_mem(cs, base, size) < 0)
-			BUG();
+		rc = gpmc_cs_insert_mem(cs, base, size);
+		if (IS_ERR_VALUE(rc)) {
+			while (--cs >= 0)
+				if (gpmc_cs_mem_enabled(cs))
+					gpmc_cs_delete_mem(cs);
+			return rc;
+		}
 	}
+
+	return 0;
 }
 
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
+	int rc;
 	u32 l;
 	struct resource *res;
 
@@ -936,7 +944,13 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	dev_info(gpmc_dev, "GPMC revision %d.%d\n", GPMC_REVISION_MAJOR(l),
 		 GPMC_REVISION_MINOR(l));
 
-	gpmc_mem_init();
+	rc = gpmc_mem_init();
+	if (IS_ERR_VALUE(rc)) {
+		clk_disable_unprepare(gpmc_l3_clk);
+		clk_put(gpmc_l3_clk);
+		dev_err(gpmc_dev, "failed to reserve memory\n");
+		return rc;
+	}
 
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
-- 
1.7.9.5
 

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121017 07:43]:
> 
> On 10/16/2012 04:26 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
> >> Hi Tony,
> >>
> >> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> >>> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> >>>> From: hvaibhav@ti.com <hvaibhav@ti.com>
> >>>>
> >>>> 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.
> >>>
> >>> I'm applying this one into omap-for-v3.7-rc1/fixes-part2.
> >>>
> >>> Next time, please also cc linux-omap@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 ...
> > 
> > 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. 

OK thanks I'll update it.

Regards,

Tony

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Tony Lindgren]

* Jon Hunter <jon-hunter@ti.com> [121017 07:43]:
> 
> On 10/16/2012 04:26 PM, Tony Lindgren wrote:
> > * Jon Hunter <jon-hunter@ti.com> [121016 14:00]:
> >> Hi Tony,
> >>
> >> On 10/16/2012 12:48 PM, Tony Lindgren wrote:
> >>> * Richard Cochran <richardcochran@gmail.com> [121015 12:18]:
> >>>> From: hvaibhav at ti.com <hvaibhav@ti.com>
> >>>>
> >>>> 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.
> >>>
> >>> 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 ...
> > 
> > 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. 

OK thanks I'll update it.

Regards,

Tony

RE: [PATCH 4/5] net: cpsw: Add parent<->child relation support between cpsw and mdio

[Hiremath, Vaibhav]

On Tue, Oct 16, 2012 at 00:46:34, Richard Cochran wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> 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 <hvaibhav@ti.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 

Thanks for stepping ahead and submitting patches from my tree.

Thanks,
Vaibhav

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index df55e24..fb1a692 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -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");
> -- 
> 1.7.2.5
> 
> 

[PATCH 4/5] net: cpsw: Add parent<->child relation support between cpsw and mdio

[Hiremath, Vaibhav]

On Tue, Oct 16, 2012 at 00:46:34, Richard Cochran wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> 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 <hvaibhav@ti.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>
> ---
>  drivers/net/ethernet/ti/cpsw.c |   16 ++++++++++++++--
>  1 files changed, 14 insertions(+), 2 deletions(-)
> 

Thanks for stepping ahead and submitting patches from my tree.

Thanks,
Vaibhav

> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index df55e24..fb1a692 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -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");
> -- 
> 1.7.2.5
> 
> 

RE: [PATCH 3/5] net: davinci_mdio: Fix type mistake in calling runtime-pm api

[Hiremath, Vaibhav]

On Tue, Oct 16, 2012 at 00:46:33, Richard Cochran wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> 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 <hvaibhav@ti.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>


Thanks for submitting the patches, I couldn't able to do it for almost 2 
weeks now. I really appreciate it.

Thanks,
Vaibhav
> ---
>  drivers/net/ethernet/ti/davinci_mdio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 51a96db..ae74280 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -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);
> -- 
> 1.7.2.5
> 
> 

[PATCH 3/5] net: davinci_mdio: Fix type mistake in calling runtime-pm api

[Hiremath, Vaibhav]

On Tue, Oct 16, 2012 at 00:46:33, Richard Cochran wrote:
> From: Vaibhav Hiremath <hvaibhav@ti.com>
> 
> 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 <hvaibhav@ti.com>
> Cc: Mugunthan V N <mugunthanvnm@ti.com>


Thanks for submitting the patches, I couldn't able to do it for almost 2 
weeks now. I really appreciate it.

Thanks,
Vaibhav
> ---
>  drivers/net/ethernet/ti/davinci_mdio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/davinci_mdio.c b/drivers/net/ethernet/ti/davinci_mdio.c
> index 51a96db..ae74280 100644
> --- a/drivers/net/ethernet/ti/davinci_mdio.c
> +++ b/drivers/net/ethernet/ti/davinci_mdio.c
> @@ -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);
> -- 
> 1.7.2.5
> 
> 

RE: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
> 
> On 10/15/2012 02:16 PM, Richard Cochran wrote:
> > From: hvaibhav@ti.com <hvaibhav@ti.com>
> > 
> > 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?
> 

That's what exactly the above comment describes.

> 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.

> 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.

> 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)


> > 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.
> 
> 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?

> 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???


On other hand, Your patch is anyway required, as that I would consider as 
cleanup of existing code (in error handling).

Thanks,
Vaibhav

> Cheers
> Jon
> 

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
> 
> On 10/15/2012 02:16 PM, Richard Cochran wrote:
> > From: hvaibhav at ti.com <hvaibhav@ti.com>
> > 
> > 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?
> 

That's what exactly the above comment describes.

> 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.

> 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.

> 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)


> > 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.
> 
> 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?

> 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???


On other hand, Your patch is anyway required, as that I would consider as 
cleanup of existing code (in error handling).

Thanks,
Vaibhav

> Cheers
> Jon
> 

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 11:16 AM, Hiremath, Vaibhav wrote:
> On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
>>
>> On 10/15/2012 02:16 PM, Richard Cochran wrote:
>>> From: hvaibhav@ti.com <hvaibhav@ti.com>
>>>
>>> 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?
>>
> 
> 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.

>> 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 ...

> 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.

>> 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.
> 
>> 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)

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

>>> 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.
>>
>> 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?

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.

>> 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???

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

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 11:16 AM, Hiremath, Vaibhav wrote:
> On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
>>
>> On 10/15/2012 02:16 PM, Richard Cochran wrote:
>>> From: hvaibhav at ti.com <hvaibhav@ti.com>
>>>
>>> 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?
>>
> 
> 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.

>> 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 ...

> 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.

>> 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.
> 
>> 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)

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

>>> 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.
>>
>> 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?

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.

>> 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???

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

RE: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
> 
> On 10/18/2012 11:16 AM, Hiremath, Vaibhav wrote:
> > On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
> >>
> >> On 10/15/2012 02:16 PM, Richard Cochran wrote:
> >>> From: hvaibhav@ti.com <hvaibhav@ti.com>
> >>>
> >>> 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?
> >>
> > 
> > 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.
> 

Ok, may be my above statement was confusing. But the bottom line is,
We should GPMC without any pre-configuration (either at u-boot or ROM) will 
have this issue.


> >> 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 ...

Oops, it was typo mistake. I meant "is set to '1'"

> 
> > And same applies to OMAP3 family of devices.
> 
> For which boot-modes? All or just the gpmc boot-modes?
> 

That's reset value, and I believe it is applicable for all 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.
> 

OMAp3430 beagle board has NAND flash available over GPMC-CS0 interface.

> >> 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.
> > 
> >> 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)
> 
> Does the boot ROM get mapped to 0x0, when booting from ROM?
> 

I expect, it should be same as OMAP4.

> >>> 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.
> >>
> >> 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?
> 
> 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.
> 

If we want to think about all such use-cases, then yes, this patch is not 
required.

> >> 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???
> 
> 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.


Thanks,
Vaibhav

> Jon
> 
> 

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
> 
> On 10/18/2012 11:16 AM, Hiremath, Vaibhav wrote:
> > On Wed, Oct 17, 2012 at 01:17:56, Hunter, Jon wrote:
> >>
> >> On 10/15/2012 02:16 PM, Richard Cochran wrote:
> >>> From: hvaibhav at ti.com <hvaibhav@ti.com>
> >>>
> >>> 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?
> >>
> > 
> > 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.
> 

Ok, may be my above statement was confusing. But the bottom line is,
We should GPMC without any pre-configuration (either at u-boot or ROM) will 
have this issue.


> >> 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 ...

Oops, it was typo mistake. I meant "is set to '1'"

> 
> > And same applies to OMAP3 family of devices.
> 
> For which boot-modes? All or just the gpmc boot-modes?
> 

That's reset value, and I believe it is applicable for all 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.
> 

OMAp3430 beagle board has NAND flash available over GPMC-CS0 interface.

> >> 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.
> > 
> >> 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)
> 
> Does the boot ROM get mapped to 0x0, when booting from ROM?
> 

I expect, it should be same as OMAP4.

> >>> 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.
> >>
> >> 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?
> 
> 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.
> 

If we want to think about all such use-cases, then yes, this patch is not 
required.

> >> 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???
> 
> 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.


Thanks,
Vaibhav

> Jon
> 
> 

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
> On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:

...

>> 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.

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

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
> On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:

...

>> 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.

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

RE: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Fri, Oct 19, 2012 at 00:00:31, Hunter, Jon wrote:
> 
> On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
> > On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
> 
> ...
> 
> >> 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.
> 
> 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.

> 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...

> we just need to fix
> gpmc_mem_init().
> 

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

Thanks,
Vaibhav

> Jon
> 

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Hiremath, Vaibhav]

On Fri, Oct 19, 2012 at 00:00:31, Hunter, Jon wrote:
> 
> On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
> > On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
> 
> ...
> 
> >> 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.
> 
> 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.

> 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...

> we just need to fix
> gpmc_mem_init().
> 

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

Thanks,
Vaibhav

> Jon
> 

Re: [PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 01:39 PM, Hiremath, Vaibhav wrote:
> On Fri, Oct 19, 2012 at 00:00:31, Hunter, Jon wrote:
>>
>> On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
>>> On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
>>
>> ...
>>
>>>> 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.
>>
>> 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.

So we can agree is disagree on that ;-)

> 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.
> 
>> 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...
> 
>> we just need to fix
>> gpmc_mem_init().
>>
> 
> Agreed, and that's what your patch rightly doing it.

No. My patch does not fix the _actual_ bug, it is still there. Why do
you think that the probe is still failing for am335x? Without fixing it
am335x will never be able to support gpmc. So gpmc_mem_init() still
needs to be fixed.

Jon

[PATCH 1/5] ARM: OMAP2+: gpmc: Fix kernel BUG for DT boot mode

[Jon Hunter]

On 10/18/2012 01:39 PM, Hiremath, Vaibhav wrote:
> On Fri, Oct 19, 2012 at 00:00:31, Hunter, Jon wrote:
>>
>> On 10/18/2012 01:04 PM, Hiremath, Vaibhav wrote:
>>> On Thu, Oct 18, 2012 at 22:12:07, Hunter, Jon wrote:
>>
>> ...
>>
>>>> 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.
>>
>> 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.

So we can agree is disagree on that ;-)

> 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.
> 
>> 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...
> 
>> we just need to fix
>> gpmc_mem_init().
>>
> 
> Agreed, and that's what your patch rightly doing it.

No. My patch does not fix the _actual_ bug, it is still there. Why do
you think that the probe is still failing for am335x? Without fixing it
am335x will never be able to support gpmc. So gpmc_mem_init() still
needs to be fixed.

Jon