[PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

Hi,

This series adds support for the fan PWM output on the Clearfog GT8K
platform, and can potentially be extended to the Macchiatobin.

The cooling maps are experimental - it seems to work well for me
without the fan speed varying too much, but what I've noticed with
fewer entries in the map is instability in the fan speed - it
continually toggles between two fan speeds as the temperature
rises and falls due to the different fan speed.  Hence, this is more
for discussion at this point (and in any case, -final is likely to be
released today.)

 .../dts/marvell/armada-8040-clearfog-gt-8k.dts     | 125 ++++++++++++
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi      |   6 +
 drivers/gpio/gpio-mvebu.c                          | 220 ++++++++++++++-------
 3 files changed, 275 insertions(+), 76 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[PATCH RFC 1/6] gpio: mvebu: convert pwm to regmap

[Russell King]

Convert mvebu's pwm support to use regmap to access the registers to
prepare the driver to support the "blink" support on CP110.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/gpio-mvebu.c | 69 ++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 30 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index e64c40095356..fa5641615db6 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -94,7 +94,8 @@
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
 struct mvebu_pwm {
-	void __iomem		*membase;
+	struct regmap		*regs;
+	u32			 offset;
 	unsigned long		 clk_rate;
 	struct gpio_desc	*gpiod;
 	struct pwm_chip		 chip;
@@ -129,6 +130,13 @@ struct mvebu_gpio_chip {
 	u32		   level_mask_regs[4];
 };
 
+static const struct regmap_config mvebu_gpio_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+};
+
 /*
  * Functions returning addresses of individual registers for a given
  * GPIO controller.
@@ -280,17 +288,17 @@ mvebu_gpio_write_level_mask(struct mvebu_gpio_chip *mvchip, u32 val)
 }
 
 /*
- * Functions returning addresses of individual registers for a given
+ * Functions returning regmap offsets of individual registers for a given
  * PWM controller.
  */
-static void __iomem *mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
+static u32 mvebu_pwmreg_blink_on_duration(struct mvebu_pwm *mvpwm)
 {
-	return mvpwm->membase + PWM_BLINK_ON_DURATION_OFF;
+	return PWM_BLINK_ON_DURATION_OFF + mvpwm->offset;
 }
 
-static void __iomem *mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
+static u32 mvebu_pwmreg_blink_off_duration(struct mvebu_pwm *mvpwm)
 {
-	return mvpwm->membase + PWM_BLINK_OFF_DURATION_OFF;
+	return PWM_BLINK_OFF_DURATION_OFF + mvpwm->offset;
 }
 
 /*
@@ -660,8 +668,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 
 	spin_lock_irqsave(&mvpwm->lock, flags);
 
-	val = (unsigned long long)
-		readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), &u);
+	val = (unsigned long long)u;
 	val *= NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
 	if (val > UINT_MAX)
@@ -671,8 +679,8 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	else
 		state->duty_cycle = 1;
 
-	val = (unsigned long long)
-		readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
+	val = (unsigned long long)u;
 	val *= NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
 	if (val < state->duty_cycle) {
@@ -726,8 +734,8 @@ static int mvebu_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	spin_lock_irqsave(&mvpwm->lock, flags);
 
-	writel_relaxed(on, mvebu_pwmreg_blink_on_duration(mvpwm));
-	writel_relaxed(off, mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm), on);
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), off);
 	if (state->enabled)
 		mvebu_gpio_blink(&mvchip->chip, pwm->hwpwm, 1);
 	else
@@ -752,10 +760,10 @@ static void __maybe_unused mvebu_pwm_suspend(struct mvebu_gpio_chip *mvchip)
 
 	regmap_read(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset,
 		    &mvpwm->blink_select);
-	mvpwm->blink_on_duration =
-		readl_relaxed(mvebu_pwmreg_blink_on_duration(mvpwm));
-	mvpwm->blink_off_duration =
-		readl_relaxed(mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm),
+		    &mvpwm->blink_on_duration);
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm),
+		    &mvpwm->blink_off_duration);
 }
 
 static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
@@ -764,10 +772,10 @@ static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
 
 	regmap_write(mvchip->regs, GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset,
 		     mvpwm->blink_select);
-	writel_relaxed(mvpwm->blink_on_duration,
-		       mvebu_pwmreg_blink_on_duration(mvpwm));
-	writel_relaxed(mvpwm->blink_off_duration,
-		       mvebu_pwmreg_blink_off_duration(mvpwm));
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_on_duration(mvpwm),
+		     mvpwm->blink_on_duration);
+	regmap_write(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm),
+		     mvpwm->blink_off_duration);
 }
 
 static int mvebu_pwm_probe(struct platform_device *pdev,
@@ -776,6 +784,7 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 {
 	struct device *dev = &pdev->dev;
 	struct mvebu_pwm *mvpwm;
+	void __iomem *base;
 	u32 set;
 
 	if (!of_device_is_compatible(mvchip->chip.of_node,
@@ -795,12 +804,14 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 		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;
 
@@ -810,9 +821,14 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	 * for the first two GPIO chips. So if the resource is missing
 	 * we can't treat it as an error.
 	 */
-	mvpwm->membase = devm_platform_ioremap_resource_byname(pdev, "pwm");
-	if (IS_ERR(mvpwm->membase))
-		return PTR_ERR(mvpwm->membase);
+	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->clk_rate = clk_get_rate(mvchip->clk);
 	if (!mvpwm->clk_rate) {
@@ -1023,13 +1039,6 @@ static int mvebu_gpio_resume(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct regmap_config mvebu_gpio_regmap_config = {
-	.reg_bits = 32,
-	.reg_stride = 4,
-	.val_bits = 32,
-	.fast_io = true,
-};
-
 static int mvebu_gpio_probe_raw(struct platform_device *pdev,
 				struct mvebu_gpio_chip *mvchip)
 {
-- 
2.20.1

[PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Russell King]

Honour deferred probing for devm_clk_get() so that we can get the clock
for PWM.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/gpio-mvebu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index fa5641615db6..ee13b11c5298 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	}
 
 	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
+	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
+		return -EPROBE_DEFER;
+
 	/* Not all SoCs require a clock.*/
 	if (!IS_ERR(mvchip->clk))
 		clk_prepare_enable(mvchip->clk);
-- 
2.20.1

Re: [PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Uwe Kleine-König]

On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote:
> Honour deferred probing for devm_clk_get() so that we can get the clock
> for PWM.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpio/gpio-mvebu.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index fa5641615db6..ee13b11c5298 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	}
>  
>  	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
> +		return -EPROBE_DEFER;
> +
>  	/* Not all SoCs require a clock.*/
>  	if (!IS_ERR(mvchip->clk))
>  		clk_prepare_enable(mvchip->clk);

I'd say the following is the right thing to do here:

	mvchip->clk = devm_clk_get_optional(...);
	if (IS_ERR(mvchip->clk))
		return ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Russell King - ARM Linux admin]

On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote:
> On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote:
> > Honour deferred probing for devm_clk_get() so that we can get the clock
> > for PWM.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  drivers/gpio/gpio-mvebu.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > index fa5641615db6..ee13b11c5298 100644
> > --- a/drivers/gpio/gpio-mvebu.c
> > +++ b/drivers/gpio/gpio-mvebu.c
> > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
> > +		return -EPROBE_DEFER;
> > +
> >  	/* Not all SoCs require a clock.*/
> >  	if (!IS_ERR(mvchip->clk))
> >  		clk_prepare_enable(mvchip->clk);
> 
> I'd say the following is the right thing to do here:
> 
> 	mvchip->clk = devm_clk_get_optional(...);
> 	if (IS_ERR(mvchip->clk))
> 		return ...

It's not that simple.  The clock is required for Armada 370, and is
optional for Armada 8040.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Uwe Kleine-König]

Hello Russell,

On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote:
> > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote:
> > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > > index fa5641615db6..ee13b11c5298 100644
> > > --- a/drivers/gpio/gpio-mvebu.c
> > > +++ b/drivers/gpio/gpio-mvebu.c
> > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> > >  	}
> > >  
> > >  	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > > +	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
> > > +		return -EPROBE_DEFER;
> > > +
> > >  	/* Not all SoCs require a clock.*/
> > >  	if (!IS_ERR(mvchip->clk))
> > >  		clk_prepare_enable(mvchip->clk);
> > 
> > I'd say the following is the right thing to do here:
> > 
> > 	mvchip->clk = devm_clk_get_optional(...);
> > 	if (IS_ERR(mvchip->clk))
> > 		return ...
> 
> It's not that simple.  The clock is required for Armada 370, and is
> optional for Armada 8040.

I'd say it is still the right approach here. On Armada 370 the dtb then
has a clk and on Armada 8040 it doesn't. So if with
devm_clk_get_optional() something goes wrong that's because the dtb is
wrong. And in fact the handling is even better than with your suggested
patch as every error (but EPROBE_DEFER) is ignored instead of passed to
the caller with your (and the existing) approach.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Russell King - ARM Linux admin]

On Sun, Mar 29, 2020 at 08:00:56PM +0200, Uwe Kleine-König wrote:
> Hello Russell,
> 
> On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote:
> > On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote:
> > > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote:
> > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > > > index fa5641615db6..ee13b11c5298 100644
> > > > --- a/drivers/gpio/gpio-mvebu.c
> > > > +++ b/drivers/gpio/gpio-mvebu.c
> > > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> > > >  	}
> > > >  
> > > >  	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > > > +	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
> > > > +		return -EPROBE_DEFER;
> > > > +
> > > >  	/* Not all SoCs require a clock.*/
> > > >  	if (!IS_ERR(mvchip->clk))
> > > >  		clk_prepare_enable(mvchip->clk);
> > > 
> > > I'd say the following is the right thing to do here:
> > > 
> > > 	mvchip->clk = devm_clk_get_optional(...);
> > > 	if (IS_ERR(mvchip->clk))
> > > 		return ...
> > 
> > It's not that simple.  The clock is required for Armada 370, and is
> > optional for Armada 8040.
> 
> I'd say it is still the right approach here. On Armada 370 the dtb then
> has a clk and on Armada 8040 it doesn't. So if with
> devm_clk_get_optional() something goes wrong that's because the dtb is
> wrong. And in fact the handling is even better than with your suggested
> patch as every error (but EPROBE_DEFER) is ignored instead of passed to
> the caller with your (and the existing) approach.

Sort of.  Every error is currently treated as "no clock", and only
later does such an error become fatal in the driver _if_ PWM is
configured into the kernel and we're running on Armada 370.  If PWM
is disabled in the kernel, or on some other SoC, then the driver
doesn't care whether getting the clock reported any kind of error.

Your proposal is to always treat any error getting the clock,
irrespective of whether there is PWM or not, as a fatal error for
the driver.

That is an entirely seperate functional change.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 2/6] gpio: mvebu: honour EPROBE_DEFER for devm_clk_get()

[Uwe Kleine-König]

Hello Russell,

On Sun, Mar 29, 2020 at 07:22:36PM +0100, Russell King - ARM Linux admin wrote:
> On Sun, Mar 29, 2020 at 08:00:56PM +0200, Uwe Kleine-König wrote:
> > On Sun, Mar 29, 2020 at 02:34:00PM +0100, Russell King - ARM Linux admin wrote:
> > > On Sun, Mar 29, 2020 at 03:16:59PM +0200, Uwe Kleine-König wrote:
> > > > On Sun, Mar 29, 2020 at 11:48:09AM +0100, Russell King wrote:
> > > > > diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> > > > > index fa5641615db6..ee13b11c5298 100644
> > > > > --- a/drivers/gpio/gpio-mvebu.c
> > > > > +++ b/drivers/gpio/gpio-mvebu.c
> > > > > @@ -1132,6 +1132,9 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
> > > > >  	}
> > > > >  
> > > > >  	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > > > > +	if (mvchip->clk == ERR_PTR(-EPROBE_DEFER))
> > > > > +		return -EPROBE_DEFER;
> > > > > +
> > > > >  	/* Not all SoCs require a clock.*/
> > > > >  	if (!IS_ERR(mvchip->clk))
> > > > >  		clk_prepare_enable(mvchip->clk);
> > > > 
> > > > I'd say the following is the right thing to do here:
> > > > 
> > > > 	mvchip->clk = devm_clk_get_optional(...);
> > > > 	if (IS_ERR(mvchip->clk))
> > > > 		return ...
> > > 
> > > It's not that simple.  The clock is required for Armada 370, and is
> > > optional for Armada 8040.
> > 
> > I'd say it is still the right approach here. On Armada 370 the dtb then
> > has a clk and on Armada 8040 it doesn't. So if with
> > devm_clk_get_optional() something goes wrong that's because the dtb is
> > wrong. And in fact the handling is even better than with your suggested
> > patch as every error (but EPROBE_DEFER) is ignored instead of passed to
> > the caller with your (and the existing) approach.
> 
> Sort of.  Every error is currently treated as "no clock", and only
> later does such an error become fatal in the driver _if_ PWM is
> configured into the kernel and we're running on Armada 370.  If PWM
> is disabled in the kernel, or on some other SoC, then the driver
> doesn't care whether getting the clock reported any kind of error.
> 
> Your proposal is to always treat any error getting the clock,
> irrespective of whether there is PWM or not, as a fatal error for
> the driver.

Is this clock (assuming it's available) needed for GPIO operation? If
not, I'd say the call to devm_clk_get should go into mvebu_pwm_probe().
And if yes, then use devm_clk_get_optional in mvebu_gpio_probe() and
either request it once more in mvebu_pwm_probe() (without _optional) or
test for mvchip->clk == NULL. (Or maybe just don't check and let the
driver fail when clk_get_rate(mvchip->clk) returns zero.)

> That is an entirely seperate functional change.

This is still different to what you do, but it is (IMHO) cleaner and
fixes the problem you want to solve en passant.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[PATCH RFC 3/6] gpio: mvebu: add PWM support for Armada 8k

[Russell King]

Add support for PWM devices on the Armada 8k, which are useful on the
Macchiatobin and Clearfog GT 8K platforms for controlling the fan
speed.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/gpio-mvebu.c | 166 +++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 55 deletions(-)

diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
index ee13b11c5298..4abe298e9c0f 100644
--- a/drivers/gpio/gpio-mvebu.c
+++ b/drivers/gpio/gpio-mvebu.c
@@ -91,8 +91,16 @@
 #define MVEBU_GPIO_SOC_VARIANT_ARMADAXP 0x3
 #define MVEBU_GPIO_SOC_VARIANT_A8K	0x4
 
+#define MVEBU_PWM_SOC_VARIANT_ARMADA370	1
+#define MVEBU_PWM_SOC_VARIANT_A8K	2
+
 #define MVEBU_MAX_GPIO_PER_BANK		32
 
+struct mvebu_gpio_soc_variant {
+	int gpio;
+	int pwm;
+};
+
 struct mvebu_pwm {
 	struct regmap		*regs;
 	u32			 offset;
@@ -679,21 +687,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
 	else
 		state->duty_cycle = 1;
 
-	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
 	val = (unsigned long long)u;
+	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
+	val += (unsigned long long)u;
 	val *= NSEC_PER_SEC;
 	do_div(val, mvpwm->clk_rate);
-	if (val < state->duty_cycle) {
+	if (val > UINT_MAX)
+		state->period = UINT_MAX;
+	else if (val)
+		state->period = val;
+	else
 		state->period = 1;
-	} else {
-		val -= state->duty_cycle;
-		if (val > UINT_MAX)
-			state->period = UINT_MAX;
-		else if (val)
-			state->period = val;
-		else
-			state->period = 1;
-	}
 
 	regmap_read(mvchip->regs, GPIO_BLINK_EN_OFF + mvchip->offset, &u);
 	if (u)
@@ -779,6 +783,7 @@ static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
 }
 
 static int mvebu_pwm_probe(struct platform_device *pdev,
+			   const struct mvebu_gpio_soc_variant *soc_variant,
 			   struct mvebu_gpio_chip *mvchip,
 			   int id)
 {
@@ -787,27 +792,9 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	void __iomem *base;
 	u32 set;
 
-	if (!of_device_is_compatible(mvchip->chip.of_node,
-				     "marvell,armada-370-gpio"))
+	if (!soc_variant->pwm)
 		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;
@@ -815,20 +802,67 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
 	mvchip->mvpwm = mvpwm;
 	mvpwm->mvchip = mvchip;
 
-	/*
-	 * 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.
-	 */
-	base = devm_platform_ioremap_resource_byname(pdev, "pwm");
-	if (IS_ERR(base))
-		return PTR_ERR(base);
+	switch (soc_variant->pwm) {
+	case MVEBU_PWM_SOC_VARIANT_ARMADA370:
+		if (IS_ERR(mvchip->clk))
+			return PTR_ERR(mvchip->clk);
+
+		/*
+		 * 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.
+		 */
+		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;
+		break;
+
+	case MVEBU_PWM_SOC_VARIANT_A8K:
+		/*
+		 * If there is no clock, this is an older DT, so avoid
+		 * registering the PWM.
+		 */
+		if (IS_ERR(mvchip->clk))
+			return 0;
+
+		mvpwm->regs = mvchip->regs;
+		switch (id) {
+		case 1:
+			mvpwm->offset = 0x1f0;
+			set = 0;
+			break;
+		case 2:
+			mvpwm->offset = 0x1f8;
+			set = U32_MAX;
+			break;
+		default:
+			return -EINVAL;
+		}
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	regmap_write(mvchip->regs,
+		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
 
 	mvpwm->clk_rate = clk_get_rate(mvchip->clk);
 	if (!mvpwm->clk_rate) {
@@ -909,26 +943,48 @@ static void mvebu_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
 #define mvebu_gpio_dbg_show NULL
 #endif
 
+static const struct mvebu_gpio_soc_variant mvebu_gpio_orion_variant = {
+	.gpio = MVEBU_GPIO_SOC_VARIANT_ORION,
+};
+
+static const struct mvebu_gpio_soc_variant mvebu_gpio_mv78200_variant = {
+	.gpio = MVEBU_GPIO_SOC_VARIANT_MV78200,
+};
+
+static const struct mvebu_gpio_soc_variant mvebu_gpio_armadaxp_variant = {
+	.gpio = MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
+};
+
+static const struct mvebu_gpio_soc_variant mvebu_gpio_armada370_variant = {
+	.gpio = MVEBU_GPIO_SOC_VARIANT_ORION,
+	.pwm = MVEBU_PWM_SOC_VARIANT_ARMADA370,
+};
+
+static const struct mvebu_gpio_soc_variant mvebu_gpio_a8k_variant = {
+	.gpio = MVEBU_GPIO_SOC_VARIANT_A8K,
+	.pwm = MVEBU_PWM_SOC_VARIANT_A8K,
+};
+
 static const struct of_device_id mvebu_gpio_of_match[] = {
 	{
 		.compatible = "marvell,orion-gpio",
-		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
+		.data	    = &mvebu_gpio_orion_variant,
 	},
 	{
 		.compatible = "marvell,mv78200-gpio",
-		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_MV78200,
+		.data	    = &mvebu_gpio_mv78200_variant,
 	},
 	{
 		.compatible = "marvell,armadaxp-gpio",
-		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
+		.data	    = &mvebu_gpio_armadaxp_variant,
 	},
 	{
 		.compatible = "marvell,armada-370-gpio",
-		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
+		.data	    = &mvebu_gpio_armada370_variant,
 	},
 	{
 		.compatible = "marvell,armada-8k-gpio",
-		.data       = (void *) MVEBU_GPIO_SOC_VARIANT_A8K,
+		.data       = &mvebu_gpio_a8k_variant,
 	},
 	{
 		/* sentinel */
@@ -1093,6 +1149,7 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
 
 static int mvebu_gpio_probe(struct platform_device *pdev)
 {
+	const struct mvebu_gpio_soc_variant *soc_variant;
 	struct mvebu_gpio_chip *mvchip;
 	const struct of_device_id *match;
 	struct device_node *np = pdev->dev.of_node;
@@ -1100,15 +1157,14 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	struct irq_chip_type *ct;
 	unsigned int ngpios;
 	bool have_irqs;
-	int soc_variant;
 	int i, cpu, id;
 	int err;
 
 	match = of_match_device(mvebu_gpio_of_match, &pdev->dev);
 	if (match)
-		soc_variant = (unsigned long) match->data;
+		soc_variant = match->data;
 	else
-		soc_variant = MVEBU_GPIO_SOC_VARIANT_ORION;
+		soc_variant = &mvebu_gpio_orion_variant;
 
 	/* Some gpio controllers do not provide irq support */
 	have_irqs = of_irq_count(np) != 0;
@@ -1139,7 +1195,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	if (!IS_ERR(mvchip->clk))
 		clk_prepare_enable(mvchip->clk);
 
-	mvchip->soc_variant = soc_variant;
+	mvchip->soc_variant = soc_variant->gpio;
 	mvchip->chip.label = dev_name(&pdev->dev);
 	mvchip->chip.parent = &pdev->dev;
 	mvchip->chip.request = gpiochip_generic_request;
@@ -1157,7 +1213,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	mvchip->chip.of_node = np;
 	mvchip->chip.dbg_show = mvebu_gpio_dbg_show;
 
-	if (soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K)
+	if (soc_variant->gpio == MVEBU_GPIO_SOC_VARIANT_A8K)
 		err = mvebu_gpio_probe_syscon(pdev, mvchip);
 	else
 		err = mvebu_gpio_probe_raw(pdev, mvchip);
@@ -1168,7 +1224,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 	/*
 	 * Mask and clear GPIO interrupts.
 	 */
-	switch (soc_variant) {
+	switch (soc_variant->gpio) {
 	case MVEBU_GPIO_SOC_VARIANT_ORION:
 	case MVEBU_GPIO_SOC_VARIANT_A8K:
 		regmap_write(mvchip->regs,
@@ -1265,7 +1321,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
 
 	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
 	if (IS_ENABLED(CONFIG_PWM))
-		return mvebu_pwm_probe(pdev, mvchip, id);
+		return mvebu_pwm_probe(pdev, soc_variant, mvchip, id);
 
 	return 0;
 
-- 
2.20.1

Re: [PATCH RFC 3/6] gpio: mvebu: add PWM support for Armada 8k

[Russell King - ARM Linux admin]

On Sun, Mar 29, 2020 at 11:48:14AM +0100, Russell King wrote:
> Add support for PWM devices on the Armada 8k, which are useful on the
> Macchiatobin and Clearfog GT 8K platforms for controlling the fan
> speed.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/gpio/gpio-mvebu.c | 166 +++++++++++++++++++++++++-------------
>  1 file changed, 111 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-mvebu.c b/drivers/gpio/gpio-mvebu.c
> index ee13b11c5298..4abe298e9c0f 100644
> --- a/drivers/gpio/gpio-mvebu.c
> +++ b/drivers/gpio/gpio-mvebu.c
> @@ -91,8 +91,16 @@
>  #define MVEBU_GPIO_SOC_VARIANT_ARMADAXP 0x3
>  #define MVEBU_GPIO_SOC_VARIANT_A8K	0x4
>  
> +#define MVEBU_PWM_SOC_VARIANT_ARMADA370	1
> +#define MVEBU_PWM_SOC_VARIANT_A8K	2
> +
>  #define MVEBU_MAX_GPIO_PER_BANK		32
>  
> +struct mvebu_gpio_soc_variant {
> +	int gpio;
> +	int pwm;
> +};
> +
>  struct mvebu_pwm {
>  	struct regmap		*regs;
>  	u32			 offset;
> @@ -679,21 +687,17 @@ static void mvebu_pwm_get_state(struct pwm_chip *chip,
>  	else
>  		state->duty_cycle = 1;
>  
> -	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
>  	val = (unsigned long long)u;
> +	regmap_read(mvpwm->regs, mvebu_pwmreg_blink_off_duration(mvpwm), &u);
> +	val += (unsigned long long)u;
>  	val *= NSEC_PER_SEC;
>  	do_div(val, mvpwm->clk_rate);
> -	if (val < state->duty_cycle) {
> +	if (val > UINT_MAX)
> +		state->period = UINT_MAX;
> +	else if (val)
> +		state->period = val;
> +	else
>  		state->period = 1;
> -	} else {
> -		val -= state->duty_cycle;
> -		if (val > UINT_MAX)
> -			state->period = UINT_MAX;
> -		else if (val)
> -			state->period = val;
> -		else
> -			state->period = 1;
> -	}

I should've split this out - there seems to be a bug in the existing
PWM implementation concerning the calculation of the period, which the
above change corrects.

One register contains the duration for the "on" part of the period, and
the other for the "off" part of the period. Therefore, the total period
is the sum of _both_ the on part and the off part.

>  
>  	regmap_read(mvchip->regs, GPIO_BLINK_EN_OFF + mvchip->offset, &u);
>  	if (u)
> @@ -779,6 +783,7 @@ static void __maybe_unused mvebu_pwm_resume(struct mvebu_gpio_chip *mvchip)
>  }
>  
>  static int mvebu_pwm_probe(struct platform_device *pdev,
> +			   const struct mvebu_gpio_soc_variant *soc_variant,
>  			   struct mvebu_gpio_chip *mvchip,
>  			   int id)
>  {
> @@ -787,27 +792,9 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>  	void __iomem *base;
>  	u32 set;
>  
> -	if (!of_device_is_compatible(mvchip->chip.of_node,
> -				     "marvell,armada-370-gpio"))
> +	if (!soc_variant->pwm)
>  		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;
> @@ -815,20 +802,67 @@ static int mvebu_pwm_probe(struct platform_device *pdev,
>  	mvchip->mvpwm = mvpwm;
>  	mvpwm->mvchip = mvchip;
>  
> -	/*
> -	 * 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.
> -	 */
> -	base = devm_platform_ioremap_resource_byname(pdev, "pwm");
> -	if (IS_ERR(base))
> -		return PTR_ERR(base);
> +	switch (soc_variant->pwm) {
> +	case MVEBU_PWM_SOC_VARIANT_ARMADA370:
> +		if (IS_ERR(mvchip->clk))
> +			return PTR_ERR(mvchip->clk);
> +
> +		/*
> +		 * 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.
> +		 */
> +		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;
> +		break;
> +
> +	case MVEBU_PWM_SOC_VARIANT_A8K:
> +		/*
> +		 * If there is no clock, this is an older DT, so avoid
> +		 * registering the PWM.
> +		 */
> +		if (IS_ERR(mvchip->clk))
> +			return 0;
> +
> +		mvpwm->regs = mvchip->regs;
> +		switch (id) {
> +		case 1:
> +			mvpwm->offset = 0x1f0;
> +			set = 0;
> +			break;
> +		case 2:
> +			mvpwm->offset = 0x1f8;
> +			set = U32_MAX;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	regmap_write(mvchip->regs,
> +		     GPIO_BLINK_CNT_SELECT_OFF + mvchip->offset, set);
>  
>  	mvpwm->clk_rate = clk_get_rate(mvchip->clk);
>  	if (!mvpwm->clk_rate) {
> @@ -909,26 +943,48 @@ static void mvebu_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
>  #define mvebu_gpio_dbg_show NULL
>  #endif
>  
> +static const struct mvebu_gpio_soc_variant mvebu_gpio_orion_variant = {
> +	.gpio = MVEBU_GPIO_SOC_VARIANT_ORION,
> +};
> +
> +static const struct mvebu_gpio_soc_variant mvebu_gpio_mv78200_variant = {
> +	.gpio = MVEBU_GPIO_SOC_VARIANT_MV78200,
> +};
> +
> +static const struct mvebu_gpio_soc_variant mvebu_gpio_armadaxp_variant = {
> +	.gpio = MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> +};
> +
> +static const struct mvebu_gpio_soc_variant mvebu_gpio_armada370_variant = {
> +	.gpio = MVEBU_GPIO_SOC_VARIANT_ORION,
> +	.pwm = MVEBU_PWM_SOC_VARIANT_ARMADA370,
> +};
> +
> +static const struct mvebu_gpio_soc_variant mvebu_gpio_a8k_variant = {
> +	.gpio = MVEBU_GPIO_SOC_VARIANT_A8K,
> +	.pwm = MVEBU_PWM_SOC_VARIANT_A8K,
> +};
> +
>  static const struct of_device_id mvebu_gpio_of_match[] = {
>  	{
>  		.compatible = "marvell,orion-gpio",
> -		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
> +		.data	    = &mvebu_gpio_orion_variant,
>  	},
>  	{
>  		.compatible = "marvell,mv78200-gpio",
> -		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_MV78200,
> +		.data	    = &mvebu_gpio_mv78200_variant,
>  	},
>  	{
>  		.compatible = "marvell,armadaxp-gpio",
> -		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ARMADAXP,
> +		.data	    = &mvebu_gpio_armadaxp_variant,
>  	},
>  	{
>  		.compatible = "marvell,armada-370-gpio",
> -		.data	    = (void *) MVEBU_GPIO_SOC_VARIANT_ORION,
> +		.data	    = &mvebu_gpio_armada370_variant,
>  	},
>  	{
>  		.compatible = "marvell,armada-8k-gpio",
> -		.data       = (void *) MVEBU_GPIO_SOC_VARIANT_A8K,
> +		.data       = &mvebu_gpio_a8k_variant,
>  	},
>  	{
>  		/* sentinel */
> @@ -1093,6 +1149,7 @@ static int mvebu_gpio_probe_syscon(struct platform_device *pdev,
>  
>  static int mvebu_gpio_probe(struct platform_device *pdev)
>  {
> +	const struct mvebu_gpio_soc_variant *soc_variant;
>  	struct mvebu_gpio_chip *mvchip;
>  	const struct of_device_id *match;
>  	struct device_node *np = pdev->dev.of_node;
> @@ -1100,15 +1157,14 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	struct irq_chip_type *ct;
>  	unsigned int ngpios;
>  	bool have_irqs;
> -	int soc_variant;
>  	int i, cpu, id;
>  	int err;
>  
>  	match = of_match_device(mvebu_gpio_of_match, &pdev->dev);
>  	if (match)
> -		soc_variant = (unsigned long) match->data;
> +		soc_variant = match->data;
>  	else
> -		soc_variant = MVEBU_GPIO_SOC_VARIANT_ORION;
> +		soc_variant = &mvebu_gpio_orion_variant;
>  
>  	/* Some gpio controllers do not provide irq support */
>  	have_irqs = of_irq_count(np) != 0;
> @@ -1139,7 +1195,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	if (!IS_ERR(mvchip->clk))
>  		clk_prepare_enable(mvchip->clk);
>  
> -	mvchip->soc_variant = soc_variant;
> +	mvchip->soc_variant = soc_variant->gpio;
>  	mvchip->chip.label = dev_name(&pdev->dev);
>  	mvchip->chip.parent = &pdev->dev;
>  	mvchip->chip.request = gpiochip_generic_request;
> @@ -1157,7 +1213,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	mvchip->chip.of_node = np;
>  	mvchip->chip.dbg_show = mvebu_gpio_dbg_show;
>  
> -	if (soc_variant == MVEBU_GPIO_SOC_VARIANT_A8K)
> +	if (soc_variant->gpio == MVEBU_GPIO_SOC_VARIANT_A8K)
>  		err = mvebu_gpio_probe_syscon(pdev, mvchip);
>  	else
>  		err = mvebu_gpio_probe_raw(pdev, mvchip);
> @@ -1168,7 +1224,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  	/*
>  	 * Mask and clear GPIO interrupts.
>  	 */
> -	switch (soc_variant) {
> +	switch (soc_variant->gpio) {
>  	case MVEBU_GPIO_SOC_VARIANT_ORION:
>  	case MVEBU_GPIO_SOC_VARIANT_A8K:
>  		regmap_write(mvchip->regs,
> @@ -1265,7 +1321,7 @@ static int mvebu_gpio_probe(struct platform_device *pdev)
>  
>  	/* Some MVEBU SoCs have simple PWM support for GPIO lines */
>  	if (IS_ENABLED(CONFIG_PWM))
> -		return mvebu_pwm_probe(pdev, mvchip, id);
> +		return mvebu_pwm_probe(pdev, soc_variant, mvchip, id);
>  
>  	return 0;
>  
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[PATCH RFC 4/6] arm64: dts: armada-cp11x: add pwm support to GPIO blocks

[Russell King]

Add PWM support to the GPIO blocks.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm64/boot/dts/marvell/armada-cp11x.dtsi | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
index 024073edfc1c..dfd251acc194 100644
--- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
@@ -311,6 +311,7 @@
 				ngpios = <32>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 0 32>;
 				interrupt-controller;
 				interrupts = <86 IRQ_TYPE_LEVEL_HIGH>,
@@ -319,6 +320,8 @@
 					<83 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
 				status = "disabled";
+				clock-names = "core";
+				clocks = <&CP11X_LABEL(clk) 1 21>;
 			};
 
 			CP11X_LABEL(gpio2): gpio@140 {
@@ -327,6 +330,7 @@
 				ngpios = <31>;
 				gpio-controller;
 				#gpio-cells = <2>;
+				#pwm-cells = <2>;
 				gpio-ranges = <&CP11X_LABEL(pinctrl) 0 32 31>;
 				interrupt-controller;
 				interrupts = <82 IRQ_TYPE_LEVEL_HIGH>,
@@ -335,6 +339,8 @@
 					<79 IRQ_TYPE_LEVEL_HIGH>;
 				#interrupt-cells = <2>;
 				status = "disabled";
+				clock-names = "core";
+				clocks = <&CP11X_LABEL(clk) 1 21>;
 			};
 		};
 
-- 
2.20.1

[PATCH RFC 5/6] arm64: dts: clearfog-gt-8k: add pwm-fan

[Russell King]

Add pwm-fan support for controlling the fan speed.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index dc531d136273..a514ae51ccbf 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
@@ -31,6 +31,11 @@
 		ethernet2 = &cp1_eth2;
 	};
 
+	pwm {
+		compatible = "pwm-fan";
+		pwms = <&cp0_gpio2 16 40000>;
+	};
+
 	v_3_3: regulator-3-3v {
 		compatible = "regulator-fixed";
 		regulator-name = "v_3_3";
-- 
2.20.1

[PATCH RFC 6/6] arm64: dts: clearfog-gt-8k: add cooling maps

[Russell King]

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../marvell/armada-8040-clearfog-gt-8k.dts    | 122 +++++++++++++++++-
 1 file changed, 121 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
index a514ae51ccbf..1e7b47affe26 100644
--- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
+++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts
@@ -31,8 +31,11 @@
 		ethernet2 = &cp1_eth2;
 	};
 
-	pwm {
+	fan: pwm {
 		compatible = "pwm-fan";
+		/* 20% steps */
+		cooling-levels = <0 51 102 153 204 255>;
+		#cooling-cells = <2>;
 		pwms = <&cp0_gpio2 16 40000>;
 	};
 
@@ -107,6 +110,123 @@
 	};
 };
 
+&ap_thermal_ic {
+	polling-delay = <1000>; /* milliseconds */
+	trips {
+		ap_active: trip-active {
+			temperature = <40000>; /* millicelsius */
+			hysteresis = <4000>; /* millicelsius */
+			type = "active";
+		};
+	};
+	cooling-maps {
+		map0 {
+			trip = <&ap_active>;
+			cooling-device = <&fan THERMAL_NO_LIMIT 4>;
+		};
+		map1 {
+			trip = <&ap_crit>;
+			cooling-device = <&fan 4 5>;
+		};
+	};
+};
+
+&cp0_thermal_ic {
+	polling-delay = <1000>; /* milliseconds */
+	trips {
+		cp0_active0: trip-active0 {
+			temperature = <40000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp0_active1: trip-active1 {
+			temperature = <45000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp0_active2: trip-active2 {
+			temperature = <50000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp0_active3: trip-active3 {
+			temperature = <60000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+	};
+	cooling-maps {
+		map0 {
+			trip = <&cp0_active0>;
+			cooling-device = <&fan 0 1>;
+		};
+		map1 {
+			trip = <&cp0_active1>;
+			cooling-device = <&fan 1 2>;
+		};
+		map2 {
+			trip = <&cp0_active2>;
+			cooling-device = <&fan 2 3>;
+		};
+		map3 {
+			trip = <&cp0_active3>;
+			cooling-device = <&fan 3 4>;
+		};
+		map4 {
+			trip = <&cp0_crit>;
+			cooling-device = <&fan 4 5>;
+		};
+	};
+};
+
+&cp1_thermal_ic {
+	polling-delay = <1000>; /* milliseconds */
+	trips {
+		cp1_active0: trip-active0 {
+			temperature = <40000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp1_active1: trip-active1 {
+			temperature = <45000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp1_active2: trip-active2 {
+			temperature = <50000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+		cp1_active3: trip-active3 {
+			temperature = <60000>; /* millicelsius */
+			hysteresis = <2500>; /* millicelsius */
+			type = "active";
+		};
+	};
+	cooling-maps {
+		map0 {
+			trip = <&cp1_active0>;
+			cooling-device = <&fan 0 1>;
+		};
+		map1 {
+			trip = <&cp1_active1>;
+			cooling-device = <&fan 1 2>;
+		};
+		map2 {
+			trip = <&cp1_active2>;
+			cooling-device = <&fan 2 3>;
+		};
+		map3 {
+			trip = <&cp1_active3>;
+			cooling-device = <&fan 3 4>;
+		};
+		map4 {
+			trip = <&cp1_crit>;
+			cooling-device = <&fan 4 5>;
+		};
+	};
+};
+
 &uart0 {
 	status = "okay";
 	pinctrl-0 = <&uart0_pins>;
-- 
2.20.1

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Linus Walleij]

On Sun, Mar 29, 2020 at 12:46 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:

> This series adds support for the fan PWM output on the Clearfog GT8K
> platform, and can potentially be extended to the Macchiatobin.

The gpio changes all look fine to me +/- fixes for review comments.

Could the MVEBU maintainers provide some feedback?
Curiously the file is only listed as a PWM driver in MAINTAINERS
so formally Thierry & Uwe review it (and Uwe did), but surely
the MVEBU platform maintainers should take a look too.

Yours,
Linus Walleij

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 09:51:37AM +0200, Linus Walleij wrote:
> On Sun, Mar 29, 2020 at 12:46 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> 
> > This series adds support for the fan PWM output on the Clearfog GT8K
> > platform, and can potentially be extended to the Macchiatobin.
> 
> The gpio changes all look fine to me +/- fixes for review comments.

I think Uwe is incorrect for his GPIO comments; the clock is only
optional on A8040.  We know this because A8040 has worked fine
without PWM support without the clock, whereas for Armada 370,
the driver has hard-failed if the clock is not present.

So, on Armada 370, I preserve this behaviour.  I also preserve the
behaviour that on Armada 8040, we don't fail the driver if the
clock is not present so that booting a newer kernel with older DT
still works (which is a requirement.)  In that case, the driver
today still tries to get the clock but never checks the result of
getting the clock (which doesn't exist in current DT files.)

So no, I'm not going to fix Uwe's comments and potentially introduce
regressions into this GPIO driver; I gave up trying to argue the
point with Uwe, and I'm at the point of not giving a damn about this
patch set if I'm to intentionally introduce regressions based on
review comments.

About the only change I would make is to move the check introduced
in patch 2 into patch 3 instead, inside the MVEBU_PWM_SOC_VARIANT_A8K
case, so that deferring for the clock works (which is necessary for
the PWM driver to be useful.)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Linus Walleij]

On Thu, Apr 16, 2020 at 10:14 AM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
> On Thu, Apr 16, 2020 at 09:51:37AM +0200, Linus Walleij wrote:

> > The gpio changes all look fine to me +/- fixes for review comments.
>
> I think Uwe is incorrect for his GPIO comments; the clock is only
> optional on A8040.  We know this because A8040 has worked fine
> without PWM support without the clock, whereas for Armada 370,
> the driver has hard-failed if the clock is not present.

It's fine. You are running the hardware and it should work for you.
I usually go by the IETF motto "rough consensus and running code".

> About the only change I would make is to move the check introduced
> in patch 2 into patch 3 instead, inside the MVEBU_PWM_SOC_VARIANT_A8K
> case, so that deferring for the clock works (which is necessary for
> the PWM driver to be useful.)

OK let's go with this.

Yours,
Linus Walleij

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 02:08:36PM +0200, Linus Walleij wrote:
> On Thu, Apr 16, 2020 at 10:14 AM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> > On Thu, Apr 16, 2020 at 09:51:37AM +0200, Linus Walleij wrote:
> 
> > > The gpio changes all look fine to me +/- fixes for review comments.
> >
> > I think Uwe is incorrect for his GPIO comments; the clock is only
> > optional on A8040.  We know this because A8040 has worked fine
> > without PWM support without the clock, whereas for Armada 370,
> > the driver has hard-failed if the clock is not present.
> 
> It's fine. You are running the hardware and it should work for you.
> I usually go by the IETF motto "rough consensus and running code".
> 
> > About the only change I would make is to move the check introduced
> > in patch 2 into patch 3 instead, inside the MVEBU_PWM_SOC_VARIANT_A8K
> > case, so that deferring for the clock works (which is necessary for
> > the PWM driver to be useful.)
> 
> OK let's go with this.

Well, it turns out to not be particularly nice to do that.  The best
I can come up with is:

        mvchip->clk = devm_clk_get(&pdev->dev, NULL);
        if (soc_variant->pwm && IS_ENABLED(CONFIG_PWM) &&
            mvchip->clk == ERR_PTR(-EPROBE_DEFER))
                return -EPROBE_DEFER;

Doing it in mvebu_pwm_probe() means that we have to deal with unwinding
the very complex probing (tearing down all the interrupt functionality
and GPIO stuff) which the driver currently does not do, even on failure.

Is this a shoddy driver that doesn't clean up after itself...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Andrew Lunn]

On Thu, Apr 16, 2020 at 09:51:37AM +0200, Linus Walleij wrote:
> On Sun, Mar 29, 2020 at 12:46 PM Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> 
> > This series adds support for the fan PWM output on the Clearfog GT8K
> > platform, and can potentially be extended to the Macchiatobin.
> 
> The gpio changes all look fine to me +/- fixes for review comments.
> 
> Could the MVEBU maintainers provide some feedback?

Hi Linus

I took a quick look at this when it was first posted. I also wrote the
PWM support in this driver. The hardware is mostly a GPIO driver, but
it has some basic PWM facilities. It is not possible to cleanly split
it into two drivers, which is why it has the current structure. And
the PWM maintainers ask that the PWM parts be listed in MAINTAINERS as
such, so they got to know about any changes.

Clocking with Marvell devices has always been interesting. Core IP
like this gets reused between different generations of SoCs. The
original Orion5x had no clock control at all. Latter SoCs have had
more and more complex clock trees. So care has to be taken to not
change old behaviour when adding support for new clocks. So Russell
2/6 patch looks good to me, and Uwe request could break on some
SoCs. It would need testing on a lot of SoCs, with and without PWM
support. 

I assume Russell will at some point repost without the RFC tag. At
that point i will take a second look and add Reviewed-by.

     Andrew

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 03:50:39PM +0200, Andrew Lunn wrote:
> On Thu, Apr 16, 2020 at 09:51:37AM +0200, Linus Walleij wrote:
> > On Sun, Mar 29, 2020 at 12:46 PM Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > 
> > > This series adds support for the fan PWM output on the Clearfog GT8K
> > > platform, and can potentially be extended to the Macchiatobin.
> > 
> > The gpio changes all look fine to me +/- fixes for review comments.
> > 
> > Could the MVEBU maintainers provide some feedback?
> 
> Hi Linus
> 
> I took a quick look at this when it was first posted. I also wrote the
> PWM support in this driver. The hardware is mostly a GPIO driver, but
> it has some basic PWM facilities. It is not possible to cleanly split
> it into two drivers, which is why it has the current structure. And
> the PWM maintainers ask that the PWM parts be listed in MAINTAINERS as
> such, so they got to know about any changes.
> 
> Clocking with Marvell devices has always been interesting. Core IP
> like this gets reused between different generations of SoCs. The
> original Orion5x had no clock control at all. Latter SoCs have had
> more and more complex clock trees. So care has to be taken to not
> change old behaviour when adding support for new clocks. So Russell
> 2/6 patch looks good to me, and Uwe request could break on some
> SoCs. It would need testing on a lot of SoCs, with and without PWM
> support. 
> 
> I assume Russell will at some point repost without the RFC tag. At
> that point i will take a second look and add Reviewed-by.

I said in the cover message "The cooling maps are experimental".
They work reasonably well for me with the fan I have (a noctua fan)
but other people may find them to be problematical, so one of the
reasons for sending it as RFC is to get people to test and see how
well it works.

I may have had greater success getting people to test if I'd added
maps for the Macchiatobin, but that wasn't my target system. That's
relatively easy to do - it's the same pin as on the gt8k, so merely
putting the same DT changes onto Macchiatobin would allow testing
with those settings.

Whether that's correct for my Macchiatobin server setup is an
entirely separate problem - where the fan PWM controls front panel
fans and there's a bigger fanless heatsink on the Armada 8040 (as
per the first revision of the Macchiatobin boards).  As the boards
ship without a PWM controllable fan, it would be of limited use.

So, really, I think the DT configuration of the PWM parameters is
"for the user" and not actually for mainline kernels - which
brings with it the problems of understanding control systems,
stability of such systems, feedback, and how to configure the
thermal subsystem... which is not easy.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Andrew Lunn]

> I said in the cover message "The cooling maps are experimental".
> They work reasonably well for me with the fan I have (a noctua fan)
> but other people may find them to be problematical, so one of the
> reasons for sending it as RFC is to get people to test and see how
> well it works.

Hi Russell

Maybe split the PWM parts from the cooling maps? I don't see any
reason not to merge the PWM parts.

Maybe sometime later I might take a closer look at the maps. I have a
wrt1900ac, which is Armada XP based. It has a fan on a GPIO pin. I
currently have a userspace daemon controlling the fan based on hwmon
device data. Getting the kernel to do it all would be nice.

       Andrew

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 04:36:45PM +0200, Andrew Lunn wrote:
> > I said in the cover message "The cooling maps are experimental".
> > They work reasonably well for me with the fan I have (a noctua fan)
> > but other people may find them to be problematical, so one of the
> > reasons for sending it as RFC is to get people to test and see how
> > well it works.
> 
> Hi Russell
> 
> Maybe split the PWM parts from the cooling maps? I don't see any
> reason not to merge the PWM parts.

That's easy - the cooling maps are an entirely separate patch.
However, one thing we need to be careful of is whether someone has a
PWM fan plugged in, and whether not having any cooling maps results
in the fan stopping, as I think PWMs default to being disabled on
initialisation.

That could be a SoC-killing combination.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Robin Murphy]

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]
> Clocking with Marvell devices has always been interesting. Core IP
> like this gets reused between different generations of SoCs. The
> original Orion5x had no clock control at all. Latter SoCs have had
> more and more complex clock trees. So care has to be taken to not
> change old behaviour when adding support for new clocks.

FWIW, that sounds like a good argument for encoding the clock 
requirements of each variant in the of_match_data, so the driver doesn't 
have to simply trust the DT and hope.

Robin.

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Andrew Lunn]

On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
> On 2020-04-16 2:50 pm, Andrew Lunn wrote:
> [...]
> > Clocking with Marvell devices has always been interesting. Core IP
> > like this gets reused between different generations of SoCs. The
> > original Orion5x had no clock control at all. Latter SoCs have had
> > more and more complex clock trees. So care has to be taken to not
> > change old behaviour when adding support for new clocks.
> 
> FWIW, that sounds like a good argument for encoding the clock requirements
> of each variant in the of_match_data, so the driver doesn't have to simply
> trust the DT and hope.

Hi Robin

It is not really hope. It is very obvious when it is wrong, the whole
machine stops dead when you are missing a clock. Very simple to test.

	Andrew

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Robin Murphy]

On 2020-04-16 3:42 pm, Andrew Lunn wrote:
> On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
>> On 2020-04-16 2:50 pm, Andrew Lunn wrote:
>> [...]
>>> Clocking with Marvell devices has always been interesting. Core IP
>>> like this gets reused between different generations of SoCs. The
>>> original Orion5x had no clock control at all. Latter SoCs have had
>>> more and more complex clock trees. So care has to be taken to not
>>> change old behaviour when adding support for new clocks.
>>
>> FWIW, that sounds like a good argument for encoding the clock requirements
>> of each variant in the of_match_data, so the driver doesn't have to simply
>> trust the DT and hope.
> 
> Hi Robin
> 
> It is not really hope. It is very obvious when it is wrong, the whole
> machine stops dead when you are missing a clock. Very simple to test.

Heh, that's still what I meant - the driver hopes that carrying on will 
be OK, and the end user is left to pick up the pieces when it isn't ;)

Obviously that's less of an issue when said end-user is a kernel 
developer making a controlled change during SoC bringup, but perhaps 
more so for an eager inexperienced hacker cobbling together DTS 
fragments to convince a distro kernel to boot on some embedded device 
(even as the former I know I've had enough frustration from unclocked 
registers showing up in unexpected places - the best is when connecting 
an external debugger to see where it's stuck happens to enable the 
offending clock and let the CPU progress to somewhere else by the time 
it actually halts...). If it's possible to make a driver robust enough 
to fail cleanly, isn't that always nicer to debug?

Robin.

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Andrew Lunn]

> > It is not really hope. It is very obvious when it is wrong, the whole
> > machine stops dead when you are missing a clock. Very simple to test.
> 
> Heh, that's still what I meant - the driver hopes that carrying on will be
> OK, and the end user is left to pick up the pieces when it isn't ;)

> Obviously that's less of an issue when said end-user is a kernel developer
> making a controlled change during SoC bringup, but perhaps more so for an
> eager inexperienced hacker cobbling together DTS fragments to convince a
> distro kernel to boot on some embedded device

Clocks are SoC level stuff, so it is in the DTSI file, not the DTS
file. An eager inexperienced hacker cobbling together DTS fragment is
not effected. Experienced kernel hackers have put together the DTSI
file and tested it. And if an eager inexperienced hacker does touch
the DTSI file, they probably deserve to get their finger burnt, and
will get a step closer to become an experience kernel hacker.

     Andrew

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
> On 2020-04-16 2:50 pm, Andrew Lunn wrote:
> [...]
> > Clocking with Marvell devices has always been interesting. Core IP
> > like this gets reused between different generations of SoCs. The
> > original Orion5x had no clock control at all. Latter SoCs have had
> > more and more complex clock trees. So care has to be taken to not
> > change old behaviour when adding support for new clocks.
> 
> FWIW, that sounds like a good argument for encoding the clock requirements
> of each variant in the of_match_data, so the driver doesn't have to simply
> trust the DT and hope.

Please read my patches.  This is exactly what I'm doing.  I'm preserving
as closely as possible the current driver behaviour while adding support
for the Armada 8040 PWM while keeping compatibility with older DT.

And I'm doing that by keying off the match data, exactly as you're
suggesting above.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Robin Murphy]

On 2020-04-16 3:55 pm, Russell King - ARM Linux admin wrote:
> On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
>> On 2020-04-16 2:50 pm, Andrew Lunn wrote:
>> [...]
>>> Clocking with Marvell devices has always been interesting. Core IP
>>> like this gets reused between different generations of SoCs. The
>>> original Orion5x had no clock control at all. Latter SoCs have had
>>> more and more complex clock trees. So care has to be taken to not
>>> change old behaviour when adding support for new clocks.
>>
>> FWIW, that sounds like a good argument for encoding the clock requirements
>> of each variant in the of_match_data, so the driver doesn't have to simply
>> trust the DT and hope.
> 
> Please read my patches.  This is exactly what I'm doing.  I'm preserving
> as closely as possible the current driver behaviour while adding support
> for the Armada 8040 PWM while keeping compatibility with older DT.
> 
> And I'm doing that by keying off the match data, exactly as you're
> suggesting above.

AFAICS you're encoding the *PWM capability* in the match data and using 
that to extend the existing behaviour, which comprises using soc_variant 
to maybe treat the stashed error code as fatal somewhere else much later 
if CONFIG_PWM happens to be enabled, and is subtle enough that at least 
two reviewers overlooked or failed to make sense of it.

Compare and contrast with how self-contained and obvious this is:

-	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
-	/* Not all SoCs require a clock.*/
-	if (!IS_ERR(mvchip->clk))
-		clk_prepare_enable(mvchip->clk);

+	/* Not all SoCs require a clock.*/
+	if (data->needs_clock)
+		mvchip->clk = devm_clk_get(&pdev->dev, NULL);
+		if (IS_ERR(mvchip->clk))
+			return PTR_ERR(mvchip_clk);
+		clk_prepare_enable(mvchip->clk);
+	}

If achieving the same end result by very different and roundabout means 
constitutes "exactly the same thing", does me having written this email 
mean that my house is exactly the same as the Arm office and someone 
else will be along to clean the kitchen shortly? Here's hoping... :D

Robin.

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 04:55:44PM +0100, Robin Murphy wrote:
> On 2020-04-16 3:55 pm, Russell King - ARM Linux admin wrote:
> > On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
> > > On 2020-04-16 2:50 pm, Andrew Lunn wrote:
> > > [...]
> > > > Clocking with Marvell devices has always been interesting. Core IP
> > > > like this gets reused between different generations of SoCs. The
> > > > original Orion5x had no clock control at all. Latter SoCs have had
> > > > more and more complex clock trees. So care has to be taken to not
> > > > change old behaviour when adding support for new clocks.
> > > 
> > > FWIW, that sounds like a good argument for encoding the clock requirements
> > > of each variant in the of_match_data, so the driver doesn't have to simply
> > > trust the DT and hope.
> > 
> > Please read my patches.  This is exactly what I'm doing.  I'm preserving
> > as closely as possible the current driver behaviour while adding support
> > for the Armada 8040 PWM while keeping compatibility with older DT.
> > 
> > And I'm doing that by keying off the match data, exactly as you're
> > suggesting above.
> 
> AFAICS you're encoding the *PWM capability* in the match data and using that
> to extend the existing behaviour, which comprises using soc_variant to maybe
> treat the stashed error code as fatal somewhere else much later if
> CONFIG_PWM happens to be enabled, and is subtle enough that at least two
> reviewers overlooked or failed to make sense of it.
> 
> Compare and contrast with how self-contained and obvious this is:
> 
> -	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> -	/* Not all SoCs require a clock.*/
> -	if (!IS_ERR(mvchip->clk))
> -		clk_prepare_enable(mvchip->clk);
> 
> +	/* Not all SoCs require a clock.*/
> +	if (data->needs_clock)
> +		mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> +		if (IS_ERR(mvchip->clk))
> +			return PTR_ERR(mvchip_clk);
> +		clk_prepare_enable(mvchip->clk);
> +	}
> 
> If achieving the same end result by very different and roundabout means
> constitutes "exactly the same thing", does me having written this email mean
> that my house is exactly the same as the Arm office and someone else will be
> along to clean the kitchen shortly? Here's hoping... :D

What if we have a platform where DT mentions the clock, and relies
on it being enabled as per how the driver is coded today?  I don't
know if that's true or not, I don't have the hardware to test.

So, while we can make improvements as you describe above, it's
dangerous to do so because we don't have the information to know
whether what's being proposed is correct or not.  Hence, it's safer
to do the minimum amount of changes, and not do gratuitous potential
regression causing cleanups as you're suggesting.

If we want to clean up the driver in potentially regression causing
ways, that can be done at a later date.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH RFC 0/6] PWM fan support on Clearfog gt8k

[Russell King - ARM Linux admin]

On Thu, Apr 16, 2020 at 05:37:48PM +0100, Russell King - ARM Linux admin wrote:
> On Thu, Apr 16, 2020 at 04:55:44PM +0100, Robin Murphy wrote:
> > On 2020-04-16 3:55 pm, Russell King - ARM Linux admin wrote:
> > > On Thu, Apr 16, 2020 at 03:37:40PM +0100, Robin Murphy wrote:
> > > > On 2020-04-16 2:50 pm, Andrew Lunn wrote:
> > > > [...]
> > > > > Clocking with Marvell devices has always been interesting. Core IP
> > > > > like this gets reused between different generations of SoCs. The
> > > > > original Orion5x had no clock control at all. Latter SoCs have had
> > > > > more and more complex clock trees. So care has to be taken to not
> > > > > change old behaviour when adding support for new clocks.
> > > > 
> > > > FWIW, that sounds like a good argument for encoding the clock requirements
> > > > of each variant in the of_match_data, so the driver doesn't have to simply
> > > > trust the DT and hope.
> > > 
> > > Please read my patches.  This is exactly what I'm doing.  I'm preserving
> > > as closely as possible the current driver behaviour while adding support
> > > for the Armada 8040 PWM while keeping compatibility with older DT.
> > > 
> > > And I'm doing that by keying off the match data, exactly as you're
> > > suggesting above.
> > 
> > AFAICS you're encoding the *PWM capability* in the match data and using that
> > to extend the existing behaviour, which comprises using soc_variant to maybe
> > treat the stashed error code as fatal somewhere else much later if
> > CONFIG_PWM happens to be enabled, and is subtle enough that at least two
> > reviewers overlooked or failed to make sense of it.
> > 
> > Compare and contrast with how self-contained and obvious this is:
> > 
> > -	mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > -	/* Not all SoCs require a clock.*/
> > -	if (!IS_ERR(mvchip->clk))
> > -		clk_prepare_enable(mvchip->clk);
> > 
> > +	/* Not all SoCs require a clock.*/
> > +	if (data->needs_clock)
> > +		mvchip->clk = devm_clk_get(&pdev->dev, NULL);
> > +		if (IS_ERR(mvchip->clk))
> > +			return PTR_ERR(mvchip_clk);
> > +		clk_prepare_enable(mvchip->clk);
> > +	}
> > 
> > If achieving the same end result by very different and roundabout means
> > constitutes "exactly the same thing", does me having written this email mean
> > that my house is exactly the same as the Arm office and someone else will be
> > along to clean the kitchen shortly? Here's hoping... :D
> 
> What if we have a platform where DT mentions the clock, and relies
> on it being enabled as per how the driver is coded today?  I don't
> know if that's true or not, I don't have the hardware to test.
> 
> So, while we can make improvements as you describe above, it's
> dangerous to do so because we don't have the information to know
> whether what's being proposed is correct or not.  Hence, it's safer
> to do the minimum amount of changes, and not do gratuitous potential
> regression causing cleanups as you're suggesting.
> 
> If we want to clean up the driver in potentially regression causing
> ways, that can be done at a later date.

And, here's proof that such an approach will cause a regression:

arch/arm/boot/dts/armada-375.dtsi:

gpio0: gpio@18100 {
        compatible = "marvell,orion-gpio";
        reg = <0x18100 0x40>;
        ngpios = <32>;
        gpio-controller;
        #gpio-cells = <2>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <GIC_SPI 53 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 54 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 55 IRQ_TYPE_LEVEL_HIGH>,
                     <GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>;
};

Uses "marvell,orion-gpio" compatible, but no clock.

arch/arm/boot/dts/kirkwood.dtsi:

gpio0: gpio@10100 {
        compatible = "marvell,orion-gpio";
        #gpio-cells = <2>;
        gpio-controller;
        reg = <0x10100 0x40>;
        ngpios = <32>;
        interrupt-controller;
        #interrupt-cells = <2>;
        interrupts = <35>, <36>, <37>, <38>;
        clocks = <&gate_clk 7>;
};

Uses "marvell,orion-gpio" compatible, but there is a clock, and the
driver will enable this clock.

So, if the decision about the clock is keyed off the compatible as
you're suggesting it _will_ cause gratuitous regressions.

Random changes to drivers, especially when they have a long history,
always tends to end up causing regressions, which is why it's better
to do as I've done when adding PWM support for the A8040, and only
make the _minimum_ number of changes.  Not to clean up the driver in
random ways to "improve" it, or make it taste better.  Because such
things cause regressions.  As I've said several times in this thread.

Point proven.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up