Re: [PATCHv2 02/11] soc: ti: add initial PRM driver with reset control support

[Tero Kristo <t-kristo@ti.com>]

On 30/08/2019 12:02, Philipp Zabel wrote:
> Hi Tero,
> 
> On Fri, 2019-08-30 at 10:28 +0300, Tero Kristo wrote:
>> On 29/08/2019 16:12, Philipp Zabel wrote:
> [...]
>>> I wonder if splitting rstctrl/rstst/rstmap out into a separate structure
>>> would make sense. That could be linked from omap_reset_data directly.
>>> That only makes sense if there'd be enough cases where it can be reused
>>> for multiple PRMs instances.
>>
>> Hmm, splitting these out would make it possible to share the bits for
>> ipu:s across devices, same for dsp:s and eve:s.
>>
>> However, adding too many levels of abstraction makes it kind of
>> difficult to follow what is happening with the code, and it would only
>> save maybe ~100 bytes of memory at the moment.
> 
> Good point, that is likely not worth the additional complexity.
> 
> [...]
>>> What does the value read from the rstst register indicate? Is it the
>>> current state of the reset line? Is it 0 until deassertion is completed,
>>> and then it turns to 1?
>>
>> Value of 1 indicates that the corresponding IP has been reset
>> successfully. Writing back 1 to the same bit clears it out, so the
>> status can be polled later on.
> 
> Then this should not be returned directly by reset_control_status:
> 
> /**
>   * reset_control_status - returns a negative errno if not supported, a
>   * positive value if the reset line is asserted, or zero if the reset
>   * line is not asserted or if the desc is NULL (optional reset).
>   */
> 
> This should return 0 only if the control bit is deasserted and the
> status bit is already set.
> 
> If either the control bit is asserted, or if the status bit is still
> cleared, this should return 1.

Hmm ok, let me fix that a bit also.

> 
> [...]
>>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +	if (!res)
>>>> +		return -ENODEV;
>>>
>>> This can be merged with devm_ioremap_resource below.
>>
>> Well, I actually use the "res" later on to map the DT node to the
>> corresponding prm_data based on address.
> 
> Ok. I glanced over the data->base loop below after questioning whether
> it is needed at all.
> 
>>>
>>>> +	match = of_match_device(omap_prm_id_table, &pdev->dev);
>>>> +	if (!match)
>>>> +		return -ENOTSUPP;
>>>> +
>>>> +	prm = devm_kzalloc(&pdev->dev, sizeof(*prm), GFP_KERNEL);
>>>> +	if (!prm)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	data = match->data;
>>>> +
>>>> +	while (data->base != res->start) {
>>>> +		if (!data->base)
>>>> +			return -EINVAL;
>>>> +		data++;
>>>> +	}
>>>
>>> Is this not something that you want to have encoded in the compatible
>>> string? They all have a different register layout.
>>
>> With the addition of all the prm instances later on, this changes. Most
>> of the prm instances will have same register layout then. See v1 data
>> that was posted earlier [1], but which I dropped for now to keep this
>> series isolated for reset handling only. In this patch, you see that for
>> DRA7, all the power domain handling related PRM instances have identical
>> register layout, they just differ based on base address.
>>
>> [1] https://www.spinics.net/lists/linux-omap/msg149872.html
>>
>> It would be possible to encode all of this based on different
>> compatibles, but then the amount of different compatible strings would
>> explode... DRA7 is just one SoC.
> 
> I see only 3 different layouts in that patch. All instances with
> identical layout could share the same compatible.
> 
> Either way, that is DT bindings territory, and not for me to decide. I
> just found it unusual to encode the device type by its base address in
> the driver.

I'll try to poke Rob for comment on that on the bindings patch.

> 
>>>
>>>> +
>>>> +	prm->data = data;
>>>> +
>>>> +	prm->base = devm_ioremap_resource(&pdev->dev, res);
>>>
>>> 	prm->base = devm_platform_ioremap_resource(pdev, 0);
>>
>> I still need the "res" pointer as indicated above.
> 
> Got it. If the lookup by base address is needed, this has to stay split.

Ok thanks for the reviews, much appreciated. I'll see if I can get reply 
from Rob on the binding item, otherwise I might just send v2 with the 
existing bindings out and send a v3 with updated bindings if necessary.

-Tero
--
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