| Message ID | 1311160106-4898-6-git-send-email-tony.lin@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Tony Lin <tony.lin@freescale.com> writes: Hi, > old driver uses some hard coding for clks' name which could > not be used for mx28. So workaround these hard codings by > judging the cpu is mx28 or not. Sascha had some patches to solve this in a different way, avoiding to add yet an other cpu_is_* check for ahb clock. Don't know their current status. See: http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/040364.html http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/040365.html http://lists.infradead.org/pipermail/linux-arm-kernel/2011-February/040363.html Arnaud
> -----Original Message----- > From: Arnaud Patard [mailto:arnaud.patard@rtp-net.org] > Sent: Wednesday, July 20, 2011 7:06 PM > To: Lin Tony-B19295 > Cc: linux-usb@vger.kernel.org; koen.beel.barco@gmail.com; balbi@ti.com; > linux-arm-kernel@lists.infradead.org; Sascha Hauer > Subject: Re: [PATCH 5/7] ehci mxc: make it more flexible to be used for > mx28 > > Tony Lin <tony.lin@freescale.com> writes: > Hi, > > > old driver uses some hard coding for clks' name which could not be > > used for mx28. So workaround these hard codings by judging the cpu is > > mx28 or not. > > Sascha had some patches to solve this in a different way, avoiding to add > yet an other cpu_is_* check for ahb clock. Don't know their current > status. Yes, using cpu_is_* is really a bad method here :-) But I had to follow that bad method so far, hope we could see some change from sascha. > > See: > http://lists.infradead.org/pipermail/linux-arm-kernel/2011- > February/040364.html > http://lists.infradead.org/pipermail/linux-arm-kernel/2011- > February/040365.html > http://lists.infradead.org/pipermail/linux-arm-kernel/2011- > February/040363.html > > Arnaud
Hi, On Wed, Jul 20, 2011 at 07:08:24PM +0800, Tony Lin wrote: > @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) > } > > /* enable clocks */ > - priv->usbclk = clk_get(dev, "usb"); > - if (IS_ERR(priv->usbclk)) { > - ret = PTR_ERR(priv->usbclk); > - goto err_clk; > + if (!cpu_is_mx28()) { this should not be used in drivers, IMHO. > + priv->usbclk = clk_get(dev, "usb"); drivers should not have to care about clock names, are you sure your clkdev support is correct ?
> -----Original Message----- > From: Felipe Balbi [mailto:balbi@ti.com] > Sent: Wednesday, July 20, 2011 7:15 PM > To: Lin Tony-B19295 > Cc: linux-usb@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > balbi@ti.com; koen.beel.barco@gmail.com > Subject: Re: [PATCH 5/7] ehci mxc: make it more flexible to be used for > mx28 > > Hi, > > On Wed, Jul 20, 2011 at 07:08:24PM +0800, Tony Lin wrote: > > @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct > platform_device *pdev) > > } > > > > /* enable clocks */ > > - priv->usbclk = clk_get(dev, "usb"); > > - if (IS_ERR(priv->usbclk)) { > > - ret = PTR_ERR(priv->usbclk); > > - goto err_clk; > > + if (!cpu_is_mx28()) { > > this should not be used in drivers, IMHO. > > > + priv->usbclk = clk_get(dev, "usb"); > > drivers should not have to care about clock names, are you sure your > clkdev support is correct ? Yes, I agree with you about above two points. I feel the same as you. But I had to following the existing framework. I didn't add any new special to the driver. Just try some ways to workaround these ugly points. > > -- > balbi
Hi, On Wed, Jul 20, 2011 at 11:21:10AM +0000, Lin Tony-B19295 wrote: > > -----Original Message----- > > From: Felipe Balbi [mailto:balbi@ti.com] > > Sent: Wednesday, July 20, 2011 7:15 PM > > To: Lin Tony-B19295 > > Cc: linux-usb@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > balbi@ti.com; koen.beel.barco@gmail.com > > Subject: Re: [PATCH 5/7] ehci mxc: make it more flexible to be used for > > mx28 > > > > Hi, > > > > On Wed, Jul 20, 2011 at 07:08:24PM +0800, Tony Lin wrote: > > > @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct > > platform_device *pdev) > > > } > > > > > > /* enable clocks */ > > > - priv->usbclk = clk_get(dev, "usb"); > > > - if (IS_ERR(priv->usbclk)) { > > > - ret = PTR_ERR(priv->usbclk); > > > - goto err_clk; > > > + if (!cpu_is_mx28()) { > > > > this should not be used in drivers, IMHO. > > > > > + priv->usbclk = clk_get(dev, "usb"); > > > > drivers should not have to care about clock names, are you sure your > > clkdev support is correct ? > > Yes, I agree with you about above two points. I feel the same as you. > But I had to following the existing framework. I didn't add any new special to the driver. > Just try some ways to workaround these ugly points. but in that case, since you're already there... why not cleaning those things up before making your changes ?
> -----Original Message----- > From: Felipe Balbi [mailto:balbi@ti.com] > Sent: Wednesday, July 20, 2011 7:23 PM > To: Lin Tony-B19295 > Cc: balbi@ti.com; linux-usb@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; koen.beel.barco@gmail.com > Subject: Re: [PATCH 5/7] ehci mxc: make it more flexible to be used for > mx28 > > Hi, > > On Wed, Jul 20, 2011 at 11:21:10AM +0000, Lin Tony-B19295 wrote: > > > -----Original Message----- > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > Sent: Wednesday, July 20, 2011 7:15 PM > > > To: Lin Tony-B19295 > > > Cc: linux-usb@vger.kernel.org; linux-arm-kernel@lists.infradead.org; > > > balbi@ti.com; koen.beel.barco@gmail.com > > > Subject: Re: [PATCH 5/7] ehci mxc: make it more flexible to be used > > > for > > > mx28 > > > > > > Hi, > > > > > > On Wed, Jul 20, 2011 at 07:08:24PM +0800, Tony Lin wrote: > > > > @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct > > > platform_device *pdev) > > > > } > > > > > > > > /* enable clocks */ > > > > - priv->usbclk = clk_get(dev, "usb"); > > > > - if (IS_ERR(priv->usbclk)) { > > > > - ret = PTR_ERR(priv->usbclk); > > > > - goto err_clk; > > > > + if (!cpu_is_mx28()) { > > > > > > this should not be used in drivers, IMHO. > > > > > > > + priv->usbclk = clk_get(dev, "usb"); > > > > > > drivers should not have to care about clock names, are you sure your > > > clkdev support is correct ? > > > > Yes, I agree with you about above two points. I feel the same as you. > > But I had to following the existing framework. I didn't add any new > special to the driver. > > Just try some ways to workaround these ugly points. > > but in that case, since you're already there... why not cleaning those > things up before making your changes ? > Ok, I'll clean up these things. Thanks, balbi. > -- > balbi
Hi, On Wed, Jul 20, 2011 at 11:25:07AM +0000, Lin Tony-B19295 wrote: > > > But I had to following the existing framework. I didn't add any new > > special to the driver. > > > Just try some ways to workaround these ugly points. > > > > but in that case, since you're already there... why not cleaning those > > things up before making your changes ? > > > Ok, I'll clean up these things. Thanks, balbi. no problem ;-)
> Yes, using cpu_is_* is really a bad method here :-) > But I had to follow that bad method so far, I don't think you 'had to'... > hope we could see some change from sascha. ...because you could have asked him about the status of these patches. Regerds, Wolfram
On Wed, 20 Jul 2011, Tony Lin wrote: > old driver uses some hard coding for clks' name which could > not be used for mx28. So workaround these hard codings by > judging the cpu is mx28 or not. > add platform callback funtions in usb irq handler in the case > usb phy need to change its disconnect detector mode after usb > device is connected and disconnected. These callbacks also > could be used for other machines whose usb phy need such kind > of operations > > Signed-off-by: Tony Lin <tony.lin@freescale.com> ... > --- a/drivers/usb/host/ehci-mxc.c > +++ b/drivers/usb/host/ehci-mxc.c ... > +static irqreturn_t fsl_ehci_irq(struct usb_hcd *hcd) > +{ > + struct mxc_usbh_platform_data *pdata; > + struct ehci_hcd *ehci = hcd_to_ehci(hcd); > + u32 status; > + > + pdata = hcd->self.controller->platform_data; > + if (pdata->plt_get_usb_connect_status == NULL || \ > + pdata->plt_usb_disconnect_detect == NULL) > + goto out; > + > + spin_lock(&ehci->lock); Do you really need to use this spinlock? I don't see any reason for it. > + status = ehci_readl(ehci, &ehci->regs->status); > + if (status & STS_PCD) { > + if (pdata->plt_get_usb_connect_status()) > + pdata->plt_usb_disconnect_detect(true); > + else > + pdata->plt_usb_disconnect_detect(false); How about just this: pdata->plt_usb_disconnect_detect( pdata->plt_get_usb_connect_status()); > + } > + spin_unlock(&ehci->lock); > +out: > + return ehci_irq(hcd); > +} Alan Stern
On Wed, Jul 20, 2011 at 07:08:24PM +0800, Tony Lin wrote: > old driver uses some hard coding for clks' name which could > not be used for mx28. So workaround these hard codings by > judging the cpu is mx28 or not. > add platform callback funtions in usb irq handler in the case > usb phy need to change its disconnect detector mode after usb > device is connected and disconnected. These callbacks also > could be used for other machines whose usb phy need such kind > of operations > > Signed-off-by: Tony Lin <tony.lin@freescale.com> > --- > drivers/usb/host/Kconfig | 2 +- > drivers/usb/host/ehci-mxc.c | 57 +++++++++++++++++++++++++++++++----------- > 2 files changed, 43 insertions(+), 16 deletions(-) > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index ab085f1..6a5905b 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -139,7 +139,7 @@ config USB_EHCI_FSL > > config USB_EHCI_MXC > bool "Support for Freescale on-chip EHCI USB controller" > - depends on USB_EHCI_HCD && ARCH_MXC > + depends on USB_EHCI_HCD && (ARCH_MXC || ARCH_MXS) > select USB_EHCI_ROOT_HUB_TT > ---help--- > Variation of ARC USB block used in some Freescale chips. > diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c > index 0c058be..65e78cd 100644 > --- a/drivers/usb/host/ehci-mxc.c > +++ b/drivers/usb/host/ehci-mxc.c > @@ -23,9 +23,7 @@ > #include <linux/usb/otg.h> > #include <linux/usb/ulpi.h> > #include <linux/slab.h> > - > -#include <mach/mxc_ehci.h> > - > +#include <linux/fsl_devices.h> > #include <asm/mach-types.h> > > #define ULPI_VIEWPORT_OFFSET 0x170 > @@ -66,6 +64,30 @@ static int ehci_mxc_setup(struct usb_hcd *hcd) > return 0; > } > > +static irqreturn_t fsl_ehci_irq(struct usb_hcd *hcd) > +{ > + struct mxc_usbh_platform_data *pdata; > + struct ehci_hcd *ehci = hcd_to_ehci(hcd); > + u32 status; > + > + pdata = hcd->self.controller->platform_data; > + if (pdata->plt_get_usb_connect_status == NULL || \ This backslash is unnecessary > + pdata->plt_usb_disconnect_detect == NULL) > + goto out; > + > + spin_lock(&ehci->lock); > + status = ehci_readl(ehci, &ehci->regs->status); > + if (status & STS_PCD) { > + if (pdata->plt_get_usb_connect_status()) > + pdata->plt_usb_disconnect_detect(true); > + else > + pdata->plt_usb_disconnect_detect(false); > + } > + spin_unlock(&ehci->lock); > +out: > + return ehci_irq(hcd); > +} > + > static const struct hc_driver ehci_mxc_hc_driver = { > .description = hcd_name, > .product_desc = "Freescale On-Chip EHCI Host Controller", > @@ -74,7 +96,7 @@ static const struct hc_driver ehci_mxc_hc_driver = { > /* > * generic hardware linkage > */ > - .irq = ehci_irq, > + .irq = fsl_ehci_irq, > .flags = HCD_USB2 | HCD_MEMORY, > > /* > @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) > } > > /* enable clocks */ > - priv->usbclk = clk_get(dev, "usb"); > - if (IS_ERR(priv->usbclk)) { > - ret = PTR_ERR(priv->usbclk); > - goto err_clk; > + if (!cpu_is_mx28()) { > + priv->usbclk = clk_get(dev, "usb"); > + if (IS_ERR(priv->usbclk)) { > + ret = PTR_ERR(priv->usbclk); > + goto err_clk; > + } > + clk_enable(priv->usbclk); > } We should start and provide dummy clocks for usb and ahb clock on platform which do not have them. Then we can get rid of the cpu checks in the driver. > - clk_enable(priv->usbclk); > - > - if (!cpu_is_mx35() && !cpu_is_mx25()) { > + if (!cpu_is_mx35() && !cpu_is_mx25() && !cpu_is_mx28()) { > priv->ahbclk = clk_get(dev, "usb_ahb"); > if (IS_ERR(priv->ahbclk)) { > ret = PTR_ERR(priv->ahbclk); > @@ -272,8 +295,10 @@ err_clk_phy: > clk_put(priv->ahbclk); > } > err_clk_ahb: > - clk_disable(priv->usbclk); > - clk_put(priv->usbclk); > + if (priv->usbclk) { > + clk_disable(priv->usbclk); > + clk_put(priv->usbclk); > + } > err_clk: > iounmap(hcd->regs); > err_ioremap: > @@ -304,8 +329,10 @@ static int __exit ehci_mxc_drv_remove(struct platform_device *pdev) > usb_put_hcd(hcd); > platform_set_drvdata(pdev, NULL); > > - clk_disable(priv->usbclk); > - clk_put(priv->usbclk); > + if (priv->usbclk) { > + clk_disable(priv->usbclk); > + clk_put(priv->usbclk); > + } > if (priv->ahbclk) { > clk_disable(priv->ahbclk); > clk_put(priv->ahbclk); > -- > 1.7.0.4 > > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
> On Wed, 20 Jul 2011, Tony Lin wrote: > > > old driver uses some hard coding for clks' name which could not be > > used for mx28. So workaround these hard codings by judging the cpu is > > mx28 or not. > > add platform callback funtions in usb irq handler in the case usb phy > > need to change its disconnect detector mode after usb device is > > connected and disconnected. These callbacks also could be used for > > other machines whose usb phy need such kind of operations > > > > Signed-off-by: Tony Lin <tony.lin@freescale.com> > > ... > > > --- a/drivers/usb/host/ehci-mxc.c > > +++ b/drivers/usb/host/ehci-mxc.c > > ... > > > +static irqreturn_t fsl_ehci_irq(struct usb_hcd *hcd) { > > + struct mxc_usbh_platform_data *pdata; > > + struct ehci_hcd *ehci = hcd_to_ehci(hcd); > > + u32 status; > > + > > + pdata = hcd->self.controller->platform_data; > > + if (pdata->plt_get_usb_connect_status == NULL || \ > > + pdata->plt_usb_disconnect_detect == NULL) > > + goto out; > > + > > + spin_lock(&ehci->lock); > > Do you really need to use this spinlock? I don't see any reason for it. > I'll remove it. Thanks for pointing out. > > + status = ehci_readl(ehci, &ehci->regs->status); > > + if (status & STS_PCD) { > > + if (pdata->plt_get_usb_connect_status()) > > + pdata->plt_usb_disconnect_detect(true); > > + else > > + pdata->plt_usb_disconnect_detect(false); > > How about just this: > > pdata->plt_usb_disconnect_detect( > pdata->plt_get_usb_connect_status()); > That's better. > > + } > > + spin_unlock(&ehci->lock); > > +out: > > + return ehci_irq(hcd); > > +} > > Alan Stern >
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index ab085f1..6a5905b 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -139,7 +139,7 @@ config USB_EHCI_FSL config USB_EHCI_MXC bool "Support for Freescale on-chip EHCI USB controller" - depends on USB_EHCI_HCD && ARCH_MXC + depends on USB_EHCI_HCD && (ARCH_MXC || ARCH_MXS) select USB_EHCI_ROOT_HUB_TT ---help--- Variation of ARC USB block used in some Freescale chips. diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c index 0c058be..65e78cd 100644 --- a/drivers/usb/host/ehci-mxc.c +++ b/drivers/usb/host/ehci-mxc.c @@ -23,9 +23,7 @@ #include <linux/usb/otg.h> #include <linux/usb/ulpi.h> #include <linux/slab.h> - -#include <mach/mxc_ehci.h> - +#include <linux/fsl_devices.h> #include <asm/mach-types.h> #define ULPI_VIEWPORT_OFFSET 0x170 @@ -66,6 +64,30 @@ static int ehci_mxc_setup(struct usb_hcd *hcd) return 0; } +static irqreturn_t fsl_ehci_irq(struct usb_hcd *hcd) +{ + struct mxc_usbh_platform_data *pdata; + struct ehci_hcd *ehci = hcd_to_ehci(hcd); + u32 status; + + pdata = hcd->self.controller->platform_data; + if (pdata->plt_get_usb_connect_status == NULL || \ + pdata->plt_usb_disconnect_detect == NULL) + goto out; + + spin_lock(&ehci->lock); + status = ehci_readl(ehci, &ehci->regs->status); + if (status & STS_PCD) { + if (pdata->plt_get_usb_connect_status()) + pdata->plt_usb_disconnect_detect(true); + else + pdata->plt_usb_disconnect_detect(false); + } + spin_unlock(&ehci->lock); +out: + return ehci_irq(hcd); +} + static const struct hc_driver ehci_mxc_hc_driver = { .description = hcd_name, .product_desc = "Freescale On-Chip EHCI Host Controller", @@ -74,7 +96,7 @@ static const struct hc_driver ehci_mxc_hc_driver = { /* * generic hardware linkage */ - .irq = ehci_irq, + .irq = fsl_ehci_irq, .flags = HCD_USB2 | HCD_MEMORY, /* @@ -165,14 +187,15 @@ static int ehci_mxc_drv_probe(struct platform_device *pdev) } /* enable clocks */ - priv->usbclk = clk_get(dev, "usb"); - if (IS_ERR(priv->usbclk)) { - ret = PTR_ERR(priv->usbclk); - goto err_clk; + if (!cpu_is_mx28()) { + priv->usbclk = clk_get(dev, "usb"); + if (IS_ERR(priv->usbclk)) { + ret = PTR_ERR(priv->usbclk); + goto err_clk; + } + clk_enable(priv->usbclk); } - clk_enable(priv->usbclk); - - if (!cpu_is_mx35() && !cpu_is_mx25()) { + if (!cpu_is_mx35() && !cpu_is_mx25() && !cpu_is_mx28()) { priv->ahbclk = clk_get(dev, "usb_ahb"); if (IS_ERR(priv->ahbclk)) { ret = PTR_ERR(priv->ahbclk); @@ -272,8 +295,10 @@ err_clk_phy: clk_put(priv->ahbclk); } err_clk_ahb: - clk_disable(priv->usbclk); - clk_put(priv->usbclk); + if (priv->usbclk) { + clk_disable(priv->usbclk); + clk_put(priv->usbclk); + } err_clk: iounmap(hcd->regs); err_ioremap: @@ -304,8 +329,10 @@ static int __exit ehci_mxc_drv_remove(struct platform_device *pdev) usb_put_hcd(hcd); platform_set_drvdata(pdev, NULL); - clk_disable(priv->usbclk); - clk_put(priv->usbclk); + if (priv->usbclk) { + clk_disable(priv->usbclk); + clk_put(priv->usbclk); + } if (priv->ahbclk) { clk_disable(priv->ahbclk); clk_put(priv->ahbclk);
old driver uses some hard coding for clks' name which could not be used for mx28. So workaround these hard codings by judging the cpu is mx28 or not. add platform callback funtions in usb irq handler in the case usb phy need to change its disconnect detector mode after usb device is connected and disconnected. These callbacks also could be used for other machines whose usb phy need such kind of operations Signed-off-by: Tony Lin <tony.lin@freescale.com> --- drivers/usb/host/Kconfig | 2 +- drivers/usb/host/ehci-mxc.c | 57 +++++++++++++++++++++++++++++++----------- 2 files changed, 43 insertions(+), 16 deletions(-)