[PATCH 2/6] arm: shmobile: lager: Add USBHS support

[PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine Barshak]

This adds USBHS phy control callbacks to the Lager board and
registers USBHS device if the driver is enabled.

Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
---
 arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
index a8d3ce6..e408fc7 100644
--- a/arch/arm/mach-shmobile/board-lager.c
+++ b/arch/arm/mach-shmobile/board-lager.c
@@ -18,6 +18,7 @@
  * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
  */
 
+#include <linux/clk.h>
 #include <linux/gpio.h>
 #include <linux/gpio_keys.h>
 #include <linux/input.h>
@@ -34,6 +35,7 @@
 #include <linux/regulator/fixed.h>
 #include <linux/regulator/machine.h>
 #include <linux/sh_eth.h>
+#include <linux/usb/renesas_usbhs.h>
 #include <mach/common.h>
 #include <mach/irqs.h>
 #include <mach/r8a7790.h>
@@ -165,6 +167,142 @@ static const struct resource ether_resources[] __initconst = {
 	DEFINE_RES_IRQ(gic_spi(162)),
 };
 
+/* USBHS */
+#if IS_ENABLED(CONFIG_USB_RENESAS_USBHS_UDC)
+static const struct resource usbhs_resources[] __initconst = {
+	DEFINE_RES_MEM(0xe6590000, 0x200),
+	DEFINE_RES_IRQ(gic_spi(107)),
+};
+
+/* USBHS registers */
+#define USBHS_LPSTS_REG			0x102
+#define USBHS_LPSTS_SUSPM		(1 << 14)
+
+#define USBHS_UGCTRL_REG		0x180
+#define USBHS_UGCTRL_CONNECT		(1 << 2)
+#define USBHS_UGCTRL_PLLRESET		(1 << 0)
+
+#define USBHS_UGCTRL2_REG		0x184
+#define USBHS_UGCTRL2_USB0_PCI		(1 << 4)
+#define USBHS_UGCTRL2_USB0_HS		(3 << 4)
+#define USBHS_UGCTRL2_USB2_PCI		(0 << 31)
+#define USBHS_UGCTRL2_USB2_SS		(1 << 31)
+
+#define USBHS_UGSTS_REG			0x190
+#define USBHS_UGSTS_LOCK		(3 << 0)
+
+struct usbhs_private {
+	struct renesas_usbhs_platform_info info;
+	struct platform_device *pdev;
+	struct clk *clk;
+};
+
+#define usbhs_get_priv(pdev) \
+	container_of(renesas_usbhs_get_info(pdev), struct usbhs_private, info)
+
+static int usbhs_power_on(void __iomem *base)
+{
+	u32 val;
+
+	val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
+	iowrite32(val, base + USBHS_UGCTRL_REG);
+
+	val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
+	iowrite16(val, base + USBHS_LPSTS_REG);
+
+	/*
+	 * The manual suggests to check PLL lock status in the UGSTS
+	 * register before enabling connect, however, it is always 0.
+	 */
+	val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
+	iowrite32(val, base + USBHS_UGCTRL_REG);
+	return 0;
+}
+
+static int usbhs_power_off(void __iomem *base)
+{
+	u32 val;
+
+	val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
+	iowrite32(val, base + USBHS_UGCTRL_REG);
+
+	val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
+	iowrite16(val, base + USBHS_LPSTS_REG);
+
+	val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
+	iowrite32(val, base + USBHS_UGCTRL_REG);
+	return 0;
+}
+
+static int usbhs_power_ctrl(struct platform_device *pdev,
+				void __iomem *base, int enable)
+{
+	if (enable)
+		return usbhs_power_on(base);
+
+	return usbhs_power_off(base);
+}
+
+static int usbhs_get_id(struct platform_device *pdev)
+{
+	return USBHS_GADGET;
+}
+
+static int usbhs_hardware_init(struct platform_device *pdev)
+{
+	struct usbhs_private *priv = usbhs_get_priv(pdev);
+	struct clk *clk;
+
+	clk = clk_get(NULL, "hsusb");
+	if (IS_ERR(clk))
+		return -ENODEV;
+
+	/* Enable clocks */
+	clk_enable(clk);
+	priv->clk = clk;
+	priv->pdev = pdev;
+	return 0;
+}
+
+static int usbhs_hardware_exit(struct platform_device *pdev)
+{
+	struct usbhs_private *priv = usbhs_get_priv(pdev);
+
+	if (!priv->clk)
+		return 0;
+
+	/* Disable clocks */
+	clk_disable(priv->clk);
+	clk_put(priv->clk);
+	priv->clk = NULL;
+	return 0;
+}
+
+static struct usbhs_private usbhs_priv __initdata = {
+	.info = {
+		.platform_callback = {
+			.power_ctrl	= usbhs_power_ctrl,
+			.get_id		= usbhs_get_id,
+			.hardware_init	= usbhs_hardware_init,
+			.hardware_exit	= usbhs_hardware_exit,
+		},
+		.driver_param = {
+			.buswait_bwait	= 4,
+		},
+	},
+};
+
+#define lager_register_usbhs()						\
+	platform_device_register_resndata(&platform_bus,		\
+					  "renesas_usbhs", -1,		\
+					  usbhs_resources,		\
+					  ARRAY_SIZE(usbhs_resources),	\
+					  &usbhs_priv.info,		\
+					  sizeof(usbhs_priv.info))
+#else	/* CONFIG_USB_RENESAS_USBHS_UDC */
+#define lager_register_usbhs()
+#endif	/* CONFIG_USB_RENESAS_USBHS_UDC */
+
 static const struct pinctrl_map lager_pinctrl_map[] = {
 	/* DU (CN10: ARGB0, CN13: LVDS) */
 	PIN_MAP_MUX_GROUP_DEFAULT("rcar-du-r8a7790", "pfc-r8a7790",
@@ -193,6 +331,9 @@ static const struct pinctrl_map lager_pinctrl_map[] = {
 				  "eth_rmii", "eth"),
 	PIN_MAP_MUX_GROUP_DEFAULT("r8a7790-ether", "pfc-r8a7790",
 				  "intc_irq0", "intc"),
+	/* USB0 */
+	PIN_MAP_MUX_GROUP_DEFAULT("renesas_usbhs", "pfc-r8a7790",
+				  "usb0", "usb0"),
 };
 
 static void __init lager_add_standard_devices(void)
@@ -222,6 +363,7 @@ static void __init lager_add_standard_devices(void)
 					  &ether_pdata, sizeof(ether_pdata));
 
 	lager_add_du_device();
+	lager_register_usbhs();
 }
 
 /*
-- 
1.8.3.1

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Kuninori Morimoto]

Hi Valentine

> This adds USBHS phy control callbacks to the Lager board and
> registers USBHS device if the driver is enabled.
> 
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> ---
>  arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index a8d3ce6..e408fc7 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c
(snip)

> +static int usbhs_hardware_init(struct platform_device *pdev)
> +{
> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
> +	struct clk *clk;
> +
> +	clk = clk_get(NULL, "hsusb");
> +	if (IS_ERR(clk))
> +		return -ENODEV;

It is automatically enable/disabled by driver
if MSTP704 clock name was "renesas_usbhs".

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Magnus Damm]

Hi Valentine,

Thanks for your patches.

On Wed, Oct 2, 2013 at 3:30 AM, Valentine Barshak
<valentine.barshak@cogentembedded.com> wrote:
> This adds USBHS phy control callbacks to the Lager board and
> registers USBHS device if the driver is enabled.
>
> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> ---
>  arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
>
> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> index a8d3ce6..e408fc7 100644
> --- a/arch/arm/mach-shmobile/board-lager.c
> +++ b/arch/arm/mach-shmobile/board-lager.c

Starting from here...

> +/* USBHS registers */
> +#define USBHS_LPSTS_REG                        0x102
> +#define USBHS_LPSTS_SUSPM              (1 << 14)
> +
> +#define USBHS_UGCTRL_REG               0x180
> +#define USBHS_UGCTRL_CONNECT           (1 << 2)
> +#define USBHS_UGCTRL_PLLRESET          (1 << 0)
> +
> +#define USBHS_UGCTRL2_REG              0x184
> +#define USBHS_UGCTRL2_USB0_PCI         (1 << 4)
> +#define USBHS_UGCTRL2_USB0_HS          (3 << 4)
> +#define USBHS_UGCTRL2_USB2_PCI         (0 << 31)
> +#define USBHS_UGCTRL2_USB2_SS          (1 << 31)
> +
> +#define USBHS_UGSTS_REG                        0x190
> +#define USBHS_UGSTS_LOCK               (3 << 0)
> +
> +struct usbhs_private {
> +       struct renesas_usbhs_platform_info info;
> +       struct platform_device *pdev;
> +       struct clk *clk;
> +};
> +
> +#define usbhs_get_priv(pdev) \
> +       container_of(renesas_usbhs_get_info(pdev), struct usbhs_private, info)
> +
> +static int usbhs_power_on(void __iomem *base)
> +{
> +       u32 val;
> +
> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> +
> +       val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
> +       iowrite16(val, base + USBHS_LPSTS_REG);
> +
> +       /*
> +        * The manual suggests to check PLL lock status in the UGSTS
> +        * register before enabling connect, however, it is always 0.
> +        */
> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> +       return 0;
> +}
> +
> +static int usbhs_power_off(void __iomem *base)
> +{
> +       u32 val;
> +
> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> +
> +       val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
> +       iowrite16(val, base + USBHS_LPSTS_REG);
> +
> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> +       return 0;
> +}
> +
> +static int usbhs_power_ctrl(struct platform_device *pdev,
> +                               void __iomem *base, int enable)
> +{
> +       if (enable)
> +               return usbhs_power_on(base);
> +
> +       return usbhs_power_off(base);
> +}

... to here it looks like this is USBHS support code for the r8a7790
SoC, not the lager board.

Have you considered putting these SoC specific bits into drivers/usb/...

Thanks,

/ magnus

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/02/2013 04:09 AM, Kuninori Morimoto wrote:
>
> Hi Valentine

Hi Morimoto-san,

> (snip)
>
>> +static int usbhs_hardware_init(struct platform_device *pdev)
>> +{
>> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
>> +	struct clk *clk;
>> +
>> +	clk = clk_get(NULL, "hsusb");
>> +	if (IS_ERR(clk))
>> +		return -ENODEV;
>
> It is automatically enable/disabled by driver
> if MSTP704 clock name was "renesas_usbhs".

The reason I did not bind usbhs clock to renesas_usbhs device is because
the same clock is also used by the lager_add_usb_devices() function in 
the next patches. We need that since the global USB settings that affect 
USBHS/USBSS and PCI USB host channel sharing are done in the USBHS 
UGCTRL2 register. So we need this clock even if the renesas_usbhs driver 
is disabled. IIUC, biding it to "renesas_usbhs" device would make it 
impossible to use the clock if renesas_usbhs device is not registered.

Thanks,
Val.

>
> Best regards
> ---
> Kuninori Morimoto
>

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/02/2013 04:18 AM, Magnus Damm wrote:
> Hi Valentine,

Hi Magnus,

>
> Thanks for your patches.
>
> On Wed, Oct 2, 2013 at 3:30 AM, Valentine Barshak
> <valentine.barshak@cogentembedded.com> wrote:
>> This adds USBHS phy control callbacks to the Lager board and
>> registers USBHS device if the driver is enabled.
>>
>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> ---
>>   arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 142 insertions(+)
>>
>> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
>> index a8d3ce6..e408fc7 100644
>> --- a/arch/arm/mach-shmobile/board-lager.c
>> +++ b/arch/arm/mach-shmobile/board-lager.c
>
> Starting from here...
>
>> +/* USBHS registers */
>> +#define USBHS_LPSTS_REG                        0x102
>> +#define USBHS_LPSTS_SUSPM              (1 << 14)
>> +
>> +#define USBHS_UGCTRL_REG               0x180
>> +#define USBHS_UGCTRL_CONNECT           (1 << 2)
>> +#define USBHS_UGCTRL_PLLRESET          (1 << 0)
>> +
>> +#define USBHS_UGCTRL2_REG              0x184
>> +#define USBHS_UGCTRL2_USB0_PCI         (1 << 4)
>> +#define USBHS_UGCTRL2_USB0_HS          (3 << 4)
>> +#define USBHS_UGCTRL2_USB2_PCI         (0 << 31)
>> +#define USBHS_UGCTRL2_USB2_SS          (1 << 31)
>> +
>> +#define USBHS_UGSTS_REG                        0x190
>> +#define USBHS_UGSTS_LOCK               (3 << 0)
>> +
>> +struct usbhs_private {
>> +       struct renesas_usbhs_platform_info info;
>> +       struct platform_device *pdev;
>> +       struct clk *clk;
>> +};
>> +
>> +#define usbhs_get_priv(pdev) \
>> +       container_of(renesas_usbhs_get_info(pdev), struct usbhs_private, info)
>> +
>> +static int usbhs_power_on(void __iomem *base)
>> +{
>> +       u32 val;
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +
>> +       val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
>> +       iowrite16(val, base + USBHS_LPSTS_REG);
>> +
>> +       /*
>> +        * The manual suggests to check PLL lock status in the UGSTS
>> +        * register before enabling connect, however, it is always 0.
>> +        */
>> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +       return 0;
>> +}
>> +
>> +static int usbhs_power_off(void __iomem *base)
>> +{
>> +       u32 val;
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +
>> +       val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
>> +       iowrite16(val, base + USBHS_LPSTS_REG);
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +       return 0;
>> +}
>> +
>> +static int usbhs_power_ctrl(struct platform_device *pdev,
>> +                               void __iomem *base, int enable)
>> +{
>> +       if (enable)
>> +               return usbhs_power_on(base);
>> +
>> +       return usbhs_power_off(base);
>> +}
>
> ... to here it looks like this is USBHS support code for the r8a7790
> SoC, not the lager board.
>
> Have you considered putting these SoC specific bits into drivers/usb/...

I have. But I've decided to put all the USBHS-specific stuff into
board-specific code because I'm not sure why the PLL lock is always 0 on 
Lager (whether it's a SoC or board specific issue) and I don't have 
other boards with the same SoC to test. Even though the PLL lock status 
doesn't seem to behave as described in the docs, I haven't noticed any
issues with the USBHS.

I can move it to SoC files, but I'm once again not sure whether it 
should go into setup-r8a7790 or setup-rcar-gen2 because both SoCs seem 
to have identical USB subsystem.

>
> Thanks,
>
> / magnus
>

Thanks,
Val.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Laurent Pinchart]

Hi Valentine,

On Wednesday 02 October 2013 16:06:33 Valentine wrote:
> On 10/02/2013 04:09 AM, Kuninori Morimoto wrote:
> > Hi Valentine
> 
> Hi Morimoto-san,
> 
> > (snip)
> > 
> >> +static int usbhs_hardware_init(struct platform_device *pdev)
> >> +{
> >> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
> >> +	struct clk *clk;
> >> +
> >> +	clk = clk_get(NULL, "hsusb");
> >> +	if (IS_ERR(clk))
> >> +		return -ENODEV;
> > 
> > It is automatically enable/disabled by driver
> > if MSTP704 clock name was "renesas_usbhs".
> 
> The reason I did not bind usbhs clock to renesas_usbhs device is because
> the same clock is also used by the lager_add_usb_devices() function in
> the next patches. We need that since the global USB settings that affect
> USBHS/USBSS and PCI USB host channel sharing are done in the USBHS
> UGCTRL2 register. So we need this clock even if the renesas_usbhs driver
> is disabled. IIUC, biding it to "renesas_usbhs" device would make it
> impossible to use the clock if renesas_usbhs device is not registered.

Can't the code from patch 6/6 that needs to enable the clock be moved to a 
proper device driver ? In that case you could just add an entry for that 
device in the lookups array in arch/arm/mach-shmobile/clock-r8a7790.c.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/02/2013 11:45 PM, Laurent Pinchart wrote:
> Hi Valentine,
>

Hi Laurent,

> On Wednesday 02 October 2013 16:06:33 Valentine wrote:
>> On 10/02/2013 04:09 AM, Kuninori Morimoto wrote:
>>> Hi Valentine
>>
>> Hi Morimoto-san,
>>
>>> (snip)
>>>
>>>> +static int usbhs_hardware_init(struct platform_device *pdev)
>>>> +{
>>>> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
>>>> +	struct clk *clk;
>>>> +
>>>> +	clk = clk_get(NULL, "hsusb");
>>>> +	if (IS_ERR(clk))
>>>> +		return -ENODEV;
>>>
>>> It is automatically enable/disabled by driver
>>> if MSTP704 clock name was "renesas_usbhs".
>>
>> The reason I did not bind usbhs clock to renesas_usbhs device is because
>> the same clock is also used by the lager_add_usb_devices() function in
>> the next patches. We need that since the global USB settings that affect
>> USBHS/USBSS and PCI USB host channel sharing are done in the USBHS
>> UGCTRL2 register. So we need this clock even if the renesas_usbhs driver
>> is disabled. IIUC, biding it to "renesas_usbhs" device would make it
>> impossible to use the clock if renesas_usbhs device is not registered.
>
> Can't the code from patch 6/6 that needs to enable the clock be moved to a
> proper device driver ? In that case you could just add an entry for that
> device in the lookups array in arch/arm/mach-shmobile/clock-r8a7790.c.
>

Which device driver are you suggesting?
The register that configures USBHS/USBSS/PCI USB channel sharing is 
located in the USBHS block. However, channel configuration should be 
done if any of the USB drivers (renesas_usbhs/pci ehci/pci ohci/xhci) is
enabled.
(the XHCI is not yet implemented, but we should still keep it in mind.)

So the code can not be placed in the renesas_usbhs driver, for example, 
because in that case the channels won't be configured properly for PCI 
USB if renesas_usbhs driver is disabled. For the same reason it cannot 
be placed in any other driver.

Hope this makes sense.

Thanks,
Val.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/02/2013 04:18 AM, Magnus Damm wrote:
> Hi Valentine,
>
> Thanks for your patches.
>
> On Wed, Oct 2, 2013 at 3:30 AM, Valentine Barshak
> <valentine.barshak@cogentembedded.com> wrote:
>> This adds USBHS phy control callbacks to the Lager board and
>> registers USBHS device if the driver is enabled.
>>
>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> ---
>>   arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
>>   1 file changed, 142 insertions(+)
>>
>> diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
>> index a8d3ce6..e408fc7 100644
>> --- a/arch/arm/mach-shmobile/board-lager.c
>> +++ b/arch/arm/mach-shmobile/board-lager.c
>
> Starting from here...
>
>> +/* USBHS registers */
>> +#define USBHS_LPSTS_REG                        0x102
>> +#define USBHS_LPSTS_SUSPM              (1 << 14)
>> +
>> +#define USBHS_UGCTRL_REG               0x180
>> +#define USBHS_UGCTRL_CONNECT           (1 << 2)
>> +#define USBHS_UGCTRL_PLLRESET          (1 << 0)
>> +
>> +#define USBHS_UGCTRL2_REG              0x184
>> +#define USBHS_UGCTRL2_USB0_PCI         (1 << 4)
>> +#define USBHS_UGCTRL2_USB0_HS          (3 << 4)
>> +#define USBHS_UGCTRL2_USB2_PCI         (0 << 31)
>> +#define USBHS_UGCTRL2_USB2_SS          (1 << 31)
>> +
>> +#define USBHS_UGSTS_REG                        0x190
>> +#define USBHS_UGSTS_LOCK               (3 << 0)
>> +
>> +struct usbhs_private {
>> +       struct renesas_usbhs_platform_info info;
>> +       struct platform_device *pdev;
>> +       struct clk *clk;
>> +};
>> +
>> +#define usbhs_get_priv(pdev) \
>> +       container_of(renesas_usbhs_get_info(pdev), struct usbhs_private, info)
>> +
>> +static int usbhs_power_on(void __iomem *base)
>> +{
>> +       u32 val;
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +
>> +       val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
>> +       iowrite16(val, base + USBHS_LPSTS_REG);
>> +
>> +       /*
>> +        * The manual suggests to check PLL lock status in the UGSTS
>> +        * register before enabling connect, however, it is always 0.
>> +        */
>> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +       return 0;
>> +}
>> +
>> +static int usbhs_power_off(void __iomem *base)
>> +{
>> +       u32 val;
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +
>> +       val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
>> +       iowrite16(val, base + USBHS_LPSTS_REG);
>> +
>> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
>> +       iowrite32(val, base + USBHS_UGCTRL_REG);
>> +       return 0;
>> +}
>> +
>> +static int usbhs_power_ctrl(struct platform_device *pdev,
>> +                               void __iomem *base, int enable)
>> +{
>> +       if (enable)
>> +               return usbhs_power_on(base);
>> +
>> +       return usbhs_power_off(base);
>> +}
>
> ... to here it looks like this is USBHS support code for the r8a7790
> SoC, not the lager board.
>
> Have you considered putting these SoC specific bits into drivers/usb/...
>

I'll try to add device id table and move it to renesas_usbhs driver.
Hope the "renesas_usbhs_rcar_h2" device name is not too long.

> Thanks,
>
> / magnus
>

Thanks,
Val.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Simon Horman]

On Wed, Oct 02, 2013 at 04:13:43PM +0400, Valentine wrote:
> On 10/02/2013 04:18 AM, Magnus Damm wrote:
> >Hi Valentine,
> 
> Hi Magnus,
> 
> >
> >Thanks for your patches.
> >
> >On Wed, Oct 2, 2013 at 3:30 AM, Valentine Barshak
> ><valentine.barshak@cogentembedded.com> wrote:
> >>This adds USBHS phy control callbacks to the Lager board and
> >>registers USBHS device if the driver is enabled.
> >>
> >>Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
> >>---
> >>  arch/arm/mach-shmobile/board-lager.c | 142 +++++++++++++++++++++++++++++++++++
> >>  1 file changed, 142 insertions(+)
> >>
> >>diff --git a/arch/arm/mach-shmobile/board-lager.c b/arch/arm/mach-shmobile/board-lager.c
> >>index a8d3ce6..e408fc7 100644
> >>--- a/arch/arm/mach-shmobile/board-lager.c
> >>+++ b/arch/arm/mach-shmobile/board-lager.c
> >
> >Starting from here...
> >
> >>+/* USBHS registers */
> >>+#define USBHS_LPSTS_REG                        0x102
> >>+#define USBHS_LPSTS_SUSPM              (1 << 14)
> >>+
> >>+#define USBHS_UGCTRL_REG               0x180
> >>+#define USBHS_UGCTRL_CONNECT           (1 << 2)
> >>+#define USBHS_UGCTRL_PLLRESET          (1 << 0)
> >>+
> >>+#define USBHS_UGCTRL2_REG              0x184
> >>+#define USBHS_UGCTRL2_USB0_PCI         (1 << 4)
> >>+#define USBHS_UGCTRL2_USB0_HS          (3 << 4)
> >>+#define USBHS_UGCTRL2_USB2_PCI         (0 << 31)
> >>+#define USBHS_UGCTRL2_USB2_SS          (1 << 31)
> >>+
> >>+#define USBHS_UGSTS_REG                        0x190
> >>+#define USBHS_UGSTS_LOCK               (3 << 0)
> >>+
> >>+struct usbhs_private {
> >>+       struct renesas_usbhs_platform_info info;
> >>+       struct platform_device *pdev;
> >>+       struct clk *clk;
> >>+};
> >>+
> >>+#define usbhs_get_priv(pdev) \
> >>+       container_of(renesas_usbhs_get_info(pdev), struct usbhs_private, info)
> >>+
> >>+static int usbhs_power_on(void __iomem *base)
> >>+{
> >>+       u32 val;
> >>+
> >>+       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
> >>+       iowrite32(val, base + USBHS_UGCTRL_REG);
> >>+
> >>+       val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
> >>+       iowrite16(val, base + USBHS_LPSTS_REG);
> >>+
> >>+       /*
> >>+        * The manual suggests to check PLL lock status in the UGSTS
> >>+        * register before enabling connect, however, it is always 0.
> >>+        */
> >>+       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
> >>+       iowrite32(val, base + USBHS_UGCTRL_REG);
> >>+       return 0;
> >>+}
> >>+
> >>+static int usbhs_power_off(void __iomem *base)
> >>+{
> >>+       u32 val;
> >>+
> >>+       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
> >>+       iowrite32(val, base + USBHS_UGCTRL_REG);
> >>+
> >>+       val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
> >>+       iowrite16(val, base + USBHS_LPSTS_REG);
> >>+
> >>+       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
> >>+       iowrite32(val, base + USBHS_UGCTRL_REG);
> >>+       return 0;
> >>+}
> >>+
> >>+static int usbhs_power_ctrl(struct platform_device *pdev,
> >>+                               void __iomem *base, int enable)
> >>+{
> >>+       if (enable)
> >>+               return usbhs_power_on(base);
> >>+
> >>+       return usbhs_power_off(base);
> >>+}
> >
> >... to here it looks like this is USBHS support code for the r8a7790
> >SoC, not the lager board.
> >
> >Have you considered putting these SoC specific bits into drivers/usb/...
> 
> I have. But I've decided to put all the USBHS-specific stuff into
> board-specific code because I'm not sure why the PLL lock is always
> 0 on Lager (whether it's a SoC or board specific issue) and I don't
> have other boards with the same SoC to test. Even though the PLL
> lock status doesn't seem to behave as described in the docs, I
> haven't noticed any
> issues with the USBHS.
> 
> I can move it to SoC files, but I'm once again not sure whether it
> should go into setup-r8a7790 or setup-rcar-gen2 because both SoCs
> seem to have identical USB subsystem.

Hi,

I may be missing the point but I think that one of the motivations
for putting the code into drivers rather than SoC or board code is
to make things easier to move over to initialisation using DT.

As we move over to DT we want to minimise if not entirely remove
board and SoC code. And I believe that putting as much as possible
in drivers is a key to achieving that.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Kuninori Morimoto]

Hi Valentine

> >> +static int usbhs_hardware_init(struct platform_device *pdev)
> >> +{
> >> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
> >> +	struct clk *clk;
> >> +
> >> +	clk = clk_get(NULL, "hsusb");
> >> +	if (IS_ERR(clk))
> >> +		return -ENODEV;
> >
> > It is automatically enable/disabled by driver
> > if MSTP704 clock name was "renesas_usbhs".
> 
> The reason I did not bind usbhs clock to renesas_usbhs device is because
> the same clock is also used by the lager_add_usb_devices() function in 
> the next patches. We need that since the global USB settings that affect 
> USBHS/USBSS and PCI USB host channel sharing are done in the USBHS 
> UGCTRL2 register. So we need this clock even if the renesas_usbhs driver 
> is disabled. IIUC, biding it to "renesas_usbhs" device would make it 
> impossible to use the clock if renesas_usbhs device is not registered.

I understand your situation, but you can use renesas_usbhs binded clock anyway ?
As Laurent mentioned, pci-rcar-gen2 care about it in driver ?
Maybe callback function is nice solution in this situation ?
Like this

--- clock --------------
  CLKDEV_DEV_ID("renesas_usbhs", &mstp_clks[MSTP704]),
  CLKDEV_ICK_ID("hsusb", "pci-rcar-gen2", &mstp_clks[MSTP704]),

--- platform -----------
int rcar_pci_hw_init(struct platform_device *pdev)
{
        clk = clk_get("hsusb", pdev->dev);
        if (IS_ERR(clk))
                 return -EIO;

        clk_enable(clk);
        clk_put(clk);

        ...

        return 0;
}

--- driver ---
static int __init rcar_pci_probe(struct platform_device *pdev)
{
        ...
        if (info->hw_init)
             ret = info->hw_init(pdev);
        ...
}

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Kuninori Morimoto]

Hi Magnus, Valentine

> >> +static int usbhs_power_on(void __iomem *base)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_PLLRESET;
> >> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> >> +
> >> +       val = ioread16(base + USBHS_LPSTS_REG) | USBHS_LPSTS_SUSPM;
> >> +       iowrite16(val, base + USBHS_LPSTS_REG);
> >> +
> >> +       /*
> >> +        * The manual suggests to check PLL lock status in the UGSTS
> >> +        * register before enabling connect, however, it is always 0.
> >> +        */
> >> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_CONNECT;
> >> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> >> +       return 0;
> >> +}
> >> +
> >> +static int usbhs_power_off(void __iomem *base)
> >> +{
> >> +       u32 val;
> >> +
> >> +       val = ioread32(base + USBHS_UGCTRL_REG) & ~USBHS_UGCTRL_CONNECT;
> >> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> >> +
> >> +       val = ioread16(base + USBHS_LPSTS_REG) & ~USBHS_LPSTS_SUSPM;
> >> +       iowrite16(val, base + USBHS_LPSTS_REG);
> >> +
> >> +       val = ioread32(base + USBHS_UGCTRL_REG) | USBHS_UGCTRL_PLLRESET;
> >> +       iowrite32(val, base + USBHS_UGCTRL_REG);
> >> +       return 0;
> >> +}
> >> +
> >> +static int usbhs_power_ctrl(struct platform_device *pdev,
> >> +                               void __iomem *base, int enable)
> >> +{
> >> +       if (enable)
> >> +               return usbhs_power_on(base);
> >> +
> >> +       return usbhs_power_off(base);
> >> +}
> >
> > ... to here it looks like this is USBHS support code for the r8a7790
> > SoC, not the lager board.
> >
> > Have you considered putting these SoC specific bits into drivers/usb/...
> >
> 
> I'll try to add device id table and move it to renesas_usbhs driver.
> Hope the "renesas_usbhs_rcar_h2" device name is not too long.

Unfortunately, I can't agree this idea.
Renesas USB sometimes has this kind of "SoC specific special initialization",
not only r8a7790, but other SoC have it too, and, these doesn't have compatibility.
Some SoC need special GPIO setting, some SoC need special clock setting etc...

This is the main reason why renesas_usbhs driver has callback functions.

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Magnus Damm]

Hi Valentine,

On Thu, Oct 3, 2013 at 7:52 AM, Valentine
<valentine.barshak@cogentembedded.com> wrote:
> On 10/02/2013 04:18 AM, Magnus Damm wrote:
>> Have you considered putting these SoC specific bits into drivers/usb/...
>
> I'll try to add device id table and move it to renesas_usbhs driver.

Sounds good, thank you!

> Hope the "renesas_usbhs_rcar_h2" device name is not too long.

Please use SoC model string "r8a7790" for DT like other drivers, as an
example see the following commit:

commit df1d0584b2292df5b9d576d7e5246e94616220a1
Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Date:   Thu Aug 29 17:14:49 2013 +0200

    ARM: shmobile: update SDHI DT compatibility string to the
<unit>-<soc> format

Cheers,

/ magnus

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/03/2013 01:01 PM, Magnus Damm wrote:
> Hi Valentine,
>
> On Thu, Oct 3, 2013 at 7:52 AM, Valentine
> <valentine.barshak@cogentembedded.com> wrote:
>> On 10/02/2013 04:18 AM, Magnus Damm wrote:
>>> Have you considered putting these SoC specific bits into drivers/usb/...
>>
>> I'll try to add device id table and move it to renesas_usbhs driver.
>
> Sounds good, thank you!

Looks like Morimoto-san doesn't agree with that.

>
>> Hope the "renesas_usbhs_rcar_h2" device name is not too long.
>
> Please use SoC model string "r8a7790" for DT like other drivers, as an
> example see the following commit:

OK, but I wasn't going to make renesas_usbhs a DT driver at least for now.
IIUC, it's going to involve a rework of USBHS bindings for other boards.
which have different platform code for PHY setup.
I thought this had to be a separate work.

Do you want me to make it DT-compatible only for r8a7790?

I planned to add a platform device id table for now. Something like
"usbhs-rcar_h2" for r8a7790 and "renesas_usbhs" for other SoCs.

>
> commit df1d0584b2292df5b9d576d7e5246e94616220a1
> Author: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Date:   Thu Aug 29 17:14:49 2013 +0200
>
>      ARM: shmobile: update SDHI DT compatibility string to the
> <unit>-<soc> format
>
> Cheers,
>
> / magnus
>

Thanks,
Val.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Laurent Pinchart]

Hi Morimoto-san,

On Wednesday 02 October 2013 21:53:26 Kuninori Morimoto wrote:
> Hi Valentine
> 
> > >> +static int usbhs_hardware_init(struct platform_device *pdev)
> > >> +{
> > >> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
> > >> +	struct clk *clk;
> > >> +
> > >> +	clk = clk_get(NULL, "hsusb");
> > >> +	if (IS_ERR(clk))
> > >> +		return -ENODEV;
> > > 
> > > It is automatically enable/disabled by driver
> > > if MSTP704 clock name was "renesas_usbhs".
> > 
> > The reason I did not bind usbhs clock to renesas_usbhs device is because
> > the same clock is also used by the lager_add_usb_devices() function in
> > the next patches. We need that since the global USB settings that affect
> > USBHS/USBSS and PCI USB host channel sharing are done in the USBHS
> > UGCTRL2 register. So we need this clock even if the renesas_usbhs driver
> > is disabled. IIUC, biding it to "renesas_usbhs" device would make it
> > impossible to use the clock if renesas_usbhs device is not registered.
> 
> I understand your situation, but you can use renesas_usbhs binded clock
> anyway ? As Laurent mentioned, pci-rcar-gen2 care about it in driver ?
> Maybe callback function is nice solution in this situation ?
> Like this
> 
> --- clock --------------
>   CLKDEV_DEV_ID("renesas_usbhs", &mstp_clks[MSTP704]),
>   CLKDEV_ICK_ID("hsusb", "pci-rcar-gen2", &mstp_clks[MSTP704]),
> 
> --- platform -----------
> int rcar_pci_hw_init(struct platform_device *pdev)
> {
>         clk = clk_get("hsusb", pdev->dev);
>         if (IS_ERR(clk))
>                  return -EIO;
> 
>         clk_enable(clk);
>         clk_put(clk);
> 
>         ...
> 
>         return 0;
> }
> 
> --- driver ---
> static int __init rcar_pci_probe(struct platform_device *pdev)
> {
>         ...
>         if (info->hw_init)
>              ret = info->hw_init(pdev);
>         ...
> }

The problem with such a mechanism is that it only works with platform data, as 
we don't have callback functions in DT.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/03/2013 01:47 PM, Laurent Pinchart wrote:
> Hi Morimoto-san,
>
> On Wednesday 02 October 2013 21:53:26 Kuninori Morimoto wrote:
>> Hi Valentine
>>
>>>>> +static int usbhs_hardware_init(struct platform_device *pdev)
>>>>> +{
>>>>> +	struct usbhs_private *priv = usbhs_get_priv(pdev);
>>>>> +	struct clk *clk;
>>>>> +
>>>>> +	clk = clk_get(NULL, "hsusb");
>>>>> +	if (IS_ERR(clk))
>>>>> +		return -ENODEV;
>>>>
>>>> It is automatically enable/disabled by driver
>>>> if MSTP704 clock name was "renesas_usbhs".
>>>
>>> The reason I did not bind usbhs clock to renesas_usbhs device is because
>>> the same clock is also used by the lager_add_usb_devices() function in
>>> the next patches. We need that since the global USB settings that affect
>>> USBHS/USBSS and PCI USB host channel sharing are done in the USBHS
>>> UGCTRL2 register. So we need this clock even if the renesas_usbhs driver
>>> is disabled. IIUC, biding it to "renesas_usbhs" device would make it
>>> impossible to use the clock if renesas_usbhs device is not registered.
>>
>> I understand your situation, but you can use renesas_usbhs binded clock
>> anyway ? As Laurent mentioned, pci-rcar-gen2 care about it in driver ?
>> Maybe callback function is nice solution in this situation ?
>> Like this
>>
>> --- clock --------------
>>    CLKDEV_DEV_ID("renesas_usbhs", &mstp_clks[MSTP704]),
>>    CLKDEV_ICK_ID("hsusb", "pci-rcar-gen2", &mstp_clks[MSTP704]),
>>
>> --- platform -----------
>> int rcar_pci_hw_init(struct platform_device *pdev)
>> {
>>          clk = clk_get("hsusb", pdev->dev);
>>          if (IS_ERR(clk))
>>                   return -EIO;
>>
>>          clk_enable(clk);
>>          clk_put(clk);
>>
>>          ...
>>
>>          return 0;
>> }
>>
>> --- driver ---
>> static int __init rcar_pci_probe(struct platform_device *pdev)
>> {
>>          ...
>>          if (info->hw_init)
>>               ret = info->hw_init(pdev);
>>          ...
>> }
>
> The problem with such a mechanism is that it only works with platform data, as
> we don't have callback functions in DT.
>

I wouldn't want to put channel fix-up in the PCI driver.
In this case we would need to put it in the XHCI driver later as well.
We could skip channel configuration in the USBHS driver and assume the 
default POR configuration (USBHS enabled).
But in case of PCI/XHCI we must ensure that there's no race conditions
and that both drivers won't attempt to access UGCTRL2 register 
simultaneously. Also we should pass the same UGGTL2 configuration value
to all the drivers.

Having this stuff done by each USB driver may cause the need of custom 
DT bindings. For example, we may need to add a USBHS reference
(phandle) to the PCI and XHCI device nodes.
In addition, both drivers would need to do the same thing: enable USBHS 
and modify UGCTRL2.

I think the channel configuration should be done outside any driver.
Having some sort of a channel fix-up in the platform code seems a bit 
cleaner. Probably a platform bus notifier callback, which enables USBHS 
only when any of the renesas_usbhs, pci-rcar-gen2 or XHCI devices is 
added, and configures the channels once and for all.
When all the devices are removed, the notify callback may disable
the USBHS clock.

Thanks,
Val.

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Kuninori Morimoto]

Hi Laurent

Thank you for your response

> The problem with such a mechanism is that it only works with platform data, as 
> we don't have callback functions in DT.

Yes, I agree.

But in renesas_usbh case, almost all Renesas USBHS chip needs
its own special initializing method.
And somechip depends on SoC, somechip depends on platform.

I'm not sure formal/correct DT solution in this case,
but, I thought we can add data table on of_platform_populate().

# BTW, I need to know how to solve FPGA special settings on DT...

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/6] arm: shmobile: lager: Add USBHS support

[Valentine]

On 10/04/2013 04:21 AM, Kuninori Morimoto wrote:
>
> Hi Laurent
>
> Thank you for your response
>
>> The problem with such a mechanism is that it only works with platform data, as
>> we don't have callback functions in DT.
>
> Yes, I agree.
>
> But in renesas_usbh case, almost all Renesas USBHS chip needs
> its own special initializing method.
> And somechip depends on SoC, somechip depends on platform.
>
> I'm not sure formal/correct DT solution in this case,
> but, I thought we can add data table on of_platform_populate().
>
> # BTW, I need to know how to solve FPGA special settings on DT...
>
> Best regards
> ---
> Kuninori Morimoto
>

I think we need a USB phy driver here.
The one that should be acquired by both renesas_usbhs and PCI USB.
I'm currently experimenting with it.

I believe that all the phy handling should be moved to the phy drivers 
eventually when we switch to using DT,

Thanks,
Val.