Re: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> This patch adds a new API "device_connection_find_by_graph()" to
> find device connection by using graph.
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  Documentation/driver-api/device_connection.rst |  4 +--
>  drivers/base/devcon.c  | 43 
> ++
>  include/linux/device.h |  2 ++
>  3 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/driver-api/device_connection.rst 
> b/Documentation/driver-api/device_connection.rst
> index affbc556..2e2d26f 100644
> --- a/Documentation/driver-api/device_connection.rst
> +++ b/Documentation/driver-api/device_connection.rst
> @@ -19,7 +19,7 @@ Device connections alone do not create a dependency between 
> the two devices.
>  They are only descriptions which are not tied to either of the devices 
> directly.
>  A dependency between the two devices exists only if one of the two endpoint
>  devices requests a reference to the other. The descriptions themselves can be
> -defined in firmware (not yet supported) or they can be built-in.
> +defined in firmware or they can be built-in.
>  
>  Usage
>  -
> @@ -40,4 +40,4 @@ API
>  ---
>  
>  .. kernel-doc:: drivers/base/devcon.c
> -   : functions: device_connection_find_match device_connection_find 
> device_connection_add device_connection_remove
> +   : functions: device_connection_find_match device_connection_find 
> device_connection_add device_connection_remove device_connection_find_by_graph
> diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> index d427e80..5a0da33 100644
> --- a/drivers/base/devcon.c
> +++ b/drivers/base/devcon.c
> @@ -7,6 +7,7 @@
>   */
>  
>  #include 
> +#include 
>  
>  static DEFINE_MUTEX(devcon_lock);
>  static LIST_HEAD(devcon_list);
> @@ -134,3 +135,45 @@ void device_connection_remove(struct device_connection 
> *con)
>   mutex_unlock(_lock);
>  }
>  EXPORT_SYMBOL_GPL(device_connection_remove);
> +
> +static int generic_graph_match(struct device *dev, void *fwnode)
> +{
> + return dev->fwnode == fwnode;
> +}
> +
> +/**
> + * device_connection_find_by_graph - Find two devices connected together
> + * @dev: Device to find connected device
> + * @port: identifier of the @dev port node
> + * @endpoint: identifier of the @dev endpoint node
> + *
> + * Find a connection with @port and @endpoint by using graph between @dev and
> + * another device. On success returns handle to the device that is connected
> + * to @dev, with the reference count for the found device incremented. 
> Returns
> + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) when
> + * a connection was found but the other device has not been enumerated yet.
> + */
> +struct device *device_connection_find_by_graph(struct device *dev, u32 port,
> +u32 endpoint)
> +{
> + struct bus_type *bus;
> + struct fwnode_handle *remote;
> + struct device *conn;
> +
> + remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> + if (!remote)
> + return NULL;
> +
> + for (bus = generic_match_buses[0]; bus; bus++) {
> + conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> + if (conn)
> + return conn;
> + }
> +
> + /*
> +  * We only get called if a connection was found, tell the caller to
> +  * wait for the other device to show up.
> +  */
> + return ERR_PTR(-EPROBE_DEFER);
> +}
> +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);

Why do we need more API for walking through the graph?

I'm not sure exactly sure what is going on here, I'll try to study
your patches more when I have time, but the approach looks wrong. That
function looks like a helper, but just not that useful one.

We really should be able to use the existing functions. In practice
device_connection_find_match() should eventually parse the graph, then
fallback to build-in connections if no graph is found. Otherwise
parsing graph here is not really useful at all.


Thanks,

-- 
heikki

Re: [PATCH/RFC v3 1/4] base: devcon: add a new API to find the graph

[Heikki Krogerus]

On Tue, May 15, 2018 at 02:19:14AM +, Yoshihiro Shimoda wrote:
> Hi Heikki,
> 
> > From: Heikki Krogerus, Sent: Monday, May 14, 2018 10:28 PM
> > 
> > On Mon, May 14, 2018 at 06:15:57PM +0900, Yoshihiro Shimoda wrote:
> 
> > > diff --git a/drivers/base/devcon.c b/drivers/base/devcon.c
> > > index d427e80..5a0da33 100644
> > > --- a/drivers/base/devcon.c
> > > +++ b/drivers/base/devcon.c
> 
> > > +static int generic_graph_match(struct device *dev, void *fwnode)
> > > +{
> > > + return dev->fwnode == fwnode;
> > > +}
> > > +
> > > +/**
> > > + * device_connection_find_by_graph - Find two devices connected together
> > > + * @dev: Device to find connected device
> > > + * @port: identifier of the @dev port node
> > > + * @endpoint: identifier of the @dev endpoint node
> > > + *
> > > + * Find a connection with @port and @endpoint by using graph between 
> > > @dev and
> > > + * another device. On success returns handle to the device that is 
> > > connected
> > > + * to @dev, with the reference count for the found device incremented. 
> > > Returns
> > > + * NULL if no matching connection was found, or ERR_PTR(-EPROBE_DEFER) 
> > > when
> > > + * a connection was found but the other device has not been enumerated 
> > > yet.
> > > + */
> > > +struct device *device_connection_find_by_graph(struct device *dev, u32 
> > > port,
> > > +u32 endpoint)
> > > +{
> > > + struct bus_type *bus;
> > > + struct fwnode_handle *remote;
> > > + struct device *conn;
> > > +
> > > + remote = fwnode_graph_get_remote_node(dev_fwnode(dev), port, endpoint);
> > > + if (!remote)
> > > + return NULL;
> > > +
> > > + for (bus = generic_match_buses[0]; bus; bus++) {
> > > + conn = bus_find_device(bus, NULL, remote, generic_graph_match);
> > > + if (conn)
> > > + return conn;
> > > + }
> > > +
> > > + /*
> > > +  * We only get called if a connection was found, tell the caller to
> > > +  * wait for the other device to show up.
> > > +  */
> > > + return ERR_PTR(-EPROBE_DEFER);
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_connection_find_by_graph);
> > 
> > Why do we need more API for walking through the graph?
> 
> I thought there is difficult to find if a device has multiple ports or 
> endpoints.
> So, I'd like to use port and endpoint number for finding a device.
> 
> > I'm not sure exactly sure what is going on here, I'll try to study
> > your patches more when I have time, but the approach looks wrong. That
> > function looks like a helper, but just not that useful one.
> > 
> > We really should be able to use the existing functions. In practice
> > device_connection_find_match() should eventually parse the graph, then
> > fallback to build-in connections if no graph is found. Otherwise
> > parsing graph here is not really useful at all.
> 
> I think using device_connection_find_match() for finding the graph becomes 
> complicated.
> The current arguments of the function is the below:
> 
> void *device_connection_find_match(struct device *dev, const char *con_id,
>  void *data,
>  void *(*match)(struct device_connection *con,
> int ep, void *data))
> 
> If finding the graph, the following arguments will be not used.
>  - con_id
>  - *con and ep of "match" arguments.
> 
> This is because these arguments are for the build-in connections.

No they're not. You do realize that we can build a temporary struct
device_connection there and then (in stack) to be supplied for the
->match callback.

> So, should I modify the arguments of the function for finding both
> the graph and built-in connections somehow?

I don't see any need for that. We may need to modify struct
device_connection, but there should not be any need to touch the API.

Your plan to use port and endpoint number is wrong, as they are just
indexes, and therefore not reliable. Luckily it should be completely
unnecessary to use them.

The way I see this working is that we parse all the endpoints the
caller device has. If we can't take advantage of the con_id for
identification (though ideally we can), we just need to call ->match
with every one of them. The implementation for the ->match callback is
then responsible of figuring out if the endpoint is the one we are
lookin

Re: [PATCH v6] usb: gadget: udc: renesas_usb3: Add register of usb role switch

[Heikki Krogerus]

On Thu, May 31, 2018 at 03:11:33PM +0900, Yoshihiro Shimoda wrote:
> This patch adds role switch support for R-Car SoCs into the USB 3.0
> peripheral driver. 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 (DRD_CON) contains
> the USB 3.0 peripheral controller side only. So, this renesas_usb3
> driver manages the DRD_CON now. 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 reinitialization by a driver, this driver also
> registers a role switch driver to manage the DRD_CON and get
> a device pointer of usb 3.0 host from "companion" property of OF.
> Then, when the usb role is changed, renesas_usb3_role_switch_set()
> will attach/release the xhci-plat driver to reinitialize the host
> hardware.
> 
> Signed-off-by: Yoshihiro Shimoda 

Reviewed-by: Heikki Krogerus 

> ---
> This patch set is based on Felipe's usb.git / testing/next branch
> (commit id = 47265c067c0d129f3a0e94bc221293a780af9d78).
> 
> Changes from v5 (https://patchwork.kernel.org/patch/10426483/):
>  - Add a new function "usb3_set_mode_by_role_sw()" instead of reusing
>usb3_set_mode() and add _usb3_set_mode().
>  - Change a coding style of container_of.
>  - Revise an error message in renesas_usb3_role_switch_set().
>  - Add "const" for "renesas_usb3_role_switch_desc".
>  - Simplify a condition check of usb_of_get_companion_dev().
> 
> Changes from RFC v4 (https://patchwork.kernel.org/patch/10418265/):
>  - Use "companion" device tree property simply instead of device_connection
>APIs with OF graph.
>  - Merge patch 2 and 3 to one.
>  - Revise the commit log (I should had revised this on RFC v4 though).
> 
> Changes from RFC v3:
>  - Rebase latest usb.git / testing/next branch.
>  - Add graph parse into device_connection_find_match().
>  - Use workqueue to call _usb3_set_mode() in patch 3.
>(I realized renesas_usb3_role_switch_set() cannot run on atomic because
> device_attach() might sleep.)
> 
> Changes from RFC v2:
>  - Add registering usb role switch into drivers/usb/gadget/udc/renesas_usb3
>because hardware resource (a register) is shared and remove individual
>usb role switch driver/dt-bindings for R-Car.
>  - Remove "usb_role_switch_get_by_graph" API because the renesas_usb3 driver
>doesn't need such API now.
> 
> Changes from RFC:
>  - Remove "device-connection-id" and "usb role switch driver" dt-bingings.
>  - Remove drivers/of code.
>  - Add a new API for find the connection by using graph on devcon.c and 
> roles.c.
>  - Use each new API on the rcar usb role switch and renesas_usb3 drivers.
>  - Update the dtsi file for r8a7795.
> 
> 
>  drivers/usb/gadget/udc/Kconfig|  1 +
>  drivers/usb/gadget/udc/renesas_usb3.c | 84 
> ++-
>  2 files changed, 84 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index b838cae..78823cd 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -193,6 +193,7 @@ config USB_RENESAS_USB3
>   tristate 'Renesas USB3.0 Peripheral controller'
>   depends on ARCH_RENESAS || COMPILE_TEST
>   depends on EXTCON && HAS_DMA
> + select USB_ROLE_SWITCH
>   help
>  Renesas USB3.0 Peripheral controller is a USB peripheral controller
>  that supports super, high, and full speed USB 3.0 data transfers.
> diff --git a/drivers/usb/gadget/udc/renesas_usb3.c 
> b/drivers/usb/gadget/udc/renesas_usb3.c
> index 5caf78b..f44a4b1 100644
> --- a/drivers/usb/gadget/udc/renesas_usb3.c
> +++ b/drivers/usb/gadget/udc/renesas_usb3.c
> @@ -23,6 +23,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  
>  /* register definitions */
>  #define USB3_AXI_INT_STA 0x008
> @@ -335,6 +337,11 @@ struct renesas_usb3 {
>   struct phy *phy;
>   struct dentry *dentry;
>  
> + struct usb_role_switch *role_sw;
> + struct device *host_dev;
> + struct work_struct role_work;
> + enum usb_role role;
> +
>   struct renesas_usb3_ep *usb3_ep;
>   int num_usb3_eps;
>  
> @@ -651,6 +658,14 @@ static void usb3_check_vbus(struct renesas_usb3 *usb3)
>   }
>  }
>  
> +static void renesas_usb3_role_work(struct work_struct *work)
>

Re: [PATCH v5] usb: gadget: udc: renesas_usb3: Add register of usb role switch

[Heikki Krogerus]

On Fri, May 25, 2018 at 04:12:56PM +0900, Yoshihiro Shimoda wrote:
> @@ -2573,6 +2635,12 @@ static void renesas_usb3_init_ram(struct renesas_usb3 
> *usb3, struct device *dev,
>   EXTCON_NONE,
>  };
>  
> +static struct usb_role_switch_desc renesas_usb3_role_switch_desc = {

You can constify that.

> + .set = renesas_usb3_role_switch_set,
> + .get = renesas_usb3_role_switch_get,
> + .allow_userspace_control = true,
> +};
> +
>  static int renesas_usb3_probe(struct platform_device *pdev)
>  {
>   struct renesas_usb3 *usb3;

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

[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/RFC 04/11] of: platform: add device connection parsing

[Heikki Krogerus]

Hi Yoshihiro,

On Wed, Apr 18, 2018 at 05:09:58PM +0900, Yoshihiro Shimoda wrote:
> This patch adds device connection parsing in of_platform_populate().
> 
> TODO:
>  - How to free the devcon memories?
>  - How to remove the devcon instances?
> 
> Signed-off-by: Yoshihiro Shimoda 
> ---
>  drivers/of/platform.c | 66 
> +++
>  include/linux/of.h|  1 +
>  2 files changed, 67 insertions(+)
> 
> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> index c00d81d..41c018d 100644
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -326,6 +327,63 @@ static const struct of_dev_auxdata *of_dev_lookup(const 
> struct of_dev_auxdata *l
>   return NULL;
>  }
>  
> +static int of_parse_device_connection(struct device_node *np)
> +{
> + struct device_node *child_ep, *parent, *remote_parent;
> + struct platform_device *pdev, *remote_pdev;
> + struct device_connection *devcon = NULL;
> + const char *id;
> +
> + if (of_node_check_flag(np, OF_DEVICE_CONNECTED)) {
> + pr_debug("%s() - skipping %pOF, already device connected\n",
> + __func__, np);
> + return 0;
> + }
> +
> + of_node_set_flag(np, OF_DEVICE_CONNECTED);
> +
> + for_each_endpoint_of_node(np, child_ep) {
> + if (of_property_read_string(child_ep, "device-connection-id",
> + ) < 0)
> + continue;
> +
> + remote_parent = of_graph_get_remote_port_parent(child_ep);
> + if (!remote_parent)
> + return 0;
> +
> + parent = of_graph_get_port_parent(child_ep);
> + if (!parent)
> + return 0;
> +
> + pdev = of_find_device_by_node(parent);
> + if (!pdev)
> + return 0;
> +
> + /*
> +  * WARN_ON in really_probe() may happen if devm_kzalloc is
> +  * used. TODO: How to free this?
> +  */
> + devcon = kzalloc(sizeof(*devcon), GFP_KERNEL);
> + if (!devcon)
> + return -ENOMEM;
> +
> + devcon->id = id;
> + remote_pdev = of_find_device_by_node(remote_parent);
> + if (!remote_pdev) {
> + kfree(devcon);
> + return 0;
> + }
> +
> + devcon->endpoint[0] = dev_name(>dev);
> + devcon->endpoint[1] = dev_name(_pdev->dev);
> +
> + /* TODO: How to remove the connection? */
> + device_connection_add(devcon);

This is wrong. You are converting a connection that is described in
graph into a "build-in" one. If the connection is already described in
graph there is no need to, actually, we _should not_, add it to the
list of "build-in" connection descriptions.

What should be done is that we parse the graph the moment
device_connection_find*() is called. And that should be done in
drivers/base/devcon.c .

> + }
> +
> + return 0;
> +}
> +
>  /**
>   * of_platform_bus_create() - Create a device for a node and its children.
>   * @bus: device node of the bus to instantiate
> @@ -477,6 +535,14 @@ int of_platform_populate(struct device_node *root,
>   }
>   of_node_set_flag(root, OF_POPULATED_BUS);
>  
> + for_each_child_of_node(root, child) {
> + rc = of_parse_device_connection(child);
> + if (rc) {
> + of_node_put(child);
> + break;
> + }
> + }
> +
>   of_node_put(root);
>   return rc;
>  }
> diff --git a/include/linux/of.h b/include/linux/of.h
> index 4d25e4f..30aa103 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -143,6 +143,7 @@ static inline void of_node_put(struct device_node *node) 
> { }
>  #define OF_DETACHED  2 /* node has been detached from the device tree */
>  #define OF_POPULATED 3 /* device already created for the node */
>  #define OF_POPULATED_BUS 4 /* of_platform_populate recursed to children 
> of this node */
> +#define OF_DEVICE_CONNECTED  5 /* checked devcon on of_platform_populate */
>  
>  #define OF_BAD_ADDR  ((u64)-1)


Thanks,

-- 
heikki