RE: [PATCH] usb: role: rcar-usb3-role-switch: add support for R-Car SoCs

[Yoshihiro Shimoda]

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

[Yoshihiro Shimoda]

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

[Heikki Krogerus]

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

[Heikki Krogerus]

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

[Yoshihiro Shimoda]

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

[Andy Shevchenko]

On Tue, Apr 10, 2018 at 3:03 PM, 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.

> +#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