| Message ID | 20201009153031.986-4-nsaenzjulienne@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Raspberry Pi 4 PoE HAT fan support | expand |
Hello, On Fri, Oct 09, 2020 at 05:30:30PM +0200, Nicolas Saenz Julienne wrote: > Adds support to control the PWM bus available in official Raspberry Pi > PoE HAT. Only RPi's co-processor has access to it, so commands have to > be sent through RPi's firmware mailbox interface. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/pwm/Kconfig | 7 ++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi.c | 216 ++++++++++++++++++++++++++++++++++ > 3 files changed, 224 insertions(+) > create mode 100644 drivers/pwm/pwm-raspberrypi.c > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 63be5362fd3a..a76997ca37d0 100644 > --- a/drivers/pwm/Kconfig > +++ b/drivers/pwm/Kconfig > @@ -379,6 +379,13 @@ config PWM_PXA > To compile this driver as a module, choose M here: the module > will be called pwm-pxa. > > +config PWM_RASPBERRYPI > + tristate "Raspberry Pi Firwmware PWM support" s/Firwmware/Firmware/ > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) This is more complicated than necessary. depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST is logically equivalent. > + help > + Enable Raspberry Pi firmware controller PWM bus used to control the > + official RPI PoE hat > + > config PWM_RCAR > tristate "Renesas R-Car PWM support" > depends on ARCH_RENESAS || COMPILE_TEST > diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile > index cbdcd55d69ee..b557b549d9f3 100644 > --- a/drivers/pwm/Makefile > +++ b/drivers/pwm/Makefile > @@ -35,6 +35,7 @@ obj-$(CONFIG_PWM_MXS) += pwm-mxs.o > obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o > obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o > obj-$(CONFIG_PWM_PXA) += pwm-pxa.o > +obj-$(CONFIG_PWM_RASPBERRYPI) += pwm-raspberrypi.o > obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o > obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o > obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o > diff --git a/drivers/pwm/pwm-raspberrypi.c b/drivers/pwm/pwm-raspberrypi.c > new file mode 100644 > index 000000000000..1ccff6b1ae34 > --- /dev/null > +++ b/drivers/pwm/pwm-raspberrypi.c > @@ -0,0 +1,216 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright 2020 Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > + */ Please add a paragraph here about the hardware. See pwm-sifive.c for a template. (Please stick to the format there to simplify grepping.) The things to point out there are: - No disable bit, so a disabled PWM is simulated by duty_cycle 0 - Only normal polarity - Fixed period Also add a note about if the currently running period is completed when the hardware is reconfigured. If possible please also add a link to a product page and/or documentation. > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > + > +#include <soc/bcm2835/raspberrypi-firmware.h> > +#include <dt-bindings/pwm/raspberrypi,firmware-pwm.h> > + > +#define RPI_PWM_MAX_DUTY 255 > +#define RPI_PWM_PERIOD_NS 80000 /* 12.5KHz */ 12.5 kHz > +#define RPI_PWM_CUR_DUTY_REG 0x0 > +#define RPI_PWM_DEF_DUTY_REG 0x1 > + > +struct raspberrypi_pwm { > + struct rpi_firmware *firmware; > + struct pwm_chip chip; > + unsigned int duty_cycle; > +}; > + > +struct raspberrypi_pwm_prop { > + __le32 reg; > + __le32 val; > + __le32 ret; > +} __packed; > + > +static inline struct raspberrypi_pwm *to_raspberrypi_pwm(struct pwm_chip *chip) > +{ > + return container_of(chip, struct raspberrypi_pwm, chip); > +} > + > +static int raspberrypi_pwm_set_property(struct rpi_firmware *firmware, > + u32 reg, u32 val) > +{ > + struct raspberrypi_pwm_prop msg = { > + .reg = cpu_to_le32(reg), > + .val = cpu_to_le32(val), > + }; > + int ret; > + > + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_SET_POE_HAT_VAL, > + &msg, sizeof(msg)); > + if (ret) > + return ret; > + else if (msg.ret) > + return -EIO; > + > + return 0; > +} > + > +static int raspberrypi_pwm_get_property(struct rpi_firmware *firmware, > + u32 reg, u32 *val) > +{ > + struct raspberrypi_pwm_prop msg = { > + .reg = reg > + }; > + int ret; > + > + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_GET_POE_HAT_VAL, > + &msg, sizeof(msg)); > + if (ret) > + return ret; > + else if (msg.ret) > + return -EIO; > + > + *val = le32_to_cpu(msg.val); > + > + return 0; > +} > + > +static void raspberrypi_pwm_get_state(struct pwm_chip *chip, > + struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip); > + > + state->period = RPI_PWM_PERIOD_NS; > + state->duty_cycle = pc->duty_cycle * RPI_PWM_PERIOD_NS / RPI_PWM_MAX_DUTY; Please round up the result of the division. (The idea is that if you apply the state .get_state() returns this should yield no change.) > + state->enabled = !!(pc->duty_cycle); > + state->polarity = PWM_POLARITY_NORMAL; > +} > + > +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip); > + unsigned int duty_cycle; > + int ret; > + You need to check for polarity here. > + if (!state->enabled) > + duty_cycle = 0; > + else > + duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / > + RPI_PWM_PERIOD_NS; > + > + if (duty_cycle == pc->duty_cycle) > + return 0; > + > + pc->duty_cycle = duty_cycle; > + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, > + pc->duty_cycle); > + if (ret) { > + dev_err(chip->dev, "Failed to set duty cycle: %d\n", ret); > + return ret; > + } What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY? I think the right thing to do here is: if (state->period < RPI_PWM_PERIOD_NS || state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; if (!state->enabled) duty_cycle = 0 else if (state->duty_cycle < RPI_PWM_PERIOD_NS) duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / RPI_PWM_PERIOD_NS; else duty_cycle = RPI_PWM_MAX_DUTY; ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, pc->duty_cycle); if (ret) ... pc->duty_cycle = duty_cycle; > + > + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, > + pc->duty_cycle); > + if (ret) { > + dev_err(chip->dev, "Failed to set default duty cycle: %d\n", ret); > + return ret; > + } Huh, why do you have to do this twice, just with different error messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the effect of writing this property? > + > + return 0; > +} > + > +static const struct pwm_ops raspberrypi_pwm_ops = { > + .get_state = raspberrypi_pwm_get_state, > + .apply = raspberrypi_pwm_apply, > + .owner = THIS_MODULE, > +}; > + > +static struct pwm_device *raspberrypi_pwm_xlate(struct pwm_chip *pc, > + const struct of_phandle_args *args) > +{ > + struct pwm_device *pwm; > + > + if (args->args[0] >= pc->npwm) > + return ERR_PTR(-EINVAL); > + > + pwm = pwm_request_from_chip(pc, args->args[0], NULL); > + if (IS_ERR(pwm)) > + return pwm; > + > + /* Firmwre won't let us change the period */ Firmware. > + pwm->args.period = RPI_PWM_PERIOD_NS; > + > + return pwm; > +} I think you don't need this function. Just fix up period in .apply(). > +static int raspberrypi_pwm_probe(struct platform_device *pdev) > +{ > + struct device_node *firmware_node; > + struct device *dev = &pdev->dev; > + struct rpi_firmware *firmware; > + struct raspberrypi_pwm *pc; What does "pc" stand for? I'd have used "rpipwm" or something similar. > + int ret; > + > + firmware_node = of_get_parent(dev->of_node); > + if (!firmware_node) { > + dev_err(dev, "Missing firmware node\n"); > + return -ENOENT; > + } > + > + firmware = rpi_firmware_get(firmware_node); > + of_node_put(firmware_node); > + if (!firmware) > + return -EPROBE_DEFER; I don't see a mechanism that prevents the driver providing the firmware going away while the PWM is still in use. > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > + if (!pc) > + return -ENOMEM; > [...] > + > +static struct platform_driver raspberrypi_pwm_driver = { > + .driver = { > + .name = "raspberrypi-pwm", > + .of_match_table = raspberrypi_pwm_of_match, > + }, > + .probe = raspberrypi_pwm_probe, > + .remove = raspberrypi_pwm_remove, > +}; > +module_platform_driver(raspberrypi_pwm_driver); > + > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de>"); > +MODULE_DESCRIPTION("Raspberry Pi Firwmare Based PWM Bus Driver"); > +MODULE_LICENSE("GPL v2"); > + Please drop the empty line at the end of file. Best regards Uwe
Hi Uwe, On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote: > Hello, > > On Fri, Oct 09, 2020 at 05:30:30PM +0200, Nicolas Saenz Julienne wrote: > > Adds support to control the PWM bus available in official Raspberry Pi > > PoE HAT. Only RPi's co-processor has access to it, so commands have to > > be sent through RPi's firmware mailbox interface. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/pwm/Kconfig | 7 ++ > > drivers/pwm/Makefile | 1 + > > drivers/pwm/pwm-raspberrypi.c | 216 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 224 insertions(+) > > create mode 100644 drivers/pwm/pwm-raspberrypi.c > > > > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > > index 63be5362fd3a..a76997ca37d0 100644 > > --- a/drivers/pwm/Kconfig > > +++ b/drivers/pwm/Kconfig > > @@ -379,6 +379,13 @@ config PWM_PXA > > To compile this driver as a module, choose M here: the module > > will be called pwm-pxa. > > > > +config PWM_RASPBERRYPI > > + tristate "Raspberry Pi Firwmware PWM support" > > s/Firwmware/Firmware/ Noted, Sorry for that. > > > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) > > This is more complicated than necessary. > > depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST > > is logically equivalent. It's not exactly the same, see patch 7ed915059c300 ("gpio: raspberrypi-ext: fix firmware dependency ") which explains why this pattern is needed. [...] > > +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > + const struct pwm_state *state) > > +{ > > + struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip); > > + unsigned int duty_cycle; > > + int ret; > > + > > You need to check for polarity here. > > > + if (!state->enabled) > > + duty_cycle = 0; > > + else > > + duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / > > + RPI_PWM_PERIOD_NS; > > + > > + if (duty_cycle == pc->duty_cycle) > > + return 0; > > + > > + pc->duty_cycle = duty_cycle; > > + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, > > + pc->duty_cycle); > > + if (ret) { > > + dev_err(chip->dev, "Failed to set duty cycle: %d\n", ret); > > + return ret; > > + } > > What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY? > > I think the right thing to do here is: > > if (state->period < RPI_PWM_PERIOD_NS || Why not using state->period != RPI_PWM_PERIOD_NS here? > state->polarity != PWM_POLARITY_NORMAL) > return -EINVAL; > > if (!state->enabled) > duty_cycle = 0 > else if (state->duty_cycle < RPI_PWM_PERIOD_NS) > duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / RPI_PWM_PERIOD_NS; > else > duty_cycle = RPI_PWM_MAX_DUTY; > > ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, > pc->duty_cycle); > if (ret) > ... > > pc->duty_cycle = duty_cycle; OK, clearly better this way. > > + > > + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, > > + pc->duty_cycle); > > + if (ret) { > > + dev_err(chip->dev, "Failed to set default duty cycle: %d\n", ret); > > + return ret; > > + } > > Huh, why do you have to do this twice, just with different error > messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the > effect of writing this property? Obviously, I failed to change the register name. This is supposed to set the default duty cycle after resetting the board. I added it so as to keep compatibility with the downstream version of this. I'll add a comment to explain this. [...] > > + pwm->args.period = RPI_PWM_PERIOD_NS; > > + > > + return pwm; > > +} > > I think you don't need this function. Just fix up period in .apply(). As commented in the dt binding patch, I'll do that. > > +static int raspberrypi_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device_node *firmware_node; > > + struct device *dev = &pdev->dev; > > + struct rpi_firmware *firmware; > > + struct raspberrypi_pwm *pc; > > What does "pc" stand for? I'd have used "rpipwm" or something similar. It was cargo culted from other pwm drivers, I saw it being used on more than one and figured it was the preferred way of naming things. I'll change it. > > > + int ret; > > + > > + firmware_node = of_get_parent(dev->of_node); > > + if (!firmware_node) { > > + dev_err(dev, "Missing firmware node\n"); > > + return -ENOENT; > > + } > > + > > + firmware = rpi_firmware_get(firmware_node); > > + of_node_put(firmware_node); > > + if (!firmware) > > + return -EPROBE_DEFER; > > I don't see a mechanism that prevents the driver providing the firmware > going away while the PWM is still in use. There isn't an explicit one. But since you depend on a symbol from the firmware driver you won't be able to remove the kernel module before removing the PMW one. > > + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); > > + if (!pc) > > + return -ENOMEM; > > [...] > > + > > +static struct platform_driver raspberrypi_pwm_driver = { > > + .driver = { > > + .name = "raspberrypi-pwm", > > + .of_match_table = raspberrypi_pwm_of_match, > > + }, > > + .probe = raspberrypi_pwm_probe, > > + .remove = raspberrypi_pwm_remove, > > +}; > > +module_platform_driver(raspberrypi_pwm_driver); > > + > > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de>"); > > +MODULE_DESCRIPTION("Raspberry Pi Firwmare Based PWM Bus Driver"); > > +MODULE_LICENSE("GPL v2"); > > + > > Please drop the empty line at the end of file. Overall I took note of your comments and I'll make the changes. Thanks for the review. Regards, Nicolas
Hello Nicolas, On Tue, Oct 13, 2020 at 01:20:00PM +0200, Nicolas Saenz Julienne wrote: > On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote: > > > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) > > > > This is more complicated than necessary. > > > > depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST > > > > is logically equivalent. > > It's not exactly the same, see patch 7ed915059c300 ("gpio: raspberrypi-ext: fix > firmware dependency ") which explains why this pattern is needed. Hmm, this is strange, but if Arnd doesn't know a better solution, then be it so. Is this idiom usual enough to not justify a comment? > > What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY? > > > > I think the right thing to do here is: > > > > if (state->period < RPI_PWM_PERIOD_NS || > > Why not using state->period != RPI_PWM_PERIOD_NS here? From the consumer's point of view having to hit the only correct period is hard. So the usual convention is to provide the biggest period not bigger than the requested one. (The idea for the future is to provide a pwm_round_state() function which allows to find out the effect of pwm_apply_state() with the same arguments. This then allows to efficiently find the best setting for the consumer.) > > Huh, why do you have to do this twice, just with different error > > messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the > > effect of writing this property? > > Obviously, I failed to change the register name. > > This is supposed to set the default duty cycle after resetting the board. I > added it so as to keep compatibility with the downstream version of this. > > I'll add a comment to explain this. fine. > > > + int ret; > > > + > > > + firmware_node = of_get_parent(dev->of_node); > > > + if (!firmware_node) { > > > + dev_err(dev, "Missing firmware node\n"); > > > + return -ENOENT; > > > + } > > > + > > > + firmware = rpi_firmware_get(firmware_node); > > > + of_node_put(firmware_node); > > > + if (!firmware) > > > + return -EPROBE_DEFER; > > > > I don't see a mechanism that prevents the driver providing the firmware > > going away while the PWM is still in use. > > There isn't an explicit one. But since you depend on a symbol from the firmware > driver you won't be able to remove the kernel module before removing the PMW > one. this prevents the that the module is unloaded, but not that the driver is unbound. Best regards Uwe
Hi Uwe, On Tue, 2020-10-13 at 14:17 +0200, Uwe Kleine-König wrote: > Hello Nicolas, > > On Tue, Oct 13, 2020 at 01:20:00PM +0200, Nicolas Saenz Julienne wrote: > > On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote: > > > > + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) > > > > > > This is more complicated than necessary. > > > > > > depends on RASPBERRYPI_FIRMWARE || COMPILE_TEST > > > > > > is logically equivalent. > > > > It's not exactly the same, see patch 7ed915059c300 ("gpio: raspberrypi-ext: fix > > firmware dependency ") which explains why this pattern is needed. I'll add a comment. > Hmm, this is strange, but if Arnd doesn't know a better solution, then > be it so. Is this idiom usual enough to not justify a comment? > > > > What happens if duty_cycle happens to be bigger than RPI_PWM_MAX_DUTY? > > > > > > I think the right thing to do here is: > > > > > > if (state->period < RPI_PWM_PERIOD_NS || > > > > Why not using state->period != RPI_PWM_PERIOD_NS here? > > From the consumer's point of view having to hit the only correct period > is hard. So the usual convention is to provide the biggest period not > bigger than the requested one. (The idea for the future is to provide a > pwm_round_state() function which allows to find out the effect of > pwm_apply_state() with the same arguments. This then allows to > efficiently find the best setting for the consumer.) Fair enough. > > > Huh, why do you have to do this twice, just with different error > > > messages? I assume you want to set RPI_PWM_DEF_DUTY_REG? What is the > > > effect of writing this property? > > > > Obviously, I failed to change the register name. > > > > This is supposed to set the default duty cycle after resetting the board. I > > added it so as to keep compatibility with the downstream version of this. > > > > I'll add a comment to explain this. > > fine. > > > > > + int ret; > > > > + > > > > + firmware_node = of_get_parent(dev->of_node); > > > > + if (!firmware_node) { > > > > + dev_err(dev, "Missing firmware node\n"); > > > > + return -ENOENT; > > > > + } > > > > + > > > > + firmware = rpi_firmware_get(firmware_node); > > > > + of_node_put(firmware_node); > > > > + if (!firmware) > > > > + return -EPROBE_DEFER; > > > > > > I don't see a mechanism that prevents the driver providing the firmware > > > going away while the PWM is still in use. > > > > There isn't an explicit one. But since you depend on a symbol from the firmware > > driver you won't be able to remove the kernel module before removing the PMW > > one. > > this prevents the that the module is unloaded, but not that the driver > is unbound. Yes, if you were to unbind the firmware device all devices that depend on it (there are a bunch of them) would access freed memory. Yet again, there is no hotplug functionality, so short of being useful for development it'd be very rare for someone to unbind it. We've been living with it as such for a long time. Not to say that is something not to fix, but from my perspective it's just a corner-case. We are using 'simple-mfd' in order to probe all devices under the firmware interface, so my first intuition would be to add support for automatically unbinding of consumer devices in of/platform.c. See: diff --git a/drivers/of/platform.c b/drivers/of/platform.c index b557a0fcd4ba..d24f2412d518 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -390,7 +390,13 @@ static int of_platform_bus_create(struct device_node *bus, } dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); - if (!dev || !of_match_node(matches, bus)) + if (!dev) + return 0; + + if (parent && of_device_is_compatible(parent->of_node, "simple-mfd")) + device_link_add(&dev->dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER); + + if (!of_match_node(matches, bus)) return 0; for_each_child_of_node(bus, child) { If this is too much for OF maintainers, we could simply create the link upon calling rpi_firmware_get(). This solves the problem of getting a kernel panic because of the use after free, but you'll still get some warnings after unbinding from the GPIO subsystem, for example, as we just removed a gpiochip that still has consumers up. I guess device links only go so far. Any ideas/comments? Regards, Nicolas
Hello Nicolas, [Cc: += Greg as base driver maintainer] On Tue, Oct 13, 2020 at 06:50:47PM +0200, Nicolas Saenz Julienne wrote: > On Tue, 2020-10-13 at 14:17 +0200, Uwe Kleine-König wrote: > > On Tue, Oct 13, 2020 at 01:20:00PM +0200, Nicolas Saenz Julienne wrote: > > > On Mon, 2020-10-12 at 09:06 +0200, Uwe Kleine-König wrote: > > > > I don't see a mechanism that prevents the driver providing the firmware > > > > going away while the PWM is still in use. > > > > > > There isn't an explicit one. But since you depend on a symbol from the firmware > > > driver you won't be able to remove the kernel module before removing the PMW > > > one. > > > > this prevents the that the module is unloaded, but not that the driver > > is unbound. > > Yes, if you were to unbind the firmware device all devices that depend on it > (there are a bunch of them) would access freed memory. Yet again, there is no > hotplug functionality, so short of being useful for development it'd be very > rare for someone to unbind it. We've been living with it as such for a long > time. Not to say that is something not to fix, but from my perspective it's > just a corner-case. I agree, that's a corner case. However in my eyes it is one that should be get right. Did you try if this is indeed a problem? > We are using 'simple-mfd' in order to probe all devices under the > firmware interface, so my first intuition would be to add support for > automatically unbinding of consumer devices in of/platform.c. See: > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index b557a0fcd4ba..d24f2412d518 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -390,7 +390,13 @@ static int of_platform_bus_create(struct device_node *bus, > } > > dev = of_platform_device_create_pdata(bus, bus_id, platform_data, parent); > - if (!dev || !of_match_node(matches, bus)) > + if (!dev) > + return 0; > + > + if (parent && of_device_is_compatible(parent->of_node, "simple-mfd")) > + device_link_add(&dev->dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER); > + > + if (!of_match_node(matches, bus)) > return 0; > > for_each_child_of_node(bus, child) { This looks wrong for generic code. A solution local to simple-mfd (or even the firmware device?) should be done (and doable). I think the straight forward approach would be to add reference counting and make .remove of the firmware device block if there are still users. (Returning an error doesn't prevent the device going away IIRC. Last time I looked .remove returning something non-0 didn't make any difference. Maybe we should change it to return void?) > If this is too much for OF maintainers, we could simply create the link upon > calling rpi_firmware_get(). I don't know how DL_FLAG_AUTOREMOVE_CONSUMER works, but this sounds better. > This solves the problem of getting a kernel panic because of the use after > free, but you'll still get some warnings after unbinding from the GPIO > subsystem, for example, as we just removed a gpiochip that still has consumers > up. I guess device links only go so far. If this is indeed a real problem, lets take this to the GPIO maintainers. Best regards Uwe
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 63be5362fd3a..a76997ca37d0 100644 --- a/drivers/pwm/Kconfig +++ b/drivers/pwm/Kconfig @@ -379,6 +379,13 @@ config PWM_PXA To compile this driver as a module, choose M here: the module will be called pwm-pxa. +config PWM_RASPBERRYPI + tristate "Raspberry Pi Firwmware PWM support" + depends on RASPBERRYPI_FIRMWARE || (COMPILE_TEST && !RASPBERRYPI_FIRMWARE) + help + Enable Raspberry Pi firmware controller PWM bus used to control the + official RPI PoE hat + config PWM_RCAR tristate "Renesas R-Car PWM support" depends on ARCH_RENESAS || COMPILE_TEST diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile index cbdcd55d69ee..b557b549d9f3 100644 --- a/drivers/pwm/Makefile +++ b/drivers/pwm/Makefile @@ -35,6 +35,7 @@ obj-$(CONFIG_PWM_MXS) += pwm-mxs.o obj-$(CONFIG_PWM_OMAP_DMTIMER) += pwm-omap-dmtimer.o obj-$(CONFIG_PWM_PCA9685) += pwm-pca9685.o obj-$(CONFIG_PWM_PXA) += pwm-pxa.o +obj-$(CONFIG_PWM_RASPBERRYPI) += pwm-raspberrypi.o obj-$(CONFIG_PWM_RCAR) += pwm-rcar.o obj-$(CONFIG_PWM_RENESAS_TPU) += pwm-renesas-tpu.o obj-$(CONFIG_PWM_ROCKCHIP) += pwm-rockchip.o diff --git a/drivers/pwm/pwm-raspberrypi.c b/drivers/pwm/pwm-raspberrypi.c new file mode 100644 index 000000000000..1ccff6b1ae34 --- /dev/null +++ b/drivers/pwm/pwm-raspberrypi.c @@ -0,0 +1,216 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright 2020 Nicolas Saenz Julienne <nsaenzjulienne@suse.de> + */ + +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> + +#include <soc/bcm2835/raspberrypi-firmware.h> +#include <dt-bindings/pwm/raspberrypi,firmware-pwm.h> + +#define RPI_PWM_MAX_DUTY 255 +#define RPI_PWM_PERIOD_NS 80000 /* 12.5KHz */ + +#define RPI_PWM_CUR_DUTY_REG 0x0 +#define RPI_PWM_DEF_DUTY_REG 0x1 + +struct raspberrypi_pwm { + struct rpi_firmware *firmware; + struct pwm_chip chip; + unsigned int duty_cycle; +}; + +struct raspberrypi_pwm_prop { + __le32 reg; + __le32 val; + __le32 ret; +} __packed; + +static inline struct raspberrypi_pwm *to_raspberrypi_pwm(struct pwm_chip *chip) +{ + return container_of(chip, struct raspberrypi_pwm, chip); +} + +static int raspberrypi_pwm_set_property(struct rpi_firmware *firmware, + u32 reg, u32 val) +{ + struct raspberrypi_pwm_prop msg = { + .reg = cpu_to_le32(reg), + .val = cpu_to_le32(val), + }; + int ret; + + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_SET_POE_HAT_VAL, + &msg, sizeof(msg)); + if (ret) + return ret; + else if (msg.ret) + return -EIO; + + return 0; +} + +static int raspberrypi_pwm_get_property(struct rpi_firmware *firmware, + u32 reg, u32 *val) +{ + struct raspberrypi_pwm_prop msg = { + .reg = reg + }; + int ret; + + ret = rpi_firmware_property(firmware, RPI_FIRMWARE_GET_POE_HAT_VAL, + &msg, sizeof(msg)); + if (ret) + return ret; + else if (msg.ret) + return -EIO; + + *val = le32_to_cpu(msg.val); + + return 0; +} + +static void raspberrypi_pwm_get_state(struct pwm_chip *chip, + struct pwm_device *pwm, + struct pwm_state *state) +{ + struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip); + + state->period = RPI_PWM_PERIOD_NS; + state->duty_cycle = pc->duty_cycle * RPI_PWM_PERIOD_NS / RPI_PWM_MAX_DUTY; + state->enabled = !!(pc->duty_cycle); + state->polarity = PWM_POLARITY_NORMAL; +} + +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct raspberrypi_pwm *pc = to_raspberrypi_pwm(chip); + unsigned int duty_cycle; + int ret; + + if (!state->enabled) + duty_cycle = 0; + else + duty_cycle = state->duty_cycle * RPI_PWM_MAX_DUTY / + RPI_PWM_PERIOD_NS; + + if (duty_cycle == pc->duty_cycle) + return 0; + + pc->duty_cycle = duty_cycle; + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, + pc->duty_cycle); + if (ret) { + dev_err(chip->dev, "Failed to set duty cycle: %d\n", ret); + return ret; + } + + ret = raspberrypi_pwm_set_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, + pc->duty_cycle); + if (ret) { + dev_err(chip->dev, "Failed to set default duty cycle: %d\n", ret); + return ret; + } + + return 0; +} + +static const struct pwm_ops raspberrypi_pwm_ops = { + .get_state = raspberrypi_pwm_get_state, + .apply = raspberrypi_pwm_apply, + .owner = THIS_MODULE, +}; + +static struct pwm_device *raspberrypi_pwm_xlate(struct pwm_chip *pc, + const struct of_phandle_args *args) +{ + struct pwm_device *pwm; + + if (args->args[0] >= pc->npwm) + return ERR_PTR(-EINVAL); + + pwm = pwm_request_from_chip(pc, args->args[0], NULL); + if (IS_ERR(pwm)) + return pwm; + + /* Firmwre won't let us change the period */ + pwm->args.period = RPI_PWM_PERIOD_NS; + + return pwm; +} + +static int raspberrypi_pwm_probe(struct platform_device *pdev) +{ + struct device_node *firmware_node; + struct device *dev = &pdev->dev; + struct rpi_firmware *firmware; + struct raspberrypi_pwm *pc; + int ret; + + firmware_node = of_get_parent(dev->of_node); + if (!firmware_node) { + dev_err(dev, "Missing firmware node\n"); + return -ENOENT; + } + + firmware = rpi_firmware_get(firmware_node); + of_node_put(firmware_node); + if (!firmware) + return -EPROBE_DEFER; + + pc = devm_kzalloc(&pdev->dev, sizeof(*pc), GFP_KERNEL); + if (!pc) + return -ENOMEM; + + pc->firmware = firmware; + + pc->chip.dev = dev; + pc->chip.ops = &raspberrypi_pwm_ops; + pc->chip.of_xlate = raspberrypi_pwm_xlate; + pc->chip.of_pwm_n_cells = 1; + pc->chip.base = -1; + pc->chip.npwm = RASPBERRYPI_FIRMWARE_PWM_NUM; + + platform_set_drvdata(pdev, pc); + + ret = raspberrypi_pwm_get_property(pc->firmware, RPI_PWM_CUR_DUTY_REG, + &pc->duty_cycle); + if (ret) { + dev_err(dev, "Failed to get duty cycle: %d\n", ret); + return ret; + } + + return pwmchip_add(&pc->chip); +} + +static int raspberrypi_pwm_remove(struct platform_device *pdev) +{ + struct raspberrypi_pwm *pc = platform_get_drvdata(pdev); + + return pwmchip_remove(&pc->chip); +} + +static const struct of_device_id raspberrypi_pwm_of_match[] = { + { .compatible = "raspberrypi,firmware-pwm", }, + { } +}; +MODULE_DEVICE_TABLE(of, raspberrypi_pwm_of_match); + +static struct platform_driver raspberrypi_pwm_driver = { + .driver = { + .name = "raspberrypi-pwm", + .of_match_table = raspberrypi_pwm_of_match, + }, + .probe = raspberrypi_pwm_probe, + .remove = raspberrypi_pwm_remove, +}; +module_platform_driver(raspberrypi_pwm_driver); + +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de>"); +MODULE_DESCRIPTION("Raspberry Pi Firwmare Based PWM Bus Driver"); +MODULE_LICENSE("GPL v2"); +
Adds support to control the PWM bus available in official Raspberry Pi PoE HAT. Only RPi's co-processor has access to it, so commands have to be sent through RPi's firmware mailbox interface. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/pwm/Kconfig | 7 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-raspberrypi.c | 216 ++++++++++++++++++++++++++++++++++ 3 files changed, 224 insertions(+) create mode 100644 drivers/pwm/pwm-raspberrypi.c