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

From: Tony Lindgren <tony@atomide.com>
To: Dan Murphy <dmurphy@ti.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	linux-omap@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	devicetree@vger.kernel.org, t-kristo@ti.com, s-anna@ti.com,
	p.zabel@pengutronix.de
Subject: Re: [RFC 08/11] ARM: dts: am33xx: Add prcm_resets node
Date: Wed, 30 Apr 2014 15:33:36 -0700	[thread overview]
Message-ID: <20140430223335.GB21341@atomide.com> (raw)
In-Reply-To: <53613D47.4000905@ti.com>

* 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