| Message ID | 20231014231548.637303-2-tanmay.shah@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: get rproc devices for clusters | expand |
On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote: > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > Multi-cluster remoteproc designs typically have the following DT > declaration: > > remoteproc_cluster { > compatible = "soc,remoteproc-cluster"; > > core0: core0 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > }; > > core1: core1 { > compatible = "soc,remoteproc-core" > memory-region; > sram; > } > }; > > A driver exists for the cluster rather than the individual cores > themselves so that operation mode and HW specific configurations > applicable to the cluster can be made. > > Because the driver exists at the cluster level and not the individual > core level, function rproc_get_by_phandle() fails to return the > remoteproc associated with the phandled it is called for. > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > driver when the driver for the immediate remoteproc's parent is not > found. > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Tested-by: Ben Levinsky <ben.levinsky@amd.com> > --- > drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > index 695cce218e8c..3a8191803885 100644 > --- a/drivers/remoteproc/remoteproc_core.c > +++ b/drivers/remoteproc/remoteproc_core.c > @@ -33,6 +33,7 @@ > #include <linux/idr.h> > #include <linux/elf.h> > #include <linux/crc32.h> > +#include <linux/of_platform.h> > #include <linux/of_reserved_mem.h> > #include <linux/virtio_ids.h> > #include <linux/virtio_ring.h> > @@ -2111,7 +2112,9 @@ EXPORT_SYMBOL(rproc_detach); > #ifdef CONFIG_OF > struct rproc *rproc_get_by_phandle(phandle phandle) > { > + struct platform_device *cluster_pdev; > struct rproc *rproc = NULL, *r; > + struct device_driver *driver; > struct device_node *np; > > np = of_find_node_by_phandle(phandle); > @@ -2122,7 +2125,30 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > list_for_each_entry_rcu(r, &rproc_list, node) { > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > /* prevent underlying implementation from being removed */ > - if (!try_module_get(r->dev.parent->driver->owner)) { > + > + /* > + * If the remoteproc's parent has a driver, the > + * remoteproc is not part of a cluster and we can use > + * that driver. > + */ > + driver = r->dev.parent->driver; > + > + /* > + * If the remoteproc's parent does not have a driver, > + * look for the driver associated with the cluster. > + */ > + if (!driver) { > + cluster_pdev = of_find_device_by_node(np->parent); Both the Ti and Xilinx drivers are using of_platform_populate(), so their r->dev.parent should have a parent reference to the cluster device. Unless I'm reading the code wrong, I think we should follow that pointer, rather than taking the detour in the DeviceTree data. Regards, Bjorn > + if (!cluster_pdev) { > + dev_err(&r->dev, "can't get parent\n"); > + break; > + } > + > + driver = cluster_pdev->dev.driver; > + put_device(&cluster_pdev->dev); > + } > + > + if (!try_module_get(driver->owner)) { > dev_err(&r->dev, "can't get owner\n"); > break; > } > -- > 2.25.1 >
On Tue, 14 Nov 2023 at 08:22, Bjorn Andersson <andersson@kernel.org> wrote: > > On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote: > > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > Multi-cluster remoteproc designs typically have the following DT > > declaration: > > > > remoteproc_cluster { > > compatible = "soc,remoteproc-cluster"; > > > > core0: core0 { > > compatible = "soc,remoteproc-core" > > memory-region; > > sram; > > }; > > > > core1: core1 { > > compatible = "soc,remoteproc-core" > > memory-region; > > sram; > > } > > }; > > > > A driver exists for the cluster rather than the individual cores > > themselves so that operation mode and HW specific configurations > > applicable to the cluster can be made. > > > > Because the driver exists at the cluster level and not the individual > > core level, function rproc_get_by_phandle() fails to return the > > remoteproc associated with the phandled it is called for. > > > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > > driver when the driver for the immediate remoteproc's parent is not > > found. > > > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > Tested-by: Ben Levinsky <ben.levinsky@amd.com> > > --- > > drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > index 695cce218e8c..3a8191803885 100644 > > --- a/drivers/remoteproc/remoteproc_core.c > > +++ b/drivers/remoteproc/remoteproc_core.c > > @@ -33,6 +33,7 @@ > > #include <linux/idr.h> > > #include <linux/elf.h> > > #include <linux/crc32.h> > > +#include <linux/of_platform.h> > > #include <linux/of_reserved_mem.h> > > #include <linux/virtio_ids.h> > > #include <linux/virtio_ring.h> > > @@ -2111,7 +2112,9 @@ EXPORT_SYMBOL(rproc_detach); > > #ifdef CONFIG_OF > > struct rproc *rproc_get_by_phandle(phandle phandle) > > { > > + struct platform_device *cluster_pdev; > > struct rproc *rproc = NULL, *r; > > + struct device_driver *driver; > > struct device_node *np; > > > > np = of_find_node_by_phandle(phandle); > > @@ -2122,7 +2125,30 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > > list_for_each_entry_rcu(r, &rproc_list, node) { > > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > > /* prevent underlying implementation from being removed */ > > - if (!try_module_get(r->dev.parent->driver->owner)) { > > + > > + /* > > + * If the remoteproc's parent has a driver, the > > + * remoteproc is not part of a cluster and we can use > > + * that driver. > > + */ > > + driver = r->dev.parent->driver; > > + > > + /* > > + * If the remoteproc's parent does not have a driver, > > + * look for the driver associated with the cluster. > > + */ > > + if (!driver) { > > + cluster_pdev = of_find_device_by_node(np->parent); > > Both the Ti and Xilinx drivers are using of_platform_populate(), so > their r->dev.parent should have a parent reference to the cluster > device. > So you are proposing to get the cluster's driver using something like r->dev.parent->parent->driver? I will have to verify the parent/child relationship is set up properly through the of_platform_populate(). If it is, following the pointer trail is an equally valid approach and I will respin this set. > Unless I'm reading the code wrong, I think we should follow that > pointer, rather than taking the detour in the DeviceTree data. > > Regards, > Bjorn > > > + if (!cluster_pdev) { > > + dev_err(&r->dev, "can't get parent\n"); > > + break; > > + } > > + > > + driver = cluster_pdev->dev.driver; > > + put_device(&cluster_pdev->dev); > > + } > > + > > + if (!try_module_get(driver->owner)) { > > dev_err(&r->dev, "can't get owner\n"); > > break; > > } > > -- > > 2.25.1 > >
On 11/14/23 10:23 AM, Mathieu Poirier wrote: > On Tue, 14 Nov 2023 at 08:22, Bjorn Andersson <andersson@kernel.org> wrote: > > > > On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote: > > > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > > > Multi-cluster remoteproc designs typically have the following DT > > > declaration: > > > > > > remoteproc_cluster { > > > compatible = "soc,remoteproc-cluster"; > > > > > > core0: core0 { > > > compatible = "soc,remoteproc-core" > > > memory-region; > > > sram; > > > }; > > > > > > core1: core1 { > > > compatible = "soc,remoteproc-core" > > > memory-region; > > > sram; > > > } > > > }; > > > > > > A driver exists for the cluster rather than the individual cores > > > themselves so that operation mode and HW specific configurations > > > applicable to the cluster can be made. > > > > > > Because the driver exists at the cluster level and not the individual > > > core level, function rproc_get_by_phandle() fails to return the > > > remoteproc associated with the phandled it is called for. > > > > > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > > > driver when the driver for the immediate remoteproc's parent is not > > > found. > > > > > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > Tested-by: Ben Levinsky <ben.levinsky@amd.com> > > > --- > > > drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > index 695cce218e8c..3a8191803885 100644 > > > --- a/drivers/remoteproc/remoteproc_core.c > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > @@ -33,6 +33,7 @@ > > > #include <linux/idr.h> > > > #include <linux/elf.h> > > > #include <linux/crc32.h> > > > +#include <linux/of_platform.h> > > > #include <linux/of_reserved_mem.h> > > > #include <linux/virtio_ids.h> > > > #include <linux/virtio_ring.h> > > > @@ -2111,7 +2112,9 @@ EXPORT_SYMBOL(rproc_detach); > > > #ifdef CONFIG_OF > > > struct rproc *rproc_get_by_phandle(phandle phandle) > > > { > > > + struct platform_device *cluster_pdev; > > > struct rproc *rproc = NULL, *r; > > > + struct device_driver *driver; > > > struct device_node *np; > > > > > > np = of_find_node_by_phandle(phandle); > > > @@ -2122,7 +2125,30 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > > > list_for_each_entry_rcu(r, &rproc_list, node) { > > > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > > > /* prevent underlying implementation from being removed */ > > > - if (!try_module_get(r->dev.parent->driver->owner)) { > > > + > > > + /* > > > + * If the remoteproc's parent has a driver, the > > > + * remoteproc is not part of a cluster and we can use > > > + * that driver. > > > + */ > > > + driver = r->dev.parent->driver; > > > + > > > + /* > > > + * If the remoteproc's parent does not have a driver, > > > + * look for the driver associated with the cluster. > > > + */ > > > + if (!driver) { > > > + cluster_pdev = of_find_device_by_node(np->parent); > > > > Both the Ti and Xilinx drivers are using of_platform_populate(), so > > their r->dev.parent should have a parent reference to the cluster > > device. > > > > So you are proposing to get the cluster's driver using something like > r->dev.parent->parent->driver? > > I will have to verify the parent/child relationship is set up properly > through the of_platform_populate(). If it is, following the pointer > trail is an equally valid approach and I will respin this set. Hi Mathieu, I addressed Bjorn's comments and verified on ZynqMP hardware that it's working. Let me know if you would like to see v4 with suggested changes. Thanks, Tanmay > > Unless I'm reading the code wrong, I think we should follow that > > pointer, rather than taking the detour in the DeviceTree data. > > > > Regards, > > Bjorn > > > > > + if (!cluster_pdev) { > > > + dev_err(&r->dev, "can't get parent\n"); > > > + break; > > > + } > > > + > > > + driver = cluster_pdev->dev.driver; > > > + put_device(&cluster_pdev->dev); > > > + } > > > + > > > + if (!try_module_get(driver->owner)) { > > > dev_err(&r->dev, "can't get owner\n"); > > > break; > > > } > > > -- > > > 2.25.1 > > >
On Wed, Dec 20, 2023 at 08:47:19AM -0600, Tanmay Shah wrote: > > On 11/14/23 10:23 AM, Mathieu Poirier wrote: > > On Tue, 14 Nov 2023 at 08:22, Bjorn Andersson <andersson@kernel.org> wrote: > > > > > > On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote: > > > > From: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > > > > > Multi-cluster remoteproc designs typically have the following DT > > > > declaration: > > > > > > > > remoteproc_cluster { > > > > compatible = "soc,remoteproc-cluster"; > > > > > > > > core0: core0 { > > > > compatible = "soc,remoteproc-core" > > > > memory-region; > > > > sram; > > > > }; > > > > > > > > core1: core1 { > > > > compatible = "soc,remoteproc-core" > > > > memory-region; > > > > sram; > > > > } > > > > }; > > > > > > > > A driver exists for the cluster rather than the individual cores > > > > themselves so that operation mode and HW specific configurations > > > > applicable to the cluster can be made. > > > > > > > > Because the driver exists at the cluster level and not the individual > > > > core level, function rproc_get_by_phandle() fails to return the > > > > remoteproc associated with the phandled it is called for. > > > > > > > > This patch enhances rproc_get_by_phandle() by looking for the cluster's > > > > driver when the driver for the immediate remoteproc's parent is not > > > > found. > > > > > > > > Reported-by: Ben Levinsky <ben.levinsky@xilinx.com> > > > > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> > > > > Tested-by: Ben Levinsky <ben.levinsky@amd.com> > > > > --- > > > > drivers/remoteproc/remoteproc_core.c | 28 +++++++++++++++++++++++++++- > > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c > > > > index 695cce218e8c..3a8191803885 100644 > > > > --- a/drivers/remoteproc/remoteproc_core.c > > > > +++ b/drivers/remoteproc/remoteproc_core.c > > > > @@ -33,6 +33,7 @@ > > > > #include <linux/idr.h> > > > > #include <linux/elf.h> > > > > #include <linux/crc32.h> > > > > +#include <linux/of_platform.h> > > > > #include <linux/of_reserved_mem.h> > > > > #include <linux/virtio_ids.h> > > > > #include <linux/virtio_ring.h> > > > > @@ -2111,7 +2112,9 @@ EXPORT_SYMBOL(rproc_detach); > > > > #ifdef CONFIG_OF > > > > struct rproc *rproc_get_by_phandle(phandle phandle) > > > > { > > > > + struct platform_device *cluster_pdev; > > > > struct rproc *rproc = NULL, *r; > > > > + struct device_driver *driver; > > > > struct device_node *np; > > > > > > > > np = of_find_node_by_phandle(phandle); > > > > @@ -2122,7 +2125,30 @@ struct rproc *rproc_get_by_phandle(phandle phandle) > > > > list_for_each_entry_rcu(r, &rproc_list, node) { > > > > if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { > > > > /* prevent underlying implementation from being removed */ > > > > - if (!try_module_get(r->dev.parent->driver->owner)) { > > > > + > > > > + /* > > > > + * If the remoteproc's parent has a driver, the > > > > + * remoteproc is not part of a cluster and we can use > > > > + * that driver. > > > > + */ > > > > + driver = r->dev.parent->driver; > > > > + > > > > + /* > > > > + * If the remoteproc's parent does not have a driver, > > > > + * look for the driver associated with the cluster. > > > > + */ > > > > + if (!driver) { > > > > + cluster_pdev = of_find_device_by_node(np->parent); > > > > > > Both the Ti and Xilinx drivers are using of_platform_populate(), so > > > their r->dev.parent should have a parent reference to the cluster > > > device. > > > > > > > So you are proposing to get the cluster's driver using something like > > r->dev.parent->parent->driver? > > > > I will have to verify the parent/child relationship is set up properly > > through the of_platform_populate(). If it is, following the pointer > > trail is an equally valid approach and I will respin this set. > > > Hi Mathieu, > > I addressed Bjorn's comments and verified on ZynqMP hardware that it's working. > > Let me know if you would like to see v4 with suggested changes. > Yes, please send a V4 with the proposed changes. > > Thanks, > > Tanmay > > > > Unless I'm reading the code wrong, I think we should follow that > > > pointer, rather than taking the detour in the DeviceTree data. > > > > > > Regards, > > > Bjorn > > > > > > > + if (!cluster_pdev) { > > > > + dev_err(&r->dev, "can't get parent\n"); > > > > + break; > > > > + } > > > > + > > > > + driver = cluster_pdev->dev.driver; > > > > + put_device(&cluster_pdev->dev); > > > > + } > > > > + > > > > + if (!try_module_get(driver->owner)) { > > > > dev_err(&r->dev, "can't get owner\n"); > > > > break; > > > > } > > > > -- > > > > 2.25.1 > > > >
diff --git a/drivers/remoteproc/remoteproc_core.c b/drivers/remoteproc/remoteproc_core.c index 695cce218e8c..3a8191803885 100644 --- a/drivers/remoteproc/remoteproc_core.c +++ b/drivers/remoteproc/remoteproc_core.c @@ -33,6 +33,7 @@ #include <linux/idr.h> #include <linux/elf.h> #include <linux/crc32.h> +#include <linux/of_platform.h> #include <linux/of_reserved_mem.h> #include <linux/virtio_ids.h> #include <linux/virtio_ring.h> @@ -2111,7 +2112,9 @@ EXPORT_SYMBOL(rproc_detach); #ifdef CONFIG_OF struct rproc *rproc_get_by_phandle(phandle phandle) { + struct platform_device *cluster_pdev; struct rproc *rproc = NULL, *r; + struct device_driver *driver; struct device_node *np; np = of_find_node_by_phandle(phandle); @@ -2122,7 +2125,30 @@ struct rproc *rproc_get_by_phandle(phandle phandle) list_for_each_entry_rcu(r, &rproc_list, node) { if (r->dev.parent && device_match_of_node(r->dev.parent, np)) { /* prevent underlying implementation from being removed */ - if (!try_module_get(r->dev.parent->driver->owner)) { + + /* + * If the remoteproc's parent has a driver, the + * remoteproc is not part of a cluster and we can use + * that driver. + */ + driver = r->dev.parent->driver; + + /* + * If the remoteproc's parent does not have a driver, + * look for the driver associated with the cluster. + */ + if (!driver) { + cluster_pdev = of_find_device_by_node(np->parent); + if (!cluster_pdev) { + dev_err(&r->dev, "can't get parent\n"); + break; + } + + driver = cluster_pdev->dev.driver; + put_device(&cluster_pdev->dev); + } + + if (!try_module_get(driver->owner)) { dev_err(&r->dev, "can't get owner\n"); break; }