diff mbox series

[v2,1/2] mips: dts: Fix PIC32MZDA GPIO register definitions

Message ID DB6P190MB0134D40A21563667C31A16CCFB2E0@DB6P190MB0134.EURP190.PROD.OUTLOOK.COM
State Accepted
Commit 81b543a4e61013fb87f5bdb526d815f1e014d0a4
Delegated to: Daniel Schwierzeck
Headers show
Series [v2,1/2] mips: dts: Fix PIC32MZDA GPIO register definitions | expand

Commit Message

John Robertson Sept. 1, 2020, 6:02 p.m. UTC 
GPIO state cannot be changed via the device tree (e.g. with gpio-hog) or
using the 'gpio' command from the console.

The root cause is a discrepancy between the driver and the device tree:
the driver code expects an absolute I/O address in the <reg> property,
while the device tree defines the address relative to a declaration in
the parent pinctrl node.

Changing the device tree to fix a driver issue would normally be wrong,
however:
- I have run the first version of U-Boot in which this driver appears
  (v2016.03) and the same problem exists, so this is not a regression;
- There is no code that references a parent device tree node that might
  suggest the intent of the author was to parse the DT as it exists now;
- The equivalent Linux PIC32 GPIO driver also uses absolute addresses
  for the GPIO <reg> property. This change brings the U-Boot DT more
  into line with Linux.

Additionally, the data sheet (Microchip ref. 60001361H) shows that the
register set to control a GPIO bank spans 0xE0 bytes, but the device
tree specified size is only 0x48 bytes.

Signed-off-by: John Robertson <john.robertson@simiatec.com>
---
Changes in v2
- Split patch
- Document change more fully

 arch/mips/dts/pic32mzda.dtsi | 45 ++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 23 deletions(-)

Comments

Daniel Schwierzeck Sept. 19, 2020, 7:34 p.m. UTC | #1 
Am Dienstag, den 01.09.2020, 18:02 +0000 schrieb John Robertson:
> GPIO state cannot be changed via the device tree (e.g. with gpio-hog) or
> using the 'gpio' command from the console.
> 
> The root cause is a discrepancy between the driver and the device tree:
> the driver code expects an absolute I/O address in the <reg> property,
> while the device tree defines the address relative to a declaration in
> the parent pinctrl node.
> 
> Changing the device tree to fix a driver issue would normally be wrong,
> however:
> - I have run the first version of U-Boot in which this driver appears
>   (v2016.03) and the same problem exists, so this is not a regression;
> - There is no code that references a parent device tree node that might
>   suggest the intent of the author was to parse the DT as it exists now;
> - The equivalent Linux PIC32 GPIO driver also uses absolute addresses
>   for the GPIO <reg> property. This change brings the U-Boot DT more
>   into line with Linux.
> 
> Additionally, the data sheet (Microchip ref. 60001361H) shows that the
> register set to control a GPIO bank spans 0xE0 bytes, but the device
> tree specified size is only 0x48 bytes.
> 
> Signed-off-by: John Robertson <john.robertson@simiatec.com>
> ---
> Changes in v2
> - Split patch
> - Document change more fully
> 
>  arch/mips/dts/pic32mzda.dtsi | 45 ++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 23 deletions(-)
> 

applied to u-boot-mips/fixes, thanks.
diff mbox series

Patch

diff --git a/arch/mips/dts/pic32mzda.dtsi b/arch/mips/dts/pic32mzda.dtsi
index 4c8b7a9a0b..8f9b4cc50e 100644
--- a/arch/mips/dts/pic32mzda.dtsi
+++ b/arch/mips/dts/pic32mzda.dtsi
@@ -69,6 +69,8 @@ 
 	};
 
 	pinctrl: pinctrl@1f801400 {
+		#address-cells = <1>;
+		#size-cells = <1>;
 		compatible = "microchip,pic32mzda-pinctrl";
 		reg = <0x1f801400 0x100>, /* in  */
 		      <0x1f801500 0x200>, /* out */
@@ -76,75 +78,72 @@ 
 		reg-names = "ppsin","ppsout","port";
 		status = "disabled";
 
-		ranges = <0 0x1f860000 0xa00>;
-		#address-cells = <1>;
-		#size-cells = <1>;
-		gpioA: gpio0@0 {
+		gpioA: gpio0@1f860000 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x000 0x48>;
+			reg = <0x1f860000 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioB: gpio1@100 {
+		gpioB: gpio1@1f860100 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x100 0x48>;
+			reg = <0x1f860100 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioC: gpio2@200 {
+		gpioC: gpio2@1f860200 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x200 0x48>;
+			reg = <0x1f860200 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioD: gpio3@300 {
+		gpioD: gpio3@1f860300 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x300 0x48>;
+			reg = <0x1f860300 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioE: gpio4@400 {
+		gpioE: gpio4@1f860400 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x400 0x48>;
+			reg = <0x1f860400 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioF: gpio5@500 {
+		gpioF: gpio5@1f860500 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x500 0x48>;
+			reg = <0x1f860500 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioG: gpio6@600 {
+		gpioG: gpio6@1f860600 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x600 0x48>;
+			reg = <0x1f860600 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioH: gpio7@700 {
+		gpioH: gpio7@1f860700 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x700 0x48>;
+			reg = <0x1f860700 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioJ: gpio8@800 {
+		gpioJ: gpio8@1f860800 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x800 0x48>;
+			reg = <0x1f860800 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};
 
-		gpioK: gpio9@900 {
+		gpioK: gpio9@1f860900 {
 			compatible = "microchip,pic32mzda-gpio";
-			reg = <0x900 0x48>;
+			reg = <0x1f860900 0xe0>;
 			gpio-controller;
 			#gpio-cells = <2>;
 		};