Skip to contentSkip to conversation

usb: host: xhci-plat: add support for the R-Car H2 and M2 xHCI controllers

PATCH In review Yoshihiro Shimoda 8 participants 13 messages May 19, 2014 – May 21, 2014 3 messages not in the archive

Subsystems (4)
USB SuperH Arm/RISC-V/Renesas architecture USB xHCI driver
lore
Share & tools
lore

Yoshihiro Shimoda

author
#0 lore raw

PATCH 2/3 usb: host: xhci-plat: add support for the R-Car H2 and M2 xHCI controllers

The R-Car H2 and M2 SoCs come with an xHCI controller that requires some specific initilization related to the firmware downloading and some specific registers. This patch adds the support for this special configuration as an xHCI quirk executed during probe and start.

Signed-off-by Yoshihiro Shimoda <[email protected]>
---
 drivers/usb/host/Kconfig     |    8 ++
 drivers/usb/host/Makefile    |    3 +
 drivers/usb/host/xhci-plat.c |   18 +++++
 drivers/usb/host/xhci-rcar.c |  182 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci-rcar.h |   28 +++++++
 5 files changed, 239 insertions(+)
 create mode 100644 drivers/usb/host/xhci-rcar.c
 create mode 100644 drivers/usb/host/xhci-rcar.h
drivers/usb/host/Kconfig +8 -0
index 9247ad2..229e968 100644
@@ -37,6 +37,14 @@ config USB_XHCI_MVEBU
 	  Say 'Y' to enable the support for the xHCI host controller
 	  found in Marvell Armada 375/38x ARM SOCs.
 
+config USB_XHCI_RCAR
+	tristate "xHCI support for Renesas R-Car SoCs"
+	select USB_XHCI_PLATFORM
+	depends on ARCH_SHMOBILE || COMPILE_TEST
+	---help---
+	  Say 'Y' to enable the support for the xHCI host controller
+	  found in Renesas R-Car ARM SoCs.
+
 endif # USB_XHCI_HCD
 
 config USB_EHCI_HCD
drivers/usb/host/Makefile +3 -0
index 7c0886a..b59ca3c 100644
@@ -22,6 +22,9 @@ ifneq ($(CONFIG_USB_XHCI_PLATFORM), )
 ifneq ($(CONFIG_USB_XHCI_MVEBU), )
 	xhci-hcd-y		+= xhci-mvebu.o
 endif
+ifneq ($(CONFIG_USB_XHCI_RCAR), )
+	xhci-hcd-y		+= xhci-rcar.o
+endif
 endif
 
 obj-$(CONFIG_USB_WHCI_HCD)	+= whci/
drivers/usb/host/xhci-plat.c +18 -0
index 561d07e..3a2da1f 100644
@@ -20,6 +20,7 @@
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+	struct device_node *of_node = hcd->self.controller->of_node;
+
+	if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+		xhci_rcar_start(hcd);
+
 	return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto unmap_registers;
 	}
 
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7791-xhci")) {
+		ret = xhci_rcar_init_quirk(pdev);
+		if (ret)
+			goto disable_clk;
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_clk;

@@ -270,6 +286,8 @@ static const struct of_device_id usb_xhci_of_match[] = {
 	{ .compatible = "xhci-platform" },
 	{ .compatible = "marvell,armada-375-xhci"},
 	{ .compatible = "marvell,armada-380-xhci"},
+	{ .compatible = "renesas,r8a7790-xhci"},
+	{ .compatible = "renesas,r8a7791-xhci"},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
drivers/usb/host/xhci-rcar.c new file +182 -0

182 lines not on this page · raw message

drivers/usb/host/xhci-rcar.h new file +28 -0
new file mode 100644
index 0000000..40f9b20
@@ -0,0 +1,28 @@
+/*
+ * drivers/usb/host/xhci-rcar.h
+ *
+ * Copyright (C) 2014 Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#ifndef _XHCI_RCAR_H
+#define _XHCI_RCAR_H
+
+#if IS_ENABLED(CONFIG_USB_XHCI_RCAR)
+int xhci_rcar_start(struct usb_hcd *hcd);
+int xhci_rcar_init_quirk(struct platform_device *pdev);
+#else
+static inline int xhci_rcar_start(struct usb_hcd *hcd)
+{
+	return 0;
+}
+
+static inline int xhci_rcar_init_quirk(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+#endif /* _XHCI_RCAR_H */
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): [email protected]

#2 lore raw

Hi Shimoda-san,

Thanks for your patches, I did however find one typo below:

Quoted 62 lines · Yoshihiro Shimoda

The R-Car H2 and M2 SoCs come with an xHCI controller that requires some specific initilization related to the firmware downloading and some specific registers. This patch adds the support for this special configuration as an xHCI quirk executed during probe and start.

Signed-off-by Yoshihiro Shimoda <[email protected]>
---
 drivers/usb/host/Kconfig     |    8 ++
 drivers/usb/host/Makefile    |    3 +
 drivers/usb/host/xhci-plat.c |   18 +++++
 drivers/usb/host/xhci-rcar.c |  182 ++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci-rcar.h |   28 +++++++
 5 files changed, 239 insertions(+)
 create mode 100644 drivers/usb/host/xhci-rcar.c
 create mode 100644 drivers/usb/host/xhci-rcar.h
drivers/usb/host/Kconfig +8 -0
index 9247ad2..229e968 100644
@@ -37,6 +37,14 @@ config USB_XHCI_MVEBU
          Say 'Y' to enable the support for the xHCI host controller
          found in Marvell Armada 375/38x ARM SOCs.
 
+config USB_XHCI_RCAR
+       tristate "xHCI support for Renesas R-Car SoCs"
+       select USB_XHCI_PLATFORM
+       depends on ARCH_SHMOBILE || COMPILE_TEST
+       ---help---
+         Say 'Y' to enable the support for the xHCI host controller
+         found in Renesas R-Car ARM SoCs.
+
 endif # USB_XHCI_HCD
 
 config USB_EHCI_HCD
drivers/usb/host/Makefile +3 -0
index 7c0886a..b59ca3c 100644
@@ -22,6 +22,9 @@ ifneq ($(CONFIG_USB_XHCI_PLATFORM), )
 ifneq ($(CONFIG_USB_XHCI_MVEBU), )
        xhci-hcd-y              += xhci-mvebu.o
 endif
+ifneq ($(CONFIG_USB_XHCI_RCAR), )
+       xhci-hcd-y              += xhci-rcar.o
+endif
 endif
 
 obj-$(CONFIG_USB_WHCI_HCD)     += whci/
drivers/usb/host/xhci-plat.c +1 -0
index 561d07e..3a2da1f 100644
@@ -20,6 +20,7 @@
 
 #include "xhci.h"
 #include "xhci-mvebu.h"
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 48

Quoted 9 lines
drivers/usb/host/xhci-plat.c +6 -0
@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+       struct device_node *of_node = hcd->self.controller->of_node;
+
+       if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+               xhci_rcar_start(hcd);
+

This is most likely a typo - I believe this is supposed to be r8a7790 and r8a7791?

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

Hi Magnus-san,

Quoted 6 lines · Magnus Damm

Hi Shimoda-san,

Thanks for your patches, I did however find one typo below:

On Mon, May 19, 2014 at 7:08 PM, Yoshihiro Shimoda <[email protected]> wrote:

< snip >

Quoted 11 lines

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 48

Quoted 8 lines

static int xhci_plat_start(struct usb_hcd *hcd)
{

drivers/usb/host/xhci-plat.c +6 -0
+       struct device_node *of_node = hcd->self.controller->of_node;
+
+       if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+               xhci_rcar_start(hcd);
+

This is most likely a typo - I believe this is supposed to be r8a7790 and r8a7791?

Thank you for the review! Yes, this is a typo... I will correct this.

Best regards,
Yoshihiro Shimoda

Quoted 4 lines

Cheers,

/ magnus

Signature · 6 lines
Yoshihiro Shimoda
EC No.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#4 lore raw
In reply to Yoshihiro Shimoda

Hi Shimoda-san,

Quoted 4 lines · Yoshihiro Shimoda

The R-Car H2 and M2 SoCs come with an xHCI controller that requires some specific initilization related to the firmware downloading and some specific registers. This patch adds the support for this special configuration as an xHCI quirk executed during probe and start.

Thanks for your patch!

Quoted 5 lines
drivers/usb/host/Kconfig +0 -0
index 9247ad2..229e968 100644

drivers/usb/host/Kconfig config USB_XHCI_MVEBU · line 46

Quoted 10 lines
drivers/usb/host/Kconfig +7 -0
@@ -37,6 +37,14 @@ config USB_XHCI_MVEBU
          Say 'Y' to enable the support for the xHCI host controller
          found in Marvell Armada 375/38x ARM SOCs.
 
+config USB_XHCI_RCAR
+       tristate "xHCI support for Renesas R-Car SoCs"
+       select USB_XHCI_PLATFORM
+       depends on ARCH_SHMOBILE || COMPILE_TEST
+       ---help---
+         Say 'Y' to enable the support for the xHCI host controller
+         found in Renesas R-Car ARM SoCs.

Does R-Car Gen1 also have xHCI, and is it compatible? If not, you may want to call this driver USB_XHCI_RCAR2.

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 46

Quoted 13 lines
drivers/usb/host/xhci-plat.c +4 -0
index 561d07e..3a2da1f 100644
@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+       struct device_node *of_node = hcd->self.controller->of_node;
+
+       if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))

r8a7791, as Magnus already pointed out.

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 47

Quoted 1 line
drivers/usb/host/xhci-plat.c +1 -0
+               xhci_rcar_start(hcd);

If CONFIG_USB_XHCI_RCAR is not defined, xhci_rcar_start() is a dummy function, but the of_device_is_compatible() checks will still be compiled in.

Hence perhaps an #ifdef CONFIG_USB_XHCI_RCAR is warranted here, possibly combined with inclusion of a C-source file, like is done in drivers/usb/host/ohci-hcd.c? It's up to the USB maintainer to decide this, though.

drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 179

Quoted 13 lines
drivers/usb/host/xhci-plat.c +6 -0
+
        return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
                        goto unmap_registers;
        }
 
+       if (of_device_is_compatible(pdev->dev.of_node,
+                                   "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(pdev->dev.of_node,
+                                   "renesas,r8a7791-xhci")) {
+               ret = xhci_rcar_init_quirk(pdev);

Same here.

drivers/usb/host/xhci-rcar.c

Quoted 12 lines
drivers/usb/host/xhci-plat.c +4 -0
+               if (ret)
+                       goto disable_clk;
+       }
+
        ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
        if (ret)
                goto disable_clk;
drivers/usb/host/xhci-rcar.c new file +1 -0
 
+/* USB3.0 Configuraion */

Configuration

Quoted 13 lines
drivers/usb/host/xhci-rcar.c new file +13 -0
+static int xhci_rcar_download_firmware(struct device *dev, void __iomem *regs)
+{
+       const struct firmware *fw;
+       int retval, index, j, time;
+       int timeout = 10000;
+       u32 data, val, temp;
+
+       /* request R-Car USB3.0 firmware */
+       retval = request_firmware(&fw, FIRMWARE_NAME, dev);
+       if (retval)
+               return retval;
+
+       /* download R-Car USB3.0 firmware */

drivers/usb/host/xhci-rcar.c line 103

Quoted 10 lines
drivers/usb/host/xhci-rcar.c new file +10 -0
+       temp = readl(regs + RCAR_USB3_DL_CTRL);
+       temp |= RCAR_USB3_DL_CTRL_ENABLE;
+       writel(temp, regs + RCAR_USB3_DL_CTRL);
+
+       for (index = 0; index < fw->size; index += 4) {
+               for (data = 0, j = 3; j >= 0; j--) {
+                       if ((j + index) >= fw->size)
+                               continue;
+                       data |= fw->data[index + j] << (8 * j);
+               }

This is your custom get_unaligned_le32(), to avoid reading beyond the end of the buffer if its size is not a multiple of 4 bytes?
Is there some way to just use get_unaligned_le32()?

If you want to keep it, I would rewrite it as

              for (data = 0, j = 3; j >= 0; j--) {
                      if ((j + index) < fw->size)
                              data |= fw->data[index + j] << (8 * j);
              }

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torval

author
#5 lore raw

Hi Geert-san,

Quoted 4 lines · Geert Uytterhoeven

Hi Shimoda-san,

On Mon, May 19, 2014 at 12:08 PM, Yoshihiro Shimoda <[email protected]> wrote:

< snip >

Quoted 10 lines

drivers/usb/host/Kconfig config USB_XHCI_MVEBU · line 46

Quoted 7 lines
drivers/usb/host/Kconfig +7 -0
+config USB_XHCI_RCAR
+       tristate "xHCI support for Renesas R-Car SoCs"
+       select USB_XHCI_PLATFORM
+       depends on ARCH_SHMOBILE || COMPILE_TEST
+       ---help---
+         Say 'Y' to enable the support for the xHCI host controller
+         found in Renesas R-Car ARM SoCs.

Does R-Car Gen1 also have xHCI, and is it compatible? If not, you may want to call this driver USB_XHCI_RCAR2.

R-Car Gen1 doesn't have xHCI.
However, next generation of R-Car may have xHCI. (But, I don't know it is compatible.) If we call this driver "USB_XHCI_RCAR2", should we also change filename to "xhci-rcar2.[ch]"?

< snip >

Quoted 8 lines

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 46

Quoted 6 lines

static int xhci_plat_start(struct usb_hcd *hcd)
{

drivers/usb/host/xhci-plat.c +4 -0
+       struct device_node *of_node = hcd->self.controller->of_node;
+
+       if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))

r8a7791, as Magnus already pointed out.

Yes, I will correct this.

Quoted 9 lines
Quoted 1 line

+ xhci_rcar_start(hcd);

If CONFIG_USB_XHCI_RCAR is not defined, xhci_rcar_start() is a dummy function, but the of_device_is_compatible() checks will still be compiled in.

Hence perhaps an #ifdef CONFIG_USB_XHCI_RCAR is warranted here, possibly combined with inclusion of a C-source file, like is done in drivers/usb/host/ohci-hcd.c? It's up to the USB maintainer to decide this, though.

This implementation is similar with the following patch. And the patch already got "Acked-by" from Mathias Nyman of USB XHCI DRIVER's maintainer.

	http://marc.info/?l=linux-usb&m=140014933101775&w=2

< snip >

Quoted 4 lines
Quoted 4 lines
drivers/usb/host/xhci-plat.c +0 -0
@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
                        goto unmap_registers;
        }
Quoted 8 lines

drivers/usb/host/xhci-plat.c static int xhci_plat_probe(struct platform_device *pdev) · line 179

Quoted 5 lines
drivers/usb/host/xhci-plat.c +5 -0
+       if (of_device_is_compatible(pdev->dev.of_node,
+                                   "renesas,r8a7790-xhci") ||
+           of_device_is_compatible(pdev->dev.of_node,
+                                   "renesas,r8a7791-xhci")) {
+               ret = xhci_rcar_init_quirk(pdev);

Same here.

Same above.

< snip >

Quoted 6 lines

drivers/usb/host/xhci-rcar.c

Quoted 4 lines
drivers/usb/host/xhci-rcar.c new file +1 -0
 
+/* USB3.0 Configuraion */

Configuration

I ran the "aspell -c" command, and I found other 2 typos. ("Initilization" and "Porariy") So, I will correct these typos.

< snip >

Quoted 9 lines

drivers/usb/host/xhci-rcar.c line 103

Quoted 6 lines
drivers/usb/host/xhci-rcar.c new file +6 -0
+       for (index = 0; index < fw->size; index += 4) {
+               for (data = 0, j = 3; j >= 0; j--) {
+                       if ((j + index) >= fw->size)
+                               continue;
+                       data |= fw->data[index + j] << (8 * j);
+               }

This is your custom get_unaligned_le32(), to avoid reading beyond the end of the buffer if its size is not a multiple of 4 bytes?

Yes, I would like to avoid it.

Quoted 1 line

Is there some way to just use get_unaligned_le32()?

Yes, I will remove the custom get_unaligned_le32() and add the following code. Do you think that this code is good?

	int i;
	u32 data;
	u8 buf[4];
< snip >
	for (i = 0; i < fw->size; i += 4) {
		memset(buf, 0, sizeof(buf));
		memcpy(buf, &fw->data[i], min(sizeof(buf), fw->size - i));
		data = get_unaligned_le32(buf);

Best regards,
Yoshihiro Shimoda

Quoted 18 lines

If you want to keep it, I would rewrite it as

              for (data = 0, j = 3; j >= 0; j--) {
                      if ((j + index) < fw->size)
                              data |= fw->data[index + j] << (8 * j);
              }

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torval

Signature · 2 lines
Yoshihiro Shimoda
EC No.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#7 lore raw

Hi Shimoda-san,

Quoted 12 lines · Yoshihiro Shimoda
Quoted 2 lines · Geert Uytterhoeven

On Mon, May 19, 2014 at 12:08 PM, Yoshihiro Shimoda <[email protected]> wrote:

< snip >

Quoted 8 lines
Quoted 7 lines
drivers/usb/host/Kconfig +7 -0
+config USB_XHCI_RCAR
+       tristate "xHCI support for Renesas R-Car SoCs"
+       select USB_XHCI_PLATFORM
+       depends on ARCH_SHMOBILE || COMPILE_TEST
+       ---help---
+         Say 'Y' to enable the support for the xHCI host controller
+         found in Renesas R-Car ARM SoCs.
Quoted 6 lines
Quoted 2 lines

Does R-Car Gen1 also have xHCI, and is it compatible? If not, you may want to call this driver USB_XHCI_RCAR2.

R-Car Gen1 doesn't have xHCI.
However, next generation of R-Car may have xHCI. (But, I don't know it is compatible.) If we call this driver "USB_XHCI_RCAR2", should we also change filename to "xhci-rcar2.[ch]"?

Iff you change the config symbol, please also change the filename.

But given the uncertainty about future version, you can leave it like it is.

Quoted 10 lines
Quoted 9 lines
Quoted 1 line

+ xhci_rcar_start(hcd);

If CONFIG_USB_XHCI_RCAR is not defined, xhci_rcar_start() is a dummy function, but the of_device_is_compatible() checks will still be compiled in.

Hence perhaps an #ifdef CONFIG_USB_XHCI_RCAR is warranted here, possibly combined with inclusion of a C-source file, like is done in drivers/usb/host/ohci-hcd.c? It's up to the USB maintainer to decide this, though.

Quoted 4 lines

This implementation is similar with the following patch. And the patch already got "Acked-by" from Mathias Nyman of USB XHCI DRIVER's maintainer.

        http://marc.info/?l=linux-usb&m=140014933101775&w=2

Fine. It can be fixed later by the maintainer, when the driver has gained too many compatible checks ;-)

Quoted 17 lines
Quoted 9 lines

drivers/usb/host/xhci-rcar.c line 103

Quoted 6 lines
drivers/usb/host/xhci-rcar.c new file +6 -0
+       for (index = 0; index < fw->size; index += 4) {
+               for (data = 0, j = 3; j >= 0; j--) {
+                       if ((j + index) >= fw->size)
+                               continue;
+                       data |= fw->data[index + j] << (8 * j);
+               }

This is your custom get_unaligned_le32(), to avoid reading beyond the end of the buffer if its size is not a multiple of 4 bytes?

Yes, I would like to avoid it.

Quoted 1 line

Is there some way to just use get_unaligned_le32()?

Yes, I will remove the custom get_unaligned_le32() and add the following code. Do you think that this code is good?

Quoted 8 lines
        int i;
        u32 data;
        u8 buf[4];
< snip >
        for (i = 0; i < fw->size; i += 4) {
                memset(buf, 0, sizeof(buf));
                memcpy(buf, &fw->data[i], min(sizeof(buf), fw->size - i));
                data = get_unaligned_le32(buf);

I'm sorry, but IMHO this looks worse.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#8 lore raw

Hi Geert-san,

Thank you for the reply again.

Quoted 19 lines · Geert Uytterhoeven

Hi Shimoda-san,

Quoted 15 lines · Yoshihiro Shimoda
Quoted 2 lines · Geert Uytterhoeven

On Mon, May 19, 2014 at 12:08 PM, Yoshihiro Shimoda <[email protected]> wrote:

< snip >

Quoted 10 lines

drivers/usb/host/Kconfig config USB_XHCI_MVEBU · line 46

Quoted 7 lines
drivers/usb/host/Kconfig +7 -0
+config USB_XHCI_RCAR
+       tristate "xHCI support for Renesas R-Car SoCs"
+       select USB_XHCI_PLATFORM
+       depends on ARCH_SHMOBILE || COMPILE_TEST
+       ---help---
+         Say 'Y' to enable the support for the xHCI host controller
+         found in Renesas R-Car ARM SoCs.

Does R-Car Gen1 also have xHCI, and is it compatible? If not, you may want to call this driver USB_XHCI_RCAR2.

Quoted 7 lines
Quoted 3 lines

R-Car Gen1 doesn't have xHCI.
However, next generation of R-Car may have xHCI. (But, I don't know it is compatible.) If we call this driver "USB_XHCI_RCAR2", should we also change filename to "xhci-rcar2.[ch]"?

Iff you change the config symbol, please also change the filename.

But given the uncertainty about future version, you can leave it like it is.

I got it. I will leave the "USB_XHCI_RCAR".

< snip >

Quoted 14 lines
Quoted 5 lines
Quoted 1 line

Is there some way to just use get_unaligned_le32()?

Yes, I will remove the custom get_unaligned_le32() and add the following code. Do you think that this code is good?

Quoted 8 lines
        int i;
        u32 data;
        u8 buf[4];
< snip >
        for (i = 0; i < fw->size; i += 4) {
                memset(buf, 0, sizeof(buf));
                memcpy(buf, &fw->data[i], min(sizeof(buf), fw->size - i));
                data = get_unaligned_le32(buf);
Quoted 1 line

I'm sorry, but IMHO this looks worse.

Thank you for the review. :)
So, I will keep the following code with an additional comment.

              for (data = 0, j = 3; j >= 0; j--) {
                      if ((j + index) < fw->size)
                              data |= fw->data[index + j] << (8 * j);
              }

Best regards,
Yoshihiro Shimoda

Quoted 15 lines

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines
Yoshihiro Shimoda
EC No.
#9 lore raw
In reply to Yoshihiro Shimoda

Hello.

Quoted 6 lines · Yoshihiro Shimoda

The R-Car H2 and M2 SoCs come with an xHCI controller that requires some specific initilization related to the firmware downloading and some specific registers. This patch adds the support for this special configuration as an xHCI quirk executed during probe and start.

Signed-off-by Yoshihiro Shimoda <[email protected]>

[...]

Quoted 13 lines
drivers/usb/host/xhci-plat.c +1 -0
index 561d07e..3a2da1f 100644
@@ -20,6 +20,7 @@
 
  #include "xhci.h"
  #include "xhci-mvebu.h"
+#include "xhci-rcar.h"
 
  static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
  {

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 46

Quoted 7 lines
drivers/usb/host/xhci-plat.c +4 -0
@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
  static int xhci_plat_start(struct usb_hcd *hcd)
  {
+	struct device_node *of_node = hcd->self.controller->of_node;
+
+	if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))

Perhaps "renesas,r8a7791-xhci"?

Quoted 5 lines
drivers/usb/host/xhci-plat.c +2 -0
+		xhci_rcar_start(hcd);
+
  	return xhci_run(hcd);
  }

[...]

Quoted 38 lines
drivers/usb/host/xhci-rcar.c new file +32 -0
new file mode 100644
index 0000000..3c0bfae
@@ -0,0 +1,182 @@
+/*
+ * xHCI host controller driver for R-Car SoCs
+ *
+ * Copyright (C) 2014 Renesas Electronics Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ */
+
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/usb/phy.h>
+
+#include "xhci.h"
+#include "xhci-rcar.h"
+
+#define FIRMWARE_NAME		"r8a779x_usb3_v1.dlmem"
+
+/*** Register Offset ***/
+#define RCAR_USB3_INT_ENA	0x224	/* Interrupt Enable */
+#define RCAR_USB3_DL_CTRL	0x250	/* FW Download Control & Status */
+#define RCAR_USB3_FW_DATA0	0x258	/* FW Data0 */
+
+#define RCAR_USB3_LCLK		0xa44	/* LCLK Select */
+#define RCAR_USB3_CONF1		0xa48	/* USB3.0 Configuration1 */
+#define RCAR_USB3_CONF2		0xa5c	/* USB3.0 Configuration2 */
+#define RCAR_USB3_CONF3		0xaa8	/* USB3.0 Configuration3 */
+#define RCAR_USB3_RX_POL	0xab0	/* USB3.0 RX Polarity */
+#define RCAR_USB3_TX_POL	0xab8	/* USB3.0 TX Polarity */
+

drivers/usb/host/xhci-rcar.c line 42

Quoted 10 lines
drivers/usb/host/xhci-rcar.c new file +10 -0
+/*** Register Settings ***/
+/* Interrupt Enable */
+#define RCAR_USB3_INT_XHC_ENA	0x00000001
+#define RCAR_USB3_INT_PME_ENA	0x00000002
+#define RCAR_USB3_INT_HSE_ENA	0x00000004
+#define RCAR_USB3_INT_ENA_VAL	(RCAR_USB3_INT_XHC_ENA | \
+				RCAR_USB3_INT_PME_ENA | RCAR_USB3_INT_HSE_ENA)
+
+/* FW Download Control & Status */
+#define RCAR_USB3_DL_CTRL		0x250

Already #define'd.

drivers/usb/host/xhci-rcar.c line 50

Quoted 1 line
drivers/usb/host/xhci-rcar.c new file +1 -0
+/* USB3.0 Configuraion */

Configuration.

drivers/usb/host/xhci-rcar.c line 62

Quoted 4 lines
drivers/usb/host/xhci-rcar.c new file +4 -0
+int xhci_rcar_start(struct usb_hcd *hcd)
+{
+	if (hcd->regs != NULL) {
+		u32 temp;

Need empty line here... and should perhaps return error if hcd->regs NULL?

Quoted 7 lines
drivers/usb/host/xhci-rcar.c new file +7 -0
+		/* Interrupt Enable */
+		temp = readl(hcd->regs + RCAR_USB3_INT_ENA);
+		temp |= RCAR_USB3_INT_ENA_VAL;
+		writel(temp, hcd->regs + RCAR_USB3_INT_ENA);
+		/* LCLK Select */
+		writel(RCAR_USB3_LCLK_ENA_VAL, hcd->regs + RCAR_USB3_LCLK);
+		/* USB3.0 Configuration */

drivers/usb/host/xhci-rcar.c line 79

Quoted 10 lines
drivers/usb/host/xhci-rcar.c new file +10 -0
+		writel(RCAR_USB3_CONF1_VAL, hcd->regs + RCAR_USB3_CONF1);
+		writel(RCAR_USB3_CONF2_VAL, hcd->regs + RCAR_USB3_CONF2);
+		writel(RCAR_USB3_CONF3_VAL, hcd->regs + RCAR_USB3_CONF3);
+		/* USB3.0 Polariy */
+		writel(RCAR_USB3_RX_POL_VAL, hcd->regs + RCAR_USB3_RX_POL);
+		writel(RCAR_USB3_TX_POL_VAL, hcd->regs + RCAR_USB3_TX_POL);
+	}
+
+	return 0;
+}

[...]

WBR, Sergei

author
#10 lore raw

Hello,

Quoted 3 lines · Sergei Shtylyov

Hello.

On 19-05-2014 14:08, Yoshihiro Shimoda wrote:

< snip >

Quoted 8 lines

drivers/usb/host/xhci-plat.c static int xhci_plat_setup(struct usb_hcd *hcd) · line 46

Quoted 6 lines
  static int xhci_plat_start(struct usb_hcd *hcd)
  {
drivers/usb/host/xhci-plat.c +4 -0
+	struct device_node *of_node = hcd->self.controller->of_node;
+
+	if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))

    Perhaps "renesas,r8a7791-xhci"?

Yes. I will correct this.

< snip >

Quoted 4 lines

drivers/usb/host/xhci-rcar.c line 42

Quoted 2 lines
drivers/usb/host/xhci-rcar.c new file +2 -0
+/* FW Download Control & Status */
+#define RCAR_USB3_DL_CTRL		0x250

Already #define'd.

Thank you for the point! I will remove this.

Quoted 3 lines
Quoted 1 line

+/* USB3.0 Configuraion */

Configuration.

I will correct this.

Quoted 6 lines

drivers/usb/host/xhci-rcar.c line 62

Quoted 4 lines
drivers/usb/host/xhci-rcar.c new file +4 -0
+int xhci_rcar_start(struct usb_hcd *hcd)
+{
+	if (hcd->regs != NULL) {
+		u32 temp;

    Need empty line here... and should perhaps return error if hcd->regs NULL?

I will add empty line.
If this function returns error and xhci_plat_start() also returns error, the xhci driver was not able to work.
So, I will change the prototype of this function to "void".

Best regards,
Yoshihiro Shimoda

Quoted 21 lines
Quoted 7 lines
drivers/usb/host/xhci-rcar.c new file +7 -0
+		/* Interrupt Enable */
+		temp = readl(hcd->regs + RCAR_USB3_INT_ENA);
+		temp |= RCAR_USB3_INT_ENA_VAL;
+		writel(temp, hcd->regs + RCAR_USB3_INT_ENA);
+		/* LCLK Select */
+		writel(RCAR_USB3_LCLK_ENA_VAL, hcd->regs + RCAR_USB3_LCLK);
+		/* USB3.0 Configuration */

drivers/usb/host/xhci-rcar.c line 79

Quoted 10 lines
drivers/usb/host/xhci-rcar.c new file +10 -0
+		writel(RCAR_USB3_CONF1_VAL, hcd->regs + RCAR_USB3_CONF1);
+		writel(RCAR_USB3_CONF2_VAL, hcd->regs + RCAR_USB3_CONF2);
+		writel(RCAR_USB3_CONF3_VAL, hcd->regs + RCAR_USB3_CONF3);
+		/* USB3.0 Polariy */
+		writel(RCAR_USB3_RX_POL_VAL, hcd->regs + RCAR_USB3_RX_POL);
+		writel(RCAR_USB3_TX_POL_VAL, hcd->regs + RCAR_USB3_TX_POL);
+	}
+
+	return 0;
+}

[...]

WBR, Sergei

Signature · 2 lines
Yoshihiro Shimoda
EC No.
#11 lore raw
In reply to Yoshihiro Shimoda
Quoted 37 lines · On Monday 19 May 2014 19:08:05 Yoshihiro Shimoda wrote:

#include "xhci.h"
#include "xhci-mvebu.h"

drivers/usb/host/xhci-plat.c +16 -0
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+	struct device_node *of_node = hcd->self.controller->of_node;
+
+	if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+		xhci_rcar_start(hcd);
+
 	return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto unmap_registers;
 	}
 
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7791-xhci")) {
+		ret = xhci_rcar_init_quirk(pdev);
+		if (ret)
+			goto disable_clk;
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_clk;

drivers/usb/host/xhci-plat.c static const struct of_device_id usb_xhci_of_match[] = { · line 293

Quoted 8 lines
drivers/usb/host/xhci-plat.c +2 -0
@@ -270,6 +286,8 @@ static const struct of_device_id usb_xhci_of_match[] = {
 	{ .compatible = "xhci-platform" },
 	{ .compatible = "marvell,armada-375-xhci"},
 	{ .compatible = "marvell,armada-380-xhci"},
+	{ .compatible = "renesas,r8a7790-xhci"},
+	{ .compatible = "renesas,r8a7791-xhci"},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);

Like the drivers before, this is way more than a quirk, and deserves to be its own driver. It would be better to have an abstract way to split out soc specific xhci front-ends and export functions from the xhci-platform code.

Arnd

author
#12 lore raw

Hi,

Quoted 43 lines · Arnd Bergmann
Quoted 42 lines · On Monday 19 May 2014 19:08:05 Yoshihiro Shimoda wrote:

#include "xhci.h"
#include "xhci-mvebu.h"

drivers/usb/host/xhci-plat.c +18 -0
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+	struct device_node *of_node = hcd->self.controller->of_node;
+
+	if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+		xhci_rcar_start(hcd);
+
 	return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
 			goto unmap_registers;
 	}
 
+	if (of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7790-xhci") ||
+	    of_device_is_compatible(pdev->dev.of_node,
+				    "renesas,r8a7791-xhci")) {
+		ret = xhci_rcar_init_quirk(pdev);
+		if (ret)
+			goto disable_clk;
+	}
+
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
 	if (ret)
 		goto disable_clk;

@@ -270,6 +286,8 @@ static const struct of_device_id usb_xhci_of_match[] = {
 	{ .compatible = "xhci-platform" },
 	{ .compatible = "marvell,armada-375-xhci"},
 	{ .compatible = "marvell,armada-380-xhci"},
+	{ .compatible = "renesas,r8a7790-xhci"},
+	{ .compatible = "renesas,r8a7791-xhci"},
Quoted 8 lines

drivers/usb/host/xhci-plat.c static const struct of_device_id usb_xhci_of_match[] = { · line 293

Quoted 3 lines
drivers/usb/host/xhci-plat.c +0 -0
 	{ },
 };
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);

Like the drivers before, this is way more than a quirk, and deserves to be its own driver. It would be better to have an abstract way to split out soc specific xhci front-ends and export functions from the xhci-platform code.

Thank you for your comment. But, I couldn't understand your comment...
Did you mean that xhci-rcar.c should call of_device_is_compatible(of_node, "renesas,...")? If so, I will modify this patch.

Best regards,
Yoshihiro Shimoda

Quoted 2 lines

Arnd

Signature · 2 lines
Yoshihiro Shimoda
EC No.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#14 lore raw
Quoted 54 lines · On Wednesday 21 May 2014 16:54:00 Yoshihiro Shimoda wrote:
Quoted 43 lines · Arnd Bergmann
Quoted 42 lines · On Monday 19 May 2014 19:08:05 Yoshihiro Shimoda wrote:

#include "xhci.h"
#include "xhci-mvebu.h"

drivers/usb/host/xhci-plat.c +18 -0
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+    struct device_node *of_node = hcd->self.controller->of_node;
+
+    if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+        of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+            xhci_rcar_start(hcd);
+
     return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
                     goto unmap_registers;
     }
 
+    if (of_device_is_compatible(pdev->dev.of_node,
+                                "renesas,r8a7790-xhci") ||
+        of_device_is_compatible(pdev->dev.of_node,
+                                "renesas,r8a7791-xhci")) {
+            ret = xhci_rcar_init_quirk(pdev);
+            if (ret)
+                    goto disable_clk;
+    }
+
     ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
     if (ret)
             goto disable_clk;

@@ -270,6 +286,8 @@ static const struct of_device_id usb_xhci_of_match[] = {
     { .compatible = "xhci-platform" },
     { .compatible = "marvell,armada-375-xhci"},
     { .compatible = "marvell,armada-380-xhci"},
+    { .compatible = "renesas,r8a7790-xhci"},
+    { .compatible = "renesas,r8a7791-xhci"},
Quoted 8 lines

drivers/usb/host/xhci-plat.c static const struct of_device_id usb_xhci_of_match[] = { · line 293

Quoted 3 lines
drivers/usb/host/xhci-plat.c +0 -0
     { },
 };
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);

Like the drivers before, this is way more than a quirk, and deserves to be its own driver. It would be better to have an abstract way to split out soc specific xhci front-ends and export functions from the xhci-platform code.

Quoted 3 lines

Thank you for your comment. But, I couldn't understand your comment...
Did you mean that xhci-rcar.c should call of_device_is_compatible(of_node, "renesas,...")? If so, I will modify this patch.

What I mean is that there should be a separate module that contains all the renesas specific code, and that module should register a platform driver that contains the match table for its own IDs.

Then instead of having a common xhci_plat_probe() that gets called as the ->probe() callback of the driver, you have a rcar_xhci_probe() function that calls into common helper functions exported by the base driver, just as we do things for all other drivers. See ehci or ahci for instance.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#15 lore raw
Quoted 60 lines · Arnd Bergmann
Quoted 54 lines · On Wednesday 21 May 2014 16:54:00 Yoshihiro Shimoda wrote:
Quoted 43 lines · Arnd Bergmann
Quoted 42 lines · On Monday 19 May 2014 19:08:05 Yoshihiro Shimoda wrote:

#include "xhci.h"
#include "xhci-mvebu.h"

drivers/usb/host/xhci-plat.c +18 -0
+#include "xhci-rcar.h"
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {

@@ -39,6 +40,12 @@ static int xhci_plat_setup(struct usb_hcd *hcd)
 
 static int xhci_plat_start(struct usb_hcd *hcd)
 {
+    struct device_node *of_node = hcd->self.controller->of_node;
+
+    if (of_device_is_compatible(of_node, "renesas,r8a7790-xhci") ||
+        of_device_is_compatible(of_node, "renesas,r8a7790-xhci"))
+            xhci_rcar_start(hcd);
+
     return xhci_run(hcd);
 }
 

@@ -165,6 +172,15 @@ static int xhci_plat_probe(struct platform_device *pdev)
                     goto unmap_registers;
     }
 
+    if (of_device_is_compatible(pdev->dev.of_node,
+                                "renesas,r8a7790-xhci") ||
+        of_device_is_compatible(pdev->dev.of_node,
+                                "renesas,r8a7791-xhci")) {
+            ret = xhci_rcar_init_quirk(pdev);
+            if (ret)
+                    goto disable_clk;
+    }
+
     ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
     if (ret)
             goto disable_clk;

@@ -270,6 +286,8 @@ static const struct of_device_id usb_xhci_of_match[] = {
     { .compatible = "xhci-platform" },
     { .compatible = "marvell,armada-375-xhci"},
     { .compatible = "marvell,armada-380-xhci"},
+    { .compatible = "renesas,r8a7790-xhci"},
+    { .compatible = "renesas,r8a7791-xhci"},
Quoted 8 lines

drivers/usb/host/xhci-plat.c static const struct of_device_id usb_xhci_of_match[] = { · line 293

Quoted 3 lines
drivers/usb/host/xhci-plat.c +0 -0
     { },
 };
 MODULE_DEVICE_TABLE(of, usb_xhci_of_match);

Like the drivers before, this is way more than a quirk, and deserves to be its own driver. It would be better to have an abstract way to split out soc specific xhci front-ends and export functions from the xhci-platform code.

Quoted 3 lines

Thank you for your comment. But, I couldn't understand your comment...
Did you mean that xhci-rcar.c should call of_device_is_compatible(of_node, "renesas,...")? If so, I will modify this patch.

Quoted 8 lines

What I mean is that there should be a separate module that contains all the renesas specific code, and that module should register a platform driver that contains the match table for its own IDs.

Then instead of having a common xhci_plat_probe() that gets called as the ->probe() callback of the driver, you have a rcar_xhci_probe() function that calls into common helper functions exported by the base driver, just as we do things for all other drivers. See ehci or ahci for instance.

Thank you very much for the prompt reply!
I will see ehci and ahci drivers.

Best regards,
Yoshihiro Shimoda

Quoted 3 lines

Arnd

Signature · 2 lines
Yoshihiro Shimoda
EC No.