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

[Simon Horman <horms@verge.net.au>]

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.