| Message ID | cover.1610364681.git.baruch@tkos.co.il |
|---|---|
| Headers | show |
| Series | gpio: mvebu: Armada 8K/7K PWM support | expand |
Hi Baruch, this caught my eye: On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote: > Update the example as well. Add the '#pwm-cells' and 'clocks' properties > for a complete working example. > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> (...) > +Optional properties: > + > +- marvell,pwm-offset: offset address of PWM duration control registers inside > + the syscon block (...) > ap_syscon: system-controller@6f4000 { > compatible = "syscon", "simple-mfd"; > @@ -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. Yours, Linus Walleij
On Tue, Jan 12, 2021 at 09:49:16AM +0100, Linus Walleij wrote: > Hi Baruch, > > this caught my eye: > > On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote: > > > Update the example as well. Add the '#pwm-cells' and 'clocks' properties > > for a complete working example. > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > (...) > > +Optional properties: > > + > > +- marvell,pwm-offset: offset address of PWM duration control registers inside > > + the syscon block > (...) > > ap_syscon: system-controller@6f4000 { > > compatible = "syscon", "simple-mfd"; > > @@ -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. I think it would be a good idea to describe the hardware more fully. For the CP110 and AP80x dies on Armada 8040: CP110 AP80x Offset Offset 00/40 5040 Data Out 04/44 5044 Data Out Enable 08/48 5048 Blink Enable 0c/4c 504c Data In polarity 10/50 5050 Data In 14/54 5054 IRQ Cause 18/58 5058 IRQ Mask 1c/5c 505c IRQ Level mask 20/60 5060 Blink Counter Select 28/68 5068 Control Set 2c/6c 506c Control Clear 30/70 5070 Data Out Set 34/74 5074 Data Out Clear f0 50c0 Blink Counter A ON duration f4 50c4 Blink Counter A OFF duration f8 50c8 Blink Counter B ON duration fc 50cc Blink Counter B OFF duration We identify both of these using a compatible of "marvell,armada-8k-gpio" which really only describes the first 64 bytes of the register set: ap_gpio: gpio@1040 { compatible = "marvell,armada-8k-gpio"; offset = <0x1040>; ... }; CP11X_LABEL(gpio1): gpio@100 { compatible = "marvell,armada-8k-gpio"; offset = <0x100>; ... }; CP11X_LABEL(gpio2): gpio@140 { compatible = "marvell,armada-8k-gpio"; offset = <0x140>; ... }; Note that on the CP11x dies, there are two GPIO controllers sharing the same set of blink counter registers - one at offset 0 the other at offset 0x40. However, the pwm-offset is the offset in the regmap of the parent node. It is possible to use a more specific compatible that would describe the PWM offset for the CP11x and AP806 (which would need two different ones) but that starts getting messy when you consider that we already describe an offset in regmap for the first 64 registers, and encoding the blink register offset in a compatible would partially end up encoding the "offset" we already have. 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.
On Tue, Jan 12, 2021 at 11:36 AM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > 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. OK I will not complain about it then, this kind of thing happens sometimes. Thanks Russell! Linus Walleij
On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote: > > 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 <baruch@tkos.co.il> > --- > drivers/gpio/gpio-mvebu.c | 101 +++++++++++++++++++++++++------------- > 1 file changed, 68 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c > index 4261e3b22b4e..6bd45c59056a 100644 > --- a/drivers/gpio/gpio-mvebu.c > +++ b/drivers/gpio/gpio-mvebu.c > @@ -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. > + */ > + 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); Hi Baruch! 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? Bartosz > > mvpwm->clk_rate = clk_get_rate(mvchip->clk); > if (!mvpwm->clk_rate) { > -- > 2.29.2 >
Hi Bartosz, Thanks for you review. On Fri, Jan 22 2021, Bartosz Golaszewski wrote: > On Mon, Jan 11, 2021 at 12:47 PM Baruch Siach <baruch@tkos.co.il> wrote: >> 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 <baruch@tkos.co.il> [...] >> + 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? Yes. The blink counter selection register is at the same offset is all chips that support the GPIO blink feature. Only the on/off registers offset is different. baruch
This version is identical to v4 with the typo fix from v5. This series has no dependency on the fixes series that I posted separately. Tested on top of v5.11-rc2. Changes in v7: * Split the get_state fix to a separate independent fixes series Changes in v6: * Reduce rounding error in the get_state fix (RMK) Changes in v5: * Add a fix for get_state * Fix typo in patch #4 subject line * Add Rob's review tag on the binding documentation patch Changes in v4: * Remove patches that are in LinusW linux-gpio for-next and fixes * Rename the 'pwm-offset' property to 'marvell,pwm-offset' as suggested by Rob Herring The original cover letter follows (with DT property name updated). The gpio-mvebu driver supports the PWM functionality of the GPIO block for earlier Armada variants like XP, 370 and 38x. This series extends support to newer Armada variants that use CP11x and AP80x, like Armada 8K and 7K. This series adds adds the 'marvell,pwm-offset' property to DT binding. 'marvell,pwm-offset' points to the base of A/B counter registers that determine the PWM period and duty cycle. The existing PWM DT binding reflects an arbitrary decision to allocate the A counter to the first GPIO block, and B counter to the other one. In attempt to provide better future flexibility, the new 'marvell,pwm-offset' property always points to the base address of both A/B counters. The driver code still allocates the counters in the same way, but this might change in the future with no change to the DT. Tested AP806 and CP110 (both) on Armada 8040 based system. Baruch Siach (3): gpio: mvebu: add pwm support for Armada 8K/7K arm64: dts: armada: add pwm offsets for ap/cp gpios dt-bindings: ap806: document gpio marvell,pwm-offset property .../arm/marvell/ap80x-system-controller.txt | 8 ++ arch/arm64/boot/dts/marvell/armada-ap80x.dtsi | 3 + arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 10 ++ drivers/gpio/gpio-mvebu.c | 101 ++++++++++++------ 4 files changed, 89 insertions(+), 33 deletions(-)