Tony

On 04/30/2014 05:33 PM, Tony Lindgren wrote:

* Dan Murphy [off-list ref] [140430 11:14]:

quoted

On 04/30/2014 01:10 PM, Tony Lindgren wrote:

quoted

* Dan Murphy [off-list ref] [140430 11:00]:

quoted

Tony and Arnd

Thanks for the comments

On 04/29/2014 07:22 PM, Tony Lindgren wrote:

quoted

* Arnd Bergmann [off-list ref] [140429 13:35]:

quoted

On Tuesday 29 April 2014 15:19:47 Dan Murphy wrote:

quoted

+ * 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