| Message ID | 1380652251-8143-3-git-send-email-valentine.barshak@cogentembedded.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) ðer_pdata, sizeof(ether_pdata)); lager_add_du_device(); + lager_register_usbhs(); } /*
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(+)