Re: [RESEND PATCH v3 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters

2024-01-02 Thread Mathieu Poirier
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  wrote:
> > >
> > > On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote:
> > > > From: Mathieu Poirier 
> > > >
> > > > 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 
> > > > Signed-off-by: Mathieu Poirier 
> > > > Tested-by: Ben Levinsky 
> > > > ---
> > > >  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 
> > > >  #include 
> > > >  #include 
> > > > +#include 
> > > >  #include 
> > > >  #include 
> > > >  #include 
> > > > @@ -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, _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(>dev, "can't get 
> > > > parent\n");
> > > > + break;
> > > > + 

Re: [RESEND PATCH v3 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters

2023-12-20 Thread Tanmay Shah


On 11/14/23 10:23 AM, Mathieu Poirier wrote:
> On Tue, 14 Nov 2023 at 08:22, Bjorn Andersson  wrote:
> >
> > On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote:
> > > From: Mathieu Poirier 
> > >
> > > 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 
> > > Signed-off-by: Mathieu Poirier 
> > > Tested-by: Ben Levinsky 
> > > ---
> > >  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 
> > >  #include 
> > >  #include 
> > > +#include 
> > >  #include 
> > >  #include 
> > >  #include 
> > > @@ -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, _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(>dev, "can't get 
> > > parent\n");
> > > + break;
> > > + }
> > > +
> > > + driver = cluster_pdev->dev.driver;
> > > + put_device(_pdev->dev);
> > > + }
> > > +
> > > + if (!try_module_get(driver->owner)) {
> > >   dev_err(>dev, "can't get owner\n");
> > >   break;
> > >   

Re: [RESEND PATCH v3 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters

2023-11-14 Thread Mathieu Poirier
On Tue, 14 Nov 2023 at 08:22, Bjorn Andersson  wrote:
>
> On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote:
> > From: Mathieu Poirier 
> >
> > 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 
> > Signed-off-by: Mathieu Poirier 
> > Tested-by: Ben Levinsky 
> > ---
> >  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 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -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, _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(>dev, "can't get 
> > parent\n");
> > + break;
> > + }
> > +
> > + driver = cluster_pdev->dev.driver;
> > + put_device(_pdev->dev);
> > + }
> > +
> > + if (!try_module_get(driver->owner)) {
> >   dev_err(>dev, "can't get owner\n");
> >   break;
> >   }
> > --
> > 2.25.1
> >


Re: [RESEND PATCH v3 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters

2023-11-14 Thread Bjorn Andersson
On Sat, Oct 14, 2023 at 04:15:47PM -0700, Tanmay Shah wrote:
> From: Mathieu Poirier 
> 
> 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 
> Signed-off-by: Mathieu Poirier 
> Tested-by: Ben Levinsky 
> ---
>  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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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, _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(>dev, "can't get parent\n");
> + break;
> + }
> +
> + driver = cluster_pdev->dev.driver;
> + put_device(_pdev->dev);
> + }
> +
> + if (!try_module_get(driver->owner)) {
>   dev_err(>dev, "can't get owner\n");
>   break;
>   }
> -- 
> 2.25.1
> 


[RESEND PATCH v3 1/2] remoteproc: Make rproc_get_by_phandle() work for clusters

2023-10-14 Thread Tanmay Shah
From: Mathieu Poirier 

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 
Signed-off-by: Mathieu Poirier 
Tested-by: Ben Levinsky 
---
 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 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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, _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(>dev, "can't get parent\n");
+   break;
+   }
+
+   driver = cluster_pdev->dev.driver;
+   put_device(_pdev->dev);
+   }
+
+   if (!try_module_get(driver->owner)) {
dev_err(>dev, "can't get owner\n");
break;
}
-- 
2.25.1