gpio: mvebu: Armada 8K/7K PWM support

Use the marvell,pwm-offset DT property to store the location of PWM signal duration registers.

Since we have more than two GPIO chips per system, we can't use the alias id to differentiate between them. Use the offset value for that.

Signed-off-by Baruch Siach <[email protected]>

---
 drivers/gpio/gpio-mvebu.c | 101 +++++++++++++++++++++++++-------------
 1 file changed, 68 insertions(+), 33 deletions(-)

drivers/gpio/gpio-mvebu.c +58 -33

index 4261e3b22b4e..6bd45c59056a 100644
@@ -70,7 +70,12 @@
  */
 #define PWM_BLINK_ON_DURATION_OFF      0x0
 #define PWM_BLINK_OFF_DURATION_OFF     0x4
+#define PWM_BLINK_COUNTER_B_OFF                0x8
 
+/* Armada 8k variant gpios register offsets */
+#define AP80X_GPIO0_OFF_A8K            0x1040
+#define CP11X_GPIO0_OFF_A8K            0x100
+#define CP11X_GPIO1_OFF_A8K            0x140
 
 /* The MV78200 has per-CPU registers for edge mask and level mask */
 #define GPIO_EDGE_MASK_MV78200_OFF(cpu)          ((cpu) ? 0x30 : 0x18)

@@ -93,6 +98,7 @@
 
 struct mvebu_pwm {
        struct regmap           *regs;
+       u32                      offset;
        unsigned long            clk_rate;
        struct gpio_desc        *gpiod;
        struct pwm_chip          chip;

@@ -283,12 +289,12 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)
  */
 static unsigned int mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
 {
-       return PWM_BLINK_ON_DURATION_OFF;
+       return mvpwm->offset + PWM_BLINK_ON_DURATION_OFF;
 }
 
 static unsigned int mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
 {
-       return PWM_BLINK_OFF_DURATION_OFF;
+       return mvpwm->offset + PWM_BLINK_OFF_DURATION_OFF;
 }
 
 /*

@@ -775,51 +781,80 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
        struct device *dev = &pdev->dev;
        struct mvebu_pwm *mvpwm;
        void __iomem *base;
+       u32 offset;
        u32 set;
 
-       if (!of_device_is_compatible(mvchip->chip.of_node,
-                                    "marvell,armada-370-gpio"))
-               return 0;
-
-       /*
-        * There are only two sets of PWM configuration registers for
-        * all the GPIO lines on those SoCs which this driver reserves
-        * for the first two GPIO chips. So if the resource is missing
-        * we can't treat it as an error.
-        */
-       if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
+       if (of_device_is_compatible(mvchip->chip.of_node,
+                                   "marvell,armada-370-gpio")) {
+               /*
+                * There are only two sets of PWM configuration registers for
+                * all the GPIO lines on those SoCs which this driver reserves
+                * for the first two GPIO chips. So if the resource is missing
+                * we can't treat it as an error.
+                */
+               if (!platform_get_resource_byname(pdev, IORESOURCE_MEM, "pwm"))
+                       return 0;
+               offset = 0;
+       } else if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
+               int ret = of_property_read_u32(dev->of_node,
+                                              "marvell,pwm-offset", &offset);
+               if (ret < 0)
+                       return 0;
+       } else {
                return 0;
+       }
 
        if (IS_ERR(mvchip->clk))
                return PTR_ERR(mvchip->clk);
 
-       /*
-        * Use set A for lines of GPIO chip with id 0, B for GPIO chip
-        * with id 1. Don't allow further GPIO chips to be used for PWM.
-        */
-       if (id == 0)
-               set = 0;
-       else if (id == 1)
-               set = U32_MAX;
-       else
-               return -EINVAL;
-       regmap_write(mvchip->regs,
-                    GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
-
        mvpwm = devm_kzalloc(dev, sizeof(struct mvebu_pwm), GFP_KERNEL);
        if (!mvpwm)
                return -ENOMEM;
        mvchip->mvpwm = mvpwm;
        mvpwm->mvchip = mvchip;
+       mvpwm->offset = offset;
+
+       if (mvchip->soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K) {
+               mvpwm->regs = mvchip->regs;
+
+               switch (mvchip->offset) {
+               case AP80X_GPIO0_OFF_A8K:
+               case CP11X_GPIO0_OFF_A8K:
+                       /* Blink counter A */
+                       set = 0;
+                       break;
+               case CP11X_GPIO1_OFF_A8K:
+                       /* Blink counter B */
+                       set = U32_MAX;
+                       mvpwm->offset += PWM_BLINK_COUNTER_B_OFF;
+                       break;
+               default:
+                       return -EINVAL;
+               }
+       } else {
+               base = devm_platform_ioremap_resource_byname(pdev, "pwm");
+               if (IS_ERR(base))
+                       return PTR_ERR(base);
 
-       base = devm_platform_ioremap_resource_byname(pdev, "pwm");
-       if (IS_ERR(base))
-               return PTR_ERR(base);
+               mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
+                                                   &mvebu_gpio_regmap_config);
+               if (IS_ERR(mvpwm->regs))
+                       return PTR_ERR(mvpwm->regs);
 
-       mvpwm->regs = devm_regmap_init_mmio(&pdev->dev, base,
-                                           &mvebu_gpio_regmap_config);
-       if (IS_ERR(mvpwm->regs))
-               return PTR_ERR(mvpwm->regs);
+               /*
+                * Use set A for lines of GPIO chip with id 0, B for GPIO chip
+                * with id 1. Don't allow further GPIO chips to be used for PWM.
+                */

drivers/gpio/gpio-mvebu.c +0 -1

        mvpwm->clk_rate = clk_get_rate(mvchip->clk);
        if (!mvpwm->clk_rate) {
--

2.29.2

Quoted 7 lines · Baruch Siach

Use the marvell,pwm-offset DT property to store the location of PWM signal duration registers.

Since we have more than two GPIO chips per system, we can't use the alias id to differentiate between them. Use the offset value for that.

Signed-off-by Baruch Siach <[email protected]>

drivers/gpio/gpio-mvebu.c static int mvebu_pwm_probe(struct platform_device *pdev, · line 857

Quoted 2 lines

drivers/gpio/gpio-mvebu.c +2 -0

+       regmap_write(mvchip->regs,
+                    GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);

Can you confirm that this line is on purpose and that it should be executed even for chips that use a separate regmap for PWM?

The 'marvell,pwm-offset' property of both GPIO blocks (per CP component) point to the same counter registers offset. The driver will decide how to use counters A/B.

This is different from the convention of pwm on earlier Armada series (370/38x). On those systems the assignment of A/B counters to GPIO blocks is coded in both DT and the driver. The actual behaviour of the current driver on Armada 8K/7K is the same as earlier systems.

Add also clock properties for base pwm frequency reference.

Signed-off-by Baruch Siach <[email protected]>

---

Quoted 12 lines

The 'marvell,pwm-offset' property of both GPIO blocks (per CP component) point to the same counter registers offset. The driver will decide how to use counters A/B.

This is different from the convention of pwm on earlier Armada series (370/38x). On those systems the assignment of A/B counters to GPIO blocks is coded in both DT and the driver. The actual behaviour of the current driver on Armada 8K/7K is the same as earlier systems.

Add also clock properties for base pwm frequency reference.

Quoted 2 lines

Signed-off-by Baruch Siach <[email protected]>

---

Andrew, Gregory, Sebastian,

Can we get your Acks on this patch? Are you fine with it going through the GPIO tree?

Bartosz

Hello Bartosz,

Quoted 16 lines · Baruch Siach

Quoted 14 lines

The 'marvell,pwm-offset' property of both GPIO blocks (per CP component) point to the same counter registers offset. The driver will decide how to use counters A/B.

This is different from the convention of pwm on earlier Armada series (370/38x). On those systems the assignment of A/B counters to GPIO blocks is coded in both DT and the driver. The actual behaviour of the current driver on Armada 8K/7K is the same as earlier systems.

Add also clock properties for base pwm frequency reference.

Signed-off-by Baruch Siach <[email protected]>

---

Quoted 4 lines

Andrew, Gregory, Sebastian,

Can we get your Acks on this patch? Are you fine with it going through the GPIO tree?

I prefer applying it on our tree to avoid merge conflicts.

Gregory

The 'marvell,pwm-offset' property of both GPIO blocks (per CP component) point to the same counter registers offset. The driver will decide how to use counters A/B.

This is different from the convention of pwm on earlier Armada series (370/38x). On those systems the assignment of A/B counters to GPIO blocks is coded in both DT and the driver. The actual behaviour of the current driver on Armada 8K/7K is the same as earlier systems.

Add also clock properties for base pwm frequency reference.

Signed-off-by Baruch Siach <[email protected]>

---
 arch/arm64/boot/dts/marvell/armada-ap80x.dtsi |  3 +++
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++++++++++
 2 files changed, 13 insertions(+)

arch/arm64/boot/dts/marvell/armada-ap80x.dtsi +3 -0

index 12e477f1aeb9..6614472100c2 100644
@@ -281,6 +281,9 @@ ap_gpio: gpio@1040 {
 					gpio-controller;
 					#gpio-cells = <2>;
 					gpio-ranges = <&ap_pinctrl 0 0 20>;
+					marvell,pwm-offset = <0x10c0>;
+					#pwm-cells = <2>;
+					clocks = <&ap_clk 3>;
 				};
 			};
 

arch/arm64/boot/dts/marvell/armada-cp11x.dtsi +10 -0

index 994a2fce449a..d774a39334d9 100644
@@ -234,12 +234,17 @@ CP11X_LABEL(gpio1): gpio@100 {
 				gpio-controller;
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 0 32>;
+				marvell,pwm-offset = <0x1f0>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				interrupts = <86 IRQ_TYPE_LEVEL_HIGH>,
 					<85 IRQ_TYPE_LEVEL_HIGH>,
 					<84 IRQ_TYPE_LEVEL_HIGH>,
 					<83 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
+				clock-names = "core", "axi";
+				clocks = <&CP11X_LABEL(clk) 1 21>,
+					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
 

@@ -250,12 +255,17 @@ CP11X_LABEL(gpio2): gpio@140 {
 				gpio-controller;
 				#gpio-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 32 31>;
+				marvell,pwm-offset = <0x1f0>;
+				#pwm-cells = <2>;
 				interrupt-controller;
 				interrupts = <82 IRQ_TYPE_LEVEL_HIGH>,
 					<81 IRQ_TYPE_LEVEL_HIGH>,
 					<80 IRQ_TYPE_LEVEL_HIGH>,
 					<79 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
+				clock-names = "core", "axi";
+				clocks = <&CP11X_LABEL(clk) 1 21>,
+					 <&CP11X_LABEL(clk) 1 17>;
 				status = "disabled";
 			};
 		};

Signature · 1 line

2.29.2

Update the example as well. Add the '#pwm-cells' and 'clocks' properties for a complete working example.

Reviewed-by Rob Herring <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt +4 -0

+Optional properties:
+
+- marvell,pwm-offset: offset address of PWM duration control registers inside
+  the syscon block

Hi Baruch,

this caught my eye:

Quoted 5 lines · Baruch Siach

Update the example as well. Add the '#pwm-cells' and 'clocks' properties for a complete working example.

Reviewed-by Rob Herring <[email protected]>

Signed-off-by Baruch Siach <[email protected]>

(...)

Quoted 4 lines

Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt +4 -0

+Optional properties:
+
+- marvell,pwm-offset: offset address of PWM duration control registers inside
+  the syscon block

(...)

Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt ap_syscon: system-controller@6f4000 { · line 109

Quoted 7 lines

 ap_syscon: system-controller@6f4000 {
        compatible = "syscon", "simple-mfd";

Documentation/devicetree/bindings/arm/marvell/ap80x-system-controller.txt +1 -0

@@ -101,6 +106,9 @@ ap_syscon: system-controller@6f4000 {
                gpio-controller;
                #gpio-cells = <2>;
                gpio-ranges = <&ap_pinctrl 0 0 19>;
+               marvell,pwm-offset = <0x10c0>;

This seems to be one of those cases where we start to encode things related to the hardware variant into the device tree.

Is this just documenting ABI that was introduced in the past and we can not do anything about now? In that case it is OK I suppose.

For a new binding we would certainly require that the system controller provide a specific tertiary compatible string for this, lest we disguise the not-so-simple system controller as "simple-mfd" so:

compatible = "syscon", "simple-mfd", "my-silicon-id";

Then detect the PWM offset by using if(of_device_is_compatibe(np, "my-silicon-id"))
in the code rather than parsing any marvell,pwm-offset property.

In any case, these offsets are a function of how it was originally chosen to describe the hardware in DT, rather than anything about the hardware itself. The choice to use a syscon/regmap is purely an implementation decision rather than something from the hardware, so this DT description is already based around describing what is required for the Linux implementation, rather than purely being a hardware description.