| Message ID | 1398802790-29287-9-git-send-email-dmurphy@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Arnd
* 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>; ... }; Regards, Tony
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? Dan > Regards, > > Tony
* 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. Regards, Tony
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 <dmurphy@ti.com> [140430 11:14]: > 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? 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? Regards, Tony
Tony On 04/30/2014 05:33 PM, Tony Lindgren wrote: > * Dan Murphy <dmurphy@ti.com> [140430 11:14]: >> 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? > 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 >
diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 07f283c..df9d9c6 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -10,6 +10,7 @@ #include <dt-bindings/gpio/gpio.h> #include <dt-bindings/pinctrl/am33xx.h> +#include <dt-bindings/reset/ti,am33xx-resets.h> #include "skeleton.dtsi" @@ -117,6 +118,11 @@ prcm_clockdomains: clockdomains { }; + + prcm_resets: resets { + compatible = "ti,am335x-resets"; + #reset-cells = <1>; + }; }; scrm: scrm@44e10000 { diff --git a/include/dt-bindings/reset/ti,am33xx-resets.h b/include/dt-bindings/reset/ti,am33xx-resets.h new file mode 100644 index 0000000..dfe7954 --- /dev/null +++ b/include/dt-bindings/reset/ti,am33xx-resets.h @@ -0,0 +1,18 @@ +/* + * 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
Add the prcm_resets node to the prcm parent node. Add the dt-bindings header to the DT file Signed-off-by: Dan Murphy <dmurphy@ti.com> --- arch/arm/boot/dts/am33xx.dtsi | 6 ++++++ include/dt-bindings/reset/ti,am33xx-resets.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 include/dt-bindings/reset/ti,am33xx-resets.h