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

[Tony Lindgren]

* Dan Murphy <dmurphy at 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 at 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