[PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Sudeep Holla]

Commit 3aa0582fdb82 ("of: platform: populate /firmware/ node from
of_platform_default_populate_init()") changed the core-code to generate
the platform devices, meaning there is no need for the driver to check
the firmware node and populate it again here.

Let us just drop the unnecessary extra check done here as the core takes
care of it.

Cc: Richard Gong <richard.gong@linux.intel.com>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/stratix10-svc.c | 17 -----------------
 1 file changed, 17 deletions(-)

Hi Richard,

I assume the subsys_initcall is essential here. If not we can remove the
whole initcalls and replace it with module_platform_driver(). Let me know.
Just found this by accident when grepping for something else.

Regards,
Sudeep

diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
index d5f0769f3761..791d70fe82c1 100644
--- a/drivers/firmware/stratix10-svc.c
+++ b/drivers/firmware/stratix10-svc.c
@@ -1094,23 +1094,6 @@ static struct platform_driver stratix10_svc_driver = {
 
 static int __init stratix10_svc_init(void)
 {
-	struct device_node *fw_np;
-	struct device_node *np;
-	int ret;
-
-	fw_np = of_find_node_by_name(NULL, "firmware");
-	if (!fw_np)
-		return -ENODEV;
-
-	np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
-	if (!np)
-		return -ENODEV;
-
-	of_node_put(np);
-	ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
-	if (ret)
-		return ret;
-
 	return platform_driver_register(&stratix10_svc_driver);
 }
 
-- 
2.17.1

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Richard Gong]

Hi Sudeep,

I tried and couldn't load stratix10-svc driver with your patch on kernel 
5.6.

Regards,
Richard

On 4/21/20 12:32 PM, Sudeep Holla wrote:
> Commit 3aa0582fdb82 ("of: platform: populate /firmware/ node from
> of_platform_default_populate_init()") changed the core-code to generate
> the platform devices, meaning there is no need for the driver to check
> the firmware node and populate it again here.
> 
> Let us just drop the unnecessary extra check done here as the core takes
> care of it.
> 
> Cc: Richard Gong <richard.gong@linux.intel.com>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>   drivers/firmware/stratix10-svc.c | 17 -----------------
>   1 file changed, 17 deletions(-)
> 
> Hi Richard,
> 
> I assume the subsys_initcall is essential here. If not we can remove the
> whole initcalls and replace it with module_platform_driver(). Let me know.
> Just found this by accident when grepping for something else.
> 
> Regards,
> Sudeep
> 
> diff --git a/drivers/firmware/stratix10-svc.c b/drivers/firmware/stratix10-svc.c
> index d5f0769f3761..791d70fe82c1 100644
> --- a/drivers/firmware/stratix10-svc.c
> +++ b/drivers/firmware/stratix10-svc.c
> @@ -1094,23 +1094,6 @@ static struct platform_driver stratix10_svc_driver = {
>   
>   static int __init stratix10_svc_init(void)
>   {
> -	struct device_node *fw_np;
> -	struct device_node *np;
> -	int ret;
> -
> -	fw_np = of_find_node_by_name(NULL, "firmware");
> -	if (!fw_np)
> -		return -ENODEV;
> -
> -	np = of_find_matching_node(fw_np, stratix10_svc_drv_match);
> -	if (!np)
> -		return -ENODEV;
> -
> -	of_node_put(np);
> -	ret = of_platform_populate(fw_np, stratix10_svc_drv_match, NULL, NULL);
> -	if (ret)
> -		return ret;
> -
>   	return platform_driver_register(&stratix10_svc_driver);
>   }
>   
> 

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Sudeep Holla]

On Wed, Apr 22, 2020 at 04:50:00PM -0500, Richard Gong wrote:
> Hi Sudeep,
>
> I tried and couldn't load stratix10-svc driver with your patch on kernel
> 5.6.
>

What exactly do you mean by not loading stratix10-svc driver ?
This patch doesn't change that part, the driver should still get loaded.
The change may affect probing part if for some reason the devices for
nodes under firmware are not populated which I still can't understand.

Do you see any change under i/sys/devices/platform/firmware\:* with
and without this change ?

Lots of drivers removed the code similar to this patch after the Commit
3aa0582fdb82 ("of: platform: populate /firmware/ node from
of_platform_default_populate_init()") and continue to work fine. I am
interested to see what is different in stratix10-svc.

--
Regards,
Sudeep

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Richard Gong]

Hi Sudeep,

In our dts, firmware is not under root node. You can refer to 
arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi for details.

This is why we need check and populate firmware node.

Regards,
Richard

On 4/23/20 3:11 AM, Sudeep Holla wrote:
> On Wed, Apr 22, 2020 at 04:50:00PM -0500, Richard Gong wrote:
>> Hi Sudeep,
>>
>> I tried and couldn't load stratix10-svc driver with your patch on kernel
>> 5.6.
>>
> 
> What exactly do you mean by not loading stratix10-svc driver ?
> This patch doesn't change that part, the driver should still get loaded.
> The change may affect probing part if for some reason the devices for
> nodes under firmware are not populated which I still can't understand.
> 
> Do you see any change under i/sys/devices/platform/firmware\:* with
> and without this change ?
> 
> Lots of drivers removed the code similar to this patch after the Commit
> 3aa0582fdb82 ("of: platform: populate /firmware/ node from
> of_platform_default_populate_init()") and continue to work fine. I am
> interested to see what is different in stratix10-svc.
> 
> --
> Regards,
> Sudeep
> 

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Sudeep Holla]

On Mon, Apr 27, 2020 at 02:12:56PM -0500, Richard Gong wrote:
> Hi Sudeep,
> 
> In our dts, firmware is not under root node. You can refer to
> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi for details.
> 
> This is why we need check and populate firmware node.
> 

Ah that's bad. One of very few DTS I see firmware node not in the root.
But this driver is the only one duplicating the code.

-- 
Regards,
Sudeep

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Richard Gong]

Hi,

On 4/28/20 3:32 AM, Sudeep Holla wrote:
> On Mon, Apr 27, 2020 at 02:12:56PM -0500, Richard Gong wrote:
>> Hi Sudeep,
>>
>> In our dts, firmware is not under root node. You can refer to
>> arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi for details.
>>
>> This is why we need check and populate firmware node.
>>
> 
> Ah that's bad. One of very few DTS I see firmware node not in the root.Per the Devicetree Specification, there is no need to define firmware 
under the root node. Some examples are fsl-ls1012a.dtsi and 
hi6220-hikey.dts.

> But this driver is the only one duplicating the codethen Commit 3aa0582fdb82 should be extended to handle firmware which is 
not defined under the root node. I am not sure if it is doable.
>
Regards,
Richard

Re: [PATCH] firmware: stratix10-svc: Drop unnecessary checking for and populating /firmware/ node

[Sudeep Holla]

On Tue, Apr 28, 2020 at 09:14:25AM -0500, Richard Gong wrote:
> Hi,
> 
> On 4/28/20 3:32 AM, Sudeep Holla wrote:
> > On Mon, Apr 27, 2020 at 02:12:56PM -0500, Richard Gong wrote:
> > > Hi Sudeep,
> > > 
> > > In our dts, firmware is not under root node. You can refer to
> > > arch/arm64/boot/dts/altera/socfpga_stratix10.dtsi for details.
> > > 
> > > This is why we need check and populate firmware node.
> > > 
> > 
> > Ah that's bad. One of very few DTS I see firmware node not in the
> > root.
>
> Per the Devicetree Specification, there is no need to define firmware
> under the root node. Some examples are fsl-ls1012a.dtsi and
> hi6220-hikey.dts.
>

Yes I checked that before I replied, otherwise I would have asked to change
😉 

> > But this driver is the only one duplicating the code
> then Commit 3aa0582fdb82 should be extended to handle firmware which is
> not defined under the root node. I am not sure if it is doable.

I agree, I need to take a look at it again.

-- 
Regards,
Sudeep