RE: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
Hi Heikki, Thank you for the comments! > From: Heikki Krogerus, Sent: Wednesday, April 11, 2018 5:02 PM > > On Wed, Apr 11, 2018 at 03:15:23AM +, Yoshihiro Shimoda wrote: > > > > + host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", > > > > 0); > > > > + if (!host_node) > > > > + return -ENODEV; > > > > + > > > > + pdev_host = of_find_device_by_node(host_node); > > > > + if (!pdev_host) > > > > + return -ENODEV; > > > > + > > > > + of_node_put(host_node); > > > > > > Isn't what Heikki tried to solve with graphs and stuff like that? > > > > Heikki prepared "device connection" framework (devcon) [1] to get a device > > pointer. > > However, IIUC, we need to improve the framework for device tree environment > > because: > > - The devcon needs each dev_name() through the endpoint array of struct > > device_connection. > > - Each dev_name() on device tree environment will be > register>., > >f.e. "ee02.usb". So, I don???t think adding such strings to a driver > > is a good way. > > That is how the build-in connection descriptions are handled, but in > this case you want to describe them in your bindings, and to do that you > should use remote-endpoints, ie. OF graph bindings: bindings/graph.txt I understood it. As I mentioned other thread, I'll try to use the usb-connector bindings. > > So, I wrote such a code by using existing APIs. > > Should I improve the framework first somehow? Or, is my understanding > > incorrect? > > You don't necessarily need to propose any changes to the device > connection framework at this point, but you do need to use graph for > describing that connection. Once we add device graph handling to the > device connection framework, it is no problem to update this driver. I got it. > Of course, if you have time and interest in preparing a proposal for > graph handling to the device connection framework, I would appreciate > it. I'll try to add graph handling to the device connection framework by end of next week. If failed, I'll inform in this thread. Best regards, Yoshihiro Shimoda > > Thanks, > > -- > heikki
RE: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
Hi Heikki, > From: Heikki Krogerus, Sent: Wednesday, April 11, 2018 4:26 PM > > On Tue, Apr 10, 2018 at 09:03:46PM +0900, Yoshihiro Shimoda wrote: > > +Example of R-Car H3 ES2.0: > > + usb3_peri0: usb@ee02 { > > + compatible = "renesas,r8a7795-usb3-peri", > > +"renesas,rcar-gen3-usb3-peri"; > > + reg = <0 0xee02 0 0x400>; > > + interrupts = ; > > + clocks = < CPG_MOD 328>; > > + power-domains = < R8A7795_PD_ALWAYS_ON>; > > + resets = < 328>; > > + > > + usb3-role-sw { > > + compatible = "renesas,rcar-usb3-role-switch"; > > + renesas,host = <>; > > I pretty sure you should model that connection using OF graph > bindings. That way I believe this will also fit nicely to the > usb-connector bindings. Check bindings/connector/usb-connector.txt Oh, I didn't know the usb-connector bindings has been merged into the linux-next. Thank you for the suggestion. I will try to use it. Best regards, Yoshihiro Shimoda > > + }; > > + }; > > > Br, > > -- > heikki
Re: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
On Wed, Apr 11, 2018 at 03:15:23AM +, Yoshihiro Shimoda wrote: > > > + host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", > > > 0); > > > + if (!host_node) > > > + return -ENODEV; > > > + > > > + pdev_host = of_find_device_by_node(host_node); > > > + if (!pdev_host) > > > + return -ENODEV; > > > + > > > + of_node_put(host_node); > > > > Isn't what Heikki tried to solve with graphs and stuff like that? > > Heikki prepared "device connection" framework (devcon) [1] to get a device > pointer. > However, IIUC, we need to improve the framework for device tree environment > because: > - The devcon needs each dev_name() through the endpoint array of struct > device_connection. > - Each dev_name() on device tree environment will be register>., >f.e. "ee02.usb". So, I don???t think adding such strings to a driver > is a good way. That is how the build-in connection descriptions are handled, but in this case you want to describe them in your bindings, and to do that you should use remote-endpoints, ie. OF graph bindings: bindings/graph.txt > So, I wrote such a code by using existing APIs. > Should I improve the framework first somehow? Or, is my understanding > incorrect? You don't necessarily need to propose any changes to the device connection framework at this point, but you do need to use graph for describing that connection. Once we add device graph handling to the device connection framework, it is no problem to update this driver. Of course, if you have time and interest in preparing a proposal for graph handling to the device connection framework, I would appreciate it. Thanks, -- heikki
Re: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
On Tue, Apr 10, 2018 at 09:03:46PM +0900, Yoshihiro Shimoda wrote: > This patch adds role switch support for R-Car SoCs. Some R-Car SoCs > (e.g. R-Car H3) have USB 3.0 dual-role device controller which has > the USB 3.0 xHCI host and Renesas USB 3.0 peripheral. > > Unfortunately, the mode change register contains the USB 3.0 peripheral > controller side only. So, the USB 3.0 peripheral driver (renesas_usb3) > manages this register. However, in peripheral mode, the host should > stop. Also the host hardware needs to reinitialize its own registers > when the mode changes from peripheral to host mode. Otherwise, > the host cannot work correctly (e.g. detect a device as high-speed). > > To achieve this by a driver, this role switch driver manages > the mode change register and attach/release the xhci-plat driver. > The renesas_usb3 udc driver should call devm_of_platform_populate() > to probe this driver. > > Signed-off-by: Yoshihiro Shimoda> --- > .../bindings/usb/renesas,rcar-usb3-role-sw.txt | 23 +++ > drivers/usb/roles/Kconfig | 12 ++ > drivers/usb/roles/Makefile | 1 + > drivers/usb/roles/rcar-usb3-role-switch.c | 200 > + > 4 files changed, 236 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt > create mode 100644 drivers/usb/roles/rcar-usb3-role-switch.c > > diff --git > a/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt > b/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt > new file mode 100644 > index 000..752bc16 > --- /dev/null > +++ b/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt > @@ -0,0 +1,23 @@ > +Renesas Electronics R-Car USB 3.0 role switch driver > + > +A renesas_usb3's node can contain this node. > + > +Required properties: > + - compatible: Must contain "renesas,rcar-usb3-role-switch". > + - renesas,host: phandle of the usb3.0 host. > + > +Example of R-Car H3 ES2.0: > + usb3_peri0: usb@ee02 { > + compatible = "renesas,r8a7795-usb3-peri", > + "renesas,rcar-gen3-usb3-peri"; > + reg = <0 0xee02 0 0x400>; > + interrupts = ; > + clocks = < CPG_MOD 328>; > + power-domains = < R8A7795_PD_ALWAYS_ON>; > + resets = < 328>; > + > + usb3-role-sw { > + compatible = "renesas,rcar-usb3-role-switch"; > + renesas,host = <>; I pretty sure you should model that connection using OF graph bindings. That way I believe this will also fit nicely to the usb-connector bindings. Check bindings/connector/usb-connector.txt > + }; > + }; Br, -- heikki
RE: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
Hi Andy, Thank you for the review! > From: Andy Shevchenko, Sent: Tuesday, April 10, 2018 9:40 PM > > On Tue, Apr 10, 2018 at 3:03 PM, Yoshihiro Shimoda >wrote: > > +#include > > +#include > > +#include > > > +#include > > Do you need this one? If using of_parse_phandle() is acceptable, I need linux/of_platform.h at least. > > +#include > > +#include > > +#include > > +#include > > +#include > > > +static const struct of_device_id rcar_usb3_role_switch_of_match[] = { > > + { .compatible = "renesas,rcar-usb3-role-switch" }, > > + { }, > > Better to remove comma at terminator line. I got it. I will fix it in v2. > > +}; > > +MODULE_DEVICE_TABLE(of, rcar_usb3_role_switch_of_match); > > > +static int rcar_usb3_role_switch_probe(struct platform_device *pdev) > > +{ > > > + /* Avoid devm_request_mem_region() calling by > > devm_ioremap_resource() */ > > Redundant comment. I will remove this comment. > > + host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", 0); > > + if (!host_node) > > + return -ENODEV; > > + > > + pdev_host = of_find_device_by_node(host_node); > > + if (!pdev_host) > > + return -ENODEV; > > + > > + of_node_put(host_node); > > Isn't what Heikki tried to solve with graphs and stuff like that? Heikki prepared "device connection" framework (devcon) [1] to get a device pointer. However, IIUC, we need to improve the framework for device tree environment because: - The devcon needs each dev_name() through the endpoint array of struct device_connection. - Each dev_name() on device tree environment will be ., f.e. "ee02.usb". So, I don’t think adding such strings to a driver is a good way. So, I wrote such a code by using existing APIs. Should I improve the framework first somehow? Or, is my understanding incorrect? [1] https://patchwork.kernel.org/patch/10297041/ > > + dev_info(>dev, "probed\n"); > > Noise. (Hint: initcall_debug is a good command line option) Thank you for the hint! I will remove the dev_info(). > > +} > > > +#ifdef CONFIG_PM_SLEEP > > Instead... > > > +static int rcar_usb3_role_switch_suspend(struct device *dev) > > ...use __maybe_unused annotation. > > > +static int rcar_usb3_role_switch_resume(struct device *dev) > > Ditto. I will fix them in v2 patch. > > +#endif > > > > +static struct platform_driver rcar_usb3_role_switch_driver = { > > + .probe = rcar_usb3_role_switch_probe, > > + .remove = rcar_usb3_role_switch_remove, > > + .driver = { > > + .name = "rcar_usb3_role_switch", > > + .pm = _usb3_role_switch_pm_ops, > > > + .of_match_table = > > of_match_ptr(rcar_usb3_role_switch_of_match), > > Is it possible to have this driver w/o OF? Does it make sense in that case? > Otherwise remove of_match_ptr() call and below modalias. This driver depends on OF now. So, I will remove of_match_ptr() and MODULE_ALIAS(). Best regards, Yoshihiro Shimoda > > + }, > > +}; > > > +MODULE_ALIAS("platform:rcar_usb3_role_switch"); > > -- > With Best Regards, > Andy Shevchenko
Re: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
On Tue, Apr 10, 2018 at 3:03 PM, Yoshihiro Shimodawrote: > This patch adds role switch support for R-Car SoCs. Some R-Car SoCs > (e.g. R-Car H3) have USB 3.0 dual-role device controller which has > the USB 3.0 xHCI host and Renesas USB 3.0 peripheral. > > Unfortunately, the mode change register contains the USB 3.0 peripheral > controller side only. So, the USB 3.0 peripheral driver (renesas_usb3) > manages this register. However, in peripheral mode, the host should > stop. Also the host hardware needs to reinitialize its own registers > when the mode changes from peripheral to host mode. Otherwise, > the host cannot work correctly (e.g. detect a device as high-speed). > > To achieve this by a driver, this role switch driver manages > the mode change register and attach/release the xhci-plat driver. > The renesas_usb3 udc driver should call devm_of_platform_populate() > to probe this driver. > +#include > +#include > +#include > +#include Do you need this one? > +#include > +#include > +#include > +#include > +#include > +static const struct of_device_id rcar_usb3_role_switch_of_match[] = { > + { .compatible = "renesas,rcar-usb3-role-switch" }, > + { }, Better to remove comma at terminator line. > +}; > +MODULE_DEVICE_TABLE(of, rcar_usb3_role_switch_of_match); > +static int rcar_usb3_role_switch_probe(struct platform_device *pdev) > +{ > + /* Avoid devm_request_mem_region() calling by devm_ioremap_resource() > */ Redundant comment. > + host_node = of_parse_phandle(pdev->dev.of_node, "renesas,host", 0); > + if (!host_node) > + return -ENODEV; > + > + pdev_host = of_find_device_by_node(host_node); > + if (!pdev_host) > + return -ENODEV; > + > + of_node_put(host_node); Isn't what Heikki tried to solve with graphs and stuff like that? > + dev_info(>dev, "probed\n"); Noise. (Hint: initcall_debug is a good command line option) > +} > +#ifdef CONFIG_PM_SLEEP Instead... > +static int rcar_usb3_role_switch_suspend(struct device *dev) ...use __maybe_unused annotation. > +static int rcar_usb3_role_switch_resume(struct device *dev) Ditto. > +#endif > +static struct platform_driver rcar_usb3_role_switch_driver = { > + .probe = rcar_usb3_role_switch_probe, > + .remove = rcar_usb3_role_switch_remove, > + .driver = { > + .name = "rcar_usb3_role_switch", > + .pm = _usb3_role_switch_pm_ops, > + .of_match_table = > of_match_ptr(rcar_usb3_role_switch_of_match), Is it possible to have this driver w/o OF? Does it make sense in that case? Otherwise remove of_match_ptr() call and below modalias. > + }, > +}; > +MODULE_ALIAS("platform:rcar_usb3_role_switch"); -- With Best Regards, Andy Shevchenko
[PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs
This patch adds role switch support for R-Car SoCs. Some R-Car SoCs (e.g. R-Car H3) have USB 3.0 dual-role device controller which has the USB 3.0 xHCI host and Renesas USB 3.0 peripheral. Unfortunately, the mode change register contains the USB 3.0 peripheral controller side only. So, the USB 3.0 peripheral driver (renesas_usb3) manages this register. However, in peripheral mode, the host should stop. Also the host hardware needs to reinitialize its own registers when the mode changes from peripheral to host mode. Otherwise, the host cannot work correctly (e.g. detect a device as high-speed). To achieve this by a driver, this role switch driver manages the mode change register and attach/release the xhci-plat driver. The renesas_usb3 udc driver should call devm_of_platform_populate() to probe this driver. Signed-off-by: Yoshihiro Shimoda--- .../bindings/usb/renesas,rcar-usb3-role-sw.txt | 23 +++ drivers/usb/roles/Kconfig | 12 ++ drivers/usb/roles/Makefile | 1 + drivers/usb/roles/rcar-usb3-role-switch.c | 200 + 4 files changed, 236 insertions(+) create mode 100644 Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt create mode 100644 drivers/usb/roles/rcar-usb3-role-switch.c diff --git a/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt b/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt new file mode 100644 index 000..752bc16 --- /dev/null +++ b/Documentation/devicetree/bindings/usb/renesas,rcar-usb3-role-sw.txt @@ -0,0 +1,23 @@ +Renesas Electronics R-Car USB 3.0 role switch driver + +A renesas_usb3's node can contain this node. + +Required properties: + - compatible: Must contain "renesas,rcar-usb3-role-switch". + - renesas,host: phandle of the usb3.0 host. + +Example of R-Car H3 ES2.0: + usb3_peri0: usb@ee02 { + compatible = "renesas,r8a7795-usb3-peri", +"renesas,rcar-gen3-usb3-peri"; + reg = <0 0xee02 0 0x400>; + interrupts = ; + clocks = < CPG_MOD 328>; + power-domains = < R8A7795_PD_ALWAYS_ON>; + resets = < 328>; + + usb3-role-sw { + compatible = "renesas,rcar-usb3-role-switch"; + renesas,host = <>; + }; + }; diff --git a/drivers/usb/roles/Kconfig b/drivers/usb/roles/Kconfig index f5a5e6f..8218035 100644 --- a/drivers/usb/roles/Kconfig +++ b/drivers/usb/roles/Kconfig @@ -11,4 +11,16 @@ config USB_ROLES_INTEL_XHCI To compile the driver as a module, choose M here: the module will be called intel-xhci-usb-role-switch. +config USB_ROLES_RCAR_USB3 + tristate "Renesas R-Car USB3.0 Role Switch" + depends on ARCH_RENESAS + help + Driver for the internal USB role switch for switching the USB data + lines between the xHCI host controller and the Renesas gadget + controller (by using renesas_usb3 driver) found on various Renesas + R-Car SoCs. + + To compile the driver as a module, choose M here: the module will + be called rcar-usb3-role-switch. + endif # USB_ROLE_SWITCH diff --git a/drivers/usb/roles/Makefile b/drivers/usb/roles/Makefile index e44b179..7ce3be3 100644 --- a/drivers/usb/roles/Makefile +++ b/drivers/usb/roles/Makefile @@ -1 +1,2 @@ obj-$(CONFIG_USB_ROLES_INTEL_XHCI) += intel-xhci-usb-role-switch.o +obj-$(CONFIG_USB_ROLES_RCAR_USB3) += rcar-usb3-role-switch.o diff --git a/drivers/usb/roles/rcar-usb3-role-switch.c b/drivers/usb/roles/rcar-usb3-role-switch.c new file mode 100644 index 000..90648a1 --- /dev/null +++ b/drivers/usb/roles/rcar-usb3-role-switch.c @@ -0,0 +1,200 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Renesas R-Car USB 3.0 role switch driver + * + * Copyright (C) 2018 Renesas Electronics Corporation + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#define USB3_DRD_CON 0x218 +#define DRD_CON_PERI_CON BIT(24) + +struct rcar_usb3_role_switch { + struct usb_role_switch *role_sw; + struct usb_role_switch_desc *desc; + void __iomem *reg; + enum usb_role save_role;/* for suspend/resume */ +}; + +static enum usb_role +rcar_usb3_role_switch_get_mode(struct rcar_usb3_role_switch *rcar_sw) +{ + u32 val = readl(rcar_sw->reg + USB3_DRD_CON); + + if (val & DRD_CON_PERI_CON) + return USB_ROLE_DEVICE; + + return USB_ROLE_HOST; +} + +static void +rcar_usb3_role_switch_set_mode(struct rcar_usb3_role_switch *rcar_sw, + bool host) +{ + void __iomem *drd_con = rcar_sw->reg + USB3_DRD_CON; + + if (host) + writel(readl(drd_con) & ~DRD_CON_PERI_CON, drd_con); + else + writel(readl(drd_con) | DRD_CON_PERI_CON,