[2/4] usb: dwc3: add dwc3 glue layer for UniPhier SoCs

Commit Message

Kunihiko Hayashi Jan. 23, 2018, 1 p.m. UTC

Add a specific glue layer for UniPhier SoC platform to support
USB host mode. It manages hardware operating sequences to enable multiple
clock gates and assert resets, and to prepare to use dwc3 controller
on the SoC.

This patch also handles the physical layer that has same register space
as the glue layer, because it needs to integrate initialziation sequence
between glue and phy.

In case of some SoCs, since some initialization values for PHY are
included in nvmem, this patch includes the way to get the values from nvmem.

It supports PXs2 and LD20 SoCs.

Signed-off-by: Kunihiko Hayashi <[e-mail]>
Signed-off-by: Motoya Tanigawa <[e-mail]>
Signed-off-by: Masami Hiramatsu <[e-mail]>
---
 drivers/usb/dwc3/Kconfig         |   9 +
 drivers/usb/dwc3/Makefile        |   1 +
 drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 564 insertions(+)
 create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c

Comments

Felipe Balbi Jan. 23, 2018, 1:12 p.m. UTC | #1

Hi,

Kunihiko Hayashi <[e-mail]> writes:
> Add a specific glue layer for UniPhier SoC platform to support
> USB host mode. It manages hardware operating sequences to enable multiple
> clock gates and assert resets, and to prepare to use dwc3 controller
> on the SoC.
>
> This patch also handles the physical layer that has same register space
> as the glue layer, because it needs to integrate initialziation sequence
> between glue and phy.
>
> In case of some SoCs, since some initialization values for PHY are
> included in nvmem, this patch includes the way to get the values from nvmem.
>
> It supports PXs2 and LD20 SoCs.
>
> Signed-off-by: Kunihiko Hayashi <[e-mail]>
> Signed-off-by: Motoya Tanigawa <[e-mail]>
> Signed-off-by: Masami Hiramatsu <[e-mail]>
> ---
>  drivers/usb/dwc3/Kconfig         |   9 +
>  drivers/usb/dwc3/Makefile        |   1 +
>  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 564 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index ab8c0e0..a5cadc6 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -106,4 +106,13 @@ config USB_DWC3_ST
>  	  inside (i.e. STiH407).
>  	  Say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_UNIPHIER
> +	tristate "Socionext UniPhier Platforms"
> +	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> +	default USB_DWC3
> +	help
> +	  Support USB2/3 functionality in UniPhier platforms.
> +	  Say 'Y' or 'M' if your system that UniPhier SoC is implemented
> +	  has USB controllers based on DWC USB3 IP.
> +
>  endif
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 7ac7250..31e82b3 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
>  obj-$(CONFIG_USB_DWC3_KEYSTONE)		+= dwc3-keystone.o
>  obj-$(CONFIG_USB_DWC3_OF_SIMPLE)	+= dwc3-of-simple.o
>  obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
> +obj-$(CONFIG_USB_DWC3_UNIPHIER)		+= dwc3-uniphier.o
> diff --git a/drivers/usb/dwc3/dwc3-uniphier.c b/drivers/usb/dwc3/dwc3-uniphier.c
> new file mode 100644
> index 0000000..58e84cd
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-uniphier.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * dwc3-uniphier.c - Socionext UniPhier DWC3 specific glue layer
> + *
> + * Copyright 2015-2018 Socionext Inc.
> + *
> + * Author:
> + *	Kunihiko Hayashi <[e-mail]>
> + * Contributors:
> + *      Motoya Tanigawa <[e-mail]>
> + *      Masami Hiramatsu <[e-mail]>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +
> +#define RESET_CTL		0x000
> +#define LINK_RESET		BIT(15)
> +
> +#define VBUS_CONTROL(n)		(0x100 + 0x10 * (n))
> +#define DRVVBUS_REG		BIT(4)
> +#define DRVVBUS_REG_EN		BIT(3)
> +
> +#define U2PHY_CFG0(n)		(0x200 + 0x10 * (n))
> +#define U2PHY_CFG0_HS_I_MASK	GENMASK(31, 28)
> +#define U2PHY_CFG0_HSDISC_MASK	GENMASK(27, 26)
> +#define U2PHY_CFG0_SWING_MASK	GENMASK(17, 16)
> +#define U2PHY_CFG0_SEL_T_MASK	GENMASK(15, 12)
> +#define U2PHY_CFG0_RTERM_MASK	GENMASK(7, 6)
> +#define U2PHY_CFG0_TRIMMASK	(U2PHY_CFG0_HS_I_MASK \
> +				 | U2PHY_CFG0_SEL_T_MASK \
> +				 | U2PHY_CFG0_RTERM_MASK)
> +
> +#define U2PHY_CFG1(n)		(0x204 + 0x10 * (n))
> +#define U2PHY_CFG1_DAT_EN	BIT(29)
> +#define U2PHY_CFG1_ADR_EN	BIT(28)
> +#define U2PHY_CFG1_ADR_MASK	GENMASK(27, 16)
> +#define U2PHY_CFG1_DAT_MASK	GENMASK(23, 16)
> +
> +#define U3PHY_TESTI(n)		(0x300 + 0x10 * (n))
> +#define U3PHY_TESTO(n)		(0x304 + 0x10 * (n))
> +#define TESTI_DAT_MASK		GENMASK(13, 6)
> +#define TESTI_ADR_MASK		GENMASK(5, 1)
> +#define TESTI_WR_EN		BIT(0)
> +
> +#define HOST_CONFIG0		0x400
> +#define NUM_U3_MASK		GENMASK(13, 11)
> +#define NUM_U2_MASK		GENMASK(10, 8)
> +
> +#define PHY_MAX_PARAMS	32
> +
> +struct dwc3u_phy_param {
> +	u32 addr;
> +	u32 mask;
> +	u32 val;
> +};
> +
> +struct dwc3u_trim_param {
> +	u32 rterm;
> +	u32 sel_t;
> +	u32 hs_i;
> +};
> +
> +#define trim_param_is_valid(p)	((p)->rterm || (p)->sel_t || (p)->hs_i)
> +
> +struct dwc3u_priv {
> +	struct device		*dev;
> +	void __iomem		*base;
> +	struct clk		**clks;
> +	int			nclks;
> +	struct reset_control	*rst;
> +	int			nvbus;
> +	const struct dwc3u_soc_data	*data;
> +};
> +
> +struct dwc3u_soc_data {
> +	int ss_nparams;
> +	struct dwc3u_phy_param ss_param[PHY_MAX_PARAMS];
> +	int hs_nparams;
> +	struct dwc3u_phy_param hs_param[PHY_MAX_PARAMS];
> +	u32 hs_config0;
> +	u32 hs_config1;
> +	void (*trim_func)(struct dwc3u_priv *priv, u32 *pconfig,
> +			  struct dwc3u_trim_param *trim);
> +};
> +
> +static inline u32 dwc3u_read(struct dwc3u_priv *priv, off_t offset)
> +{
> +	return readl(priv->base + offset);
> +}
> +
> +static inline void dwc3u_write(struct dwc3u_priv *priv,
> +			       off_t offset, u32 val)
> +{
> +	writel(val, priv->base + offset);
> +}
> +
> +static inline void dwc3u_maskwrite(struct dwc3u_priv *priv,
> +				   off_t offset, u32 mask, u32 val)
> +{
> +	u32 tmp;
> +
> +	tmp = dwc3u_read(priv, offset);
> +	dwc3u_write(priv, offset, (tmp & ~mask) | (val & mask));
> +}
> +
> +static int dwc3u_get_hsport_num(struct dwc3u_priv *priv)
> +{
> +	return FIELD_GET(NUM_U2_MASK, dwc3u_read(priv, HOST_CONFIG0));
> +}
> +
> +static int dwc3u_get_ssport_num(struct dwc3u_priv *priv)
> +{
> +	return FIELD_GET(NUM_U3_MASK, dwc3u_read(priv, HOST_CONFIG0));
> +}
> +
> +static int dwc3u_get_nvparam(struct dwc3u_priv *priv,
> +			     const char *basename, int index, u8 *dst,
> +			     int maxlen)
> +{
> +	struct nvmem_cell *cell;
> +	char name[16];
> +	size_t len;
> +	u8 *buf;
> +
> +	snprintf(name, sizeof(name) - 1, "%s%d", basename, index);
> +	memset(dst, 0, maxlen);
> +
> +	cell = nvmem_cell_get(priv->dev, name);
> +	if (IS_ERR(cell))
> +		return PTR_ERR(cell);
> +
> +	buf = nvmem_cell_read(cell, &len);
> +	nvmem_cell_put(cell);
> +	if (IS_ERR(buf))
> +		return PTR_ERR(buf);
> +
> +	len = min_t(u32, len, maxlen);
> +	memcpy(dst, buf, len);
> +	kfree(buf);
> +
> +	return 0;
> +}
> +
> +static int dwc3u_get_nvparam_u32(struct dwc3u_priv *priv,
> +				 const char *basename, int index, u32 *p_val)
> +{
> +	return dwc3u_get_nvparam(priv, basename, index, (u8 *)p_val,
> +				 sizeof(u32));
> +}
> +
> +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
> +				     u32 data)

anything with sshphy or hsphy in the name should probably be part of a
PHY driver using drivers/phy/ framework.

> +static void dwc3u_vbus_enable(struct dwc3u_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->nvbus; i++) {
> +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
> +				DRVVBUS_REG_EN | DRVVBUS_REG,
> +				DRVVBUS_REG_EN | DRVVBUS_REG);
> +	}
> +}
> +
> +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
> +{
> +	int i;
> +
> +	for (i = 0; i < priv->nvbus; i++) {
> +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
> +				DRVVBUS_REG_EN | DRVVBUS_REG,
> +				DRVVBUS_REG_EN | 0);
> +	}
> +}

drivers/regulator maybe?

> +static void dwc3u_reset_init(struct dwc3u_priv *priv)
> +{
> +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> +	usleep_range(1000, 2000);
> +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
> +}
> +
> +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
> +{
> +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> +}

drivers/reset ?

> +static int dwc3u_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node;
> +	struct dwc3u_priv *priv;
> +	struct resource	*res;
> +	struct clk *clk;
> +	int i, nr_clks;
> +	int ret = 0;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->data = of_device_get_match_data(dev);
> +	if (WARN_ON(!priv->data))
> +		return -EINVAL;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	priv->dev = dev;
> +
> +	node = dev->of_node;
> +	nr_clks = of_clk_get_parent_count(node);
> +	if (!nr_clks) {
> +		dev_err(dev, "failed to get clock property\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->clks = devm_kcalloc(priv->dev, nr_clks, sizeof(struct clk *),
> +				  GFP_KERNEL);
> +	if (!priv->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < nr_clks; i++) {
> +		clk = of_clk_get(node, i);
> +		if (IS_ERR(clk)) {
> +			ret = PTR_ERR(clk);
> +			goto out_clk_disable;
> +		}
> +		ret = clk_prepare_enable(clk);
> +		if (ret < 0) {
> +			clk_put(clk);
> +			goto out_clk_disable;
> +		}
> +		priv->clks[i] = clk;
> +		priv->nclks = i;
> +	}
> +
> +	priv->rst = devm_reset_control_array_get_optional_shared(priv->dev);
> +	if (IS_ERR(priv->rst)) {
> +		ret = PTR_ERR(priv->rst);
> +		goto out_clk_disable;
> +	}
> +	ret = reset_control_deassert(priv->rst);
> +	if (ret)
> +		goto out_clk_disable;
> +
> +	ret = dwc3u_init(priv);
> +	if (ret)
> +		goto out_rst_assert;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	ret = of_platform_populate(node, NULL, NULL, priv->dev);
> +	if (ret)
> +		goto out_exit;

with the stuff that should be using generic frameworks removed, this
looks like dwc3-of-simple.c, which you should be using.

Kunihiko Hayashi Jan. 24, 2018, 12:52 p.m. UTC | #2

Hello Felipe,

Thank you for your comments.

On Tue, 23 Jan 2018 15:12:36 +0200 <[e-mail]> wrote:

> 
> Hi,
> 
> Kunihiko Hayashi <[e-mail]> writes:
> > Add a specific glue layer for UniPhier SoC platform to support
> > USB host mode. It manages hardware operating sequences to enable multiple
> > clock gates and assert resets, and to prepare to use dwc3 controller
> > on the SoC.
> >
> > This patch also handles the physical layer that has same register space
> > as the glue layer, because it needs to integrate initialziation sequence
> > between glue and phy.
> >
> > In case of some SoCs, since some initialization values for PHY are
> > included in nvmem, this patch includes the way to get the values from nvmem.
> >
> > It supports PXs2 and LD20 SoCs.
> >
> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
> > Signed-off-by: Motoya Tanigawa <[e-mail]>
> > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > ---
> >  drivers/usb/dwc3/Kconfig         |   9 +
> >  drivers/usb/dwc3/Makefile        |   1 +
> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 564 insertions(+)
> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c

...snip...

> > +
> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
> > +				     u32 data)
> 
> anything with sshphy or hsphy in the name should probably be part of a
> PHY driver using drivers/phy/ framework.

I can try to separate phy control from this driver.
However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
and this area also contains a reset bit for "dwc3-core" hardware.

Although the phy driver is called from dwc3-core driver,
we need to deassert the reset bit before probing dwc3-core driver.

As shown later, I think that it's difficut to determine the order of
initializing the registers in this area.

> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < priv->nvbus; i++) {
> > +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
> > +				DRVVBUS_REG_EN | DRVVBUS_REG,
> > +				DRVVBUS_REG_EN | 0);
> > +	}
> > +}
> 
> drivers/regulator maybe?

VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
enables vbus connected with "regulator" hardware.

The regulator driver should manage "regulator" hardware, and
I don't think that the driver should manage this register.

> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
> > +{
> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> > +	usleep_range(1000, 2000);
> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
> > +}
> > +
> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
> > +{
> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> > +}
> 
> drivers/reset ?

The reset driver manages "sysctrl" IO map area in our SoC.

RESET_CTL register belongs to "dwc3-glue" IO map area,
and the kernel can't access this area until enabling usb clocks and
deasserting usb resets in "sysctrl".

I think that "dwc3-glue" register control should be separated from
"sysctrl".

> > +static int dwc3u_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *node;
> > +	struct dwc3u_priv *priv;
> > +	struct resource	*res;
> > +	struct clk *clk;
> > +	int i, nr_clks;
> > +	int ret = 0;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +	if (WARN_ON(!priv->data))
> > +		return -EINVAL;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	priv->base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(priv->base))
> > +		return PTR_ERR(priv->base);
> > +
> > +	priv->dev = dev;
> > +
> > +	node = dev->of_node;
> > +	nr_clks = of_clk_get_parent_count(node);
> > +	if (!nr_clks) {
> > +		dev_err(dev, "failed to get clock property\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	priv->clks = devm_kcalloc(priv->dev, nr_clks, sizeof(struct clk *),
> > +				  GFP_KERNEL);
> > +	if (!priv->clks)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < nr_clks; i++) {
> > +		clk = of_clk_get(node, i);
> > +		if (IS_ERR(clk)) {
> > +			ret = PTR_ERR(clk);
> > +			goto out_clk_disable;
> > +		}
> > +		ret = clk_prepare_enable(clk);
> > +		if (ret < 0) {
> > +			clk_put(clk);
> > +			goto out_clk_disable;
> > +		}
> > +		priv->clks[i] = clk;
> > +		priv->nclks = i;
> > +	}
> > +
> > +	priv->rst = devm_reset_control_array_get_optional_shared(priv->dev);
> > +	if (IS_ERR(priv->rst)) {
> > +		ret = PTR_ERR(priv->rst);
> > +		goto out_clk_disable;
> > +	}
> > +	ret = reset_control_deassert(priv->rst);
> > +	if (ret)
> > +		goto out_clk_disable;
> > +
> > +	ret = dwc3u_init(priv);
> > +	if (ret)
> > +		goto out_rst_assert;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	ret = of_platform_populate(node, NULL, NULL, priv->dev);
> > +	if (ret)
> > +		goto out_exit;
> 
> with the stuff that should be using generic frameworks removed, this
> looks like dwc3-of-simple.c, which you should be using.

Before probing dwc3-core driver and accessing "dwc3-core" register,
we need to do the following items in order:

- enable clock and deassert resets in "sysctrl" with calling the
  clock/reset driver, so that the kernel can access "dwc3-glue"
  IO map area.

- enable vbus and setup phy with the "dwc3-glue" register.

- deassert "dwc3-core" reset with the "dwc3-glue" register,
  so that the kernel can access "dwc3-core" IO map area.

The dwc3-of-simple driver only enables the clock driver before
probing dwc3-core driver, and it seems that the driver have no ways
to deassert the reset driver and manage "dwc3-glue" IO map area.

Are there any ways to manage them before probing dwc3-core driver?

Thank you,

---
Best Regards,
Kunihiko Hayashi

Felipe Balbi Jan. 24, 2018, 12:58 p.m. UTC | #3

Hi,

Kunihiko Hayashi <[e-mail]> writes:
> Hello Felipe,
>
> Thank you for your comments.
>
> On Tue, 23 Jan 2018 15:12:36 +0200 <[e-mail]> wrote:
>
>> 
>> Hi,
>> 
>> Kunihiko Hayashi <[e-mail]> writes:
>> > Add a specific glue layer for UniPhier SoC platform to support
>> > USB host mode. It manages hardware operating sequences to enable multiple
>> > clock gates and assert resets, and to prepare to use dwc3 controller
>> > on the SoC.
>> >
>> > This patch also handles the physical layer that has same register space
>> > as the glue layer, because it needs to integrate initialziation sequence
>> > between glue and phy.
>> >
>> > In case of some SoCs, since some initialization values for PHY are
>> > included in nvmem, this patch includes the way to get the values from nvmem.
>> >
>> > It supports PXs2 and LD20 SoCs.
>> >
>> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
>> > Signed-off-by: Motoya Tanigawa <[e-mail]>
>> > Signed-off-by: Masami Hiramatsu <[e-mail]>
>> > ---
>> >  drivers/usb/dwc3/Kconfig         |   9 +
>> >  drivers/usb/dwc3/Makefile        |   1 +
>> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
>> >  3 files changed, 564 insertions(+)
>> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> ...snip...
>
>> > +
>> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
>> > +				     u32 data)
>> 
>> anything with sshphy or hsphy in the name should probably be part of a
>> PHY driver using drivers/phy/ framework.
>
> I can try to separate phy control from this driver.
> However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
> and this area also contains a reset bit for "dwc3-core" hardware.
>
> Although the phy driver is called from dwc3-core driver,
> we need to deassert the reset bit before probing dwc3-core driver.
>
> As shown later, I think that it's difficut to determine the order of
> initializing the registers in this area.
>
>> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
>> > +{
>> > +	int i;
>> > +
>> > +	for (i = 0; i < priv->nvbus; i++) {
>> > +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
>> > +				DRVVBUS_REG_EN | DRVVBUS_REG,
>> > +				DRVVBUS_REG_EN | 0);
>> > +	}
>> > +}
>> 
>> drivers/regulator maybe?
>
> VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> enables vbus connected with "regulator" hardware.
>
> The regulator driver should manage "regulator" hardware, and
> I don't think that the driver should manage this register.
>
>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>> > +{
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +	usleep_range(1000, 2000);
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>> > +}
>> > +
>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>> > +{
>> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +}
>> 
>> drivers/reset ?
>
> The reset driver manages "sysctrl" IO map area in our SoC.
>
> RESET_CTL register belongs to "dwc3-glue" IO map area,
> and the kernel can't access this area until enabling usb clocks and
> deasserting usb resets in "sysctrl".
>
> I think that "dwc3-glue" register control should be separated from
> "sysctrl".

Just split your address space and treat your glue as a device with
several children:

glue@65b00000 {
	compatible = "foo"

	phy@bar {
        	...
	};

	sysctrl@baz {
        	...
	};

	dwc3@foo {
        	compatible = "snps, dwc3";
                ...
	};
};

Then you know that you can let dwc3/core.c handle the PHY for you. If we
need to teach dwc3/core.c about regulators, we can do that. But we don't
need SoC-specific hacks ;-)

Kunihiko Hayashi Jan. 25, 2018, 2:09 a.m. UTC | #4

Hello Felipe,

On Wed, 24 Jan 2018 14:58:12 +0200 <[e-mail]> wrote:

> 
> Hi,
> 
> Kunihiko Hayashi <[e-mail]> writes:
> > Hello Felipe,
> >
> > Thank you for your comments.
> >
> > On Tue, 23 Jan 2018 15:12:36 +0200 <[e-mail]> wrote:
> >
> >> 
> >> Hi,
> >> 
> >> Kunihiko Hayashi <[e-mail]> writes:
> >> > Add a specific glue layer for UniPhier SoC platform to support
> >> > USB host mode. It manages hardware operating sequences to enable multiple
> >> > clock gates and assert resets, and to prepare to use dwc3 controller
> >> > on the SoC.
> >> >
> >> > This patch also handles the physical layer that has same register space
> >> > as the glue layer, because it needs to integrate initialziation sequence
> >> > between glue and phy.
> >> >
> >> > In case of some SoCs, since some initialization values for PHY are
> >> > included in nvmem, this patch includes the way to get the values from nvmem.
> >> >
> >> > It supports PXs2 and LD20 SoCs.
> >> >
> >> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
> >> > Signed-off-by: Motoya Tanigawa <[e-mail]>
> >> > Signed-off-by: Masami Hiramatsu <[e-mail]>
> >> > ---
> >> >  drivers/usb/dwc3/Kconfig         |   9 +
> >> >  drivers/usb/dwc3/Makefile        |   1 +
> >> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
> >> >  3 files changed, 564 insertions(+)
> >> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
> >
> > ...snip...
> >
> >> > +
> >> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
> >> > +				     u32 data)
> >> 
> >> anything with sshphy or hsphy in the name should probably be part of a
> >> PHY driver using drivers/phy/ framework.
> >
> > I can try to separate phy control from this driver.
> > However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
> > and this area also contains a reset bit for "dwc3-core" hardware.
> >
> > Although the phy driver is called from dwc3-core driver,
> > we need to deassert the reset bit before probing dwc3-core driver.
> >
> > As shown later, I think that it's difficut to determine the order of
> > initializing the registers in this area.
> >
> >> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
> >> > +{
> >> > +	int i;
> >> > +
> >> > +	for (i = 0; i < priv->nvbus; i++) {
> >> > +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
> >> > +				DRVVBUS_REG_EN | DRVVBUS_REG,
> >> > +				DRVVBUS_REG_EN | 0);
> >> > +	}
> >> > +}
> >> 
> >> drivers/regulator maybe?
> >
> > VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> > enables vbus connected with "regulator" hardware.
> >
> > The regulator driver should manage "regulator" hardware, and
> > I don't think that the driver should manage this register.
> >
> >> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
> >> > +{
> >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> >> > +	usleep_range(1000, 2000);
> >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
> >> > +}
> >> > +
> >> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
> >> > +{
> >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> >> > +}
> >> 
> >> drivers/reset ?
> >
> > The reset driver manages "sysctrl" IO map area in our SoC.
> >
> > RESET_CTL register belongs to "dwc3-glue" IO map area,
> > and the kernel can't access this area until enabling usb clocks and
> > deasserting usb resets in "sysctrl".
> >
> > I think that "dwc3-glue" register control should be separated from
> > "sysctrl".
> 
> Just split your address space and treat your glue as a device with
> several children:
> 
> glue@65b00000 {
> 	compatible = "foo"

Just to confirm, instead of SoC-specific glue driver, dwc3-of-simple.c
should handle compatible "foo", right?

> 
> 	phy@bar {
>         	...
> 	};
> 
> 	sysctrl@baz {
>         	...
> 	};
> 
> 	dwc3@foo {
>         	compatible = "snps, dwc3";
>                 ...
> 	};
> };
> 
> Then you know that you can let dwc3/core.c handle the PHY for you. If we
> need to teach dwc3/core.c about regulators, we can do that. But we don't
> need SoC-specific hacks ;-)

I also don't think that dwc3/core.c should have any SoC-specific controls.
I've thought the SoC-specific glue driver (dwc3-***.c) calls "clocks",
"resets", and "regulators".

However, although dwc3-of-simple.c can handle "clocks", it can't handle
"resets" and "regulators". Who should call them?

Thank you,

---
Best Regards,
Kunihiko Hayashi

kernel test robot Jan. 25, 2018, 9:16 p.m. UTC | #5

Hi Kunihiko,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.15-rc9 next-20180119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Kunihiko-Hayashi/usb-dwc3-add-UniPhier-dwc3-glue-layer-support/20180126-035928
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: ia64-allmodconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 7.2.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=ia64 

All errors (new ones prefixed by >>):

   drivers/usb//dwc3/dwc3-uniphier.c: In function 'dwc3u_probe':
>> drivers/usb//dwc3/dwc3-uniphier.c:428:12: error: implicit declaration of function 'of_clk_get_parent_count'; did you mean 'clk_get_parent'? [-Werror=implicit-function-declaration]
     nr_clks = of_clk_get_parent_count(node);
               ^~~~~~~~~~~~~~~~~~~~~~~
               clk_get_parent
   cc1: some warnings being treated as errors

vim +428 drivers/usb//dwc3/dwc3-uniphier.c

   401	
   402	static int dwc3u_probe(struct platform_device *pdev)
   403	{
   404		struct device *dev = &pdev->dev;
   405		struct device_node *node;
   406		struct dwc3u_priv *priv;
   407		struct resource	*res;
   408		struct clk *clk;
   409		int i, nr_clks;
   410		int ret = 0;
   411	
   412		priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
   413		if (!priv)
   414			return -ENOMEM;
   415	
   416		priv->data = of_device_get_match_data(dev);
   417		if (WARN_ON(!priv->data))
   418			return -EINVAL;
   419	
   420		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
   421		priv->base = devm_ioremap_resource(dev, res);
   422		if (IS_ERR(priv->base))
   423			return PTR_ERR(priv->base);
   424	
   425		priv->dev = dev;
   426	
   427		node = dev->of_node;
 > 428		nr_clks = of_clk_get_parent_count(node);
   429		if (!nr_clks) {
   430			dev_err(dev, "failed to get clock property\n");
   431			return -ENODEV;
   432		}
   433	
   434		priv->clks = devm_kcalloc(priv->dev, nr_clks, sizeof(struct clk *),
   435					  GFP_KERNEL);
   436		if (!priv->clks)
   437			return -ENOMEM;
   438	
   439		for (i = 0; i < nr_clks; i++) {
   440			clk = of_clk_get(node, i);
   441			if (IS_ERR(clk)) {
   442				ret = PTR_ERR(clk);
   443				goto out_clk_disable;
   444			}
   445			ret = clk_prepare_enable(clk);
   446			if (ret < 0) {
   447				clk_put(clk);
   448				goto out_clk_disable;
   449			}
   450			priv->clks[i] = clk;
   451			priv->nclks = i;
   452		}
   453	
   454		priv->rst = devm_reset_control_array_get_optional_shared(priv->dev);
   455		if (IS_ERR(priv->rst)) {
   456			ret = PTR_ERR(priv->rst);
   457			goto out_clk_disable;
   458		}
   459		ret = reset_control_deassert(priv->rst);
   460		if (ret)
   461			goto out_clk_disable;
   462	
   463		ret = dwc3u_init(priv);
   464		if (ret)
   465			goto out_rst_assert;
   466	
   467		platform_set_drvdata(pdev, priv);
   468	
   469		ret = of_platform_populate(node, NULL, NULL, priv->dev);
   470		if (ret)
   471			goto out_exit;
   472	
   473		return 0;
   474	
   475	out_exit:
   476		dwc3u_exit(priv);
   477	out_rst_assert:
   478		reset_control_assert(priv->rst);
   479	out_clk_disable:
   480		dwc3u_disable_clk(priv);
   481	
   482		return ret;
   483	}
   484	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Kunihiko Hayashi Jan. 26, 2018, 7:28 a.m. UTC | #6

Hi,

On Thu, 25 Jan 2018 11:09:30 +0900 <[e-mail]> wrote:

> Hello Felipe,
> 
> On Wed, 24 Jan 2018 14:58:12 +0200 <[e-mail]> wrote:
> 
> > 
> > Hi,
> > 
> > Kunihiko Hayashi <[e-mail]> writes:
> > > Hello Felipe,
> > >
> > > Thank you for your comments.
> > >
> > > On Tue, 23 Jan 2018 15:12:36 +0200 <[e-mail]> wrote:
> > >
> > >> 
> > >> Hi,
> > >> 
> > >> Kunihiko Hayashi <[e-mail]> writes:
> > >> > Add a specific glue layer for UniPhier SoC platform to support
> > >> > USB host mode. It manages hardware operating sequences to enable multiple
> > >> > clock gates and assert resets, and to prepare to use dwc3 controller
> > >> > on the SoC.
> > >> >
> > >> > This patch also handles the physical layer that has same register space
> > >> > as the glue layer, because it needs to integrate initialziation sequence
> > >> > between glue and phy.
> > >> >
> > >> > In case of some SoCs, since some initialization values for PHY are
> > >> > included in nvmem, this patch includes the way to get the values from nvmem.
> > >> >
> > >> > It supports PXs2 and LD20 SoCs.
> > >> >
> > >> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
> > >> > Signed-off-by: Motoya Tanigawa <[e-mail]>
> > >> > Signed-off-by: Masami Hiramatsu <[e-mail]>
> > >> > ---
> > >> >  drivers/usb/dwc3/Kconfig         |   9 +
> > >> >  drivers/usb/dwc3/Makefile        |   1 +
> > >> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
> > >> >  3 files changed, 564 insertions(+)
> > >> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
> > >
> > > ...snip...
> > >
> > >> > +
> > >> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
> > >> > +				     u32 data)
> > >> 
> > >> anything with sshphy or hsphy in the name should probably be part of a
> > >> PHY driver using drivers/phy/ framework.
> > >
> > > I can try to separate phy control from this driver.
> > > However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
> > > and this area also contains a reset bit for "dwc3-core" hardware.
> > >
> > > Although the phy driver is called from dwc3-core driver,
> > > we need to deassert the reset bit before probing dwc3-core driver.
> > >
> > > As shown later, I think that it's difficut to determine the order of
> > > initializing the registers in this area.
> > >
> > >> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
> > >> > +{
> > >> > +	int i;
> > >> > +
> > >> > +	for (i = 0; i < priv->nvbus; i++) {
> > >> > +		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
> > >> > +				DRVVBUS_REG_EN | DRVVBUS_REG,
> > >> > +				DRVVBUS_REG_EN | 0);
> > >> > +	}
> > >> > +}
> > >> 
> > >> drivers/regulator maybe?
> > >
> > > VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> > > enables vbus connected with "regulator" hardware.
> > >
> > > The regulator driver should manage "regulator" hardware, and
> > > I don't think that the driver should manage this register.
> > >
> > >> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
> > >> > +{
> > >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> > >> > +	usleep_range(1000, 2000);
> > >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
> > >> > +}
> > >> > +
> > >> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
> > >> > +{
> > >> > +	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
> > >> > +}
> > >> 
> > >> drivers/reset ?
> > >
> > > The reset driver manages "sysctrl" IO map area in our SoC.
> > >
> > > RESET_CTL register belongs to "dwc3-glue" IO map area,
> > > and the kernel can't access this area until enabling usb clocks and
> > > deasserting usb resets in "sysctrl".
> > >
> > > I think that "dwc3-glue" register control should be separated from
> > > "sysctrl".
> > 
> > Just split your address space and treat your glue as a device with
> > several children:
> > 
> > glue@65b00000 {
> > 	compatible = "foo"
> 
> Just to confirm, instead of SoC-specific glue driver, dwc3-of-simple.c
> should handle compatible "foo", right?

I understood this the answer was no.
We can define glue node as "simple-mfd" and/or "syscon", and the node
can include another function nodes like phy, sysctrl, and even dwc3-of-simple.

> > 
> > 	phy@bar {
> >         	...
> > 	};
> > 
> > 	sysctrl@baz {
> >         	...
> > 	};
> > 
> > 	dwc3@foo {
> >         	compatible = "snps, dwc3";
> >                 ...
> > 	};
> > };
> > 
> > Then you know that you can let dwc3/core.c handle the PHY for you. If we
> > need to teach dwc3/core.c about regulators, we can do that. But we don't
> > need SoC-specific hacks ;-)
> 
> I also don't think that dwc3/core.c should have any SoC-specific controls.
> I've thought the SoC-specific glue driver (dwc3-***.c) calls "clocks",
> "resets", and "regulators".
> 
> However, although dwc3-of-simple.c can handle "clocks", it can't handle
> "resets" and "regulators". Who should call them?

Since The dwc3/core.c calls phy APIs, phy_power_on(), then we can handle
regulators in phy driver.

On the other hand, I think it's necessary to deassert "resets"
before probing dwc3/core.c, same as "clocks".  I'll suggest a patch to solve
the issue next.

Thank you,

---
Best Regards,
Kunihiko Hayashi

Masahiro Yamada Feb. 26, 2018, 2:48 p.m. UTC | #7

Hi Felipe,


2018-01-24 21:58 GMT+09:00 Felipe Balbi <[e-mail]>:
>
> Hi,
>
> Kunihiko Hayashi <[e-mail]> writes:
>> Hello Felipe,
>>
>> Thank you for your comments.
>>
>> On Tue, 23 Jan 2018 15:12:36 +0200 <[e-mail]> wrote:
>>
>>>
>>> Hi,
>>>
>>> Kunihiko Hayashi <[e-mail]> writes:
>>> > Add a specific glue layer for UniPhier SoC platform to support
>>> > USB host mode. It manages hardware operating sequences to enable multiple
>>> > clock gates and assert resets, and to prepare to use dwc3 controller
>>> > on the SoC.
>>> >
>>> > This patch also handles the physical layer that has same register space
>>> > as the glue layer, because it needs to integrate initialziation sequence
>>> > between glue and phy.
>>> >
>>> > In case of some SoCs, since some initialization values for PHY are
>>> > included in nvmem, this patch includes the way to get the values from nvmem.
>>> >
>>> > It supports PXs2 and LD20 SoCs.
>>> >
>>> > Signed-off-by: Kunihiko Hayashi <[e-mail]>
>>> > Signed-off-by: Motoya Tanigawa <[e-mail]>
>>> > Signed-off-by: Masami Hiramatsu <[e-mail]>
>>> > ---
>>> >  drivers/usb/dwc3/Kconfig         |   9 +
>>> >  drivers/usb/dwc3/Makefile        |   1 +
>>> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 +++++++++++++++++++++++++++++++++++++++
>>> >  3 files changed, 564 insertions(+)
>>> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>>
>> ...snip...
>>
>>> > +
>>> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
>>> > +                               u32 data)
>>>
>>> anything with sshphy or hsphy in the name should probably be part of a
>>> PHY driver using drivers/phy/ framework.
>>
>> I can try to separate phy control from this driver.
>> However, phy registers belongs to "dwc3-glue" IO map area (65b00000),
>> and this area also contains a reset bit for "dwc3-core" hardware.
>>
>> Although the phy driver is called from dwc3-core driver,
>> we need to deassert the reset bit before probing dwc3-core driver.
>>
>> As shown later, I think that it's difficut to determine the order of
>> initializing the registers in this area.
>>
>>> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
>>> > +{
>>> > +  int i;
>>> > +
>>> > +  for (i = 0; i < priv->nvbus; i++) {
>>> > +          dwc3u_maskwrite(priv, VBUS_CONTROL(i),
>>> > +                          DRVVBUS_REG_EN | DRVVBUS_REG,
>>> > +                          DRVVBUS_REG_EN | 0);
>>> > +  }
>>> > +}
>>>
>>> drivers/regulator maybe?
>>
>> VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
>> enables vbus connected with "regulator" hardware.
>>
>> The regulator driver should manage "regulator" hardware, and
>> I don't think that the driver should manage this register.
>>
>>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>>> > +{
>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>>> > +  usleep_range(1000, 2000);
>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>>> > +}
>>> > +
>>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>>> > +{
>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>>> > +}
>>>
>>> drivers/reset ?
>>
>> The reset driver manages "sysctrl" IO map area in our SoC.
>>
>> RESET_CTL register belongs to "dwc3-glue" IO map area,
>> and the kernel can't access this area until enabling usb clocks and
>> deasserting usb resets in "sysctrl".
>>
>> I think that "dwc3-glue" register control should be separated from
>> "sysctrl".
>
> Just split your address space and treat your glue as a device with
> several children:
>
> glue@65b00000 {
>         compatible = "foo"
>
>         phy@bar {
>                 ...
>         };
>
>         sysctrl@baz {
>                 ...
>         };
>
>         dwc3@foo {
>                 compatible = "snps, dwc3";
>                 ...
>         };
> };
>
> Then you know that you can let dwc3/core.c handle the PHY for you. If we
> need to teach dwc3/core.c about regulators, we can do that. But we don't
> need SoC-specific hacks ;-)
>
> --
> balbi


Slightly related question.


Why don't we put clocks and resets to dwc3/core.c?

dwc3-of-simple.c only handles clocks and resets.
This is generic enough to be added to dwc3/core.c, I think.


I checked the two instances of dwc3-of-simple.

"qcom,dwc3"
https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/qcom/msm8996.dtsi#L780

"rockchip,rk3399-dwc3"
https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L395


They just contain clocks, resets, and "snps,dwc3" sub-node.


If we do that,

usb@7600000 {
        compatible = "qcom,dwc3";
        clocks = ...;

        dwc3@7600000 {
                compatible = "snps,dwc3";
                reg = ...;
                interrupts = ...;
                phys = ...;
        }
};


will be turned into


dwc3@7600000 {
        compatible = "qcom,dwc3", "snps,dwc3";
        reg = ...;
        clocks = ...;
        interrupts = ...;
        phys = ...;
};


This looks simpler.




Also, dwc3/core.c and dwc3-of-simple.c
duplicate runtime PM.

Felipe Balbi March 9, 2018, 9:04 a.m. UTC | #8

Hi,

Masahiro Yamada <[e-mail]> writes:
>>>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>>>> > +{
>>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>>>> > +  usleep_range(1000, 2000);
>>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>>>> > +}
>>>> > +
>>>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>>>> > +{
>>>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>>>> > +}
>>>>
>>>> drivers/reset ?
>>>
>>> The reset driver manages "sysctrl" IO map area in our SoC.
>>>
>>> RESET_CTL register belongs to "dwc3-glue" IO map area,
>>> and the kernel can't access this area until enabling usb clocks and
>>> deasserting usb resets in "sysctrl".
>>>
>>> I think that "dwc3-glue" register control should be separated from
>>> "sysctrl".
>>
>> Just split your address space and treat your glue as a device with
>> several children:
>>
>> glue@65b00000 {
>>         compatible = "foo"
>>
>>         phy@bar {
>>                 ...
>>         };
>>
>>         sysctrl@baz {
>>                 ...
>>         };
>>
>>         dwc3@foo {
>>                 compatible = "snps, dwc3";
>>                 ...
>>         };
>> };
>>
>> Then you know that you can let dwc3/core.c handle the PHY for you. If we
>> need to teach dwc3/core.c about regulators, we can do that. But we don't
>> need SoC-specific hacks ;-)
>>
>> --
>> balbi
>
>
> Slightly related question.
>
>
> Why don't we put clocks and resets to dwc3/core.c?

We can do that for the simpler platforms, no worries.

> dwc3-of-simple.c only handles clocks and resets.
> This is generic enough to be added to dwc3/core.c, I think.
>
>
> I checked the two instances of dwc3-of-simple.
>
> "qcom,dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/qcom/msm8996.dtsi#L780
>
> "rockchip,rk3399-dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L395
>
>
> They just contain clocks, resets, and "snps,dwc3" sub-node.
>
>
> If we do that,
>
> usb@7600000 {
>         compatible = "qcom,dwc3";
>         clocks = ...;
>
>         dwc3@7600000 {
>                 compatible = "snps,dwc3";
>                 reg = ...;
>                 interrupts = ...;
>                 phys = ...;
>         }
> };
>
>
> will be turned into
>
>
> dwc3@7600000 {
>         compatible = "qcom,dwc3", "snps,dwc3";
>         reg = ...;
>         clocks = ...;
>         interrupts = ...;
>         phys = ...;
> };
>
>
> This looks simpler.

yup. This will only work for the simpler platforms, though. TI platforms
and PCI-based platforms, this really won't work :-)

> Also, dwc3/core.c and dwc3-of-simple.c
> duplicate runtime PM.

they "kinda" duplicate :-) Some platforms have platform-specific clocks
which are not generic enough to be stuffed into dwc3/core.c. Some of
those clocks may need special handling of some sorts. It's best to keep
the option for peculiar clock tree setups available.

If your platform is simple enough that you can get away without a glue
layer, sure thing. More power for you :-)

Patch

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index ab8c0e0..a5cadc6 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -106,4 +106,13 @@  config USB_DWC3_ST
 	  inside (i.e. STiH407).
 	  Say 'Y' or 'M' if you have one such device.
 
+config USB_DWC3_UNIPHIER
+	tristate "Socionext UniPhier Platforms"
+	depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
+	default USB_DWC3
+	help
+	  Support USB2/3 functionality in UniPhier platforms.
+	  Say 'Y' or 'M' if your system that UniPhier SoC is implemented
+	  has USB controllers based on DWC USB3 IP.
+
 endif
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index 7ac7250..31e82b3 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -48,3 +48,4 @@  obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
 obj-$(CONFIG_USB_DWC3_KEYSTONE)		+= dwc3-keystone.o
 obj-$(CONFIG_USB_DWC3_OF_SIMPLE)	+= dwc3-of-simple.o
 obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
+obj-$(CONFIG_USB_DWC3_UNIPHIER)		+= dwc3-uniphier.o
diff --git a/drivers/usb/dwc3/dwc3-uniphier.c b/drivers/usb/dwc3/dwc3-uniphier.c
new file mode 100644
index 0000000..58e84cd
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-uniphier.c
@@ -0,0 +1,554 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * dwc3-uniphier.c - Socionext UniPhier DWC3 specific glue layer
+ *
+ * Copyright 2015-2018 Socionext Inc.
+ *
+ * Author:
+ *	Kunihiko Hayashi <[e-mail]>
+ * Contributors:
+ *      Motoya Tanigawa <[e-mail]>
+ *      Masami Hiramatsu <[e-mail]>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/slab.h>
+
+#define RESET_CTL		0x000
+#define LINK_RESET		BIT(15)
+
+#define VBUS_CONTROL(n)		(0x100 + 0x10 * (n))
+#define DRVVBUS_REG		BIT(4)
+#define DRVVBUS_REG_EN		BIT(3)
+
+#define U2PHY_CFG0(n)		(0x200 + 0x10 * (n))
+#define U2PHY_CFG0_HS_I_MASK	GENMASK(31, 28)
+#define U2PHY_CFG0_HSDISC_MASK	GENMASK(27, 26)
+#define U2PHY_CFG0_SWING_MASK	GENMASK(17, 16)
+#define U2PHY_CFG0_SEL_T_MASK	GENMASK(15, 12)
+#define U2PHY_CFG0_RTERM_MASK	GENMASK(7, 6)
+#define U2PHY_CFG0_TRIMMASK	(U2PHY_CFG0_HS_I_MASK \
+				 | U2PHY_CFG0_SEL_T_MASK \
+				 | U2PHY_CFG0_RTERM_MASK)
+
+#define U2PHY_CFG1(n)		(0x204 + 0x10 * (n))
+#define U2PHY_CFG1_DAT_EN	BIT(29)
+#define U2PHY_CFG1_ADR_EN	BIT(28)
+#define U2PHY_CFG1_ADR_MASK	GENMASK(27, 16)
+#define U2PHY_CFG1_DAT_MASK	GENMASK(23, 16)
+
+#define U3PHY_TESTI(n)		(0x300 + 0x10 * (n))
+#define U3PHY_TESTO(n)		(0x304 + 0x10 * (n))
+#define TESTI_DAT_MASK		GENMASK(13, 6)
+#define TESTI_ADR_MASK		GENMASK(5, 1)
+#define TESTI_WR_EN		BIT(0)
+
+#define HOST_CONFIG0		0x400
+#define NUM_U3_MASK		GENMASK(13, 11)
+#define NUM_U2_MASK		GENMASK(10, 8)
+
+#define PHY_MAX_PARAMS	32
+
+struct dwc3u_phy_param {
+	u32 addr;
+	u32 mask;
+	u32 val;
+};
+
+struct dwc3u_trim_param {
+	u32 rterm;
+	u32 sel_t;
+	u32 hs_i;
+};
+
+#define trim_param_is_valid(p)	((p)->rterm || (p)->sel_t || (p)->hs_i)
+
+struct dwc3u_priv {
+	struct device		*dev;
+	void __iomem		*base;
+	struct clk		**clks;
+	int			nclks;
+	struct reset_control	*rst;
+	int			nvbus;
+	const struct dwc3u_soc_data	*data;
+};
+
+struct dwc3u_soc_data {
+	int ss_nparams;
+	struct dwc3u_phy_param ss_param[PHY_MAX_PARAMS];
+	int hs_nparams;
+	struct dwc3u_phy_param hs_param[PHY_MAX_PARAMS];
+	u32 hs_config0;
+	u32 hs_config1;
+	void (*trim_func)(struct dwc3u_priv *priv, u32 *pconfig,
+			  struct dwc3u_trim_param *trim);
+};
+
+static inline u32 dwc3u_read(struct dwc3u_priv *priv, off_t offset)
+{
+	return readl(priv->base + offset);
+}
+
+static inline void dwc3u_write(struct dwc3u_priv *priv,
+			       off_t offset, u32 val)
+{
+	writel(val, priv->base + offset);
+}
+
+static inline void dwc3u_maskwrite(struct dwc3u_priv *priv,
+				   off_t offset, u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = dwc3u_read(priv, offset);
+	dwc3u_write(priv, offset, (tmp & ~mask) | (val & mask));
+}
+
+static int dwc3u_get_hsport_num(struct dwc3u_priv *priv)
+{
+	return FIELD_GET(NUM_U2_MASK, dwc3u_read(priv, HOST_CONFIG0));
+}
+
+static int dwc3u_get_ssport_num(struct dwc3u_priv *priv)
+{
+	return FIELD_GET(NUM_U3_MASK, dwc3u_read(priv, HOST_CONFIG0));
+}
+
+static int dwc3u_get_nvparam(struct dwc3u_priv *priv,
+			     const char *basename, int index, u8 *dst,
+			     int maxlen)
+{
+	struct nvmem_cell *cell;
+	char name[16];
+	size_t len;
+	u8 *buf;
+
+	snprintf(name, sizeof(name) - 1, "%s%d", basename, index);
+	memset(dst, 0, maxlen);
+
+	cell = nvmem_cell_get(priv->dev, name);
+	if (IS_ERR(cell))
+		return PTR_ERR(cell);
+
+	buf = nvmem_cell_read(cell, &len);
+	nvmem_cell_put(cell);
+	if (IS_ERR(buf))
+		return PTR_ERR(buf);
+
+	len = min_t(u32, len, maxlen);
+	memcpy(dst, buf, len);
+	kfree(buf);
+
+	return 0;
+}
+
+static int dwc3u_get_nvparam_u32(struct dwc3u_priv *priv,
+				 const char *basename, int index, u32 *p_val)
+{
+	return dwc3u_get_nvparam(priv, basename, index, (u8 *)p_val,
+				 sizeof(u32));
+}
+
+static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
+				     u32 data)
+{
+	/* need to read TESTO twice after accessing TESTI */
+	dwc3u_write(priv, U3PHY_TESTI(port), data);
+	dwc3u_read(priv, U3PHY_TESTO(port));
+	dwc3u_read(priv, U3PHY_TESTO(port));
+}
+
+static void dwc3u_ssphy_set_param(struct dwc3u_priv *priv, int port,
+				  const struct dwc3u_phy_param *p)
+{
+	u32 val, val_prev;
+
+	/* read previous data */
+	dwc3u_ssphy_testio_write(priv, port,
+				 FIELD_PREP(TESTI_DAT_MASK, 1) |
+				 FIELD_PREP(TESTI_ADR_MASK, p->addr));
+	val_prev = dwc3u_read(priv, U3PHY_TESTO(port));
+
+	/* update value */
+	val = FIELD_PREP(TESTI_DAT_MASK,
+			 (val_prev & ~p->mask) | (p->val & p->mask)) |
+		FIELD_PREP(TESTI_ADR_MASK, p->addr);
+
+	dwc3u_ssphy_testio_write(priv, port, val);
+	dwc3u_ssphy_testio_write(priv, port, val | TESTI_WR_EN);
+	dwc3u_ssphy_testio_write(priv, port, val);
+
+	/* read current data as dummy */
+	dwc3u_ssphy_testio_write(priv, port,
+				 FIELD_PREP(TESTI_DAT_MASK, 1) |
+				 FIELD_PREP(TESTI_ADR_MASK, p->addr));
+	dwc3u_read(priv, U3PHY_TESTO(port));
+}
+
+static void dwc3u_ssphy_init(struct dwc3u_priv *priv)
+{
+	int nparams = min_t(u32, priv->data->ss_nparams, PHY_MAX_PARAMS);
+	int nports = dwc3u_get_ssport_num(priv);
+	int i, j;
+
+	for (i = 0; i < nports; i++)
+		for (j = 0; j < nparams; j++)
+			dwc3u_ssphy_set_param(priv, i,
+					      &priv->data->ss_param[j]);
+}
+
+static void dwc3u_hsphy_trim_ld20(struct dwc3u_priv *priv, u32 *pconfig,
+				  struct dwc3u_trim_param *ptrim)
+{
+	*pconfig = (*pconfig & ~U2PHY_CFG0_TRIMMASK) |
+		FIELD_PREP(U2PHY_CFG0_RTERM_MASK, ptrim->rterm) |
+		FIELD_PREP(U2PHY_CFG0_SEL_T_MASK, ptrim->sel_t) |
+		FIELD_PREP(U2PHY_CFG0_HS_I_MASK,  ptrim->hs_i);
+}
+
+static int dwc3u_hsphy_get_nvparams(struct dwc3u_priv *priv, int port,
+				    struct dwc3u_trim_param *ptrim)
+{
+	int ret;
+
+	ret = dwc3u_get_nvparam_u32(priv, "rterm", port, &ptrim->rterm);
+	if (ret)
+		return ret;
+
+	ret = dwc3u_get_nvparam_u32(priv, "sel_t", port, &ptrim->sel_t);
+	if (ret)
+		return ret;
+
+	return dwc3u_get_nvparam_u32(priv, "hs_i", port, &ptrim->hs_i);
+}
+
+static int dwc3u_hsphy_update_config(struct dwc3u_priv *priv, int port,
+				     u32 *pconfig)
+{
+	struct dwc3u_trim_param trim;
+	int ret, trimmed = 0;
+
+	if (priv->data->trim_func) {
+		ret = dwc3u_hsphy_get_nvparams(priv, port, &trim);
+		if (ret == -EPROBE_DEFER)
+			return ret;
+
+		/*
+		 * call trim_func only when trimming parameters that aren't
+		 * all-zero can be acquired. All-zero parameters mean nothing
+		 * has been written to nvmem.
+		 */
+		if (!ret && trim_param_is_valid(&trim)) {
+			priv->data->trim_func(priv, pconfig, &trim);
+			trimmed = 1;
+		} else {
+			dev_dbg(priv->dev,
+				"can't get parameter for port%d from nvmem\n",
+				port);
+		}
+	}
+
+	/* use default parameters without trimming values */
+	if (!trimmed)
+		*pconfig = (*pconfig & ~U2PHY_CFG0_HSDISC_MASK) |
+			FIELD_PREP(U2PHY_CFG0_HSDISC_MASK, 3);
+
+	return 0;
+}
+
+static void dwc3u_hsphy_set_config(struct dwc3u_priv *priv, int port,
+				   u32 config0, u32 config1)
+{
+	dwc3u_write(priv, U2PHY_CFG0(port), config0);
+	dwc3u_write(priv, U2PHY_CFG1(port), config1);
+
+	dwc3u_maskwrite(priv, U2PHY_CFG0(port),
+			U2PHY_CFG0_SWING_MASK,
+			FIELD_PREP(U2PHY_CFG0_SWING_MASK, 2));
+}
+
+static void dwc3u_hsphy_set_param(struct dwc3u_priv *priv, int port,
+				  const struct dwc3u_phy_param *p)
+{
+	dwc3u_maskwrite(priv, U2PHY_CFG1(port),
+			U2PHY_CFG1_ADR_EN | U2PHY_CFG1_ADR_MASK,
+			U2PHY_CFG1_ADR_EN |
+			FIELD_PREP(U2PHY_CFG1_ADR_MASK, p->addr));
+	dwc3u_maskwrite(priv, U2PHY_CFG1(port),
+			U2PHY_CFG1_ADR_EN, 0);
+
+	dwc3u_maskwrite(priv, U2PHY_CFG1(port),
+			U2PHY_CFG1_DAT_EN |
+			FIELD_PREP(U2PHY_CFG1_DAT_MASK, p->mask),
+			U2PHY_CFG1_DAT_EN |
+			FIELD_PREP(U2PHY_CFG1_DAT_MASK, p->val));
+	dwc3u_maskwrite(priv, U2PHY_CFG1(port),
+			U2PHY_CFG1_DAT_EN, 0);
+}
+
+static int dwc3u_hsphy_init(struct dwc3u_priv *priv)
+{
+	int nparams = min(priv->data->hs_nparams, PHY_MAX_PARAMS);
+	int nports = dwc3u_get_hsport_num(priv);
+	u32 config0, config1;
+	int i, ret, port;
+
+	for (port = 0; port < nports; port++) {
+		config0 = priv->data->hs_config0;
+		config1 = priv->data->hs_config1;
+
+		ret = dwc3u_hsphy_update_config(priv, port, &config0);
+		if (ret)
+			return ret;
+
+		dwc3u_hsphy_set_config(priv, port, config0, config1);
+
+		for (i = 0; i < nparams; i++)
+			dwc3u_hsphy_set_param(priv, port,
+					      &priv->data->hs_param[i]);
+	}
+
+	return 0;
+}
+
+static int dwc3u_phy_init(struct dwc3u_priv *priv)
+{
+	dwc3u_ssphy_init(priv);
+
+	return dwc3u_hsphy_init(priv);
+}
+
+static void dwc3u_vbus_enable(struct dwc3u_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->nvbus; i++) {
+		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
+				DRVVBUS_REG_EN | DRVVBUS_REG,
+				DRVVBUS_REG_EN | DRVVBUS_REG);
+	}
+}
+
+static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->nvbus; i++) {
+		dwc3u_maskwrite(priv, VBUS_CONTROL(i),
+				DRVVBUS_REG_EN | DRVVBUS_REG,
+				DRVVBUS_REG_EN | 0);
+	}
+}
+
+static void dwc3u_reset_init(struct dwc3u_priv *priv)
+{
+	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
+	usleep_range(1000, 2000);
+	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
+}
+
+static void dwc3u_reset_clear(struct dwc3u_priv *priv)
+{
+	dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
+}
+
+static int dwc3u_init(struct dwc3u_priv *priv)
+{
+	int nr_hsports, nr_ssports;
+	int ret;
+
+	nr_hsports = dwc3u_get_hsport_num(priv);
+	nr_ssports = dwc3u_get_ssport_num(priv);
+	priv->nvbus = max(nr_hsports, nr_ssports);
+
+	dwc3u_vbus_enable(priv);
+
+	ret = dwc3u_phy_init(priv);
+	if (ret)
+		return ret;
+
+	dwc3u_reset_init(priv);
+
+	return 0;
+}
+
+static void dwc3u_exit(struct dwc3u_priv *priv)
+{
+	dwc3u_reset_clear(priv);
+	dwc3u_vbus_disable(priv);
+}
+
+static void dwc3u_disable_clk(struct dwc3u_priv *priv)
+{
+	int i;
+
+	for (i = 0; i < priv->nclks; i++) {
+		clk_disable_unprepare(priv->clks[i]);
+		clk_put(priv->clks[i]);
+	}
+}
+
+static int dwc3u_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *node;
+	struct dwc3u_priv *priv;
+	struct resource	*res;
+	struct clk *clk;
+	int i, nr_clks;
+	int ret = 0;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->data = of_device_get_match_data(dev);
+	if (WARN_ON(!priv->data))
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	priv->dev = dev;
+
+	node = dev->of_node;
+	nr_clks = of_clk_get_parent_count(node);
+	if (!nr_clks) {
+		dev_err(dev, "failed to get clock property\n");
+		return -ENODEV;
+	}
+
+	priv->clks = devm_kcalloc(priv->dev, nr_clks, sizeof(struct clk *),
+				  GFP_KERNEL);
+	if (!priv->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < nr_clks; i++) {
+		clk = of_clk_get(node, i);
+		if (IS_ERR(clk)) {
+			ret = PTR_ERR(clk);
+			goto out_clk_disable;
+		}
+		ret = clk_prepare_enable(clk);
+		if (ret < 0) {
+			clk_put(clk);
+			goto out_clk_disable;
+		}
+		priv->clks[i] = clk;
+		priv->nclks = i;
+	}
+
+	priv->rst = devm_reset_control_array_get_optional_shared(priv->dev);
+	if (IS_ERR(priv->rst)) {
+		ret = PTR_ERR(priv->rst);
+		goto out_clk_disable;
+	}
+	ret = reset_control_deassert(priv->rst);
+	if (ret)
+		goto out_clk_disable;
+
+	ret = dwc3u_init(priv);
+	if (ret)
+		goto out_rst_assert;
+
+	platform_set_drvdata(pdev, priv);
+
+	ret = of_platform_populate(node, NULL, NULL, priv->dev);
+	if (ret)
+		goto out_exit;
+
+	return 0;
+
+out_exit:
+	dwc3u_exit(priv);
+out_rst_assert:
+	reset_control_assert(priv->rst);
+out_clk_disable:
+	dwc3u_disable_clk(priv);
+
+	return ret;
+}
+
+static int dwc3u_remove(struct platform_device *pdev)
+{
+	struct dwc3u_priv *priv = platform_get_drvdata(pdev);
+
+	of_platform_depopulate(&pdev->dev);
+	dwc3u_exit(priv);
+
+	reset_control_assert(priv->rst);
+	dwc3u_disable_clk(priv);
+
+	return 0;
+}
+
+static const struct dwc3u_soc_data dwc3u_pxs2_data = {
+	.ss_nparams = 7,
+	.ss_param = {
+		{  7, 0x0f, 0x0a },
+		{  8, 0x0f, 0x03 },
+		{  9, 0x0f, 0x05 },
+		{ 11, 0x0f, 0x09 },
+		{ 13, 0x60, 0x40 },
+		{ 27, 0x07, 0x07 },
+		{ 28, 0x03, 0x01 },
+	},
+	.hs_nparams = 0,
+};
+
+static const struct dwc3u_soc_data dwc3u_ld20_data = {
+	.ss_nparams = 3,
+	.ss_param = {
+		{  7, 0x0f, 0x06 },
+		{ 13, 0xff, 0xcc },
+		{ 26, 0xf0, 0x50 },
+	},
+	.hs_nparams = 1,
+	.hs_param = {
+		{ 10, 0x60, 0x60 },
+	},
+	.trim_func = dwc3u_hsphy_trim_ld20,
+	.hs_config0 = 0x92306680,
+	.hs_config1 = 0x00000106,
+};
+
+static const struct of_device_id of_dwc3u_match[] = {
+	{
+		.compatible = "socionext,uniphier-pxs2-dwc3",
+		.data = &dwc3u_pxs2_data,
+	},
+	{
+		.compatible = "socionext,uniphier-ld20-dwc3",
+		.data = &dwc3u_ld20_data,
+	},
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, of_dwc3u_match);
+
+static struct platform_driver dwc3u_driver = {
+	.probe = dwc3u_probe,
+	.remove = dwc3u_remove,
+	.driver	= {
+		.name = "uniphier-dwc3",
+		.of_match_table	= of_dwc3u_match,
+	},
+};
+
+module_platform_driver(dwc3u_driver);
+
+MODULE_AUTHOR("Kunihiko Hayashi <[e-mail]>");
+MODULE_DESCRIPTION("DesignWare USB3 UniPhier glue layer");
+MODULE_LICENSE("GPL v2");