Tony On 04/30/2014 05:33 PM, Tony Lindgren wrote: > * Dan Murphy <dmurphy@xxxxxx> [140430 11:14]: >> On 04/30/2014 01:10 PM, Tony Lindgren wrote: >>> * Dan Murphy <dmurphy@xxxxxx> [140430 11:00]: >>>> Tony and Arnd >>>> >>>> Thanks for the comments >>>> >>>> On 04/29/2014 07:22 PM, Tony Lindgren wrote: >>>>> * Arnd Bergmann <arnd@xxxxxxxx> [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? > I don't have issues with that but others may. In this case it seems > like you should get away just defining few different types of reset > controllers without adding any any custom properties? > > In your example above, the rstctrl_offs should be just standard > reg entry as an offset from the prcm_resets base address. Then you > you only really need the bit shift and the type of the reset > controller? And the type could be the compatible flag? Well the only issue I have with declaring the reg with the standard dt entry is I have two register offsets here. I will have to see if I can do this per the standard. I may have to have children called control and status and define the reg and bits that way. Dan > Regards, > > Tony > -- ------------------ Dan Murphy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html