[PATCH v2 01/10] firmware: raspberrypi: Introduce rpi_firmware_put()
When unbinding the firmware device we need to make sure it has no
consumers left. Otherwise we'd leave them with a firmware handle
pointing at freed memory.
Keep a reference count of all consumers and make sure they all finished
unbinding before we do.
Suggested-by: Uwe Kleine-König
Signed-off-by: Nicolas Saenz Julienne
---
@Uwe: I didn't found it necessary to call 'try_module_get()' as the rest
of modules depend on the 'rpi_firmware_get/put()' symbols which already
block users from unloading the module prematurely.
drivers/firmware/raspberrypi.c | 30 +-
include/soc/bcm2835/raspberrypi-firmware.h | 3 +++
2 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/raspberrypi.c b/drivers/firmware/raspberrypi.c
index 2371d08bdd17..e5ba609e3985 100644
--- a/drivers/firmware/raspberrypi.c
+++ b/drivers/firmware/raspberrypi.c
@@ -11,7 +11,9 @@
#include
#include
#include
+#include
#include
+#include
#include
#define MBOX_MSG(chan, data28) (((data28) & ~0xf) | ((chan) & 0xf))
@@ -27,6 +29,9 @@ struct rpi_firmware {
struct mbox_chan *chan; /* The property channel. */
struct completion c;
u32 enabled;
+
+ refcount_t consumers;
+ wait_queue_head_t wait;
};
static DEFINE_MUTEX(transaction_lock);
@@ -247,6 +252,8 @@ static int rpi_firmware_probe(struct platform_device *pdev)
}
init_completion(>c);
+ refcount_set(>consumers, 1);
+ init_waitqueue_head(>wait);
platform_set_drvdata(pdev, fw);
@@ -275,6 +282,8 @@ static int rpi_firmware_remove(struct platform_device *pdev)
rpi_hwmon = NULL;
platform_device_unregister(rpi_clk);
rpi_clk = NULL;
+
+ wait_event(fw->wait, refcount_dec_if_one(>consumers));
mbox_free_channel(fw->chan);
return 0;
@@ -289,14 +298,33 @@ static int rpi_firmware_remove(struct platform_device
*pdev)
struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node)
{
struct platform_device *pdev = of_find_device_by_node(firmware_node);
+ struct rpi_firmware *fw;
if (!pdev)
return NULL;
- return platform_get_drvdata(pdev);
+ fw = platform_get_drvdata(pdev);
+ if (!fw)
+ return NULL;
+
+ if (!refcount_inc_not_zero(>consumers))
+ return NULL;
+
+ return fw;
}
EXPORT_SYMBOL_GPL(rpi_firmware_get);
+/**
+ * rpi_firmware_put - Put pointer to rpi_firmware structure.
+ * @rpi_firmware:Pointer to struct rpi_firmware
+ */
+void rpi_firmware_put(struct rpi_firmware *fw)
+{
+ refcount_dec(>consumers);
+ wake_up(>wait);
+}
+EXPORT_SYMBOL_GPL(rpi_firmware_put);
+
static const struct of_device_id rpi_firmware_of_match[] = {
{ .compatible = "raspberrypi,bcm2835-firmware", },
{},
diff --git a/include/soc/bcm2835/raspberrypi-firmware.h
b/include/soc/bcm2835/raspberrypi-firmware.h
index cc9cdbc66403..7836ea51fbdf 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -141,6 +141,7 @@ int rpi_firmware_property(struct rpi_firmware *fw,
int rpi_firmware_property_list(struct rpi_firmware *fw,
void *data, size_t tag_size);
struct rpi_firmware *rpi_firmware_get(struct device_node *firmware_node);
+void rpi_firmware_put(struct rpi_firmware *fw);
#else
static inline int rpi_firmware_property(struct rpi_firmware *fw, u32 tag,
void *data, size_t len)
@@ -158,6 +159,8 @@ static inline struct rpi_firmware *rpi_firmware_get(struct
device_node *firmware
{
return NULL;
}
+
+void rpi_firmware_put(struct rpi_firmware *fw) { }
#endif
#endif /* __SOC_RASPBERRY_FIRMWARE_H__ */
--
2.28.0
[PATCH v2 05/10] soc: bcm: raspberrypi-power: Release firmware handle on unbind
Upon unbinding the device make sure we release RPi's firmware interface.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/soc/bcm/raspberrypi-power.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/soc/bcm/raspberrypi-power.c
b/drivers/soc/bcm/raspberrypi-power.c
index 5d1aacdd84ef..a0b38db5886c 100644
--- a/drivers/soc/bcm/raspberrypi-power.c
+++ b/drivers/soc/bcm/raspberrypi-power.c
@@ -225,6 +225,20 @@ static int rpi_power_probe(struct platform_device *pdev)
return 0;
}
+static int rpi_power_remove(struct platform_device *pdev)
+{
+ struct rpi_power_domains *rpi_domains = platform_get_drvdata(pdev);
+
+ of_genpd_del_provider(dev->of_node);
+
+ for (i = 0; i < RPI_POWER_DOMAIN_COUNT; i++)
+ pm_genpd_remove(_domains->domains[i].base);
+
+ rpi_firmware_put(rpi_domaina->fw);
+
+ return 0;
+}
+
static const struct of_device_id rpi_power_of_match[] = {
{ .compatible = "raspberrypi,bcm2835-power", },
{},
@@ -237,6 +251,7 @@ static struct platform_driver rpi_power_driver = {
.of_match_table = rpi_power_of_match,
},
.probe = rpi_power_probe,
+ .remove = rpi_power_remove,
};
builtin_platform_driver(rpi_power_driver);
--
2.28.0
[PATCH v2 07/10] input: raspberrypi-ts: Release firmware handle when not needed
After passing the DMA buffer address through the firmware interface,
release the firmware handle, we won't need it anymore.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/input/touchscreen/raspberrypi-ts.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/touchscreen/raspberrypi-ts.c
b/drivers/input/touchscreen/raspberrypi-ts.c
index ef6aaed217cf..29c878a00018 100644
--- a/drivers/input/touchscreen/raspberrypi-ts.c
+++ b/drivers/input/touchscreen/raspberrypi-ts.c
@@ -165,6 +165,7 @@ static int rpi_ts_probe(struct platform_device *pdev)
dev_warn(dev, "Failed to set touchbuf, %d\n", error);
return error;
}
+ rpi_firmware_put(fw);
input = devm_input_allocate_device(dev);
if (!input) {
--
2.28.0
[PATCH v2 04/10] reset: raspberrypi: Release firmware handle on unbind
Upon unbinding the device make sure we release RPi's firmware interface.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/reset/reset-raspberrypi.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/drivers/reset/reset-raspberrypi.c
b/drivers/reset/reset-raspberrypi.c
index 02f59c06f69b..29311192e2c9 100644
--- a/drivers/reset/reset-raspberrypi.c
+++ b/drivers/reset/reset-raspberrypi.c
@@ -99,7 +99,17 @@ static int rpi_reset_probe(struct platform_device *pdev)
priv->rcdev.ops = _reset_ops;
priv->rcdev.of_node = dev->of_node;
- return devm_reset_controller_register(dev, >rcdev);
+ return reset_controller_register(>rcdev);
+}
+
+static int rpi_reset_remove(struct platform_device *pdev)
+{
+ struct rpi_reset *priv = platform_get_drvdata(pdev);
+
+ reset_controller_unregister(>rcdev);
+ rpi_firmware_put(priv->fw);
+
+ return 0;
}
static const struct of_device_id rpi_reset_of_match[] = {
@@ -110,6 +120,7 @@ MODULE_DEVICE_TABLE(of, rpi_reset_of_match);
static struct platform_driver rpi_reset_driver = {
.probe = rpi_reset_probe,
+ .remove = rpi_reset_remove,
.driver = {
.name = "raspberrypi-reset",
.of_match_table = rpi_reset_of_match,
--
2.28.0
[PATCH v2 00/10] Raspberry Pi PoE HAT fan support
The aim of this series is to add support to the fan found on RPi's PoE HAT. Some commentary on the design can be found below. But the imporant part to the people CC'd here not involved with PWM is that, in order to achieve this properly, we also have to fix the firmware interface the driver uses to communicate with the PWM bus (and many other low level functions). Specifically, we have to make sure the firmware interface isn't unbound while consumers are still up. So, patch #1 introduces reference counting in the firwmware interface driver and patches #2 to #7 update all firmware users. Patches #8 to #10 introduce the new PWM driver. I sent everything as a single series as the final version of the PWM drivers depends on the firwmare fixes, but I'll be happy to split this into two separate series if you think it's better. --- Original cover letter below --- This series aims at adding support to RPi's official PoE HAT fan[1]. The HW setup is the following: | Raspberry Pi | PoE HAT| arm core -> Mailbox -> RPi co-processor -> I2C -> Atmel MCU -> PWM -> FAN The arm cores have only access to the mailbox interface, as i2c0, even if physically accessible, is to be used solely by the co-processor (VideoCore 4/6). This series implements a PWM bus, and has pwm-fan sitting on top of it as per this discussion: https://lkml.org/lkml/2018/9/2/486. Although this design has a series of shortcomings: - It depends on a DT binding: it's not flexible if a new hat shows up with new functionality, we're not 100% sure we'll be able to expand it without breaking backwards compatibility. But without it we can't make use of DT thermal-zones, which IMO is overkill. - We're using pwm-fan, writing a hwmon driver would, again, give us more flexibility, but it's not really needed at the moment. I personally think that it's not worth the effort, it's unlikely we'll get things right in advance. And ultimately, if the RPi people come up with something new, we can always write a new driver/bindings from scratch (as in not reusing previous code). That said, I'm more than happy to change things if there is a consensus that another design will do the trick. [1] https://www.raspberrypi.org/blog/introducing-power-over-ethernet-poe-hat/ --- Changes since v1: - Address PWM driver changes - Fix binding, now with 2 cells - Add reference count to rpi_firmware_get() Nicolas Saenz Julienne (10): firmware: raspberrypi: Introduce rpi_firmware_put() clk: bcm: rpi: Release firmware handle on unbind gpio: raspberrypi-exp: Release firmware handle on unbind reset: raspberrypi: Release firmware handle on unbind soc: bcm: raspberrypi-power: Release firmware handle on unbind staging: vchiq: Release firmware handle on unbind input: raspberrypi-ts: Release firmware handle when not needed dt-bindings: pwm: Add binding for RPi firmware PWM bus DO NOT MERGE: ARM: dts: Add RPi's official PoE hat support pwm: Add Raspberry Pi Firmware based PWM bus .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 20 ++ arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 54 + drivers/clk/bcm/clk-raspberrypi.c | 1 + drivers/firmware/raspberrypi.c| 30 ++- drivers/gpio/gpio-raspberrypi-exp.c | 14 +- drivers/input/touchscreen/raspberrypi-ts.c| 1 + drivers/pwm/Kconfig | 9 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-raspberrypi.c | 221 ++ drivers/reset/reset-raspberrypi.c | 13 +- drivers/soc/bcm/raspberrypi-power.c | 15 ++ .../interface/vchiq_arm/vchiq_arm.c | 3 + .../pwm/raspberrypi,firmware-pwm.h| 13 ++ include/soc/bcm2835/raspberrypi-firmware.h| 3 + 14 files changed, 395 insertions(+), 3 deletions(-) create mode 100644 drivers/pwm/pwm-raspberrypi.c create mode 100644 include/dt-bindings/pwm/raspberrypi,firmware-pwm.h -- 2.28.0
[PATCH v2 10/10] pwm: Add Raspberry Pi Firmware based PWM bus
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
---
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
- Add comment explaining weird Kconfig construct
drivers/pwm/Kconfig | 9 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-raspberrypi.c | 221 ++
3 files changed, 231 insertions(+)
create mode 100644 drivers/pwm/pwm-raspberrypi.c
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 63be5362fd3a..b0ffb1e690c0 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -379,6 +379,15 @@ 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"
+ # 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 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 ..72dc0fc5a206
--- /dev/null
+++ b/drivers/pwm/pwm-raspberrypi.c
@@ -0,0 +1,221 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Nicolas Saenz Julienne
+ * 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
+#include
+#include
+#include
+
+#include
+#include
+
+#define RPI_PWM_MAX_DUTY 255
+#define RPI_PWM_PERIOD_NS 8 /* 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,
+ , 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,
+ , 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 *rpipwm = to_raspberrypi_pwm(chip);
+
+ state->period = RPI_PWM_PERIOD_NS;
+ state->duty_cycle = DIV_ROUND_CLOSEST(rpipwm->duty_cycle *
RPI_PWM_PERIOD_NS,
+ RPI_PWM_MAX_DUTY);
+ state->enabled = !!(rpip
[PATCH v2 02/10] clk: bcm: rpi: Release firmware handle on unbind
Upon unbinding the clock device make sure we release RPi's firmware interface. Signed-off-by: Nicolas Saenz Julienne --- drivers/clk/bcm/clk-raspberrypi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c index f89b9cfc4309..845510ff7514 100644 --- a/drivers/clk/bcm/clk-raspberrypi.c +++ b/drivers/clk/bcm/clk-raspberrypi.c @@ -353,6 +353,7 @@ static int raspberrypi_clk_remove(struct platform_device *pdev) struct raspberrypi_clk *rpi = platform_get_drvdata(pdev); platform_device_unregister(rpi->cpufreq); + rpi_firmware_put(rpi->firmware); return 0; } -- 2.28.0
[PATCH v2 03/10] gpio: raspberrypi-exp: Release firmware handle on unbind
Upon unbinding the device make sure we release RPi's firmware interface.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/gpio/gpio-raspberrypi-exp.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpio/gpio-raspberrypi-exp.c
b/drivers/gpio/gpio-raspberrypi-exp.c
index bb100e0124e6..c008336e1131 100644
--- a/drivers/gpio/gpio-raspberrypi-exp.c
+++ b/drivers/gpio/gpio-raspberrypi-exp.c
@@ -231,8 +231,19 @@ static int rpi_exp_gpio_probe(struct platform_device *pdev)
rpi_gpio->gc.get = rpi_exp_gpio_get;
rpi_gpio->gc.set = rpi_exp_gpio_set;
rpi_gpio->gc.can_sleep = true;
+ platform_set_drvdata(pdev, rpi_gpio);
- return devm_gpiochip_add_data(dev, _gpio->gc, rpi_gpio);
+ return gpiochip_add_data(_gpio->gc, rpi_gpio);
+}
+
+static int rpi_exp_gpio_remove(struct platform_device *pdev)
+{
+ struct rpi_exp_gpio *rpi_gpio = platform_get_drvdata(pdev);
+
+ gpiochip_remove(_gpio->gc);
+ rpi_firmware_put(rpi_gpio->fw);
+
+ return 0;
}
static const struct of_device_id rpi_exp_gpio_ids[] = {
@@ -247,6 +258,7 @@ static struct platform_driver rpi_exp_gpio_driver = {
.of_match_table = of_match_ptr(rpi_exp_gpio_ids),
},
.probe = rpi_exp_gpio_probe,
+ .remove = rpi_exp_gpio_remove,
};
module_platform_driver(rpi_exp_gpio_driver);
--
2.28.0
[PATCH v2 08/10] dt-bindings: pwm: Add binding for RPi firmware PWM bus
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
---
Changes since v1:
- Update bindings to use 2 #pwm-cells
.../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 20 +++
.../pwm/raspberrypi,firmware-pwm.h| 13
2 files changed, 33 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..8029ce958c48 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-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-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 ..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
+ */
+
+#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
Re: [PATCH v4 3/7] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 2020-10-22 at 14:23 +0200, Ard Biesheuvel wrote:
> > +/**
> > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA
> > + * @np: The node to start searching from or NULL to start from the root
> > + *
> > + * Gets the highest CPU physical address that is addressable by all DMA
> > masters
> > + * in the sub-tree pointed by np, or the whole tree if NULL is passed. If
> > no
> > + * DMA constrained device is found, it returns PHYS_ADDR_MAX.
> > + */
> > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
> > +{
> > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX;
> > + struct of_range_parser parser;
> > + phys_addr_t subtree_max_addr;
> > + struct device_node *child;
> > + struct of_range range;
> > + const __be32 *ranges;
> > + u64 cpu_end = 0;
> > + int len;
> > +
> > + if (!np)
> > + np = of_root;
> > +
> > + ranges = of_get_property(np, "dma-ranges", );
> > + if (ranges && len) {
> > + of_dma_range_parser_init(, np);
> > + for_each_of_range(, )
> > + if (range.cpu_addr + range.size > cpu_end)
> > + cpu_end = range.cpu_addr + range.size;
>
>
> Shouldn't this be 'range.cpu_addr + range.size - 1' ?
Yes, I agree. In that case arm64's counterpart should be:
zone_dma_bits = max(32U, fls64(of_dma_get_max_cpu_address(NULL)));
I'll update it.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
[PATCH v4 5/7] arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges
We recently introduced a 1 GB sized ZONE_DMA to cater for platforms
incorporating masters that can address less than 32 bits of DMA, in
particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has
peripherals that can only address up to 1 GB (and its PCIe host
bridge can only access the bottom 3 GB)
The DMA layer also needs to be able to allocate memory that is
guaranteed to meet those DMA constraints, for bounce buffering as well
as allocating the backing for consistent mappings. This is why the 1 GB
ZONE_DMA was introduced recently. Unfortunately, it turns out the having
a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes problems with kdump, and
potentially in other places where allocations cannot cross zone
boundaries. Therefore, we should avoid having two separate DMA zones
when possible.
So, with the help of of_dma_get_max_cpu_address() get the topmost
physical address accessible to all DMA masters in system and use that
information to fine-tune ZONE_DMA's size. In the absence of addressing
limited masters ZONE_DMA will span the whole 32-bit address space,
otherwise, in the case of the Raspberry Pi 4 it'll only span the 30-bit
address space, and have ZONE_DMA32 cover the rest of the 32-bit address
space.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v3:
- Simplify code for readability.
Changes since v2:
- Updated commit log by shamelessly copying Ard's ACPI commit log
arch/arm64/mm/init.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 410721fc4fc0..94e38f99748b 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -42,8 +42,6 @@
#include
#include
-#define ARM64_ZONE_DMA_BITS30
-
/*
* We need to be able to catch inadvertent references to memstart_addr
* that occur (potentially in generic code) before arm64_memblock_init()
@@ -188,9 +186,11 @@ static phys_addr_t __init max_zone_phys(unsigned int
zone_bits)
static void __init zone_sizes_init(unsigned long min, unsigned long max)
{
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
+ unsigned int __maybe_unused dt_zone_dma_bits;
#ifdef CONFIG_ZONE_DMA
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
+ dt_zone_dma_bits = ilog2(of_dma_get_max_cpu_address(NULL));
+ zone_dma_bits = min(32U, dt_zone_dma_bits);
arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
--
2.28.0
[PATCH v4 3/7] of/address: Introduce of_dma_get_max_cpu_address()
Introduce of_dma_get_max_cpu_address(), which provides the highest CPU
physical address addressable by all DMA masters in the system. It's
specially useful for setting memory zones sizes at early boot time.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v3:
- use u64 with cpu_end
Changes since v2:
- Use PHYS_ADDR_MAX
- return phys_dma_t
- Rename function
- Correct subject
- Add support to start parsing from an arbitrary device node in order
for the function to work with unit tests
drivers/of/address.c | 42 ++
include/linux/of.h | 7 +++
2 files changed, 49 insertions(+)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index eb9ab4f1e80b..47dfe5881e18 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const
struct bus_dma_region **map)
}
#endif /* CONFIG_HAS_DMA */
+/**
+ * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA
+ * @np: The node to start searching from or NULL to start from the root
+ *
+ * Gets the highest CPU physical address that is addressable by all DMA masters
+ * in the sub-tree pointed by np, or the whole tree if NULL is passed. If no
+ * DMA constrained device is found, it returns PHYS_ADDR_MAX.
+ */
+phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
+{
+ phys_addr_t max_cpu_addr = PHYS_ADDR_MAX;
+ struct of_range_parser parser;
+ phys_addr_t subtree_max_addr;
+ struct device_node *child;
+ struct of_range range;
+ const __be32 *ranges;
+ u64 cpu_end = 0;
+ int len;
+
+ if (!np)
+ np = of_root;
+
+ ranges = of_get_property(np, "dma-ranges", );
+ if (ranges && len) {
+ of_dma_range_parser_init(, np);
+ for_each_of_range(, )
+ if (range.cpu_addr + range.size > cpu_end)
+ cpu_end = range.cpu_addr + range.size;
+
+ if (max_cpu_addr > cpu_end)
+ max_cpu_addr = cpu_end;
+ }
+
+ for_each_available_child_of_node(np, child) {
+ subtree_max_addr = of_dma_get_max_cpu_address(child);
+ if (max_cpu_addr > subtree_max_addr)
+ max_cpu_addr = subtree_max_addr;
+ }
+
+ return max_cpu_addr;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np:device node
diff --git a/include/linux/of.h b/include/linux/of.h
index 481ec0467285..db8db8f2c967 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id,
const char *map_name, const char *map_mask_name,
struct device_node **target, u32 *id_out);
+phys_addr_t of_dma_get_max_cpu_address(struct device_node *np);
+
#else /* CONFIG_OF */
static inline void of_core_init(void)
@@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id,
return -EINVAL;
}
+static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
+{
+ return PHYS_ADDR_MAX;
+}
+
#define of_match_ptr(_ptr) NULL
#define of_match_node(_matches, _node) NULL
#endif /* CONFIG_OF */
--
2.28.0
[PATCH v4 2/7] arm64: mm: Move zone_dma_bits initialization into zone_sizes_init()
zone_dma_bits's initialization happens earlier that it's actually
needed, in arm64_memblock_init(). So move it into the more suitable
zone_sizes_init().
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index fc4ab0d6d5d2..410721fc4fc0 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -190,6 +190,8 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
#ifdef CONFIG_ZONE_DMA
+ zone_dma_bits = ARM64_ZONE_DMA_BITS;
+ arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
#ifdef CONFIG_ZONE_DMA32
@@ -376,11 +378,6 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
- if (IS_ENABLED(CONFIG_ZONE_DMA)) {
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
- arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
- }
-
if (IS_ENABLED(CONFIG_ZONE_DMA32))
arm64_dma32_phys_limit = max_zone_phys(32);
else
--
2.28.0
[PATCH v4 6/7] arm64: mm: Set ZONE_DMA size based on early IORT scan
From: Ard Biesheuvel
We recently introduced a 1 GB sized ZONE_DMA to cater for platforms
incorporating masters that can address less than 32 bits of DMA, in
particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has
peripherals that can only address up to 1 GB (and its PCIe host
bridge can only access the bottom 3 GB)
Instructing the DMA layer about these limitations is straight-forward,
even though we had to fix some issues regarding memory limits set in
the IORT for named components, and regarding the handling of ACPI _DMA
methods. However, the DMA layer also needs to be able to allocate
memory that is guaranteed to meet those DMA constraints, for bounce
buffering as well as allocating the backing for consistent mappings.
This is why the 1 GB ZONE_DMA was introduced recently. Unfortunately,
it turns out the having a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes
problems with kdump, and potentially in other places where allocations
cannot cross zone boundaries. Therefore, we should avoid having two
separate DMA zones when possible.
So let's do an early scan of the IORT, and only create the ZONE_DMA
if we encounter any devices that need it. This puts the burden on
the firmware to describe such limitations in the IORT, which may be
redundant (and less precise) if _DMA methods are also being provided.
However, it should be noted that this situation is highly unusual for
arm64 ACPI machines. Also, the DMA subsystem still gives precedence to
the _DMA method if implemented, and so we will not lose the ability to
perform streaming DMA outside the ZONE_DMA if the _DMA method permits
it.
Cc: Jeremy Linton
Cc: Lorenzo Pieralisi
Cc: Nicolas Saenz Julienne
Cc: Rob Herring
Cc: Christoph Hellwig
Cc: Robin Murphy
Cc: Hanjun Guo
Cc: Sudeep Holla
Cc: Anshuman Khandual
Signed-off-by: Ard Biesheuvel
[nsaenz: Rebased, removed documentation change and add declaration in
acpi_iort.h]
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v3:
- Use min_not_zero()
- Check ACPI revision
- Remove unnecessary #ifdef in zone_sizes_init()
arch/arm64/mm/init.c | 3 ++-
drivers/acpi/arm64/iort.c | 52 +++
include/linux/acpi_iort.h | 4 +++
3 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 94e38f99748b..f5d4f85506e4 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -190,7 +191,7 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
#ifdef CONFIG_ZONE_DMA
dt_zone_dma_bits = ilog2(of_dma_get_max_cpu_address(NULL));
- zone_dma_bits = min(32U, dt_zone_dma_bits);
+ zone_dma_bits = min3(32U, dt_zone_dma_bits,
acpi_iort_get_zone_dma_size());
arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 9929ff50c0c0..05fe4a076bab 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1718,3 +1718,55 @@ void __init acpi_iort_init(void)
iort_init_platform_devices();
}
+
+#ifdef CONFIG_ZONE_DMA
+/*
+ * Check the IORT whether any devices exist whose DMA mask is < 32 bits.
+ * If so, return the smallest value encountered, or 32 otherwise.
+ */
+unsigned int __init acpi_iort_get_zone_dma_size(void)
+{
+ struct acpi_table_iort *iort;
+ struct acpi_iort_node *node, *end;
+ acpi_status status;
+ u8 limit = 32;
+ int i;
+
+ if (acpi_disabled)
+ return limit;
+
+ status = acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **));
+ if (ACPI_FAILURE(status))
+ return limit;
+
+ node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
+ end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
+
+ for (i = 0; i < iort->node_count; i++) {
+ if (node >= end)
+ break;
+
+ switch (node->type) {
+ struct acpi_iort_named_component *ncomp;
+ struct acpi_iort_root_complex *rc;
+
+ case ACPI_IORT_NODE_NAMED_COMPONENT:
+ ncomp = (struct acpi_iort_named_component
*)node->node_data;
+ limit = min_not_zero(limit,
ncomp->memory_address_limit);
+ break;
+
+ case ACPI_IORT_NODE_PCI_ROOT_COMPLEX:
+ if (node->revision < 1)
+ break;
+
+ rc = (struct acpi_iort_root_complex *)node->node_data;
+ limit = min_not_zero(limit, rc->memory_address_limit);
+ break;
+ }
+ node = ACPI_ADD_PTR(st
[PATCH v4 4/7] of: unittest: Add test for of_dma_get_max_cpu_address()
Introduce a test for of_dma_get_max_cup_address(), it uses the same DT
data as the rest of dma-ranges unit tests.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v3:
- Remove HAS_DMA guards
drivers/of/unittest.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 06cc988faf78..b9a4d047a95e 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -869,6 +869,23 @@ static void __init of_unittest_changeset(void)
#endif
}
+static void __init of_unittest_dma_get_max_cpu_address(void)
+{
+ struct device_node *np;
+ phys_addr_t cpu_addr;
+
+ np = of_find_node_by_path("/testcase-data/address-tests");
+ if (!np) {
+ pr_err("missing testcase data\n");
+ return;
+ }
+
+ cpu_addr = of_dma_get_max_cpu_address(np);
+ unittest(cpu_addr == 0x5000,
+"of_dma_get_max_cpu_address: wrong CPU addr %pad (expecting
%x)\n",
+_addr, 0x5000);
+}
+
static void __init of_unittest_dma_ranges_one(const char *path,
u64 expect_dma_addr, u64 expect_paddr)
{
@@ -3266,6 +3283,7 @@ static int __init of_unittest(void)
of_unittest_changeset();
of_unittest_parse_interrupts();
of_unittest_parse_interrupts_extended();
+ of_unittest_dma_get_max_cpu_address();
of_unittest_parse_dma_ranges();
of_unittest_pci_dma_ranges();
of_unittest_match_node();
--
2.28.0
[PATCH v4 7/7] mm: Remove examples from enum zone_type comment
We can't really list every setup in common code. On top of that they are
unlikely to stay true for long as things change in the arch trees
independently of this comment.
Suggested-by: Christoph Hellwig
Signed-off-by: Nicolas Saenz Julienne
---
include/linux/mmzone.h | 20
1 file changed, 20 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fb3bf696c05e..9d0c454d23cd 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -354,26 +354,6 @@ enum zone_type {
* DMA mask is assumed when ZONE_DMA32 is defined. Some 64-bit
* platforms may need both zones as they support peripherals with
* different DMA addressing limitations.
-*
-* Some examples:
-*
-* - i386 and x86_64 have a fixed 16M ZONE_DMA and ZONE_DMA32 for the
-*rest of the lower 4G.
-*
-* - arm only uses ZONE_DMA, the size, up to 4G, may vary depending on
-*the specific device.
-*
-* - arm64 has a fixed 1G ZONE_DMA and ZONE_DMA32 for the rest of the
-*lower 4G.
-*
-* - powerpc only uses ZONE_DMA, the size, up to 2G, may vary
-*depending on the specific device.
-*
-* - s390 uses ZONE_DMA fixed to the lower 2G.
-*
-* - ia64 and riscv only use ZONE_DMA32.
-*
-* - parisc uses neither.
*/
#ifdef CONFIG_ZONE_DMA
ZONE_DMA,
--
2.28.0
[PATCH v4 1/7] arm64: mm: Move reserve_crashkernel() into mem_init()
crashkernel might reserve memory located in ZONE_DMA. We plan to delay ZONE_DMA's initialization after unflattening the devicetree and ACPI's boot table initialization, so move it later in the boot process. Specifically into mem_init(), this is the last place crashkernel will be able to reserve the memory before the page allocator kicks in. There isn't any apparent reason for doing this earlier. Signed-off-by: Nicolas Saenz Julienne --- arch/arm64/mm/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 095540667f0f..fc4ab0d6d5d2 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -386,8 +386,6 @@ void __init arm64_memblock_init(void) else arm64_dma32_phys_limit = PHYS_MASK + 1; - reserve_crashkernel(); - reserve_elfcorehdr(); high_memory = __va(memblock_end_of_DRAM() - 1) + 1; @@ -508,6 +506,8 @@ void __init mem_init(void) else swiotlb_force = SWIOTLB_NO_FORCE; + reserve_crashkernel(); + set_max_mapnr(max_pfn - PHYS_PFN_OFFSET); #ifndef CONFIG_SPARSEMEM_VMEMMAP -- 2.28.0
[PATCH v4 0/7] arm64: Default to 32-bit wide ZONE_DMA
Using two distinct DMA zones turned out to be problematic. Here's an attempt go back to a saner default. I tested this on both a RPi4 and QEMU. --- Changes since v3: - Drop patch adding define in dma-mapping - Address small review changes - Update Ard's patch - Add new patch removing examples from mmzone.h Changes since v2: - Introduce Ard's patch - Improve OF dma-ranges parsing function - Add unit test for OF function - Address small changes - Move crashkernel reservation later in boot process Changes since v1: - Parse dma-ranges instead of using machine compatible string Ard Biesheuvel (1): arm64: mm: Set ZONE_DMA size based on early IORT scan Nicolas Saenz Julienne (6): arm64: mm: Move reserve_crashkernel() into mem_init() arm64: mm: Move zone_dma_bits initialization into zone_sizes_init() of/address: Introduce of_dma_get_max_cpu_address() of: unittest: Add test for of_dma_get_max_cpu_address() arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges mm: Remove examples from enum zone_type comment arch/arm64/mm/init.c | 16 ++-- drivers/acpi/arm64/iort.c | 52 +++ drivers/of/address.c | 42 +++ drivers/of/unittest.c | 18 ++ include/linux/acpi_iort.h | 4 +++ include/linux/mmzone.h| 20 --- include/linux/of.h| 7 ++ 7 files changed, 130 insertions(+), 29 deletions(-) -- 2.28.0
Re: [RFC] of/platform: Create device link between simple-mfd and its children
Hi Uwe,
Sorry for the late reply, got distracted with other stuff.
On Mon, 2020-10-19 at 08:52 +0200, Uwe Kleine-König wrote:
> On Fri, Oct 16, 2020 at 05:26:56PM +0200, Nicolas Saenz Julienne wrote:
> > On Fri, 2020-10-16 at 09:38 -0500, Rob Herring wrote:
> > > On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne
> > > wrote:
> > > > 'simple-mfd' usage implies there might be some kind of resource sharing
> > > > between the parent device and its children.
> > >
> > > It does? No! The reason behind simple-mfd was specifically because
> > > there was no parent driver or dependency on the parent. No doubt
> > > simple-mfd has been abused.
> >
> > Fair enough, so we're doing things wrong. Just for the record, I'm looking
> > at
> > RPi´s firmware interface:
> >
> > firmware: firmware {
> > compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
> > #address-cells = <1>;
> > #size-cells = <1>;
> > mboxes = <>;
> >
> > firmware_clocks: clocks {
> > compatible = "raspberrypi,firmware-clocks";
> > #clock-cells = <1>;
> > };
> >
> > reset: reset {
> > compatible = "raspberrypi,firmware-reset";
> > #reset-cells = <1>;
> > };
> > [...]
> > };
> >
> > Note that "raspberrypi,bcm2835-firmware" has a driver, it's not just a
> > placeholder. Consumer drivers get a handle to RPi's firmware interface
> > through
> > the supplier's API, rpi_firmware_get(). The handle to firmware becomes
> > meaningless if it is unbinded, which I want to protect myself against.
> >
> > A simpler solution would be to manually create a device link between both
> > devices ("raspberrypi,bcm2835-firmware" and "raspberrypi,firmware-clocks"
> > for
> > example) upon calling rpi_firmware_get(). But I wanted to try addressing the
> > problem in a generic way first.
>
> IMHO rpi_firmware_get() should get a reference on the firmware device
> (and call try_module_get()) which prevents unbinding it.
Yes, it seems the way to go. Just one last question WRT this, since
'drv->remove(dev)' can't fail should I just block until the reference count
hits zero?
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [RFC] of/platform: Create device link between simple-mfd and its children
Hi Saravana, thanks for your comments. On Thu, 2020-10-15 at 09:52 -0700, Saravana Kannan wrote: > On Thu, Oct 15, 2020 at 4:43 AM Nicolas Saenz Julienne > wrote: > > 'simple-mfd' usage implies there might be some kind of resource sharing > > between the parent device and its children. By creating a device link > > with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time > > the parent device is unbound while leaving its children unaware that > > some of their resources disappeared. > > Doesn't the parent child relationship already ensure that? If not, > maybe that's what needs fixing? Well as Rob puts it, we're not using simple-mfd as it was intended. So that pretty much settles it for generic solutions. > > > Signed-off-by: Nicolas Saenz Julienne [...] > > > - If applying this to all simple-mfd devices is a bit too much, would > > this be acceptable for a specific device setup. For example RPi4's > > firmware interface (simple-mfd user) is passed to consumer drivers > > trough a custom API (see rpi_firmware_get()). So, when unbound, > > consumers are left with a firmware handle that points to nothing. > > You can always create device link between the real suppliers and consumers. RPi's firmware consumers use a custom API to get a handle to the firmware interface itself, rpi_firmware_get(). So no trace of the relationship is expressed in DT. If the firmware interface device, the supplier, is unbinded, that firmware handle now points nowhere and consumers will end up triggering kernel panics. Would it make sense to make a device link between the two in that case? Or I'd be, again, abusing the concept? Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [RFC] of/platform: Create device link between simple-mfd and its children
On Fri, 2020-10-16 at 09:38 -0500, Rob Herring wrote:
> On Thu, Oct 15, 2020 at 6:43 AM Nicolas Saenz Julienne
> wrote:
> > 'simple-mfd' usage implies there might be some kind of resource sharing
> > between the parent device and its children.
>
> It does? No! The reason behind simple-mfd was specifically because
> there was no parent driver or dependency on the parent. No doubt
> simple-mfd has been abused.
Fair enough, so we're doing things wrong. Just for the record, I'm looking at
RPi´s firmware interface:
firmware: firmware {
compatible = "raspberrypi,bcm2835-firmware", "simple-mfd";
#address-cells = <1>;
#size-cells = <1>;
mboxes = <>;
firmware_clocks: clocks {
compatible = "raspberrypi,firmware-clocks";
#clock-cells = <1>;
};
reset: reset {
compatible = "raspberrypi,firmware-reset";
#reset-cells = <1>;
};
[...]
};
Note that "raspberrypi,bcm2835-firmware" has a driver, it's not just a
placeholder. Consumer drivers get a handle to RPi's firmware interface through
the supplier's API, rpi_firmware_get(). The handle to firmware becomes
meaningless if it is unbinded, which I want to protect myself against.
A simpler solution would be to manually create a device link between both
devices ("raspberrypi,bcm2835-firmware" and "raspberrypi,firmware-clocks" for
example) upon calling rpi_firmware_get(). But I wanted to try addressing the
problem in a generic way first.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v3 7/8] arm64: mm: Set ZONE_DMA size based on early IORT scan
On Thu, 2020-10-15 at 22:26 +0800, Hanjun Guo wrote: > On 2020/10/15 3:12, Nicolas Saenz Julienne wrote: > > From: Ard Biesheuvel > > > > We recently introduced a 1 GB sized ZONE_DMA to cater for platforms > > incorporating masters that can address less than 32 bits of DMA, in > > particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has > > peripherals that can only address up to 1 GB (and its PCIe host > > bridge can only access the bottom 3 GB) > > > > Instructing the DMA layer about these limitations is straight-forward, > > even though we had to fix some issues regarding memory limits set in > > the IORT for named components, and regarding the handling of ACPI _DMA > > methods. However, the DMA layer also needs to be able to allocate > > memory that is guaranteed to meet those DMA constraints, for bounce > > buffering as well as allocating the backing for consistent mappings. > > > > This is why the 1 GB ZONE_DMA was introduced recently. Unfortunately, > > it turns out the having a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes > > problems with kdump, and potentially in other places where allocations > > cannot cross zone boundaries. Therefore, we should avoid having two > > separate DMA zones when possible. > > > > So let's do an early scan of the IORT, and only create the ZONE_DMA > > if we encounter any devices that need it. This puts the burden on > > the firmware to describe such limitations in the IORT, which may be > > redundant (and less precise) if _DMA methods are also being provided. > > However, it should be noted that this situation is highly unusual for > > arm64 ACPI machines. Also, the DMA subsystem still gives precedence to > > the _DMA method if implemented, and so we will not lose the ability to > > perform streaming DMA outside the ZONE_DMA if the _DMA method permits > > it. > > Sorry, I'm still a little bit confused. With this patch, if we have > a device which set the right _DMA method (DMA size >= 32), but with the > wrong DMA size in IORT, we still have the ZONE_DMA created which > is actually not needed? Yes, that would be the case. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 6/7] ARM: dts: Add dts for Raspberry Pi 4 + Cirrus Logic Lochnagar2
On Thu, 2020-10-15 at 12:14 +0100, Richard Fitzgerald wrote: > > Sadly I don't think creating a new device tree is a good solution here. If > > we > > were to do so for every RPi hat/usage it'd become unmanageable very fast. > > There > > is a way to maintain this in the open nonetheless. I suggest you build a DT > > overlay and submit it to https://github.com/raspberrypi/linux, see > > 'arch/arm/boot/dts/overlays.' The Raspberry Pi engineers have a kernel > > branch > > We want something in mainline so that it can be used by people > developing on mainline and taken as a starting point for configuring > the codecs for other host platforms. The RPi is a convenient platform to > use as the base because it is widely available and low-cost. If what you want to convey is the proper way of configuring your specific device the way to go is writing a devicetree binding. See Documentation/devicetree. It's even possible to validate a given devicetree against the bindings (given they are written in yaml format). Regards, Nicolas signature.asc Description: This is a digitally signed message part
[RFC] of/platform: Create device link between simple-mfd and its children
'simple-mfd' usage implies there might be some kind of resource sharing
between the parent device and its children. By creating a device link
with DL_FLAG_AUTOREMOVE_CONSUMER we make sure that at no point in time
the parent device is unbound while leaving its children unaware that
some of their resources disappeared.
Signed-off-by: Nicolas Saenz Julienne
---
Some questions:
- To what extent do we care about cleanly unbinding platform devices at
runtime? My rationale here is: "It's a platform device, for all you
know you might be unbinding someting essential to the system. So if
you're doing it, you better know what you're doing."
- Would this be an abuse of device links?
- If applying this to all simple-mfd devices is a bit too much, would
this be acceptable for a specific device setup. For example RPi4's
firmware interface (simple-mfd user) is passed to consumer drivers
trough a custom API (see rpi_firmware_get()). So, when unbound,
consumers are left with a firmware handle that points to nothing.
drivers/of/platform.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index b557a0fcd4ba..8d5b55b81764 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -390,8 +390,14 @@ 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))
- return 0;
+ if (!dev)
+ return 0;
+
+ if (parent && of_device_is_compatible(parent->of_node, "simple-mfd"))
+ device_link_add(>dev, parent, DL_FLAG_AUTOREMOVE_CONSUMER);
+
+ if (!of_match_node(matches, bus))
+ return 0;
for_each_child_of_node(bus, child) {
pr_debug(" create child: %pOF\n", child);
--
2.28.0
Re: [PATCH 6/7] ARM: dts: Add dts for Raspberry Pi 4 + Cirrus Logic Lochnagar2
Hi Richard, your series is very welcome, upstream support for audio codecs on the RPi4 has always been lackluster. Could you provide more information on the actual products? Are there custom made hats for the RPi4 or this wired into a generic development board. On Wed, 2020-10-14 at 15:54 +0100, Richard Fitzgerald wrote: > This is based on the default bcm2711-rpi-4-b.dts. Note that you could've included bcm2711-rpi-4.dts (as if it was a .dtsi). > Configurations are provided for Cirrus Logic codecs CS42L92, CS47L15, > CS47L24, CS47L35, CS47L90 and WM8998. > > For each codec there is a sound node and a codec device node and both > default to disabled. Enable the pair for the codec in use. > > Signed-off-by: Richard Fitzgerald > --- Sadly I don't think creating a new device tree is a good solution here. If we were to do so for every RPi hat/usage it'd become unmanageable very fast. There is a way to maintain this in the open nonetheless. I suggest you build a DT overlay and submit it to https://github.com/raspberrypi/linux, see 'arch/arm/boot/dts/overlays.' The Raspberry Pi engineers have a kernel branch that tracks of the latest kernel release, so once you get the rest of patches sorted out and they are included in a release it'll make sense to do so. I can't tell for other distros, but opensuse packages overlays, so the effort will ultimately be useful to users. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v3 6/8] arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges
On Thu, 2020-10-15 at 07:39 +0200, Christoph Hellwig wrote: > On Wed, Oct 14, 2020 at 09:12:08PM +0200, Nicolas Saenz Julienne wrote: > > + zone_dma_bits = min(zone_dma_bits, > > + (unsigned > > int)ilog2(of_dma_get_max_cpu_address(NULL))); > > Plase avoid pointlessly long lines. Especially if it is completely trivial > by using either min_t or not overindenting like here. Noted signature.asc Description: This is a digitally signed message part
Re: [PATCH v3 5/8] dma-direct: Turn zone_dma_bits default value into a define
On Thu, 2020-10-15 at 07:38 +0200, Christoph Hellwig wrote: > On Wed, Oct 14, 2020 at 09:12:07PM +0200, Nicolas Saenz Julienne wrote: > > Set zone_dma_bits default value through a define so as for architectures > > to be able to override it with their default value. > > Architectures can do that already by assigning a value to zone_dma_bits > at runtime. I really do not want to add the extra clutter. I'll remove it then. signature.asc Description: This is a digitally signed message part
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 2020-10-15 at 07:42 +0200, Christoph Hellwig wrote:
> > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
> > +{
> > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX;
> > + struct of_range_parser parser;
> > + phys_addr_t subtree_max_addr;
> > + struct device_node *child;
> > + phys_addr_t cpu_end = 0;
> > + struct of_range range;
> > + const __be32 *ranges;
> > + int len;
> > +
> > + if (!np)
> > + np = of_root;
>
> Requiring of_root to be passed explicitly would seem more natural
> to me than the magic NULL argument. There doesn't seem to be any
> precedent for that kind of calling convention either.
I inspired that behavior from __of_find_all_nodes(). I'll change it.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v3 4/8] of: unittest: Add test for of_dma_get_max_cpu_address()
On Wed, 2020-10-14 at 17:04 -0500, Rob Herring wrote:
> On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne
> wrote:
> > Introduce a test for of_dma_get_max_cup_address(), it uses the same DT
> > data as the rest of dma-ranges unit tests.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> > drivers/of/unittest.c | 20
> > 1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
> > index 06cc988faf78..2cbf2a585c9f 100644
> > --- a/drivers/of/unittest.c
> > +++ b/drivers/of/unittest.c
> > @@ -869,6 +869,25 @@ static void __init of_unittest_changeset(void)
> > #endif
> > }
> >
> > +static void __init of_unittest_dma_get_max_cpu_address(void)
> > +{
> > +#ifdef CONFIG_HAS_DMA
>
> Can't the unittest run without this? I run the unittests under UML.
It was cargo culted from its sibling of_unittest_dma_ranges_one(), now that you
mention it, I can't seem to find the reason why it's here in the first place,
nor for other similar usages in OF code.
I ran the test in UML with all HAS_DMA conditionals removed from OF code and
things went well. I'll prepare a fix for that.
> > + struct device_node *np;
> > + phys_addr_t cpu_addr;
> > +
> > + np = of_find_node_by_path("/testcase-data/address-tests");
> > + if (!np) {
> > + pr_err("missing testcase data\n");
> > + return;
> > + }
> > +
> > + cpu_addr = of_dma_get_max_cpu_address(np);
> > + unittest(cpu_addr == 0x5000ULL,
> > +"of_dma_get_max_cpu_address: wrong CPU addr %pad
> > (expecting %llx)\n",
> > +_addr, 0x5000ULL);
> > +#endif
> > +}
> > +
> > static void __init of_unittest_dma_ranges_one(const char *path,
> > u64 expect_dma_addr, u64 expect_paddr)
> > {
> > @@ -3266,6 +3285,7 @@ static int __init of_unittest(void)
> > of_unittest_changeset();
> > of_unittest_parse_interrupts();
> > of_unittest_parse_interrupts_extended();
> > + of_unittest_dma_get_max_cpu_address();
> > of_unittest_parse_dma_ranges();
> > of_unittest_pci_dma_ranges();
> > of_unittest_match_node();
> > --
> > 2.28.0
> >
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Thu, 2020-10-15 at 08:56 +0200, Ard Biesheuvel wrote:
> On Thu, 15 Oct 2020 at 00:03, Rob Herring wrote:
> > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne
> > wrote:
> > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU
> > > physical address addressable by all DMA masters in the system. It's
> > > specially useful for setting memory zones sizes at early boot time.
> > >
> > > Signed-off-by: Nicolas Saenz Julienne
> > >
> > > ---
> > >
> > > Changes since v2:
> > > - Use PHYS_ADDR_MAX
> > > - return phys_dma_t
> > > - Rename function
> > > - Correct subject
> > > - Add support to start parsing from an arbitrary device node in order
> > >for the function to work with unit tests
> > >
> > > drivers/of/address.c | 42 ++
> > > include/linux/of.h | 7 +++
> > > 2 files changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index eb9ab4f1e80b..b5a9695aaf82 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const
> > > struct bus_dma_region **map)
> > > }
> > > #endif /* CONFIG_HAS_DMA */
> > >
> > > +/**
> > > + * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA
> > > + * @np: The node to start searching from or NULL to start from the root
> > > + *
> > > + * Gets the highest CPU physical address that is addressable by all DMA
> > > masters
> > > + * in the system (or subtree when np is non-NULL). If no DMA constrained
> > > device
> > > + * is found, it returns PHYS_ADDR_MAX.
> > > + */
> > > +phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
> > > +{
> > > + phys_addr_t max_cpu_addr = PHYS_ADDR_MAX;
> >
> > One issue with using phys_addr_t is it may be 32-bit even though the
> > DT is 64-bit addresses. LPAE capable system with LPAE disabled. Maybe
> > the truncation is fine here? Maybe not.
> >
>
> PHYS_ADDR_MAX is the max addressable CPU address on the system, and so
> it makes sense to use it for the return type, and for the preliminary
> return value: this is actually what /prevents/ truncation, because we
> will only overwrite max_cpu_addr if the new u64 value is lower.
>
Actually I now see how things might go south.
> > > + if (ranges && len) {
> > > + of_dma_range_parser_init(, np);
> > > + for_each_of_range(, )
> > > + if (range.cpu_addr + range.size > cpu_end)
> > > + cpu_end = range.cpu_addr + range.size;
If cpu_end hits 0x1_, it'll overflow to 0. This is possible on 32-bit
systems (LPAE or not). And something similar might happen on LPAE disabled
systems.
I could add some extra logic, something like:
/* We overflowed */
if (cpu_end < range.cpu_addr)
cpu_end = PHYS_ADDR_MAX;
Which is not perfect but will cover most sensible cases.
Or simply deal internally in u64s, and upon returning, check if "max_cpu_addr"
falls higher than PHYS_ADDR_MAX.
> > > +
> > > + if (max_cpu_addr > cpu_end)
> > > + max_cpu_addr = cpu_end;
> > > + }
> > > +
> > > + for_each_available_child_of_node(np, child) {
> > > + subtree_max_addr = of_dma_get_max_cpu_address(child);
> > > + if (max_cpu_addr > subtree_max_addr)
> > > + max_cpu_addr = subtree_max_addr;
> > > + }
> > > +
> > > + return max_cpu_addr;
> > > +}
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v3 1/8] arm64: mm: Move reserve_crashkernel() into mem_init()
On Thu, 2020-10-15 at 09:40 +0100, Will Deacon wrote: > On Wed, Oct 14, 2020 at 09:12:03PM +0200, Nicolas Saenz Julienne wrote: > > crashkernel might reserve memory located in ZONE_DMA. We plan to delay > > ZONE_DMA's initialization after unflattening the devicetree and ACPI's > > boot table initialization, so move it later in the boot process. > > Specifically into mem_init(), this is the last place crashkernel will be > > able to reserve the memory before the page allocator kicks in and there is > > no need to do it earlier. > > > > Signed-off-by: Nicolas Saenz Julienne > > --- > > arch/arm64/mm/init.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > Please can you cc me on the whole series next time? I know different > maintainers have different preferences here, but I find it much easier to > figure out what's happening when I can see all of the changes together. Will do. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
On Wed, 2020-10-14 at 17:02 -0500, Rob Herring wrote: > On Wed, Oct 14, 2020 at 2:12 PM Nicolas Saenz Julienne > wrote: > > Introduce of_dma_get_max_cpu_address(), which provides the highest CPU > > physical address addressable by all DMA masters in the system. It's > > specially useful for setting memory zones sizes at early boot time. > > > > Signed-off-by: Nicolas Saenz Julienne > > > > --- [...] > > + struct of_range_parser parser; > > + phys_addr_t subtree_max_addr; > > + struct device_node *child; > > + phys_addr_t cpu_end = 0; > > + struct of_range range; > > + const __be32 *ranges; > > + int len; > > + > > + if (!np) > > + np = of_root; > > + > > + ranges = of_get_property(np, "dma-ranges", ); > > I'm not really following why you changed the algorithm here. You're > skipping disabled nodes which is good. Was there some other reason? Yes, it's a little more complex. But I had to change it in order to be able to start parsing down from an arbitrary device node, which is needed for the unit tests. for_each_of_allnodes() and friends will traverse the whole tree, regardless of the starting point. I couldn't find a similar function that would just iterate over a subsection of the tree, so I went with this recursive approach. Regards, Nicolas signature.asc Description: This is a digitally signed message part
[PATCH v3 6/8] arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges
We recently introduced a 1 GB sized ZONE_DMA to cater for platforms
incorporating masters that can address less than 32 bits of DMA, in
particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has
peripherals that can only address up to 1 GB (and its PCIe host
bridge can only access the bottom 3 GB)
The DMA layer also needs to be able to allocate memory that is
guaranteed to meet those DMA constraints, for bounce buffering as well
as allocating the backing for consistent mappings. This is why the 1 GB
ZONE_DMA was introduced recently. Unfortunately, it turns out the having
a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes problems with kdump, and
potentially in other places where allocations cannot cross zone
boundaries. Therefore, we should avoid having two separate DMA zones
when possible.
So, with the help of of_dma_get_max_cpu_address() get the topmost
physical address accessible to all DMA masters in system and use that
information to fine-tune ZONE_DMA's size. In the absence of addressing
limited masters ZONE_DMA will span the whole 32-bit address space,
otherwise, in the case of the Raspberry Pi 4 it'll only span the 30-bit
address space, and have ZONE_DMA32 cover the rest of the 32-bit address
space.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v2:
- Updated commit log by shamelessly copying Ard's ACPI counterpart commit log
arch/arm64/include/asm/processor.h | 1 +
arch/arm64/mm/init.c | 5 ++---
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/processor.h
b/arch/arm64/include/asm/processor.h
index fce8cbecd6bc..c09d3f1a9a6b 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -97,6 +97,7 @@
extern phys_addr_t arm64_dma_phys_limit;
#define ARCH_LOW_ADDRESS_LIMIT (arm64_dma_phys_limit - 1)
+#define ZONE_DMA_BITS_DEFAULT 32
struct debug_info {
#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index ef0ef0087e2c..97b0d2768349 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -42,8 +42,6 @@
#include
#include
-#define ARM64_ZONE_DMA_BITS30
-
/*
* We need to be able to catch inadvertent references to memstart_addr
* that occur (potentially in generic code) before arm64_memblock_init()
@@ -196,7 +194,8 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
#ifdef CONFIG_ZONE_DMA
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
+ zone_dma_bits = min(zone_dma_bits,
+ (unsigned
int)ilog2(of_dma_get_max_cpu_address(NULL)));
arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
--
2.28.0
[PATCH v3 3/8] of/address: Introduce of_dma_get_max_cpu_address()
Introduce of_dma_get_max_cpu_address(), which provides the highest CPU
physical address addressable by all DMA masters in the system. It's
specially useful for setting memory zones sizes at early boot time.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v2:
- Use PHYS_ADDR_MAX
- return phys_dma_t
- Rename function
- Correct subject
- Add support to start parsing from an arbitrary device node in order
for the function to work with unit tests
drivers/of/address.c | 42 ++
include/linux/of.h | 7 +++
2 files changed, 49 insertions(+)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index eb9ab4f1e80b..b5a9695aaf82 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -1024,6 +1024,48 @@ int of_dma_get_range(struct device_node *np, const
struct bus_dma_region **map)
}
#endif /* CONFIG_HAS_DMA */
+/**
+ * of_dma_get_max_cpu_address - Gets highest CPU address suitable for DMA
+ * @np: The node to start searching from or NULL to start from the root
+ *
+ * Gets the highest CPU physical address that is addressable by all DMA masters
+ * in the system (or subtree when np is non-NULL). If no DMA constrained device
+ * is found, it returns PHYS_ADDR_MAX.
+ */
+phys_addr_t __init of_dma_get_max_cpu_address(struct device_node *np)
+{
+ phys_addr_t max_cpu_addr = PHYS_ADDR_MAX;
+ struct of_range_parser parser;
+ phys_addr_t subtree_max_addr;
+ struct device_node *child;
+ phys_addr_t cpu_end = 0;
+ struct of_range range;
+ const __be32 *ranges;
+ int len;
+
+ if (!np)
+ np = of_root;
+
+ ranges = of_get_property(np, "dma-ranges", );
+ if (ranges && len) {
+ of_dma_range_parser_init(, np);
+ for_each_of_range(, )
+ if (range.cpu_addr + range.size > cpu_end)
+ cpu_end = range.cpu_addr + range.size;
+
+ if (max_cpu_addr > cpu_end)
+ max_cpu_addr = cpu_end;
+ }
+
+ for_each_available_child_of_node(np, child) {
+ subtree_max_addr = of_dma_get_max_cpu_address(child);
+ if (max_cpu_addr > subtree_max_addr)
+ max_cpu_addr = subtree_max_addr;
+ }
+
+ return max_cpu_addr;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np:device node
diff --git a/include/linux/of.h b/include/linux/of.h
index 481ec0467285..db8db8f2c967 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id,
const char *map_name, const char *map_mask_name,
struct device_node **target, u32 *id_out);
+phys_addr_t of_dma_get_max_cpu_address(struct device_node *np);
+
#else /* CONFIG_OF */
static inline void of_core_init(void)
@@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id,
return -EINVAL;
}
+static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
+{
+ return PHYS_ADDR_MAX;
+}
+
#define of_match_ptr(_ptr) NULL
#define of_match_node(_matches, _node) NULL
#endif /* CONFIG_OF */
--
2.28.0
[PATCH v3 2/8] arm64: mm: Move zone_dma_bits initialization into zone_sizes_init()
zone_dma_bits's initialization happens earlier that it's actually
needed, in arm64_memblock_init(). So move it into the more suitable
zone_sizes_init().
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 1d29f2ca81c7..ef0ef0087e2c 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -196,6 +196,8 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
#ifdef CONFIG_ZONE_DMA
+ zone_dma_bits = ARM64_ZONE_DMA_BITS;
+ arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
#ifdef CONFIG_ZONE_DMA32
@@ -386,11 +388,6 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
- if (IS_ENABLED(CONFIG_ZONE_DMA)) {
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
- arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
- }
-
if (IS_ENABLED(CONFIG_ZONE_DMA32))
arm64_dma32_phys_limit = max_zone_phys(32);
else
--
2.28.0
[PATCH v3 0/8] arm64: Default to 32-bit wide ZONE_DMA
Using two distinct DMA zones turned out to be problematic. Here's an attempt go back to a saner default. I tested this on both a RPi4 and QEMU. --- Changes since v2: - Introduce Ard's patch - Improve OF dma-ranges parsing function - Add unit test for OF function - Address small changes - Move crashkernel reservation later in boot process Changes since v1: - Parse dma-ranges instead of using machine compatible string Ard Biesheuvel (1): arm64: mm: Set ZONE_DMA size based on early IORT scan Nicolas Saenz Julienne (7): arm64: mm: Move reserve_crashkernel() into mem_init() arm64: mm: Move zone_dma_bits initialization into zone_sizes_init() of/address: Introduce of_dma_get_max_cpu_address() of: unittest: Add test for of_dma_get_max_cpu_address() dma-direct: Turn zone_dma_bits default value into a define arm64: mm: Set ZONE_DMA size based on devicetree's dma-ranges mm: Update DMA zones description arch/arm64/include/asm/processor.h | 1 + arch/arm64/mm/init.c | 20 ++-- drivers/acpi/arm64/iort.c | 51 ++ drivers/of/address.c | 42 drivers/of/unittest.c | 20 include/linux/acpi_iort.h | 4 +++ include/linux/dma-direct.h | 3 ++ include/linux/mmzone.h | 5 +-- include/linux/of.h | 7 kernel/dma/direct.c| 2 +- 10 files changed, 143 insertions(+), 12 deletions(-) -- 2.28.0
[PATCH v3 1/8] arm64: mm: Move reserve_crashkernel() into mem_init()
crashkernel might reserve memory located in ZONE_DMA. We plan to delay ZONE_DMA's initialization after unflattening the devicetree and ACPI's boot table initialization, so move it later in the boot process. Specifically into mem_init(), this is the last place crashkernel will be able to reserve the memory before the page allocator kicks in and there is no need to do it earlier. Signed-off-by: Nicolas Saenz Julienne --- arch/arm64/mm/init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index a53c1e0fb017..1d29f2ca81c7 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -396,8 +396,6 @@ void __init arm64_memblock_init(void) else arm64_dma32_phys_limit = PHYS_MASK + 1; - reserve_crashkernel(); - reserve_elfcorehdr(); high_memory = __va(memblock_end_of_DRAM() - 1) + 1; @@ -518,6 +516,8 @@ void __init mem_init(void) else swiotlb_force = SWIOTLB_NO_FORCE; + reserve_crashkernel(); + set_max_mapnr(max_pfn - PHYS_PFN_OFFSET); #ifndef CONFIG_SPARSEMEM_VMEMMAP -- 2.28.0
[PATCH v3 7/8] arm64: mm: Set ZONE_DMA size based on early IORT scan
From: Ard Biesheuvel
We recently introduced a 1 GB sized ZONE_DMA to cater for platforms
incorporating masters that can address less than 32 bits of DMA, in
particular the Raspberry Pi 4, which has 4 or 8 GB of DRAM, but has
peripherals that can only address up to 1 GB (and its PCIe host
bridge can only access the bottom 3 GB)
Instructing the DMA layer about these limitations is straight-forward,
even though we had to fix some issues regarding memory limits set in
the IORT for named components, and regarding the handling of ACPI _DMA
methods. However, the DMA layer also needs to be able to allocate
memory that is guaranteed to meet those DMA constraints, for bounce
buffering as well as allocating the backing for consistent mappings.
This is why the 1 GB ZONE_DMA was introduced recently. Unfortunately,
it turns out the having a 1 GB ZONE_DMA as well as a ZONE_DMA32 causes
problems with kdump, and potentially in other places where allocations
cannot cross zone boundaries. Therefore, we should avoid having two
separate DMA zones when possible.
So let's do an early scan of the IORT, and only create the ZONE_DMA
if we encounter any devices that need it. This puts the burden on
the firmware to describe such limitations in the IORT, which may be
redundant (and less precise) if _DMA methods are also being provided.
However, it should be noted that this situation is highly unusual for
arm64 ACPI machines. Also, the DMA subsystem still gives precedence to
the _DMA method if implemented, and so we will not lose the ability to
perform streaming DMA outside the ZONE_DMA if the _DMA method permits
it.
Cc: Jeremy Linton
Cc: Lorenzo Pieralisi
Cc: Nicolas Saenz Julienne
Cc: Rob Herring
Cc: Christoph Hellwig
Cc: Robin Murphy
Cc: Hanjun Guo
Cc: Sudeep Holla
Cc: Anshuman Khandual
Signed-off-by: Ard Biesheuvel
[nsaenz: Rebased, removed documentation change, warnings and add
declaration in acpi_iort.h]
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 6 +
drivers/acpi/arm64/iort.c | 51 +++
include/linux/acpi_iort.h | 4 +++
3 files changed, 61 insertions(+)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 97b0d2768349..f321761eedb2 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -196,6 +197,11 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
#ifdef CONFIG_ZONE_DMA
zone_dma_bits = min(zone_dma_bits,
(unsigned
int)ilog2(of_dma_get_max_cpu_address(NULL)));
+
+ if (IS_ENABLED(CONFIG_ACPI))
+ zone_dma_bits = min(zone_dma_bits,
+ acpi_iort_get_zone_dma_size());
+
arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
index 9929ff50c0c0..8f530bf3c03b 100644
--- a/drivers/acpi/arm64/iort.c
+++ b/drivers/acpi/arm64/iort.c
@@ -1718,3 +1718,54 @@ void __init acpi_iort_init(void)
iort_init_platform_devices();
}
+
+#ifdef CONFIG_ZONE_DMA
+/*
+ * Check the IORT whether any devices exist whose DMA mask is < 32 bits.
+ * If so, return the smallest value encountered, or 32 otherwise.
+ */
+unsigned int __init acpi_iort_get_zone_dma_size(void)
+{
+ struct acpi_table_iort *iort;
+ struct acpi_iort_node *node, *end;
+ acpi_status status;
+ u8 limit = 32;
+ int i;
+
+ if (acpi_disabled)
+ return limit;
+
+ status = acpi_get_table(ACPI_SIG_IORT, 0,
+ (struct acpi_table_header **));
+ if (ACPI_FAILURE(status))
+ return limit;
+
+ node = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->node_offset);
+ end = ACPI_ADD_PTR(struct acpi_iort_node, iort, iort->header.length);
+
+ for (i = 0; i < iort->node_count; i++) {
+ if (node >= end)
+ break;
+
+ switch (node->type) {
+ struct acpi_iort_named_component *ncomp;
+ struct acpi_iort_root_complex *rc;
+
+ case ACPI_IORT_NODE_NAMED_COMPONENT:
+ ncomp = (struct acpi_iort_named_component
*)node->node_data;
+ if (ncomp->memory_address_limit)
+ limit = min(limit, ncomp->memory_address_limit);
+ break;
+
+ case ACPI_IORT_NODE_PCI_ROOT_COMPLEX:
+ rc = (struct acpi_iort_root_complex *)node->node_data;
+ if (rc->memory_address_limit)
+ limit = min(limit, rc->memory_address_limit);
+ break;
+ }
+ node = ACPI_ADD_PTR(struct acpi_iort_node, node, node->
[PATCH v3 8/8] mm: Update DMA zones description
The default behavior for arm64 changed, so reflect that.
Signed-off-by: Nicolas Saenz Julienne
Acked-by: Catalin Marinas
---
include/linux/mmzone.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fb3bf696c05e..4ee2306351b9 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -363,8 +363,9 @@ enum zone_type {
* - arm only uses ZONE_DMA, the size, up to 4G, may vary depending on
*the specific device.
*
-* - arm64 has a fixed 1G ZONE_DMA and ZONE_DMA32 for the rest of the
-*lower 4G.
+* - arm64 uses a single 4GB ZONE_DMA, except on the Raspberry Pi 4,
+*in which ZONE_DMA covers the first GB and ZONE_DMA32 the rest of
+*the lower 4GB.
*
* - powerpc only uses ZONE_DMA, the size, up to 2G, may vary
*depending on the specific device.
--
2.28.0
[PATCH v3 5/8] dma-direct: Turn zone_dma_bits default value into a define
Set zone_dma_bits default value through a define so as for architectures to be able to override it with their default value. Signed-off-by: Nicolas Saenz Julienne --- include/linux/dma-direct.h | 3 +++ kernel/dma/direct.c| 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index 18aade195884..e433d90cbacf 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -12,6 +12,9 @@ #include #include +#ifndef ZONE_DMA_BITS_DEFAULT +#define ZONE_DMA_BITS_DEFAULT 24 +#endif extern unsigned int zone_dma_bits; /* diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 06c111544f61..c0d97f536e93 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -20,7 +20,7 @@ * it for entirely different regions. In that case the arch code needs to * override the variable below for dma-direct to work properly. */ -unsigned int zone_dma_bits __ro_after_init = 24; +unsigned int zone_dma_bits __ro_after_init = ZONE_DMA_BITS_DEFAULT; static inline dma_addr_t phys_to_dma_direct(struct device *dev, phys_addr_t phys) -- 2.28.0
[PATCH v3 4/8] of: unittest: Add test for of_dma_get_max_cpu_address()
Introduce a test for of_dma_get_max_cup_address(), it uses the same DT
data as the rest of dma-ranges unit tests.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/of/unittest.c | 20
1 file changed, 20 insertions(+)
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 06cc988faf78..2cbf2a585c9f 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -869,6 +869,25 @@ static void __init of_unittest_changeset(void)
#endif
}
+static void __init of_unittest_dma_get_max_cpu_address(void)
+{
+#ifdef CONFIG_HAS_DMA
+ struct device_node *np;
+ phys_addr_t cpu_addr;
+
+ np = of_find_node_by_path("/testcase-data/address-tests");
+ if (!np) {
+ pr_err("missing testcase data\n");
+ return;
+ }
+
+ cpu_addr = of_dma_get_max_cpu_address(np);
+ unittest(cpu_addr == 0x5000ULL,
+"of_dma_get_max_cpu_address: wrong CPU addr %pad (expecting
%llx)\n",
+_addr, 0x5000ULL);
+#endif
+}
+
static void __init of_unittest_dma_ranges_one(const char *path,
u64 expect_dma_addr, u64 expect_paddr)
{
@@ -3266,6 +3285,7 @@ static int __init of_unittest(void)
of_unittest_changeset();
of_unittest_parse_interrupts();
of_unittest_parse_interrupts_extended();
+ of_unittest_dma_get_max_cpu_address();
of_unittest_parse_dma_ranges();
of_unittest_pci_dma_ranges();
of_unittest_match_node();
--
2.28.0
Re: [PATCH v2 2/5] of/address: Introduce of_dma_lower_bus_limit()
Hi Rob,
On Mon, 2020-10-12 at 10:25 -0500, Rob Herring wrote:
> On Sat, Oct 10, 2020 at 10:12 AM Nicolas Saenz Julienne
> wrote:
> > The function provides the CPU physical address addressable by the most
> > constrained bus in the system. It might be useful in order to
> > dynamically set up memory zones during boot.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> > drivers/of/address.c | 34 ++
> > include/linux/of.h | 7 +++
> > 2 files changed, 41 insertions(+)
> >
> > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > index eb9ab4f1e80b..755e97b65096 100644
> > --- a/drivers/of/address.c
> > +++ b/drivers/of/address.c
> > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const
> > struct bus_dma_region **map)
> > }
> > #endif /* CONFIG_HAS_DMA */
> >
> > +/**
> > + * of_dma_safe_phys_limit - Get system wide DMA safe address space
> > + *
> > + * Gets the CPU physical address limit for safe DMA addressing system wide
> > by
> > + * searching for the most constraining dma-range. Otherwise it returns
> > ~0ULL.
> > + */
> > +u64 __init of_dma_safe_phys_limit(void)
> > +{
> > + struct device_node *np = NULL;
> > + struct of_range_parser parser;
> > + const __be32 *ranges = NULL;
> > + u64 phys_dma_limit = ~0ULL;
> > + struct of_range range;
> > + int len;
> > +
> > + for_each_of_allnodes(np) {
> > + dma_addr_t cpu_end = 0;
> > +
> > + ranges = of_get_property(np, "dma-ranges", );
> > + if (!ranges || !len)
> > + continue;
> > +
> > + of_dma_range_parser_init(, np);
> > + for_each_of_range(, )
> > + if (range.cpu_addr + range.size > cpu_end)
> > + cpu_end = range.cpu_addr + range.size;
>
> This doesn't work if you have more than one level of dma-ranges. The
> address has to be translated first. It should be okay to do that on
> the start or end address (if not, your DT is broken).
for_each_of_range() calls of_pci_range_parser_one() which utimately populates
range.cpu_addr with of_translate_dma_address() results. Isn't that good enough?
> Please add/extend a unittest for this.
Will do.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2 2/5] of/address: Introduce of_dma_lower_bus_limit()
On Sun, 2020-10-11 at 09:47 +0200, Ard Biesheuvel wrote: > Hi Nicolas, > > $SUBJECT is out of sync with the patch below. Also, for legibility, it > helps if the commit log is intelligible by itself, rather than relying > on $SUBJECT being the first line of the first paragraph. Noted, I'll update all commit logs. > On Sat, 10 Oct 2020 at 17:12, Nicolas Saenz Julienne > wrote: > > The function provides the CPU physical address addressable by the most > > constrained bus in the system. It might be useful in order to > > dynamically set up memory zones during boot. > > > > Signed-off-by: Nicolas Saenz Julienne > > --- > > drivers/of/address.c | 34 ++ > > include/linux/of.h | 7 +++ > > 2 files changed, 41 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index eb9ab4f1e80b..755e97b65096 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const > > struct bus_dma_region **map) > > } > > #endif /* CONFIG_HAS_DMA */ > > > > +/** > > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > > + * > > + * Gets the CPU physical address limit for safe DMA addressing system wide > > by > > + * searching for the most constraining dma-range. Otherwise it returns > > ~0ULL. > > + */ > > +u64 __init of_dma_safe_phys_limit(void) > > I don't think 'safe' strikes the right tone here. You are looking for > the highest CPU address that is addressable by all DMA masters in the > system. > > Something like > > of_dma_get_max_cpu_address(void) > > perhaps? Also, since this is generic code, phys_addr_t is probably a > better type to return. Sonds good to me, I dindn't like the name I used either. Will use with phys_addr_t. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] pwm: Add Raspberry Pi Firmware based PWM bus
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, 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
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/3] dt-bindings: pwm: Add binding for RPi firmware PWM bus
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 > > > > --- > > > > .../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 signature.asc Description: This is a digitally signed message part
Re: [PATCH 3/3] pwm: Add Raspberry Pi Firmware based PWM bus
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
> > ---
> > 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 = >dev;
> > + struct rpi_firmware *firmware;
>
Re: [PATCH 1/3] dt-bindings: pwm: Add binding for RPi firmware PWM bus
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 > > --- > > .../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 signature.asc Description: This is a digitally signed message part
[PATCH v2 4/5] arm64: mm: Dynamically resize zone_dma_bits based on system's constraints
With the help of of_dma_safe_phys_limit() we can get the topmost
physical address accessible for DMA to the whole system and use that
information to properly setup zone_dma_bits.
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/include/asm/processor.h | 1 +
arch/arm64/mm/init.c | 5 ++---
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/include/asm/processor.h
b/arch/arm64/include/asm/processor.h
index fce8cbecd6bc..c09d3f1a9a6b 100644
--- a/arch/arm64/include/asm/processor.h
+++ b/arch/arm64/include/asm/processor.h
@@ -97,6 +97,7 @@
extern phys_addr_t arm64_dma_phys_limit;
#define ARCH_LOW_ADDRESS_LIMIT (arm64_dma_phys_limit - 1)
+#define ZONE_DMA_BITS_DEFAULT 32
struct debug_info {
#ifdef CONFIG_HAVE_HW_BREAKPOINT
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index 0eca5865dcb1..5934df93bf4d 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -42,8 +42,6 @@
#include
#include
-#define ARM64_ZONE_DMA_BITS30
-
/*
* We need to be able to catch inadvertent references to memstart_addr
* that occur (potentially in generic code) before arm64_memblock_init()
@@ -196,7 +194,8 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
#ifdef CONFIG_ZONE_DMA
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
+ zone_dma_bits = min(zone_dma_bits,
+ (unsigned int)ilog2(of_dma_safe_phys_limit()));
if (IS_ENABLED(CONFIG_ACPI)) {
extern unsigned int acpi_iort_get_zone_dma_size(void);
--
2.28.0
[PATCH v2 1/5] arm64: mm: Move zone_dma_bits initialization into zone_sizes_init()
There no use for initializing it earlier in arm64_memblock_init().
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index f6902a2b4ea6..0eca5865dcb1 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -196,14 +196,16 @@ static void __init zone_sizes_init(unsigned long min,
unsigned long max)
unsigned long max_zone_pfns[MAX_NR_ZONES] = {0};
#ifdef CONFIG_ZONE_DMA
+ zone_dma_bits = ARM64_ZONE_DMA_BITS;
+
if (IS_ENABLED(CONFIG_ACPI)) {
extern unsigned int acpi_iort_get_zone_dma_size(void);
zone_dma_bits = min(zone_dma_bits,
acpi_iort_get_zone_dma_size());
- arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
}
+ arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
#endif
#ifdef CONFIG_ZONE_DMA32
@@ -394,11 +396,6 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
- if (IS_ENABLED(CONFIG_ZONE_DMA)) {
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
- arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
- }
-
if (IS_ENABLED(CONFIG_ZONE_DMA32))
arm64_dma32_phys_limit = max_zone_phys(32);
else
--
2.28.0
[PATCH v2 5/5] mm: Update DMA zones description
The default behavior for arm64 changed, so reflect that.
Signed-off-by: Nicolas Saenz Julienne
Acked-by: Catalin Marinas
---
include/linux/mmzone.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fb3bf696c05e..4ee2306351b9 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -363,8 +363,9 @@ enum zone_type {
* - arm only uses ZONE_DMA, the size, up to 4G, may vary depending on
*the specific device.
*
-* - arm64 has a fixed 1G ZONE_DMA and ZONE_DMA32 for the rest of the
-*lower 4G.
+* - arm64 uses a single 4GB ZONE_DMA, except on the Raspberry Pi 4,
+*in which ZONE_DMA covers the first GB and ZONE_DMA32 the rest of
+*the lower 4GB.
*
* - powerpc only uses ZONE_DMA, the size, up to 2G, may vary
*depending on the specific device.
--
2.28.0
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Sat, 2020-10-10 at 12:36 +0200, Ard Biesheuvel wrote: > On Fri, 9 Oct 2020 at 19:10, Catalin Marinas wrote: > > On Fri, Oct 09, 2020 at 06:23:06PM +0200, Ard Biesheuvel wrote: > > > On Fri, 9 Oct 2020 at 17:24, Lorenzo Pieralisi > > > wrote: > > > > We can move this check to IORT code and call it from arm64 if it > > > > can be made to work. > > > > > > Finding the smallest value in the IORT, and assigning it to > > > zone_dma_bits if it is < 32 should be easy. But as I understand it, > > > having these separate DMA and DMA32 zones is what breaks kdump, no? So > > > how is this going to fix the underlying issue? > > > > If zone_dma_bits is 32, ZONE_DMA32 disappears into ZONE_DMA (GFP_DMA32 > > allocations fall back to ZONE_DMA). > > > > kdump wants DMA-able memory and, without a 30-bit ZONE_DMA, that would > > be the bottom 32-bit. With the introduction of ZONE_DMA, this suddenly > > became 1GB. We could change kdump to allocate ZONE_DMA32 but this one > > may also be small as it lost 1GB to ZONE_DMA. However, the kdump kernel > > would need to be rebuilt without ZONE_DMA since it won't have any. IIRC > > (it's been a while since I looked), the kdump allocation couldn't span > > multiple zones. > > > > In a separate thread, we try to fix kdump to use allocations above 4G as > > a fallback but this only fixes platforms with enough RAM (and maybe it's > > only those platforms that care about kdump). > > > > One thing that strikes me as odd is that we are applying the same > shifting logic to ZONE_DMA as we are applying to ZONE_DMA32, i.e., if > DRAM starts outside of the zone, it is shifted upwards. > > On a typical ARM box, this gives me > > [0.00] Zone ranges: > [0.00] DMA [mem 0x8000-0xbfff] > [0.00] DMA32[mem 0xc000-0x] > [0.00] Normal [mem 0x0001-0x000f] > > i.e., the 30-bit addressable range has bit 31 set, which is weird. Yes I agree it's weird, and IMO plain useless. I sent a series this summer to address this[1], which ultimately triggered the discussion we're having right now. Although with with your latest patch and the DT counterpart, we should be OK. It would be weird for a HW description to define DMA constraints that are impossible to reach on that system. > I wonder if it wouldn't be better (and less problematic in the general > case) to drop this logic for ZONE_DMA, and simply let it remain empty > unless there is really some memory there. From my experience, you can't have empty ZONE_DMA when enabled. Empty ZONE_DMA32 is OK though. Although I'm sure it's something that can be changed. Regards, Nicolas [1] https://lkml.org/lkml/2020/8/19/1022 signature.asc Description: This is a digitally signed message part
[PATCH v2 2/5] of/address: Introduce of_dma_lower_bus_limit()
The function provides the CPU physical address addressable by the most
constrained bus in the system. It might be useful in order to
dynamically set up memory zones during boot.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/of/address.c | 34 ++
include/linux/of.h | 7 +++
2 files changed, 41 insertions(+)
diff --git a/drivers/of/address.c b/drivers/of/address.c
index eb9ab4f1e80b..755e97b65096 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const
struct bus_dma_region **map)
}
#endif /* CONFIG_HAS_DMA */
+/**
+ * of_dma_safe_phys_limit - Get system wide DMA safe address space
+ *
+ * Gets the CPU physical address limit for safe DMA addressing system wide by
+ * searching for the most constraining dma-range. Otherwise it returns ~0ULL.
+ */
+u64 __init of_dma_safe_phys_limit(void)
+{
+ struct device_node *np = NULL;
+ struct of_range_parser parser;
+ const __be32 *ranges = NULL;
+ u64 phys_dma_limit = ~0ULL;
+ struct of_range range;
+ int len;
+
+ for_each_of_allnodes(np) {
+ dma_addr_t cpu_end = 0;
+
+ ranges = of_get_property(np, "dma-ranges", );
+ if (!ranges || !len)
+ continue;
+
+ of_dma_range_parser_init(, np);
+ for_each_of_range(, )
+ if (range.cpu_addr + range.size > cpu_end)
+ cpu_end = range.cpu_addr + range.size;
+
+ if (phys_dma_limit > cpu_end)
+ phys_dma_limit = cpu_end;
+ }
+
+ return phys_dma_limit;
+}
+
/**
* of_dma_is_coherent - Check if device is coherent
* @np:device node
diff --git a/include/linux/of.h b/include/linux/of.h
index 481ec0467285..958c64cffa92 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id,
const char *map_name, const char *map_mask_name,
struct device_node **target, u32 *id_out);
+u64 of_dma_safe_phys_limit(void);
+
#else /* CONFIG_OF */
static inline void of_core_init(void)
@@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id,
return -EINVAL;
}
+static inline u64 of_dma_safe_phys_limit(void)
+{
+ return ~0ULL;
+}
+
#define of_match_ptr(_ptr) NULL
#define of_match_node(_matches, _node) NULL
#endif /* CONFIG_OF */
--
2.28.0
[PATCH v2 0/5] arm64: Default to 32-bit wide ZONE_DMA
I realized this morning after reading Ard's patch fixing the same issue in ACPI that we can move the zone_dma_bits initialization later in the init process. This permits the use of OF to parse dma-ranges in the system. Something we though we couldn't do on previous iterations of this. The series sits on top of Ard's patch "arm64: mm: set ZONE_DMA size based on early IORT scan." --- Original cover letter Using two distinct DMA zones turned out to be problematic. Here's an attempt go back to a saner default. I tested this on both a RPi4 and QEMU. --- Changes since v1: - Parse dma-ranges instead of using machine compatible string Nicolas Saenz Julienne (5): arm64: mm: Move zone_dma_bits initialization into zone_sizes_init() of/address: Introduce of_dma_lower_bus_limit() dma-direct: Turn zone_dma_bits default value into a define arm64: mm: Dynamically resize zone_dma_bits based on system's constraints mm: Update DMA zones description arch/arm64/include/asm/processor.h | 1 + arch/arm64/mm/init.c | 12 --- drivers/of/address.c | 34 ++ include/linux/dma-direct.h | 3 +++ include/linux/mmzone.h | 5 +++-- include/linux/of.h | 7 ++ kernel/dma/direct.c| 2 +- 7 files changed, 53 insertions(+), 11 deletions(-) -- 2.28.0
[PATCH v2 3/5] dma-direct: Turn zone_dma_bits default value into a define
So as for architecture code to set their own default values when relevant. Signed-off-by: Nicolas Saenz Julienne --- Note: This is not really needed, but I think it nicer having architectures use this than setting zone_dma_bits in a random place in arch code. That said, I'll hapily edit it out if you don't agree. include/linux/dma-direct.h | 3 +++ kernel/dma/direct.c| 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index 18aade195884..e433d90cbacf 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -12,6 +12,9 @@ #include #include +#ifndef ZONE_DMA_BITS_DEFAULT +#define ZONE_DMA_BITS_DEFAULT 24 +#endif extern unsigned int zone_dma_bits; /* diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 06c111544f61..c0d97f536e93 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -20,7 +20,7 @@ * it for entirely different regions. In that case the arch code needs to * override the variable below for dma-direct to work properly. */ -unsigned int zone_dma_bits __ro_after_init = 24; +unsigned int zone_dma_bits __ro_after_init = ZONE_DMA_BITS_DEFAULT; static inline dma_addr_t phys_to_dma_direct(struct device *dev, phys_addr_t phys) -- 2.28.0
[PATCH 3/3] pwm: Add Raspberry Pi Firmware based PWM bus
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
---
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 ..1ccff6b1ae34
--- /dev/null
+++ b/drivers/pwm/pwm-raspberrypi.c
@@ -0,0 +1,216 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2020 Nicolas Saenz Julienne
+ */
+
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define RPI_PWM_MAX_DUTY 255
+#define RPI_PWM_PERIOD_NS 8 /* 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,
+ , 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,
+ , 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);
+ i
[PATCH 0/3] Raspberry Pi 4 PoE HAT fan support
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
[PATCH 1/3] dt-bindings: pwm: Add binding for RPi firmware PWM bus
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
---
.../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 ..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
+ */
+
+#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
[PATCH 2/3] DO NOT MERGE: ARM: dts: Add RPi's official PoE hat support
This is an example on how to enable the fan on top of RPi's official PoE
hat.
Signed-off-by: Nicolas Saenz Julienne
---
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
+#include
/ {
compatible = "raspberrypi,4-model-b", "brcm,bcm2711";
@@ -68,6 +69,54 @@ sd_vcc_reg: sd_vcc_reg {
enable-active-high;
gpio = < 6 GPIO_ACTIVE_HIGH>;
};
+
+ fan: pwm-fan {
+ compatible = "pwm-fan";
+ cooling-levels = <0 50 150 255>;
+ #cooling-cells = <2>;
+ pwms = < 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 = <5>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+
+ cpu_hot: cpu_hot@0 {
+ temperature = <55000>;
+ hysteresis = <2000>;
+ type = "active";
+ };
+ };
+
+ cooling-maps {
+ map0 {
+ trip = <>;
+ cooling-device = < 0 1>;
+ };
+
+ map1 {
+ trip = <>;
+ cooling-device = < 1 2>;
+ };
+
+ map2 {
+ trip = <_hot>;
+ cooling-device = < 2 3>;
+ };
+ };
+ };
+ };
};
{
@@ -103,6 +152,11 @@ reset: reset {
compatible = "raspberrypi,firmware-reset";
#reset-cells = <1>;
};
+
+ fwpwm: pwm {
+ compatible = "raspberrypi,firmware-pwm";
+ #pwm-cells = <1>;
+ };
};
{
--
2.28.0
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Fri, 2020-10-09 at 11:13 +0200, Ard Biesheuvel wrote: > On Fri, 9 Oct 2020 at 10:36, Nicolas Saenz Julienne > wrote: > > On Fri, 2020-10-09 at 09:37 +0200, Ard Biesheuvel wrote: > > > On Fri, 9 Oct 2020 at 09:11, Christoph Hellwig wrote: > > > > On Thu, Oct 08, 2020 at 12:05:25PM +0200, Nicolas Saenz Julienne wrote: > > > > > Sadly I just realised that the series is incomplete, we have RPi4 > > > > > users that > > > > > want to boot unsing ACPI, and this series would break things for > > > > > them. I'll > > > > > have a word with them to see what we can do for their use-case. > > > > > > > > Stupid question: why do these users insist on a totally unsuitable > > > > interface? And why would we as Linux developers care to support such > > > > a aims? > > > > > > The point is really whether we want to revert changes in Linux that > > > made both DT and ACPI boot work without quirks on RPi4. > > > > Well, and broke a big amount of devices that were otherwise fine. > > > > Yeah that was unfortunate. > > > > Having to check the RPi4 compatible string or OEM id in core init code is > > > awful, regardless of whether you boot via ACPI or via DT. > > > > > > The problem with this hardware is that it uses a DMA mask which is > > > narrower than 32, and the arm64 kernel is simply not set up to deal > > > with that at all. On DT, we have DMA ranges properties and the likes > > > to describe such limitations, on ACPI we have _DMA methods as well as > > > DMA range attributes in the IORT, both of which are now handled > > > correctly. So all the information is there, we just have to figure out > > > how to consume it early on. > > > > Is it worth the effort just for a single board? I don't know about ACPI but > > parsing dma-ranges that early at boot time is not trivial. My intuition > > tells > > me that it'd be even harder for ACPI, being a more complex data structure. > > > > Yes, it will be harder, especially for the _DMA methods. > > > > Interestingly, this limitation always existed in the SoC, but it > > > wasn't until they started shipping it with more than 1 GB of DRAM that > > > it became a problem. This means issues like this could resurface in > > > the future with existing SoCs when they get shipped with more memory, > > > and so I would prefer fixing this in a generic way. > > > > Actually what I proposed here is pretty generic. Specially from arm64's > > perspective. We call early_init_dt_scan(), which sets up zone_dma_bits > > based on > > whatever it finds in DT. Both those operations are architecture independent. > > arm64 arch code doesn't care about the logic involved in ascertaining > > zone_dma_bits. I get that the last step isn't generic. But it's all setup > > so as > > to make it as such whenever it's worth the effort. > > > > The problem is that, while we are providing a full description of the > SoC's capabilities, we short circuit this by inserting knowledge into > the code (that is shared between all DT architectures) that > "brcm,bcm2711" is special, and needs a DMA zone override. Yes I understand this and I sympathize with it, not the most beautiful thing out there :). But that's only half the issue, as I said, implementing this early at boot time is a tangible amount of work and a burden to maintain just for one board. So this is the compromise we discussed with the DT maintainer (RobH). The series sets things up so as to be able to implement the right thing transparently to arm64's architecture when deemed worth the effort. Ultimately, if you're worried about inserting knowledge into the code, aren't we doing that, in a more extreme way, when imposing an extra unwarranted zone to the whole arm64 ecosystem? Note that I'm more that happy to work on alternative solutions, but let's first settle on what would be acceptable to everyone. > I think for ACPI boot, we might be able to work around this by cold > plugging the memory above 1 GB, but I have to double check whether it > won't get pulled into ZONE_DMA32 anyway (unless anyone can answer that > for me here from the top of their head) Don't know much about what ACPI memory cold plugging involves, but we'll still need a proper ZONE_DMA32 (i.e. spanning the whole 32-bit address space) for RPi4. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
Hi Jeremy.
On Thu, 2020-10-08 at 22:59 -0500, Jeremy Linton wrote:
> On 10/8/20 2:43 PM, Ard Biesheuvel wrote:
> > (+ Lorenzo)
> >
> > On Thu, 8 Oct 2020 at 12:14, Catalin Marinas
> > wrote:
> > > On Thu, Oct 08, 2020 at 12:05:25PM +0200, Nicolas Saenz Julienne wrote:
> > > > On Fri, 2020-10-02 at 12:55 +0100, Catalin Marinas wrote:
> > > > > On Thu, Oct 01, 2020 at 07:31:19PM +0200, Nicolas Saenz Julienne
> > > > > wrote:
> > > > > > On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> > > > > > > On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > > > > > > > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz
> > > > > > > > Julienne wrote:
> > > > > > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > > > > > index 4602e467ca8b..cd0d115ef329 100644
> > > > > > > > > --- a/drivers/of/fdt.c
> > > > > > > > > +++ b/drivers/of/fdt.c
> > > > > > > > > @@ -25,6 +25,7 @@
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > > #include
> > > > > > > > > +#include /* for zone_dma_bits */
> > > > > > > > >
> > > > > > > > > #include /* for COMMAND_LINE_SIZE */
> > > > > > > > > #include
> > > > > > > > > @@ -1198,6 +1199,14 @@ void __init
> > > > > > > > > early_init_dt_scan_nodes(void)
> > > > > > > > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > > > > > > > +{
> > > > > > > > > + unsigned long dt_root = of_get_flat_dt_root();
> > > > > > > > > +
> > > > > > > > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > > > > > > > + zone_dma_bits = 30;
> > > > > > > > > +}
> > > > > > > >
> > > > > > > > I think we could keep this entirely in the arm64
> > > > > > > > setup_machine_fdt() and
> > > > > > > > not pollute the core code with RPi4-specific code.
> > > > > > >
> > > > > > > Actually, even better, could we not move the check to
> > > > > > > arm64_memblock_init() when we initialise zone_dma_bits?
> > > > > >
> > > > > > I did it this way as I vaguely remembered Rob saying he wanted to
> > > > > > centralise
> > > > > > all early boot fdt code in one place. But I'll be happy to move it
> > > > > > there.
> > > > >
> > > > > I can see Rob replied and I'm fine if that's his preference. However,
> > > > > what I don't particularly like is that in the arm64 code, if
> > > > > zone_dma_bits == 24, we set it to 32 assuming that it wasn't touched
> > > > > by
> > > > > the early_init_dt_update_zone_dma_bits(). What if at some point we'll
> > > > > get a platform that actually needs 24 here (I truly hope not, but just
> > > > > the principle of relying on magic values)?
> > > > >
> > > > > So rather than guessing, I'd prefer if the arch code can override
> > > > > ZONE_DMA_BITS_DEFAULT. Then, in arm64, we'll just set it to 32 and no
> > > > > need to explicitly touch the zone_dma_bits variable.
> > > >
> > > > Yes, sonds like the way to go. TBH I wasn't happy with that solution
> > > > either,
> > > > but couldn't think of a nicer alternative.
> > > >
> > > > Sadly I just realised that the series is incomplete, we have RPi4 users
> > > > that
> > > > want to boot unsing ACPI, and this series would break things for them.
> > > > I'll
> > > > have a word with them to see what we can do for their use-case.
> > >
> > > Is there a way to get some SoC information from ACPI?
> > >
> >
> > This is unfortunate. We used ACPI _DMA methods as they were designed
> > to communica
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Fri, 2020-10-09 at 09:37 +0200, Ard Biesheuvel wrote: > On Fri, 9 Oct 2020 at 09:11, Christoph Hellwig wrote: > > On Thu, Oct 08, 2020 at 12:05:25PM +0200, Nicolas Saenz Julienne wrote: > > > Sadly I just realised that the series is incomplete, we have RPi4 users > > > that > > > want to boot unsing ACPI, and this series would break things for them. > > > I'll > > > have a word with them to see what we can do for their use-case. > > > > Stupid question: why do these users insist on a totally unsuitable > > interface? And why would we as Linux developers care to support such > > a aims? > > The point is really whether we want to revert changes in Linux that > made both DT and ACPI boot work without quirks on RPi4. Well, and broke a big amount of devices that were otherwise fine. > Having to check the RPi4 compatible string or OEM id in core init code is > awful, regardless of whether you boot via ACPI or via DT. > > The problem with this hardware is that it uses a DMA mask which is > narrower than 32, and the arm64 kernel is simply not set up to deal > with that at all. On DT, we have DMA ranges properties and the likes > to describe such limitations, on ACPI we have _DMA methods as well as > DMA range attributes in the IORT, both of which are now handled > correctly. So all the information is there, we just have to figure out > how to consume it early on. Is it worth the effort just for a single board? I don't know about ACPI but parsing dma-ranges that early at boot time is not trivial. My intuition tells me that it'd be even harder for ACPI, being a more complex data structure. > Interestingly, this limitation always existed in the SoC, but it > wasn't until they started shipping it with more than 1 GB of DRAM that > it became a problem. This means issues like this could resurface in > the future with existing SoCs when they get shipped with more memory, > and so I would prefer fixing this in a generic way. Actually what I proposed here is pretty generic. Specially from arm64's perspective. We call early_init_dt_scan(), which sets up zone_dma_bits based on whatever it finds in DT. Both those operations are architecture independent. arm64 arch code doesn't care about the logic involved in ascertaining zone_dma_bits. I get that the last step isn't generic. But it's all setup so as to make it as such whenever it's worth the effort. > Also, I assume papering over the issue like this does not fix the > kdump issue fundamentally, it just works around it, and so we might > run into this again in the future. Any ideas? The way I understand it the kdump issue is just a shortcoming of the memory zones design. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
Hi Catalin, sorry for the late reply.
On Fri, 2020-10-02 at 12:55 +0100, Catalin Marinas wrote:
> On Thu, Oct 01, 2020 at 07:31:19PM +0200, Nicolas Saenz Julienne wrote:
> > On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> > > On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > > > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > > index 4602e467ca8b..cd0d115ef329 100644
> > > > > --- a/drivers/of/fdt.c
> > > > > +++ b/drivers/of/fdt.c
> > > > > @@ -25,6 +25,7 @@
> > > > > #include
> > > > > #include
> > > > > #include
> > > > > +#include /* for zone_dma_bits */
> > > > >
> > > > > #include /* for COMMAND_LINE_SIZE */
> > > > > #include
> > > > > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > > > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > > > }
> > > > >
> > > > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > > > +{
> > > > > + unsigned long dt_root = of_get_flat_dt_root();
> > > > > +
> > > > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > > > + zone_dma_bits = 30;
> > > > > +}
> > > >
> > > > I think we could keep this entirely in the arm64 setup_machine_fdt() and
> > > > not pollute the core code with RPi4-specific code.
> > >
> > > Actually, even better, could we not move the check to
> > > arm64_memblock_init() when we initialise zone_dma_bits?
> >
> > I did it this way as I vaguely remembered Rob saying he wanted to centralise
> > all early boot fdt code in one place. But I'll be happy to move it there.
>
> I can see Rob replied and I'm fine if that's his preference. However,
> what I don't particularly like is that in the arm64 code, if
> zone_dma_bits == 24, we set it to 32 assuming that it wasn't touched by
> the early_init_dt_update_zone_dma_bits(). What if at some point we'll
> get a platform that actually needs 24 here (I truly hope not, but just
> the principle of relying on magic values)?
>
> So rather than guessing, I'd prefer if the arch code can override
> ZONE_DMA_BITS_DEFAULT. Then, in arm64, we'll just set it to 32 and no
> need to explicitly touch the zone_dma_bits variable.
Yes, sonds like the way to go. TBH I wasn't happy with that solution either,
but couldn't think of a nicer alternative.
Sadly I just realised that the series is incomplete, we have RPi4 users that
want to boot unsing ACPI, and this series would break things for them. I'll
have a word with them to see what we can do for their use-case.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v1] PCI: brcmstb: fix error return paths in probe()
On Thu, 2020-10-01 at 11:18 -0400, Jim Quinlan wrote:
> Fixes two cases where we were returning without calling
> clk_disable_unprepare(). Although there is a common place to go on probe()
> errors (the 'fail' label), one can only jump there after executing
> brcm_pcie_setup(), so we have to add clk_disable_unprepare() calls to the
> two error paths.
>
> Fixes: b98f52bc6495 ("PCI: brcmstb: Add control of rescal reset")
> Reported-by: kernel test robot
> Reported-by: Dan Carpenter
> Signed-off-by: Jim Quinlan
> ---
Reviewed-by: Nicolas Saenz Julienne
Thanks!
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
Hi Dave, sorry for the late reply. On Tue, 2020-10-06 at 18:14 +0100, Dave Stevenson wrote: > Hi Maxime > > On Tue, 6 Oct 2020 at 16:26, Maxime Ripard wrote: > > Hi Dave, > > > > On Fri, Oct 02, 2020 at 04:57:05PM +0100, Dave Stevenson wrote: > > > Hi Maxime > > > > > > On Fri, 2 Oct 2020 at 16:19, Maxime Ripard wrote: > > > > Hi Tim, > > > > > > > > On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote: > > > > > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC > > > > > VCO to support a core-frequency of 550 MHz which is the minimum > > > > > frequency required by the HVS at 4Kp60. The side effect is that if the > > > > > display clock requirements are lower than 4Kp60 then you will see > > > > > different core frequencies selected by DVFS. > > > > > > > > > > If enable_uart=1 and the mini-uart is selected (default unless > > > > > bluetooth is disabled) then the firmware will pin the core-frequency > > > > > to either core_freq max (500 or 550). Although, I think there is a way > > > > > of pinning it to a lower fixed frequency. > > > > > > > > > > The table in overclocking.md defines options for setting the maximum > > > > > core frequency but unless core_freq_min is specified DVFS will > > > > > automatically pick the lowest idle frequency required by the display > > > > > resolution. > > > > > > > > I'm wondering if there's some way to detect this from Linux? I guess it > > > > would be nice to be able to at least detect a broken config to warn / > > > > prevent an user that their situation is not going to be reliable / work > > > > really well (like if they have a 4k display without hdmi_enable_4kp60 > > > > set, or the issue we're discussing here) > > > > > > The main filter in the firmware is the parameter > > > hdmi_pixel_freq_limit. That can either be set manually from > > > config.txt, or defaults appropriately based on hdmi_enable_4kp60. > > > Under firmware_kms [1] I read back those values to use as a filter > > > within crtc_mode_valid[2]. > > > I can't think of a nice way of exposing that without the vc4 driver > > > gaining a DT link to the firmware, and that starts to get ugly. > > > > I had in mind something like if the clock driver can infer that somehow > > through some the boundaries reported by the firmware maybe? IIRC, > > hdmi_enable_4kp60 will already change the max frequency reported to > > 550MHz instead of 500MHz > > Yes, that's plausible, but I don't know enough about the clock > infrastructure for advertising limits to know what works there. > Tell me what you need from the mailbox service and I'll see what I can do. > > We do already have RPI_FIRMWARE_GET_MAX_CLOCK_RATE and > RPI_FIRMWARE_GET_MIN_CLOCK_RATE. It'd take a few minutes of staring at > the code (or a quick test) to confirm if they definitely are changed > for CORE clock by hdmi_enable_4kp60 - I think it does. Tim commented earlier that you meant to pin CORE frequency when enable_uart=1. In practice it doesn't seem to be the case, but it would solve the UART problem for most use cases. Would that be feasible? If we have to change the CORE frequency during runtime we could, while we search for a proper solution, print a warning that we're about to break the low speed peripherals. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > Hi Nicolas,
> >
> > Thanks for putting this together.
> >
> > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > index 4602e467ca8b..cd0d115ef329 100644
> > > --- a/drivers/of/fdt.c
> > > +++ b/drivers/of/fdt.c
> > > @@ -25,6 +25,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include /* for zone_dma_bits */
> > >
> > > #include /* for COMMAND_LINE_SIZE */
> > > #include
> > > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > }
> > >
> > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > +{
> > > + unsigned long dt_root = of_get_flat_dt_root();
> > > +
> > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > + zone_dma_bits = 30;
> > > +}
> >
> > I think we could keep this entirely in the arm64 setup_machine_fdt() and
> > not pollute the core code with RPi4-specific code.
>
> Actually, even better, could we not move the check to
> arm64_memblock_init() when we initialise zone_dma_bits?
I did it this way as I vaguely remembered Rob saying he wanted to centralise
all early boot fdt code in one place. But I'll be happy to move it there.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
Hi Tim, thanks for the info! On Thu, 2020-10-01 at 11:15 +0100, Tim Gover wrote: > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC > VCO to support a core-frequency of 550 MHz which is the minimum > frequency required by the HVS at 4Kp60. The side effect is that if the > display clock requirements are lower than 4Kp60 then you will see > different core frequencies selected by DVFS. > > If enable_uart=1 and the mini-uart is selected (default unless What is the actual test made to check if mini-uart is selected? I can't get firmware to trigger this behaviour with 64-bit upstream kernel/dts. Note that I see the core clk setup at 200MHz just before having VC4 set it to 500MHz. The only thing I've got on my config.txt is: enable_uart=1 arm_64bit=1 Maybe we're missing some kind of DT alias upstream? Regards, Nicolas > bluetooth is disabled) then the firmware will pin the core-frequency > to either core_freq max (500 or 550). Although, I think there is a way > of pinning it to a lower fixed frequency. > > The table in overclocking.md defines options for setting the maximum > core frequency but unless core_freq_min is specified DVFS will > automatically pick the lowest idle frequency required by the display > resolution. signature.asc Description: This is a digitally signed message part
[PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
arm64 wants to be able to set ZONE_DMA's size depending on the specific
platform its being run on. Ideally this could be achieved in a smart way
by parsing all dma-ranges and calculating the smaller DMA constraint in
the system. Easier said than done. We compromised on a simpler solution
as the only platform interested in using this is the Raspberry Pi 4.
So update zone_dma_bits if the machine's compatible string matches
Raspberry Pi 4's, otherwise let arm64's mm code deal with it.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/of/fdt.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4602e467ca8b..cd0d115ef329 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include /* for zone_dma_bits */
#include /* for COMMAND_LINE_SIZE */
#include
@@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
of_scan_flat_dt(early_init_dt_scan_memory, NULL);
}
+void __init early_init_dt_update_zone_dma_bits(void)
+{
+ unsigned long dt_root = of_get_flat_dt_root();
+
+ if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
+ zone_dma_bits = 30;
+}
+
bool __init early_init_dt_scan(void *params)
{
bool status;
@@ -1207,6 +1216,7 @@ bool __init early_init_dt_scan(void *params)
return false;
early_init_dt_scan_nodes();
+ early_init_dt_update_zone_dma_bits();
return true;
}
--
2.28.0
[PATCH 3/4] arm64: Default to 32-bit ZONE_DMA
The Raspberry Pi 4 needs two DMA zones as some of its devices can only
DMA into the 30-bit physical address space. We solved that by creating
an extra ZONE_DMA covering the 30-bit. It turns out that creating extra
zones unnecessarily broke Kdump on large systems. So default to a single
32-bit wide ZONE_DMA and only define both zones if running on RPi4.
Signed-off-by: Nicolas Saenz Julienne
---
arch/arm64/mm/init.c | 12
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index e1a69a618832..3c3f462466eb 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -43,8 +43,6 @@
#include
#include
-#define ARM64_ZONE_DMA_BITS30
-
/*
* We need to be able to catch inadvertent references to memstart_addr
* that occur (potentially in generic code) before arm64_memblock_init()
@@ -388,8 +386,14 @@ void __init arm64_memblock_init(void)
early_init_fdt_scan_reserved_mem();
if (IS_ENABLED(CONFIG_ZONE_DMA)) {
- zone_dma_bits = ARM64_ZONE_DMA_BITS;
- arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
+ /*
+* early_init_dt_scan() might alter zone_dma_bits based on the
+* device's DT. Otherwise, have it cover the 32-bit address
+* space.
+*/
+ if (zone_dma_bits == ZONE_DMA_BITS_DEFAULT)
+ zone_dma_bits = 32;
+ arm64_dma_phys_limit = max_zone_phys(zone_dma_bits);
}
if (IS_ENABLED(CONFIG_ZONE_DMA32))
--
2.28.0
[PATCH 4/4] mm: Update DMA zones description
The default behavior for arm64 changed, so reflect that.
Signed-off-by: Nicolas Saenz Julienne
---
include/linux/mmzone.h | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index fb3bf696c05e..d28ce77ccc2a 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -363,8 +363,9 @@ enum zone_type {
* - arm only uses ZONE_DMA, the size, up to 4G, may vary depending on
*the specific device.
*
-* - arm64 has a fixed 1G ZONE_DMA and ZONE_DMA32 for the rest of the
-*lower 4G.
+* - arm64 uses a single 4GB ZONE_DMA, except on the Raspberry Pi 4,
+*in which ZONE_DMA covers the first GB and ZONE_DMA32 the rest of
+*the lower 4GB.
*
* - powerpc only uses ZONE_DMA, the size, up to 2G, may vary
*depending on the specific device.
--
2.28.0
[PATCH 2/4] dma-direct: Turn zone_dma_bits default value into a define
Other code might need to reference it. Signed-off-by: Nicolas Saenz Julienne --- include/linux/dma-direct.h | 1 + kernel/dma/direct.c| 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h index 38ed3b55034d..ef19ff505d09 100644 --- a/include/linux/dma-direct.h +++ b/include/linux/dma-direct.h @@ -12,6 +12,7 @@ #include #include +#define ZONE_DMA_BITS_DEFAULT 24 extern unsigned int zone_dma_bits; /* diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c index 121a9c1969dd..4b3cfa02532b 100644 --- a/kernel/dma/direct.c +++ b/kernel/dma/direct.c @@ -20,7 +20,7 @@ * it for entirely different regions. In that case the arch code needs to * override the variable below for dma-direct to work properly. */ -unsigned int zone_dma_bits __ro_after_init = 24; +unsigned int zone_dma_bits __ro_after_init = ZONE_DMA_BITS_DEFAULT; static inline dma_addr_t phys_to_dma_direct(struct device *dev, phys_addr_t phys) -- 2.28.0
[PATCH 0/4] arm64: Default to 32-bit wide ZONE_DMA
Using two distinct DMA zones turned out to be problematic. Here's an attempt go back to a saner default. I tested this on both a RPi4 and QEMU. --- Nicolas Saenz Julienne (4): of/fdt: Update zone_dma_bits when running in bcm2711 dma-direct: Turn zone_dma_bits default value into a define arm64: Default to 32-bit ZONE_DMA mm: Update DMA zones description with arm64 newer behavior arch/arm64/mm/init.c | 12 drivers/of/fdt.c | 10 ++ include/linux/dma-direct.h | 1 + include/linux/mmzone.h | 5 +++-- kernel/dma/direct.c| 2 +- 5 files changed, 23 insertions(+), 7 deletions(-) -- 2.28.0
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
On Wed, 2020-09-30 at 09:38 -0700, Nathan Chancellor wrote: > On Wed, Sep 30, 2020 at 04:07:58PM +0200, Maxime Ripard wrote: > > Hi Nathan, > > > > On Tue, Sep 29, 2020 at 03:15:26PM -0700, Nathan Chancellor wrote: > > > On Thu, Sep 03, 2020 at 10:01:52AM +0200, Maxime Ripard wrote: > > > > Now that all the drivers have been adjusted for it, let's bring in the > > > > necessary device tree changes. > > > > > > > > The VEC and PV3 are left out for now, since it will require a more > > > > specific > > > > clock setup. > > > > > > > > Reviewed-by: Dave Stevenson > > > > Tested-by: Chanwoo Choi > > > > Tested-by: Hoegeun Kwon > > > > Tested-by: Stefan Wahren > > > > Signed-off-by: Maxime Ripard > > > > > > Apologies if this has already been reported or have a solution but this > > > patch (and presumably series) breaks output to the serial console after > > > a certain point during init. On Raspbian, I see systemd startup messages > > > then the output just turns into complete garbage. It looks like this > > > patch is merged first in linux-next, which is why my bisect fell on the > > > DRM merge. I am happy to provide whatever information could be helpful > > > for debugging this. I am on the latest version of the firmware > > > (currently 26620cc9a63c6cb9965374d509479b4ee2c30241). > > > > Unfortunately, the miniUART is in the same clock tree than the core > > clock and will thus have those kind of issues when the core clock is > > changed (which is also something that one should expect when using the > > DRM or other drivers). > > > > The only real workaround there would be to switch to one of the PL011 > > UARTs. I guess we can also somehow make the UART react to the core clock > > frequency changes, but that's going to require some effort > > > > Maxime > > Ack, thank you for the reply! There does not really seem to be a whole > ton of documentation around using one of the other PL011 UARTs so for > now, I will just revert this commit locally. Nathan, a less intrusive solution would be to add 'core_freq_min=500' into your config.txt. Funnily enough core_freq=500 doesn't seem to work in that regard. It might be related with what Maxime is commenting. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH 06/42] mfd: bcm2835: use PLATFORM_DEVID_NONE
On Mon, 2020-09-21 at 22:49 +0200, Krzysztof Kozlowski wrote: > Use PLATFORM_DEVID_NONE define instead of "-1" value because: > - it brings some meaning, > - it might point attention why auto device ID was not used. > > Signed-off-by: Krzysztof Kozlowski > --- Reviewed-by: Nicolas Saenz Julienne > drivers/mfd/bcm2835-pm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/mfd/bcm2835-pm.c b/drivers/mfd/bcm2835-pm.c > index 42fe67f1538e..a76014512bde 100644 > --- a/drivers/mfd/bcm2835-pm.c > +++ b/drivers/mfd/bcm2835-pm.c > @@ -44,7 +44,7 @@ static int bcm2835_pm_probe(struct platform_device *pdev) > if (IS_ERR(pm->base)) > return PTR_ERR(pm->base); > > - ret = devm_mfd_add_devices(dev, -1, > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > bcm2835_pm_devs, ARRAY_SIZE(bcm2835_pm_devs), > NULL, 0, NULL); > if (ret) > @@ -60,7 +60,7 @@ static int bcm2835_pm_probe(struct platform_device *pdev) > if (IS_ERR(pm->asb)) > return PTR_ERR(pm->asb); > > - ret = devm_mfd_add_devices(dev, -1, > + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, > bcm2835_power_devs, > ARRAY_SIZE(bcm2835_power_devs), > NULL, 0, NULL); signature.asc Description: This is a digitally signed message part
[GIT PULL 1/1] bcm2835-dt-next-2020-09-08
Hi Florian, The following changes since commit 9123e3a74ec7b934a4a099e98af6a61c2f80bbf5: Linux 5.9-rc1 (2020-08-16 13:04:57 -0700) are available in the Git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/nsaenz/linux-rpi.git tags/bcm2835-dt-next-2020-09-08 for you to fetch changes up to 4564363351e2680e55edc23c7953aebd2acb4ab7: ARM: dts: bcm2711: Enable the display pipeline (2020-09-08 18:28:23 +0200) Maxime Ripard enables vc4 on BCM2711 (RPi4), which among other things adds HDMI functionality (no 4K yet). Maxime Ripard (1): ARM: dts: bcm2711: Enable the display pipeline arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 48 + arch/arm/boot/dts/bcm2711.dtsi| 122 +- 2 files changed, 169 insertions(+), 1 deletion(-)
Re: [PATCH v2 0/4] drm/vc4: Support HDMI QHD or higher output
On Tue, 2020-09-01 at 13:07 +0900, Hoegeun Kwon wrote: > Hi everyone, > > There is a problem that the output does not work at a resolution > exceeding FHD. To solve this, we need to adjust the bvb clock at a > resolution exceeding FHD. > > Rebased on top of next-20200708 and [1]. > > [1] : [PATCH v4 00/78] drm/vc4: Support BCM2711 Display Pipeline (Maxime's > patchset) > > Changes from v1: > - Added dt-bindings documents > - Change patch order, first fix driver and then device tree > > Hoegeun Kwon (4): > clk: bcm: rpi: Add register to control pixel bvb clk > drm/vc4: hdmi: Add pixel bvb clock control > dt-bindings: display: vc4: hdmi: Add bvb clock-names property > ARM: dts: bcm2711: Add bvb clock for hdmi-pixel > > .../bindings/display/brcm,bcm2711-hdmi.yaml | 12 ++--- > arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 6 +++-- > drivers/clk/bcm/clk-raspberrypi.c | 1 + > drivers/gpu/drm/vc4/vc4_hdmi.c| 25 +++ > drivers/gpu/drm/vc4/vc4_hdmi.h| 1 + > 5 files changed, 39 insertions(+), 6 deletions(-) Small note to anyone reviewing this, patches 3 & 4 where squashed into this series: https://lkml.org/lkml/2020/9/3/219 Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
On Thu, 2020-09-03 at 10:01 +0200, Maxime Ripard wrote: > Now that all the drivers have been adjusted for it, let's bring in the > necessary device tree changes. > > The VEC and PV3 are left out for now, since it will require a more specific > clock setup. > > Reviewed-by: Dave Stevenson > Tested-by: Chanwoo Choi > Tested-by: Hoegeun Kwon > Tested-by: Stefan Wahren > Signed-off-by: Maxime Ripard > --- Applied for-next. Thanks! Nicolas signature.asc Description: This is a digitally signed message part
Re: [RFC] arm64: mm: Do not use both DMA zones when 30-bit address space unavailable
Hi Catalin, thanks for taking the time.
On Tue, 2020-09-08 at 12:14 +0100, Catalin Marinas wrote:
> > Also note the usage of 'zone_dma_bits' in the DMA code, which assumes that
> > ZONE_DMA's physical address space is always smaller than (1 <<
> > zone_dma_bits) -
> > 1.
>
> I think part of those uses are broken. dma_direct_supported() does the
> right thing and uses the DMA address instead of the physical one. Here
> __phys_to_dma() subtracts the dma_pfn_offset, which in my above example
> would be (0b10 << (30 - PAGE_SHIFT)).
>
> dma_direct_optimal_gfp_mask(), OTOH, seems to start ok with a
> __dma_to_phys() on the dma_limit but it ends up comparing the physical
> address with the DMA mask. This gives the wrong result on arm64
> platforms where RAM starts above 4GB and still expect a ZONE_DMA32. It
> should compare *phys_limit with __dma_to_phys(DMA_BIT_MASK(...)). I
> guess it ends up bouncing via swiotlb more often.
I'll look into this.
> We assumed such offsets on arm64 since commit d50314a6b070 ("arm64:
> Create non-empty ZONE_DMA when DRAM starts above 4GB").
>
> > > An alternative (and I think we had a patch at some point) is to make it
> > > generic and parse the dma-range in the DT to identify the minimum mask
> > > and set ZONE_DMA accordingly. But this doesn't solve ACPI, so if Linux
> > > can boot with ACPI on RPi4 it would still be broken.
> >
> > ACPI is being worked on by, among others, Jeremy Linton (one of your
> > colleagues
> > I believe).
> >
> > We could always use sane defaults for ACPI and be smarter with DT. Yet,
> > implementing this entails translating nested dma-ranges with the only help
> > of
> > libfdt, which isn't trivial (see devices/of/address.c). IIRC RobH said that
> > it
> > wasn't worth the effort just for a board.
>
> That would have been the ideal, more generic solution. But I agree that
> it's not worth the effort if the only SoC affected is RPi4.
>
> To summarise, I'd like ZONE_DMA to overlap with ZONE_DMA32 (i.e. expand
> zone_dma_bits to 32 and drop ZONE_DMA32) for all SoCs other than RPi4.
> The solutions so far:
>
> 1. Assume that, if RAM starts at 0, we need a zone_dma_bits == 30. This
>also assumes that it's only RPi4 in this category or that any such
>future SoC has a need for 30-bit DMA.
>
> 2. Adjust zone_dma_bits at boot-time only if the SoC is RPi4.
>
> 3. Use the more complex dma-ranges approach to calculate the correct
>zone_dma_bits as a minimum of all dma masks in the DT.
>
> We can discount (3) as not worth the effort. I'd go with (1) (this
> patch) if we can guarantee that no current or future SoC has RAM
> starting at 0 while not needing 30-bit DMA masks. If not, we can go with
> (2) unless others have a better suggestion.
After a quick check at the devices we have for testing at suse it's clear that
(1) is impossible. So I'll push for solution (2).
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2 1/4] clk: bcm: rpi: Add register to control pixel bvb clk
On Tue, 2020-09-01 at 13:07 +0900, Hoegeun Kwon wrote: > To use QHD or higher, we need to modify the pixel_bvb_clk value. So > add register to control this clock. > > Signed-off-by: Hoegeun Kwon > --- > drivers/clk/bcm/clk-raspberrypi.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c > b/drivers/clk/bcm/clk-raspberrypi.c > index 5cc82954e1ce..f89b9cfc4309 100644 > --- a/drivers/clk/bcm/clk-raspberrypi.c > +++ b/drivers/clk/bcm/clk-raspberrypi.c > @@ -271,6 +271,7 @@ static int raspberrypi_discover_clocks(struct > raspberrypi_clk *rpi, > case RPI_FIRMWARE_CORE_CLK_ID: > case RPI_FIRMWARE_M2MC_CLK_ID: > case RPI_FIRMWARE_V3D_CLK_ID: > + case RPI_FIRMWARE_PIXEL_BVB_CLK_ID: > hw = raspberrypi_clk_register(rpi, clks->parent, > clks->id); > if (IS_ERR(hw)) Acked-by: Nicolas Saenz Julienne Tested-by: Nicolas Saenz Julienne Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [PATCH v5 00/80] drm/vc4: Support BCM2711 Display Pipeline
Hi Maxime, On Mon, 2020-09-07 at 18:22 +0200, Maxime Ripard wrote: > Hi, > > On Thu, Sep 03, 2020 at 10:00:32AM +0200, Maxime Ripard wrote: > > Hi everyone, > > > > Here's a (pretty long) series to introduce support in the VC4 DRM driver > > for the display pipeline found in the BCM2711 (and thus the RaspberryPi 4). > > > > The main differences are that there's two HDMI controllers and that there's > > more pixelvalve now. Those pixelvalve come with a mux in the HVS that still > > have only 3 FIFOs. Both of those differences are breaking a bunch of > > expectations in the driver, so we first need a good bunch of cleanup and > > reworks to introduce support for the new controllers. > > > > Similarly, the HDMI controller has all its registers shuffled and split in > > multiple controllers now, so we need a bunch of changes to support this as > > well. > > > > Only the HDMI support is enabled for now (even though the DPI and DSI > > outputs have been tested too). > > I've applied the patches 1-79 to drm-misc. I guess the final DT patch > should go through the arm-soc tree? I'll take care of it tomorrow. Regards, Nicolas signature.asc Description: This is a digitally signed message part
Re: [RFC] arm64: mm: Do not use both DMA zones when 30-bit address space unavailable
Hi Catalin, sorry for the late reply, I was on vacation.
On Fri, 2020-08-28 at 18:43 +0100, Catalin Marinas wrote:
> Hi Nicolas,
>
> On Wed, Aug 19, 2020 at 08:24:33PM +0200, Nicolas Saenz Julienne wrote:
> > There is no benefit in splitting the 32-bit address space into two
> > distinct DMA zones when the 30-bit address space isn't even available on
> > a device. If that is the case, default to one big ZONE_DMA spanning the
> > whole 32-bit address space.
> >
> > This will help reduce some of the issues we've seen with big crash
> > kernel allocations.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> > ---
> >
> > Whith this patch, on a 8GB RPi4 the setup looks like this:
> >
> > DMA [mem 0x-0x3fff]
> > DMA32[mem 0x4000-0x]
> > Normal [mem 0x0001-0x0001]
> >
> > And stock 8GB virtme/qemu:
> >
> > DMA [mem 0x4000-0x]
> > DMA32empty
> > Normal [mem 0x0001-0x00023fff]
> >
> > arch/arm64/mm/init.c | 29 +
> > 1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> > index b6881d61b818..857a62611d7a 100644
> > --- a/arch/arm64/mm/init.c
> > +++ b/arch/arm64/mm/init.c
> > @@ -183,13 +183,20 @@ static void __init reserve_elfcorehdr(void)
> >
> > /*
> > * Return the maximum physical address for a zone with a given address size
> > - * limit. It currently assumes that for memory starting above 4G, 32-bit
> > - * devices will use a DMA offset.
> > + * limit or zero if memory starts from an address higher than the zone
> > targeted.
> > + * It currently assumes that for memory starting above 4G, 32-bit devices
> > will
> > + * use a DMA offset.
> > */
> > static phys_addr_t __init max_zone_phys(unsigned int zone_bits)
> > {
> > - phys_addr_t offset = memblock_start_of_DRAM() & GENMASK_ULL(63,
> > zone_bits);
> > - return min(offset + (1ULL << zone_bits), memblock_end_of_DRAM());
> > + phys_addr_t base = memblock_start_of_DRAM();
> > + phys_addr_t offset = base & GENMASK_ULL(63, 32);
> > + s64 zone_size = (1ULL << zone_bits) - (base & DMA_BIT_MASK(32));
> > +
> > + if (zone_size <= 0)
> > + return 0;
> > +
> > + return min(base + zone_size + offset, memblock_end_of_DRAM());
> > }
>
> OK, so we can still get some ZONE_DMA if DRAM starts in the first GB.
>
> I don't think it entirely solves the problem.
Agree. Didn't mean to imply it.
> It just happens that the
> other affected SoCs don't have memory in the first GB. With this patch,
> we go by the assumption that ZONE_DMA/DMA32 split is only needed if
> there is memory in the low 1GB and such <32-bit devices don't have a DMA
> offset.
The way I understand it is: "we may have 30 bit DMA limited devices, the rest
can deal with 32 bit."
On top of that, I believe it makes little sense to use an offset in the
physical address space below the 32bit mark. You'd be limiting the amount of
memory available to the system. So, if you're going support DMA limited devices
on your otherwise RAM hungry SoC, you'll have to have that physical address
space directly available, or at least part of it.
All in all, I believe that assuming no 30 bit DMA limited devices exist in the
system if the physical addresses don't exist is a fairly safe.
Also note the usage of 'zone_dma_bits' in the DMA code, which assumes that
ZONE_DMA's physical address space is always smaller than (1 << zone_dma_bits) -
1.
> Adding Rob H (it's easier to ask him than grep'ing the DT files ;)), we
> may be ok with this assumption on current SoCs.
From what I've personally grep'd there is no new devices with odd ranges in
sight.
> An alternative (and I think we had a patch at some point) is to make it
> generic and parse the dma-range in the DT to identify the minimum mask
> and set ZONE_DMA accordingly. But this doesn't solve ACPI, so if Linux
> can boot with ACPI on RPi4 it would still be broken.
ACPI is being worked on by, among others, Jeremy Linton (one of your colleagues
I believe).
We could always use sane defaults for ACPI and be smarter with DT. Yet,
implementing this entails translating nested dma-ranges with the only help of
libfdt, which isn't trivial (see devices/of/address.c). IIRC RobH said that it
wasn't worth the effort just for a board.
Regards,
Nicolas
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2 28/47] staging: vchi: Get rid of vchiq_shim's message callback
Hi Jacopo, sorry if I'm a little late with my replies but I'm on vacation. I'll be back Sept 7th, but wanted to reply since I don't want to stop your work. On Fri, 2020-08-28 at 16:31 +0200, Jacopo Mondi wrote: > Hi Nicolas, > >I'm working on a v2 of the bcm2835-isp support which was sent along > with UNICAM v4l2 driver and some misc changes you have collected in > this series. Reference to v1: > https://lore.kernel.org/linux-media/20200504092611.9798-1-laurent.pinch...@ideasonboard.com/ > > On Mon, Jun 29, 2020 at 05:09:26PM +0200, Nicolas Saenz Julienne wrote: > > As vchiq_shim's callback does nothing aside from pushing messages into > > the service's queue, let's bypass it and jump directly to the service's > > callbacks, letting them choose whether to use the message queue. > > I admit this patch caused me some pain, as after a few days chasing > why the ISP got stuck in importing buffers into the VPU through the vc-sm-cma > driver I realized that this patch removed a significant part of the > process.. Sorry for the pain, I made my best to keep the downstream code in mind, and also to keep the amount of functional changes needed in the services minimal. That said, getting rid of VCHI is, IMO, a necessary step towards making VCHIQ upstreamable. > > It turns out most services don't need to use the message queue, which > > makes for simpler code in the end. > > > > - > > - if (reason == VCHIQ_MESSAGE_AVAILABLE) > > - vchiq_msg_queue_push(service->handle, header); > > This one '-.- > > I wonder if this was intentional and it is expected all services now > handle the message queue (it seems so according to your commit > message). Indeed, it was intentional. Upstream services (mmal & audio) don't need it and IIRC vchiq's ioctl interface has it's own queue implementation. So I figured it would be best to keep its usage optional. > Fair enough, I could add in the vc-sma-cma callback a call to > vchiq_msg_queue_push() but I wonder if it wouldn't be better to do so > in vchiq_core.c:parse_rx_slots(), just before calling the service's > callback, so that this has not to be re-implemented in all services. > > What would you suggest ? Actually, in hindsight my suggestion would be to get rid of the vchiq message queue altogether[1], keeping VCHIQ as simple as possible is a must if we want to get it upstream, and since vc-sma-cma is the only queue user there is little benefit to having a generic implementation. Let the service do it's own custom queueing and just force all services to release the messages when they see fit. It'll make for a simpler VCHIQ usage. > And by the way I see mmal-vchiq.c:service_callback() releasing > messages but never pushing them to the queue. Is this intended as well ? Yes, sorry, it's pretty confusing. That call, vchiq_release_message(), has nothing to do with the queue. It's the call that really gets the core to release the message from its slot and the only mandatory thing to do after receiving a message. All in all VCHIQ isn't documented an pretty cryptic in its design. I don't claim to be an expert. So feel free to contradict me anytime, I'll be happy to work out something that fits all of us. Regards, Nicolas [1] Here I mean vchiq_msg_queue_push() & vchiq_msg_hold()
[RFC] arm64: mm: Do not use both DMA zones when 30-bit address space unavailable
There is no benefit in splitting the 32-bit address space into two
distinct DMA zones when the 30-bit address space isn't even available on
a device. If that is the case, default to one big ZONE_DMA spanning the
whole 32-bit address space.
This will help reduce some of the issues we've seen with big crash
kernel allocations.
Signed-off-by: Nicolas Saenz Julienne
---
Whith this patch, on a 8GB RPi4 the setup looks like this:
DMA [mem 0x-0x3fff]
DMA32[mem 0x4000-0x]
Normal [mem 0x0001-0x0001]
And stock 8GB virtme/qemu:
DMA [mem 0x4000-0x]
DMA32empty
Normal [mem 0x0001-0x00023fff]
arch/arm64/mm/init.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index b6881d61b818..857a62611d7a 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -183,13 +183,20 @@ static void __init reserve_elfcorehdr(void)
/*
* Return the maximum physical address for a zone with a given address size
- * limit. It currently assumes that for memory starting above 4G, 32-bit
- * devices will use a DMA offset.
+ * limit or zero if memory starts from an address higher than the zone
targeted.
+ * It currently assumes that for memory starting above 4G, 32-bit devices will
+ * use a DMA offset.
*/
static phys_addr_t __init max_zone_phys(unsigned int zone_bits)
{
- phys_addr_t offset = memblock_start_of_DRAM() & GENMASK_ULL(63,
zone_bits);
- return min(offset + (1ULL << zone_bits), memblock_end_of_DRAM());
+ phys_addr_t base = memblock_start_of_DRAM();
+ phys_addr_t offset = base & GENMASK_ULL(63, 32);
+ s64 zone_size = (1ULL << zone_bits) - (base & DMA_BIT_MASK(32));
+
+ if (zone_size <= 0)
+ return 0;
+
+ return min(base + zone_size + offset, memblock_end_of_DRAM());
}
static void __init zone_sizes_init(unsigned long min, unsigned long max)
@@ -390,6 +397,20 @@ void __init arm64_memblock_init(void)
if (IS_ENABLED(CONFIG_ZONE_DMA)) {
zone_dma_bits = ARM64_ZONE_DMA_BITS;
arm64_dma_phys_limit = max_zone_phys(ARM64_ZONE_DMA_BITS);
+
+ /*
+* We don't want to split the 32 bit address space into two DMA
+* zones when the target lower physical address range doesn't
+* exist. For example, if memory starts at 0x8000 it's
+* pointless to create a ZONE_DMA distinct of ZONE_DMA32 as
+* there's no way to access the 30-bit address space. If that's
+* the case just expand ZONE_DMA to cover the whole 32-bit
+* address space.
+*/
+ if (!arm64_dma_phys_limit) {
+ zone_dma_bits = 32;
+ arm64_dma_phys_limit = max_zone_phys(32);
+ }
}
if (IS_ENABLED(CONFIG_ZONE_DMA32))
--
2.28.0
[PATCH v4 2/2] dma-pool: fix coherent pool allocations for IOMMU mappings
From: Christoph Hellwig
When allocating coherent pool memory for an IOMMU mapping we don't care
about the DMA mask. Move the guess for the initial GFP mask into the
dma_direct_alloc_pages and pass dma_coherent_ok as a function pointer
argument so that it doesn't get applied to the IOMMU case.
Signed-off-by: Christoph Hellwig
---
Changes since v1:
- Check if phys_addr_ok() exists prior calling it
drivers/iommu/dma-iommu.c | 4 +-
include/linux/dma-direct.h | 3 -
include/linux/dma-mapping.h | 5 +-
kernel/dma/direct.c | 13 ++--
kernel/dma/pool.c | 114 +++-
5 files changed, 62 insertions(+), 77 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..5141d49a046b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1035,8 +1035,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t
size,
if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
!gfpflags_allow_blocking(gfp) && !coherent)
- cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
- gfp);
+ page = dma_alloc_from_pool(dev, PAGE_ALIGN(size), _addr,
+ gfp, NULL);
else
cpu_addr = iommu_dma_alloc_pages(dev, size, , gfp, attrs);
if (!cpu_addr)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 5a3ce2a24794..6e87225600ae 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -73,9 +73,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t
addr, size_t size,
}
u64 dma_direct_get_required_mask(struct device *dev);
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
- u64 *phys_mask);
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size);
void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t gfp, unsigned long attrs);
void dma_direct_free(struct device *dev, size_t size, void *cpu_addr,
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 016b96b384bd..52635e91143b 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -522,8 +522,9 @@ void *dma_common_pages_remap(struct page **pages, size_t
size,
pgprot_t prot, const void *caller);
void dma_common_free_remap(void *cpu_addr, size_t size);
-void *dma_alloc_from_pool(struct device *dev, size_t size,
- struct page **ret_page, gfp_t flags);
+struct page *dma_alloc_from_pool(struct device *dev, size_t size,
+ void **cpu_addr, gfp_t flags,
+ bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t));
bool dma_free_from_pool(struct device *dev, void *start, size_t size);
int
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index bb0041e99659..db6ef07aec3b 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -43,7 +43,7 @@ u64 dma_direct_get_required_mask(struct device *dev)
return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
}
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
+static gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
u64 *phys_limit)
{
u64 dma_limit = min_not_zero(dma_mask, dev->bus_dma_limit);
@@ -68,7 +68,7 @@ gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64
dma_mask,
return 0;
}
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
+static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
return phys_to_dma_direct(dev, phys) + size - 1 <=
min_not_zero(dev->coherent_dma_mask,
dev->bus_dma_limit);
@@ -161,8 +161,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
size,
size = PAGE_ALIGN(size);
if (dma_should_alloc_from_pool(dev, gfp, attrs)) {
- ret = dma_alloc_from_pool(dev, size, , gfp);
- if (!ret)
+ u64 phys_mask;
+
+ gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
+ _mask);
+ page = dma_alloc_from_pool(dev, size, , gfp,
+ dma_coherent_ok);
+ if (!page)
return NULL;
goto done;
}
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 57f4a0f32a92..b0aaba4197ae 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -225,93 +225,75 @@ static int __init dma_atomic_pool_init(void)
}
postcore_initcall(dma_atomic_pool_init);
-static inline struct gen_pool *dma_guess_pool_from_device(struct device *dev)
+static inline struct gen_pool *dma_guess_pool(struct gen_pool *prev, gfp_t gfp)
{
- u64 phys_mask;
- gfp_t gfp;
-
- gfp =
[PATCH v4 0/2] dma-pool fixes
Now that we have an explanation to Amir's issue, we can re-spin this series. --- Changes since v3: - Do not use memblock_start_of_DRAM() Changes since v2: - Go back to v1's behavior for patch #2 Changes since v1: - Make cma_in_zone() more strict, GFP_KERNEL doesn't default to true now - Check if phys_addr_ok() exists prior calling it Christoph Hellwig (1): dma-pool: fix coherent pool allocations for IOMMU mappings Nicolas Saenz Julienne (1): dma-pool: Only allocate from CMA when in same memory zone drivers/iommu/dma-iommu.c | 4 +- include/linux/dma-direct.h | 3 - include/linux/dma-mapping.h | 5 +- kernel/dma/direct.c | 13 +++- kernel/dma/pool.c | 145 +++- 5 files changed, 92 insertions(+), 78 deletions(-) -- 2.28.0
[PATCH v4 1/2] dma-pool: Only allocate from CMA when in same memory zone
There is no guarantee to CMA's placement, so allocating a zone specific
atomic pool from CMA might return memory from a completely different
memory zone. To get around this double check CMA's placement before
allocating from it.
Signed-off-by: Nicolas Saenz Julienne
[hch: rebased, added a fallback to the page allocator, allow dipping into
lower CMA pools]
Signed-off-by: Christoph Hellwig
---
Changes since v3:
- Do not use memblock_start_of_DRAM()
Changes since v2:
- Go back to v1 behavior
kernel/dma/pool.c | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 6bc74a2d5127..57f4a0f32a92 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -3,7 +3,9 @@
* Copyright (C) 2012 ARM Ltd.
* Copyright (C) 2020 Google LLC
*/
+#include
#include
+#include
#include
#include
#include
@@ -55,6 +57,29 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t size)
pool_size_kernel += size;
}
+static bool cma_in_zone(gfp_t gfp)
+{
+ unsigned long size;
+ phys_addr_t end;
+ struct cma *cma;
+
+ cma = dev_get_cma_area(NULL);
+ if (!cma)
+ return false;
+
+ size = cma_get_size(cma);
+ if (!size)
+ return false;
+
+ /* CMA can't cross zone boundaries, see cma_activate_area() */
+ end = cma_get_base(cma) + size - 1;
+ if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA))
+ return end <= DMA_BIT_MASK(zone_dma_bits);
+ if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32))
+ return end <= DMA_BIT_MASK(32);
+ return true;
+}
+
static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
gfp_t gfp)
{
@@ -68,7 +93,11 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t
pool_size,
do {
pool_size = 1 << (PAGE_SHIFT + order);
- page = alloc_pages(gfp, order);
+ if (cma_in_zone(gfp))
+ page = dma_alloc_from_contiguous(NULL, 1 << order,
+order, false);
+ if (!page)
+ page = alloc_pages(gfp, order);
} while (!page && order-- > 0);
if (!page)
goto out;
--
2.28.0
Re: [PATCH v5 0/9] Raspberry Pi 4 USB firmware initialization rework
On Fri, 2020-08-14 at 08:11 +0200, Greg KH wrote: > On Thu, Aug 13, 2020 at 12:17:49PM -0700, Florian Fainelli wrote: > > > > On 8/13/2020 3:01 AM, Nicolas Saenz Julienne wrote: > > > Hi everyone. > > > > > > On Mon, 2020-06-29 at 18:18 +0200, Nicolas Saenz Julienne wrote: > > > > On the Raspberry Pi 4, after a PCI reset, VL805's firmware may either be > > > > loaded directly from an EEPROM or, if not present, by the SoC's > > > > co-processor, VideoCore. This series reworks how we handle this. > > > > > > > > The previous solution makes use of PCI quirks and exporting platform > > > > specific functions. Albeit functional it feels pretty shoehorned. This > > > > proposes an alternative way of handling the triggering of the xHCI chip > > > > initialization trough means of a reset controller. > > > > > > > > The benefits are pretty evident: less platform churn in core xHCI code, > > > > and no explicit device dependency management in pcie-brcmstb. > > > > > > > > Note that patch #1 depends on another series[1], that was just applied > > > > into the clk maintainer's tree. > > > > > > > > The series is based on v5.8-rc3 > > > > > > > > v3: https://www.spinics.net/lists/arm-kernel/msg813612.html > > > > v2: https://lkml.org/lkml/2020/6/9/875 > > > > v1: > > > > https://lore.kernel.org/linux-usb/20200608192701.18355-1-nsaenzjulie...@suse.de/T/#t > > > > > > > > [1] > > > > https://lore.kernel.org/linux-clk/159304773261.62212.983376627029743...@swboyd.mtv.corp.google.com/T/#t > > > > > > > > --- > > > > > > We were waiting on a dependency to be merged upstream to get this. They > > > are now > > > in, so could we move things forward? > > > > > > I can take the device tree patches, I guess philipp can take the reset > > > controller code. But I'm not so sure who should be taking the PCI/USB > > > counterparts. > > > > Should we route everything through the USB tree since that is where the > > changes that do require synchronization with other subsystems and DTS is > > needed the most? > > -- > > Florian > > That's fine with me, if everyone else is ok with it :) Sounds good to me.
Re: [PATCH v3 2/2] dma-pool: Only allocate from CMA when in same memory zone
On Fri, 2020-08-14 at 08:06 +0200, Christoph Hellwig wrote: > On Sat, Aug 08, 2020 at 08:33:54AM +0200, Christoph Hellwig wrote: > > On Fri, Aug 07, 2020 at 10:50:19AM +0200, Nicolas Saenz Julienne wrote: > > > On Fri, 2020-08-07 at 07:21 +0200, Christoph Hellwig wrote: > > > > On Thu, Aug 06, 2020 at 08:47:55PM +0200, Nicolas Saenz Julienne wrote: > > > > > There is no guarantee to CMA's placement, so allocating a zone > > > > > specific > > > > > atomic pool from CMA might return memory from a completely different > > > > > memory zone. To get around this double check CMA's placement before > > > > > allocating from it. > > > > > > > > As the builtbot pointed out, memblock_start_of_DRAM can't be used from > > > > non-__init code. But lookig at it I think throwing that in > > > > is bogus anyway, as cma_get_base returns a proper physical address > > > > already. > > > > > > It does indeed, but I'm comparing CMA's base with bitmasks that don't > > > take into > > > account where the memory starts. Say memory starts at 0x8000, and CMA > > > falls > > > into ZONE_DMA [0x8000 0xC000], if you want to compare it with > > > DMA_BIT_MASK(zone_dma_bits) you're forced to unify the memory bases. > > > > > > That said, I now realize that this doesn't work for ZONE_DMA32 which has > > > a hard > > > limit on 32bit addresses reglardless of the memory base. > > > > > > That said I still need to call memblock_start_of_DRAM() any suggestions > > > WRT > > > that? I could save the value in dma_atomic_pool_init(), which is __init > > > code. > > > > The pool must be about a 32-bit physical address. The offsets can be > > different for every device.. I now see what you mean. I was trying to blindly fit CMA with arm64's DMA zone setup, which, as it turns out, doesn't really honor its purpose. arm64 introduced ZONE_DMA to provide a 30-bit address space, but we're creating it regardless of whether it exists or not. This creates a mismatch between zone_dma_bits and ZONE_DMA's real placement. I'll try to look at fixing that in arm64. > Do you plan to resend this one without the memblock_start_of_DRAM > thingy? Yes, sorry for the wait, I've been on vacation and short on time, I'll send it during the day. Regards, Nicolas
Re: [PATCH v5 0/9] Raspberry Pi 4 USB firmware initialization rework
Hi everyone. On Mon, 2020-06-29 at 18:18 +0200, Nicolas Saenz Julienne wrote: > On the Raspberry Pi 4, after a PCI reset, VL805's firmware may either be > loaded directly from an EEPROM or, if not present, by the SoC's > co-processor, VideoCore. This series reworks how we handle this. > > The previous solution makes use of PCI quirks and exporting platform > specific functions. Albeit functional it feels pretty shoehorned. This > proposes an alternative way of handling the triggering of the xHCI chip > initialization trough means of a reset controller. > > The benefits are pretty evident: less platform churn in core xHCI code, > and no explicit device dependency management in pcie-brcmstb. > > Note that patch #1 depends on another series[1], that was just applied > into the clk maintainer's tree. > > The series is based on v5.8-rc3 > > v3: https://www.spinics.net/lists/arm-kernel/msg813612.html > v2: https://lkml.org/lkml/2020/6/9/875 > v1: > https://lore.kernel.org/linux-usb/20200608192701.18355-1-nsaenzjulie...@suse.de/T/#t > > [1] > https://lore.kernel.org/linux-clk/159304773261.62212.983376627029743...@swboyd.mtv.corp.google.com/T/#t > > --- We were waiting on a dependency to be merged upstream to get this. They are now in, so could we move things forward? I can take the device tree patches, I guess philipp can take the reset controller code. But I'm not so sure who should be taking the PCI/USB counterparts. Regards, Nicolas > > Changes since v4: > - Adress Andy's comments > > Changes since v3: > - Rework dt patch to include root bridge as a separate node > - Update xhci-pci patch now that the xhci dev has a dt node (it was >getting it in the past from its bus) > > Changes since v2: > - Add reset to resume routine in xhci-pci > - Correct of refcount in pci-quirks > - Correct typos > - Use include file to define firmware reset IDs > > Changes since v1: > - Rework reset controller so it's less USB centric > - Use correct reset controller API in xhci-pci > - Correct typos > > Nicolas Saenz Julienne (9): > dt-bindings: reset: Add a binding for the RPi Firmware reset > controller > reset: Add Raspberry Pi 4 firmware reset controller > ARM: dts: bcm2711: Add firmware usb reset node > ARM: dts: bcm2711: Add reset controller to xHCI node > usb: xhci-pci: Add support for reset controllers > Revert "USB: pci-quirks: Add Raspberry Pi 4 quirk" > usb: host: pci-quirks: Bypass xHCI quirks for Raspberry Pi 4 > Revert "firmware: raspberrypi: Introduce vl805 init routine" > Revert "PCI: brcmstb: Wait for Raspberry Pi's firmware when present" > > .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 21 +++ > arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 22 > drivers/firmware/Kconfig | 3 +- > drivers/firmware/raspberrypi.c| 61 - > drivers/pci/controller/pcie-brcmstb.c | 17 --- > drivers/reset/Kconfig | 11 ++ > drivers/reset/Makefile| 1 + > drivers/reset/reset-raspberrypi.c | 122 ++ > drivers/usb/host/pci-quirks.c | 22 ++-- > drivers/usb/host/xhci-pci.c | 10 ++ > drivers/usb/host/xhci.h | 2 + > .../reset/raspberrypi,firmware-reset.h| 13 ++ > include/soc/bcm2835/raspberrypi-firmware.h| 7 - > 13 files changed, 215 insertions(+), 97 deletions(-) > create mode 100644 drivers/reset/reset-raspberrypi.c > create mode 100644 include/dt-bindings/reset/raspberrypi,firmware-reset.h >
Re: [PATCH v3 2/2] dma-pool: Only allocate from CMA when in same memory zone
On Fri, 2020-08-07 at 07:21 +0200, Christoph Hellwig wrote: > On Thu, Aug 06, 2020 at 08:47:55PM +0200, Nicolas Saenz Julienne wrote: > > There is no guarantee to CMA's placement, so allocating a zone specific > > atomic pool from CMA might return memory from a completely different > > memory zone. To get around this double check CMA's placement before > > allocating from it. > > As the builtbot pointed out, memblock_start_of_DRAM can't be used from > non-__init code. But lookig at it I think throwing that in > is bogus anyway, as cma_get_base returns a proper physical address > already. It does indeed, but I'm comparing CMA's base with bitmasks that don't take into account where the memory starts. Say memory starts at 0x8000, and CMA falls into ZONE_DMA [0x8000 0xC000], if you want to compare it with DMA_BIT_MASK(zone_dma_bits) you're forced to unify the memory bases. That said, I now realize that this doesn't work for ZONE_DMA32 which has a hard limit on 32bit addresses reglardless of the memory base. That said I still need to call memblock_start_of_DRAM() any suggestions WRT that? I could save the value in dma_atomic_pool_init(), which is __init code. signature.asc Description: This is a digitally signed message part
[PATCH v3 0/2] dma-pool fixes
Now that we have an explanation to Amir's issue, we can re-spin this series. --- Changes since v2: - Go back to v1's behavior for patch #2 Changes since v1: - Make cma_in_zone() more strict, GFP_KERNEL doesn't default to true now - Check if phys_addr_ok() exists prior calling it Christoph Hellwig (1): dma-pool: fix coherent pool allocations for IOMMU mappings Nicolas Saenz Julienne (1): dma-pool: Only allocate from CMA when in same memory zone drivers/iommu/dma-iommu.c | 4 +- include/linux/dma-direct.h | 3 - include/linux/dma-mapping.h | 5 +- kernel/dma/direct.c | 13 +++- kernel/dma/pool.c | 145 +++- 5 files changed, 92 insertions(+), 78 deletions(-) -- 2.28.0
[PATCH v3 2/2] dma-pool: Only allocate from CMA when in same memory zone
There is no guarantee to CMA's placement, so allocating a zone specific
atomic pool from CMA might return memory from a completely different
memory zone. To get around this double check CMA's placement before
allocating from it.
Signed-off-by: Nicolas Saenz Julienne
[hch: rebased, added a fallback to the page allocator, allow dipping into
lower CMA pools]
Signed-off-by: Christoph Hellwig
---
Changes since v2:
- Go back to v1 behavior
kernel/dma/pool.c | 31 ++-
1 file changed, 30 insertions(+), 1 deletion(-)
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 5d071d4a3cba..30d28d761490 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -3,7 +3,9 @@
* Copyright (C) 2012 ARM Ltd.
* Copyright (C) 2020 Google LLC
*/
+#include
#include
+#include
#include
#include
#include
@@ -55,6 +57,29 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t size)
pool_size_kernel += size;
}
+static bool cma_in_zone(gfp_t gfp)
+{
+ unsigned long size;
+ phys_addr_t end;
+ struct cma *cma;
+
+ cma = dev_get_cma_area(NULL);
+ if (!cma)
+ return false;
+
+ size = cma_get_size(cma);
+ if (!size)
+ return false;
+
+ /* CMA can't cross zone boundaries, see cma_activate_area() */
+ end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
+ if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA))
+ return end <= DMA_BIT_MASK(zone_dma_bits);
+ if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32))
+ return end <= DMA_BIT_MASK(32);
+ return true;
+}
+
static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
gfp_t gfp)
{
@@ -68,7 +93,11 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t
pool_size,
do {
pool_size = 1 << (PAGE_SHIFT + order);
- page = alloc_pages(gfp, order);
+ if (cma_in_zone(gfp))
+ page = dma_alloc_from_contiguous(NULL, 1 << order,
+order, false);
+ if (!page)
+ page = alloc_pages(gfp, order);
} while (!page && order-- > 0);
if (!page)
goto out;
--
2.28.0
[PATCH v3 1/2] dma-pool: fix coherent pool allocations for IOMMU mappings
From: Christoph Hellwig
When allocating coherent pool memory for an IOMMU mapping we don't care
about the DMA mask. Move the guess for the initial GFP mask into the
dma_direct_alloc_pages and pass dma_coherent_ok as a function pointer
argument so that it doesn't get applied to the IOMMU case.
Signed-off-by: Christoph Hellwig
---
Changes since v1:
- Check if phys_addr_ok() exists prior calling it
drivers/iommu/dma-iommu.c | 4 +-
include/linux/dma-direct.h | 3 -
include/linux/dma-mapping.h | 5 +-
kernel/dma/direct.c | 13 ++--
kernel/dma/pool.c | 114 +++-
5 files changed, 62 insertions(+), 77 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..5141d49a046b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1035,8 +1035,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t
size,
if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
!gfpflags_allow_blocking(gfp) && !coherent)
- cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
- gfp);
+ page = dma_alloc_from_pool(dev, PAGE_ALIGN(size), _addr,
+ gfp, NULL);
else
cpu_addr = iommu_dma_alloc_pages(dev, size, , gfp, attrs);
if (!cpu_addr)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index 5a3ce2a24794..6e87225600ae 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -73,9 +73,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t
addr, size_t size,
}
u64 dma_direct_get_required_mask(struct device *dev);
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
- u64 *phys_mask);
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size);
void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t gfp, unsigned long attrs);
void dma_direct_free(struct device *dev, size_t size, void *cpu_addr,
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 016b96b384bd..52635e91143b 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -522,8 +522,9 @@ void *dma_common_pages_remap(struct page **pages, size_t
size,
pgprot_t prot, const void *caller);
void dma_common_free_remap(void *cpu_addr, size_t size);
-void *dma_alloc_from_pool(struct device *dev, size_t size,
- struct page **ret_page, gfp_t flags);
+struct page *dma_alloc_from_pool(struct device *dev, size_t size,
+ void **cpu_addr, gfp_t flags,
+ bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t));
bool dma_free_from_pool(struct device *dev, void *start, size_t size);
int
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index bb0041e99659..db6ef07aec3b 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -43,7 +43,7 @@ u64 dma_direct_get_required_mask(struct device *dev)
return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
}
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
+static gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
u64 *phys_limit)
{
u64 dma_limit = min_not_zero(dma_mask, dev->bus_dma_limit);
@@ -68,7 +68,7 @@ gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64
dma_mask,
return 0;
}
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
+static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
return phys_to_dma_direct(dev, phys) + size - 1 <=
min_not_zero(dev->coherent_dma_mask,
dev->bus_dma_limit);
@@ -161,8 +161,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
size,
size = PAGE_ALIGN(size);
if (dma_should_alloc_from_pool(dev, gfp, attrs)) {
- ret = dma_alloc_from_pool(dev, size, , gfp);
- if (!ret)
+ u64 phys_mask;
+
+ gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
+ _mask);
+ page = dma_alloc_from_pool(dev, size, , gfp,
+ dma_coherent_ok);
+ if (!page)
return NULL;
goto done;
}
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 6bc74a2d5127..5d071d4a3cba 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -196,93 +196,75 @@ static int __init dma_atomic_pool_init(void)
}
postcore_initcall(dma_atomic_pool_init);
-static inline struct gen_pool *dma_guess_pool_from_device(struct device *dev)
+static inline struct gen_pool *dma_guess_pool(struct gen_pool *prev, gfp_t gfp)
{
- u64 phys_mask;
- gfp_t gfp;
-
- gfp =
Re: [PATCH v2 2/2] dma-pool: Only allocate from CMA when in same memory zone
On Thu, 2020-08-06 at 07:18 +0200, Christoph Hellwig wrote: > On Tue, Aug 04, 2020 at 11:43:15AM +0200, Nicolas Saenz Julienne wrote: > > > Second I don't see the need (and actually some harm) in preventing > > > GFP_KERNEL > > > allocations from dipping into lower CMA areas - something that we did > > > support > > > before 5.8 with the single pool. > > > > My thinking is the least we pressure CMA the better, it's generally scarse, > > and > > it'll not grow as the atomic pools grow. As far as harm is concerned, we now > > check addresses for correctness, so we shouldn't run into problems. > > > > There is a potential case for architectures defining a default CMA but not > > defining DMA zones where this could be problematic. But isn't that just > > plain > > abusing CMA? If you need low memory allocations, you should be defining DMA > > zones. > > The latter is pretty much what I expect, as we only support the default and > per-device DMA CMAs. Fair enough, should I send a v3 with everything cleaned-up/rebased, or you'd rather pick it up from your version? signature.asc Description: This is a digitally signed message part
Re: [PATCH v2 2/2] dma-pool: Only allocate from CMA when in same memory zone
On Tue, 2020-08-04 at 08:06 +0200, Christoph Hellwig wrote: > On Mon, Aug 03, 2020 at 06:09:56PM +0200, Nicolas Saenz Julienne wrote: > > + if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA)) > > + return end <= DMA_BIT_MASK(zone_dma_bits); > > + if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32)) > > + return end <= DMA_BIT_MASK(32); > > + if (gfp & GFP_KERNEL) > > + return end > DMA_BIT_MASK(32); > > So the GFP_KERNEL one here looks weird. For one I don't think the if > line is needed at all, and it just confuses things. Yes, sorry, shoud've seen that. > Second I don't see the need (and actually some harm) in preventing GFP_KERNEL > allocations from dipping into lower CMA areas - something that we did support > before 5.8 with the single pool. My thinking is the least we pressure CMA the better, it's generally scarse, and it'll not grow as the atomic pools grow. As far as harm is concerned, we now check addresses for correctness, so we shouldn't run into problems. There is a potential case for architectures defining a default CMA but not defining DMA zones where this could be problematic. But isn't that just plain abusing CMA? If you need low memory allocations, you should be defining DMA zones. Regards, Nicolas signature.asc Description: This is a digitally signed message part
[PATCH v2 0/2] dma-pool fixes
Now that we have an explanation to Amir's issue, I took the liberty to respin the previous dma-pool fixes series with some changes/fixes of my own. --- Changes since v1: - Make cma_in_zone() more strict, GFP_KERNEL doesn't default to true now - Check if phys_addr_ok() exists prior calling it Christoph Hellwig (1): dma-pool: fix coherent pool allocations for IOMMU mappings Nicolas Saenz Julienne (1): dma-pool: Only allocate from CMA when in same memory zone drivers/iommu/dma-iommu.c | 4 +- include/linux/dma-direct.h | 3 - include/linux/dma-mapping.h | 5 +- kernel/dma/direct.c | 13 +++- kernel/dma/pool.c | 148 5 files changed, 95 insertions(+), 78 deletions(-) -- 2.28.0
[PATCH v2 1/2] dma-pool: fix coherent pool allocations for IOMMU mappings
From: Christoph Hellwig
When allocating coherent pool memory for an IOMMU mapping we don't care
about the DMA mask. Move the guess for the initial GFP mask into the
dma_direct_alloc_pages and pass dma_coherent_ok as a function pointer
argument so that it doesn't get applied to the IOMMU case.
Signed-off-by: Christoph Hellwig
---
Changes since v1:
- Check if phys_addr_ok() exists prior calling it
drivers/iommu/dma-iommu.c | 4 +-
include/linux/dma-direct.h | 3 -
include/linux/dma-mapping.h | 5 +-
kernel/dma/direct.c | 13 ++--
kernel/dma/pool.c | 114 +++-
5 files changed, 62 insertions(+), 77 deletions(-)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 4959f5df21bd..5141d49a046b 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1035,8 +1035,8 @@ static void *iommu_dma_alloc(struct device *dev, size_t
size,
if (IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
!gfpflags_allow_blocking(gfp) && !coherent)
- cpu_addr = dma_alloc_from_pool(dev, PAGE_ALIGN(size), ,
- gfp);
+ page = dma_alloc_from_pool(dev, PAGE_ALIGN(size), _addr,
+ gfp, NULL);
else
cpu_addr = iommu_dma_alloc_pages(dev, size, , gfp, attrs);
if (!cpu_addr)
diff --git a/include/linux/dma-direct.h b/include/linux/dma-direct.h
index ab2e20cba951..ba22952c24e2 100644
--- a/include/linux/dma-direct.h
+++ b/include/linux/dma-direct.h
@@ -67,9 +67,6 @@ static inline bool dma_capable(struct device *dev, dma_addr_t
addr, size_t size,
}
u64 dma_direct_get_required_mask(struct device *dev);
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
- u64 *phys_mask);
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size);
void *dma_direct_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
gfp_t gfp, unsigned long attrs);
void dma_direct_free(struct device *dev, size_t size, void *cpu_addr,
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index a33ed3954ed4..0dc08701d7b7 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -715,8 +715,9 @@ void *dma_common_pages_remap(struct page **pages, size_t
size,
pgprot_t prot, const void *caller);
void dma_common_free_remap(void *cpu_addr, size_t size);
-void *dma_alloc_from_pool(struct device *dev, size_t size,
- struct page **ret_page, gfp_t flags);
+struct page *dma_alloc_from_pool(struct device *dev, size_t size,
+ void **cpu_addr, gfp_t flags,
+ bool (*phys_addr_ok)(struct device *, phys_addr_t, size_t));
bool dma_free_from_pool(struct device *dev, void *start, size_t size);
int
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 67f060b86a73..f17aec9d01f0 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -45,7 +45,7 @@ u64 dma_direct_get_required_mask(struct device *dev)
return (1ULL << (fls64(max_dma) - 1)) * 2 - 1;
}
-gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
+static gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64 dma_mask,
u64 *phys_limit)
{
u64 dma_limit = min_not_zero(dma_mask, dev->bus_dma_limit);
@@ -70,7 +70,7 @@ gfp_t dma_direct_optimal_gfp_mask(struct device *dev, u64
dma_mask,
return 0;
}
-bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
+static bool dma_coherent_ok(struct device *dev, phys_addr_t phys, size_t size)
{
return phys_to_dma_direct(dev, phys) + size - 1 <=
min_not_zero(dev->coherent_dma_mask,
dev->bus_dma_limit);
@@ -163,8 +163,13 @@ void *dma_direct_alloc_pages(struct device *dev, size_t
size,
size = PAGE_ALIGN(size);
if (dma_should_alloc_from_pool(dev, gfp, attrs)) {
- ret = dma_alloc_from_pool(dev, size, , gfp);
- if (!ret)
+ u64 phys_mask;
+
+ gfp |= dma_direct_optimal_gfp_mask(dev, dev->coherent_dma_mask,
+ _mask);
+ page = dma_alloc_from_pool(dev, size, , gfp,
+ dma_coherent_ok);
+ if (!page)
return NULL;
goto done;
}
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 6bc74a2d5127..5d071d4a3cba 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -196,93 +196,75 @@ static int __init dma_atomic_pool_init(void)
}
postcore_initcall(dma_atomic_pool_init);
-static inline struct gen_pool *dma_guess_pool_from_device(struct device *dev)
+static inline struct gen_pool *dma_guess_pool(struct gen_pool *prev, gfp_t gfp)
{
- u64 phys_mask;
- gfp_t gfp;
-
- gfp =
[PATCH v2 2/2] dma-pool: Only allocate from CMA when in same memory zone
There is no guarantee to CMA's placement, so allocating a zone specific
atomic pool from CMA might return memory from a completely different
memory zone. To get around this double check CMA's placement before
allocating from it.
Signed-off-by: Nicolas Saenz Julienne
---
Changes since v1:
- Make cma_in_zone() more strict, GFP_KERNEL doesn't default to true
now
kernel/dma/pool.c | 34 +-
1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/kernel/dma/pool.c b/kernel/dma/pool.c
index 5d071d4a3cba..582523ccf4fe 100644
--- a/kernel/dma/pool.c
+++ b/kernel/dma/pool.c
@@ -3,7 +3,9 @@
* Copyright (C) 2012 ARM Ltd.
* Copyright (C) 2020 Google LLC
*/
+#include
#include
+#include
#include
#include
#include
@@ -55,6 +57,32 @@ static void dma_atomic_pool_size_add(gfp_t gfp, size_t size)
pool_size_kernel += size;
}
+static bool cma_in_zone(gfp_t gfp)
+{
+ unsigned long size;
+ phys_addr_t end;
+ struct cma *cma;
+
+ cma = dev_get_cma_area(NULL);
+ if (!cma)
+ return false;
+
+ size = cma_get_size(cma);
+ if (!size)
+ return false;
+
+ /* CMA can't cross zone boundaries, see cma_activate_area() */
+ end = cma_get_base(cma) - memblock_start_of_DRAM() + size - 1;
+ if (IS_ENABLED(CONFIG_ZONE_DMA) && (gfp & GFP_DMA))
+ return end <= DMA_BIT_MASK(zone_dma_bits);
+ if (IS_ENABLED(CONFIG_ZONE_DMA32) && (gfp & GFP_DMA32))
+ return end <= DMA_BIT_MASK(32);
+ if (gfp & GFP_KERNEL)
+ return end > DMA_BIT_MASK(32);
+
+ return false;
+}
+
static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
gfp_t gfp)
{
@@ -68,7 +96,11 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t
pool_size,
do {
pool_size = 1 << (PAGE_SHIFT + order);
- page = alloc_pages(gfp, order);
+ if (cma_in_zone(gfp))
+ page = dma_alloc_from_contiguous(NULL, 1 << order,
+order, false);
+ if (!page)
+ page = alloc_pages(gfp, order);
} while (!page && order-- > 0);
if (!page)
goto out;
--
2.28.0
[PATCH] of: unittest: Use bigger address cells to catch parser regressions
Getting address and size cells for dma-ranges/ranges parsing is tricky
and shouldn't rely on the node's count_cells() method. The function
starts looking for cells on the parent node, as its supposed to work
with device nodes, which doesn't work when input with bus nodes, as
generally done when parsing ranges.
Add test to catch regressions on that specific quirk as developers will
be tempted to edit it out in favor of the default method.
Signed-off-by: Nicolas Saenz Julienne
---
drivers/of/unittest-data/tests-address.dtsi | 10 +-
drivers/of/unittest.c | 2 +-
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/drivers/of/unittest-data/tests-address.dtsi
b/drivers/of/unittest-data/tests-address.dtsi
index 3fe5d3987beb..6604a52bf6cb 100644
--- a/drivers/of/unittest-data/tests-address.dtsi
+++ b/drivers/of/unittest-data/tests-address.dtsi
@@ -23,13 +23,13 @@ device@7000 {
};
bus@8000 {
- #address-cells = <1>;
- #size-cells = <1>;
- ranges = <0x0 0x8000 0x10>;
- dma-ranges = <0x1000 0x0 0x4000>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges = <0x0 0x0 0x8000 0x0 0x10>;
+ dma-ranges = <0x1 0x0 0x0 0x20 0x0>;
device@1000 {
- reg = <0x1000 0x1000>;
+ reg = <0x0 0x1000 0x0 0x1000>;
};
};
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 398de04fd19c..9b7e84bdc7d4 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -900,7 +900,7 @@ static void __init of_unittest_parse_dma_ranges(void)
of_unittest_dma_ranges_one("/testcase-data/address-tests/device@7000",
0x0, 0x2000, 0x4000);
of_unittest_dma_ranges_one("/testcase-data/address-tests/bus@8000/device@1000",
- 0x1000, 0x2000, 0x4000);
+ 0x1, 0x2000, 0x20);
of_unittest_dma_ranges_one("/testcase-data/address-tests/pci@9000",
0x8000, 0x2000, 0x1000);
}
--
2.28.0
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v9 09/12] PCI: brcmstb: Set additional internal memory DMA viewport sizes
Hi Jim,
On Fri, 2020-07-24 at 16:33 -0400, Jim Quinlan wrote:
> The Raspberry Pi (RPI) is currently the only chip using this driver
> (pcie-brcmstb.c). There, only one memory controller is used, without an
> extension region, and the SCB0 viewport size is set to the size of the
> first and only dma-range region. Other BrcmSTB SOCs have more complicated
> memory configurations that require setting additional viewport sizes.
>
> BrcmSTB PCIe controllers are intimately connected to the memory
> controller(s) on the SOC. The SOC may have one to three memory
> controllers; they are indicated by the term SCBi. Each controller has a
> base region and an optional extension region. In physical memory, the base
> and extension regions of a controller are not adjacent, but in PCIe-space
> they are.
>
> There is a "viewport" for each memory controller that allows DMA from
> endpoint devices. Each viewport's size must be set to a power of two, and
> that size must be equal to or larger than the amount of memory each
> controller supports which is the sum of base region and its optional
> extension. Further, the 1-3 viewports are also adjacent in PCIe-space.
>
> Unfortunately the viewport sizes cannot be ascertained from the
> "dma-ranges" property so they have their own property, "brcm,scb-sizes".
> This is because dma-range information does not indicate what memory
> controller it is associated. For example, consider the following case
> where the size of one dma-range is 2GB and the second dma-range is 1GB:
>
> /* Case 1: SCB0 size set to 4GB */
> dma-range0: 2GB (from memc0-base)
> dma-range1: 1GB (from memc0-extension)
>
> /* Case 2: SCB0 size set to 2GB, SCB1 size set to 1GB */
> dma-range0: 2GB (from memc0-base)
> dma-range1: 1GB (from memc0-extension)
>
> By just looking at the dma-ranges information, one cannot tell which
> situation applies. That is why an additional property is needed. Its
> length indicates the number of memory controllers being used and each value
> indicates the viewport size.
>
> Note that the RPI DT does not have a "brcm,scb-sizes" property value,
> as it is assumed that it only requires one memory controller and no
> extension. So the optional use of "brcm,scb-sizes" will be backwards
> compatible.
>
> One last layer of complexity exists: all of the viewports sizes must be
> added and rounded up to a power of two to determine what the "BAR" size is.
> Further, an offset must be given that indicates the base PCIe address of
> this "BAR". The use of the term BAR is typically associated with endpoint
> devices, and the term is used here because the PCIe HW may be used as an RC
> or an EP. In the former case, all of the system memory appears in a single
> "BAR" region in PCIe memory. As it turns out, BrcmSTB PCIe HW is rarely
> used in the EP role and its system of mapping memory is an artifact that
> requires multiple dma-ranges regions.
>
> Signed-off-by: Jim Quinlan
> Acked-by: Florian Fainelli
> ---
> drivers/pci/controller/pcie-brcmstb.c | 68 ---
> 1 file changed, 50 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pci/controller/pcie-brcmstb.c
> b/drivers/pci/controller/pcie-brcmstb.c
> index 8dacb9d3b7b6..3ef2d37cc43b 100644
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -715,22 +720,44 @@ static inline int
> brcm_pcie_get_rc_bar2_size_and_offset(struct brcm_pcie *pcie,
> u64 *rc_bar2_offset)
> {
> struct pci_host_bridge *bridge = pci_host_bridge_from_priv(pcie);
> - struct device *dev = pcie->dev;
> struct resource_entry *entry;
> + struct device *dev = pcie->dev;
> + u64 lowest_pcie_addr = ~(u64)0;
> + int ret, i = 0;
> + u64 size = 0;
>
> - entry = resource_list_first_type(>dma_ranges, IORESOURCE_MEM);
> - if (!entry)
> - return -ENODEV;
> + resource_list_for_each_entry(entry, >dma_ranges) {
> + u64 pcie_beg = entry->res->start - entry->offset;
>
> + size += entry->res->end - entry->res->start + 1;
> + if (pcie_beg < lowest_pcie_addr)
> + lowest_pcie_addr = pcie_beg;
> + }
>
> - /*
> - * The controller expects the inbound window offset to be calculated as
> - * the difference between PCIe's address space and CPU's. The offset
> - * provided by the firmware is calculated the opposite way, so we
> - * negate it.
> - */
> - *rc_bar2_offset = -entry->offset;
> - *rc_bar2_size = 1ULL << fls64(entry->res->end - entry->res->start);
> + if (lowest_pcie_addr == ~(u64)0) {
> + dev_err(dev, "DT node has no dma-ranges\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_variable_u64_array(pcie->np, "brcm,scb-sizes",
> pcie->memc_size, 1,
> +
Re: [PATCH] dma-pool: Do not allocate pool memory from CMA
Hi Nathan,
On Thu, 2020-07-30 at 18:10 -0700, Nathan Chancellor wrote:
> On Tue, Jul 28, 2020 at 12:09:18PM +0200, Christoph Hellwig wrote:
> > Ok, I found a slight bug that wasn't intended. I wanted to make sure
> > we can always fall back to a lower pool, but got that wrong. Should be
> > fixed in the next version.
>
> Hi Christoph and Nicolas,
>
> Did a version of that series ever get send out? I am coming into the
> conversation late but I am running into an issue with the Raspberry Pi 4
> not booting on linux-next, which appears to be due to this patch now in
> mainline as commit d9765e41d8e9 ("dma-pool: do not allocate pool memory
> from CMA") combined with
> https://lore.kernel.org/lkml/20200725014529.1143208-2-jiaxun.y...@flygoat.com/
> in -next:
>
> [1.423163] raspberrypi-firmware soc:firmware: Request 0x0001 returned
> status 0x
> [1.431883] raspberrypi-firmware soc:firmware: Request 0x00030046 returned
> status 0x
> [1.443888] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.452527] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 0
> config (-22 80)
> [1.460836] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.469445] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
> config (-22 81)
> [1.477735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.486350] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
> config (-22 82)
> [1.494639] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.503246] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 3
> config (-22 83)
> [1.511529] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.520131] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
> config (-22 84)
> [1.528414] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.537017] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 5
> config (-22 85)
> [1.545299] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.553903] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
> config (-22 86)
> [1.562184] raspberrypi-firmware soc:firmware: Request 0x00030043
> [1.570787] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 7
> config (-22 87)
> [1.579897] raspberrypi-firmware soc:firmware: Request 0x00030030 returned
> status 0x
> [1.589419] raspberrypi-firmware soc:firmware: Request 0x00028001 returned
> status 0x
> [1.599391] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.608018] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
> config (-22 81)
> [1.616313] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.624932] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 1
> config (-22 81)
> [1.633195] pwrseq_simple: probe of wifi-pwrseq failed with error -22
> [1.643904] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.652544] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
> config (-22 82)
> [1.660839] raspberrypi-firmware soc:firmware: Request 0x00030041 returned
> status 0x
> [1.669446] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 2
> state (-22 82)
> [1.677727] leds-gpio: probe of leds failed with error -22
> [1.683735] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.692346] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
> config (-22 86)
> [1.700636] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.709240] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 6
> config (-22 86)
> [1.717496] reg-fixed-voltage: probe of sd_vcc_reg failed with error -22
> [1.725546] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.734176] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
> config (-22 84)
> [1.742465] raspberrypi-firmware soc:firmware: Request 0x00030043 returned
> status 0x
> [1.751072] raspberrypi-exp-gpio soc:firmware:gpio: Failed to get GPIO 4
> config (-22 84)
> [1.759332] gpio-regulator: probe of sd_io_1v8_reg failed with error -22
> [1.768042] raspberrypi-firmware soc:firmware: Request 0x00028001 returned
> status 0x
> [1.780871] ALSA device list:
> [1.783960] No soundcards found.
> [1.787633] Waiting for root device PARTUUID=45a8dd8a-02...
>
> I am unsure if it is related to the issue that Amit is having or
> if that makes sense at all but I can reliably reproduce it.
>
> v5.8-rc1:
