diff mbox

[RFC,v2,2/6] ARM: TI: Describe the ti reset DT entries

Message ID 1399320567-3639-3-git-send-email-dmurphy@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Dan Murphy May 5, 2014, 8:09 p.m. UTC
Describe the TI reset DT entries for TI SoC's.

Signed-off-by: Dan Murphy <dmurphy@ti.com>
---
 .../devicetree/bindings/reset/ti,reset.txt         |  103 ++++++++++++++++++++
 1 file changed, 103 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/ti,reset.txt

Comments

Tony Lindgren May 5, 2014, 10:01 p.m. UTC | #1
* Dan Murphy <dmurphy@ti.com> [140505 13:10]:
> +
> +Required parent properties:
> +- compatible : Should be one of,
> +		"ti,omap4-prm" for OMAP4 PRM instances
> +		"ti,omap5-prm" for OMAP5 PRM instances
> +		"ti,dra7-prm" for DRA7xx PRM instances
> +		"ti,am4-prcm" for AM43xx PRCM instances
> +		"ti,am3-prcm" for AM33xx PRCM instances
> +
> +Required child reset property:
> +- compatible : Should be
> +		"resets" for All TI SoCs
> +
> +example:
> +		prm: prm@4ae06000 {
> +			compatible = "ti,omap5-prm";
> +			reg = <0x4ae06000 0x3000>;
> +
> +			prm_resets: resets {
> +				#address-cells = <1>;
> +				#size-cells = <1>;
> +				#reset-cells = <1>;
> +			};
> +		};

The reg entries you have in the example below has different format
compared to this?

> +Reset node declaration
> +==============================================
> +The reset node is declared in a parent child relationship.  The main parent
> +is the PRCM module which contains the base address.  The first child within
> +the reset parent declares the target modules reset name.  This is followed by
> +the control and status offsets.
> +
> +Within the first reset child node is a secondary child node which declares the
> +reset signal of interest.  Under this node the control and status bits
> +are declared.  These bits declare the bit mask for the target reset.
> +
> +
> +Required properties:
> +reg - This is the register offset from the PRCM parent.
> +	This must be declared as:
> +
> +	reg = <control register offset>,
> +		  <status register offset>;
> +
> +control-bit - This is the bit within the register which controls the reset
> +	of the target module.  This is declared as a bit mask for the register.
> +status-bit - This is the bit within the register which contains the status of
> +	the reset of the target module.
> +	This is declared as a bit mask for the register.
> +
> +example:
> +&prm_resets {
> +	dsp_rstctrl {
> +		reg = <0x1c00>,
> +			  <0x1c04>;

Shouldn't this be start and size instead of start and end here?

> +		dsp_reset: dsp_reset {
> +			control-bit = <0x01>;
> +			status-bit = <0x01>;
> +		};
> +	};
> +};

Are the control-bit and status-bit always the same? If so, you can
keep that knowlede private to the the driver.

And maybe you can have the bit offset in a reg property here to
avoid adding any custom properties? Something like:

	dsp_reset: reset@1 {
		reg = 1;
	};

If reg is not suitable for that, it seems that some generic property
to describe the bit offset is needed by quite a few drivers
anyways, for things like clocks and regulators. 

> +Client Node Declaration
> +==============================================
> +This is the consumer of the parent node to declare what resets this
> +particular module is interested in.
> +
> +example:
> +	src: src@55082000 {
> +		    resets = <&reset_src phandle>;
> +			reset-names = "<reset_name>";
> +	};
> +
> +Required Properties:
> +reset_src - This is the parent DT entry for the reset controller
> +phandle - This is the phandle of the specific reset to be used by the clien
> +	driver.
> +reset-names - This is the reset name of module that the device driver
> +	needs to be able to reset.  This value must correspond to a value within
> +	the reset controller array.
> +
> +example:
> +resets = <&prm_resets &dsp_mmu_reset>;
> +reset-names = "dsp_mmu_reset";

This part looks OK to me, not sure if we need the reset-names property
if we have one already why not.

Regards,

Tony
Dan Murphy May 6, 2014, 1:18 p.m. UTC | #2
Tony

Thanks for the comments

On 05/05/2014 05:01 PM, Tony Lindgren wrote:
> * Dan Murphy <dmurphy@ti.com> [140505 13:10]:
>> +
>> +Required parent properties:
>> +- compatible : Should be one of,
>> +		"ti,omap4-prm" for OMAP4 PRM instances
>> +		"ti,omap5-prm" for OMAP5 PRM instances
>> +		"ti,dra7-prm" for DRA7xx PRM instances
>> +		"ti,am4-prcm" for AM43xx PRCM instances
>> +		"ti,am3-prcm" for AM33xx PRCM instances
>> +
>> +Required child reset property:
>> +- compatible : Should be
>> +		"resets" for All TI SoCs
>> +
>> +example:
>> +		prm: prm@4ae06000 {
>> +			compatible = "ti,omap5-prm";
>> +			reg = <0x4ae06000 0x3000>;
>> +
>> +			prm_resets: resets {
>> +				#address-cells = <1>;
>> +				#size-cells = <1>;
>> +				#reset-cells = <1>;
>> +			};
>> +		};
> The reg entries you have in the example below has different format
> compared to this?

This is the parent to the resets.  The reg entries below are for
the child reset signals.

>
>> +Reset node declaration
>> +==============================================
>> +The reset node is declared in a parent child relationship.  The main parent
>> +is the PRCM module which contains the base address.  The first child within
>> +the reset parent declares the target modules reset name.  This is followed by
>> +the control and status offsets.
>> +
>> +Within the first reset child node is a secondary child node which declares the
>> +reset signal of interest.  Under this node the control and status bits
>> +are declared.  These bits declare the bit mask for the target reset.
>> +
>> +
>> +Required properties:
>> +reg - This is the register offset from the PRCM parent.
>> +	This must be declared as:
>> +
>> +	reg = <control register offset>,
>> +		  <status register offset>;
>> +
>> +control-bit - This is the bit within the register which controls the reset
>> +	of the target module.  This is declared as a bit mask for the register.
>> +status-bit - This is the bit within the register which contains the status of
>> +	the reset of the target module.
>> +	This is declared as a bit mask for the register.
>> +
>> +example:
>> +&prm_resets {
>> +	dsp_rstctrl {
>> +		reg = <0x1c00>,
>> +			  <0x1c04>;
> Shouldn't this be start and size instead of start and end here?

I could do start and size.  But the size will be the same for each register.
AM33xx really hurts the consistency model here as the other patches in the series
it appears that the control and status are consistent but AM33xx the registers are all over
the place.

I have not looked at OMAP2->4 yet for control and status register offsets.

>> +		dsp_reset: dsp_reset {
>> +			control-bit = <0x01>;
>> +			status-bit = <0x01>;
>> +		};
>> +	};
>> +};
> Are the control-bit and status-bit always the same? If so, you can
> keep that knowlede private to the the driver.

No there is a case in the am33xx resets file where the status and control bits are not the same.

>
> And maybe you can have the bit offset in a reg property here to
> avoid adding any custom properties? Something like:
>
> 	dsp_reset: reset@1 {
> 		reg = 1;
> 	};
>
> If reg is not suitable for that, it seems that some generic property
> to describe the bit offset is needed by quite a few drivers
> anyways, for things like clocks and regulators. 

I will have to look into this.  Maybe a generic entry called bit-offset
that defines the bit or a mask for the registers.  It can mimic the reg
entry.

>> +Client Node Declaration
>> +==============================================
>> +This is the consumer of the parent node to declare what resets this
>> +particular module is interested in.
>> +
>> +example:
>> +	src: src@55082000 {
>> +		    resets = <&reset_src phandle>;
>> +			reset-names = "<reset_name>";
>> +	};
>> +
>> +Required Properties:
>> +reset_src - This is the parent DT entry for the reset controller
>> +phandle - This is the phandle of the specific reset to be used by the clien
>> +	driver.
>> +reset-names - This is the reset name of module that the device driver
>> +	needs to be able to reset.  This value must correspond to a value within
>> +	the reset controller array.
>> +
>> +example:
>> +resets = <&prm_resets &dsp_mmu_reset>;
>> +reset-names = "dsp_mmu_reset";
> This part looks OK to me, not sure if we need the reset-names property
> if we have one already why not.

reset-names are part of the reset.txt file.  I could probably drop the resets and reset-name information here
and point to the reset.txt file for further explanation.

>
> Regards,
>
> Tony
Suman Anna May 12, 2014, 6:46 p.m. UTC | #3
Dan,

On 05/06/2014 08:18 AM, Murphy, Dan wrote:
> Tony
> 
> Thanks for the comments
> 
> On 05/05/2014 05:01 PM, Tony Lindgren wrote:
>> * Dan Murphy <dmurphy@ti.com> [140505 13:10]:
>>> +
>>> +Required parent properties:
>>> +- compatible : Should be one of,
>>> +		"ti,omap4-prm" for OMAP4 PRM instances
>>> +		"ti,omap5-prm" for OMAP5 PRM instances
>>> +		"ti,dra7-prm" for DRA7xx PRM instances
>>> +		"ti,am4-prcm" for AM43xx PRCM instances
>>> +		"ti,am3-prcm" for AM33xx PRCM instances
>>> +
>>> +Required child reset property:
>>> +- compatible : Should be
>>> +		"resets" for All TI SoCs
>>> +
>>> +example:
>>> +		prm: prm@4ae06000 {
>>> +			compatible = "ti,omap5-prm";
>>> +			reg = <0x4ae06000 0x3000>;
>>> +
>>> +			prm_resets: resets {
>>> +				#address-cells = <1>;
>>> +				#size-cells = <1>;
>>> +				#reset-cells = <1>;
>>> +			};
>>> +		};
>> The reg entries you have in the example below has different format
>> compared to this?

Right, the #size-cells should have been 0, or each reg field updated
with an additional field value of 4 as the registers are all of constant
width - 4 bytes.

> 
> This is the parent to the resets.  The reg entries below are for
> the child reset signals.
> 
>>
>>> +Reset node declaration
>>> +==============================================
>>> +The reset node is declared in a parent child relationship.  The main parent
>>> +is the PRCM module which contains the base address.  The first child within
>>> +the reset parent declares the target modules reset name.  This is followed by
>>> +the control and status offsets.
>>> +
>>> +Within the first reset child node is a secondary child node which declares the
>>> +reset signal of interest.  Under this node the control and status bits
>>> +are declared.  These bits declare the bit mask for the target reset.
>>> +
>>> +
>>> +Required properties:
>>> +reg - This is the register offset from the PRCM parent.
>>> +	This must be declared as:
>>> +
>>> +	reg = <control register offset>,
>>> +		  <status register offset>;
>>> +
>>> +control-bit - This is the bit within the register which controls the reset
>>> +	of the target module.  This is declared as a bit mask for the register.
>>> +status-bit - This is the bit within the register which contains the status of
>>> +	the reset of the target module.
>>> +	This is declared as a bit mask for the register.
>>> +
>>> +example:
>>> +&prm_resets {
>>> +	dsp_rstctrl {
>>> +		reg = <0x1c00>,
>>> +			  <0x1c04>;
>> Shouldn't this be start and size instead of start and end here?

These are two registers - one for control and one for status.

> 
> I could do start and size.  But the size will be the same for each register.
> AM33xx really hurts the consistency model here as the other patches in the series
> it appears that the control and status are consistent but AM33xx the registers are all over
> the place.
> 
> I have not looked at OMAP2->4 yet for control and status register offsets.
> 
>>> +		dsp_reset: dsp_reset {
>>> +			control-bit = <0x01>;
>>> +			status-bit = <0x01>;
>>> +		};
>>> +	};
>>> +};
>> Are the control-bit and status-bit always the same? If so, you can
>> keep that knowlede private to the the driver.
> 
> No there is a case in the am33xx resets file where the status and control bits are not the same.

Do we really need two separate properties to represent this, as these
are essentially the relevant bits in the corresponding reg elements.
I guess this is somewhat same as Tony's suggestion about using the reg
field for these sub-nodes, in which case, you would have to add another
#address-cells as 1 for each of the child reset nodes.

regards
Suman

> 
>>
>> And maybe you can have the bit offset in a reg property here to
>> avoid adding any custom properties? Something like:
>>
>> 	dsp_reset: reset@1 {
>> 		reg = 1;
>> 	};
>>
>> If reg is not suitable for that, it seems that some generic property
>> to describe the bit offset is needed by quite a few drivers
>> anyways, for things like clocks and regulators.
> 
> I will have to look into this.  Maybe a generic entry called bit-offset
> that defines the bit or a mask for the registers.  It can mimic the reg
> entry.
> 
>>> +Client Node Declaration
>>> +==============================================
>>> +This is the consumer of the parent node to declare what resets this
>>> +particular module is interested in.
>>> +
>>> +example:
>>> +	src: src@55082000 {
>>> +		    resets = <&reset_src phandle>;
>>> +			reset-names = "<reset_name>";
>>> +	};
>>> +
>>> +Required Properties:
>>> +reset_src - This is the parent DT entry for the reset controller
>>> +phandle - This is the phandle of the specific reset to be used by the clien
>>> +	driver.
>>> +reset-names - This is the reset name of module that the device driver
>>> +	needs to be able to reset.  This value must correspond to a value within
>>> +	the reset controller array.
>>> +
>>> +example:
>>> +resets = <&prm_resets &dsp_mmu_reset>;
>>> +reset-names = "dsp_mmu_reset";
>> This part looks OK to me, not sure if we need the reset-names property
>> if we have one already why not.
> 
> reset-names are part of the reset.txt file.  I could probably drop the resets and reset-name information here
> and point to the reset.txt file for further explanation.
> 
>>
>> Regards,
>>
>> Tony
> 
>
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/reset/ti,reset.txt b/Documentation/devicetree/bindings/reset/ti,reset.txt
new file mode 100644
index 0000000..9d5c29c
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/ti,reset.txt
@@ -0,0 +1,103 @@ 
+Texas Instruments Reset Controller
+======================================
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+Specifying the reset entries for the IP module
+==============================================
+Parent module:
+This is the module node that contains the reset registers and bits.
+
+example:
+			prcm_resets: resets {
+				compatible = "ti,dra7-resets";
+				#reset-cells = <1>;
+			};
+
+Required parent properties:
+- compatible : Should be one of,
+		"ti,omap4-prm" for OMAP4 PRM instances
+		"ti,omap5-prm" for OMAP5 PRM instances
+		"ti,dra7-prm" for DRA7xx PRM instances
+		"ti,am4-prcm" for AM43xx PRCM instances
+		"ti,am3-prcm" for AM33xx PRCM instances
+
+Required child reset property:
+- compatible : Should be
+		"resets" for All TI SoCs
+
+example:
+		prm: prm@4ae06000 {
+			compatible = "ti,omap5-prm";
+			reg = <0x4ae06000 0x3000>;
+
+			prm_resets: resets {
+				#address-cells = <1>;
+				#size-cells = <1>;
+				#reset-cells = <1>;
+			};
+		};
+
+
+Reset node declaration
+==============================================
+The reset node is declared in a parent child relationship.  The main parent
+is the PRCM module which contains the base address.  The first child within
+the reset parent declares the target modules reset name.  This is followed by
+the control and status offsets.
+
+Within the first reset child node is a secondary child node which declares the
+reset signal of interest.  Under this node the control and status bits
+are declared.  These bits declare the bit mask for the target reset.
+
+
+Required properties:
+reg - This is the register offset from the PRCM parent.
+	This must be declared as:
+
+	reg = <control register offset>,
+		  <status register offset>;
+
+control-bit - This is the bit within the register which controls the reset
+	of the target module.  This is declared as a bit mask for the register.
+status-bit - This is the bit within the register which contains the status of
+	the reset of the target module.
+	This is declared as a bit mask for the register.
+
+example:
+&prm_resets {
+	dsp_rstctrl {
+		reg = <0x1c00>,
+			  <0x1c04>;
+
+		dsp_reset: dsp_reset {
+			control-bit = <0x01>;
+			status-bit = <0x01>;
+		};
+	};
+};
+
+
+
+Client Node Declaration
+==============================================
+This is the consumer of the parent node to declare what resets this
+particular module is interested in.
+
+example:
+	src: src@55082000 {
+		    resets = <&reset_src phandle>;
+			reset-names = "<reset_name>";
+	};
+
+Required Properties:
+reset_src - This is the parent DT entry for the reset controller
+phandle - This is the phandle of the specific reset to be used by the clien
+	driver.
+reset-names - This is the reset name of module that the device driver
+	needs to be able to reset.  This value must correspond to a value within
+	the reset controller array.
+
+example:
+resets = <&prm_resets &dsp_mmu_reset>;
+reset-names = "dsp_mmu_reset";