[RFC] Add support for am33xx which has two musb ports

[RFC] Add support for am33xx which has two musb ports

[Sebastian Andrzej Siewior]

Hi Felipe,

so with these two I can use the second port on my am335x-evm in hostmode.
After the second ports gets noticed by Linux I see over and over:
|musb_bus_suspend 2457: trying to suspend as a_wait_bcon while active

Which disappears once I plug in a device and does not come back after I
unplug it.

Port 0 in device seems to run as well. On the first time i see

| CAUTION: musb: Babble Interrupt Occurred
| g_ncm gadget: high-speed config #1: CDC Ethernet (NCM)

and on the second time I plug it in there is no such babble error. Maybe
there is some kind of init missing here.
Any comments?

Sebastian

[PATCH 1/2] musb: musb: dsps: support multiple instances

[Sebastian Andrzej Siewior]

If we specify right now more than once instance then we attempt to add
the platform device twice. The nop driver does not mind the second add
because it checks for it and returns without a word. At removal time a
segfault is likely because the first intance clean ups the phy and
second, well, goes boom.
This patchs adds two dummy device node to the am33xx for the dummy phy
which we have now. During probe time, we grab the phy based on the
device node if we have one. If not, we use the same hack we used so far.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
 arch/arm/boot/dts/am33xx.dtsi |  8 ++++++++
 drivers/usb/musb/musb_dsps.c  | 18 +++++++++++++-----
 include/linux/usb/musb.h      |  1 +
 3 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 8e1248f..30d0d45 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -341,6 +341,14 @@
 			port1-mode = <3>;
 			power = <250>;
 			ti,hwmods = "usb_otg_hs";
+			phys = <&nopphy0 &nopphy1>;
+		};
+
+		nopphy0: usbphy@0 {
+			compatible = "usb-nop-xceiv";
+		};
+		nopphy1: usbphy@1 {
+			compatible = "usb-nop-xceiv";
 		};
 
 		mac: ethernet@4a100000 {
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index e1b661d..d9ff390 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -415,9 +415,14 @@ static int dsps_musb_init(struct musb *musb)
 	/* mentor core register starts at offset of 0x400 from musb base */
 	musb->mregs += wrp->musb_core_offset;
 
-	/* NOP driver needs change if supporting dual instance */
-	usb_nop_xceiv_register();
-	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
+	if (!glue->dev->of_node) {
+		/* This hack works only for a single instance. */
+		usb_nop_xceiv_register();
+		musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
+	} else {
+		musb->xceiv = devm_usb_get_phy_by_phandle(glue->dev, "phys",
+				musb->config->instance);
+	}
 	if (IS_ERR_OR_NULL(musb->xceiv))
 		return -EPROBE_DEFER;
 
@@ -449,7 +454,8 @@ static int dsps_musb_init(struct musb *musb)
 	return 0;
 err0:
 	usb_put_phy(musb->xceiv);
-	usb_nop_xceiv_unregister();
+	if (!glue->dev->of_node)
+		usb_nop_xceiv_unregister();
 	return status;
 }
 
@@ -466,7 +472,8 @@ static int dsps_musb_exit(struct musb *musb)
 
 	/* NOP driver needs change if supporting dual instance */
 	usb_put_phy(musb->xceiv);
-	usb_nop_xceiv_unregister();
+	if (!glue->dev->of_node)
+		usb_nop_xceiv_unregister();
 
 	return 0;
 }
@@ -570,6 +577,7 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue, u8 id)
 		of_property_read_u32(np, res_name, (u32 *)&pdata->mode);
 		of_property_read_u32(np, "power", (u32 *)&pdata->power);
 		config->multipoint = of_property_read_bool(np, "multipoint");
+		config->instance = id;
 
 		pdata->config		= config;
 	}
diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
index 053c268..e027705 100644
--- a/include/linux/usb/musb.h
+++ b/include/linux/usb/musb.h
@@ -83,6 +83,7 @@ struct musb_hdrc_config {
 	u8		vendor_stat __deprecated; /* vendor status reg witdh */
 	u8		dma_req_chan __deprecated; /* bitmask for required dma channels */
 	u8		ram_bits;	/* ram address size */
+	u8		instance;
 
 	struct musb_hdrc_eps_bits *eps_bits __deprecated;
 #ifdef CONFIG_BLACKFIN
-- 
1.8.3.1

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

Re: [PATCH 1/2] musb: musb: dsps: support multiple instances

[Felipe Balbi]

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

On Mon, Jun 17, 2013 at 05:13:33PM +0200, Sebastian Andrzej Siewior wrote:
> If we specify right now more than once instance then we attempt to add
> the platform device twice. The nop driver does not mind the second add
> because it checks for it and returns without a word. At removal time a
> segfault is likely because the first intance clean ups the phy and
> second, well, goes boom.
> This patchs adds two dummy device node to the am33xx for the dummy phy
> which we have now. During probe time, we grab the phy based on the
> device node if we have one. If not, we use the same hack we used so far.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  arch/arm/boot/dts/am33xx.dtsi |  8 ++++++++
>  drivers/usb/musb/musb_dsps.c  | 18 +++++++++++++-----
>  include/linux/usb/musb.h      |  1 +
>  3 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
> index 8e1248f..30d0d45 100644
> --- a/arch/arm/boot/dts/am33xx.dtsi
> +++ b/arch/arm/boot/dts/am33xx.dtsi
> @@ -341,6 +341,14 @@
>  			port1-mode = <3>;
>  			power = <250>;
>  			ti,hwmods = "usb_otg_hs";
> +			phys = <&nopphy0 &nopphy1>;
> +		};
> +
> +		nopphy0: usbphy@0 {
> +			compatible = "usb-nop-xceiv";
> +		};
> +		nopphy1: usbphy@1 {
> +			compatible = "usb-nop-xceiv";
>  		};
>  
>  		mac: ethernet@4a100000 {
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index e1b661d..d9ff390 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -415,9 +415,14 @@ static int dsps_musb_init(struct musb *musb)
>  	/* mentor core register starts at offset of 0x400 from musb base */
>  	musb->mregs += wrp->musb_core_offset;
>  
> -	/* NOP driver needs change if supporting dual instance */
> -	usb_nop_xceiv_register();
> -	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
> +	if (!glue->dev->of_node) {
> +		/* This hack works only for a single instance. */
> +		usb_nop_xceiv_register();
> +		musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);

I think you can drop this altogether, am335x is DT-only anyway :-)

> +	} else {
> +		musb->xceiv = devm_usb_get_phy_by_phandle(glue->dev, "phys",
> +				musb->config->instance);
> +	}

after doing all this, perhaps we should re-factor phy_get into
musb_core.c, so that we can remove this sort of support from all glue
layers.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] musb: musb: dsps: support multiple instances

[Sebastian Andrzej Siewior]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 06/18/2013 10:27 AM, Felipe Balbi wrote:
>> diff --git a/drivers/usb/musb/musb_dsps.c
>> b/drivers/usb/musb/musb_dsps.c index e1b661d..d9ff390 100644 ---
>> a/drivers/usb/musb/musb_dsps.c +++
>> b/drivers/usb/musb/musb_dsps.c @@ -415,9 +415,14 @@ static int
>> dsps_musb_init(struct musb *musb) /* mentor core register starts
>> at offset of 0x400 from musb base */ musb->mregs +=
>> wrp->musb_core_offset;
>> 
>> -	/* NOP driver needs change if supporting dual instance */ -
>> usb_nop_xceiv_register(); -	musb->xceiv =
>> usb_get_phy(USB_PHY_TYPE_USB2); +	if (!glue->dev->of_node) { +
>> /* This hack works only for a single instance. */ +
>> usb_nop_xceiv_register(); +		musb->xceiv =
>> usb_get_phy(USB_PHY_TYPE_USB2);
> 
> I think you can drop this altogether, am335x is DT-only anyway :-)

Yes, but this is also used by:

$ git grep "musb-ti81xx"
arch/arm/mach-omap2/usb-musb.c:         name = "musb-ti81xx";
drivers/usb/musb/musb_dsps.c:           .name   = "musb-ti81xx",

Is that one also am33xx?

> 
>> +	} else { +		musb->xceiv =
>> devm_usb_get_phy_by_phandle(glue->dev, "phys", +
>> musb->config->instance); +	}
> 
> after doing all this, perhaps we should re-factor phy_get into 
> musb_core.c, so that we can remove this sort of support from all
> glue layers.

So keep this is as is and add later a phy_get into musb? I would have
check if everyone has a phy instance but I guess so.

Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJRwBucAAoJEHuW6BYqjPXRhK4P/3SjhljlSmWO0XN1pjXCu+L+
m6yxlz+epFfJSnXKX8FWmVQOahtQrbclv+KhcMo/KWeHj/epp30u924avkexwOc7
RRPCYEldhJdySNEkv14MpAan5WfOXNm8NuD7wCnrADahQxw6vQPoU3wnDIRKuW5i
b4kR+V2cSJatlg5tNeXiYR+LLygtSaBprBwcdUSeJ/BsivZBYx4TnwQ/LpMD24br
vORIbivMf+SKkJAmxBqHg956L38vL6C3iLYR0GHnk7a4x9cQ4Kk5jJVz6HbphYkJ
zzIrYojFXQ5SE7F4+jyEidUx2hQnNhNT6TAFGXG9fCpdNoi+3zhxBKUW1b7aIAHP
fbQJbNM4wRNBmxKvQPTLzlsnLTFbjfBb+vd1KBst8niIjLfhaYiV9oLiwAUilrvw
ugndxyzGgfKWbBDwcS6pVNwfP3eE54ZunOrRJnS/EF1GRrC1yaPZb0xuTxuMiqN2
97R9H1SPyUv9qJidB/EoJKFMyoq6fWlNTMTMGK+S80ctdQp+bOWdNcpJGaPx7XGc
8z+AKoQZapC/TunWqrJrNz6mcTh2ibyG+hfTD7FJULq9NYM7oAtAn9MOser4gwTB
YxsJnEIj29TNJQx4g7/29rr+vNKNX1DPNTgW7r3v4JFKPClx4wZpXLe9qOG48LKa
ciA5Sp9sWio6AwtyhEyl
=xL1i
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] musb: musb: dsps: support multiple instances

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 10:34:41AM +0200, Sebastian Andrzej Siewior wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA256
> 
> On 06/18/2013 10:27 AM, Felipe Balbi wrote:
> >> diff --git a/drivers/usb/musb/musb_dsps.c
> >> b/drivers/usb/musb/musb_dsps.c index e1b661d..d9ff390 100644 ---
> >> a/drivers/usb/musb/musb_dsps.c +++
> >> b/drivers/usb/musb/musb_dsps.c @@ -415,9 +415,14 @@ static int
> >> dsps_musb_init(struct musb *musb) /* mentor core register starts
> >> at offset of 0x400 from musb base */ musb->mregs +=
> >> wrp->musb_core_offset;
> >> 
> >> -	/* NOP driver needs change if supporting dual instance */ -
> >> usb_nop_xceiv_register(); -	musb->xceiv =
> >> usb_get_phy(USB_PHY_TYPE_USB2); +	if (!glue->dev->of_node) { +
> >> /* This hack works only for a single instance. */ +
> >> usb_nop_xceiv_register(); +		musb->xceiv =
> >> usb_get_phy(USB_PHY_TYPE_USB2);
> > 
> > I think you can drop this altogether, am335x is DT-only anyway :-)
> 
> Yes, but this is also used by:
> 
> $ git grep "musb-ti81xx"
> arch/arm/mach-omap2/usb-musb.c:         name = "musb-ti81xx";
> drivers/usb/musb/musb_dsps.c:           .name   = "musb-ti81xx",
> 
> Is that one also am33xx?

my bad, it's not :-)

> >> +	} else { +		musb->xceiv =
> >> devm_usb_get_phy_by_phandle(glue->dev, "phys", +
> >> musb->config->instance); +	}
> > 
> > after doing all this, perhaps we should re-factor phy_get into 
> > musb_core.c, so that we can remove this sort of support from all
> > glue layers.
> 
> So keep this is as is and add later a phy_get into musb? I would have
> check if everyone has a phy instance but I guess so.

sure, let's keep it as is :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/2] musb: musb: dsps: support multiple instances

[Sebastian Andrzej Siewior]

On 06/18/2013 10:27 AM, Felipe Balbi wrote:
>> --- a/arch/arm/boot/dts/am33xx.dtsi +++
>> b/arch/arm/boot/dts/am33xx.dtsi @@ -341,6 +341,14 @@ port1-mode =
>> <3>; power = <250>; ti,hwmods = "usb_otg_hs"; +			phys =
>> <&nopphy0 &nopphy1>; +		}; + +		nopphy0: usbphy@0 { +
>> compatible = "usb-nop-xceiv"; +		}; +		nopphy1: usbphy@1 { +
>> compatible = "usb-nop-xceiv"; };
>> 
>> mac: ethernet@4a100000 {

Any opinion on those phy nodes? Is it likely that we need a real phy
driver here and also expose a little more information like the memory
register or reset / vcc supply?

Sebastian

[PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Sebastian Andrzej Siewior]

There is no need to hardcode the number of instances here. It is better to
determine them at runtime. Even if the device provides two instances one
might only want to use one of them.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/usb/musb/musb_dsps.c | 33 +++++++++++++++++++++++----------
 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index d9ff390..0ac9934 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -110,8 +110,6 @@ struct dsps_musb_wrapper {
 	/* miscellaneous stuff */
 	u32		musb_core_offset;
 	u8		poll_seconds;
-	/* number of musb instances */
-	u8		instances;
 };
 
 /**
@@ -124,6 +122,7 @@ struct dsps_glue {
 	struct timer_list timer[2];	/* otg_workaround timer */
 	unsigned long last_timer[2];    /* last timer data for each instance */
 	u32 __iomem *usb_ctrl[2];
+	u8 instances;
 };
 
 #define	DSPS_AM33XX_CONTROL_MODULE_PHYS_0	0x44e10620
@@ -646,6 +645,23 @@ static int dsps_probe(struct platform_device *pdev)
 	}
 	platform_set_drvdata(pdev, glue);
 
+	i = 1;
+	do {
+		iomem = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!iomem) {
+			i--;
+			break;
+		}
+		i++;
+	} while (1);
+
+	glue->instances = i;
+	if (glue->instances < 1) {
+		dev_err(&pdev->dev, "Need atleast iomem for one port.\n");
+		ret = -EINVAL;
+		goto err1_5;
+	}
+
 	/* enable the usbss clocks */
 	pm_runtime_enable(&pdev->dev);
 
@@ -656,7 +672,7 @@ static int dsps_probe(struct platform_device *pdev)
 	}
 
 	/* create the child platform device for all instances of musb */
-	for (i = 0; i < wrp->instances ; i++) {
+	for (i = 0; i < glue->instances; i++) {
 		ret = dsps_create_musb_pdev(glue, i);
 		if (ret != 0) {
 			dev_err(&pdev->dev, "failed to create child pdev\n");
@@ -673,6 +689,7 @@ static int dsps_probe(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 err2:
 	pm_runtime_disable(&pdev->dev);
+err1_5:
 	kfree(glue->wrp);
 err1:
 	kfree(glue);
@@ -682,11 +699,10 @@ static int dsps_probe(struct platform_device *pdev)
 static int dsps_remove(struct platform_device *pdev)
 {
 	struct dsps_glue *glue = platform_get_drvdata(pdev);
-	const struct dsps_musb_wrapper *wrp = glue->wrp;
 	int i;
 
 	/* delete the child platform device */
-	for (i = 0; i < wrp->instances ; i++)
+	for (i = 0; i < glue->instances; i++)
 		platform_device_unregister(glue->musb[i]);
 
 	/* disable usbss clocks */
@@ -702,10 +718,9 @@ static int dsps_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev->parent);
 	struct dsps_glue *glue = platform_get_drvdata(pdev);
-	const struct dsps_musb_wrapper *wrp = glue->wrp;
 	int i;
 
-	for (i = 0; i < wrp->instances; i++)
+	for (i = 0; i < glue->instances; i++)
 		musb_dsps_phy_control(glue, i, 0);
 
 	return 0;
@@ -715,10 +730,9 @@ static int dsps_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev->parent);
 	struct dsps_glue *glue = platform_get_drvdata(pdev);
-	const struct dsps_musb_wrapper *wrp = glue->wrp;
 	int i;
 
-	for (i = 0; i < wrp->instances; i++)
+	for (i = 0; i < glue->instances; i++)
 		musb_dsps_phy_control(glue, i, 1);
 
 	return 0;
@@ -755,7 +769,6 @@ static const struct dsps_musb_wrapper ti81xx_driver_data = {
 	.rxep_bitmap		= (0xfffe << 16),
 	.musb_core_offset	= 0x400,
 	.poll_seconds		= 2,
-	.instances		= 1,
 };
 
 static const struct platform_device_id musb_dsps_id_table[] = {
-- 
1.8.3.1

Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Felipe Balbi]

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

Hi,

On Mon, Jun 17, 2013 at 05:13:34PM +0200, Sebastian Andrzej Siewior wrote:
> There is no need to hardcode the number of instances here. It is better to
> determine them at runtime. Even if the device provides two instances one
> might only want to use one of them.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
>  drivers/usb/musb/musb_dsps.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index d9ff390..0ac9934 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -110,8 +110,6 @@ struct dsps_musb_wrapper {
>  	/* miscellaneous stuff */
>  	u32		musb_core_offset;
>  	u8		poll_seconds;
> -	/* number of musb instances */
> -	u8		instances;
>  };
>  
>  /**
> @@ -124,6 +122,7 @@ struct dsps_glue {
>  	struct timer_list timer[2];	/* otg_workaround timer */
>  	unsigned long last_timer[2];    /* last timer data for each instance */
>  	u32 __iomem *usb_ctrl[2];
> +	u8 instances;
>  };
>  
>  #define	DSPS_AM33XX_CONTROL_MODULE_PHYS_0	0x44e10620
> @@ -646,6 +645,23 @@ static int dsps_probe(struct platform_device *pdev)
>  	}
>  	platform_set_drvdata(pdev, glue);
>  
> +	i = 1;
> +	do {
> +		iomem = platform_get_resource(pdev, IORESOURCE_MEM, i);

IIRC this index starts at zero, no ? Meaning that this should be:

i = 0
do {
	iomem = platform_get_resource(pdev, IORESOURCE_MEM, i);
	if (!iomem)
		break;

	i++;
} while (true);

glue->instances = i + 1;

Also, why are you getting the resource and doing nothing with it ? Is it
just to figure out the amount of instances ? Isn't there a DT helper to
count how many child nodes a certain node has ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Sebastian Andrzej Siewior]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 06/18/2013 10:31 AM, Felipe Balbi wrote:
> Hi,
Hi Felipe,

>> --- a/drivers/usb/musb/musb_dsps.c +++
>> b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct
>> dsps_musb_wrapper { @@ -646,6 +645,23 @@ static int
>> dsps_probe(struct platform_device *pdev) } 
>> platform_set_drvdata(pdev, glue);
>> 
>> +	i = 1; +	do { +		iomem = platform_get_resource(pdev,
>> IORESOURCE_MEM, i);
> 
> IIRC this index starts at zero, no ? Meaning that this should be:
> 
> i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); 
> if (!iomem) break;
> 
> i++; } while (true);
> 
> glue->instances = i + 1;

No. 3x ioadress i.e. 0, 1, 2 means two instances because we have 0 as
the glue io address and two musb port (1 and 2).

> Also, why are you getting the resource and doing nothing with it ?
> Is it just to figure out the amount of instances ? Isn't there a DT
> helper to

exactly, just to count.

> count how many child nodes a certain node has ?
This isn't exactly a child node, is it? There is of_get_child_count()
but this isn't a child, it is one property.

Sebastian
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQIcBAEBCAAGBQJRwB3LAAoJEHuW6BYqjPXRu1MQALwY/DHJWIKBZm18OdlJizmC
UZMfZCqwtObDtHcbUpMx5jFmF6jblVCY56Vx713oarRYwona65jRuCVp+BmPpvbh
rH7Vfdj7ZK02sSXD49lu/eM+StOoLAEuCdZV5cP2sQCQHzGY50eDVXynjFTmr6wH
nIC/MCPrTLGgH3bWLM1PshmIThbeovp/UupGXU0ABxThL3Dm2EAolBEWjvwAgbZy
CUL3OoUvb6SdJiPkI4ORUvHzyS78JDsvEj1zuCio13m+/FW3uvvQEjIWefS/lAmP
0AyoDf+6VTdAhJVcEev5RJNJpIzFMZDQYjarHO8q5NSwr5WN+h+QOEQjYdQsAfkf
Jv68nnj6mwE1HUv32EnTIu3il0101QU+dU4zDsMykCneMhY2GBXbRqjrfZXiTqKO
WjtJg0HeKhIrG6i5ffLt/4kfPIyXYYJriOqJf2nvlBf7/RqsX4OT5ThIW2BAV7Zx
7qY9CgwDdSjyBq1GpcIGbpBNvzrVGxJfLxNec73EOJmHdu6zCZWLWpujAQIvv3kf
XJWtPXYAVNB7lpSBaxJO6CeSE6YAy5CkxaL8EMeTcqKxwh/ntRw/YLt45MUb8Ao5
J82znl3hByO/9FNPi9KA4A6TDuFtRNNu5lm8o+PdQndTO6heMzJTs8AL/PqH1vRA
q2e6Wxg65ZTs4aUUyVFF
=f5YG
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 10:43:55AM +0200, Sebastian Andrzej Siewior wrote:
> >> --- a/drivers/usb/musb/musb_dsps.c +++
> >> b/drivers/usb/musb/musb_dsps.c @@ -110,8 +110,6 @@ struct
> >> dsps_musb_wrapper { @@ -646,6 +645,23 @@ static int
> >> dsps_probe(struct platform_device *pdev) } 
> >> platform_set_drvdata(pdev, glue);
> >> 
> >> +	i = 1; +	do { +		iomem = platform_get_resource(pdev,
> >> IORESOURCE_MEM, i);
> > 
> > IIRC this index starts at zero, no ? Meaning that this should be:
> > 
> > i = 0 do { iomem = platform_get_resource(pdev, IORESOURCE_MEM, i); 
> > if (!iomem) break;
> > 
> > i++; } while (true);
> > 
> > glue->instances = i + 1;
> 
> No. 3x ioadress i.e. 0, 1, 2 means two instances because we have 0 as
> the glue io address and two musb port (1 and 2).

heh, true.

> > Also, why are you getting the resource and doing nothing with it ?
> > Is it just to figure out the amount of instances ? Isn't there a DT
> > helper to
> 
> exactly, just to count.
> 
> > count how many child nodes a certain node has ?
> This isn't exactly a child node, is it? There is of_get_child_count()
> but this isn't a child, it is one property.

is it because we haven't added DTS support for musb core ? Eventually we
can/should add and convert this to of_get_child_count() then. For now,
we can go ahead with your approach.

We have such a large amount of function pointers to sort out anyway :-(

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Sebastian Andrzej Siewior]

On 06/18/2013 10:54 AM, Felipe Balbi wrote:
> Hi,

Hi Felipe,

>> This isn't exactly a child node, is it? There is
>> of_get_child_count() but this isn't a child, it is one property.
> 
> is it because we haven't added DTS support for musb core ?
> Eventually we can/should add and convert this to
> of_get_child_count() then. For now, we can go ahead with your
> approach.

 mother {
  	 child1 {
	 };
	 child1 {
	 };
 };

That would be a child imho.
For counting of phandles (for the number of assigned gpios for instance)
you would use
  of_count_phandle_with_args(node, "gpios", "#gpio-cell-size");

We don't have an equivalent for "#gpio-cell-size". In our case it would
be the sum of "#address-cells" and "#size-cells" minus 1 because we
don't have a phandle and abuse a function :)
Counting the number of "addresses" is special since bother its
members (address and size) can be either 32bit or 64bit in size.

> We have such a large amount of function pointers to sort out anyway
> :-(

If you want get the while() loop replaced by something else I have a
few suggestions:
- check for iomem(2). If it is there we have two instances. If not,
  just 1. This includes also the hope that we don't get a third port.

- loop over of_address_to_resource(). This is what of_device_alloc() is
  doing:
   if (of_can_translate_address(np))
         while (of_address_to_resource(np, num_reg, &temp_res) == 0)
                    num_reg++;
  So it is different.

Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] musb: musb: dsps: determine the number of instances at runtime

[Felipe Balbi]

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

Hi,

On Tue, Jun 18, 2013 at 11:24:40AM +0200, Sebastian Andrzej Siewior wrote:
> On 06/18/2013 10:54 AM, Felipe Balbi wrote:
> > Hi,
> 
> Hi Felipe,
> 
> >> This isn't exactly a child node, is it? There is
> >> of_get_child_count() but this isn't a child, it is one property.
> > 
> > is it because we haven't added DTS support for musb core ?
> > Eventually we can/should add and convert this to
> > of_get_child_count() then. For now, we can go ahead with your
> > approach.
> 
>  mother {
>   	 child1 {
> 	 };
> 	 child1 {
> 	 };
>  };
> 
> That would be a child imho.
> For counting of phandles (for the number of assigned gpios for instance)
> you would use
>   of_count_phandle_with_args(node, "gpios", "#gpio-cell-size");
> 
> We don't have an equivalent for "#gpio-cell-size". In our case it would
> be the sum of "#address-cells" and "#size-cells" minus 1 because we
> don't have a phandle and abuse a function :)
> Counting the number of "addresses" is special since bother its
> members (address and size) can be either 32bit or 64bit in size.
> 
> > We have such a large amount of function pointers to sort out anyway
> > :-(
> 
> If you want get the while() loop replaced by something else I have a
> few suggestions:
> - check for iomem(2). If it is there we have two instances. If not,
>   just 1. This includes also the hope that we don't get a third port.
> 
> - loop over of_address_to_resource(). This is what of_device_alloc() is
>   doing:
>    if (of_can_translate_address(np))
>          while (of_address_to_resource(np, num_reg, &temp_res) == 0)
>                     num_reg++;
>   So it is different.

let's keep it as it is, I think once we convert musb-core to dt, all
those issues will vanish and we will be able to use of_get_child_count()
anyway. Forget my noise :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]