Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node

[Dan Murphy <dmurphy@ti.com>]

Tony

On 04/30/2014 01:10 PM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [140430 11:00]:
>> Tony and Arnd
>>
>> Thanks for the comments
>>
>> On 04/29/2014 07:22 PM, Tony Lindgren wrote:
>>> * Arnd Bergmann <arnd@arndb.de> [140429 13:35]:
>>>> On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:
>>>>> + * AM33xx reset index for PRCM Module
>>>>> + *
>>>>> + * Copyright 2014 Texas Instruments Inc.
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or modify
>>>>> + * it under the terms of the GNU General Public License version 2 as
>>>>> + * published by the Free Software Foundation.
>>>>> + */
>>>>> +
>>>>> +#ifndef _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>> +#define _DT_BINDINGS_RESET_TI_AM33XX_H
>>>>> +
>>>>> +#define RESET_DEVICE_RESET                     0
>>>>> +#define RESET_GFX_RESET                                1
>>>>> +#define RESET_PER_RESET                                2
>>>>> +
>>>>> +#endif
>>>> Interfaces like this should only be used if you can't use hardware
>>>> numbers, in general. If these numbers are in the data sheet, just
>>>> put them directly into the dts file, as we do for interrupt numbers,
>>>> gpio numbers, register offsets etc.
>>>>
>>>> If you have made them up to define an interface between the driver
>>>> and DT because there is no usable hardare ID, I'd suggest just using
>>>> a single file across all SoCs that have this driver, and have
>>>> a unified name space.
>>> Also, it's a bit unclear how the reset controller phandle is used
>>> referenced and used by the consumer device.. Maybe setting that up
>>> first in a Linux generic way is a good point starting point.
>>>
>>> Maybe something like this along the same way as clocks are set up
>>> (completely untested):
>>>
>>> &reset1 {
>>> 	iva_reset: reset1 {
>>> 		reg = /bits/ 8 <0>;
>>> 	};
>>> 	gfx_reset: reset1 {
>>> 		reg = /bits/ 8 <1>;
>>> 	};
>>> 	...
>>> };
>>>
>>> &iva {
>>> 	compatible = "ti,ivahd";
>>> 	resets = <&reset1 1>;
>>> 	...
>>> };
>> I had something very similar to this when I was developing this driver but moved away from this.
>>
>> Following the clocks implementation I had a separate dtsi for resets for each device and had the data defined like so
>> for each SoC.
>>
>> &prcm_resets {
>>        device_reset: device_reset {
>>                rstctrl_offs = <0x1104>;
>>                ctrl_bit-shift = <0>;
>>                rstst_offs      = <0x1114>;
>>                sts_bit-shift   = <0>;
>>        };
>>
>>        gpu_reset: gpu_reset {
>>                rstctrl_offs = <0x0D00>;
>>                ctrl_bit-shift = <3>;
>>                rstst_offs      = <0x0D0C>;
>>                sts_bit-shift = <5>;
>>        };
>> };
>>
>> And then any client interested in a specific reset driver would include this
>>
>> resets = <&prcm_resets &gpu_reset>;
>> reset-names = "gpu_reset";
>>
>> Our reset code would then retrieve the register data through the phandle instead of an index.
>>
>> Thoughts?
> Using phandles makes sense here because it avoids the indexing. Indexing
> has a problem of needing to be in sync with the .dts files and the
> device driver(s). Using an index also easily causes misuse of virtual
> indexes being added that no longer describe the hardware at all.

Thanks.  What about placing register data in the dts files?  Is there any issue with this concept?

Dan

> Regards,
>
> Tony


-- 
------------------
Dan Murphy