PWM fan support on Clearfog gt8k

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

Signed-off-by Russell King <[email protected]>

---
 drivers/gpio/gpio-mvebu.c | 3 +++
 1 file changed, 3 insertions(+)

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

index fa5641615db6..ee13b11c5298 100644

Quoted 13 lines · Russell King

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

Signed-off-by Russell King <[email protected]>

---
 drivers/gpio/gpio-mvebu.c | 3 +++
 1 file changed, 3 insertions(+)

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

index fa5641615db6..ee13b11c5298 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1140

Quoted 9 lines

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

@@ -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 ...

Quoted 18 lines · Uwe Kleine-König

Quoted 5 lines · Russell King

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

index fa5641615db6..ee13b11c5298 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1140

Quoted 9 lines

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

@@ -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:

Quoted 3 lines

	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.

Hello Russell,

Quoted 19 lines · Russell King - ARM Linux admin

Quoted 18 lines · Uwe Kleine-König

Quoted 5 lines · Russell King

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

index fa5641615db6..ee13b11c5298 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1140

Quoted 9 lines

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

@@ -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:

Quoted 6 lines

Quoted 3 lines

	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.

Quoted 27 lines · Uwe Kleine-König

Quoted 19 lines · Russell King - ARM Linux admin

Quoted 18 lines · Uwe Kleine-König

Quoted 5 lines · Russell King

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

index fa5641615db6..ee13b11c5298 100644

drivers/gpio/gpio-mvebu.c static int mvebu_gpio_probe(struct platform_device *pdev) · line 1140

Quoted 9 lines

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

@@ -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:

Quoted 6 lines

Quoted 3 lines

	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.

Quoted 6 lines

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.

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 <[email protected]>

---
 drivers/gpio/gpio-mvebu.c | 166 +++++++++++++++++++++++++-------------
 1 file changed, 111 insertions(+), 55 deletions(-)

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

index ee13b11c5298..4abe298e9c0f 100644
@@ -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

drivers/gpio/gpio-mvebu.c +96 -44

 	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;
 

Signature · 1 line

2.20.1

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

Quoted 2 lines · Russell King - ARM Linux admin

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.

Quoted 1 line · Linus Walleij

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.

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.)

Quoted 8 lines · Russell King - ARM Linux admin

Quoted 1 line · Linus Walleij

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".

Quoted 4 lines

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.

Quoted 2 lines · Russell King - ARM Linux admin

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?

Quoted 9 lines · Linus Walleij

Quoted 2 lines · Russell King - ARM Linux admin

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.

Quoted 5 lines

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.

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.

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 8 lines · Robin Murphy

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 11 lines · Robin Murphy

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 13 lines · Russell King - ARM Linux admin

Quoted 11 lines · Robin Murphy

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 6 lines

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:

(quoted diff) +7 -4

-	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

Quoted 42 lines · Robin Murphy

Quoted 13 lines · Russell King - ARM Linux admin

Quoted 11 lines · Robin Murphy

On 2020-04-16 2:50 pm, Andrew Lunn wrote:
[...]

Quoted 5 lines

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.

Quoted 6 lines

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:

(quoted diff) +7 -4

-	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);
+	}

Quoted 4 lines

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.