| Message ID | 20190923033439.20070-1-lokeshvutla@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: ti: ti_sci_pm_domains: Store device id in platform device | expand |
On 23/09/2019 06:34, Lokesh Vutla wrote: > Device ID that is passed from power-domains is used by peripheral > drivers for communicating with sysfw. Instead of individual drivers > traversing power-domains entry in DT node, store the device ID in > platform_device so that drivers can directly access it. Uhm, isn't this kind of wrong place to allocate the ID? The power domain solution itself is a client also. In theory, someone could access the pdev->id before this. pdev->id should be assigned by bus driver so that it can be properly handled within platform_device_add. -Tero > > Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> > --- > drivers/soc/ti/ti_sci_pm_domains.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/soc/ti/ti_sci_pm_domains.c b/drivers/soc/ti/ti_sci_pm_domains.c > index 8c2a2f23982c..a124ac409124 100644 > --- a/drivers/soc/ti/ti_sci_pm_domains.c > +++ b/drivers/soc/ti/ti_sci_pm_domains.c > @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, > struct of_phandle_args pd_args; > struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain); > const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; > + struct platform_device *pdev = to_platform_device(dev); > struct ti_sci_genpd_dev_data *sci_dev_data; > struct generic_pm_domain_data *genpd_data; > int idx, ret = 0; > @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, > return -EINVAL; > > idx = pd_args.args[0]; > + pdev->id = idx; > > /* > * Check the validity of the requested idx, if the index is not valid > -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 23/09/19 12:07 PM, Tero Kristo wrote: > On 23/09/2019 06:34, Lokesh Vutla wrote: >> Device ID that is passed from power-domains is used by peripheral >> drivers for communicating with sysfw. Instead of individual drivers >> traversing power-domains entry in DT node, store the device ID in >> platform_device so that drivers can directly access it. > > Uhm, isn't this kind of wrong place to allocate the ID? The power domain I do agree that this might not be a right place, but I couldn't find a better place to populate id. Below is the flow on how platform_device gets created. of_platform_default_populate_init ->of_platform_default_populate -> of_platform_populate ->of_platform_bus_create -> of_platform_device_create_pdata -> of_device_alloc -> platform_device_alloc("", PLATFORM_DEVID_NONE); At this point platform_device gets created with dummy device_id. Also there are no hooks to add custom device_ids. > solution itself is a client also. In theory, someone could access the pdev->id Nope, this is done in dev_pm_domain_attach which is called before driver probe in platform_drv_probe(). > before this. pdev->id should be assigned by bus driver so that it can be > properly handled within platform_device_add. DT doesn't provide any such facility for populating device_add. I am open for any suggestions :) Thanks and regards, Lokesh > > -Tero > >> >> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >> --- >> drivers/soc/ti/ti_sci_pm_domains.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/soc/ti/ti_sci_pm_domains.c >> b/drivers/soc/ti/ti_sci_pm_domains.c >> index 8c2a2f23982c..a124ac409124 100644 >> --- a/drivers/soc/ti/ti_sci_pm_domains.c >> +++ b/drivers/soc/ti/ti_sci_pm_domains.c >> @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >> *domain, >> struct of_phandle_args pd_args; >> struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain); >> const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; >> + struct platform_device *pdev = to_platform_device(dev); >> struct ti_sci_genpd_dev_data *sci_dev_data; >> struct generic_pm_domain_data *genpd_data; >> int idx, ret = 0; >> @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >> *domain, >> return -EINVAL; >> idx = pd_args.args[0]; >> + pdev->id = idx; >> /* >> * Check the validity of the requested idx, if the index is not valid >> > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Tero, On 24/09/19 10:15 AM, Lokesh Vutla wrote: > Hi Tero, > > On 23/09/19 12:07 PM, Tero Kristo wrote: >> On 23/09/2019 06:34, Lokesh Vutla wrote: >>> Device ID that is passed from power-domains is used by peripheral >>> drivers for communicating with sysfw. Instead of individual drivers >>> traversing power-domains entry in DT node, store the device ID in >>> platform_device so that drivers can directly access it. >> >> Uhm, isn't this kind of wrong place to allocate the ID? The power domain > > I do agree that this might not be a right place, but I couldn't find a better > place to populate id. Below is the flow on how platform_device gets created. > of_platform_default_populate_init > ->of_platform_default_populate > -> of_platform_populate > ->of_platform_bus_create > -> of_platform_device_create_pdata > -> of_device_alloc > -> platform_device_alloc("", PLATFORM_DEVID_NONE); > > At this point platform_device gets created with dummy device_id. Also there are > no hooks to add custom device_ids. > >> solution itself is a client also. In theory, someone could access the pdev->id > > Nope, this is done in dev_pm_domain_attach which is called before driver probe > in platform_drv_probe(). If there are no objections, can this patch be picked? Thanks and regards, Lokesh > >> before this. pdev->id should be assigned by bus driver so that it can be >> properly handled within platform_device_add. > > DT doesn't provide any such facility for populating device_add. I am open for > any suggestions :) > > Thanks and regards, > Lokesh > >> >> -Tero >> >>> >>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >>> --- >>> drivers/soc/ti/ti_sci_pm_domains.c | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/drivers/soc/ti/ti_sci_pm_domains.c >>> b/drivers/soc/ti/ti_sci_pm_domains.c >>> index 8c2a2f23982c..a124ac409124 100644 >>> --- a/drivers/soc/ti/ti_sci_pm_domains.c >>> +++ b/drivers/soc/ti/ti_sci_pm_domains.c >>> @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >>> *domain, >>> struct of_phandle_args pd_args; >>> struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain); >>> const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; >>> + struct platform_device *pdev = to_platform_device(dev); >>> struct ti_sci_genpd_dev_data *sci_dev_data; >>> struct generic_pm_domain_data *genpd_data; >>> int idx, ret = 0; >>> @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >>> *domain, >>> return -EINVAL; >>> idx = pd_args.args[0]; >>> + pdev->id = idx; >>> /* >>> * Check the validity of the requested idx, if the index is not valid >>> >> >> -- >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. >> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
On 26/09/2019 06:33, Lokesh Vutla wrote: > Hi Tero, > > On 24/09/19 10:15 AM, Lokesh Vutla wrote: >> Hi Tero, >> >> On 23/09/19 12:07 PM, Tero Kristo wrote: >>> On 23/09/2019 06:34, Lokesh Vutla wrote: >>>> Device ID that is passed from power-domains is used by peripheral >>>> drivers for communicating with sysfw. Instead of individual drivers >>>> traversing power-domains entry in DT node, store the device ID in >>>> platform_device so that drivers can directly access it. >>> >>> Uhm, isn't this kind of wrong place to allocate the ID? The power domain >> >> I do agree that this might not be a right place, but I couldn't find a better >> place to populate id. Below is the flow on how platform_device gets created. >> of_platform_default_populate_init >> ->of_platform_default_populate >> -> of_platform_populate >> ->of_platform_bus_create >> -> of_platform_device_create_pdata >> -> of_device_alloc >> -> platform_device_alloc("", PLATFORM_DEVID_NONE); >> >> At this point platform_device gets created with dummy device_id. Also there are >> no hooks to add custom device_ids. >> >>> solution itself is a client also. In theory, someone could access the pdev->id >> >> Nope, this is done in dev_pm_domain_attach which is called before driver probe >> in platform_drv_probe(). > > If there are no objections, can this patch be picked? I don't think this patch is still quite right, as it is clearly a hack (you modify a platform device field outside the chain that actually allocates it and uses it for some purpose.) The issue I see here is really easy potential breakage if the pdev->id is changed to be used something in the OF platform device chain. This hack would then break as it is completely TI K3 specific, and if any drivers depend on it, they would break also. So, IMHO it is a NAK for this patch from my side. It is too hackish, and there is a way to handle this from drivers currently (yes, the alternative is bit more painful but it is certain to work going forward also.) -Tero > > Thanks and regards, > Lokesh > >> >>> before this. pdev->id should be assigned by bus driver so that it can be >>> properly handled within platform_device_add. >> >> DT doesn't provide any such facility for populating device_add. I am open for >> any suggestions :) >> >> Thanks and regards, >> Lokesh >> >>> >>> -Tero >>> >>>> >>>> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> >>>> --- >>>> drivers/soc/ti/ti_sci_pm_domains.c | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/drivers/soc/ti/ti_sci_pm_domains.c >>>> b/drivers/soc/ti/ti_sci_pm_domains.c >>>> index 8c2a2f23982c..a124ac409124 100644 >>>> --- a/drivers/soc/ti/ti_sci_pm_domains.c >>>> +++ b/drivers/soc/ti/ti_sci_pm_domains.c >>>> @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >>>> *domain, >>>> struct of_phandle_args pd_args; >>>> struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain); >>>> const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; >>>> + struct platform_device *pdev = to_platform_device(dev); >>>> struct ti_sci_genpd_dev_data *sci_dev_data; >>>> struct generic_pm_domain_data *genpd_data; >>>> int idx, ret = 0; >>>> @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain >>>> *domain, >>>> return -EINVAL; >>>> idx = pd_args.args[0]; >>>> + pdev->id = idx; >>>> /* >>>> * Check the validity of the requested idx, if the index is not valid >>>> >>> >>> -- >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
diff --git a/drivers/soc/ti/ti_sci_pm_domains.c b/drivers/soc/ti/ti_sci_pm_domains.c index 8c2a2f23982c..a124ac409124 100644 --- a/drivers/soc/ti/ti_sci_pm_domains.c +++ b/drivers/soc/ti/ti_sci_pm_domains.c @@ -116,6 +116,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, struct of_phandle_args pd_args; struct ti_sci_pm_domain *ti_sci_genpd = genpd_to_ti_sci_pd(domain); const struct ti_sci_handle *ti_sci = ti_sci_genpd->ti_sci; + struct platform_device *pdev = to_platform_device(dev); struct ti_sci_genpd_dev_data *sci_dev_data; struct generic_pm_domain_data *genpd_data; int idx, ret = 0; @@ -129,6 +130,7 @@ static int ti_sci_pd_attach_dev(struct generic_pm_domain *domain, return -EINVAL; idx = pd_args.args[0]; + pdev->id = idx; /* * Check the validity of the requested idx, if the index is not valid
Device ID that is passed from power-domains is used by peripheral drivers for communicating with sysfw. Instead of individual drivers traversing power-domains entry in DT node, store the device ID in platform_device so that drivers can directly access it. Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com> --- drivers/soc/ti/ti_sci_pm_domains.c | 2 ++ 1 file changed, 2 insertions(+)