by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v2 01/18] usb: host: xhci-plat: Sort the headers in alphabetic order

On Fri, Apr 25, 2014 at 04:06:59PM +0200, Gregory CLEMENT wrote:
> Sorting the headers in alphabetic order will help to reduce the conflict
> when adding new headers later.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---

Acked-by: Felipe Balbi <[email protected]>

> drivers/usb/host/xhci-plat.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 151901ce1ba9..f5351af4b2c5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -11,11 +11,11 @@
> * version 2 as published by the Free Software Foundation.
> */
>
> -#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> #include <linux/module.h>
> -#include <linux/slab.h>
> #include <linux/of.h>
> -#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
>
> #include "xhci.h"
>
> --
> 1.8.1.2
>

--
balbi

Attachments:

(No filename) (986.00 B)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-04-25 15:02:58

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v2 02/18] usb: host: xhci-plat: Add clocks support

Hi,

On Fri, Apr 25, 2014 at 04:07:00PM +0200, Gregory CLEMENT wrote:
> Some platform (such as the Armada 38x ones) can gate the clock of
> their USB controller. This patch add the support for the clock, by
> enabling them during probe and disabling them on remove.
>
> As not all platforms have clock support then enabling and disabling
> the clocks have been placed in separate functions. Then if the clocks
> are not supported we still can use the same calls, and there is no
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> drivers/usb/host/xhci-plat.c | 52 ++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 50 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index f5351af4b2c5..bb5d563f729c 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -11,6 +11,7 @@
> * version 2 as published by the Free Software Foundation.
> */
>
> +#include <linux/clk.h>
> #include <linux/dma-mapping.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -85,6 +86,42 @@ static const struct hc_driver xhci_plat_xhci_driver = {
> .bus_resume = xhci_bus_resume,
> };
>
> +#if defined(CONFIG_HAVE_CLK)

I don't think this ifdef is necessary, clk api provides stubs.

> +static int try_enable_clk(struct platform_device *pdev)

prepend this with xhci_plat_ as all other definitions here.

> +{
> + struct clk *clk = devm_clk_get(&pdev->dev, NULL);

you need to create a private xhci_plat structure and save a clock
pointer there.

> +
> + /* Not all platforms have a clk so it is not an error if the clock
> + does not exists. */

comment style is wrong.

> + if (!IS_ERR(clk))

even though some don't like it, the clk API is safe against NULL
pointers, one trick you could use is to set the clk pointer to NULL when
you fail to get it, then you'll have a single place where you check for
IS_ERR().

> + if (clk_prepare_enable(clk))
> + return -ENODEV;
> + return 0;

these three lines could be rewritten as:

return clk_prepare_enable(clk);

> +}
> +
> +static int try_disable_clk(struct platform_device *pdev)
> +{
> + struct clk *clk = devm_clk_get(&pdev->dev, NULL);
> +
> + /* Not all platforms have a clk so it is not an error if the clock
> + does not exists. */
> + if (!IS_ERR(clk))
> + clk_disable_unprepare(clk);

similar comments to previous function.

> static int xhci_plat_probe(struct platform_device *pdev)
> {
> const struct hc_driver *driver;
> @@ -107,6 +144,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (!res)
> return -ENODEV;
>
> + ret = try_enable_clk(pdev);
> + if (ret)
> + return ret;
> +
> /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (ret)
> @@ -117,8 +158,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
> dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>
> hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
> - if (!hcd)
> - return -ENOMEM;
> + if (!hcd) {
> + ret = -ENOMEM;
> + goto disable_clk;
> + }
>
> hcd->rsrc_start = res->start;
> hcd->rsrc_len = resource_size(res);
> @@ -182,6 +225,9 @@ release_mem_region:
> put_hcd:
> usb_put_hcd(hcd);
>
> +disable_clk:
> + try_disable_clk(pdev);
> +
> return ret;
> }
>
> @@ -199,6 +245,8 @@ static int xhci_plat_remove(struct platform_device *dev)
> usb_put_hcd(hcd);
> kfree(xhci);
>
> + try_disable_clk(dev);

you never call clk_put(). what gives ?

--
balbi

Attachments:

(No filename) (3.49 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-04-25 15:05:26

On Fri, Apr 25, 2014 at 06:07:05PM +0200, Andrew Lunn wrote:
> On Fri, Apr 25, 2014 at 04:07:12PM +0200, Gregory CLEMENT wrote:
> > The Armada 375 SoC comes with an USB2 host and device controller and
> > an USB3 controller. The USB cluster control register allows to manage
> > common features of both USB controllers.
> >
> > Signed-off-by: Gregory CLEMENT <[email protected]>
> > ---
> > arch/arm/mach-mvebu/Makefile | 2 +-
> > arch/arm/mach-mvebu/usb-cluster.c | 96 +++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 97 insertions(+), 1 deletion(-)
> > create mode 100644 arch/arm/mach-mvebu/usb-cluster.c
> >
> > diff --git a/arch/arm/mach-mvebu/Makefile b/arch/arm/mach-mvebu/Makefile
> > index a63e43b6b451..dec05e7e1802 100644
> > --- a/arch/arm/mach-mvebu/Makefile
> > +++ b/arch/arm/mach-mvebu/Makefile
> > @@ -4,7 +4,7 @@ ccflags-$(CONFIG_ARCH_MULTIPLATFORM) := -I$(srctree)/$(src)/include \
> > AFLAGS_coherency_ll.o := -Wa,-march=armv7-a
> >
> > obj-y += system-controller.o mvebu-soc-id.o
> > -obj-$(CONFIG_MACH_MVEBU_V7) += board-v7.o
> > +obj-$(CONFIG_MACH_MVEBU_V7) += board-v7.o usb-cluster.o
> > obj-$(CONFIG_MACH_DOVE) += dove.o
> > obj-$(CONFIG_ARCH_MVEBU) += coherency.o coherency_ll.o pmsu.o
> > obj-$(CONFIG_SMP) += platsmp.o headsmp.o
> > diff --git a/arch/arm/mach-mvebu/usb-cluster.c b/arch/arm/mach-mvebu/usb-cluster.c
> > new file mode 100644
> > index 000000000000..4c15d282db23
> > --- /dev/null
> > +++ b/arch/arm/mach-mvebu/usb-cluster.c
> > @@ -0,0 +1,96 @@
> > +/*
> > + * USB cluster support for Armada 375 platform.
> > + *
> > + * Copyright (C) 2014 Marvell
> > + *
> > + * Gregory CLEMENT <[email protected]>
> > + *
> > + * This file is licensed under the terms of the GNU General Public
> > + * License version 2. This program is licensed "as is" without any
> > + * warranty of any kind, whether express or implied.
> > + *
> > + * Armada 375 comes with an USB2 host and device controller and an
> > + * USB3 controller. The USB cluster control register allows to manage
> > + * common features of both USB controller.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/init.h>
> > +#include <linux/of_address.h>
> > +#include <linux/io.h>
> > +#include <linux/slab.h>
> > +
> > +#define USB2_PHY_CONFIG_ENABLE BIT(0) /* active low */
> > +
> > +static struct of_device_id of_usb_cluster_table[] = {
> > + { .compatible = "marvell,armada-375-usb-cluster", },
> > + { /* end of list */ },
> > +};
> > +
> > +static int __init mvebu_usb_cluster_init(void)
> > +{
> > + struct device_node *np;
> > +
> > + np = of_find_matching_node(NULL, of_usb_cluster_table);
> > + if (np) {
> > + void __iomem *usb_cluster_base;
> > + u32 reg;
> > + struct device_node *ehci_node, *xhci_node;
> > + struct property *ehci_status;
> > + bool use_usb3 = false;
> > +
> > + usb_cluster_base = of_iomap(np, 0);
> > + BUG_ON(!usb_cluster_base);
> > +
> > + xhci_node = of_find_compatible_node(NULL, NULL,
> > + "marvell,armada-375-xhci");
> > +
> > + if (xhci_node && of_device_is_available(xhci_node))
> > + use_usb3 = true;
> > +
> > + ehci_node = of_find_compatible_node(NULL, NULL,
> > + "marvell,orion-ehci");
> > +
> > + if (ehci_node && of_device_is_available(ehci_node)
> > + && use_usb3) {
> > + /*
> > + * We can't use usb2 and usb3 in the same time, so let's
> > + * disbale usb2 and complain about it to the user askinf
> > + * to fix the device tree.
> > + */
> > +
> > + ehci_status = kzalloc(sizeof(struct property),
> > + GFP_KERNEL);
> > + WARN_ON(!ehci_status);
> > +
> > + ehci_status->value = kstrdup("disabled", GFP_KERNEL);
> > + WARN_ON(!ehci_status->value);
> > +
> > + ehci_status->length = 8;
> > + ehci_status->name = kstrdup("status", GFP_KERNEL);
> > + WARN_ON(!ehci_status->name);
> > +
> > + of_update_property(ehci_node, ehci_status);
> > + pr_err("%s: armada-375-xhci and orion-ehci are incompatible for this SoC.\n",
> > + __func__);
> > + pr_err("Please fix your dts!\n");
> > + pr_err("orion-ehci have been disabled by default...\n");
> > +
> > + }
> > +
> > + reg = readl(usb_cluster_base);
> > + if (use_usb3)
> > + reg |= USB2_PHY_CONFIG_ENABLE;
> > + else
> > + reg &= ~USB2_PHY_CONFIG_ENABLE;
> > + writel(reg, usb_cluster_base);
>
> Am i right in saying this in the end is about enabling the USB2 phy or
> a USB3 why? So why not just implement a phy driver?

+1 we have a generic phy layer (drivers/phy) for that.

--
balbi

Attachments:

(No filename) (4.47 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-04-25 17:55:18

by Sergei Shtylyov

[permalink] [raw]

Subject: Re: [PATCH v2 04/18] xhci-platform: Add a new controller using xhci: Armada 38x

Hello.

On 04/25/2014 06:07 PM, Gregory CLEMENT wrote:

> Extend the compatible string list with armada-380-xhci. It is used to
> describe xhci controller which is in the Armada 38x SoCs.

> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/usb-xhci.txt | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)

> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index 90f8f607d125..cbc91414fd59 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -1,7 +1,8 @@
> USB xHCI controllers
>
> Required properties:
> - - compatible: should be "generic-xhci" (deprecated: "xhci-platform").
> + - compatible: should be on of "generic-xhci",

s/on/one/

> + "marvell,armada-380-xhci" (deprecated: "xhci-platform").

What's the difference of "marvell,armada-380-xhci" to "generic-xhci"?

WBR, Sergei

2014-04-25 20:02:55

by Arnd Bergmann

[permalink] [raw]

Subject: Re: [PATCH v2 03/18] usb: host: xhci-plat: Add support for the Armada 38x

On Friday 25 April 2014 16:07:01 Gregory CLEMENT wrote:
> @@ -148,6 +149,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> + if (of_device_is_compatible(pdev->dev.of_node, "marvell,armada-380-xhci"))
> + xhci_mvebu_mbus_init_quirk(pdev);
> +
> /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (ret)

I think you're doing it the wrong way around: You have a specialized
version of the generic xhci-plat driver. The normal way to handle this
is to have a loadable module that contains all the Armada specific
code and that registers a platform_driver. In the probe() function of
that driver, you can do the platform specific setup and then call
the generic xhci_plat_probe() function, which of course has to
be provided using EXPORT_SYMBOL_GPL.

We have just spent a lot of effort converting the EHCI and OHCI drivers
to the more generic model, so we shouldn't do it the wrong way for
xhci. I just noticed that we already have this creeping into the
xhci driver in the from of a common xhci_hcd_init() function calling
xhci_register_pci() and xhci_register_plat(). It would be good to
clean that up at the same time, it should only require exporting
a few symbols from xhci.c for use by the two front-ends.

Arnd

2014-04-25 20:11:41

by Felipe Balbi

[permalink] [raw]

Subject: Re: [PATCH v2 03/18] usb: host: xhci-plat: Add support for the Armada 38x

Hi,

On Fri, Apr 25, 2014 at 10:01:51PM +0200, Arnd Bergmann wrote:
> On Friday 25 April 2014 16:07:01 Gregory CLEMENT wrote:
> > @@ -148,6 +149,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > if (ret)
> > return ret;
> >
> > + if (of_device_is_compatible(pdev->dev.of_node, "marvell,armada-380-xhci"))
> > + xhci_mvebu_mbus_init_quirk(pdev);
> > +
> > /* Initialize dma_mask and coherent_dma_mask to 32-bits */
> > ret = dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(32));
> > if (ret)
>
> I think you're doing it the wrong way around: You have a specialized

no, it's the correct way around.

> version of the generic xhci-plat driver. The normal way to handle this
> is to have a loadable module that contains all the Armada specific
> code and that registers a platform_driver. In the probe() function of

no, that'll cause too much duplication of boilerplate code. I asked him
to write it as a quirk because then we won't end up with and xhci
platform_driver for each of the ARM licensees when all they have are
small quirks here and there.

Imagine if you had one PCI driver for each possible XHCI PCI controller
out there.

It's pointless maintainenance burden.

> that driver, you can do the platform specific setup and then call
> the generic xhci_plat_probe() function, which of course has to
> be provided using EXPORT_SYMBOL_GPL.

yeah, that sucks. Exposing a probe() function like that. I rather have
probe() actually *probe* for the HW differences.

--
balbi

Attachments:

(No filename) (1.53 kB)
signature.asc (819.00 B)
Digital signature Download all attachments

2014-04-25 20:25:43

by Thomas Petazzoni

[permalink] [raw]

Subject: Re: [PATCH v2 03/18] usb: host: xhci-plat: Add support for the Armada 38x

Dear Arnd Bergmann,

On Fri, 25 Apr 2014 22:01:51 +0200, Arnd Bergmann wrote:

> I think you're doing it the wrong way around: You have a specialized
> version of the generic xhci-plat driver. The normal way to handle this
> is to have a loadable module that contains all the Armada specific
> code and that registers a platform_driver. In the probe() function of
> that driver, you can do the platform specific setup and then call
> the generic xhci_plat_probe() function, which of course has to
> be provided using EXPORT_SYMBOL_GPL.

You should have a look at the v1 Gregory sent: it was implementing
exactly what you suggest here, but Felipe explicitly requested the
patches to be changed like is now proposed in v2.

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com