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