mbox series

[v3,00/11] Raspberry Pi PoE HAT fan support

Message ID 20201104103938.1286-1-nsaenzjulienne@suse.de
Headers show
Series Raspberry Pi PoE HAT fan support | expand

Message

Nicolas Saenz Julienne Nov. 4, 2020, 10:39 a.m. UTC
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 introduces
reference counting in the firwmware interface driver and patches #2 to
#7 update all firmware users. Patches #8 to #10 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/

---

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
 - Add reference count to rpi_firmware_get()

Nicolas Saenz Julienne (11):
  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
  firmware: raspberrypi: Get rid of rpi_firmware_get()
  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                |  37 ++-
 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-pwm.h            |  13 ++
 include/soc/bcm2835/raspberrypi-firmware.h    |   6 +-
 14 files changed, 356 insertions(+), 12 deletions(-)
 create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c
 create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-pwm.h

Comments

Bartosz Golaszewski Nov. 5, 2020, 9:13 a.m. UTC | #1
On Wed, Nov 4, 2020 at 11:39 AM 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
> devm_rpi_firmware_get() which will 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>
>
> ---
>
> Changes since v2:
>  - Create devm_rpi_firmware_get()
>
>  drivers/firmware/raspberrypi.c             | 46 ++++++++++++++++++++++
>  include/soc/bcm2835/raspberrypi-firmware.h |  8 ++++
>  2 files changed, 54 insertions(+)
>
> diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
> index 2371d08bdd17..74bdb3bde9dc 100644
> --- a/drivers/firmware/raspberrypi.c
> +++ b/drivers/firmware/raspberrypi.c
> @@ -11,7 +11,9 @@
>  #include <linux/module.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/refcount.h>
>  #include <linux/slab.h>
> +#include <linux/wait.h>
>  #include <soc/bcm2835/raspberrypi-firmware.h>
>
>  #define MBOX_MSG(chan, data28)         (((data28) & ~0xf) | ((chan) & 0xf))
> @@ -27,6 +29,9 @@ struct rpi_firmware {
>         struct mbox_chan *chan; /* The property channel. */
>         struct completion c;
>         u32 enabled;
> +
> +       refcount_t consumers;
> +       wait_queue_head_t wait;
>  };
>
>  static DEFINE_MUTEX(transaction_lock);
> @@ -247,6 +252,8 @@ static int rpi_firmware_probe(struct platform_device *pdev)
>         }
>
>         init_completion(&fw->c);
> +       refcount_set(&fw->consumers, 1);
> +       init_waitqueue_head(&fw->wait);
>
>         platform_set_drvdata(pdev, fw);
>
> @@ -275,11 +282,21 @@ static int rpi_firmware_remove(struct platform_device *pdev)
>         rpi_hwmon = NULL;
>         platform_device_unregister(rpi_clk);
>         rpi_clk = NULL;
> +
> +       wait_event(fw->wait, refcount_dec_if_one(&fw->consumers));
>         mbox_free_channel(fw->chan);
>
>         return 0;
>  }
>
> +static void rpi_firmware_put(void *data)
> +{
> +       struct rpi_firmware *fw = data;
> +
> +       refcount_dec(&fw->consumers);
> +       wake_up(&fw->wait);
> +}
> +
>  /**
>   * rpi_firmware_get - Get pointer to rpi_firmware structure.
>   * @firmware_node:    Pointer to the firmware Device Tree node.
> @@ -297,6 +314,35 @@ 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 platform_device *pdev = of_find_device_by_node(firmware_node);
> +       struct rpi_firmware *fw;
> +
> +       if (!pdev)
> +               return NULL;
> +
> +       fw = platform_get_drvdata(pdev);
> +       if (!fw)
> +               return NULL;
> +
> +       if (!refcount_inc_not_zero(&fw->consumers))
> +               return NULL;
> +
> +       if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> +               return NULL;
> +
> +       return fw;
> +}
> +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);

Usually I'd expect the devres variant to simply call
rpi_firmware_get() and then schedule a release callback which would
call whatever function is the release counterpart for it currently.
Devres actions are for drivers which want to schedule some more
unusual tasks at driver detach. Any reason for designing it this way?

Bartosz

> +
>  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 cc9cdbc66403..8fe64f53a394 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -141,6 +141,8 @@ int rpi_firmware_property(struct rpi_firmware *fw,
>  int rpi_firmware_property_list(struct rpi_firmware *fw,
>                                void *data, size_t tag_size);
>  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)
> @@ -158,6 +160,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.1
>
Nicolas Saenz Julienne Nov. 5, 2020, 9:28 a.m. UTC | #2
Hi Bartosz, thanks for the review.

On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:
> > +/**
> > + * 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 platform_device *pdev = of_find_device_by_node(firmware_node);
> > +       struct rpi_firmware *fw;
> > +
> > +       if (!pdev)
> > +               return NULL;
> > +
> > +       fw = platform_get_drvdata(pdev);
> > +       if (!fw)
> > +               return NULL;
> > +
> > +       if (!refcount_inc_not_zero(&fw->consumers))
> > +               return NULL;
> > +
> > +       if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> > +               return NULL;
> > +
> > +       return fw;
> > +}
> > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> 
> Usually I'd expect the devres variant to simply call
> rpi_firmware_get() and then schedule a release callback which would
> call whatever function is the release counterpart for it currently.
> Devres actions are for drivers which want to schedule some more
> unusual tasks at driver detach. Any reason for designing it this way?

Yes, see patch #8 where I get rid of rpi_firmware_get() altogether after
converting all users to devres. Since there is no use for the vanilla version
of the function anymore, I figured it'd be better to merge everything into
devm_rpi_firmware_get(). That said it's not something I have strong feelings
about.

Regards,
Nicolas
Bartosz Golaszewski Nov. 5, 2020, 9:42 a.m. UTC | #3
On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne
<nsaenzjulienne@suse.de> wrote:
>
> Hi Bartosz, thanks for the review.
>
> On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:
> > > +/**
> > > + * 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 platform_device *pdev = of_find_device_by_node(firmware_node);
> > > +       struct rpi_firmware *fw;
> > > +
> > > +       if (!pdev)
> > > +               return NULL;
> > > +
> > > +       fw = platform_get_drvdata(pdev);
> > > +       if (!fw)
> > > +               return NULL;
> > > +
> > > +       if (!refcount_inc_not_zero(&fw->consumers))
> > > +               return NULL;
> > > +
> > > +       if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> > > +               return NULL;
> > > +
> > > +       return fw;
> > > +}
> > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> >
> > Usually I'd expect the devres variant to simply call
> > rpi_firmware_get() and then schedule a release callback which would
> > call whatever function is the release counterpart for it currently.
> > Devres actions are for drivers which want to schedule some more
> > unusual tasks at driver detach. Any reason for designing it this way?
>
> Yes, see patch #8 where I get rid of rpi_firmware_get() altogether after
> converting all users to devres. Since there is no use for the vanilla version
> of the function anymore, I figured it'd be better to merge everything into
> devm_rpi_firmware_get(). That said it's not something I have strong feelings
> about.
>

I see. So the previous version didn't really have any reference
counting and it leaked the reference returned by
of_find_device_by_node(), got it. Could you just clarify for me the
logic behind the wait_queue in rpi_firmware_remove()? If the firmware
driver gets detached and remove() stops on the wait_queue - it will be
stuck until the last user releases the firmware. I'm not sure this is
correct. I'd prefer to see a kref with a release callback and remove
would simply decrease the kref and return. Each user would do the same
and then after the last user is detached the firmware would be
destroyed.

Don't we really have some centralized firmware subsystem that would handle this?

Bartosz
Nicolas Saenz Julienne Nov. 10, 2020, 1:38 p.m. UTC | #4
Hi Bartosz, thanks for the feedback.

On Thu, 2020-11-05 at 10:42 +0100, Bartosz Golaszewski wrote:
> On Thu, Nov 5, 2020 at 10:28 AM Nicolas Saenz Julienne
> <nsaenzjulienne@suse.de> wrote:
> > Hi Bartosz, thanks for the review.
> > 
> > On Thu, 2020-11-05 at 10:13 +0100, Bartosz Golaszewski wrote:
> > > > +/**
> > > > + * 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 platform_device *pdev = of_find_device_by_node(firmware_node);
> > > > +       struct rpi_firmware *fw;
> > > > +
> > > > +       if (!pdev)
> > > > +               return NULL;
> > > > +
> > > > +       fw = platform_get_drvdata(pdev);
> > > > +       if (!fw)
> > > > +               return NULL;
> > > > +
> > > > +       if (!refcount_inc_not_zero(&fw->consumers))
> > > > +               return NULL;
> > > > +
> > > > +       if (devm_add_action_or_reset(dev, rpi_firmware_put, fw))
> > > > +               return NULL;
> > > > +
> > > > +       return fw;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(devm_rpi_firmware_get);
> > > 
> > > Usually I'd expect the devres variant to simply call
> > > rpi_firmware_get() and then schedule a release callback which would
> > > call whatever function is the release counterpart for it currently.
> > > Devres actions are for drivers which want to schedule some more
> > > unusual tasks at driver detach. Any reason for designing it this way?
> > 
> > Yes, see patch #8 where I get rid of rpi_firmware_get() altogether after
> > converting all users to devres. Since there is no use for the vanilla version
> > of the function anymore, I figured it'd be better to merge everything into
> > devm_rpi_firmware_get(). That said it's not something I have strong feelings
> > about.
> > 
> 
> I see. So the previous version didn't really have any reference
> counting and it leaked the reference returned by
> of_find_device_by_node(), got it. Could you just clarify for me the
> logic behind the wait_queue in rpi_firmware_remove()? If the firmware
> driver gets detached and remove() stops on the wait_queue - it will be
> stuck until the last user releases the firmware. I'm not sure this is
> correct.

Yes, that's what I meant to implement.

> I'd prefer to see a kref with a release callback and remove
> would simply decrease the kref and return. Each user would do the same
> and then after the last user is detached the firmware would be
> destroyed.

Sounds good to me. I'll update it.

> Don't we really have some centralized firmware subsystem that would handle this?

Sadly no, this is an RPi specific thing, it doesn't live behind a standard like
other firmware based protocols (for ex. SCMI), and evolves as the needs arise.

Regards,
Nicolas