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 important
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 firmware 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 firmware 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 v6:
- Address PWM driver comments
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 | 220 ++++++++++++++++++
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, 399 insertions(+), 9 deletions(-)
create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c
create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h
--
2.29.2
Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
interface when unbinding the device.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Acked-by: Bartosz Golaszewski <[email protected]>
---
drivers/gpio/gpio-raspberrypi-exp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-raspberrypi-exp.c b/drivers/gpio/gpio-raspberrypi-exp.c
index bb100e0124e6..64a552ecc2ad 100644
--- a/drivers/gpio/gpio-raspberrypi-exp.c
+++ b/drivers/gpio/gpio-raspberrypi-exp.c
@@ -208,7 +208,7 @@ static int rpi_exp_gpio_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(fw_node);
+ fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
of_node_put(fw_node);
if (!fw)
return -EPROBE_DEFER;
--
2.29.2
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 <[email protected]>
---
Changes since v6:
- Use %pe
- Round divisions properly
- Use dev_err_probe()
- Pass check_patch
Changes since v3:
- Rename compatible string to be more explicit WRT to bus's limitations
Changes since v2:
- Use devm_rpi_firmware_get()
- Rename driver
- Small cleanups
Changes since v1:
- Use default pwm bindings and get rid of xlate() function
- Correct spelling errors
- Correct apply() function
- Round values
- Fix divisions in arm32 mode
- Small cleanups
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-raspberrypi-poe.c | 220 ++++++++++++++++++++++++++++++
3 files changed, 230 insertions(+)
create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0937e1c047ac..75e2344703b3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -423,6 +423,15 @@ config PWM_PXA
To compile this driver as a module, choose M here: the module
will be called pwm-pxa.
+config PWM_RASPBERRYPI_POE
+ tristate "Raspberry Pi Firwmware PoE Hat PWM support"
+ # Make sure not 'y' when RASPBERRYPI_FIRMWARE is 'm'. This can only
+ # happen when COMPILE_TEST=y, hence the added !RASPBERRYPI_FIRMWARE.
+ 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 18b89d7fd092..ed28d7bd4c64 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -38,6 +38,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_POE) += pwm-raspberrypi-poe.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-poe.c b/drivers/pwm/pwm-raspberrypi-poe.c
new file mode 100644
index 000000000000..ca845e8fabe6
--- /dev/null
+++ b/drivers/pwm/pwm-raspberrypi-poe.c
@@ -0,0 +1,220 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Nicolas Saenz Julienne <[email protected]>
+ * For more information on Raspberry Pi's PoE hat see:
+ * https://www.raspberrypi.org/products/poe-hat/
+ *
+ * Limitations:
+ * - No disable bit, so a disabled PWM is simulated by duty_cycle 0
+ * - Only normal polarity
+ * - Fixed 12.5 kHz period
+ *
+ * The current period is completed when HW is reconfigured.
+ */
+
+#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-poe-pwm.h>
+
+#define RPI_PWM_MAX_DUTY 255
+#define RPI_PWM_PERIOD_NS 80000 /* 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 *raspberrypi_pwm_from_chip(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;
+ 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;
+ 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 *rpipwm = raspberrypi_pwm_from_chip(chip);
+
+ state->period = RPI_PWM_PERIOD_NS;
+ state->duty_cycle = DIV_ROUND_UP(rpipwm->duty_cycle * RPI_PWM_PERIOD_NS,
+ RPI_PWM_MAX_DUTY);
+ state->enabled = !!(rpipwm->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 *rpipwm = raspberrypi_pwm_from_chip(chip);
+ unsigned int duty_cycle;
+ int ret;
+
+ 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 = DIV_ROUND_DOWN_ULL(state->duty_cycle * RPI_PWM_MAX_DUTY,
+ RPI_PWM_PERIOD_NS);
+ 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: %pe\n",
+ ERR_PTR(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: %pe\n",
+ ERR_PTR(ret));
+ return ret;
+ }
+
+ rpipwm->duty_cycle = duty_cycle;
+
+ return 0;
+}
+
+static const struct pwm_ops raspberrypi_pwm_ops = {
+ .get_state = raspberrypi_pwm_get_state,
+ .apply = raspberrypi_pwm_apply,
+ .owner = THIS_MODULE,
+};
+
+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 *rpipwm;
+ int ret;
+
+ firmware_node = of_get_parent(dev->of_node);
+ if (!firmware_node) {
+ dev_err(dev, "Missing firmware node\n");
+ return -ENOENT;
+ }
+
+ firmware = devm_rpi_firmware_get(&pdev->dev, firmware_node);
+ of_node_put(firmware_node);
+ if (!firmware)
+ return dev_err_probe(dev, -EPROBE_DEFER,
+ "Failed to get firmware handle\n");
+
+ rpipwm = devm_kzalloc(&pdev->dev, sizeof(*rpipwm), GFP_KERNEL);
+ if (!rpipwm)
+ return -ENOMEM;
+
+ rpipwm->firmware = firmware;
+ rpipwm->chip.dev = dev;
+ rpipwm->chip.ops = &raspberrypi_pwm_ops;
+ rpipwm->chip.base = -1;
+ rpipwm->chip.npwm = RASPBERRYPI_FIRMWARE_PWM_NUM;
+
+ platform_set_drvdata(pdev, rpipwm);
+
+ ret = raspberrypi_pwm_get_property(rpipwm->firmware, RPI_PWM_CUR_DUTY_REG,
+ &rpipwm->duty_cycle);
+ if (ret) {
+ dev_err(dev, "Failed to get duty cycle: %pe\n", ERR_PTR(ret));
+ return ret;
+ }
+
+ return pwmchip_add(&rpipwm->chip);
+}
+
+static int raspberrypi_pwm_remove(struct platform_device *pdev)
+{
+ struct raspberrypi_pwm *rpipwm = platform_get_drvdata(pdev);
+
+ return pwmchip_remove(&rpipwm->chip);
+}
+
+static const struct of_device_id raspberrypi_pwm_of_match[] = {
+ { .compatible = "raspberrypi,firmware-poe-pwm", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, raspberrypi_pwm_of_match);
+
+static struct platform_driver raspberrypi_pwm_driver = {
+ .driver = {
+ .name = "raspberrypi-poe-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 <[email protected]>");
+MODULE_DESCRIPTION("Raspberry Pi Firmware Based PWM Bus Driver");
+MODULE_LICENSE("GPL v2");
--
2.29.2
The PWM bus controlling the fan in RPi's official PoE hat can only be
controlled by the board's co-processor.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
Changes since v4:
- Rename compatible string to be more explicit with the bus' limitations
Changes since v3:
- Fix example
Changes since v1:
- Update bindings to use 2 #pwm-cells
.../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 20 +++++++++++++++++++
.../pwm/raspberrypi,firmware-poe-pwm.h | 13 ++++++++++++
2 files changed, 33 insertions(+)
create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h
diff --git a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
index a2c63c8b1d10..e3664eab0f6a 100644
--- a/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
+++ b/Documentation/devicetree/bindings/arm/bcm/raspberrypi,bcm2835-firmware.yaml
@@ -64,6 +64,21 @@ properties:
- compatible
- "#reset-cells"
+ pwm:
+ type: object
+
+ properties:
+ compatible:
+ const: raspberrypi,firmware-poe-pwm
+
+ "#pwm-cells":
+ # See pwm.yaml in this directory for a description of the cells format.
+ const: 2
+
+ required:
+ - compatible
+ - "#pwm-cells"
+
additionalProperties: false
required:
@@ -87,5 +102,10 @@ examples:
compatible = "raspberrypi,firmware-reset";
#reset-cells = <1>;
};
+
+ pwm: pwm {
+ compatible = "raspberrypi,firmware-poe-pwm";
+ #pwm-cells = <2>;
+ };
};
...
diff --git a/include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h b/include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h
new file mode 100644
index 000000000000..27c5ce68847b
--- /dev/null
+++ b/include/dt-bindings/pwm/raspberrypi,firmware-poe-pwm.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2020 Nicolas Saenz Julienne
+ * Author: Nicolas Saenz Julienne <[email protected]>
+ */
+
+#ifndef _DT_BINDINGS_RASPBERRYPI_FIRMWARE_PWM_H
+#define _DT_BINDINGS_RASPBERRYPI_FIRMWARE_PWM_H
+
+#define RASPBERRYPI_FIRMWARE_PWM_POE 0
+#define RASPBERRYPI_FIRMWARE_PWM_NUM 1
+
+#endif
--
2.29.2
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]>
---
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 403bacf986eb..9ae44620c5b8 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-poe-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-poe-pwm";
+ #pwm-cells = <2>;
+ };
};
&gpio {
--
2.29.2
Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
interface when unbinding the device.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
drivers/reset/reset-raspberrypi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/reset/reset-raspberrypi.c b/drivers/reset/reset-raspberrypi.c
index 02f59c06f69b..fa23db554bcf 100644
--- a/drivers/reset/reset-raspberrypi.c
+++ b/drivers/reset/reset-raspberrypi.c
@@ -82,7 +82,7 @@ static int rpi_reset_probe(struct platform_device *pdev)
return -ENOENT;
}
- fw = rpi_firmware_get(np);
+ fw = devm_rpi_firmware_get(&pdev->dev, np);
of_node_put(np);
if (!fw)
return -EPROBE_DEFER;
--
2.29.2
Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
interface when unbinding the device.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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 59e45dc03a97..f27072837247 100644
--- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
+++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
@@ -2734,7 +2734,7 @@ static int vchiq_probe(struct platform_device *pdev)
return -ENOENT;
}
- drvdata->fw = rpi_firmware_get(fw_node);
+ drvdata->fw = devm_rpi_firmware_get(&pdev->dev, fw_node);
of_node_put(fw_node);
if (!drvdata->fw)
return -EPROBE_DEFER;
--
2.29.2
There is no use for the firmware interface after getting the touch
buffer address, so release it.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Acked-by: Dmitry Torokhov <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
Changes since v5:
- Correct commit message
Changes since v3:
- Release firmware handle in probe function
Changes since v2:
- Use devm_rpi_firmware_get(), instead of remove function
drivers/input/touchscreen/raspberrypi-ts.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c b/drivers/input/touchscreen/raspberrypi-ts.c
index ef6aaed217cf..5000f5fd9ec3 100644
--- a/drivers/input/touchscreen/raspberrypi-ts.c
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -160,7 +160,7 @@ static int rpi_ts_probe(struct platform_device *pdev)
touchbuf = (u32)ts->fw_regs_phys;
error = rpi_firmware_property(fw, RPI_FIRMWARE_FRAMEBUFFER_SET_TOUCHBUF,
&touchbuf, sizeof(touchbuf));
-
+ rpi_firmware_put(fw);
if (error || touchbuf != 0) {
dev_warn(dev, "Failed to set touchbuf, %d\n", error);
return error;
--
2.29.2
Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
interface when unbinding the device.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Acked-by: Stephen Boyd <[email protected]>
drivers/clk/bcm/clk-raspberrypi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index f89b9cfc4309..dd3b71eafabf 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -314,7 +314,7 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
return -ENOENT;
}
- firmware = rpi_firmware_get(firmware_node);
+ firmware = devm_rpi_firmware_get(&pdev->dev, firmware_node);
of_node_put(firmware_node);
if (!firmware)
return -EPROBE_DEFER;
--
2.29.2
Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
interface when unbinding the device.
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
---
drivers/soc/bcm/raspberrypi-power.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/bcm/raspberrypi-power.c b/drivers/soc/bcm/raspberrypi-power.c
index 5d1aacdd84ef..068715d6e66d 100644
--- a/drivers/soc/bcm/raspberrypi-power.c
+++ b/drivers/soc/bcm/raspberrypi-power.c
@@ -177,7 +177,7 @@ static int rpi_power_probe(struct platform_device *pdev)
return -ENODEV;
}
- rpi_domains->fw = rpi_firmware_get(fw_np);
+ rpi_domains->fw = devm_rpi_firmware_get(&pdev->dev, fw_np);
of_node_put(fw_np);
if (!rpi_domains->fw)
return -EPROBE_DEFER;
--
2.29.2
It'll simplify the firmware handling for most consumers.
Suggested-by: Bartosz Golaszewski <[email protected]>
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Bartosz Golaszewski <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Nicolas Saenz Julienne <[email protected]>
Reviewed-by: Florian Fainelli <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
Reviewed-by: Bartosz Golaszewski <[email protected]>
---
Changes since v5:
- Add comment to avoid people blindly switching the memory allocation
to the devm variant.
- Fix function documentation as per Florian's comment.
Changes since v3:
- Use kref instead of waiting on refcount
drivers/firmware/raspberrypi.c | 40 ++++++++++++++++++++--
include/soc/bcm2835/raspberrypi-firmware.h | 2 ++
2 files changed, 39 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 30259dc9b805..b65e4c495772 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -7,6 +7,7 @@
*/
#include <linux/dma-mapping.h>
+#include <linux/kref.h>
#include <linux/mailbox_client.h>
#include <linux/module.h>
#include <linux/of_platform.h>
@@ -27,6 +28,8 @@ struct rpi_firmware {
struct mbox_chan *chan; /* The property channel. */
struct completion c;
u32 enabled;
+
+ struct kref consumers;
};
static DEFINE_MUTEX(transaction_lock);
@@ -225,12 +228,31 @@ static void rpi_register_clk_driver(struct device *dev)
-1, NULL, 0);
}
+static void rpi_firmware_delete(struct kref *kref)
+{
+ struct rpi_firmware *fw = container_of(kref, struct rpi_firmware,
+ consumers);
+
+ mbox_free_channel(fw->chan);
+ kfree(fw);
+}
+
+void rpi_firmware_put(struct rpi_firmware *fw)
+{
+ kref_put(&fw->consumers, rpi_firmware_delete);
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_put);
+
static int rpi_firmware_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct rpi_firmware *fw;
- fw = devm_kzalloc(dev, sizeof(*fw), GFP_KERNEL);
+ /*
+ * Memory will be freed by rpi_firmware_delete() once all users have
+ * released their firmware handles. Don't use devm_kzalloc() here.
+ */
+ fw = kzalloc(sizeof(*fw), GFP_KERNEL);
if (!fw)
return -ENOMEM;
@@ -247,6 +269,7 @@ static int rpi_firmware_probe(struct platform_device *pdev)
}
init_completion(&fw->c);
+ kref_init(&fw->consumers);
platform_set_drvdata(pdev, fw);
@@ -275,7 +298,8 @@ static int rpi_firmware_remove(struct platform_device *pdev)
rpi_hwmon = NULL;
platform_device_unregister(rpi_clk);
rpi_clk = NULL;
- mbox_free_channel(fw->chan);
+
+ rpi_firmware_put(fw);
return 0;
}
@@ -284,16 +308,26 @@ static int rpi_firmware_remove(struct platform_device *pdev)
* rpi_firmware_get - Get pointer to rpi_firmware structure.
* @firmware_node: Pointer to the firmware Device Tree node.
*
+ * The reference to rpi_firmware has to be released with rpi_firmware_put().
+ *
* Returns NULL is the firmware device is not ready.
*/
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 (!kref_get_unless_zero(&fw->consumers))
+ return NULL;
+
+ return fw;
}
EXPORT_SYMBOL_GPL(rpi_firmware_get);
diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index cc9cdbc66403..fdfef7fe40df 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -140,6 +140,7 @@ int rpi_firmware_property(struct rpi_firmware *fw,
u32 tag, void *data, size_t len);
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);
#else
static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
@@ -154,6 +155,7 @@ static inline int rpi_firmware_property_list(struct rpi_firmware *fw,
return -ENOSYS;
}
+static inline void rpi_firmware_put(struct rpi_firmware *fw) { }
static inline struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node)
{
return NULL;
--
2.29.2
On Mon, Jan 18, 2021 at 01:32:40PM +0100, Nicolas Saenz Julienne wrote:
> Use devm_rpi_firmware_get() so as to make sure we release RPi's firmware
> interface when unbinding the device.
>
> Signed-off-by: Nicolas Saenz Julienne <[email protected]>
> Reviewed-by: Florian Fainelli <[email protected]>
> ---
> drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Greg Kroah-Hartman <[email protected]>
On Mon, 2021-01-18 at 13:32 +0100, 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 <[email protected]>
>
> ---
ping :)
Regards,
Nicolas
On Mon, 2021-01-18 at 13:32 +0100, 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 <[email protected]>
>
> ---
ping :)
> Changes since v6:
> - Use %pe
> - Round divisions properly
> - Use dev_err_probe()
> - Pass check_patch
>
> Changes since v3:
> - Rename compatible string to be more explicit WRT to bus's limitations
>
> Changes since v2:
> - Use devm_rpi_firmware_get()
> - Rename driver
> - Small cleanups
>
> Changes since v1:
> - Use default pwm bindings and get rid of xlate() function
> - Correct spelling errors
> - Correct apply() function
> - Round values
> - Fix divisions in arm32 mode
> - Small cleanups
>
> drivers/pwm/Kconfig | 9 ++
> drivers/pwm/Makefile | 1 +
> drivers/pwm/pwm-raspberrypi-poe.c | 220 ++++++++++++++++++++++++++++++
> 3 files changed, 230 insertions(+)
> create mode 100644 drivers/pwm/pwm-raspberrypi-poe.c
>
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0937e1c047ac..75e2344703b3 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -423,6 +423,15 @@ config PWM_PXA
> To compile this driver as a module, choose M here: the module
> will be called pwm-pxa.
>
>
> +config PWM_RASPBERRYPI_POE
> + tristate "Raspberry Pi Firwmware PoE Hat PWM support"
> + # Make sure not 'y' when RASPBERRYPI_FIRMWARE is 'm'. This can only
> + # happen when COMPILE_TEST=y, hence the added !RASPBERRYPI_FIRMWARE.
> + 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 18b89d7fd092..ed28d7bd4c64 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -38,6 +38,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_POE) += pwm-raspberrypi-poe.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-poe.c b/drivers/pwm/pwm-raspberrypi-poe.c
> new file mode 100644
> index 000000000000..ca845e8fabe6
> --- /dev/null
> +++ b/drivers/pwm/pwm-raspberrypi-poe.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Nicolas Saenz Julienne <[email protected]>
> + * For more information on Raspberry Pi's PoE hat see:
> + * https://www.raspberrypi.org/products/poe-hat/
> + *
> + * Limitations:
> + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0
> + * - Only normal polarity
> + * - Fixed 12.5 kHz period
> + *
> + * The current period is completed when HW is reconfigured.
> + */
> +
> +#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-poe-pwm.h>
> +
> +#define RPI_PWM_MAX_DUTY 255
> +#define RPI_PWM_PERIOD_NS 80000 /* 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 *raspberrypi_pwm_from_chip(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;
> + 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;
> + 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 *rpipwm = raspberrypi_pwm_from_chip(chip);
> +
> + state->period = RPI_PWM_PERIOD_NS;
> + state->duty_cycle = DIV_ROUND_UP(rpipwm->duty_cycle * RPI_PWM_PERIOD_NS,
> + RPI_PWM_MAX_DUTY);
> + state->enabled = !!(rpipwm->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 *rpipwm = raspberrypi_pwm_from_chip(chip);
> + unsigned int duty_cycle;
> + int ret;
> +
> + 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 = DIV_ROUND_DOWN_ULL(state->duty_cycle * RPI_PWM_MAX_DUTY,
> + RPI_PWM_PERIOD_NS);
> + 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: %pe\n",
> + ERR_PTR(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: %pe\n",
> + ERR_PTR(ret));
> + return ret;
> + }
> +
> + rpipwm->duty_cycle = duty_cycle;
> +
> + return 0;
> +}
> +
> +static const struct pwm_ops raspberrypi_pwm_ops = {
> + .get_state = raspberrypi_pwm_get_state,
> + .apply = raspberrypi_pwm_apply,
> + .owner = THIS_MODULE,
> +};
> +
> +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 *rpipwm;
> + int ret;
> +
> + firmware_node = of_get_parent(dev->of_node);
> + if (!firmware_node) {
> + dev_err(dev, "Missing firmware node\n");
> + return -ENOENT;
> + }
> +
> + firmware = devm_rpi_firmware_get(&pdev->dev, firmware_node);
> + of_node_put(firmware_node);
> + if (!firmware)
> + return dev_err_probe(dev, -EPROBE_DEFER,
> + "Failed to get firmware handle\n");
> +
> + rpipwm = devm_kzalloc(&pdev->dev, sizeof(*rpipwm), GFP_KERNEL);
> + if (!rpipwm)
> + return -ENOMEM;
> +
> + rpipwm->firmware = firmware;
> + rpipwm->chip.dev = dev;
> + rpipwm->chip.ops = &raspberrypi_pwm_ops;
> + rpipwm->chip.base = -1;
> + rpipwm->chip.npwm = RASPBERRYPI_FIRMWARE_PWM_NUM;
> +
> + platform_set_drvdata(pdev, rpipwm);
> +
> + ret = raspberrypi_pwm_get_property(rpipwm->firmware, RPI_PWM_CUR_DUTY_REG,
> + &rpipwm->duty_cycle);
> + if (ret) {
> + dev_err(dev, "Failed to get duty cycle: %pe\n", ERR_PTR(ret));
> + return ret;
> + }
> +
> + return pwmchip_add(&rpipwm->chip);
> +}
> +
> +static int raspberrypi_pwm_remove(struct platform_device *pdev)
> +{
> + struct raspberrypi_pwm *rpipwm = platform_get_drvdata(pdev);
> +
> + return pwmchip_remove(&rpipwm->chip);
> +}
> +
> +static const struct of_device_id raspberrypi_pwm_of_match[] = {
> + { .compatible = "raspberrypi,firmware-poe-pwm", },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, raspberrypi_pwm_of_match);
> +
> +static struct platform_driver raspberrypi_pwm_driver = {
> + .driver = {
> + .name = "raspberrypi-poe-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 <[email protected]>");
> +MODULE_DESCRIPTION("Raspberry Pi Firmware Based PWM Bus Driver");
> +MODULE_LICENSE("GPL v2");
Hello Nicolas,
On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
> diff --git a/drivers/pwm/pwm-raspberrypi-poe.c b/drivers/pwm/pwm-raspberrypi-poe.c
> new file mode 100644
> index 000000000000..ca845e8fabe6
> --- /dev/null
> +++ b/drivers/pwm/pwm-raspberrypi-poe.c
> @@ -0,0 +1,220 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2020 Nicolas Saenz Julienne <[email protected]>
> + * For more information on Raspberry Pi's PoE hat see:
> + * https://www.raspberrypi.org/products/poe-hat/
> + *
> + * Limitations:
> + * - No disable bit, so a disabled PWM is simulated by duty_cycle 0
> + * - Only normal polarity
> + * - Fixed 12.5 kHz period
> + *
> + * The current period is completed when HW is reconfigured.
nice.
> + */
> +
> [...]
> +static int raspberrypi_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> + const struct pwm_state *state)
> +{
> + struct raspberrypi_pwm *rpipwm = raspberrypi_pwm_from_chip(chip);
> + unsigned int duty_cycle;
> + int ret;
> +
> + 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 = DIV_ROUND_DOWN_ULL(state->duty_cycle * RPI_PWM_MAX_DUTY,
> + RPI_PWM_PERIOD_NS);
> + 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: %pe\n",
> + ERR_PTR(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: %pe\n",
> + ERR_PTR(ret));
> + return ret;
This only has an effect for the next reboot, right? If so I wonder if it
is a good idea in general. (Think: The current PWM setting enables a
motor that makes a self-driving car move at 100 km/h. Consider the rpi
crashes, do I want to car to pick up driving 100 km/h at power up even
before Linux is up again?) And if we agree it's a good idea: Should
raspberrypi_pwm_apply return 0 if setting the duty cycle succeeded and
only setting the default didn't?
Other than that the patch looks fine.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Hi Uwe,
thanks for taking the time to look into this. :)
On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:
> Hello Nicolas,
>
> On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
[...]
> > + /*
> > + * 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: %pe\n",
> > + ERR_PTR(ret));
> > + return ret;
>
> This only has an effect for the next reboot, right?
It effects all reboots until it's further changed.
> If so I wonder if it is a good idea in general. (Think: The current PWM
> setting enables a motor that makes a self-driving car move at 100 km/h.
> Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
> power up even before Linux is up again?)
I get your point. But this isn't used as a general purpose PWM. For now the
interface is solely there to drive a PWM fan that's arguably harmless. This
doesn't mean that the RPi foundation will not reuse the firmware interface for
other means in the future. In such case we can always use a new DT compatible
and bypass this feature (the current DT string is
'raspberrypi,firmware-poe-pwm', which is specific to this use-case).
My aim here is to be on par feature wise with RPi's downstream implementation.
So as for them to be able to use it as is and avoid duplication. Now, if this
is blocking the driver from being merged, I'd rather remove it. It'll be a
patch for the downstream kernel to maintain, but better than nothing.
> And if we agree it's a good idea: Should raspberrypi_pwm_apply return 0 if
> setting the duty cycle succeeded and only setting the default didn't?
Good point. I don't think so. We'd be also missing on the following by
returning early:
rpipwm->duty_cycle = duty_cycle;
I propose to change it to a 'best effort' approach, if it fails, log it and
continue successfully.
Regards,
Nicolas
Hello Nicolas,
On Thu, Mar 11, 2021 at 02:01:00PM +0100, Nicolas Saenz Julienne wrote:
> On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-K?nig wrote:
> > On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
>
> [...]
>
> > > + /*
> > > + * 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: %pe\n",
> > > + ERR_PTR(ret));
> > > + return ret;
> >
> > This only has an effect for the next reboot, right?
>
> It effects all reboots until it's further changed.
>
> > If so I wonder if it is a good idea in general. (Think: The current PWM
> > setting enables a motor that makes a self-driving car move at 100 km/h.
> > Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
> > power up even before Linux is up again?)
>
> I get your point. But this isn't used as a general purpose PWM. For now the
> interface is solely there to drive a PWM fan that's arguably harmless. This
> doesn't mean that the RPi foundation will not reuse the firmware interface for
> other means in the future. In such case we can always use a new DT compatible
> and bypass this feature (the current DT string is
> 'raspberrypi,firmware-poe-pwm', which is specific to this use-case).
>
> My aim here is to be on par feature wise with RPi's downstream implementation.
Just because the downstream kernel does it should not be the (single)
reason to do that. My gut feeling is: For a motor restoring the PWM
config on reboot is bad and for a fan it doesn't really hurt if it
doesn't restart automatically. So I'd prefer to to drop this feature.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Thu, 2021-03-11 at 14:18 +0100, Uwe Kleine-König wrote:
> Hello Nicolas,
>
> On Thu, Mar 11, 2021 at 02:01:00PM +0100, Nicolas Saenz Julienne wrote:
> > On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:
> > > On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
> >
> > [...]
> >
> > > > + /*
> > > > + * 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: %pe\n",
> > > > + ERR_PTR(ret));
> > > > + return ret;
> > >
> > > This only has an effect for the next reboot, right?
> >
> > It effects all reboots until it's further changed.
> >
> > > If so I wonder if it is a good idea in general. (Think: The current PWM
> > > setting enables a motor that makes a self-driving car move at 100 km/h.
> > > Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
> > > power up even before Linux is up again?)
> >
> > I get your point. But this isn't used as a general purpose PWM. For now the
> > interface is solely there to drive a PWM fan that's arguably harmless. This
> > doesn't mean that the RPi foundation will not reuse the firmware interface for
> > other means in the future. In such case we can always use a new DT compatible
> > and bypass this feature (the current DT string is
> > 'raspberrypi,firmware-poe-pwm', which is specific to this use-case).
> >
> > My aim here is to be on par feature wise with RPi's downstream implementation.
>
> Just because the downstream kernel does it should not be the (single)
> reason to do that. My gut feeling is: For a motor restoring the PWM
> config on reboot is bad and for a fan it doesn't really hurt if it
> doesn't restart automatically. So I'd prefer to to drop this feature.
Fair enough, I'll remove it then.
Regards,
Nicolas