[PATCH 0/5] ARM: am335x/am437x: Correct pwm bindings

[PATCH 0/5] ARM: am335x/am437x: Correct pwm bindings

[Franklin S Cooper Jr]

Unlike the majority of other SOCs the PWM node uses ehrpwm instead of
the generic pwm node name. This patch series switches to the pwm node
name and while at it fix some other minor binding documentation issues.

This patch series also includes a patch to insure ABI compatibility.

This series was tested on AM335x and AM437x GP evm. I also validated that
ABI compatibility was maintained.

Franklin S Cooper Jr (5):
  clk: ti: am335x/am4372: Add tbclk to pwm node
  ARM: DTS: da850/am4372/am33xx: Use generic node name for ehrpwm
  ARM: DTS: am33xx: Set pwmss ranges property to an empty value
  pwm: pwm-tipwmss: Update documentation to use empty range property
  pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

 Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt |  4 ++--
 Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt  | 12 ++++--------
 arch/arm/boot/dts/am33xx.dtsi                          | 18 ++++++------------
 arch/arm/boot/dts/am4372.dtsi                          | 12 ++++++------
 arch/arm/boot/dts/da850.dtsi                           |  4 ++--
 drivers/clk/ti/clk-33xx.c                              |  3 +++
 drivers/clk/ti/clk-43xx.c                              |  6 ++++++
 7 files changed, 29 insertions(+), 30 deletions(-)

-- 
2.7.0

[PATCH 1/5] clk: ti: am335x/am4372: Add tbclk to pwm node

[Franklin S Cooper Jr]

Add tblck to the pwm nodes. This insures that the ehrpwm driver has access
to the time-based clk.

Do not remove similar entries for ehrpwm node. Later patches will switch
from using ehrpwm node name to pwm. But to maintain ABI compatibility we
shouldn't remove the old entries.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 drivers/clk/ti/clk-33xx.c | 3 +++
 drivers/clk/ti/clk-43xx.c | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/clk/ti/clk-33xx.c b/drivers/clk/ti/clk-33xx.c
index ef2ec64..0e47d95 100644
--- a/drivers/clk/ti/clk-33xx.c
+++ b/drivers/clk/ti/clk-33xx.c
@@ -108,6 +108,9 @@ static struct ti_dt_clk am33xx_clks[] = {
 	DT_CLK("48300200.ehrpwm", "tbclk", "ehrpwm0_tbclk"),
 	DT_CLK("48302200.ehrpwm", "tbclk", "ehrpwm1_tbclk"),
 	DT_CLK("48304200.ehrpwm", "tbclk", "ehrpwm2_tbclk"),
+	DT_CLK("48300200.pwm", "tbclk", "ehrpwm0_tbclk"),
+	DT_CLK("48302200.pwm", "tbclk", "ehrpwm1_tbclk"),
+	DT_CLK("48304200.pwm", "tbclk", "ehrpwm2_tbclk"),
 	{ .node_name = NULL },
 };
 
diff --git a/drivers/clk/ti/clk-43xx.c b/drivers/clk/ti/clk-43xx.c
index 097fc90..7255aa8 100644
--- a/drivers/clk/ti/clk-43xx.c
+++ b/drivers/clk/ti/clk-43xx.c
@@ -115,6 +115,12 @@ static struct ti_dt_clk am43xx_clks[] = {
 	DT_CLK("48306200.ehrpwm", "tbclk", "ehrpwm3_tbclk"),
 	DT_CLK("48308200.ehrpwm", "tbclk", "ehrpwm4_tbclk"),
 	DT_CLK("4830a200.ehrpwm", "tbclk", "ehrpwm5_tbclk"),
+	DT_CLK("48300200.pwm", "tbclk", "ehrpwm0_tbclk"),
+	DT_CLK("48302200.pwm", "tbclk", "ehrpwm1_tbclk"),
+	DT_CLK("48304200.pwm", "tbclk", "ehrpwm2_tbclk"),
+	DT_CLK("48306200.pwm", "tbclk", "ehrpwm3_tbclk"),
+	DT_CLK("48308200.pwm", "tbclk", "ehrpwm4_tbclk"),
+	DT_CLK("4830a200.pwm", "tbclk", "ehrpwm5_tbclk"),
 	{ .node_name = NULL },
 };
 
-- 
2.7.0

Re: [PATCH 1/5] clk: ti: am335x/am4372: Add tbclk to pwm node

[Stephen Boyd]

On 03/07, Franklin S Cooper Jr wrote:
> Add tblck to the pwm nodes. This insures that the ehrpwm driver has access
> to the time-based clk.
> 
> Do not remove similar entries for ehrpwm node. Later patches will switch
> from using ehrpwm node name to pwm. But to maintain ABI compatibility we
> shouldn't remove the old entries.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---

Acked-by: Stephen Boyd <sboyd@codeaurora.org>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/5] ARM: DTS: da850/am4372/am33xx: Use generic node name for ehrpwm

[Franklin S Cooper Jr]

When possible generic node names should be used. So change the node name
from ehrpwm to pwm.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi |  6 +++---
 arch/arm/boot/dts/am4372.dtsi | 12 ++++++------
 arch/arm/boot/dts/da850.dtsi  |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 1fafaad..4016254 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -688,7 +688,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm0: ehrpwm@48300200 {
+			ehrpwm0: pwm@48300200 {
 				compatible = "ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48300200 0x80>;
@@ -718,7 +718,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm1: ehrpwm@48302200 {
+			ehrpwm1: pwm@48302200 {
 				compatible = "ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48302200 0x80>;
@@ -748,7 +748,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm2: ehrpwm@48304200 {
+			ehrpwm2: pwm@48304200 {
 				compatible = "ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48304200 0x80>;
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index 92068fb..33f417c 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -679,7 +679,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm0: ehrpwm@48300200 {
+			ehrpwm0: pwm@48300200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48300200 0x80>;
@@ -705,7 +705,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm1: ehrpwm@48302200 {
+			ehrpwm1: pwm@48302200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48302200 0x80>;
@@ -731,7 +731,7 @@
 				status = "disabled";
 			};
 
-			ehrpwm2: ehrpwm@48304200 {
+			ehrpwm2: pwm@48304200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48304200 0x80>;
@@ -749,7 +749,7 @@
 			ti,hwmods = "epwmss3";
 			status = "disabled";
 
-			ehrpwm3: ehrpwm@48306200 {
+			ehrpwm3: pwm@48306200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48306200 0x80>;
@@ -767,7 +767,7 @@
 			ti,hwmods = "epwmss4";
 			status = "disabled";
 
-			ehrpwm4: ehrpwm@48308200 {
+			ehrpwm4: pwm@48308200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x48308200 0x80>;
@@ -785,7 +785,7 @@
 			ti,hwmods = "epwmss5";
 			status = "disabled";
 
-			ehrpwm5: ehrpwm@4830a200 {
+			ehrpwm5: pwm@4830a200 {
 				compatible = "ti,am4372-ehrpwm","ti,am33xx-ehrpwm";
 				#pwm-cells = <3>;
 				reg = <0x4830a200 0x80>;
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 226cda7..c3910e2 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -247,13 +247,13 @@
 			dma-names = "rx", "tx";
 			status = "disabled";
 		};
-		ehrpwm0: ehrpwm@01f00000 {
+		ehrpwm0: pwm@01f00000 {
 			compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
 			#pwm-cells = <3>;
 			reg = <0x300000 0x2000>;
 			status = "disabled";
 		};
-		ehrpwm1: ehrpwm@01f02000 {
+		ehrpwm1: pwm@01f02000 {
 			compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
 			#pwm-cells = <3>;
 			reg = <0x302000 0x2000>;
-- 
2.7.0

[PATCH 3/5] ARM: DTS: am33xx: Set pwmss ranges property to an empty value

[Franklin S Cooper Jr]

The eCAP and ePWM are in the same address space as the PWMSS. Therefore,
no address translation is needed which means the ranges property should be
empty.

This mimics how the PWMSS nodes look in the AM4372.dtsi which is similar
to AM335x.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4016254..82c0976 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -674,9 +674,7 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			status = "disabled";
-			ranges = <0x48300100 0x48300100 0x80   /* ECAP */
-				  0x48300180 0x48300180 0x80   /* EQEP */
-				  0x48300200 0x48300200 0x80>; /* EHRPWM */
+			ranges;
 
 			ecap0: ecap@48300100 {
 				compatible = "ti,am33xx-ecap";
@@ -704,9 +702,7 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			status = "disabled";
-			ranges = <0x48302100 0x48302100 0x80   /* ECAP */
-				  0x48302180 0x48302180 0x80   /* EQEP */
-				  0x48302200 0x48302200 0x80>; /* EHRPWM */
+			ranges;
 
 			ecap1: ecap@48302100 {
 				compatible = "ti,am33xx-ecap";
@@ -734,9 +730,7 @@
 			#address-cells = <1>;
 			#size-cells = <1>;
 			status = "disabled";
-			ranges = <0x48304100 0x48304100 0x80   /* ECAP */
-				  0x48304180 0x48304180 0x80   /* EQEP */
-				  0x48304200 0x48304200 0x80>; /* EHRPWM */
+			ranges;
 
 			ecap2: ecap@48304100 {
 				compatible = "ti,am33xx-ecap";
-- 
2.7.0

[PATCH 4/5] pwm: pwm-tipwmss: Update documentation to use empty range property

[Franklin S Cooper Jr]

Since the PWMSS and its subdevices (eCAP and ePWM) use the same address
space then the range property should be empty. Update the documentation
to show the correct usage.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
index f7eae77..672fa71 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
@@ -7,11 +7,9 @@ Required properties:
 		  Should set to 1.
 - size-cells: specify number of u32 entries needed to specify child nodes size
 		in reg property. Should set to 1.
-- ranges: describes the address mapping of a memory-mapped bus. Should set to
-	   physical address map of child's base address, physical address within
-	   parent's address  space and length of the address map. For am33xx,
-	   3 set of child register maps present, ECAP register space, EQEP
-	   register space, EHRPWM register space.
+- ranges: describes the address mapping of a memory-mapped bus. Its value
+	  should be empty since no address translation is needed between the
+	  parent and the child.
 
 Also child nodes should also populated under PWMSS DT node.
 
@@ -23,9 +21,7 @@ pwmss0: pwmss@48300000 {
 	#address-cells = <1>;
 	#size-cells = <1>;
 	status = "disabled";
-	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
-		  0x48300180 0x48300180 0x80   /* EQEP */
-		  0x48300200 0x48300200 0x80>; /* EHRPWM */
+	ranges;
 
 	/* child nodes go here */
 };
-- 
2.7.0

Re: [PATCH 4/5] pwm: pwm-tipwmss: Update documentation to use empty range property

[Rob Herring]

On Mon, Mar 07, 2016 at 01:51:57PM -0600, Franklin S Cooper Jr wrote:
> Since the PWMSS and its subdevices (eCAP and ePWM) use the same address
> space then the range property should be empty. Update the documentation
> to show the correct usage.

Why does it matter? An empty ranges is generally not preferred.

> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
> index f7eae77..672fa71 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
> @@ -7,11 +7,9 @@ Required properties:
>  		  Should set to 1.
>  - size-cells: specify number of u32 entries needed to specify child nodes size
>  		in reg property. Should set to 1.
> -- ranges: describes the address mapping of a memory-mapped bus. Should set to
> -	   physical address map of child's base address, physical address within
> -	   parent's address  space and length of the address map. For am33xx,
> -	   3 set of child register maps present, ECAP register space, EQEP
> -	   register space, EHRPWM register space.
> +- ranges: describes the address mapping of a memory-mapped bus. Its value
> +	  should be empty since no address translation is needed between the
> +	  parent and the child.
>  
>  Also child nodes should also populated under PWMSS DT node.
>  
> @@ -23,9 +21,7 @@ pwmss0: pwmss@48300000 {
>  	#address-cells = <1>;
>  	#size-cells = <1>;
>  	status = "disabled";
> -	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
> -		  0x48300180 0x48300180 0x80   /* EQEP */
> -		  0x48300200 0x48300200 0x80>; /* EHRPWM */
> +	ranges;
>  
>  	/* child nodes go here */
>  };
> -- 
> 2.7.0
> 

Re: [PATCH 4/5] pwm: pwm-tipwmss: Update documentation to use empty range property

[Franklin S Cooper Jr.]

On 03/17/2016 10:01 AM, Rob Herring wrote:
> On Mon, Mar 07, 2016 at 01:51:57PM -0600, Franklin S Cooper Jr wrote:
>> Since the PWMSS and its subdevices (eCAP and ePWM) use the same address
>> space then the range property should be empty. Update the documentation
>> to show the correct usage.
> Why does it matter? An empty ranges is generally not preferred.

Someone pointed out that ranges should probably be empty. I
double checked it with what is in the ePAPR doc and based on
the definition it should be set to empty. I also checked
against the am33xx.dtsi and saw that both USB and ethernet
uses an empty value for ranges.

Can you elaborate on why this isn't preferable?

>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt | 12 ++++--------
>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>> index f7eae77..672fa71 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>> @@ -7,11 +7,9 @@ Required properties:
>>  		  Should set to 1.
>>  - size-cells: specify number of u32 entries needed to specify child nodes size
>>  		in reg property. Should set to 1.
>> -- ranges: describes the address mapping of a memory-mapped bus. Should set to
>> -	   physical address map of child's base address, physical address within
>> -	   parent's address  space and length of the address map. For am33xx,
>> -	   3 set of child register maps present, ECAP register space, EQEP
>> -	   register space, EHRPWM register space.
>> +- ranges: describes the address mapping of a memory-mapped bus. Its value
>> +	  should be empty since no address translation is needed between the
>> +	  parent and the child.
>>  
>>  Also child nodes should also populated under PWMSS DT node.
>>  
>> @@ -23,9 +21,7 @@ pwmss0: pwmss@48300000 {
>>  	#address-cells = <1>;
>>  	#size-cells = <1>;
>>  	status = "disabled";
>> -	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
>> -		  0x48300180 0x48300180 0x80   /* EQEP */
>> -		  0x48300200 0x48300200 0x80>; /* EHRPWM */
>> +	ranges;
>>  
>>  	/* child nodes go here */
>>  };
>> -- 
>> 2.7.0
>>

Re: [PATCH 4/5] pwm: pwm-tipwmss: Update documentation to use empty range property

[Franklin S Cooper Jr.]

On 03/17/2016 11:56 AM, Franklin S Cooper Jr. wrote:
>
> On 03/17/2016 10:01 AM, Rob Herring wrote:
>> On Mon, Mar 07, 2016 at 01:51:57PM -0600, Franklin S Cooper Jr wrote:
>>> Since the PWMSS and its subdevices (eCAP and ePWM) use the same address
>>> space then the range property should be empty. Update the documentation
>>> to show the correct usage.
>> Why does it matter? An empty ranges is generally not preferred.
> Someone pointed out that ranges should probably be empty. I
> double checked it with what is in the ePAPR doc and based on
> the definition it should be set to empty. I also checked
> against the am33xx.dtsi and saw that both USB and ethernet
> uses an empty value for ranges.
>
> Can you elaborate on why this isn't preferable?

I'm going to drop this patch and the previous patch for now
that messes with the ranges property. I can resubmit them
separately later on if that is the route we decide to take.
>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt | 12 ++++--------
>>>  1 file changed, 4 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>>> index f7eae77..672fa71 100644
>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tipwmss.txt
>>> @@ -7,11 +7,9 @@ Required properties:
>>>  		  Should set to 1.
>>>  - size-cells: specify number of u32 entries needed to specify child nodes size
>>>  		in reg property. Should set to 1.
>>> -- ranges: describes the address mapping of a memory-mapped bus. Should set to
>>> -	   physical address map of child's base address, physical address within
>>> -	   parent's address  space and length of the address map. For am33xx,
>>> -	   3 set of child register maps present, ECAP register space, EQEP
>>> -	   register space, EHRPWM register space.
>>> +- ranges: describes the address mapping of a memory-mapped bus. Its value
>>> +	  should be empty since no address translation is needed between the
>>> +	  parent and the child.
>>>  
>>>  Also child nodes should also populated under PWMSS DT node.
>>>  
>>> @@ -23,9 +21,7 @@ pwmss0: pwmss@48300000 {
>>>  	#address-cells = <1>;
>>>  	#size-cells = <1>;
>>>  	status = "disabled";
>>> -	ranges = <0x48300100 0x48300100 0x80   /* ECAP */
>>> -		  0x48300180 0x48300180 0x80   /* EQEP */
>>> -		  0x48300200 0x48300200 0x80>; /* EHRPWM */
>>> +	ranges;
>>>  
>>>  	/* child nodes go here */
>>>  };
>>> -- 
>>> 2.7.0
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Franklin S Cooper Jr]

Now that the node name has been changed from ehrpwm to pwm the document
should show this proper usage. Also change the unit address in the example
from 0 to the proper physical address value that should be used.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
 Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
index 9c100b2..20211ed 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
@@ -15,14 +15,14 @@ Optional properties:
 
 Example:
 
-ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
+ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
 	compatible = "ti,am33xx-ehrpwm";
 	#pwm-cells = <3>;
 	reg = <0x48300200 0x100>;
 	ti,hwmods = "ehrpwm0";
 };
 
-ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
+ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
 	compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
 	#pwm-cells = <3>;
 	reg = <0x300000 0x2000>;
-- 
2.7.0

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Rob Herring]

On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
> Now that the node name has been changed from ehrpwm to pwm the document
> should show this proper usage. Also change the unit address in the example
> from 0 to the proper physical address value that should be used.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> index 9c100b2..20211ed 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
> @@ -15,14 +15,14 @@ Optional properties:
>  
>  Example:
>  
> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>  	compatible = "ti,am33xx-ehrpwm";
>  	#pwm-cells = <3>;
>  	reg = <0x48300200 0x100>;
>  	ti,hwmods = "ehrpwm0";
>  };
>  
> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */

No leading 0s, but more importantly the address is wrong.

>  	compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>  	#pwm-cells = <3>;
>  	reg = <0x300000 0x2000>;
> -- 
> 2.7.0
> 

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Franklin S Cooper Jr.]

On 03/17/2016 10:03 AM, Rob Herring wrote:
> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>> Now that the node name has been changed from ehrpwm to pwm the document
>> should show this proper usage. Also change the unit address in the example
>> from 0 to the proper physical address value that should be used.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>> index 9c100b2..20211ed 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>> @@ -15,14 +15,14 @@ Optional properties:
>>  
>>  Example:
>>  
>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>  	compatible = "ti,am33xx-ehrpwm";
>>  	#pwm-cells = <3>;
>>  	reg = <0x48300200 0x100>;
>>  	ti,hwmods = "ehrpwm0";
>>  };
>>  
>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
> No leading 0s, but more importantly the address is wrong.

I will remove the leading 0. However, this value was taken
from the .dtsi and I just double checked and I see the same
value in the datasheet. I believe DA850,OMAP-L138 and AM18x
all have the same memory mapping. I'm looking at
http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
addresses match up what is seen here and in the .dtsi.

Can you point me to which document your looking at that
shows a different value?


>
>>  	compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>  	#pwm-cells = <3>;
>>  	reg = <0x300000 0x2000>;
>> -- 
>> 2.7.0
>>

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Rob Herring]

On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>
>
> On 03/17/2016 10:03 AM, Rob Herring wrote:
>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>> Now that the node name has been changed from ehrpwm to pwm the document
>>> should show this proper usage. Also change the unit address in the example
>>> from 0 to the proper physical address value that should be used.
>>>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>> index 9c100b2..20211ed 100644
>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>> @@ -15,14 +15,14 @@ Optional properties:
>>>
>>>  Example:
>>>
>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>      compatible = "ti,am33xx-ehrpwm";
>>>      #pwm-cells = <3>;
>>>      reg = <0x48300200 0x100>;
>>>      ti,hwmods = "ehrpwm0";
>>>  };
>>>
>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>> No leading 0s, but more importantly the address is wrong.
>
> I will remove the leading 0. However, this value was taken
> from the .dtsi and I just double checked and I see the same
> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
> all have the same memory mapping. I'm looking at
> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
> addresses match up what is seen here and in the .dtsi.
>
> Can you point me to which document your looking at that
> shows a different value?

Ummm, ...

>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>      #pwm-cells = <3>;
>>>      reg = <0x300000 0x2000>;

right here.

>>> --
>>> 2.7.0
>>>
>

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Franklin S Cooper Jr.]

On 03/17/2016 01:00 PM, Rob Herring wrote:
> On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper-l0cyMroinI0@public.gmane.org> wrote:
>>
>> On 03/17/2016 10:03 AM, Rob Herring wrote:
>>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>>> Now that the node name has been changed from ehrpwm to pwm the document
>>>> should show this proper usage. Also change the unit address in the example
>>>> from 0 to the proper physical address value that should be used.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>> index 9c100b2..20211ed 100644
>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>> @@ -15,14 +15,14 @@ Optional properties:
>>>>
>>>>  Example:
>>>>
>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>>      compatible = "ti,am33xx-ehrpwm";
>>>>      #pwm-cells = <3>;
>>>>      reg = <0x48300200 0x100>;
>>>>      ti,hwmods = "ehrpwm0";
>>>>  };
>>>>
>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>>> No leading 0s, but more importantly the address is wrong.
>> I will remove the leading 0. However, this value was taken
>> from the .dtsi and I just double checked and I see the same
>> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
>> all have the same memory mapping. I'm looking at
>> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
>> addresses match up what is seen here and in the .dtsi.
>>
>> Can you point me to which document your looking at that
>> shows a different value?
> Ummm, ...
>
>>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>>      #pwm-cells = <3>;
>>>>      reg = <0x300000 0x2000>;
> right here.

So I don't know the history but the SOC node specifies a
ranges value of ranges = <0x0 0x01c00000 0x400000>;. It
seems that all child nodes of SOC have a reg property then
is based on an offset of 0x01c00000. So this is true for
UART, rtc, i2c, wdt, mmc, spi etc... So using a base offset
of 0x01c00000 + 0x300000 (reg value of the pwm) equals the
physical address of the ehrpwm0 register 0x1f00000.

For the child nodes within the SOC node, the unit-address is
always based on the physical address not based on the offset
address.

So the values documented is simply following the convention
that has already been established in the .dtsi.
>
>>>> --
>>>> 2.7.0
>>>>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Rob Herring]

On Thu, Mar 17, 2016 at 1:20 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>
>
> On 03/17/2016 01:00 PM, Rob Herring wrote:
>> On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>
>>> On 03/17/2016 10:03 AM, Rob Herring wrote:
>>>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>>>> Now that the node name has been changed from ehrpwm to pwm the document
>>>>> should show this proper usage. Also change the unit address in the example
>>>>> from 0 to the proper physical address value that should be used.
>>>>>
>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>> index 9c100b2..20211ed 100644
>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>> @@ -15,14 +15,14 @@ Optional properties:
>>>>>
>>>>>  Example:
>>>>>
>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>>>      compatible = "ti,am33xx-ehrpwm";
>>>>>      #pwm-cells = <3>;
>>>>>      reg = <0x48300200 0x100>;
>>>>>      ti,hwmods = "ehrpwm0";
>>>>>  };
>>>>>
>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>>>> No leading 0s, but more importantly the address is wrong.
>>> I will remove the leading 0. However, this value was taken
>>> from the .dtsi and I just double checked and I see the same
>>> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
>>> all have the same memory mapping. I'm looking at
>>> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
>>> addresses match up what is seen here and in the .dtsi.
>>>
>>> Can you point me to which document your looking at that
>>> shows a different value?
>> Ummm, ...
>>
>>>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>>>      #pwm-cells = <3>;
>>>>>      reg = <0x300000 0x2000>;
>> right here.
>
> So I don't know the history but the SOC node specifies a
> ranges value of ranges = <0x0 0x01c00000 0x400000>;. It
> seems that all child nodes of SOC have a reg property then
> is based on an offset of 0x01c00000. So this is true for
> UART, rtc, i2c, wdt, mmc, spi etc... So using a base offset
> of 0x01c00000 + 0x300000 (reg value of the pwm) equals the
> physical address of the ehrpwm0 register 0x1f00000.
>
> For the child nodes within the SOC node, the unit-address is
> always based on the physical address not based on the offset
> address.

They are all wrong and should be fixed. Unit address should match the
reg property unless the bus has defined something different (e.g.
PCI).

Rob

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Franklin S Cooper Jr.]

+Sekhar

On 03/17/2016 01:56 PM, Rob Herring wrote:
> On Thu, Mar 17, 2016 at 1:20 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>
>> On 03/17/2016 01:00 PM, Rob Herring wrote:
>>> On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>> On 03/17/2016 10:03 AM, Rob Herring wrote:
>>>>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>>>>> Now that the node name has been changed from ehrpwm to pwm the document
>>>>>> should show this proper usage. Also change the unit address in the example
>>>>>> from 0 to the proper physical address value that should be used.
>>>>>>
>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>> index 9c100b2..20211ed 100644
>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>> @@ -15,14 +15,14 @@ Optional properties:
>>>>>>
>>>>>>  Example:
>>>>>>
>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>>>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>>>>      compatible = "ti,am33xx-ehrpwm";
>>>>>>      #pwm-cells = <3>;
>>>>>>      reg = <0x48300200 0x100>;
>>>>>>      ti,hwmods = "ehrpwm0";
>>>>>>  };
>>>>>>
>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>>>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>>>>> No leading 0s, but more importantly the address is wrong.
>>>> I will remove the leading 0. However, this value was taken
>>>> from the .dtsi and I just double checked and I see the same
>>>> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
>>>> all have the same memory mapping. I'm looking at
>>>> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
>>>> addresses match up what is seen here and in the .dtsi.
>>>>
>>>> Can you point me to which document your looking at that
>>>> shows a different value?
>>> Ummm, ...
>>>
>>>>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>>>>      #pwm-cells = <3>;
>>>>>>      reg = <0x300000 0x2000>;
>>> right here.
>> So I don't know the history but the SOC node specifies a
>> ranges value of ranges = <0x0 0x01c00000 0x400000>;. It
>> seems that all child nodes of SOC have a reg property then
>> is based on an offset of 0x01c00000. So this is true for
>> UART, rtc, i2c, wdt, mmc, spi etc... So using a base offset
>> of 0x01c00000 + 0x300000 (reg value of the pwm) equals the
>> physical address of the ehrpwm0 register 0x1f00000.
>>
>> For the child nodes within the SOC node, the unit-address is
>> always based on the physical address not based on the offset
>> address.
> They are all wrong and should be fixed. Unit address should match the
> reg property unless the bus has defined something different (e.g.
> PCI).

I added Sekhar to hopefully comment if it would be better to
try to get the reg properties to use their physical
addresses or change the unit address to use the offset values.

I don't mind shooting a patch that makes this massive switch
for all the various peripherals and documentation. However,
this is a bit beyond this series.  In this series I am not
setting the unit address or reg address I am simply
documenting what is currently being used. This patch set is
a dependency for another patchset to get PWM support for DRA7.

Are you ok with me doing the below?
I submit a v2 that removes the leading 0 in this patch. This
way this patchset and my PWMSS support for DRA7 patchset can
get merged. I can then follow up with another separate
patchset that insures both the unit address and the value in
the reg property are aligned.
>
> Rob

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Rob Herring]

On Thu, Mar 17, 2016 at 2:25 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
> +Sekhar
>
> On 03/17/2016 01:56 PM, Rob Herring wrote:
>> On Thu, Mar 17, 2016 at 1:20 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>
>>> On 03/17/2016 01:00 PM, Rob Herring wrote:
>>>> On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>>> On 03/17/2016 10:03 AM, Rob Herring wrote:
>>>>>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>>>>>> Now that the node name has been changed from ehrpwm to pwm the document
>>>>>>> should show this proper usage. Also change the unit address in the example
>>>>>>> from 0 to the proper physical address value that should be used.
>>>>>>>
>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>> ---
>>>>>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>> index 9c100b2..20211ed 100644
>>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>> @@ -15,14 +15,14 @@ Optional properties:
>>>>>>>
>>>>>>>  Example:
>>>>>>>
>>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>>>>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>>>>>      compatible = "ti,am33xx-ehrpwm";
>>>>>>>      #pwm-cells = <3>;
>>>>>>>      reg = <0x48300200 0x100>;
>>>>>>>      ti,hwmods = "ehrpwm0";
>>>>>>>  };
>>>>>>>
>>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>>>>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>>>>>> No leading 0s, but more importantly the address is wrong.
>>>>> I will remove the leading 0. However, this value was taken
>>>>> from the .dtsi and I just double checked and I see the same
>>>>> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
>>>>> all have the same memory mapping. I'm looking at
>>>>> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
>>>>> addresses match up what is seen here and in the .dtsi.
>>>>>
>>>>> Can you point me to which document your looking at that
>>>>> shows a different value?
>>>> Ummm, ...
>>>>
>>>>>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>>>>>      #pwm-cells = <3>;
>>>>>>>      reg = <0x300000 0x2000>;
>>>> right here.
>>> So I don't know the history but the SOC node specifies a
>>> ranges value of ranges = <0x0 0x01c00000 0x400000>;. It
>>> seems that all child nodes of SOC have a reg property then
>>> is based on an offset of 0x01c00000. So this is true for
>>> UART, rtc, i2c, wdt, mmc, spi etc... So using a base offset
>>> of 0x01c00000 + 0x300000 (reg value of the pwm) equals the
>>> physical address of the ehrpwm0 register 0x1f00000.
>>>
>>> For the child nodes within the SOC node, the unit-address is
>>> always based on the physical address not based on the offset
>>> address.
>> They are all wrong and should be fixed. Unit address should match the
>> reg property unless the bus has defined something different (e.g.
>> PCI).
>
> I added Sekhar to hopefully comment if it would be better to
> try to get the reg properties to use their physical
> addresses or change the unit address to use the offset values.

Change the unit addresses (i.e. use ranges). The main reason ranges
with translation is preferred is it limits the scope of the bus as the
h/w design typically would.

> I don't mind shooting a patch that makes this massive switch
> for all the various peripherals and documentation. However,
> this is a bit beyond this series.  In this series I am not
> setting the unit address or reg address I am simply
> documenting what is currently being used. This patch set is
> a dependency for another patchset to get PWM support for DRA7.

Agreed. I'm not asking for everything to be fixed.

> Are you ok with me doing the below?
> I submit a v2 that removes the leading 0 in this patch. This
> way this patchset and my PWMSS support for DRA7 patchset can
> get merged. I can then follow up with another separate
> patchset that insures both the unit address and the value in
> the reg property are aligned.

The example is an example. It doesn't have to match dts files (if it
always did, then examples would be pointless). So please fix the
documentation now. The dts files can be updated separately.

Rob

Re: [PATCH 5/5] pwm: pwm-tiehrpwm: Update dt binding document to use generic node name

[Franklin S Cooper Jr.]

On 03/17/2016 02:48 PM, Rob Herring wrote:
> On Thu, Mar 17, 2016 at 2:25 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>> +Sekhar
>>
>> On 03/17/2016 01:56 PM, Rob Herring wrote:
>>> On Thu, Mar 17, 2016 at 1:20 PM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>> On 03/17/2016 01:00 PM, Rob Herring wrote:
>>>>> On Thu, Mar 17, 2016 at 11:49 AM, Franklin S Cooper Jr. <fcooper@ti.com> wrote:
>>>>>> On 03/17/2016 10:03 AM, Rob Herring wrote:
>>>>>>> On Mon, Mar 07, 2016 at 01:51:58PM -0600, Franklin S Cooper Jr wrote:
>>>>>>>> Now that the node name has been changed from ehrpwm to pwm the document
>>>>>>>> should show this proper usage. Also change the unit address in the example
>>>>>>>> from 0 to the proper physical address value that should be used.
>>>>>>>>
>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>> ---
>>>>>>>>  Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt | 4 ++--
>>>>>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>>> index 9c100b2..20211ed 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/pwm/pwm-tiehrpwm.txt
>>>>>>>> @@ -15,14 +15,14 @@ Optional properties:
>>>>>>>>
>>>>>>>>  Example:
>>>>>>>>
>>>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on am33xx */
>>>>>>>> +ehrpwm0: pwm@48300200 { /* EHRPWM on am33xx */
>>>>>>>>      compatible = "ti,am33xx-ehrpwm";
>>>>>>>>      #pwm-cells = <3>;
>>>>>>>>      reg = <0x48300200 0x100>;
>>>>>>>>      ti,hwmods = "ehrpwm0";
>>>>>>>>  };
>>>>>>>>
>>>>>>>> -ehrpwm0: ehrpwm@0 { /* EHRPWM on da850 */
>>>>>>>> +ehrpwm0: pwm@01f00000 { /* EHRPWM on da850 */
>>>>>>> No leading 0s, but more importantly the address is wrong.
>>>>>> I will remove the leading 0. However, this value was taken
>>>>>> from the .dtsi and I just double checked and I see the same
>>>>>> value in the datasheet. I believe DA850,OMAP-L138 and AM18x
>>>>>> all have the same memory mapping. I'm looking at
>>>>>> http://www.ti.com/lit/ds/symlink/am1808.pdf page 233 and the
>>>>>> addresses match up what is seen here and in the .dtsi.
>>>>>>
>>>>>> Can you point me to which document your looking at that
>>>>>> shows a different value?
>>>>> Ummm, ...
>>>>>
>>>>>>>>      compatible = "ti,da850-ehrpwm", "ti,am33xx-ehrpwm";
>>>>>>>>      #pwm-cells = <3>;
>>>>>>>>      reg = <0x300000 0x2000>;
>>>>> right here.
>>>> So I don't know the history but the SOC node specifies a
>>>> ranges value of ranges = <0x0 0x01c00000 0x400000>;. It
>>>> seems that all child nodes of SOC have a reg property then
>>>> is based on an offset of 0x01c00000. So this is true for
>>>> UART, rtc, i2c, wdt, mmc, spi etc... So using a base offset
>>>> of 0x01c00000 + 0x300000 (reg value of the pwm) equals the
>>>> physical address of the ehrpwm0 register 0x1f00000.
>>>>
>>>> For the child nodes within the SOC node, the unit-address is
>>>> always based on the physical address not based on the offset
>>>> address.
>>> They are all wrong and should be fixed. Unit address should match the
>>> reg property unless the bus has defined something different (e.g.
>>> PCI).
>> I added Sekhar to hopefully comment if it would be better to
>> try to get the reg properties to use their physical
>> addresses or change the unit address to use the offset values.
> Change the unit addresses (i.e. use ranges). The main reason ranges
> with translation is preferred is it limits the scope of the bus as the
> h/w design typically would.
>
>> I don't mind shooting a patch that makes this massive switch
>> for all the various peripherals and documentation. However,
>> this is a bit beyond this series.  In this series I am not
>> setting the unit address or reg address I am simply
>> documenting what is currently being used. This patch set is
>> a dependency for another patchset to get PWM support for DRA7.
> Agreed. I'm not asking for everything to be fixed.
>
>> Are you ok with me doing the below?
>> I submit a v2 that removes the leading 0 in this patch. This
>> way this patchset and my PWMSS support for DRA7 patchset can
>> get merged. I can then follow up with another separate
>> patchset that insures both the unit address and the value in
>> the reg property are aligned.
> The example is an example. It doesn't have to match dts files (if it
> always did, then examples would be pointless). So please fix the
> documentation now. The dts files can be updated separately.

Ok will do.
>
> Rob