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 v1:
- Address PWM driver changes
- Fix binding, now with 2 cells
- Add reference count to rpi_firmware_get()
Nicolas Saenz Julienne (10):
firmware: raspberrypi: Introduce rpi_firmware_put()
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 | 1 +
drivers/firmware/raspberrypi.c | 30 ++-
drivers/gpio/gpio-raspberrypi-exp.c | 14 +-
drivers/input/touchscreen/raspberrypi-ts.c | 1 +
drivers/pwm/Kconfig | 9 +
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-raspberrypi.c | 221 ++++++++++++++++++
drivers/reset/reset-raspberrypi.c | 13 +-
drivers/soc/bcm/raspberrypi-power.c | 15 ++
.../interface/vchiq_arm/vchiq_arm.c | 3 +
.../pwm/raspberrypi,firmware-pwm.h | 13 ++
include/soc/bcm2835/raspberrypi-firmware.h | 3 +
14 files changed, 395 insertions(+), 3 deletions(-)
create mode 100644 drivers/pwm/pwm-raspberrypi.c
create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-pwm.h
--
2.28.0
Upon unbinding the device make sure we release RPi's firmware interface.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
index 01125d9f991b..dfa4d144faa8 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2771,11 +2771,14 @@ static int vchiq_probe(struct platform_device *pdev)
static int vchiq_remove(struct platform_device *pdev)
{
+ struct vchiq_drvdata *drvdata = platform_get_drvdata(pdev);
+
platform_device_unregister(bcm2835_audio);
platform_device_unregister(bcm2835_camera);
vchiq_debugfs_deinit();
device_destroy(vchiq_class, vchiq_devid);
cdev_del(&vchiq_cdev);
+ rpi_firmware_put(drvdata->fw);
return 0;
}
--
2.28.0
Upon unbinding the device make sure we release RPi's firmware interface.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
drivers/soc/bcm/raspberrypi-power.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/drivers/soc/bcm/raspberrypi-power.c b/drivers/soc/bcm/raspberrypi-power.c
index 5d1aacdd84ef..a0b38db5886c 100644
--- a/drivers/soc/bcm/raspberrypi-power.c
+++ b/drivers/soc/bcm/raspberrypi-power.c
@@ -225,6 +225,20 @@ static int rpi_power_probe(struct platform_device *pdev)
return 0;
}
+static int rpi_power_remove(struct platform_device *pdev)
+{
+ struct rpi_power_domains *rpi_domains = platform_get_drvdata(pdev);
+
+ of_genpd_del_provider(dev->of_node);
+
+ for (i = 0; i < RPI_POWER_DOMAIN_COUNT; i++)
+ pm_genpd_remove(&rpi_domains->domains[i].base);
+
+ rpi_firmware_put(rpi_domaina->fw);
+
+ return 0;
+}
+
static const struct of_device_id rpi_power_of_match[] = {
{ .compatible = "raspberrypi,bcm2835-power", },
{},
@@ -237,6 +251,7 @@ static struct platform_driver rpi_power_driver = {
.of_match_table = rpi_power_of_match,
},
.probe = rpi_power_probe,
+ .remove = rpi_power_remove,
};
builtin_platform_driver(rpi_power_driver);
--
2.28.0
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 make sure they all finished
unbinding before we do.
Suggested-by: Uwe Kleine-König <[email protected]>
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
@Uwe: I didn't found it necessary to call 'try_module_get()' as the rest
of modules depend on the 'rpi_firmware_get/put()' symbols which already
block users from unloading the module prematurely.
drivers/firmware/raspberrypi.c | 30 +++++++++++++++++++++-
include/soc/bcm2835/raspberrypi-firmware.h | 3 +++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 2371d08bdd17..e5ba609e3985 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,6 +282,8 @@ 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;
@@ -289,14 +298,33 @@ static int rpi_firmware_remove(struct platform_device *pdev)
struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node)
{
struct platform_device *pdev = of_find_device_by_node(firmware_node);
+ struct rpi_firmware *fw;
if (!pdev)
return NULL;
- return platform_get_drvdata(pdev);
+ fw = platform_get_drvdata(pdev);
+ if (!fw)
+ return NULL;
+
+ if (!refcount_inc_not_zero(&fw->consumers))
+ return NULL;
+
+ return fw;
}
EXPORT_SYMBOL_GPL(rpi_firmware_get);
+/**
+ * rpi_firmware_put - Put pointer to rpi_firmware structure.
+ * @rpi_firmware: Pointer to struct rpi_firmware
+ */
+void rpi_firmware_put(struct rpi_firmware *fw)
+{
+ refcount_dec(&fw->consumers);
+ wake_up(&fw->wait);
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_put);
+
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..7836ea51fbdf 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -141,6 +141,7 @@ 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);
+void rpi_firmware_put(struct rpi_firmware *fw);
#else
static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
void *data, size_t len)
@@ -158,6 +159,8 @@ static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware
{
return NULL;
}
+
+void rpi_firmware_put(struct rpi_firmware *fw) { }
#endif
#endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
--
2.28.0
This is an example on how to enable the fan on top of RPi's official PoE
hat.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
---
Changes since v1:
- Update patch to use 2 pwm cells
arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 54 +++++++++++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
index 09a1182c2936..361db82619a4 100644
--- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
+++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
@@ -5,6 +5,7 @@
#include "bcm283x-rpi-usb-peripheral.dtsi"
#include <dt-bindings/reset/raspberrypi,firmware-reset.h>
+#include <dt-bindings/pwm/raspberrypi,firmware-pwm.h>
/ {
compatible = "raspberrypi,4-model-b", "brcm,bcm2711";
@@ -68,6 +69,54 @@ sd_vcc_reg: sd_vcc_reg {
enable-active-high;
gpio = <&expgpio 6 GPIO_ACTIVE_HIGH>;
};
+
+ fan: pwm-fan {
+ compatible = "pwm-fan";
+ cooling-levels = <0 50 150 255>;
+ #cooling-cells = <2>;
+ pwms = <&fwpwm RASPBERRYPI_FIRMWARE_PWM_POE 80000>;
+ };
+
+ thermal-zones {
+ cpu_thermal: cpu-thermal {
+ trips {
+ threshold: trip-point@0 {
+ temperature = <45000>;
+ hysteresis = <5000>;
+ type = "active";
+ };
+
+ target: trip-point@1 {
+ temperature = <50000>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+
+ cpu_hot: cpu_hot@0 {
+ temperature = <55000>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <&threshold>;
+ cooling-device = <&fan 0 1>;
+ };
+
+ map1 {
+ trip = <&target>;
+ cooling-device = <&fan 1 2>;
+ };
+
+ map2 {
+ trip = <&cpu_hot>;
+ cooling-device = <&fan 2 3>;
+ };
+ };
+ };
+ };
};
&ddc0 {
@@ -103,6 +152,11 @@ reset: reset {
compatible = "raspberrypi,firmware-reset";
#reset-cells = <1>;
};
+
+ fwpwm: pwm {
+ compatible = "raspberrypi,firmware-pwm";
+ #pwm-cells = <2>;
+ };
};
&gpio {
--
2.28.0
On Thu, Oct 22, 2020 at 9:06 PM Nicolas Saenz Julienne
<[email protected]> 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 make sure they all finished
> unbinding before we do.
Wait, if it's a device, why do we need all these?
get_device() / put_device() along with module_get() / module_put()
should be sufficient, no?
--
With Best Regards,
Andy Shevchenko
On Thu, 2020-10-22 at 17:58 +0200, Nicolas Saenz Julienne wrote:
> Upon unbinding the device make sure we release RPi's firmware interface.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> ---
> drivers/soc/bcm/raspberrypi-power.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/soc/bcm/raspberrypi-power.c b/drivers/soc/bcm/raspberrypi-power.c
> index 5d1aacdd84ef..a0b38db5886c 100644
> --- a/drivers/soc/bcm/raspberrypi-power.c
> +++ b/drivers/soc/bcm/raspberrypi-power.c
> @@ -225,6 +225,20 @@ static int rpi_power_probe(struct platform_device *pdev)
> return 0;
> }
>
> +static int rpi_power_remove(struct platform_device *pdev)
> +{
> + struct rpi_power_domains *rpi_domains = platform_get_drvdata(pdev);
> +
> + of_genpd_del_provider(dev->of_node);
> +
> + for (i = 0; i < RPI_POWER_DOMAIN_COUNT; i++)
> + pm_genpd_remove(&rpi_domains->domains[i].base);
> +
> + rpi_firmware_put(rpi_domaina->fw);
I Just realised I failed to squash a fix for this patch, so this will not
build. Sorry for that.
Regards,
Nicolas
> +
> + return 0;
> +}
> +
> static const struct of_device_id rpi_power_of_match[] = {
> { .compatible = "raspberrypi,bcm2835-power", },
> {},
> @@ -237,6 +251,7 @@ static struct platform_driver rpi_power_driver = {
> .of_match_table = rpi_power_of_match,
> },
> .probe = rpi_power_probe,
> + .remove = rpi_power_remove,
> };
> builtin_platform_driver(rpi_power_driver);
>
On Thu, 2020-10-22 at 21:46 +0300, Andy Shevchenko wrote:
> On Thu, Oct 22, 2020 at 9:06 PM Nicolas Saenz Julienne
> <[email protected]> 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 make sure they all finished
> > unbinding before we do.
>
> Wait, if it's a device, why do we need all these?
> get_device() / put_device() along with module_get() / module_put()
> should be sufficient, no?
Could you expand here a little, I do see how I could use get_device()'s
reference count. But it seems to me I'd be digging way too deep into kobj in
order to get the functionality I need.
If you meant to say that it should be automatically taken care by the platform
bus, just FYI we're using 'simple-mfd' in DT. Where firmware supplier is the
parent and all consumers are children.
Regards,
Nicolas