Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
On Wed, Jul 23, 2014 at 03:33:23PM +0100, Peter Griffin wrote: > > > > > + reset_control_assert(dwc3_data->rstc_pwrdn); > > > > > + > > > > > + pinctrl_pm_select_sleep_state(dev); > > > > pinctrl will select sleep and default states automatically for you. > > I've left this in v3, as greping around I couldn't see how that could > happen automatically. > > Also I just double checked with linusw on irc who confirmed that the > only state which is ever auto-selected is "default". All other states, > as well as going back to default state need to be explicitly called. > > Hope thats ok. yeah, that's fine. Thanks :-) -- balbi signature.asc Description: Digital signature
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
Hi Felipe,
Thanks for reviewing, see my comments inline: -
> > > Just use {read,write}l_relaxed() directly.
> >
> > Ok, unabstracted in v3
>
> no, no... all other glues add their own local helpers for register
> access. This is good for tracing, it's very easy to add a tracepoint to
> this sort of function and get very low overhead tracing of every
> register access.
I've put the IO accessors back in for V3
> > They are just bit setting macros, I've converted them over to use BIT macro
> > now,
> > so it no longer takes a parameter.
>
> the macros are better, but make them upper case as everybody else.
Fixed in v3.
> > I've asked ST how this value was derirved and why. It came from validation.
> > The docs don't mention that it is necessary, and removing it
> > seems to have no ill effects. So I've removed this udelay in v3.
>
> make sure to test with many, many iterations just to make sure.
Yes will do, I've been booting my board all day, and so far no failures.
> > Ok. Do the DT folks have any comment on this?
>
> look at the child's dr-mode property instead of adding your own.
Thanks for the hint, now using dr-mode binding in V3 :-)
>
> > > > + dwc3_data->glue_base = devm_request_and_ioremap(dev, res);
>
> use devm_ioremap_resource()
Fixed in V3
> > Your right, this was wrong. It was some legacy code which is
> > unnecessary and I've removed this in v3.
>
> if you're going for DT, why don't you create the parent and the child
> from DT as omap/exynos/qcom are doing ?
Now creating parent and child from DT like OMAP in v3
> > > > + reset_control_assert(dwc3_data->rstc_pwrdn);
> > > > +
> > > > + pinctrl_pm_select_sleep_state(dev);
>
> pinctrl will select sleep and default states automatically for you.
I've left this in v3, as greping around I couldn't see how that could happen
automatically.
Also I just double checked with linusw on irc who confirmed that the only state
which is
ever auto-selected is "default". All other states, as well as going back to
default
state need to be explicitly called.
Hope thats ok.
regards,
Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
On Tue, 22 Jul 2014, Felipe Balbi wrote:
> On Tue, Jul 22, 2014 at 04:45:03PM +0100, Lee Jones wrote:
> > > > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > > > +{
> > > > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > > > +
> > > > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > > > + reg &= ~sw_pipew_reset_n(1);
> > > > >
> > > > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> > > >
> > > > They are just bit setting macros, I've converted them over to use BIT
> > > > macro now,
> > > > so it no longer takes a parameter.
> > >
> > > the macros are better, but make them upper case as everybody else.
> >
> > When you say that the macros are better, what do you mean?
> >
> > Defines do the job in a manner which is a great deal cleaner:
> >
> > #define AUX_CLK_ENBIT(0)
> > #define SW_PIPEW_RESET_N BIT(4)
> > #define EXT_CFG_RESET_N BIT(8)
> > #define XHCI_REVISION BIT(12)
> >
> > reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
> > reg &= ~SW_PIPEW_RESET_N
>
> this is what I meant ;-) I see what I wrote can be very ambiguous :-p
Okay great, thanks for clarifying. :)
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
On Tue, Jul 22, 2014 at 04:45:03PM +0100, Lee Jones wrote:
> > > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > > +{
> > > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > > +
> > > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > > + reg &= ~sw_pipew_reset_n(1);
> > > >
> > > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> > >
> > > They are just bit setting macros, I've converted them over to use BIT
> > > macro now,
> > > so it no longer takes a parameter.
> >
> > the macros are better, but make them upper case as everybody else.
>
> When you say that the macros are better, what do you mean?
>
> Defines do the job in a manner which is a great deal cleaner:
>
> #define AUX_CLK_ENBIT(0)
> #define SW_PIPEW_RESET_N BIT(4)
> #define EXT_CFG_RESET_N BIT(8)
> #define XHCI_REVISION BIT(12)
>
> reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
> reg &= ~SW_PIPEW_RESET_N
this is what I meant ;-) I see what I wrote can be very ambiguous :-p
cheers
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
> > > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > > +{
> > > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > > +
> > > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > > + reg &= ~sw_pipew_reset_n(1);
> > >
> > > 1? Better to add defines for these magic numbers. What is 1 anyway?
> >
> > They are just bit setting macros, I've converted them over to use BIT macro
> > now,
> > so it no longer takes a parameter.
>
> the macros are better, but make them upper case as everybody else.
When you say that the macros are better, what do you mean?
Defines do the job in a manner which is a great deal cleaner:
#define AUX_CLK_ENBIT(0)
#define SW_PIPEW_RESET_N BIT(4)
#define EXT_CFG_RESET_N BIT(8)
#define XHCI_REVISION BIT(12)
reg = AUX_CLK_EN | SW_PIPEW_RESET_N | XHCI_REVISION;
reg &= ~SW_PIPEW_RESET_N
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
Hi,
On Tue, Jul 22, 2014 at 10:18:00AM +0100, Peter Griffin wrote:
> > > +static inline u32 st_dwc3_readl(void __iomem *base, u32 offset)
> > > +{
> > > + return readl_relaxed(base + offset);
> > > +}
> > > +
> > > +static inline void st_dwc3_writel(void __iomem *base, u32 offset, u32
> > > value)
> > > +{
> > > + writel_relaxed(value, base + offset);
> > > +}
> >
> > Why are these being abstracted?
> >
> > Just use {read,write}l_relaxed() directly.
>
> Ok, unabstracted in v3
no, no... all other glues add their own local helpers for register
access. This is good for tracing, it's very easy to add a tracepoint to
this sort of function and get very low overhead tracing of every
register access.
> > > + if (dwc3_data->drd_device_conf)
> > > + val |= USB_SET_PORT_DEVICE;
> > > + else
> > > + val &= USB_HOST_DEFAULT_MASK;
> > > +
> > > + return regmap_write(dwc3_data->regmap, dwc3_data->syscfg_reg_off, val);
> > > +}
> > > +
> > > +/**
> > > + * st_dwc3_init: init the controller via glue logic
> > > + * @dwc3_data: driver private structure
> > > + */
> > > +static void st_dwc3_init(struct st_dwc3 *dwc3_data)
> > > +{
> > > + u32 reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > +
> > > + reg |= aux_clk_en(1) | ext_cfg_reset_n(1) | xhci_revision(1);
> > > + reg &= ~sw_pipew_reset_n(1);
> >
> > 1? Better to add defines for these magic numbers. What is 1 anyway?
>
> They are just bit setting macros, I've converted them over to use BIT macro
> now,
> so it no longer takes a parameter.
the macros are better, but make them upper case as everybody else.
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg);
> > > +
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1);
> > > + reg |= SEL_OVERRIDE_VBUSVALID(1) | SEL_OVERRIDE_POWERPRESENT(1) |
> > > + SEL_OVERRIDE_BVALID(1);
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_VBUS_MNGMNT_SEL1, reg);
> > > + udelay(100);
> >
> > Why 100?
>
> I've asked ST how this value was derirved and why. It came from validation.
> The docs don't mention that it is necessary, and removing it
> seems to have no ill effects. So I've removed this udelay in v3.
make sure to test with many, many iterations just to make sure.
> > > + reg = st_dwc3_readl(dwc3_data->glue_base, USB2_CLKRST_CTRL);
> > > + reg |= sw_pipew_reset_n(1);
> > > + st_dwc3_writel(dwc3_data->glue_base, USB2_CLKRST_CTRL, reg);
> > > +}
> > > +
> > > +static void st_dwc3_dt_get_pdata(struct platform_device *pdev,
> > > + struct st_dwc3 *dwc3_data)
> > > +{
> > > + struct device_node *np = pdev->dev.of_node;
> > > +
> > > + dwc3_data->drd_device_conf =
> > > + of_property_read_bool(np, "st,dwc3-drd-device");
> >
> > This requires a DT Ack.
>
> Ok. Do the DT folks have any comment on this?
look at the child's dr-mode property instead of adding your own.
> > > + dwc3_data->glue_base = devm_request_and_ioremap(dev, res);
use devm_ioremap_resource()
> > > + regmap = syscon_regmap_lookup_by_phandle(node, "st,syscfg");
> > > + if (IS_ERR(regmap))
> > > + return PTR_ERR(regmap);
> > > +
> > > + dwc3 = platform_device_alloc("st-dwc3", PLATFORM_DEVID_AUTO);
> > > + if (!dwc3) {
> > > + dev_err(&pdev->dev, "couldn't allocate dwc3 device\n");
> > > + return -ENOMEM;
> > > + }
> >
> > I'm confused. What is this doing? Isn't this the DWC3 driver, which
> > already had a platform device structure associated to it? Perhaps a
> > small ASCII diagram describing the layers might be useful.
>
> Your right, this was wrong. It was some legacy code which is
> unnecessary and I've removed this in v3.
if you're going for DT, why don't you create the parent and the child
from DT as omap/exynos/qcom are doing ?
> > > + dma_set_coherent_mask(&dwc3->dev, dev->coherent_dma_mask);
> > > +
> > > + dwc3->dev.parent = &pdev->dev;
> > > + dwc3->dev.dma_mask = pdev->dev.dma_mask;
> > > + dwc3->dev.dma_parms = pdev->dev.dma_parms;
stick to DT device creation. Look into dwc3-omap.c
> > > +static int st_dwc3_remove(struct platform_device *pdev)
> > > +{
> > > + struct st_dwc3 *dwc3_data = platform_get_drvdata(pdev);
> > > +
> > > + platform_device_unregister(dwc3_data->dwc3);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int st_dwc3_suspend(struct device *dev)
> > > +{
> > > + struct st_dwc3 *dwc3_data = dev_get_drvdata(dev);
> > > +
> > > + reset_control_assert(dwc3_data->rstc_pwrdn);
> > > +
> > > + pinctrl_pm_select_sleep_state(dev);
pinctrl will select sleep and default states automatically for you.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
> Thanks for reviewing, see my comments inline below: - In future, it's best to only reply to questions, or review comments that you disagree with. Anything that you will action or agree with can be snipped along with any irrelevant code from your reply and replaced with "" or "[...]". If you are planning on actioning everything and no not disagree with anything there's no need to reply at all. [...] -- Lee Jones Linaro STMicroelectronics Landing Team Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
Hi Jingoo,
Sorry for the delay in replying. Thanks for reviewing,
see my comments inline below: -
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
>
> Would you re-order these headers alphabetically?
> It enhances the readability.
Ok fixed in V3
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "reg-glue");
> > + if (!res)
> > + return -ENXIO;
> > +
> > + dwc3_data->glue_base = devm_request_and_ioremap(dev, res);
>
> Please don't use devm_request_and_ioremap() any more. It was deprecated
> and will be removed from 3.17-rc1.
>
> Please, use devm_ioremap_resource() instead.
Ok changed over to use devm_ioremap_resource in V3.
>
> + dwc3_data->glue_base = devm_ioremap_resource(dev, res);
> + if (IS_ERR(dwc3_data->glue_base))
> + return PTR_ERR(dwc3_data->glue_base);
>
> > + if (!dwc3_data->glue_base)
> > + return -EADDRNOTAVAIL;
> > +
> > +
> > +static const struct dev_pm_ops st_dwc3_dev_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(st_dwc3_suspend, st_dwc3_resume)
> > +};
> > +
> > +static struct of_device_id st_dwc3_match[] = {
>
> Please add 'const' as below. This is because all OF functions
> handle of_device_id as const.
>
> static const struct of_device_id st_dwc3_match[] = {
Ok, fixed in V3
>
> > + { .compatible = "st,stih407-dwc3" },
> > + { /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, st_dwc3_match);
> > +
> > +static struct platform_driver st_dwc3_driver = {
> > + .probe = st_dwc3_probe,
> > + .remove = st_dwc3_remove,
> > + .driver = {
> > + .name = "usb-st-dwc3",
> > + .owner = THIS_MODULE,
> > + .of_match_table = of_match_ptr(st_dwc3_match),
>
> You already use OF dependency as below. So, of_match_ptr() is
> NOT necessary.
>
> +config USB_DWC3_ST
> + tristate "STMicroelectronics Platforms"
> + depends on ARCH_STI && OF
>
> Please remove of_match_ptr() as below.
>
> + .of_match_table = st_dwc3_match,
Ok fixed in V3
regards,
Peter.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
Hi Lee, Thanks for reviewing, see my comments inline below: - On Mon, 07 Jul 2014, Lee Jones wrote: > On Sat, 05 Jul 2014, Peter Griffin wrote: > > > This patch adds the ST glue logic to manage the DWC3 HC > > on STiH407 SoC family. It manages the powerdown signal, > > and configures the internal glue logic and syscfg registers. > > > > Signed-off-by: Giuseppe Cavallaro > > Signed-off-by: Peter Griffin > > --- > > drivers/usb/dwc3/Kconfig | 9 ++ > > drivers/usb/dwc3/Makefile | 1 + > > drivers/usb/dwc3/dwc3-st.c | 325 > > + > > 3 files changed, 335 insertions(+) > > create mode 100644 drivers/usb/dwc3/dwc3-st.c > > > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > > index 8eb996e..6c85c43 100644 > > --- a/drivers/usb/dwc3/Kconfig > > +++ b/drivers/usb/dwc3/Kconfig > > @@ -79,6 +79,15 @@ config USB_DWC3_KEYSTONE > > Support of USB2/3 functionality in TI Keystone2 platforms. > > Say 'Y' or 'M' here if you have one such device > > > > +config USB_DWC3_ST > > + tristate "STMicroelectronics Platforms" > > + depends on ARCH_STI && OF > > + default USB_DWC3_HOST > > + help > > + STMicroelectronics SoCs with one DesignWare Core USB3 IP > > + inside (i.e. STiH407). > > + Say 'Y' or 'M' if you have one such device. > > + > > comment "Debugging features" > > > > config USB_DWC3_DEBUG > > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile > > index 10ac3e7..11c9f54 100644 > > --- a/drivers/usb/dwc3/Makefile > > +++ b/drivers/usb/dwc3/Makefile > > @@ -33,3 +33,4 @@ obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o > > obj-$(CONFIG_USB_DWC3_EXYNOS) += dwc3-exynos.o > > obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o > > obj-$(CONFIG_USB_DWC3_KEYSTONE)+= dwc3-keystone.o > > +obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o > > diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c > > new file mode 100644 > > index 000..2cae9d3 > > --- /dev/null > > +++ b/drivers/usb/dwc3/dwc3-st.c > > @@ -0,0 +1,325 @@ > > +/** > > + * dwc3-st.c Support for dwc3 platform devices on ST Microelectronics > > platforms > > + * > > + * This is a small platform driver for the dwc3 to provide the glue logic > > + * to configure the controller. Tested on STi platforms. > > Not sure about the use of the term 'platform driver' here and in the > title. We don't normally differentiate. I can find examples to the > contrary, but not many. Ok, removed 'platform' in V3 > > > + * Copyright (C) 2014 Stmicroelectronics > > + * > > + * Author: Giuseppe Cavallaro > > + * Contributors: Aymen Bouattay > > + * Peter Griffin > > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + * > > + * Inspired by dwc3-omap.c and dwc3-exynos.c. > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "core.h" > > +#include "io.h" > > + > > +/* Reg glue registers */ > > +#define USB2_CLKRST_CTRL 0x00 > > +#define aux_clk_en(n) ((n)<<0) > > +#define sw_pipew_reset_n(n) ((n)<<4) > > +#define ext_cfg_reset_n(n) ((n)<<8) > > +#define xhci_revision(n) ((n)<<12) > > These all need reformatting, see CodingStyle - 3.1: Spaces Ok I have added a space either side of the shift operator and aligned using tabs. > > #define xhci_revision(n) ((n) << 12) > > Lining them up with TABs would make them easier to read. Ok fixed in v3 > > Also, I don't think there is a requirement to encapsulate the 'n'. Ok removed brackets around the 'n' > > > +#define USB2_VBUS_MNGMNT_SEL1 0x2C > > +/* > > + * 2'b00 : Override value from Reg 0x30 is selected > > + * 2'b01 : utmiotg_vbusvalid from usb3_top top is selected > > + * 2'b10 : pipew_powerpresent from PIPEW instance is selected > > + * 2'b11 : value is 1'b0 > > + */ > > What is this documenting? Isn't documentation meant to make things > clearer? Now I'm just really confused - by the documentation. It is documenting the bitfields in VBUS_MNGMNT_SEL1 register. I've hopefully made it a bit clearer by adding the following comment and slightly adjusting the descriptions a little. /* * For all fields in USB2_VBUS_MNGMNT_SEL1 * 2’b00 : Override value from Reg 0x30 is selected * 2’b01 : utmiotg_ from usb3_top is selected * 2’b10 : pipew_ from PIPEW instance is selected * 2’b11 : value is 1'b0 */ Apart from that it's a standard way to describe bitfields. You can find some examples in cx231xx-pcb-cfg.h, bnx2x_link.h and cx231xx-avcore.c > > > +#define SEL_OVERRIDE_VBUSVALID(n) ((n)<<0) > > +#define SEL_OVERRIDE_POWERPRESEN
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
On Saturday, July 05, 2014 3:25 PM, Peter Griffin wrote:
>
> This patch adds the ST glue logic to manage the DWC3 HC
> on STiH407 SoC family. It manages the powerdown signal,
> and configures the internal glue logic and syscfg registers.
>
> Signed-off-by: Giuseppe Cavallaro
> Signed-off-by: Peter Griffin
> ---
> drivers/usb/dwc3/Kconfig | 9 ++
> drivers/usb/dwc3/Makefile | 1 +
> drivers/usb/dwc3/dwc3-st.c | 325
> +
> 3 files changed, 335 insertions(+)
> create mode 100644 drivers/usb/dwc3/dwc3-st.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 8eb996e..6c85c43 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -79,6 +79,15 @@ config USB_DWC3_KEYSTONE
> Support of USB2/3 functionality in TI Keystone2 platforms.
> Say 'Y' or 'M' here if you have one such device
>
> +config USB_DWC3_ST
> + tristate "STMicroelectronics Platforms"
> + depends on ARCH_STI && OF
> + default USB_DWC3_HOST
> + help
> + STMicroelectronics SoCs with one DesignWare Core USB3 IP
> + inside (i.e. STiH407).
> + Say 'Y' or 'M' if you have one such device.
> +
> comment "Debugging features"
>
> config USB_DWC3_DEBUG
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 10ac3e7..11c9f54 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o
> obj-$(CONFIG_USB_DWC3_EXYNOS)+= dwc3-exynos.o
> obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o
> obj-$(CONFIG_USB_DWC3_KEYSTONE) += dwc3-keystone.o
> +obj-$(CONFIG_USB_DWC3_ST)+= dwc3-st.o
> diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> new file mode 100644
> index 000..2cae9d3
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-st.c
> @@ -0,0 +1,325 @@
> +/**
> + * dwc3-st.c Support for dwc3 platform devices on ST Microelectronics
> platforms
> + *
> + * This is a small platform driver for the dwc3 to provide the glue logic
> + * to configure the controller. Tested on STi platforms.
> + *
> + * Copyright (C) 2014 Stmicroelectronics
> + *
> + * Author: Giuseppe Cavallaro
> + * Contributors: Aymen Bouattay
> + * Peter Griffin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Inspired by dwc3-omap.c and dwc3-exynos.c.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Would you re-order these headers alphabetically?
It enhances the readability.
> +
> +#include "core.h"
> +#include "io.h"
> +
> +/* Reg glue registers */
> +#define USB2_CLKRST_CTRL 0x00
> +#define aux_clk_en(n) ((n)<<0)
> +#define sw_pipew_reset_n(n) ((n)<<4)
> +#define ext_cfg_reset_n(n) ((n)<<8)
> +#define xhci_revision(n) ((n)<<12)
> +
> +#define USB2_VBUS_MNGMNT_SEL1 0x2C
> +/*
> + * 2'b00 : Override value from Reg 0x30 is selected
> + * 2'b01 : utmiotg_vbusvalid from usb3_top top is selected
> + * 2'b10 : pipew_powerpresent from PIPEW instance is selected
> + * 2'b11 : value is 1'b0
> + */
> +#define SEL_OVERRIDE_VBUSVALID(n) ((n)<<0)
> +#define SEL_OVERRIDE_POWERPRESENT(n) ((n)<<4)
> +#define SEL_OVERRIDE_BVALID(n) ((n)<<8)
> +
> +#define USB2_VBUS_MNGMNT_VAL1 0x30
> +#define OVERRIDE_VBUSVALID_VAL (1 << 0)
> +#define OVERRIDE_POWERPRESENT_VAL (1 << 4)
> +#define OVERRIDE_BVALID_VAL (1 << 8)
> +
> +/* Static DRD configuration */
> +#define USB_HOST_DEFAULT_MASK0xffe
> +#define USB_SET_PORT_DEVICE 0x1
> +
> +struct st_dwc3 {
> + struct platform_device *dwc3; /* platform device pointer */
> + struct device *dev; /* device pointer */
> + void __iomem *glue_base;/* ioaddr for programming the glue */
> + struct regmap *regmap; /* regmap for getting syscfg */
> + int syscfg_reg_off; /* usb syscfg control offset */
> + bool drd_device_conf; /* DRD static host/device conf */
> + struct reset_control *rstc_pwrdn;/* Rst control for powerdown*/
> +};
> +
> +static inline u32 st_dwc3_readl(void __iomem *base, u32 offset)
> +{
> + return readl_relaxed(base + offset);
> +}
> +
> +static inline void st_dwc3_writel(void __iomem *base, u32 offset, u32 value)
> +{
> + writel_relaxed(value, base + offset);
> +}
> +
> +/**
> + * st_dwc3_drd_init: program the port
> + * @dwc3_data: driver private structure
> + * Description: this function is to program the port as either host or device
> + * according to the static configuration passed from devicetree.
> + * OTG and dual role are not yet supported!
> + */
> +static int st_dwc3_drd_init(struct st_dwc3 *dwc3_data)
Re: [PATCH v2 1/3] usb: dwc3: add ST dwc3 glue layer to manage dwc3 HC
On Sat, 05 Jul 2014, Peter Griffin wrote:
> This patch adds the ST glue logic to manage the DWC3 HC
> on STiH407 SoC family. It manages the powerdown signal,
> and configures the internal glue logic and syscfg registers.
>
> Signed-off-by: Giuseppe Cavallaro
> Signed-off-by: Peter Griffin
> ---
> drivers/usb/dwc3/Kconfig | 9 ++
> drivers/usb/dwc3/Makefile | 1 +
> drivers/usb/dwc3/dwc3-st.c | 325
> +
> 3 files changed, 335 insertions(+)
> create mode 100644 drivers/usb/dwc3/dwc3-st.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 8eb996e..6c85c43 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -79,6 +79,15 @@ config USB_DWC3_KEYSTONE
> Support of USB2/3 functionality in TI Keystone2 platforms.
> Say 'Y' or 'M' here if you have one such device
>
> +config USB_DWC3_ST
> + tristate "STMicroelectronics Platforms"
> + depends on ARCH_STI && OF
> + default USB_DWC3_HOST
> + help
> + STMicroelectronics SoCs with one DesignWare Core USB3 IP
> + inside (i.e. STiH407).
> + Say 'Y' or 'M' if you have one such device.
> +
> comment "Debugging features"
>
> config USB_DWC3_DEBUG
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 10ac3e7..11c9f54 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o
> obj-$(CONFIG_USB_DWC3_EXYNOS)+= dwc3-exynos.o
> obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o
> obj-$(CONFIG_USB_DWC3_KEYSTONE) += dwc3-keystone.o
> +obj-$(CONFIG_USB_DWC3_ST)+= dwc3-st.o
> diff --git a/drivers/usb/dwc3/dwc3-st.c b/drivers/usb/dwc3/dwc3-st.c
> new file mode 100644
> index 000..2cae9d3
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-st.c
> @@ -0,0 +1,325 @@
> +/**
> + * dwc3-st.c Support for dwc3 platform devices on ST Microelectronics
> platforms
> + *
> + * This is a small platform driver for the dwc3 to provide the glue logic
> + * to configure the controller. Tested on STi platforms.
Not sure about the use of the term 'platform driver' here and in the
title. We don't normally differentiate. I can find examples to the
contrary, but not many.
> + * Copyright (C) 2014 Stmicroelectronics
> + *
> + * Author: Giuseppe Cavallaro
> + * Contributors: Aymen Bouattay
> + * Peter Griffin
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * Inspired by dwc3-omap.c and dwc3-exynos.c.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "core.h"
> +#include "io.h"
> +
> +/* Reg glue registers */
> +#define USB2_CLKRST_CTRL 0x00
> +#define aux_clk_en(n) ((n)<<0)
> +#define sw_pipew_reset_n(n) ((n)<<4)
> +#define ext_cfg_reset_n(n) ((n)<<8)
> +#define xhci_revision(n) ((n)<<12)
These all need reformatting, see CodingStyle - 3.1: Spaces
#define xhci_revision(n) ((n) << 12)
Lining them up with TABs would make them easier to read.
Also, I don't think there is a requirement to encapsulate the 'n'.
> +#define USB2_VBUS_MNGMNT_SEL1 0x2C
> +/*
> + * 2'b00 : Override value from Reg 0x30 is selected
> + * 2'b01 : utmiotg_vbusvalid from usb3_top top is selected
> + * 2'b10 : pipew_powerpresent from PIPEW instance is selected
> + * 2'b11 : value is 1'b0
> + */
What is this documenting? Isn't documentation meant to make things
clearer? Now I'm just really confused - by the documentation.
> +#define SEL_OVERRIDE_VBUSVALID(n) ((n)<<0)
> +#define SEL_OVERRIDE_POWERPRESENT(n) ((n)<<4)
> +#define SEL_OVERRIDE_BVALID(n) ((n)<<8)
> +
> +#define USB2_VBUS_MNGMNT_VAL1 0x30
> +#define OVERRIDE_VBUSVALID_VAL (1 << 0)
> +#define OVERRIDE_POWERPRESENT_VAL (1 << 4)
> +#define OVERRIDE_BVALID_VAL (1 << 8)
Use BIT() for all of these bit setting macros.
> +/* Static DRD configuration */
> +#define USB_HOST_DEFAULT_MASK0xffe
> +#define USB_SET_PORT_DEVICE 0x1
> +
> +struct st_dwc3 {
> + struct platform_device *dwc3; /* platform device pointer */
> + struct device *dev; /* device pointer */
> + void __iomem *glue_base;/* ioaddr for programming the glue */
> + struct regmap *regmap; /* regmap for getting syscfg */
> + int syscfg_reg_off; /* usb syscfg control offset */
> + bool drd_device_conf; /* DRD static host/device conf */
> + struct reset_control *rstc_pwrdn;/* Rst control for powerdown*/
> +};
This is a mess. Use kerneldoc formatting instead.
> +static inline u32 st_dwc3_readl(void __iomem *base, u32 offset)
> +{
> + return readl_relaxed(
