| Message ID | 20201211164801.7838-1-nsaenzjulienne@suse.de |
|---|---|
| Headers | show |
| Series | Raspberry Pi PoE HAT fan support | expand |
On Fri, Dec 11, 2020 at 5:48 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > It'll simplify the firmware handling for most consumers. > > Suggested-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- > > Changes since v4: > - Rearrange function calls for clarity, same functionality > > Changes since v2: > - Create devm_rpi_firmware_get() > > drivers/firmware/raspberrypi.c | 29 ++++++++++++++++++++++ > include/soc/bcm2835/raspberrypi-firmware.h | 8 ++++++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > index b65e4c495772..250e01680742 100644 > --- a/drivers/firmware/raspberrypi.c > +++ b/drivers/firmware/raspberrypi.c > @@ -243,6 +243,13 @@ void rpi_firmware_put(struct rpi_firmware *fw) > } > EXPORT_SYMBOL_GPL(rpi_firmware_put); > > +static void devm_rpi_firmware_put(void *data) > +{ > + struct rpi_firmware *fw = data; > + > + rpi_firmware_put(fw); > +} > + > static int rpi_firmware_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -331,6 +338,28 @@ struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > } > EXPORT_SYMBOL_GPL(rpi_firmware_get); > > +/** > + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure. > + * @firmware_node: Pointer to the firmware Device Tree node. > + * > + * Returns NULL is the firmware device is not ready. > + */ > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > + struct device_node *firmware_node) > +{ > + struct rpi_firmware *fw; > + > + fw = rpi_firmware_get(firmware_node); > + if (!fw) > + return NULL; > + > + if (devm_add_action_or_reset(dev, devm_rpi_firmware_put, fw)) > + return NULL; > + > + return fw; > +} > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get); > + > static const struct of_device_id rpi_firmware_of_match[] = { > { .compatible = "raspberrypi,bcm2835-firmware", }, > {}, > diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h > index fdfef7fe40df..73ad784fca96 100644 > --- a/include/soc/bcm2835/raspberrypi-firmware.h > +++ b/include/soc/bcm2835/raspberrypi-firmware.h > @@ -142,6 +142,8 @@ int rpi_firmware_property_list(struct rpi_firmware *fw, > void *data, size_t tag_size); > void rpi_firmware_put(struct rpi_firmware *fw); > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node); > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > + struct device_node *firmware_node); > #else > static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, > void *data, size_t len) > @@ -160,6 +162,12 @@ static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware > { > return NULL; > } > + > +static inline struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > + struct device_node *firmware_node) > +{ > + return NULL; > +} > #endif > > #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */ > -- > 2.29.2 > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
On Wed, 2020-12-16 at 11:35 +0100, Bartosz Golaszewski wrote: > On Fri, Dec 11, 2020 at 5:48 PM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > > > It'll simplify the firmware handling for most consumers. > > > > Suggested-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > > > Changes since v4: > > - Rearrange function calls for clarity, same functionality > > > > Changes since v2: > > - Create devm_rpi_firmware_get() > > > > drivers/firmware/raspberrypi.c | 29 ++++++++++++++++++++++ > > include/soc/bcm2835/raspberrypi-firmware.h | 8 ++++++ > > 2 files changed, 37 insertions(+) > > > > diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c > > index b65e4c495772..250e01680742 100644 > > --- a/drivers/firmware/raspberrypi.c > > +++ b/drivers/firmware/raspberrypi.c > > @@ -243,6 +243,13 @@ void rpi_firmware_put(struct rpi_firmware *fw) > > } > > EXPORT_SYMBOL_GPL(rpi_firmware_put); > > > > +static void devm_rpi_firmware_put(void *data) > > +{ > > + struct rpi_firmware *fw = data; > > + > > + rpi_firmware_put(fw); > > +} > > + > > static int rpi_firmware_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > @@ -331,6 +338,28 @@ struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node) > > } > > EXPORT_SYMBOL_GPL(rpi_firmware_get); > > > > +/** > > + * devm_rpi_firmware_get - Get pointer to rpi_firmware structure. > > + * @firmware_node: Pointer to the firmware Device Tree node. > > + * > > + * Returns NULL is the firmware device is not ready. > > + */ > > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > > + struct device_node *firmware_node) > > +{ > > + struct rpi_firmware *fw; > > + > > + fw = rpi_firmware_get(firmware_node); > > + if (!fw) > > + return NULL; > > + > > + if (devm_add_action_or_reset(dev, devm_rpi_firmware_put, fw)) > > + return NULL; > > + > > + return fw; > > +} > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get); > > + > > static const struct of_device_id rpi_firmware_of_match[] = { > > { .compatible = "raspberrypi,bcm2835-firmware", }, > > {}, > > diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h > > index fdfef7fe40df..73ad784fca96 100644 > > --- a/include/soc/bcm2835/raspberrypi-firmware.h > > +++ b/include/soc/bcm2835/raspberrypi-firmware.h > > @@ -142,6 +142,8 @@ int rpi_firmware_property_list(struct rpi_firmware *fw, > > void *data, size_t tag_size); > > void rpi_firmware_put(struct rpi_firmware *fw); > > struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node); > > +struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > > + struct device_node *firmware_node); > > #else > > static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag, > > void *data, size_t len) > > @@ -160,6 +162,12 @@ static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware > > { > > return NULL; > > } > > + > > +static inline struct rpi_firmware *devm_rpi_firmware_get(struct device *dev, > > + struct device_node *firmware_node) > > +{ > > + return NULL; > > +} > > #endif > > > > #endif /* __SOC_RASPBERRY_FIRMWARE_H__ */ > > -- > > 2.29.2 > > > > Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> Thanks for the reviews!
On Fri, Dec 11, 2020 at 5:48 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > When unbinding the firmware device we need to make sure it has no > consumers left. Otherwise we'd leave them with a firmware handle > pointing at freed memory. > > Keep a reference count of all consumers and introduce rpi_firmware_put() > which will permit automatically decrease the reference count upon > unbinding consumer drivers. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > --- Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
On Fri, 2020-12-11 at 17:47 +0100, Nicolas Saenz Julienne wrote: > The aim of this series is to add support to the fan found on RPi's PoE > HAT. Some commentary on the design can be found below. But the imporant > part to the people CC'd here not involved with PWM is that, in order to > achieve this properly, we also have to fix the firmware interface the > driver uses to communicate with the PWM bus (and many other low level > functions). Specifically, we have to make sure the firmware interface > isn't unbound while consumers are still up. So, patch #1 & #2 introduce > reference counting in the firwmware interface driver and patches #3 to > #8 update all firmware users. Patches #9 to #11 introduce the new PWM > driver. > > I sent everything as a single series as the final version of the PWM > drivers depends on the firwmare fixes, but I'll be happy to split this > into two separate series if you think it's better. > > --- Original cover letter below --- > > This series aims at adding support to RPi's official PoE HAT fan[1]. > > The HW setup is the following: > > > Raspberry Pi | PoE HAT | > arm core -> Mailbox -> RPi co-processor -> I2C -> Atmel MCU -> PWM -> FAN > > The arm cores have only access to the mailbox interface, as i2c0, even if > physically accessible, is to be used solely by the co-processor > (VideoCore 4/6). > > This series implements a PWM bus, and has pwm-fan sitting on top of it as per > this discussion: https://lkml.org/lkml/2018/9/2/486. Although this design has a > series of shortcomings: > > - It depends on a DT binding: it's not flexible if a new hat shows up with new > functionality, we're not 100% sure we'll be able to expand it without > breaking backwards compatibility. But without it we can't make use of DT > thermal-zones, which IMO is overkill. > > - We're using pwm-fan, writing a hwmon driver would, again, give us more > flexibility, but it's not really needed at the moment. > > I personally think that it's not worth the effort, it's unlikely we'll get > things right in advance. And ultimately, if the RPi people come up with > something new, we can always write a new driver/bindings from scratch (as in > not reusing previous code). > > That said, I'm more than happy to change things if there is a consensus that > another design will do the trick. > > [1] https://www.raspberrypi.org/blog/introducing-power-over-ethernet-poe-hat/ > > --- I'd say at this point the series is pretty clean and, AFAIK, there aren't any objections. I'm not so sure who should take it, given that it covers numerous subsystems. Any suggestions on how to handle it? Regards, Nicolas > Changes since v5: > - Small cleanups > - Add extra code comments > > Changes since v4: > - Cleanup devm calls > - Rename compatible string so it's unique to the PoE HAT > > Changes since v3: > - Split first patch, #1 introduces refcount, then #2 the devm function > - Fix touchscreen function > - Use kref > > Changes since v2: > - Introduce devm_rpi_firmware_get() > - Small cleanups in PWM driver > > Changes since v1: > - Address PWM driver changes > - Fix binding, now with 2 cells > > Nicolas Saenz Julienne (11): > firmware: raspberrypi: Keep count of all consumers > firmware: raspberrypi: Introduce devm_rpi_firmware_get() > clk: bcm: rpi: Release firmware handle on unbind > gpio: raspberrypi-exp: Release firmware handle on unbind > reset: raspberrypi: Release firmware handle on unbind > soc: bcm: raspberrypi-power: Release firmware handle on unbind > staging: vchiq: Release firmware handle on unbind > input: raspberrypi-ts: Release firmware handle when not needed > dt-bindings: pwm: Add binding for RPi firmware PWM bus > DO NOT MERGE: ARM: dts: Add RPi's official PoE hat support > pwm: Add Raspberry Pi Firmware based PWM bus > > .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 20 ++ > arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 54 +++++ > drivers/clk/bcm/clk-raspberrypi.c | 2 +- > drivers/firmware/raspberrypi.c | 69 +++++- > drivers/gpio/gpio-raspberrypi-exp.c | 2 +- > drivers/input/touchscreen/raspberrypi-ts.c | 2 +- > drivers/pwm/Kconfig | 9 + > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-raspberrypi-poe.c | 216 ++++++++++++++++++ > drivers/reset/reset-raspberrypi.c | 2 +- > drivers/soc/bcm/raspberrypi-power.c | 2 +- > .../interface/vchiq_arm/vchiq_arm.c | 2 +- > .../pwm/raspberrypi,firmware-poe-pwm.h | 13 ++ > include/soc/bcm2835/raspberrypi-firmware.h | 10 + > 14 files changed, 395 insertions(+), 9 deletions(-) > create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c > create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h >
On Mon, Jan 11, 2021 at 10:02 PM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > I'd say at this point the series is pretty clean and, AFAIK, there aren't any > objections. I'm not so sure who should take it, given that it covers numerous > subsystems. Any suggestions on how to handle it? This is one of those cases where I would suggest collect ACKs from affected subsystem maintainers and send a pull request to the SoC tree for this hairy bundle. Yours, Linus Walleij
Hi Uwe, thanks for the review. On Tue, 2021-01-12 at 10:18 +0100, Uwe Kleine-König wrote: [...] > > + duty_cycle = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * RPI_PWM_MAX_DUTY, > > + RPI_PWM_PERIOD_NS); > > ... and round down here. > > Just to be sure: writing RPI_PWM_MAX_DUTY (i.e. 255) yields 100% duty > cycle, right? Yes, at 255 the signal is flat. > > + else > > + duty_cycle = RPI_PWM_MAX_DUTY; > > + > > + if (duty_cycle == rpipwm->duty_cycle) > > + return 0; > > + > > + ret = raspberrypi_pwm_set_property(rpipwm->firmware, RPI_PWM_CUR_DUTY_REG, > > + duty_cycle); > > + if (ret) { > > + dev_err(chip->dev, "Failed to set duty cycle: %d\n", ret); > > + return ret; > > + } > > + > > + /* > > + * This sets the default duty cycle after resetting the board, we > > + * updated it every time to mimic Raspberry Pi's downstream's driver > > + * behaviour. > > + */ > > + ret = raspberrypi_pwm_set_property(rpipwm->firmware, RPI_PWM_DEF_DUTY_REG, > > + duty_cycle); > > + if (ret) { > > + dev_err(chip->dev, "Failed to set default duty cycle: %d\n", ret); > > + return ret; > > + } > > + > > + rpipwm->duty_cycle = duty_cycle; > > Please use tabs for indention. (The general hint is to use checkpatch > which (I hope) tells you about problems like this.) Sorry for that. I took note of the rest of comments and will update the code. Regards, Nicolas