| Message ID | 20200329104549.GX25745@shell.armlinux.org.uk |
|---|---|
| Headers | show |
| Series | PWM fan support on Clearfog gt8k | expand |
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
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.)
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
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
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.
> 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
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.
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.
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
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...
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.
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.
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.
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.
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.
> > 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