[PATCH 0/5] USB: orion ehci patches for 2.6.35

[PATCH 0/5] USB: orion ehci patches for 2.6.35

[Saeed Bishara]

Saeed Bishara (5):
      USB: add support for phy init for the Dove SoC
      ARM: use the Dove USB phy setup
      USB: add clk structure for systems that support clkdev framework
      USB: manage the orion ehci clock using the clkdev
      USB: add power management support for orion ehci

 arch/arm/mach-dove/common.c                   |    2
 arch/arm/plat-orion/include/plat/ehci-orion.h |    1
 drivers/usb/core/hcd.h                        |    4
 drivers/usb/host/ehci-orion.c                 |  139 ++++++++++++++++++++----
 drivers/usb/host/ehci-orion.c                   |  108 ++++++++++++++++++
 5 files changed, 233 insertions(+), 21 deletions(-)

[PATCH 1/5] USB: add support for phy init for the Dove SoC

[Saeed Bishara]

This patch adds support for initializing the USB phy of the Marvell's Dove SoC.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 arch/arm/plat-orion/include/plat/ehci-orion.h |    1 +
 drivers/usb/host/ehci-orion.c                 |  139 +++++++++++++++++++++----
 2 files changed, 120 insertions(+), 20 deletions(-)

diff --git a/arch/arm/plat-orion/include/plat/ehci-orion.h b/arch/arm/plat-orion/include/plat/ehci-orion.h
index 4ec668e..67df73e 100644
--- a/arch/arm/plat-orion/include/plat/ehci-orion.h
+++ b/arch/arm/plat-orion/include/plat/ehci-orion.h
@@ -13,6 +13,7 @@
 
 enum orion_ehci_phy_ver {
 	EHCI_PHY_ORION,
+	EHCI_PHY_DOVE,
 	EHCI_PHY_DD,
 	EHCI_PHY_KW,
 	EHCI_PHY_NA,
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 0f87dc7..ba659e0 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -25,6 +25,7 @@
 #define USB_WINDOW_BASE(i)	(0x324 + ((i) << 4))
 #define USB_IPG			0x360
 #define USB_PHY_PWR_CTRL	0x400
+#define USB_PHY_PLL_CTRL	0x410
 #define USB_PHY_TX_CTRL		0x420
 #define USB_PHY_RX_CTRL		0x430
 #define USB_PHY_IVREF_CTRL	0x440
@@ -100,6 +101,93 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
 	wrl(USB_MODE, 0x13);
 }
 
+static void orion_usb_phy_v2_setup(struct usb_hcd *hcd)
+{
+	u32 reg;
+
+	/* The below GLs are according to the Orion Errata document */
+	/*
+	 * Clear interrupt cause and mask
+	 */
+	wrl(USB_CAUSE, 0);
+	wrl(USB_MASK, 0);
+
+	/*
+	 * Reset controller
+	 */
+	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
+	while (rdl(USB_CMD) & 0x2);
+
+	/* Clear bits 30 and 31. */
+	reg = rdl(USB_IPG);
+	reg &= ~(0x3 << 30);
+	/* Change bits[14:8] - IPG for non Start of Frame Packets
+	 * from 0x9(default) to 0xD
+	 */
+	reg &= ~(0x7f << 8);
+	reg |= 0xd << 8;
+	wrl(USB_IPG, reg);
+
+	/* VCO recalibrate */
+	wrl(USB_PHY_PLL_CTRL, rdl(USB_PHY_PLL_CTRL) | (1 << 21));
+	udelay(100);
+	wrl(USB_PHY_PLL_CTRL, rdl(USB_PHY_PLL_CTRL) & ~(1 << 21));
+
+	reg = rdl(USB_PHY_TX_CTRL);
+	reg |= 1 << 11; /* LOWVDD_EN */
+	reg |= 1 << 12; /* REG_RCAL_START */
+	/* bits[16:14]     (IMPCAL_VTH[2:0] = 101) */
+	reg &= ~(0x7 << 14);
+	reg |= (0x5 << 14);
+
+	reg &= ~(1 << 21); /* TX_BLOCK_EN */
+	reg &= ~(1 << 31); /* HS_STRESS_CTRL */
+	wrl(USB_PHY_TX_CTRL, reg);
+	udelay(100);
+	reg = rdl(USB_PHY_TX_CTRL);
+	reg &= ~(1 << 12); /* REG_RCAL_START */
+	wrl(USB_PHY_TX_CTRL, reg);
+
+	reg = rdl(USB_PHY_RX_CTRL);
+	reg &= ~(3 << 2); /* LPL_COEF */
+	reg |= 1 << 2;
+
+	reg &= ~(0xf << 4);
+	reg |= 0xc << 4; /* SQ_THRESH */
+
+	reg &= ~(3 << 15); /* REG_SQ_LENGTH */
+	reg |= 1 << 15;
+	reg &= ~(1 << 21); /* CDR_FASTLOCK_EN */
+	reg &= ~(3 << 26); /* EDGE_DET */
+	wrl(USB_PHY_RX_CTRL, reg);
+
+
+	/*
+	 * USB PHY IVREF Control
+	 * TXVDD12[9:8]=0x3
+	 */
+	wrl(USB_PHY_IVREF_CTRL, rdl(USB_PHY_IVREF_CTRL) | (0x3 << 8));
+
+
+	/*
+	 * GL# USB-3 GL# USB-9: USB PHY Test Group Control
+	 * REG_FIFO_SQ_RST[15]=0
+	 */
+	wrl(USB_PHY_TST_GRP_CTRL, rdl(USB_PHY_TST_GRP_CTRL) & ~0x8000);
+
+	/*
+	 * Stop and reset controller
+	 */
+	wrl(USB_CMD, rdl(USB_CMD) & ~0x1);
+	wrl(USB_CMD, rdl(USB_CMD) | 0x2);
+	while (rdl(USB_CMD) & 0x2);
+
+	/*
+	 * GL# USB-4 Setup USB Host mode
+	 */
+	wrl(USB_MODE, 0x3);
+}
+
 static int ehci_orion_setup(struct usb_hcd *hcd)
 {
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
@@ -190,6 +278,36 @@ ehci_orion_conf_mbus_windows(struct usb_hcd *hcd,
 	}
 }
 
+static void __devinit
+ehci_orion_hw_init(struct usb_hcd *hcd,  struct orion_ehci_data *pd)
+{
+	/*
+	 * (Re-)program MBUS remapping windows if we are asked to.
+	 */
+	if (pd != NULL && pd->dram != NULL)
+		ehci_orion_conf_mbus_windows(hcd, pd->dram);
+
+
+	/*
+	 * setup Orion USB controller.
+	 */
+	switch (pd->phy_version) {
+	case EHCI_PHY_NA:	/* dont change USB phy settings */
+		break;
+	case EHCI_PHY_ORION:
+		orion_usb_phy_v1_setup(hcd);
+		break;
+	case EHCI_PHY_DOVE:
+		orion_usb_phy_v2_setup(hcd);
+		break;
+	case EHCI_PHY_DD:
+	case EHCI_PHY_KW:
+	default:
+		printk(KERN_WARNING "Orion ehci -USB phy version isn't supported.\n");
+	}
+
+}
+
 static int __devinit ehci_orion_drv_probe(struct platform_device *pdev)
 {
 	struct orion_ehci_data *pd = pdev->dev.platform_data;
@@ -255,26 +373,7 @@ static int __devinit ehci_orion_drv_probe(struct platform_device *pdev)
 	hcd->has_tt = 1;
 	ehci->sbrn = 0x20;
 
-	/*
-	 * (Re-)program MBUS remapping windows if we are asked to.
-	 */
-	if (pd != NULL && pd->dram != NULL)
-		ehci_orion_conf_mbus_windows(hcd, pd->dram);
-
-	/*
-	 * setup Orion USB controller.
-	 */
-	switch (pd->phy_version) {
-	case EHCI_PHY_NA:	/* dont change USB phy settings */
-		break;
-	case EHCI_PHY_ORION:
-		orion_usb_phy_v1_setup(hcd);
-		break;
-	case EHCI_PHY_DD:
-	case EHCI_PHY_KW:
-	default:
-		printk(KERN_WARNING "Orion ehci -USB phy version isn't supported.\n");
-	}
+	ehci_orion_hw_init(hcd, pd);
 
 	err = usb_add_hcd(hcd, irq, IRQF_SHARED | IRQF_DISABLED);
 	if (err)
-- 
1.6.0.4

[PATCH 2/5] ARM: use the Dove USB phy setup

[Saeed Bishara]

The patch makes the dove SoC use the proper USB phy setup.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 arch/arm/mach-dove/common.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-dove/common.c b/arch/arm/mach-dove/common.c
index 5da2cf4..a83069c 100644
--- a/arch/arm/mach-dove/common.c
+++ b/arch/arm/mach-dove/common.c
@@ -74,7 +74,7 @@ void __init dove_map_io(void)
  ****************************************************************************/
 static struct orion_ehci_data dove_ehci_data = {
 	.dram		= &dove_mbus_dram_info,
-	.phy_version	= EHCI_PHY_NA,
+	.phy_version	= EHCI_PHY_DOVE,
 };
 
 static u64 ehci_dmamask = DMA_BIT_MASK(32);
-- 
1.6.0.4

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[Saeed Bishara]

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/usb/core/hcd.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/core/hcd.h b/drivers/usb/core/hcd.h
index a3cdb09..24ce9fe 100644
--- a/drivers/usb/core/hcd.h
+++ b/drivers/usb/core/hcd.h
@@ -22,6 +22,7 @@
 #ifdef __KERNEL__
 
 #include <linux/rwsem.h>
+#include <linux/clk.h>
 
 #define MAX_TOPO_LEVEL		6
 
@@ -110,6 +111,9 @@ struct usb_hcd {
 	u64			rsrc_start;	/* memory/io resource start */
 	u64			rsrc_len;	/* memory/io resource length */
 	unsigned		power_budget;	/* in mA, 0 = no limit */
+#if defined(CONFIG_HAVE_CLK)
+	struct clk		*clk;
+#endif
 
 	/* bandwidth_mutex should be taken before adding or removing
 	 * any new bus bandwidth constraints:
-- 
1.6.0.4

[PATCH 4/5] USB: manage the orion ehci clock using the clkdev

[Saeed Bishara]

This patch will enable the ehci controller's clock when needed.

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/usb/host/ehci-orion.c |   21 +++++++++++++++++++++
 1 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index ba659e0..92680ad 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -361,6 +361,14 @@ static int __devinit ehci_orion_drv_probe(struct platform_device *pdev)
 		goto err3;
 	}
 
+#if defined(CONFIG_HAVE_CLK)
+	hcd->clk = clk_get(&pdev->dev, NULL);
+	if (IS_ERR(hcd->clk))
+		dev_notice(&pdev->dev, "cannot get clkdev\n");
+	else
+		clk_enable(hcd->clk);
+#endif
+
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 	hcd->regs = regs;
@@ -382,6 +390,13 @@ static int __devinit ehci_orion_drv_probe(struct platform_device *pdev)
 	return 0;
 
 err4:
+#if defined(CONFIG_HAVE_CLK)
+	if (!IS_ERR(hcd->clk)) {
+		clk_disable(hcd->clk);
+		clk_put(hcd->clk);
+	}
+#endif
+
 	usb_put_hcd(hcd);
 err3:
 	iounmap(regs);
@@ -402,6 +417,12 @@ static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
 	iounmap(hcd->regs);
 	release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
 	usb_put_hcd(hcd);
+#if defined(CONFIG_HAVE_CLK)
+	if (!IS_ERR(hcd->clk)) {
+		clk_disable(hcd->clk);
+		clk_put(hcd->clk);
+	}
+#endif
 
 	return 0;
 }
-- 
1.6.0.4

[PATCH 5/5] USB: add power management support for orion ehci

[Saeed Bishara]

Signed-off-by: Saeed Bishara <saeed@marvell.com>
---
 drivers/usb/host/ehci-orion.c |   87 +++++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 92680ad..2008ef2 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -427,11 +427,98 @@ static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int ehci_orion_suspend(struct platform_device *pdev,
+			      pm_message_t state)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
+	unsigned long		flags;
+	int			rc = 0;
+
+	if (time_before(jiffies, ehci->next_statechange))
+		msleep(10);
+
+	/* Root hub was already suspended. Disable irq emission and
+	 * mark HW unaccessible, bail out if RH has been resumed. Use
+	 * the spinlock to properly synchronize with possible pending
+	 * RH suspend or resume activity.
+	 *
+	 * This is still racy as hcd->state is manipulated outside of
+	 * any locks =P But that will be a different fix.
+	 */
+	spin_lock_irqsave(&ehci->lock, flags);
+	if (hcd->state != HC_STATE_SUSPENDED) {
+		rc = -EINVAL;
+		goto bail;
+	}
+	ehci_writel(ehci, 0, &ehci->regs->intr_enable);
+	(void)ehci_readl(ehci, &ehci->regs->intr_enable);
+
+	/* make sure snapshot being resumed re-enumerates everything */
+	if (state.event == PM_EVENT_PRETHAW) {
+		ehci_halt(ehci);
+		ehci_reset(ehci);
+	}
+
+	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+bail:
+	spin_unlock_irqrestore(&ehci->lock, flags);
+
+	return rc;
+}
+static int ehci_orion_resume(struct platform_device *pdev)
+{
+	struct orion_ehci_data *pd = pdev->dev.platform_data;
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
+
+	if (time_before(jiffies, ehci->next_statechange))
+		msleep(100);
+
+	/* Mark hardware accessible again as we are out of D3 state by now */
+	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+
+	ehci_dbg(ehci, "lost power, restarting\n");
+	usb_root_hub_lost_power(hcd->self.root_hub);
+
+	/* Else reset, to cope with power loss or flush-to-storage
+	 * style "resume" having let BIOS kick in during reboot.
+	 */
+	(void) ehci_halt(ehci);
+	(void) ehci_reset(ehci);
+	ehci_orion_hw_init(hcd, pd);
+
+	/* emptying the schedule aborts any urbs */
+	spin_lock_irq(&ehci->lock);
+	if (ehci->reclaim)
+		end_unlink_async(ehci);
+	ehci_work(ehci);
+	spin_unlock_irq(&ehci->lock);
+
+	ehci_writel(ehci, ehci->command, &ehci->regs->command);
+	ehci_writel(ehci, FLAG_CF, &ehci->regs->configured_flag);
+	ehci_readl(ehci, &ehci->regs->command);	/* unblock posted writes */
+
+	/* here we "know" root ports should always stay powered */
+	ehci_port_power(ehci, 1);
+
+	hcd->state = HC_STATE_SUSPENDED;
+	return 0;
+}
+
+#else
+#define ehci_orion_suspend	NULL
+#define ehci_orion_resume	NULL
+#endif
+
 MODULE_ALIAS("platform:orion-ehci");
 
 static struct platform_driver ehci_orion_driver = {
 	.probe		= ehci_orion_drv_probe,
 	.remove		= __exit_p(ehci_orion_drv_remove),
+	.suspend	= ehci_orion_suspend,
+	.resume		= ehci_orion_resume,
 	.shutdown	= usb_hcd_platform_shutdown,
 	.driver.name	= "orion-ehci",
 };
-- 
1.6.0.4

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[Russell King - ARM Linux]

On Sun, May 02, 2010 at 05:22:40PM +0300, Saeed Bishara wrote:
> @@ -110,6 +111,9 @@ struct usb_hcd {
>  	u64			rsrc_start;	/* memory/io resource start */
>  	u64			rsrc_len;	/* memory/io resource length */
>  	unsigned		power_budget;	/* in mA, 0 = no limit */
> +#if defined(CONFIG_HAVE_CLK)
> +	struct clk		*clk;
> +#endif

We have other hci's using the clk API, why do we need to add this for
them too?  In other words, why can't the orion HCI driver work like
the other HCI drivers such as ohci-pxa27x.c or ehci-omap.c ?

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[saeed bishara]

On Sun, May 2, 2010 at 5:36 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, May 02, 2010 at 05:22:40PM +0300, Saeed Bishara wrote:
>> @@ -110,6 +111,9 @@ struct usb_hcd {
>> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_start; ? ? /* memory/io resource start */
>> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_len; ? ? ? /* memory/io resource length */
>> ? ? ? unsigned ? ? ? ? ? ? ? ?power_budget; ? /* in mA, 0 = no limit */
>> +#if defined(CONFIG_HAVE_CLK)
>> + ? ? struct clk ? ? ? ? ? ? ?*clk;
>> +#endif
>
> We have other hci's using the clk API, why do we need to add this for
> them too? ?In other words, why can't the orion HCI driver work like
> the other HCI drivers such as ohci-pxa27x.c or ehci-omap.c ?
>
if most of those drivers need clk structure then why not to add it to
the usb_hcd which hold the common stuff?
if I get approval for this then we can change the other drivers to use
this clk instead of each one having its own variable.

saeed

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[Russell King - ARM Linux]

On Sun, May 02, 2010 at 06:05:32PM +0300, saeed bishara wrote:
> On Sun, May 2, 2010 at 5:36 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Sun, May 02, 2010 at 05:22:40PM +0300, Saeed Bishara wrote:
> >> @@ -110,6 +111,9 @@ struct usb_hcd {
> >> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_start; ? ? /* memory/io resource start */
> >> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_len; ? ? ? /* memory/io resource length */
> >> ? ? ? unsigned ? ? ? ? ? ? ? ?power_budget; ? /* in mA, 0 = no limit */
> >> +#if defined(CONFIG_HAVE_CLK)
> >> + ? ? struct clk ? ? ? ? ? ? ?*clk;
> >> +#endif
> >
> > We have other hci's using the clk API, why do we need to add this for
> > them too? ?In other words, why can't the orion HCI driver work like
> > the other HCI drivers such as ohci-pxa27x.c or ehci-omap.c ?
> >
> if most of those drivers need clk structure then why not to add it to
> the usb_hcd which hold the common stuff?

drivers/usb/host/ohci-s3c2410.c:	2
drivers/usb/host/ohci-omap.c:		2
drivers/usb/host/r8a66597-hcd.c:	1
drivers/usb/host/ehci-mxc.c:		2
drivers/usb/host/ohci-da8xx.c:		2
drivers/usb/host/ohci-pxa27x.c:		1
drivers/usb/host/ohci-pnx4008.c:	1
drivers/usb/host/imx21-hcd.c:		1
drivers/usb/host/ohci-ep93xx.c:		1
drivers/usb/host/ehci-atmel.c:		2
drivers/usb/host/ehci-omap.c:		5
drivers/usb/host/ohci-at91.c:		2

So, five drivers need one clock, six drivers need two clocks, and one
driver needs five clocks.  So maybe you should be catering for the
common case by providing two struct clk's, or maybe catering for the
maximal case of five clocks?

> if I get approval for this then we can change the other drivers to use
> this clk instead of each one having its own variable.

That wasn't covered in the original patch - who gets to create these
patches, and how soon will they be created after 'approval' ?

Having these patches with this patch submission may actually help your
case by showing the benefits of such a cleanup - eg, if it eliminates
private driver data structures entirely, it's definitely worth it.

However, if it results in one clk structure in one structure and a
bunch of other clk structures elsewhere, imho it makes very little
sense - I'd go as far as to suggest that it creates confusion.

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[saeed bishara]

On Sun, May 2, 2010 at 6:14 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, May 02, 2010 at 06:05:32PM +0300, saeed bishara wrote:
>> On Sun, May 2, 2010 at 5:36 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> wrote:
>> > On Sun, May 02, 2010 at 05:22:40PM +0300, Saeed Bishara wrote:
>> >> @@ -110,6 +111,9 @@ struct usb_hcd {
>> >> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_start; ? ? /* memory/io resource start */
>> >> ? ? ? u64 ? ? ? ? ? ? ? ? ? ? rsrc_len; ? ? ? /* memory/io resource length */
>> >> ? ? ? unsigned ? ? ? ? ? ? ? ?power_budget; ? /* in mA, 0 = no limit */
>> >> +#if defined(CONFIG_HAVE_CLK)
>> >> + ? ? struct clk ? ? ? ? ? ? ?*clk;
>> >> +#endif
>> >
>> > We have other hci's using the clk API, why do we need to add this for
>> > them too? ?In other words, why can't the orion HCI driver work like
>> > the other HCI drivers such as ohci-pxa27x.c or ehci-omap.c ?
>> >
>> if most of those drivers need clk structure then why not to add it to
>> the usb_hcd which hold the common stuff?
>
> drivers/usb/host/ohci-s3c2410.c: ? ? ? ?2
> drivers/usb/host/ohci-omap.c: ? ? ? ? ? 2
> drivers/usb/host/r8a66597-hcd.c: ? ? ? ?1
> drivers/usb/host/ehci-mxc.c: ? ? ? ? ? ?2
> drivers/usb/host/ohci-da8xx.c: ? ? ? ? ?2
> drivers/usb/host/ohci-pxa27x.c: ? ? ? ? 1
> drivers/usb/host/ohci-pnx4008.c: ? ? ? ?1
> drivers/usb/host/imx21-hcd.c: ? ? ? ? ? 1
> drivers/usb/host/ohci-ep93xx.c: ? ? ? ? 1
> drivers/usb/host/ehci-atmel.c: ? ? ? ? ?2
> drivers/usb/host/ehci-omap.c: ? ? ? ? ? 5
> drivers/usb/host/ohci-at91.c: ? ? ? ? ? 2
>
> So, five drivers need one clock, six drivers need two clocks, and one
> driver needs five clocks. ?So maybe you should be catering for the
> common case by providing two struct clk's, or maybe catering for the
> maximal case of five clocks?
well, I think that those drivers that have more than one clk can be
redesigned by adding virtual clk for the the usb host, and the clk
implementation for that soc should manage all the underlying physical
clocks. I've looked at the omap,at91 and atmel, and it looks to me
that this is doable. you see can see that the clk stuff in those
driver has nothing to do with usb itself. what do you think?

>
> Having these patches with this patch submission may actually help your
> case by showing the benefits of such a cleanup - eg, if it eliminates
> private driver data structures entirely, it's definitely worth it.
your right, I have had to send kind of RFC for this issue.
>

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[Russell King - ARM Linux]

On Sun, May 02, 2010 at 06:21:52PM +0300, saeed bishara wrote:
> On Sun, May 2, 2010 at 6:14 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > drivers/usb/host/ohci-s3c2410.c: ? ? ? ?2
> > drivers/usb/host/ohci-omap.c: ? ? ? ? ? 2
> > drivers/usb/host/r8a66597-hcd.c: ? ? ? ?1
> > drivers/usb/host/ehci-mxc.c: ? ? ? ? ? ?2
> > drivers/usb/host/ohci-da8xx.c: ? ? ? ? ?2
> > drivers/usb/host/ohci-pxa27x.c: ? ? ? ? 1
> > drivers/usb/host/ohci-pnx4008.c: ? ? ? ?1
> > drivers/usb/host/imx21-hcd.c: ? ? ? ? ? 1
> > drivers/usb/host/ohci-ep93xx.c: ? ? ? ? 1
> > drivers/usb/host/ehci-atmel.c: ? ? ? ? ?2
> > drivers/usb/host/ehci-omap.c: ? ? ? ? ? 5
> > drivers/usb/host/ohci-at91.c: ? ? ? ? ? 2
> >
> > So, five drivers need one clock, six drivers need two clocks, and one
> > driver needs five clocks. ?So maybe you should be catering for the
> > common case by providing two struct clk's, or maybe catering for the
> > maximal case of five clocks?
> well, I think that those drivers that have more than one clk can be
> redesigned by adding virtual clk for the the usb host, and the clk
> implementation for that soc should manage all the underlying physical
> clocks. I've looked at the omap,at91 and atmel, and it looks to me
> that this is doable. you see can see that the clk stuff in those
> driver has nothing to do with usb itself. what do you think?

Not happy with this for two reasons:
1. You're assuming that they can be managed as one entity; that doesn't seem
   true for some of the drivers.
2. I don't see any reason to force this complication into the clk layer for
   these platforms when they don't actually need it.

Due to (1), using your current approach will result in those drivers
scattering their clks across two structures, which as I've said in my
previous email would be confusing.

So I don't think this approach makes much sense.

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[saeed bishara]

>> well, I think that those drivers that have more than one clk can be
>> redesigned by adding virtual clk for the the usb host, and the clk
>> implementation for that soc should manage all the underlying physical
>> clocks. I've looked at the omap,at91 and atmel, and it looks to me
>> that this is doable. you see can see that the clk stuff in those
>> driver has nothing to do with usb itself. what do you think?
>
> Not happy with this for two reasons:
> 1. You're assuming that they can be managed as one entity; that doesn't seem
> ? true for some of the drivers.
can you please give an example?

saeed

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[Russell King - ARM Linux]

On Sun, May 02, 2010 at 06:54:15PM +0300, saeed bishara wrote:
> >> well, I think that those drivers that have more than one clk can be
> >> redesigned by adding virtual clk for the the usb host, and the clk
> >> implementation for that soc should manage all the underlying physical
> >> clocks. I've looked at the omap,at91 and atmel, and it looks to me
> >> that this is doable. you see can see that the clk stuff in those
> >> driver has nothing to do with usb itself. what do you think?
> >
> > Not happy with this for two reasons:
> > 1. You're assuming that they can be managed as one entity; that doesn't seem
> > ? true for some of the drivers.
> can you please give an example?

drivers/usb/host/ohci-da8xx.c has two clocks, and needs to know the
configuration of the USB block to know whether to manage the USB2.0
clock:

        cfgchip2 = __raw_readl(CFGCHIP2);
        if (on) {
                clk_enable(usb11_clk);

                /*
                 * If USB 1.1 reference clock is sourced from USB 2.0 PHY, we
                 * need to enable the USB 2.0 module clocking, start its PHY,
                 * and not allow it to stop the clock during USB 2.0 suspend.
                 */
                if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX)) {
                        clk_enable(usb20_clk);

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[saeed bishara]

On Sun, May 2, 2010 at 7:05 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, May 02, 2010 at 06:54:15PM +0300, saeed bishara wrote:
>> >> well, I think that those drivers that have more than one clk can be
>> >> redesigned by adding virtual clk for the the usb host, and the clk
>> >> implementation for that soc should manage all the underlying physical
>> >> clocks. I've looked at the omap,at91 and atmel, and it looks to me
>> >> that this is doable. you see can see that the clk stuff in those
>> >> driver has nothing to do with usb itself. what do you think?
>> >
>> > Not happy with this for two reasons:
>> > 1. You're assuming that they can be managed as one entity; that doesn't seem
>> > ? true for some of the drivers.
>> can you please give an example?
>
> drivers/usb/host/ohci-da8xx.c has two clocks, and needs to know the
> configuration of the USB block to know whether to manage the USB2.0
> clock:
>
> ? ? ? ?cfgchip2 = __raw_readl(CFGCHIP2);
> ? ? ? ?if (on) {
> ? ? ? ? ? ? ? ?clk_enable(usb11_clk);
>
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * If USB 1.1 reference clock is sourced from USB 2.0 PHY, we
> ? ? ? ? ? ? ? ? * need to enable the USB 2.0 module clocking, start its PHY,
> ? ? ? ? ? ? ? ? * and not allow it to stop the clock during USB 2.0 suspend.
> ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ?if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX)) {
> ? ? ? ? ? ? ? ? ? ? ? ?clk_enable(usb20_clk);
>
David, can I've your opinion about this patch? adding the clk struct
to the hcd struct will save the orion-ehci driver from having private
data structure.

[PATCH 3/5] USB: add clk structure for systems that support clkdev framework

[saeed bishara]

On Sun, May 2, 2010 at 7:05 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sun, May 02, 2010 at 06:54:15PM +0300, saeed bishara wrote:
>> >> well, I think that those drivers that have more than one clk can be
>> >> redesigned by adding virtual clk for the the usb host, and the clk
>> >> implementation for that soc should manage all the underlying physical
>> >> clocks. I've looked at the omap,at91 and atmel, and it looks to me
>> >> that this is doable. you see can see that the clk stuff in those
>> >> driver has nothing to do with usb itself. what do you think?
>> >
>> > Not happy with this for two reasons:
>> > 1. You're assuming that they can be managed as one entity; that doesn't seem
>> > ? true for some of the drivers.
>> can you please give an example?
>
> drivers/usb/host/ohci-da8xx.c has two clocks, and needs to know the
> configuration of the USB block to know whether to manage the USB2.0
> clock:
>
> ? ? ? ?cfgchip2 = __raw_readl(CFGCHIP2);
> ? ? ? ?if (on) {
> ? ? ? ? ? ? ? ?clk_enable(usb11_clk);
>
> ? ? ? ? ? ? ? ?/*
> ? ? ? ? ? ? ? ? * If USB 1.1 reference clock is sourced from USB 2.0 PHY, we
> ? ? ? ? ? ? ? ? * need to enable the USB 2.0 module clocking, start its PHY,
> ? ? ? ? ? ? ? ? * and not allow it to stop the clock during USB 2.0 suspend.
> ? ? ? ? ? ? ? ? */
> ? ? ? ? ? ? ? ?if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX)) {
> ? ? ? ? ? ? ? ? ? ? ? ?clk_enable(usb20_clk);
>
guys, can I've your opinion about this patch? adding the clk struct
to the hcd struct will save the orion-ehci driver from having private
data structure.

saeed