Re: [PATCH] soc: ti: ti_sci_pm_domains: Store device id in platform device

From: Tero Kristo <t-kristo@ti•com>
To: Lokesh Vutla <lokeshvutla@ti•com>, Nishanth Menon <nm@ti•com>,
	Santosh Shilimkar <ssantosh@kernel•org>
Cc: Sekhar Nori <nsekhar@ti•com>,
	Linux ARM Mailing List <linux-arm-kernel@lists•infradead.org>
Subject: Re: [PATCH] soc: ti: ti_sci_pm_domains: Store device id in platform device
Date: Thu, 26 Sep 2019 09:20:39 +0300	[thread overview]
Message-ID: <88e0cb92-58c8-dabf-be0f-33851f2ad38c@ti.com> (raw)
In-Reply-To: <42e35997-24d5-22d6-28f6-63e4aeac230d@ti.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists•infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel