| Message ID | 20231012132214.257207-1-emkan@prevas.dk (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | pmdomain: imx: gpc: set fwnode from parent device | expand |
On 10/12/23 15:22, Emil Kronborg Andersen wrote: > Before commit 3fb16866b51d ("driver core: fw_devlink: Make cycle > detection more robust"), consumer devices without their fwnode set > printed errors like the one below: The commit message is wrong. Instead of "Before commit...", it should read "After commit...". Do you want me to send a new patch or can you fix it? Best regards, Emil
Hi Emil, On Fri, Oct 13, 2023 at 6:57 AM Emil Kronborg Andersen <emil.andersen@prevas.dk> wrote: > > On 10/12/23 15:22, Emil Kronborg Andersen wrote: > > Before commit 3fb16866b51d ("driver core: fw_devlink: Make cycle > > detection more robust"), consumer devices without their fwnode set > > printed errors like the one below: > > The commit message is wrong. Instead of "Before commit...", it should > read "After commit...". Do you want me to send a new patch or can you > fix it? Please send a new version with the commit message fixed and also with the Fixes tag. Thanks
On Thu, Oct 12, 2023 at 6:22 AM Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > Before commit 3fb16866b51d ("driver core: fw_devlink: Make cycle > detection more robust"), consumer devices without their fwnode set > printed errors like the one below: > > [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu > > To fix this, set the fwnode so fw_devlink can find and handle it > properly. > > Signed-off-by: Emil Kronborg Andersen <emkan@prevas.dk> > --- > drivers/pmdomain/imx/gpc.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c > index 90a8b2c0676f..8759eab72880 100644 > --- a/drivers/pmdomain/imx/gpc.c > +++ b/drivers/pmdomain/imx/gpc.c > @@ -411,6 +411,8 @@ static int imx_gpc_probe(struct platform_device *pdev) > void __iomem *base; > int ret; > > + device_set_node(&pdev->dev, dev_fwnode(pdev->dev.parent)); > + Similar question as the other patch. Can we do this at a pmdomain framework level instead of per driver? -Saravana > pgc_node = of_get_child_by_name(pdev->dev.of_node, "pgc"); > > /* bail out if DT too old and doesn't provide the necessary info */ > -- > 2.41.0 >
On Fri, 13 Oct 2023 at 23:30, Saravana Kannan <saravanak@google.com> wrote: > > On Thu, Oct 12, 2023 at 6:22 AM Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > > > Before commit 3fb16866b51d ("driver core: fw_devlink: Make cycle > > detection more robust"), consumer devices without their fwnode set > > printed errors like the one below: > > > > [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu > > > > To fix this, set the fwnode so fw_devlink can find and handle it > > properly. > > > > Signed-off-by: Emil Kronborg Andersen <emkan@prevas.dk> > > --- > > drivers/pmdomain/imx/gpc.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c > > index 90a8b2c0676f..8759eab72880 100644 > > --- a/drivers/pmdomain/imx/gpc.c > > +++ b/drivers/pmdomain/imx/gpc.c > > @@ -411,6 +411,8 @@ static int imx_gpc_probe(struct platform_device *pdev) > > void __iomem *base; > > int ret; > > > > + device_set_node(&pdev->dev, dev_fwnode(pdev->dev.parent)); > > + > > Similar question as the other patch. Can we do this at a pmdomain > framework level instead of per driver? Unfortunately, it's not that easy. It's not required by pm_genpd_init() (or of_genpd_add_provider_*) to use a struct *device when registering a genpd provider. In fact, that's also the reason why pm_genpd_init() needs to initialize its own struct device. That said, can you please explain the special condition that caused this thing to be needed in the first place? [...] Kind regards Uffe
Hi Ulf, On Mon, Oct 16, 2023 at 11:53 +0200, Ulf Hansson wrote: > Unfortunately, it's not that easy. It's not required by > pm_genpd_init() (or of_genpd_add_provider_*) to use a struct *device > when registering a genpd provider. In fact, that's also the reason why > pm_genpd_init() needs to initialize its own struct device. Do you see any way to do it properly at pmdomain level instead, or is that not feasible? I agree with Saravana that a centralized solution, akin to what was implemented for tty devices in commit 1a5ecc73b2bf ("serdev: Set fwnode for serdev devices"), would be ideal. However, I was not able to find other reports or patches of problems regarding fw_devlink at pmdomain level, so it might not be a general problem here, and fixes per driver might be okay in this case? > That said, can you please explain the special condition that caused > this thing to be needed in the first place? I noticed this issue when I updated from 5.15.93 to 6.1.37. Except for the commit I bisected, i.e., 3fb16866b51d ("driver core: fw_devlink: Make cycle detection more robust"), I found that the issue was introduced somewhere between 6.1.16 and 6.1.15. Here, some commits regarding fw_devlink was backported, which might be relevant: $ git log --oneline v6.1.15..v6.1.16 -- drivers/base/core.c | grep fw_devlink 11d93294b7c3 driver core: fw_devlink: Avoid spurious error message 3dd596616d10 driver core: fw_devlink: Make cycle detection more robust a3c171751512 driver core: fw_devlink: Improve check for fwnode with no device/driver 7a8ce4d2fbbc driver core: fw_devlink: Consolidate device link flag computation 16aa2487cf15 driver core: fw_devlink: Allow marking a fwnode link as being part of a cycle eaf9b5612a47 driver core: fw_devlink: Don't purge child fwnode's consumer links 2455b81afe68 driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links Best regards, Emil
On Tue, 17 Oct 2023 at 10:00, Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > Hi Ulf, > > On Mon, Oct 16, 2023 at 11:53 +0200, Ulf Hansson wrote: > > Unfortunately, it's not that easy. It's not required by > > pm_genpd_init() (or of_genpd_add_provider_*) to use a struct *device > > when registering a genpd provider. In fact, that's also the reason why > > pm_genpd_init() needs to initialize its own struct device. > > Do you see any way to do it properly at pmdomain level instead, or is > that not feasible? I agree with Saravana that a centralized solution, > akin to what was implemented for tty devices in commit 1a5ecc73b2bf > ("serdev: Set fwnode for serdev devices"), would be ideal. However, I > was not able to find other reports or patches of problems regarding > fw_devlink at pmdomain level, so it might not be a general problem here, > and fixes per driver might be okay in this case? Sure, we could fix it though a centralized solution, but it requires us to patch each and every genpd provider driver too. At this point I would therefore say, let's just fix it for those that need it. > > > That said, can you please explain the special condition that caused > > this thing to be needed in the first place? > > I noticed this issue when I updated from 5.15.93 to 6.1.37. Except for > the commit I bisected, i.e., 3fb16866b51d ("driver core: fw_devlink: > Make cycle detection more robust"), I found that the issue was > introduced somewhere between 6.1.16 and 6.1.15. Here, some commits > regarding fw_devlink was backported, which might be relevant: > > $ git log --oneline v6.1.15..v6.1.16 -- drivers/base/core.c | grep fw_devlink > 11d93294b7c3 driver core: fw_devlink: Avoid spurious error message > 3dd596616d10 driver core: fw_devlink: Make cycle detection more robust > a3c171751512 driver core: fw_devlink: Improve check for fwnode with no device/driver > 7a8ce4d2fbbc driver core: fw_devlink: Consolidate device link flag computation > 16aa2487cf15 driver core: fw_devlink: Allow marking a fwnode link as being part of a cycle > eaf9b5612a47 driver core: fw_devlink: Don't purge child fwnode's consumer links > 2455b81afe68 driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links Thanks for sharing the commits. I haven't looked much closer to them, but maybe Saravana has some ideas about whether this potentially can be fixed in the fw_devlink parsing instead? Kind regards Uffe
+ Pengfei Li On Tue, 17 Oct 2023 at 12:17, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > On Tue, 17 Oct 2023 at 10:00, Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > > > Hi Ulf, > > > > On Mon, Oct 16, 2023 at 11:53 +0200, Ulf Hansson wrote: > > > Unfortunately, it's not that easy. It's not required by > > > pm_genpd_init() (or of_genpd_add_provider_*) to use a struct *device > > > when registering a genpd provider. In fact, that's also the reason why > > > pm_genpd_init() needs to initialize its own struct device. > > > > Do you see any way to do it properly at pmdomain level instead, or is > > that not feasible? I agree with Saravana that a centralized solution, > > akin to what was implemented for tty devices in commit 1a5ecc73b2bf > > ("serdev: Set fwnode for serdev devices"), would be ideal. However, I > > was not able to find other reports or patches of problems regarding > > fw_devlink at pmdomain level, so it might not be a general problem here, > > and fixes per driver might be okay in this case? > > Sure, we could fix it though a centralized solution, but it requires > us to patch each and every genpd provider driver too. At this point I > would therefore say, let's just fix it for those that need it. > > > > > > That said, can you please explain the special condition that caused > > > this thing to be needed in the first place? > > > > I noticed this issue when I updated from 5.15.93 to 6.1.37. Except for > > the commit I bisected, i.e., 3fb16866b51d ("driver core: fw_devlink: > > Make cycle detection more robust"), I found that the issue was > > introduced somewhere between 6.1.16 and 6.1.15. Here, some commits > > regarding fw_devlink was backported, which might be relevant: > > > > $ git log --oneline v6.1.15..v6.1.16 -- drivers/base/core.c | grep fw_devlink > > 11d93294b7c3 driver core: fw_devlink: Avoid spurious error message > > 3dd596616d10 driver core: fw_devlink: Make cycle detection more robust > > a3c171751512 driver core: fw_devlink: Improve check for fwnode with no device/driver > > 7a8ce4d2fbbc driver core: fw_devlink: Consolidate device link flag computation > > 16aa2487cf15 driver core: fw_devlink: Allow marking a fwnode link as being part of a cycle > > eaf9b5612a47 driver core: fw_devlink: Don't purge child fwnode's consumer links > > 2455b81afe68 driver core: fw_devlink: Add DL_FLAG_CYCLE support to device links > > Thanks for sharing the commits. I haven't looked much closer to them, > but maybe Saravana has some ideas about whether this potentially can > be fixed in the fw_devlink parsing instead? I had a bit closer look at this. If I understand things correctly, this problem is because the imx_gpc_driver's ->probe() (imx_gpc_probe) is allocating/adding a new platform device, per child-of-node to represent a power-domain. I suspect in most other cases where a platform device is being allocated per child node, is when the child node has a compatible-string and the parent driver is calling of_platform_populate(), or similar. In those cases, the "dev.fwnode" would be set accordingly, but not in the imx_gpc_driver case as it needs to be set explicitly. That said, it looks like we should move forward with the $subject patch. However, it looks like the patch [1] from Pengfei Li is more correct than the $subject patch. Kind regards Uffe [1] https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/
On Tue, Oct 24, 2023 at 14:42 +0200, Ulf Hansson wrote: > I had a bit closer look at this. If I understand things correctly, > this problem is because the imx_gpc_driver's ->probe() (imx_gpc_probe) > is allocating/adding a new platform device, per child-of-node to > represent a power-domain. > > I suspect in most other cases where a platform device is being > allocated per child node, is when the child node has a > compatible-string and the parent driver is calling > of_platform_populate(), or similar. In those cases, the "dev.fwnode" > would be set accordingly, but not in the imx_gpc_driver case as it > needs to be set explicitly. > > That said, it looks like we should move forward with the $subject > patch. However, it looks like the patch [1] from Pengfei Li is more > correct than the $subject patch. > > Kind regards > Uffe > > [1] > https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/ That is also how I understand it works. I was unsure if the patch from Pengfei Li [1] solves the same issue as this patch, since there are no errors included in the commit message. However, after testing the patch in [1], I can confirm that it fixes: [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu I based my solution on other patches, where device_set_node() is used. For example, a26cc2934331 ("drm/mipi-dsi: Set the fwnode for mipi_dsi_device") and 6dbe6c07f94f ("gpio: Propagate firmware node from a parent"). I am curious: Why is the approach in [1] considered more correct? It seems logical to set the fwnode as early as possible from the parent its device. Thanks! Best regards, Emil [1] https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/
On Wed, 25 Oct 2023 at 12:12, Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > On Tue, Oct 24, 2023 at 14:42 +0200, Ulf Hansson wrote: > > I had a bit closer look at this. If I understand things correctly, > > this problem is because the imx_gpc_driver's ->probe() (imx_gpc_probe) > > is allocating/adding a new platform device, per child-of-node to > > represent a power-domain. > > > > I suspect in most other cases where a platform device is being > > allocated per child node, is when the child node has a > > compatible-string and the parent driver is calling > > of_platform_populate(), or similar. In those cases, the "dev.fwnode" > > would be set accordingly, but not in the imx_gpc_driver case as it > > needs to be set explicitly. > > > > That said, it looks like we should move forward with the $subject > > patch. However, it looks like the patch [1] from Pengfei Li is more > > correct than the $subject patch. > > > > Kind regards > > Uffe > > > > [1] > > https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/ > > That is also how I understand it works. > > I was unsure if the patch from Pengfei Li [1] solves the same issue as > this patch, since there are no errors included in the commit message. > However, after testing the patch in [1], I can confirm that it fixes: > > [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu > > I based my solution on other patches, where device_set_node() is used. > For example, a26cc2934331 ("drm/mipi-dsi: Set the fwnode for > mipi_dsi_device") and 6dbe6c07f94f ("gpio: Propagate firmware node from > a parent"). I am curious: Why is the approach in [1] considered more > correct? It seems logical to set the fwnode as early as possible from > the parent its device. Why do we want to use the parent's node? The devices (platform-device) that we allocate belongs to the corresponding parsed child node, doesn't it? Kind regards Uffe
On Wed, Oct 25, 2023 at 17:35 +0200, Ulf Hansson wrote: > On Wed, 25 Oct 2023 at 12:12, Emil Kronborg Andersen <emkan@prevas.dk> wrote: > > > > On Tue, Oct 24, 2023 at 14:42 +0200, Ulf Hansson wrote: > > > I had a bit closer look at this. If I understand things correctly, > > > this problem is because the imx_gpc_driver's ->probe() (imx_gpc_probe) > > > is allocating/adding a new platform device, per child-of-node to > > > represent a power-domain. > > > > > > I suspect in most other cases where a platform device is being > > > allocated per child node, is when the child node has a > > > compatible-string and the parent driver is calling > > > of_platform_populate(), or similar. In those cases, the "dev.fwnode" > > > would be set accordingly, but not in the imx_gpc_driver case as it > > > needs to be set explicitly. > > > > > > That said, it looks like we should move forward with the $subject > > > patch. However, it looks like the patch [1] from Pengfei Li is more > > > correct than the $subject patch. > > > > > > Kind regards > > > Uffe > > > > > > [1] > > > https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/ > > > > That is also how I understand it works. > > > > I was unsure if the patch from Pengfei Li [1] solves the same issue as > > this patch, since there are no errors included in the commit message. > > However, after testing the patch in [1], I can confirm that it fixes: > > > > [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu > > > > I based my solution on other patches, where device_set_node() is used. > > For example, a26cc2934331 ("drm/mipi-dsi: Set the fwnode for > > mipi_dsi_device") and 6dbe6c07f94f ("gpio: Propagate firmware node from > > a parent"). I am curious: Why is the approach in [1] considered more > > correct? It seems logical to set the fwnode as early as possible from > > the parent its device. > > Why do we want to use the parent's node? The devices (platform-device) > that we allocate belongs to the corresponding parsed child node, > doesn't it? > > Kind regards > Uffe > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel I think you are right about that. In [1], dev.fwnode is set directly, but I think it is more correct to use device_set_node() instead, to ensure that dev.of_node is also set correctly like so: diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c index 90a8b2c0676f..22d5c29da79e 100644 --- a/drivers/pmdomain/imx/gpc.c +++ b/drivers/pmdomain/imx/gpc.c @@ -497,7 +497,7 @@ static int imx_gpc_probe(struct platform_device *pdev) domain->ipg_rate_mhz = ipg_rate_mhz; pd_pdev->dev.parent = &pdev->dev; - pd_pdev->dev.of_node = np; + device_set_node(&pd_pdev->dev, of_fwnode_handle(np)); ret = platform_device_add(pd_pdev); if (ret) { [1] https://lore.kernel.org/all/20231020185949.537083-1-pengfei.li_1@nxp.com/ Best regards, Emil
diff --git a/drivers/pmdomain/imx/gpc.c b/drivers/pmdomain/imx/gpc.c index 90a8b2c0676f..8759eab72880 100644 --- a/drivers/pmdomain/imx/gpc.c +++ b/drivers/pmdomain/imx/gpc.c @@ -411,6 +411,8 @@ static int imx_gpc_probe(struct platform_device *pdev) void __iomem *base; int ret; + device_set_node(&pdev->dev, dev_fwnode(pdev->dev.parent)); + pgc_node = of_get_child_by_name(pdev->dev.of_node, "pgc"); /* bail out if DT too old and doesn't provide the necessary info */
Before commit 3fb16866b51d ("driver core: fw_devlink: Make cycle detection more robust"), consumer devices without their fwnode set printed errors like the one below: [ 1.039830] imx-gpc 20dc000.gpc: Failed to create device link (0x180) with 20c8000.anatop:regulator-vddpu To fix this, set the fwnode so fw_devlink can find and handle it properly. Signed-off-by: Emil Kronborg Andersen <emkan@prevas.dk> --- drivers/pmdomain/imx/gpc.c | 2 ++ 1 file changed, 2 insertions(+)