* [PATCH v1 2/3] soc/tegra: regulators: Add regulators coupler for Tegra20
From: Dmitry Osipenko @ 2019-07-15 19:45 UTC (permalink / raw)
To: Rob Herring, Thierry Reding, Peter De Schrijver, Jonathan Hunter,
Mark Brown
Cc: devicetree, linux-tegra, linux-kernel
In-Reply-To: <20190715194503.19100-1-digetx@gmail.com>
Add regulators coupler for Tegra20 SoCs that performs voltage balancing
of a coupled regulators and thus provides voltage scaling functionality.
There are 3 coupled regulators on all Tegra20 SoCs: CORE, RTC and CPU.
The CORE and RTC voltages shall be in range of 170mV from each other and
they both shall be higher than the CPU voltage by at least 120mV. This
sounds like it could be handle by a generic voltage balancer, but the CORE
voltage scaling isn't implemented in any of the upstream drivers yet.
It will take quite some time and effort to hook up voltage scaling for
all of the drivers, hence we will use a custom coupler that will manage
the CPU voltage scaling for the starter.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/soc/tegra/Kconfig | 6 +
drivers/soc/tegra/Makefile | 1 +
drivers/soc/tegra/regulators-tegra20.c | 350 +++++++++++++++++++++++++
3 files changed, 357 insertions(+)
create mode 100644 drivers/soc/tegra/regulators-tegra20.c
diff --git a/drivers/soc/tegra/Kconfig b/drivers/soc/tegra/Kconfig
index d98c69efb7e0..7986ab80e07a 100644
--- a/drivers/soc/tegra/Kconfig
+++ b/drivers/soc/tegra/Kconfig
@@ -14,6 +14,7 @@ config ARCH_TEGRA_2x_SOC
select PL310_ERRATA_769419 if CACHE_L2X0
select SOC_TEGRA_FLOWCTRL
select SOC_TEGRA_PMC
+ select SOC_TEGRA20_VOLTAGE_COUPLER
select TEGRA_TIMER
help
Support for NVIDIA Tegra AP20 and T20 processors, based on the
@@ -134,3 +135,8 @@ config SOC_TEGRA_POWERGATE_BPMP
def_bool y
depends on PM_GENERIC_DOMAINS
depends on TEGRA_BPMP
+
+config SOC_TEGRA20_VOLTAGE_COUPLER
+ bool "Voltage scaling support for Tegra20 SoCs"
+ depends on ARCH_TEGRA_2x_SOC || COMPILE_TEST
+
diff --git a/drivers/soc/tegra/Makefile b/drivers/soc/tegra/Makefile
index 902759fe5f4d..9f0bdd53bef8 100644
--- a/drivers/soc/tegra/Makefile
+++ b/drivers/soc/tegra/Makefile
@@ -5,3 +5,4 @@ obj-y += common.o
obj-$(CONFIG_SOC_TEGRA_FLOWCTRL) += flowctrl.o
obj-$(CONFIG_SOC_TEGRA_PMC) += pmc.o
obj-$(CONFIG_SOC_TEGRA_POWERGATE_BPMP) += powergate-bpmp.o
+obj-$(CONFIG_SOC_TEGRA20_VOLTAGE_COUPLER) += regulators-tegra20.o
diff --git a/drivers/soc/tegra/regulators-tegra20.c b/drivers/soc/tegra/regulators-tegra20.c
new file mode 100644
index 000000000000..12c8206cea16
--- /dev/null
+++ b/drivers/soc/tegra/regulators-tegra20.c
@@ -0,0 +1,350 @@
+// SPDX-License-Identifier: GPL-2.0+
+//
+// Voltage regulators coupler for NVIDIA Tegra20
+// Copyright (C) 2019 GRATE-DRIVER project
+//
+// Voltage constraints borrowed from downstream kernel sources
+// Copyright (C) 2010-2011 NVIDIA Corporation
+
+#define pr_fmt(fmt) "tegra voltage-coupler: " fmt
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/regulator/coupler.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+
+struct tegra_regulator_coupler {
+ struct regulator_coupler coupler;
+ struct regulator_dev *core_rdev;
+ struct regulator_dev *cpu_rdev;
+ struct regulator_dev *rtc_rdev;
+ int core_min_uV;
+};
+
+static inline struct tegra_regulator_coupler *
+to_tegra_coupler(struct regulator_coupler *coupler)
+{
+ return container_of(coupler, struct tegra_regulator_coupler, coupler);
+}
+
+static int tegra20_core_limit(struct tegra_regulator_coupler *tegra,
+ struct regulator_dev *core_rdev)
+{
+ int core_min_uV = 0;
+ int core_max_uV;
+ int core_cur_uV;
+ int err;
+
+ if (tegra->core_min_uV > 0)
+ return tegra->core_min_uV;
+
+ core_cur_uV = regulator_get_voltage_rdev(core_rdev);
+ if (core_cur_uV < 0)
+ return core_cur_uV;
+
+ core_max_uV = max(core_cur_uV, 1200000);
+
+ err = regulator_check_voltage(core_rdev, &core_min_uV, &core_max_uV);
+ if (err)
+ return err;
+
+ /*
+ * Limit minimum CORE voltage to a value left from bootloader or,
+ * if it's unreasonably low value, to the most common 1.2v or to
+ * whatever maximum value defined via board's device-tree.
+ */
+ tegra->core_min_uV = core_max_uV;
+
+ pr_info("core minimum voltage limited to %duV\n", tegra->core_min_uV);
+
+ return tegra->core_min_uV;
+}
+
+static int tegra20_core_rtc_max_spread(struct regulator_dev *core_rdev,
+ struct regulator_dev *rtc_rdev)
+{
+ struct coupling_desc *c_desc = &core_rdev->coupling_desc;
+ struct regulator_dev *rdev;
+ int max_spread;
+ unsigned int i;
+
+ for (i = 1; i < c_desc->n_coupled; i++) {
+ max_spread = core_rdev->constraints->max_spread[i - 1];
+ rdev = c_desc->coupled_rdevs[i];
+
+ if (rdev == rtc_rdev && max_spread)
+ return max_spread;
+ }
+
+ pr_err_once("rtc-core max-spread is undefined in device-tree\n");
+
+ return 150000;
+}
+
+static int tegra20_core_rtc_update(struct tegra_regulator_coupler *tegra,
+ struct regulator_dev *core_rdev,
+ struct regulator_dev *rtc_rdev,
+ int cpu_uV, int cpu_min_uV)
+{
+ int core_min_uV, core_max_uV = INT_MAX;
+ int rtc_min_uV, rtc_max_uV = INT_MAX;
+ int core_target_uV;
+ int rtc_target_uV;
+ int max_spread;
+ int core_uV;
+ int rtc_uV;
+ int err;
+
+ /*
+ * RTC and CORE voltages should be no more than 170mV from each other,
+ * CPU should be below RTC and CORE by at least 120mV. This applies
+ * to all Tegra20 SoC's.
+ */
+ max_spread = tegra20_core_rtc_max_spread(core_rdev, rtc_rdev);
+
+ /*
+ * The core voltage scaling is currently not hooked up in drivers,
+ * hence we will limit the minimum core voltage to a reasonable value.
+ * This should be good enough for the time being.
+ */
+ core_min_uV = tegra20_core_limit(tegra, core_rdev);
+ if (core_min_uV < 0)
+ return core_min_uV;
+
+ err = regulator_check_voltage(core_rdev, &core_min_uV, &core_max_uV);
+ if (err)
+ return err;
+
+ err = regulator_check_consumers(core_rdev, &core_min_uV, &core_max_uV,
+ PM_SUSPEND_ON);
+ if (err)
+ return err;
+
+ core_min_uV = max(cpu_min_uV + 125000, core_min_uV);
+ if (core_min_uV > core_max_uV)
+ return -EINVAL;
+
+ core_uV = regulator_get_voltage_rdev(core_rdev);
+ if (core_uV < 0)
+ return core_uV;
+
+ if (cpu_uV + 120000 > core_uV)
+ pr_err("core-cpu voltage constraint violated: %d %d\n",
+ core_uV, cpu_uV + 120000);
+
+ rtc_uV = regulator_get_voltage_rdev(rtc_rdev);
+ if (rtc_uV < 0)
+ return rtc_uV;
+
+ if (cpu_uV + 120000 > rtc_uV)
+ pr_err("rtc-cpu voltage constraint violated: %d %d\n",
+ rtc_uV, cpu_uV + 120000);
+
+ if (abs(core_uV - rtc_uV) > 170000)
+ pr_err("core-rtc voltage constraint violated: %d %d\n",
+ core_uV, rtc_uV);
+
+ rtc_min_uV = max(cpu_min_uV + 125000, core_min_uV - max_spread);
+
+ err = regulator_check_voltage(rtc_rdev, &rtc_min_uV, &rtc_max_uV);
+ if (err)
+ return err;
+
+ while (core_uV != core_min_uV || rtc_uV != rtc_min_uV) {
+ if (core_uV < core_min_uV) {
+ core_target_uV = min(core_uV + max_spread, core_min_uV);
+ core_target_uV = min(rtc_uV + max_spread, core_target_uV);
+ } else {
+ core_target_uV = max(core_uV - max_spread, core_min_uV);
+ core_target_uV = max(rtc_uV - max_spread, core_target_uV);
+ }
+
+ err = regulator_set_voltage_rdev(core_rdev,
+ core_target_uV,
+ core_max_uV,
+ PM_SUSPEND_ON);
+ if (err)
+ return err;
+
+ core_uV = core_target_uV;
+
+ if (rtc_uV < rtc_min_uV) {
+ rtc_target_uV = min(rtc_uV + max_spread, rtc_min_uV);
+ rtc_target_uV = min(core_uV + max_spread, rtc_target_uV);
+ } else {
+ rtc_target_uV = max(rtc_uV - max_spread, rtc_min_uV);
+ rtc_target_uV = max(core_uV - max_spread, rtc_target_uV);
+ }
+
+ err = regulator_set_voltage_rdev(rtc_rdev,
+ rtc_target_uV,
+ rtc_max_uV,
+ PM_SUSPEND_ON);
+ if (err)
+ return err;
+
+ rtc_uV = rtc_target_uV;
+ }
+
+ return 0;
+}
+
+static int tegra20_core_voltage_update(struct tegra_regulator_coupler *tegra,
+ struct regulator_dev *cpu_rdev,
+ struct regulator_dev *core_rdev,
+ struct regulator_dev *rtc_rdev)
+{
+ int cpu_uV;
+
+ cpu_uV = regulator_get_voltage_rdev(cpu_rdev);
+ if (cpu_uV < 0)
+ return cpu_uV;
+
+ return tegra20_core_rtc_update(tegra, core_rdev, rtc_rdev,
+ cpu_uV, cpu_uV);
+}
+
+static int tegra20_cpu_voltage_update(struct tegra_regulator_coupler *tegra,
+ struct regulator_dev *cpu_rdev,
+ struct regulator_dev *core_rdev,
+ struct regulator_dev *rtc_rdev)
+{
+ int cpu_max_uV = INT_MAX;
+ int cpu_min_uV = 0;
+ int cpu_uV;
+ int err;
+
+ err = regulator_check_voltage(cpu_rdev, &cpu_min_uV, &cpu_max_uV);
+ if (err)
+ return err;
+
+ err = regulator_check_consumers(cpu_rdev, &cpu_min_uV, &cpu_max_uV,
+ PM_SUSPEND_ON);
+ if (err)
+ return err;
+
+ cpu_uV = regulator_get_voltage_rdev(cpu_rdev);
+ if (cpu_uV < 0)
+ return cpu_uV;
+
+ if (cpu_min_uV > cpu_uV) {
+ err = tegra20_core_rtc_update(tegra, core_rdev, rtc_rdev,
+ cpu_uV, cpu_min_uV);
+ if (err)
+ return err;
+
+ err = regulator_set_voltage_rdev(cpu_rdev, cpu_min_uV,
+ cpu_max_uV, PM_SUSPEND_ON);
+ if (err)
+ return err;
+ } else if (cpu_min_uV < cpu_uV) {
+ err = regulator_set_voltage_rdev(cpu_rdev, cpu_min_uV,
+ cpu_max_uV, PM_SUSPEND_ON);
+ if (err)
+ return err;
+
+ err = tegra20_core_rtc_update(tegra, core_rdev, rtc_rdev,
+ cpu_uV, cpu_min_uV);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int tegra20_regulator_balance_voltage(struct regulator_coupler *coupler,
+ struct regulator_dev *rdev,
+ suspend_state_t state)
+{
+ struct tegra_regulator_coupler *tegra = to_tegra_coupler(coupler);
+ struct regulator_dev *core_rdev = tegra->core_rdev;
+ struct regulator_dev *cpu_rdev = tegra->cpu_rdev;
+ struct regulator_dev *rtc_rdev = tegra->rtc_rdev;
+
+ if ((core_rdev != rdev && cpu_rdev != rdev && rtc_rdev != rdev) ||
+ state != PM_SUSPEND_ON) {
+ pr_err("regulators are not coupled properly\n");
+ return -EINVAL;
+ }
+
+ if (rdev == cpu_rdev)
+ return tegra20_cpu_voltage_update(tegra, cpu_rdev,
+ core_rdev, rtc_rdev);
+
+ if (rdev == core_rdev)
+ return tegra20_core_voltage_update(tegra, cpu_rdev,
+ core_rdev, rtc_rdev);
+
+ pr_err("changing %s voltage not permitted\n", rdev_get_name(rtc_rdev));
+
+ return -EPERM;
+}
+
+static int tegra20_regulator_attach(struct regulator_coupler *coupler,
+ struct regulator_dev *rdev)
+{
+ struct tegra_regulator_coupler *tegra = to_tegra_coupler(coupler);
+ struct device_node *np = rdev->dev.of_node;
+
+ if (of_property_read_bool(np, "nvidia,tegra-core-regulator") &&
+ !tegra->core_rdev) {
+ tegra->core_rdev = rdev;
+ return 0;
+ }
+
+ if (of_property_read_bool(np, "nvidia,tegra-rtc-regulator") &&
+ !tegra->rtc_rdev) {
+ tegra->rtc_rdev = rdev;
+ return 0;
+ }
+
+ if (of_property_read_bool(np, "nvidia,tegra-cpu-regulator") &&
+ !tegra->cpu_rdev) {
+ tegra->cpu_rdev = rdev;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int tegra20_regulator_detach(struct regulator_coupler *coupler,
+ struct regulator_dev *rdev)
+{
+ struct tegra_regulator_coupler *tegra = to_tegra_coupler(coupler);
+
+ if (tegra->core_rdev == rdev) {
+ tegra->core_rdev = NULL;
+ return 0;
+ }
+
+ if (tegra->rtc_rdev == rdev) {
+ tegra->rtc_rdev = NULL;
+ return 0;
+ }
+
+ if (tegra->cpu_rdev == rdev) {
+ tegra->cpu_rdev = NULL;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static struct tegra_regulator_coupler tegra20_coupler = {
+ .coupler = {
+ .attach_regulator = tegra20_regulator_attach,
+ .detach_regulator = tegra20_regulator_detach,
+ .balance_voltage = tegra20_regulator_balance_voltage,
+ },
+};
+
+static int __init tegra_regulator_coupler_init(void)
+{
+ if (!of_machine_is_compatible("nvidia,tegra20"))
+ return 0;
+
+ return regulator_coupler_register(&tegra20_coupler.coupler);
+}
+arch_initcall(tegra_regulator_coupler_init);
--
2.22.0
^ permalink raw reply related
* [PATCH v1 1/3] dt-bindings: regulator: Document regulators coupling of NVIDIA Tegra20/30 SoCs
From: Dmitry Osipenko @ 2019-07-15 19:45 UTC (permalink / raw)
To: Rob Herring, Thierry Reding, Peter De Schrijver, Jonathan Hunter,
Mark Brown
Cc: devicetree, linux-tegra, linux-kernel
In-Reply-To: <20190715194503.19100-1-digetx@gmail.com>
There is voltage coupling between three regulators on Tegra20 boards and
between two on Tegra30. The voltage coupling is a SoC-level feature and
thus it is mandatory and common for all of the Tegra boards.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
.../nvidia,tegra-regulators-coupling.txt | 65 +++++++++++++++++++
1 file changed, 65 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
diff --git a/Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt b/Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
new file mode 100644
index 000000000000..4bf2dbf7c6cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
@@ -0,0 +1,65 @@
+NVIDIA Tegra Regulators Coupling
+================================
+
+NVIDIA Tegra SoC's have a mandatory voltage-coupling between regulators.
+Thus on Tegra20 there are 3 coupled regulators and on NVIDIA Tegra30
+there are 2.
+
+Tegra20 voltage coupling
+------------------------
+
+On Tegra20 SoC's there are 3 coupled regulators: CORE, RTC and CPU.
+The CORE and RTC voltages shall be in a range of 170mV from each other
+and they both shall be higher than the CPU voltage by at least 120mV.
+
+Tegra30 voltage coupling
+------------------------
+
+On Tegra30 SoC's there are 2 coupled regulators: CORE and CPU. The CORE
+and CPU voltages shall be in a range of 300mV from each other and CORE
+voltage shall be higher than the CPU by N mV, where N depends on the CPU
+voltage.
+
+Required properties:
+- nvidia,tegra-core-regulator: Boolean property that designates regulator
+ as the "Core domain" voltage regulator.
+- nvidia,tegra-rtc-regulator: Boolean property that designates regulator
+ as the "RTC domain" voltage regulator.
+- nvidia,tegra-cpu-regulator: Boolean property that designates regulator
+ as the "CPU domain" voltage regulator.
+
+Example:
+
+ pmic {
+ regulators {
+ core_vdd_reg: core {
+ regulator-name = "vdd_core";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-coupled-with = <&rtc_vdd_reg &cpu_vdd_reg>;
+ regulator-coupled-max-spread = <170000 550000>;
+
+ nvidia,tegra-core-regulator;
+ };
+
+ rtc_vdd_reg: rtc {
+ regulator-name = "vdd_rtc";
+ regulator-min-microvolt = <950000>;
+ regulator-max-microvolt = <1300000>;
+ regulator-coupled-with = <&core_vdd_reg &cpu_vdd_reg>;
+ regulator-coupled-max-spread = <170000 550000>;
+
+ nvidia,tegra-rtc-regulator;
+ };
+
+ cpu_vdd_reg: cpu {
+ regulator-name = "vdd_cpu";
+ regulator-min-microvolt = <750000>;
+ regulator-max-microvolt = <1125000>;
+ regulator-coupled-with = <&core_vdd_reg &rtc_vdd_reg>;
+ regulator-coupled-max-spread = <550000 550000>;
+
+ nvidia,tegra-cpu-regulator;
+ };
+ };
+ };
--
2.22.0
^ permalink raw reply related
* [PATCH v1 0/3] Support regulators coupling on NVIDIA Tegra20/30
From: Dmitry Osipenko @ 2019-07-15 19:45 UTC (permalink / raw)
To: Rob Herring, Thierry Reding, Peter De Schrijver, Jonathan Hunter,
Mark Brown
Cc: devicetree, linux-tegra, linux-kernel
Hello,
The voltage regulators need to be coupled on NVIDIA Tegra20 and Tegra30
SoCs in order to provide voltage scaling functionality in a generic way.
All necessary regulator-core patches that added support for the regulators
coupling are already have been merge into mainline kernel. This series
adds customized voltage couplers for Tegra20/30 SoCs, paving the way for
a refined CPUFreq driver that will utilize voltage scaling and other neat
features. This is a resend of a leftover patches from a previous series
[1] that was partially applied by Mark Brown. Please review, thanks in
advance!
[1] https://patchwork.ozlabs.org/project/linux-tegra/list/?series=115626
Dmitry Osipenko (3):
dt-bindings: regulator: Document regulators coupling of NVIDIA
Tegra20/30 SoCs
soc/tegra: regulators: Add regulators coupler for Tegra20
soc/tegra: regulators: Add regulators coupler for Tegra30
.../nvidia,tegra-regulators-coupling.txt | 65 ++++
drivers/soc/tegra/Kconfig | 10 +
drivers/soc/tegra/Makefile | 2 +
drivers/soc/tegra/regulators-tegra20.c | 350 ++++++++++++++++++
drivers/soc/tegra/regulators-tegra30.c | 302 +++++++++++++++
5 files changed, 729 insertions(+)
create mode 100644 Documentation/devicetree/bindings/regulator/nvidia,tegra-regulators-coupling.txt
create mode 100644 drivers/soc/tegra/regulators-tegra20.c
create mode 100644 drivers/soc/tegra/regulators-tegra30.c
--
2.22.0
^ permalink raw reply
* Re: [PATCH v4 0/7] Allwinner H6 SPDIF support
From: Mark Brown @ 2019-07-15 19:38 UTC (permalink / raw)
To: Clément Péron
Cc: Liam Girdwood, Jaroslav Kysela, Takashi Iwai, Linux-ALSA,
devicetree, linux-arm-kernel, Mark Rutland, Rob Herring,
Maxime Ripard, Chen-Yu Tsai, linux-kernel, linux-sunxi,
Jagan Teki
In-Reply-To: <CAJiuCcc3_1jZWV7G3+fFQYRZ8b6qcAbnH+K6pkRvww6_D=OMAw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1411 bytes --]
On Mon, Jul 15, 2019 at 09:21:01PM +0200, Clément Péron wrote:
> Hi,
>
> I'm missing ACK from ASoC Maintainers patch 2-3-4.
>
> It's really small paches, if you could have a look at it.
Please don't send content free pings and please allow a reasonable time
for review. People get busy, go on holiday, attend conferences and so
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review. If there have been
review comments then people may be waiting for those to be addressed.
Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.
--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20190715193842.GC4503%40sirena.org.uk.
For more options, visit https://groups.google.com/d/optout.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 0/7] Allwinner H6 SPDIF support
From: Clément Péron @ 2019-07-15 19:21 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Jaroslav Kysela, Takashi Iwai
Cc: Linux-ALSA, devicetree, linux-arm-kernel, Mark Rutland,
Rob Herring, Maxime Ripard, Chen-Yu Tsai, linux-kernel,
linux-sunxi, Jagan Teki
In-Reply-To: <CAJiuCcfUhBxEr=o7VVpPROQZadQh7z1QC0SkWSYt-53Sj3H2qw@mail.gmail.com>
Hi,
I'm missing ACK from ASoC Maintainers patch 2-3-4.
It's really small paches, if you could have a look at it.
Many thanks,
Clément
On Fri, 14 Jun 2019 at 10:29, Clément Péron <peron.clem@gmail.com> wrote:
>
> Hi,
>
> On Mon, 27 May 2019 at 22:10, Clément Péron <peron.clem@gmail.com> wrote:
> >
> > *H6 DMA support IS REQUIRED*
>
> DMA has been merged, so this series can be merge when ASoC maintainers
> have reviewed it.
>
> Regards,
> Clément
>
> >
> > Allwinner H6 SoC has a SPDIF controller called One Wire Audio (OWA) which
> > is different from the previous H3 generation and not compatible.
> >
> > Difference are an increase of fifo sizes, some memory mapping are different
> > and there is now the possibility to output the master clock on a pin.
> >
> > Actually all these features are unused and only a bit for flushing the TX
> > fifo is required.
> >
> > Also this series requires the DMA working on H6, a first version has been
> > submitted by Jernej Škrabec[1] but has not been accepted yet.
> >
> > [1] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=89011
> >
> > Changes since v3:
> > - rename reg_fctl_ftx to val_fctl_ftx
> > - rebase this series on sound-next
> > - fix dt-bindings due to change in sound-next
> > - change node name sound_spdif to sound-spdif
> >
> > Changes since v2:
> > - Split quirks and H6 support patch
> > - Add specific section for quirks comment
> >
> > Changes since v1:
> > - Remove H3 compatible
> > - Add TX fifo bit flush quirks
> > - Add H6 bindings in SPDIF driver
> >
> > Clément Péron (7):
> > dt-bindings: sound: sun4i-spdif: Add Allwinner H6 compatible
> > ASoC: sun4i-spdif: Move quirks to the top
> > ASoC: sun4i-spdif: Add TX fifo bit flush quirks
> > ASoC: sun4i-spdif: Add support for H6 SoC
> > arm64: dts: allwinner: Add SPDIF node for Allwinner H6
> > arm64: dts: allwinner: h6: Enable SPDIF for Beelink GS1
> > arm64: defconfig: Enable Sun4i SPDIF module
> >
> > .../sound/allwinner,sun4i-a10-spdif.yaml | 1 +
> > .../dts/allwinner/sun50i-h6-beelink-gs1.dts | 4 ++
> > arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 38 ++++++++++++++
> > arch/arm64/configs/defconfig | 1 +
> > sound/soc/sunxi/sun4i-spdif.c | 49 ++++++++++++++++---
> > 5 files changed, 87 insertions(+), 6 deletions(-)
> >
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core
From: Dan Murphy @ 2019-07-15 18:59 UTC (permalink / raw)
To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt,
mark.rutland, daniel.thompson
Cc: linux-leds, linux-kernel, devicetree
In-Reply-To: <20190715155657.22976-3-jjhiblot@ti.com>
JJ
On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> A LED is usually powered by a voltage/current regulator. Let the LED core
> know about it. This allows the LED core to turn on or off the power supply
> as needed.
This allows the LED core to turn on or off managed power supplies.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
> drivers/leds/led-class.c | 15 ++++++++++++
> drivers/leds/led-core.c | 50 +++++++++++++++++++++++++++++++++++++---
> drivers/leds/leds.h | 1 +
> include/linux/leds.h | 4 ++++
> 4 files changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
> index 4793e77808e2..cadd43c30d50 100644
> --- a/drivers/leds/led-class.c
> +++ b/drivers/leds/led-class.c
> @@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
> {
> char name[LED_MAX_NAME_SIZE];
> int ret;
> + struct regulator *regulator;
>
> ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
> if (ret < 0)
> @@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
> dev_warn(parent, "Led %s renamed to %s due to name collision",
> led_cdev->name, dev_name(led_cdev->dev));
>
> + regulator = devm_regulator_get_optional(led_cdev->dev, "power");
Store the regulator in led_cdev->regulator and drop the else case below.
> + if (IS_ERR(regulator)) {
> + if (PTR_ERR(regulator) != -ENODEV) {
> + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> + led_cdev->name);
> + device_unregister(led_cdev->dev);
> + mutex_unlock(&led_cdev->led_access);
> + return PTR_ERR(regulator);
> + }
> + led_cdev->regulator = NULL;
> + } else {
> + led_cdev->regulator = regulator;
> + }
> +
> if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
> ret = led_add_brightness_hw_changed(led_cdev);
> if (ret) {
> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
> index 7107cd7e87cf..a12b880b0a2f 100644
> --- a/drivers/leds/led-core.c
> +++ b/drivers/leds/led-core.c
> @@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
> LIST_HEAD(leds_list);
> EXPORT_SYMBOL_GPL(leds_list);
>
> +static bool __led_need_regulator_update(struct led_classdev *led_cdev,
> + int brightness)
> +{
> + bool new_state = (brightness != LED_OFF);
> +
> + return led_cdev->regulator && led_cdev->regulator_state != new_state;
> +}
> +
> +static int __led_handle_regulator(struct led_classdev *led_cdev,
> + int brightness)
> +{
> + int rc;
Should there be a check for the regulator pointer.
If (!led_cdev->regulator)
return 0;
Otherwise
Reviewed-by: Dan Murphy <dmurphy@ti.com>
<snip>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property
From: Dan Murphy @ 2019-07-15 18:52 UTC (permalink / raw)
To: Jean-Jacques Hiblot, jacek.anaszewski, pavel, robh+dt,
mark.rutland, daniel.thompson
Cc: linux-leds, linux-kernel, devicetree
In-Reply-To: <20190715155657.22976-2-jjhiblot@ti.com>
JJ
On 7/15/19 10:56 AM, Jean-Jacques Hiblot wrote:
> Most of the LEDs are powered by a voltage/current regulator. Describing it
> in the device-tree makes it possible for the LED core to enable/disable it
> when needed.
>
> Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
> ---
> Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
> index 70876ac11367..539e124b1457 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -61,6 +61,11 @@ Optional properties for child nodes:
> - panic-indicator : This property specifies that the LED should be used,
> if at all possible, as a panic indicator.
>
> +- power-supply : A voltage/current regulator used to to power the LED. When a
Is the phandle to a voltage/current regulator used to to power the LED
> + LED is turned off, the LED core disable its regulator. The
The regulator is only disabled if it is the only consumer and/or the
number of users = 0.
> + same regulator can power many LED (or other) devices. It is
> + turned off only when all of its users disabled it.
> +
> - trigger-sources : List of devices which should be used as a source triggering
> this LED activity. Some LEDs can be related to a specific
> device and should somehow indicate its state. E.g. USB 2.0
> @@ -106,6 +111,7 @@ gpio-leds {
> label = "Status";
> linux,default-trigger = "heartbeat";
> gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
> + power-supply = <&led_regulator>;
> };
>
> usb {
Reviewed-by: Dan Murphy <dmurphy@ti.com>
^ permalink raw reply
* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-07-15 18:40 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team
In-Reply-To: <07812739-0e6b-6598-ac58-8e0ea74a3331@gmail.com>
Replying again because the previous email accidentally included HTML.
Thanks for taking the time to reconsider the wording Frank. Your
intention was clear to me in the first email too.
A kernel command line option can also completely disable this
functionality easily and cleanly. Can we pick that as an option? I've
an implementation of that in the v5 series I sent out last week.
-Saravana
On Mon, Jul 15, 2019 at 7:39 AM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 7/15/19 7:26 AM, Frank Rowand wrote:
> > HiRob,
> >
> > Sorry for such a late reply...
> >
> >
> > On 7/1/19 8:25 PM, Saravana Kannan wrote:
> >> On Mon, Jul 1, 2019 at 6:32 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>>
> >>> On Mon, Jul 1, 2019 at 6:48 PM Saravana Kannan <saravanak@google.com> wrote:
> >>>>
> >>>> Add device-links after the devices are created (but before they are
> >>>> probed) by looking at common DT bindings like clocks and
> >>>> interconnects.
> >>>>
> >>>> Automatically adding device-links for functional dependencies at the
> >>>> framework level provides the following benefits:
> >>>>
> >>>> - Optimizes device probe order and avoids the useless work of
> >>>> attempting probes of devices that will not probe successfully
> >>>> (because their suppliers aren't present or haven't probed yet).
> >>>>
> >>>> For example, in a commonly available mobile SoC, registering just
> >>>> one consumer device's driver at an initcall level earlier than the
> >>>> supplier device's driver causes 11 failed probe attempts before the
> >>>> consumer device probes successfully. This was with a kernel with all
> >>>> the drivers statically compiled in. This problem gets a lot worse if
> >>>> all the drivers are loaded as modules without direct symbol
> >>>> dependencies.
> >>>>
> >>>> - Supplier devices like clock providers, interconnect providers, etc
> >>>> need to keep the resources they provide active and at a particular
> >>>> state(s) during boot up even if their current set of consumers don't
> >>>> request the resource to be active. This is because the rest of the
> >>>> consumers might not have probed yet and turning off the resource
> >>>> before all the consumers have probed could lead to a hang or
> >>>> undesired user experience.
> >>>>
> >>>> Some frameworks (Eg: regulator) handle this today by turning off
> >>>> "unused" resources at late_initcall_sync and hoping all the devices
> >>>> have probed by then. This is not a valid assumption for systems with
> >>>> loadable modules. Other frameworks (Eg: clock) just don't handle
> >>>> this due to the lack of a clear signal for when they can turn off
> >>>> resources. This leads to downstream hacks to handle cases like this
> >>>> that can easily be solved in the upstream kernel.
> >>>>
> >>>> By linking devices before they are probed, we give suppliers a clear
> >>>> count of the number of dependent consumers. Once all of the
> >>>> consumers are active, the suppliers can turn off the unused
> >>>> resources without making assumptions about the number of consumers.
> >>>>
> >>>> By default we just add device-links to track "driver presence" (probe
> >>>> succeeded) of the supplier device. If any other functionality provided
> >>>> by device-links are needed, it is left to the consumer/supplier
> >>>> devices to change the link when they probe.
> >>>>
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>> ---
> >>>> drivers/of/Kconfig | 9 ++++++++
> >>>> drivers/of/platform.c | 52 +++++++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 61 insertions(+)
> >>>>
> >>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >>>> index 37c2ccbefecd..7c7fa7394b4c 100644
> >>>> --- a/drivers/of/Kconfig
> >>>> +++ b/drivers/of/Kconfig
> >>>> @@ -103,4 +103,13 @@ config OF_OVERLAY
> >>>> config OF_NUMA
> >>>> bool
> >>>>
> >>>> +config OF_DEVLINKS
> >>>
> >>> I'd prefer this not be a config option. After all, we want one kernel
> >>> build that works for all platforms.
> >>
> >> We need a lot more changes before one kernel build can work for all
> >> platforms. At least until then, I think we need this. Lot less chance
> >> of breaking existing platforms before all the missing pieces are
> >> created.
> >>
> >>> A kernel command line option to disable might be useful for debugging.
> >>
> >> Or we can have a command line to enable this for platforms that want
> >> to use it and have it default off.
> >
>
> > Given the fragility of the current boot sequence (without this patch set)
> > and the potential breakage of existing systems, I think that if we choose
> > to accept this patch set that it should first bake in the -next tree for
> > at least one major release cycle. Maybe even two major release cycles.
>
> I probably didn't state that very well. I was trying to not sound like
> I was accusing this patch series of being fragile. The issue is that
> adding the patches to systems that weren't expecting the new ordering
> may cause boot problems for some systems. I'm not concerned with
> pointing fingers, just want to make sure that we proceed cautiously
> until we know that the resulting system is robust.
>
> -Frank
>
> >
> > -Frank
> >
> >
> >>
> >>>> + bool "Device links from DT bindings"
> >>>> + help
> >>>> + Common DT bindings like clocks, interconnects, etc represent a
> >>>> + consumer device's dependency on suppliers devices. This option
> >>>> + creates device links from these common bindings so that consumers are
> >>>> + probed only after all their suppliers are active and suppliers can
> >>>> + tell when all their consumers are active.
> >>>> +
> >>>> endif # OF
> >>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >>>> index 04ad312fd85b..a53717168aca 100644
> >>>> --- a/drivers/of/platform.c
> >>>> +++ b/drivers/of/platform.c
> >>>> @@ -61,6 +61,57 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
> >>>> EXPORT_SYMBOL(of_find_device_by_node);
> >>>>
> >>>> #ifdef CONFIG_OF_ADDRESS
> >>>> +static int of_link_binding(struct device *dev, char *binding, char *cell)
> >>>
> >>> Under CONFIG_OF_ADDRESS seems like a strange location.
> >>
> >> Yeah, but the rest of the file seems to be under this. So I'm not
> >> touching that. I can probably move this function further down (close
> >> to platform populate) if you want that.
> >>>
> >>>> +{
> >>>> + struct of_phandle_args sup_args;
> >>>> + struct platform_device *sup_dev;
> >>>> + unsigned int i = 0, links = 0;
> >>>> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> >>>> +
> >>>> + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
> >>>> + &sup_args)) {
> >>>> + i++;
> >>>> + sup_dev = of_find_device_by_node(sup_args.np);
> >>>> + if (!sup_dev)
> >>>> + continue;
> >>>> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
> >>>> + links++;
> >>>> + put_device(&sup_dev->dev);
> >>>> + }
> >>>> + if (links < i)
> >>>> + return -ENODEV;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * List of bindings and their cell names (use NULL if no cell names) from which
> >>>> + * device links need to be created.
> >>>> + */
> >>>> +static char *link_bindings[] = {
> >>>
> >>> const
> >>
> >> Ack
> >>
> >>>
> >>>> +#ifdef CONFIG_OF_DEVLINKS
> >>>> + "clocks", "#clock-cells",
> >>>> + "interconnects", "#interconnect-cells",
> >>>
> >>> Planning to add others?
> >>
> >> Not in this patch.
> >>
> >> Regulators are the other big missing piece that I'm aware of now but
> >> they need a lot of discussion (see email from David and my reply).
> >>
> >> Not sure what other resources are shared where they can be "turned
> >> off" and cause devices set up at boot to fail. For example, I don't
> >> think interrupts need functional dependency tracking because they
> >> aren't really turned off by consumer 1 in a way that breaks things for
> >> consumer 2. Just masked and the consumer 2 can unmask and use it once
> >> it probes.
> >>
> >> I'm only intimately familiar with clocks, interconnects and regulators
> >> (to some extent). I'm open to adding other supplier categories in
> >> future patches as I educate myself of those or if other people want to
> >> add support for more categories.
> >>
> >> -Saravana
> >>
> >>>> +#endif
> >>>> +};
> >>>> +
> >>>> +static int of_link_to_suppliers(struct device *dev)
> >>>> +{
> >>>> + unsigned int i = 0;
> >>>> + bool done = true;
> >>>> +
> >>>> + if (unlikely(!dev->of_node))
> >>>> + return 0;
> >>>> +
> >>>> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> >>>> + if (of_link_binding(dev, link_bindings[i * 2],
> >>>> + link_bindings[i * 2 + 1]))
> >>>> + done = false;
> >>>> +
> >>>> + if (!done)
> >>>> + return -ENODEV;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> /*
> >>>> * The following routines scan a subtree and registers a device for
> >>>> * each applicable node.
> >>>> @@ -524,6 +575,7 @@ static int __init of_platform_default_populate_init(void)
> >>>> if (!of_have_populated_dt())
> >>>> return -ENODEV;
> >>>>
> >>>> + platform_bus_type.add_links = of_link_to_suppliers;
> >>>> /*
> >>>> * Handle certain compatibles explicitly, since we don't want to create
> >>>> * platform_devices for every node in /reserved-memory with a
> >>>> --
> >>>> 2.22.0.410.gd8fdbe21b5-goog
> >>>>
> >>
> >
> >
>
^ permalink raw reply
* Re: [PATCH v4 7/8] dt-bindings: arm: sunxi: add binding for Lichee Zero Plus core board
From: Rob Herring @ 2019-07-15 17:03 UTC (permalink / raw)
To: Icenowy Zheng
Cc: Maxime Ripard, Chen-Yu Tsai, Linus Walleij,
devicetree-u79uwXL29TY76Z2rM5mHXA,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk,
open list:GPIO SUBSYSTEM, linux-sunxi
In-Reply-To: <20190713034634.44585-8-icenowy-h8G6r0blFSE@public.gmane.org>
On Fri, Jul 12, 2019 at 9:49 PM Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org> wrote:
>
> The Lichee Zero Plus is a core board made by Sipeed, with a microUSB
> connector on it, TF slot or WSON8 SD chip, optional eMMC or SPI Flash.
> It has a gold finger connector for expansion, and UART is available from
> reserved pins w/ 2.54mm pitch. The board can use either SoChip S3 or
> Allwinner V3L SoCs.
>
> Add the device tree binding of the basic version of the core board --
> w/o eMMC or SPI Flash, w/ TF slot or WSON8 SD, and use S3 SoC.
>
> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
> ---
> No changes since v3.
>
> Patch introduced in v2.
>
> Documentation/devicetree/bindings/arm/sunxi.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Rob
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Rob Herring @ 2019-07-15 17:03 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, jassisinghbrar@gmail.com,
sudeep.holla@arm.com, andre.przywara@arm.com,
f.fainelli@gmail.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dl-linux-imx
In-Reply-To: <1563184103-8493-2-git-send-email-peng.fan@nxp.com>
On Mon, Jul 15, 2019 at 4:10 AM Peng Fan <peng.fan@nxp.com> wrote:
>
> From: Peng Fan <peng.fan@nxp.com>
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>
> V3:
> Convert to yaml
> Drop interrupt
> Introudce transports to indicate mem/reg
> The func id is still kept as optional, because like SCMI it only
> cares about message.
>
> V2:
> Introduce interrupts as a property.
>
> .../devicetree/bindings/mailbox/arm-smc.yaml | 124 +++++++++++++++++++++
> 1 file changed, 124 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..da9b1a03bc4e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,124 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan <peng.fan@nxp.com>
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. By nature this operation
> + is synchronous and this mailbox provides no way for asynchronous messages
> + to be delivered the other way round, from firmware to the OS, but
> + asynchronous notification could also be supported. However the value of
> + r0/w0/x0 the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so a synchronous communication can be
> + established, for a asynchronous notification, no value will be returned.
> + The exact meaning of both the action the mailbox triggers as well as the
> + return value is defined by their users and is not subject to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared memory
> + to transfer commands and parameters, and a mailbox to trigger a function
> + call. This allows SoCs without a separate management processor (or when
> + such a processor is not available or used) to use this standardized
> + interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers, or listed in the the arm,func-ids
> + properties as described below. The firmware can return one value in
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long as
> + a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> + const: arm,smc-mbox
> +
> + "#mbox-cells":
> + const: 1
> +
> + arm,num-chans:
> + description: The number of channels supported.
> + $ref: /schemas/types.yaml#/definitions/uint32
Constraints? 0 is valid? 2^32?
> +
> + method:
> + items:
> + - enum:
> + - smc
> + - hvc
> +
> + transports:
> + items:
> + - enum:
> + - mem
> + - reg
What if someone wants to configure this per channel? Perhaps
#mbox-cells should be 2 and this can be a client parameter.
Minimally, this needs a 'arm' vendor prefix if it stays.
> +
> + arm,func-ids:
> + description: |
> + An array of 32-bit values specifying the function IDs used by each
> + mailbox channel. Those function IDs follow the ARM SMC calling
> + convention standard [1].
What's the default if not specified? Or this should be required?
> +
> + There is one identifier per channel and the number of supported
> + channels is determined by the length of this array.
> + minItems: 0
> + maxItems: 4096 # Should be enough?
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> + - arm,num-chans
> + - transports
> + - method
> +
> +examples:
> + - |
> + sram@910000 {
> + compatible = "mmio-sram";
> + reg = <0x0 0x93f000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x0 0x93f000 0x1000>;
> +
> + cpu_scp_lpri: scp-shmem@0 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x0 0x200>;
> + };
> +
> + cpu_scp_hpri: scp-shmem@200 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x200 0x200>;
> + };
> + };
> +
> + firmware {
> + smc_mbox: mailbox {
> + #mbox-cells = <1>;
> + compatible = "arm,smc-mbox";
> + method = "smc";
> + arm,num-chans = <0x2>;
> + transports = "mem";
> + /* Optional */
> + arm,func-ids = <0xc20000fe>, <0xc20000ff>;
> + };
> +
> + scmi {
> + compatible = "arm,scmi";
> + mboxes = <&mailbox 0 &mailbox 1>;
> + mbox-names = "tx", "rx";
> + shmem = <&cpu_scp_lpri &cpu_scp_hpri>;
> + };
> + };
> +
> +...
> --
> 2.16.4
>
^ permalink raw reply
* Re: [PATCH V3 1/3] mmc: mmci: fix read status for busy detect
From: Ulf Hansson @ 2019-07-15 16:31 UTC (permalink / raw)
To: Ludovic Barre
Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin,
Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML,
linux-mmc@vger.kernel.org, linux-stm32
In-Reply-To: <1559577325-19266-2-git-send-email-ludovic.Barre@st.com>
On Mon, 3 Jun 2019 at 17:55, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> "busy_detect_flag" is used to read & clear busy value of mmci status.
> "busy_detect_mask" is used to manage busy irq of mmci mask.
> So to read mmci status the busy_detect_flag must be take account.
> if the variant does not support busy detect feature the flag is null
> and there is no impact.
By reading the changelog, it doesn't tell me the purpose of this
change. When going forward, please work harder on your changelogs.
Make sure to answer the questions, *why* is this change needed,
*what/how* does the change do.
>
> Not need to re-read the status register in mmci_cmd_irq, the
> status parameter can be used.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
> drivers/mmc/host/mmci.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 356833a..5b5cc45 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -1240,7 +1240,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> */
> if (!host->busy_status &&
> !(status & (MCI_CMDCRCFAIL|MCI_CMDTIMEOUT)) &&
> - (readl(base + MMCISTATUS) & host->variant->busy_detect_flag)) {
> + (status & host->variant->busy_detect_flag)) {
I suggested you to do this change through some of my earlier comments,
however I think it should be made as a stand alone change.
Anyway, when looking at the details in your series, I decided to try
to help out a bit, so I have prepared a couple of related patches for
cleaning up and clarifying the busy detection code/comments in mmci. I
have incorporated the above change, so let me post them asap.
>
> /* Clear the busy start IRQ */
> writel(host->variant->busy_detect_mask,
> @@ -1517,7 +1517,8 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> * to make sure that both start and end interrupts are always
> * cleared one after the other.
> */
> - status &= readl(host->base + MMCIMASK0);
> + status &= readl(host->base + MMCIMASK0) |
> + host->variant->busy_detect_flag;
As I told earlier in the review, this looks wrong to me.
It means that you will add the bit for the ->busy_detect_flag to the
status field we have just read from the MMCISTATUS register. That
means the busy status may be set when it shouldn't.
> if (host->variant->busy_detect)
> writel(status & ~host->variant->busy_detect_mask,
> host->base + MMCICLEAR);
> --
> 2.7.4
>
By looking at the other changes in the series, I assume @subject patch
is intended to prepare for the other changes on top. But it's not
really clear.
Anyway, in that regards, the below is my observations of what seems to
be important part, when supporting busy detection for the stm32 sdmmc
variant (except the timeout things in patch2, which I intend to
comment separately on).
I figured, these are the involved register bits/masks:
MMCISTATUS:
MCI_STM32_BUSYD0 BIT(20)
MCI_STM32_BUSYD0END BIT(21)
MMCIMASK0:
MCI_STM32_BUSYD0ENDMASK BIT(21)
For the legacy ST variant, there is only one register bit in
MMCISTATUS that is used for indicating busy (MCI_ST_CARDBUSY BIT(24)).
There is no dedicated busy-end bit for the busy-end IRQ, which I
believe is the reason to why the current code also is bit messy.
It seems like the stm32 sdmmc variant have a separate status bit for
the busy-end IRQ, correct?
If I understand correctly by looking at patch3, you don't use the
dedicated busy-end status bit (MCI_STM32_BUSYD0END), right? Then why
not?
Thoughts?
Kind regards
Uffe
^ permalink raw reply
* [PATCH v2 2/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-15 15:56 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
In-Reply-To: <20190715155657.22976-1-jjhiblot@ti.com>
A LED is usually powered by a voltage/current regulator. Let the LED core
know about it. This allows the LED core to turn on or off the power supply
as needed.
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
drivers/leds/led-class.c | 15 ++++++++++++
drivers/leds/led-core.c | 50 +++++++++++++++++++++++++++++++++++++---
drivers/leds/leds.h | 1 +
include/linux/leds.h | 4 ++++
4 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 4793e77808e2..cadd43c30d50 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -253,6 +253,7 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
{
char name[LED_MAX_NAME_SIZE];
int ret;
+ struct regulator *regulator;
ret = led_classdev_next_name(led_cdev->name, name, sizeof(name));
if (ret < 0)
@@ -272,6 +273,20 @@ int of_led_classdev_register(struct device *parent, struct device_node *np,
dev_warn(parent, "Led %s renamed to %s due to name collision",
led_cdev->name, dev_name(led_cdev->dev));
+ regulator = devm_regulator_get_optional(led_cdev->dev, "power");
+ if (IS_ERR(regulator)) {
+ if (PTR_ERR(regulator) != -ENODEV) {
+ dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
+ led_cdev->name);
+ device_unregister(led_cdev->dev);
+ mutex_unlock(&led_cdev->led_access);
+ return PTR_ERR(regulator);
+ }
+ led_cdev->regulator = NULL;
+ } else {
+ led_cdev->regulator = regulator;
+ }
+
if (led_cdev->flags & LED_BRIGHT_HW_CHANGED) {
ret = led_add_brightness_hw_changed(led_cdev);
if (ret) {
diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
index 7107cd7e87cf..a12b880b0a2f 100644
--- a/drivers/leds/led-core.c
+++ b/drivers/leds/led-core.c
@@ -23,6 +23,33 @@ EXPORT_SYMBOL_GPL(leds_list_lock);
LIST_HEAD(leds_list);
EXPORT_SYMBOL_GPL(leds_list);
+static bool __led_need_regulator_update(struct led_classdev *led_cdev,
+ int brightness)
+{
+ bool new_state = (brightness != LED_OFF);
+
+ return led_cdev->regulator && led_cdev->regulator_state != new_state;
+}
+
+static int __led_handle_regulator(struct led_classdev *led_cdev,
+ int brightness)
+{
+ int rc;
+
+ if (__led_need_regulator_update(led_cdev, brightness)) {
+
+ if (brightness != LED_OFF)
+ rc = regulator_enable(led_cdev->regulator);
+ else
+ rc = regulator_disable(led_cdev->regulator);
+ if (rc)
+ return rc;
+
+ led_cdev->regulator_state = (brightness != LED_OFF);
+ }
+ return 0;
+}
+
static int __led_set_brightness(struct led_classdev *led_cdev,
enum led_brightness value)
{
@@ -80,6 +107,7 @@ static void led_timer_function(struct timer_list *t)
}
led_set_brightness_nosleep(led_cdev, brightness);
+ __led_handle_regulator(led_cdev, brightness);
/* Return in next iteration if led is in one-shot mode and we are in
* the final blink state so that the led is toggled each delay_on +
@@ -115,6 +143,8 @@ static void set_brightness_delayed(struct work_struct *ws)
if (ret == -ENOTSUPP)
ret = __led_set_brightness_blocking(led_cdev,
led_cdev->delayed_set_value);
+ __led_handle_regulator(led_cdev, led_cdev->delayed_set_value);
+
if (ret < 0 &&
/* LED HW might have been unplugged, therefore don't warn */
!(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
@@ -141,6 +171,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
/* never on - just set to off */
if (!delay_on) {
led_set_brightness_nosleep(led_cdev, LED_OFF);
+ __led_handle_regulator(led_cdev, LED_OFF);
return;
}
@@ -148,6 +179,7 @@ static void led_set_software_blink(struct led_classdev *led_cdev,
if (!delay_off) {
led_set_brightness_nosleep(led_cdev,
led_cdev->blink_brightness);
+ __led_handle_regulator(led_cdev, led_cdev->blink_brightness);
return;
}
@@ -256,8 +288,14 @@ void led_set_brightness_nopm(struct led_classdev *led_cdev,
enum led_brightness value)
{
/* Use brightness_set op if available, it is guaranteed not to sleep */
- if (!__led_set_brightness(led_cdev, value))
- return;
+ if (!__led_set_brightness(led_cdev, value)) {
+ /*
+ * if regulator state doesn't need to be changed, that is all/
+ * Otherwise delegate the change to a work queue
+ */
+ if (!__led_need_regulator_update(led_cdev, value))
+ return;
+ }
/* If brightness setting can sleep, delegate it to a work queue task */
led_cdev->delayed_set_value = value;
@@ -280,6 +318,8 @@ EXPORT_SYMBOL_GPL(led_set_brightness_nosleep);
int led_set_brightness_sync(struct led_classdev *led_cdev,
enum led_brightness value)
{
+ int ret;
+
if (led_cdev->blink_delay_on || led_cdev->blink_delay_off)
return -EBUSY;
@@ -288,7 +328,11 @@ int led_set_brightness_sync(struct led_classdev *led_cdev,
if (led_cdev->flags & LED_SUSPENDED)
return 0;
- return __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ ret = __led_set_brightness_blocking(led_cdev, led_cdev->brightness);
+ if (ret)
+ return ret;
+
+ return __led_handle_regulator(led_cdev, led_cdev->brightness);
}
EXPORT_SYMBOL_GPL(led_set_brightness_sync);
diff --git a/drivers/leds/leds.h b/drivers/leds/leds.h
index 47b229469069..5aa5c038bd38 100644
--- a/drivers/leds/leds.h
+++ b/drivers/leds/leds.h
@@ -11,6 +11,7 @@
#include <linux/rwsem.h>
#include <linux/leds.h>
+#include <linux/regulator/consumer.h>
static inline int led_get_brightness(struct led_classdev *led_cdev)
{
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 9b2bf574a17a..bee8e3f8dddd 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -123,6 +123,10 @@ struct led_classdev {
/* Ensures consistent access to the LED Flash Class device */
struct mutex led_access;
+
+ /* regulator */
+ struct regulator *regulator;
+ bool regulator_state;
};
extern int of_led_classdev_register(struct device *parent,
--
2.17.1
^ permalink raw reply related
* [PATCH v2 1/2] dt-bindings: leds: document the "power-supply" property
From: Jean-Jacques Hiblot @ 2019-07-15 15:56 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
In-Reply-To: <20190715155657.22976-1-jjhiblot@ti.com>
Most of the LEDs are powered by a voltage/current regulator. Describing it
in the device-tree makes it possible for the LED core to enable/disable it
when needed.
Signed-off-by: Jean-Jacques Hiblot <jjhiblot@ti.com>
---
Documentation/devicetree/bindings/leds/common.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/leds/common.txt b/Documentation/devicetree/bindings/leds/common.txt
index 70876ac11367..539e124b1457 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -61,6 +61,11 @@ Optional properties for child nodes:
- panic-indicator : This property specifies that the LED should be used,
if at all possible, as a panic indicator.
+- power-supply : A voltage/current regulator used to to power the LED. When a
+ LED is turned off, the LED core disable its regulator. The
+ same regulator can power many LED (or other) devices. It is
+ turned off only when all of its users disabled it.
+
- trigger-sources : List of devices which should be used as a source triggering
this LED activity. Some LEDs can be related to a specific
device and should somehow indicate its state. E.g. USB 2.0
@@ -106,6 +111,7 @@ gpio-leds {
label = "Status";
linux,default-trigger = "heartbeat";
gpios = <&gpio0 0 GPIO_ACTIVE_HIGH>;
+ power-supply = <&led_regulator>;
};
usb {
--
2.17.1
^ permalink raw reply related
* [PATCH v2 0/2] leds: Add control of the voltage/current regulator to the LED core
From: Jean-Jacques Hiblot @ 2019-07-15 15:56 UTC (permalink / raw)
To: jacek.anaszewski, pavel, robh+dt, mark.rutland, daniel.thompson
Cc: dmurphy, linux-leds, linux-kernel, devicetree,
Jean-Jacques Hiblot
This series makes it possible for the LED core to manage the power supply
of a LED. It uses the regulator API to disable/enable the power if when the
LED is turned on/off.
This is especially useful in situations where the LED driver/controller is
not supplying the power.
changes in v2:
- use devm_regulator_get_optional() to avoid using the dummy regulator and do
some unnecessary work
Jean-Jacques Hiblot (2):
dt-bindings: leds: document the "power-supply" property
leds: Add control of the voltage/current regulator to the LED core
.../devicetree/bindings/leds/common.txt | 6 +++
drivers/leds/led-class.c | 15 ++++++
drivers/leds/led-core.c | 50 +++++++++++++++++--
drivers/leds/leds.h | 1 +
include/linux/leds.h | 4 ++
5 files changed, 73 insertions(+), 3 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH 1/2] leds: Add control of the voltage/current regulator to the LED core
From: Daniel Thompson @ 2019-07-15 15:19 UTC (permalink / raw)
To: Jean-Jacques Hiblot
Cc: Dan Murphy, jacek.anaszewski, pavel, robh+dt, mark.rutland,
linux-leds, linux-kernel, devicetree
In-Reply-To: <9c53f54f-d0d4-50d4-09da-34389085269a@ti.com>
On Mon, Jul 15, 2019 at 11:47:08AM +0200, Jean-Jacques Hiblot wrote:
> > > > > + if (IS_ERR(led_cdev->regulator)) {
> > > > > + dev_err(led_cdev->dev, "Cannot get the power supply for %s\n",
> > > > > + led_cdev->name);
> > > > > + device_unregister(led_cdev->dev);
> > > > > + mutex_unlock(&led_cdev->led_access);
> > > > > + return PTR_ERR(led_cdev->regulator);
> > > > This is listed as optional in the DT doc. This appears to be required.
> > > The regulator core will provide a dummy regulator if none is given in the
> > > device tree. I would rather have an error in that case, but that is not how
> > > it works.
> > If you actively wanted to get -ENODEV back when there is no regulator
> > then you can use devm_regulator_get_optional() for that.
> >
> > However perhaps be careful what you wish for. If you use get_optional()
> > then you will have to sprinkle NULL or IS_ERR() checks everywhere. I'd
> > favour using the current approach!
>
> Thanks for the info. I think I'll use the get_optionnal(). That will add a
> bit of complexity, but it will avoid deferring some work in
> led_set_brightness_nopm() when it is not needed.
Makes sense, I didn't notice that it allows you to avoid deferred work.
Daniel.
^ permalink raw reply
* Re: [PATCH 6/8] PCI: al: Add support for DW based driver type
From: Chocron, Jonathan @ 2019-07-15 15:18 UTC (permalink / raw)
To: helgaas@kernel.org
Cc: linux-kernel@vger.kernel.org, robh+dt@kernel.org,
jingoohan1@gmail.com, Woodhouse, David, Hanoch, Uri,
devicetree@vger.kernel.org, lorenzo.pieralisi@arm.com,
gustavo.pimentel@synopsys.com, Wasserstrom, Barak, Saidi, Ali,
mark.rutland@arm.com, Hawa, Hanna, Shenhar, Talel, Krupnik, Ronen,
linux-pci@vger.kernel.org, benh@kernel.crashing.org
In-Reply-To: <20190712134217.GD46935@google.com>
On Fri, 2019-07-12 at 08:42 -0500, Bjorn Helgaas wrote:
> On Thu, Jul 11, 2019 at 05:57:05PM +0300, Jonathan Chocron wrote:
> > This driver is DT based and utilizes the DesignWare APIs.
> > It allows using a smaller ECAM range for a larger bus range -
> > usually an entire bus uses 1MB of address space, but the driver
> > can use it for a larger number of buses.
> >
> > All link initializations are handled by the boot FW.
> >
> > Signed-off-by: Jonathan Chocron <jonnyc@amazon.com>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 11 +
> > drivers/pci/controller/dwc/pcie-al.c | 397
> > +++++++++++++++++++++++++++
> > 2 files changed, 408 insertions(+)
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig
> > b/drivers/pci/controller/dwc/Kconfig
> > index a6ce1ee51b4c..51da9cb219aa 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ b/drivers/pci/controller/dwc/Kconfig
> > @@ -230,4 +230,15 @@ config PCIE_UNIPHIER
> > Say Y here if you want PCIe controller support on UniPhier
> > SoCs.
> > This driver supports LD20 and PXs3 SoCs.
> >
> > +config PCIE_AL
> > + bool "Amazon Annapurna Labs PCIe controller"
> > + depends on OF && (ARM64 || COMPILE_TEST)
> > + depends on PCI_MSI_IRQ_DOMAIN
> > + select PCIE_DW_HOST
> > + help
> > + Say Y here to enable support of the Annapurna Labs PCIe
> > controller IP
> > + on Amazon SoCs.
> > + The PCIe controller uses the DesignWare core plus
> > + Amazon-specific hardware wrappers.
>
> Is this one paragraph or two?
>
This was originally a single paragraph, but I hacked it a bit to pass
the checkpatch minimum 4 line description requirement :)
> This should mention the fact that only DT platforms need
> CONFIG_PCIE_AL. ACPI platforms with the Annapurna Labs PCIe
> controller don't need this.
>
No problem, will add. The reason I didn't explicitly state this is
because I didn't see it mentioned in PCIE_HISI, which has similar
support of both ACPI and DT-based drivers.
> > +
> > endmenu
> > diff --git a/drivers/pci/controller/dwc/pcie-al.c
> > b/drivers/pci/controller/dwc/pcie-al.c
> > index 3ab58f0584a8..2742a0895aab 100644
> > --- a/drivers/pci/controller/dwc/pcie-al.c
> > +++ b/drivers/pci/controller/dwc/pcie-al.c
> > @@ -91,3 +91,400 @@ struct pci_ecam_ops al_pcie_ops = {
> > };
> >
> > #endif /* defined(CONFIG_ACPI) && defined(CONFIG_PCI_QUIRKS) */
> > +
> > +#ifdef CONFIG_PCIE_AL
> > +
> > +#include <linux/of_pci.h>
> > +#include "pcie-designware.h"
> > +
> > +#define AL_PCIE_REV_ID_2 2
> > +#define AL_PCIE_REV_ID_3 3
> > +#define AL_PCIE_REV_ID_4 4
> > +
> > +/* The AXI regs are always at the beginning of the PCIE controller
> > reg space. */
> > +#define AXI_BASE_OFFSET 0x0
> > +
> > +#define DEVICE_ID_OFFSET 0x16c
> > +
> > +#define DEVICE_REV_ID 0x0
> > +#define DEVICE_REV_ID_DEV_ID_MASK 0xFFFF0000
> > +#define DEVICE_REV_ID_DEV_ID_SHIFT 16
> > +
> > +#define DEVICE_REV_ID_DEV_ID_X4 (0 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +#define DEVICE_REV_ID_DEV_ID_X8 (2 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +#define DEVICE_REV_ID_DEV_ID_X16 (4 <<
> > DEVICE_REV_ID_DEV_ID_SHIFT)
> > +
> > +/* The ob_ctrl offset inside the axi regs is different between
> > revisions.
> > + */
>
> s/axi/AXI/ to be consistent.
>
Ack.
> Fix comment style (either single-line with "/* ... */" or multi-line
> with "/*" on first line and "*/" on last line).
>
Ack. I'd expect checkpatch to catch that, no?
> > +#define OB_CTRL_REV1_2_OFFSET 0x0040
> > +#define OB_CTRL_REV3_5_OFFSET 0x0030
> > +
> > +#define CFG_TARGET_BUS 0x0
> > +#define CFG_TARGET_BUS_MASK_SHIFT 0
> > +#define CFG_TARGET_BUS_BUSNUM_SHIFT 8
> > +
> > +#define CFG_CONTROL 0x4
> > +#define CFG_CONTROL_SUBBUS_MASK 0x0000FF00
> > +#define CFG_CONTROL_SUBBUS_SHIFT 8
> > +#define CFG_CONTROL_SEC_BUS_MASK 0x00FF0000
> > +#define CFG_CONTROL_SEC_BUS_SHIFT 16
> > +
> > +struct al_pcie_reg_offsets {
> > + unsigned int ob_ctrl;
> > +};
> > +
> > +struct al_pcie_target_bus_cfg {
> > + u8 reg_val;
> > + u8 reg_mask;
> > + u8 ecam_mask;
> > +};
> > +
> > +struct al_pcie {
> > + struct dw_pcie *pci;
> > + void __iomem *controller_base; /* base of PCIe unit (not
> > DW core) */
> > + void __iomem *dbi_base;
>
> I *guess* this is different from the dw_pcie.dbi_base?
Removed (leftover from an early revision).
>
> > + resource_size_t ecam_size;
> > + struct device *dev;
> > + unsigned int controller_rev_id;
> > + struct al_pcie_reg_offsets reg_offsets;
> > + struct al_pcie_target_bus_cfg target_bus_cfg;
> > +};
> > +
> > +#define PCIE_ECAM_DEVFN(x) (((x) & 0xff) << 12)
> > +
> > +#define to_al_pcie(x) dev_get_drvdata((x)->dev)
> > +
> > +static int al_pcie_rev_id_get(struct al_pcie *pcie, unsigned int
> > *rev_id)
>
> I think al_pcie_rev_id_get() and al_pcie_reg_offsets_set() are more
> complicated than necessary, at least for the current code. All
> you're
> really using is something like this:
>
> static void al_pcie_rev_id_get(...)
> {
> ...
> switch (dev_rev_id) {
> case DEVICE_REV_ID_DEV_ID_X4:
> pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> break;
> case DEVICE_REV_ID_DEV_ID_X8:
> case DEVICE_REV_ID_DEV_ID_X16:
> pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> break;
> default:
> dev_err("unsupported rev_id\n");
> break;
> }
> }
>
I understand, this was more of a future preparation, in case a
configuration would be different for a future HW revision. So you think
I should entirely remove the controller_rev_id from the al_pcie
context, or simply unify the logic into a single
"al_pcie_rev_id_setup()" func?
> > +{
> > + void __iomem *dev_rev_id_addr;
> > + u32 dev_rev_id;
> > +
> > + dev_rev_id_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > + AXI_BASE_OFFSET + DEVICE_ID_OFFSET +
> > DEVICE_REV_ID);
> > +
> > + dev_rev_id = readl(dev_rev_id_addr) &
> > DEVICE_REV_ID_DEV_ID_MASK;
> > +
> > + switch (dev_rev_id) {
> > + case DEVICE_REV_ID_DEV_ID_X4:
> > + *rev_id = AL_PCIE_REV_ID_2;
> > + break;
> > + case DEVICE_REV_ID_DEV_ID_X8:
> > + *rev_id = AL_PCIE_REV_ID_3;
> > + break;
> > + case DEVICE_REV_ID_DEV_ID_X16:
> > + *rev_id = AL_PCIE_REV_ID_4;
> > + break;
> > + default:
> > + dev_err(pcie->dev, "Unsupported dev_rev_id (0x%08x)\n",
> > + dev_rev_id);
> > + return -EINVAL;
> > + }
> > +
> > + dev_dbg(pcie->dev, "dev_rev_id: 0x%08x\n", dev_rev_id);
> > +
> > + return 0;
> > +}
> > +
> > +static int al_pcie_reg_offsets_set(struct al_pcie *pcie)
> > +{
> > + switch (pcie->controller_rev_id) {
> > + case AL_PCIE_REV_ID_2:
> > + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV1_2_OFFSET;
> > + break;
> > + case AL_PCIE_REV_ID_3:
> > + case AL_PCIE_REV_ID_4:
> > + pcie->reg_offsets.ob_ctrl = OB_CTRL_REV3_5_OFFSET;
> > + break;
> > + default:
> > + dev_err(pcie->dev, "Unsupported controller rev_id:
> > 0x%x\n",
> > + pcie->controller_rev_id);
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline void al_pcie_target_bus_set(struct al_pcie *pcie,
> > + u8 target_bus,
> > + u8 mask_target_bus)
> > +{
> > + void __iomem *cfg_control_addr;
> > + u32 reg = (target_bus << CFG_TARGET_BUS_BUSNUM_SHIFT) |
> > + mask_target_bus;
> > +
> > + cfg_control_addr = (void __iomem *)((uintptr_t)pcie-
> > >controller_base +
> > + AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl
> > +
> > + CFG_TARGET_BUS);
> > +
> > + writel(reg, cfg_control_addr);
> > +}
> > +
> > +static void __iomem *al_pcie_conf_addr_map(struct al_pcie *pcie,
> > + unsigned int busnr,
> > + unsigned int devfn)
> > +{
> > + void __iomem *pci_base_addr;
> > + struct pcie_port *pp = &pcie->pci->pp;
> > + struct al_pcie_target_bus_cfg *target_bus_cfg = &pcie-
> > >target_bus_cfg;
> > + unsigned int busnr_ecam = busnr & target_bus_cfg->ecam_mask;
> > + unsigned int busnr_reg = busnr & target_bus_cfg->reg_mask;
> > +
> > + pci_base_addr = (void __iomem *)((uintptr_t)pp->va_cfg0_base +
> > + (busnr_ecam << 20) +
> > + PCIE_ECAM_DEVFN(devfn));
> > +
> > + if (busnr_reg != target_bus_cfg->reg_val) {
> > + dev_dbg(pcie->pci->dev, "Changing target bus busnum val
> > from %d to %d\n",
> > + target_bus_cfg->reg_val, busnr_reg);
> > + target_bus_cfg->reg_val = busnr_reg;
> > + al_pcie_target_bus_set(pcie,
> > + target_bus_cfg->reg_val,
> > + target_bus_cfg->reg_mask);
> > + }
> > +
> > + return pci_base_addr;
> > +}
> > +
> > +static int al_pcie_rd_other_conf(struct pcie_port *pp,
> > + struct pci_bus *bus,
> > + unsigned int devfn,
> > + int where,
> > + int size,
> > + u32 *val)
>
> Rewrap to use fewer lines.
>
Done.
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + unsigned int busnr = bus->number;
> > + void __iomem *pci_addr;
> > + int rc;
> > +
> > + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > + rc = dw_pcie_read(pci_addr + where, size, val);
> > +
> > + dev_dbg(pci->dev, "%d-byte config read from %04x:%02x:%02x.%d
> > offset %#x (pci_addr: 0x%p) - val:0x%x\n",
Do you think I should change the %p here to %px?
> > + size, pci_domain_nr(bus), bus->number,
> > + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > + (pci_addr + where), *val);
> > +
> > + return rc;
> > +}
> > +
> > +static int al_pcie_wr_other_conf(struct pcie_port *pp,
> > + struct pci_bus *bus,
> > + unsigned int devfn,
> > + int where,
> > + int size,
> > + u32 val)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + unsigned int busnr = bus->number;
> > + void __iomem *pci_addr;
> > + int rc;
> > +
> > + pci_addr = al_pcie_conf_addr_map(pcie, busnr, devfn);
> > +
> > + rc = dw_pcie_write(pci_addr + where, size, val);
> > +
> > + dev_dbg(pci->dev, "%d-byte config write to %04x:%02x:%02x.%d
> > offset %#x (pci_addr: 0x%p) - val:0x%x\n",
> > + size, pci_domain_nr(bus), bus->number,
> > + PCI_SLOT(devfn), PCI_FUNC(devfn), where,
> > + (pci_addr + where), val);
> > +
> > + return rc;
> > +}
> > +
> > +static int al_pcie_config_prepare(struct al_pcie *pcie)
> > +{
> > + struct al_pcie_target_bus_cfg *target_bus_cfg;
> > + struct pcie_port *pp = &pcie->pci->pp;
> > + unsigned int ecam_bus_mask;
> > + u8 secondary_bus;
> > + u8 subordinate_bus;
> > + void __iomem *cfg_control_addr;
> > + u32 cfg_control;
> > + u32 reg = 0;
>
> Why is this initialized? It doesn't look like it can be used before
> being set.
Removed initialization.
>
> > + target_bus_cfg = &pcie->target_bus_cfg;
> > +
> > + ecam_bus_mask = (pcie->ecam_size >> 20) - 1;
> > + if (ecam_bus_mask > 255) {
> > + dev_warn(pcie->dev, "ECAM window size is larger than
> > 256MB. Cutting off at 256\n");
> > + ecam_bus_mask = 255;
> > + }
> > +
> > + /* This portion is taken from the transaction address */
> > + target_bus_cfg->ecam_mask = ecam_bus_mask;
> > + /* This portion is taken from the cfg_target_bus reg */
> > + target_bus_cfg->reg_mask = ~target_bus_cfg->ecam_mask;
> > + target_bus_cfg->reg_val = pp->busn->start & target_bus_cfg-
> > >reg_mask;
> > +
> > + al_pcie_target_bus_set(pcie,
> > + target_bus_cfg->reg_val,
> > + target_bus_cfg->reg_mask);
> > +
> > + secondary_bus = pp->busn->start + 1;
> > + subordinate_bus = pp->busn->end;
> > +
> > + /* Set the valid values of secondary and subordinate buses */
> > + cfg_control_addr = pcie->controller_base +
> > + AXI_BASE_OFFSET + pcie->reg_offsets.ob_ctrl +
> > CFG_CONTROL;
> > +
> > + cfg_control = readl(cfg_control_addr);
> > +
> > + reg = (cfg_control & ~CFG_CONTROL_SEC_BUS_MASK) |
> > + (secondary_bus << CFG_CONTROL_SEC_BUS_SHIFT);
> > + reg = (reg & ~CFG_CONTROL_SUBBUS_MASK) |
> > + (subordinate_bus << CFG_CONTROL_SUBBUS_SHIFT);
> > +
> > + writel(reg, cfg_control_addr);
> > +
> > + return 0;
> > +}
> > +
> > +static int al_pcie_host_init(struct pcie_port *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct al_pcie *pcie = to_al_pcie(pci);
> > + int link_up = 0;
> > + u32 reg = 0;
>
> Why initialize link_up and reg?
>
Removed initialization.
> > + int rc;
> > +
> > + reg = dw_pcie_readb_dbi(pci, PCI_HEADER_TYPE);
> > + if (reg != PCI_HEADER_TYPE_BRIDGE) {
> > + dev_err(pci->dev, "PCIe controller is not set to bridge
> > type (%d)!\n",
> > + reg);
> > + return -EIO;
> > + }
> > +
Removing this validation as it is done generically in PATCH 7/8.
> > + link_up = dw_pcie_link_up(pci);
> > + if (!link_up) {
> > + dev_err(pci->dev, "link in not up!\n");
>
> s/in/is/
Ack.
>
> > + return -ENOLINK;
> > + }
> > +
> > + dev_info(pci->dev, "link is up\n");
> > +
> > + rc = al_pcie_rev_id_get(pcie, &pcie->controller_rev_id);
> > + if (rc)
> > + return rc;
> > +
> > + rc = al_pcie_reg_offsets_set(pcie);
> > + if (rc)
> > + return rc;
> > +
> > + rc = al_pcie_config_prepare(pcie);
> > + if (rc)
> > + return rc;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops al_pcie_host_ops = {
> > + .rd_other_conf = al_pcie_rd_other_conf,
> > + .wr_other_conf = al_pcie_wr_other_conf,
> > + .host_init = al_pcie_host_init,
> > +};
> > +
> > +static int al_add_pcie_port(struct pcie_port *pp,
> > + struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + int ret;
> > +
> > + pp->ops = &al_pcie_host_ops;
> > +
> > + ret = dw_pcie_host_init(pp);
> > + if (ret) {
> > + dev_err(dev, "failed to initialize host\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops dw_pcie_ops = {
> > +};
> > +
> > +static int al_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct al_pcie *al_pcie;
> > + struct dw_pcie *pci;
> > + struct resource *dbi_res;
> > + struct resource *controller_res;
> > + struct resource *ecam_res;
> > + int ret;
> > +
> > + al_pcie = devm_kzalloc(dev, sizeof(*al_pcie), GFP_KERNEL);
> > + if (!al_pcie)
> > + return -ENOMEM;
> > +
> > + pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > + if (!pci)
> > + return -ENOMEM;
> > +
> > + pci->dev = dev;
> > + pci->ops = &dw_pcie_ops;
> > +
> > + al_pcie->pci = pci;
> > + al_pcie->dev = dev;
> > +
> > + dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "dbi");
> > + pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_res);
> > + if (IS_ERR(pci->dbi_base)) {
> > + dev_err(dev, "couldn't remap dbi base %p\n", dbi_res);
>
> %pR (see below).
>
Fixed.
> > + return PTR_ERR(pci->dbi_base);
> > + }
> > +
> > + ecam_res = platform_get_resource_byname(pdev,
> > + IORESOURCE_MEM,
> > + "config");
> > + if (!ecam_res) {
> > + dev_err(dev, "couldn't find 'config' reg in DT\n");
> > + return -ENOENT;
> > + }
> > + al_pcie->ecam_size = resource_size(ecam_res);
> > +
> > + controller_res = platform_get_resource_byname(pdev,
> > + IORESOURCE_MEM,
> > + "controller");
> > + al_pcie->controller_base = devm_ioremap_resource(dev,
> > + controller_res
> > );
>
> Several of these function calls would fit on one line, or at least
> fewer than they currently use.
>
Fixed.
> > + if (IS_ERR(al_pcie->controller_base)) {
> > + dev_err(dev, "couldn't remap controller base %p\n",
> > + controller_res);
>
> Use %pR. I think this %p prints the address of the struct resource,
> not the address you tried to map.
>
Fixed.
> > + return PTR_ERR(al_pcie->controller_base);
> > + }
> > +
> > + dev_dbg(dev, "From DT: dbi_base: 0x%llx, controller_base:
> > 0x%llx\n",
> > + dbi_res->start, controller_res->start);
>
> Use %pR instead of printing dbi_res->start, controller_res->start.
>
Done.
> > +
> > + platform_set_drvdata(pdev, al_pcie);
> > +
> > + ret = al_add_pcie_port(&pci->pp, pdev);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > +
> > +static const struct of_device_id al_pcie_of_match[] = {
> > + { .compatible = "amazon,al-pcie",
> > + .data = NULL,
>
> ".data = NULL" is unnecessary.
>
Done.
> > + },
> > + {},
> > +};
> > +
> > +static struct platform_driver al_pcie_driver = {
> > + .driver = {
> > + .name = "al-pcie",
> > + .of_match_table = al_pcie_of_match,
> > + .suppress_bind_attrs = true,
> > + },
> > + .probe = al_pcie_probe,
> > +};
> > +builtin_platform_driver(al_pcie_driver);
> > +
> > +#endif /* CONFIG_PCIE_AL*/
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [v4,1/2] rtc/fsl: add FTM alarm driver as the wakeup source
From: Alexandre Belloni @ 2019-07-15 14:56 UTC (permalink / raw)
To: Biwen Li
Cc: a.zummo, leoyang.li, robh+dt, linux-rtc, linux-kernel, xiaobo.xie,
jiafei.pan, ran.wang_1, mark.rutland, devicetree
In-Reply-To: <20190715101520.22562-1-biwen.li@nxp.com>
On 15/07/2019 18:15:19+0800, Biwen Li wrote:
> + device_init_wakeup(&pdev->dev, true);
> + rtc->rtc_dev = devm_rtc_device_register(&pdev->dev, "ftm-alarm",
> + &ftm_rtc_ops,
> + THIS_MODULE);
To be clear, I want you to not use devm_rtc_device_register and use
devm_rtc_allocate_device and rtc_register_device.
> + if (IS_ERR(rtc->rtc_dev)) {
> + dev_err(&pdev->dev, "can't register rtc device\n");
> + return PTR_ERR(rtc->rtc_dev);
> + }
> +
--
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Saravana Kannan @ 2019-07-15 14:47 UTC (permalink / raw)
To: Frank Rowand
Cc: Rob Herring, Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki,
devicetree, linux-kernel@vger.kernel.org, David Collins,
Android Kernel Team
In-Reply-To: <07812739-0e6b-6598-ac58-8e0ea74a3331@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 9663 bytes --]
A kernel command line option can also completely disable this functionality
easily and cleanly. Can we pick that as an option? I've an implementation
of that in the v5 series I sent out last week.
-Saravana
On Mon, Jul 15, 2019, 7:39 AM Frank Rowand <frowand.list@gmail.com> wrote:
> On 7/15/19 7:26 AM, Frank Rowand wrote:
> > HiRob,
> >
> > Sorry for such a late reply...
> >
> >
> > On 7/1/19 8:25 PM, Saravana Kannan wrote:
> >> On Mon, Jul 1, 2019 at 6:32 PM Rob Herring <robh+dt@kernel.org> wrote:
> >>>
> >>> On Mon, Jul 1, 2019 at 6:48 PM Saravana Kannan <saravanak@google.com>
> wrote:
> >>>>
> >>>> Add device-links after the devices are created (but before they are
> >>>> probed) by looking at common DT bindings like clocks and
> >>>> interconnects.
> >>>>
> >>>> Automatically adding device-links for functional dependencies at the
> >>>> framework level provides the following benefits:
> >>>>
> >>>> - Optimizes device probe order and avoids the useless work of
> >>>> attempting probes of devices that will not probe successfully
> >>>> (because their suppliers aren't present or haven't probed yet).
> >>>>
> >>>> For example, in a commonly available mobile SoC, registering just
> >>>> one consumer device's driver at an initcall level earlier than the
> >>>> supplier device's driver causes 11 failed probe attempts before the
> >>>> consumer device probes successfully. This was with a kernel with all
> >>>> the drivers statically compiled in. This problem gets a lot worse if
> >>>> all the drivers are loaded as modules without direct symbol
> >>>> dependencies.
> >>>>
> >>>> - Supplier devices like clock providers, interconnect providers, etc
> >>>> need to keep the resources they provide active and at a particular
> >>>> state(s) during boot up even if their current set of consumers don't
> >>>> request the resource to be active. This is because the rest of the
> >>>> consumers might not have probed yet and turning off the resource
> >>>> before all the consumers have probed could lead to a hang or
> >>>> undesired user experience.
> >>>>
> >>>> Some frameworks (Eg: regulator) handle this today by turning off
> >>>> "unused" resources at late_initcall_sync and hoping all the devices
> >>>> have probed by then. This is not a valid assumption for systems with
> >>>> loadable modules. Other frameworks (Eg: clock) just don't handle
> >>>> this due to the lack of a clear signal for when they can turn off
> >>>> resources. This leads to downstream hacks to handle cases like this
> >>>> that can easily be solved in the upstream kernel.
> >>>>
> >>>> By linking devices before they are probed, we give suppliers a clear
> >>>> count of the number of dependent consumers. Once all of the
> >>>> consumers are active, the suppliers can turn off the unused
> >>>> resources without making assumptions about the number of consumers.
> >>>>
> >>>> By default we just add device-links to track "driver presence" (probe
> >>>> succeeded) of the supplier device. If any other functionality provided
> >>>> by device-links are needed, it is left to the consumer/supplier
> >>>> devices to change the link when they probe.
> >>>>
> >>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >>>> ---
> >>>> drivers/of/Kconfig | 9 ++++++++
> >>>> drivers/of/platform.c | 52
> +++++++++++++++++++++++++++++++++++++++++++
> >>>> 2 files changed, 61 insertions(+)
> >>>>
> >>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >>>> index 37c2ccbefecd..7c7fa7394b4c 100644
> >>>> --- a/drivers/of/Kconfig
> >>>> +++ b/drivers/of/Kconfig
> >>>> @@ -103,4 +103,13 @@ config OF_OVERLAY
> >>>> config OF_NUMA
> >>>> bool
> >>>>
> >>>> +config OF_DEVLINKS
> >>>
> >>> I'd prefer this not be a config option. After all, we want one kernel
> >>> build that works for all platforms.
> >>
> >> We need a lot more changes before one kernel build can work for all
> >> platforms. At least until then, I think we need this. Lot less chance
> >> of breaking existing platforms before all the missing pieces are
> >> created.
> >>
> >>> A kernel command line option to disable might be useful for debugging.
> >>
> >> Or we can have a command line to enable this for platforms that want
> >> to use it and have it default off.
> >
>
> > Given the fragility of the current boot sequence (without this patch set)
> > and the potential breakage of existing systems, I think that if we choose
> > to accept this patch set that it should first bake in the -next tree for
> > at least one major release cycle. Maybe even two major release cycles.
>
> I probably didn't state that very well. I was trying to not sound like
> I was accusing this patch series of being fragile. The issue is that
> adding the patches to systems that weren't expecting the new ordering
> may cause boot problems for some systems. I'm not concerned with
> pointing fingers, just want to make sure that we proceed cautiously
> until we know that the resulting system is robust.
>
> -Frank
>
> >
> > -Frank
> >
> >
> >>
> >>>> + bool "Device links from DT bindings"
> >>>> + help
> >>>> + Common DT bindings like clocks, interconnects, etc
> represent a
> >>>> + consumer device's dependency on suppliers devices. This
> option
> >>>> + creates device links from these common bindings so that
> consumers are
> >>>> + probed only after all their suppliers are active and
> suppliers can
> >>>> + tell when all their consumers are active.
> >>>> +
> >>>> endif # OF
> >>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
> >>>> index 04ad312fd85b..a53717168aca 100644
> >>>> --- a/drivers/of/platform.c
> >>>> +++ b/drivers/of/platform.c
> >>>> @@ -61,6 +61,57 @@ struct platform_device
> *of_find_device_by_node(struct device_node *np)
> >>>> EXPORT_SYMBOL(of_find_device_by_node);
> >>>>
> >>>> #ifdef CONFIG_OF_ADDRESS
> >>>> +static int of_link_binding(struct device *dev, char *binding, char
> *cell)
> >>>
> >>> Under CONFIG_OF_ADDRESS seems like a strange location.
> >>
> >> Yeah, but the rest of the file seems to be under this. So I'm not
> >> touching that. I can probably move this function further down (close
> >> to platform populate) if you want that.
> >>>
> >>>> +{
> >>>> + struct of_phandle_args sup_args;
> >>>> + struct platform_device *sup_dev;
> >>>> + unsigned int i = 0, links = 0;
> >>>> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
> >>>> +
> >>>> + while (!of_parse_phandle_with_args(dev->of_node, binding,
> cell, i,
> >>>> + &sup_args)) {
> >>>> + i++;
> >>>> + sup_dev = of_find_device_by_node(sup_args.np);
> >>>> + if (!sup_dev)
> >>>> + continue;
> >>>> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
> >>>> + links++;
> >>>> + put_device(&sup_dev->dev);
> >>>> + }
> >>>> + if (links < i)
> >>>> + return -ENODEV;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> +/*
> >>>> + * List of bindings and their cell names (use NULL if no cell names)
> from which
> >>>> + * device links need to be created.
> >>>> + */
> >>>> +static char *link_bindings[] = {
> >>>
> >>> const
> >>
> >> Ack
> >>
> >>>
> >>>> +#ifdef CONFIG_OF_DEVLINKS
> >>>> + "clocks", "#clock-cells",
> >>>> + "interconnects", "#interconnect-cells",
> >>>
> >>> Planning to add others?
> >>
> >> Not in this patch.
> >>
> >> Regulators are the other big missing piece that I'm aware of now but
> >> they need a lot of discussion (see email from David and my reply).
> >>
> >> Not sure what other resources are shared where they can be "turned
> >> off" and cause devices set up at boot to fail. For example, I don't
> >> think interrupts need functional dependency tracking because they
> >> aren't really turned off by consumer 1 in a way that breaks things for
> >> consumer 2. Just masked and the consumer 2 can unmask and use it once
> >> it probes.
> >>
> >> I'm only intimately familiar with clocks, interconnects and regulators
> >> (to some extent). I'm open to adding other supplier categories in
> >> future patches as I educate myself of those or if other people want to
> >> add support for more categories.
> >>
> >> -Saravana
> >>
> >>>> +#endif
> >>>> +};
> >>>> +
> >>>> +static int of_link_to_suppliers(struct device *dev)
> >>>> +{
> >>>> + unsigned int i = 0;
> >>>> + bool done = true;
> >>>> +
> >>>> + if (unlikely(!dev->of_node))
> >>>> + return 0;
> >>>> +
> >>>> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
> >>>> + if (of_link_binding(dev, link_bindings[i * 2],
> >>>> + link_bindings[i * 2 + 1]))
> >>>> + done = false;
> >>>> +
> >>>> + if (!done)
> >>>> + return -ENODEV;
> >>>> + return 0;
> >>>> +}
> >>>> +
> >>>> /*
> >>>> * The following routines scan a subtree and registers a device for
> >>>> * each applicable node.
> >>>> @@ -524,6 +575,7 @@ static int __init
> of_platform_default_populate_init(void)
> >>>> if (!of_have_populated_dt())
> >>>> return -ENODEV;
> >>>>
> >>>> + platform_bus_type.add_links = of_link_to_suppliers;
> >>>> /*
> >>>> * Handle certain compatibles explicitly, since we don't want
> to create
> >>>> * platform_devices for every node in /reserved-memory with a
> >>>> --
> >>>> 2.22.0.410.gd8fdbe21b5-goog
> >>>>
> >>
> >
> >
>
>
[-- Attachment #2: Type: text/html, Size: 13627 bytes --]
^ permalink raw reply
* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-07-15 14:38 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, devicetree,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team
In-Reply-To: <9e75b3dd-380b-c868-728f-46379e53bc11@gmail.com>
On 7/15/19 7:26 AM, Frank Rowand wrote:
> HiRob,
>
> Sorry for such a late reply...
>
>
> On 7/1/19 8:25 PM, Saravana Kannan wrote:
>> On Mon, Jul 1, 2019 at 6:32 PM Rob Herring <robh+dt@kernel.org> wrote:
>>>
>>> On Mon, Jul 1, 2019 at 6:48 PM Saravana Kannan <saravanak@google.com> wrote:
>>>>
>>>> Add device-links after the devices are created (but before they are
>>>> probed) by looking at common DT bindings like clocks and
>>>> interconnects.
>>>>
>>>> Automatically adding device-links for functional dependencies at the
>>>> framework level provides the following benefits:
>>>>
>>>> - Optimizes device probe order and avoids the useless work of
>>>> attempting probes of devices that will not probe successfully
>>>> (because their suppliers aren't present or haven't probed yet).
>>>>
>>>> For example, in a commonly available mobile SoC, registering just
>>>> one consumer device's driver at an initcall level earlier than the
>>>> supplier device's driver causes 11 failed probe attempts before the
>>>> consumer device probes successfully. This was with a kernel with all
>>>> the drivers statically compiled in. This problem gets a lot worse if
>>>> all the drivers are loaded as modules without direct symbol
>>>> dependencies.
>>>>
>>>> - Supplier devices like clock providers, interconnect providers, etc
>>>> need to keep the resources they provide active and at a particular
>>>> state(s) during boot up even if their current set of consumers don't
>>>> request the resource to be active. This is because the rest of the
>>>> consumers might not have probed yet and turning off the resource
>>>> before all the consumers have probed could lead to a hang or
>>>> undesired user experience.
>>>>
>>>> Some frameworks (Eg: regulator) handle this today by turning off
>>>> "unused" resources at late_initcall_sync and hoping all the devices
>>>> have probed by then. This is not a valid assumption for systems with
>>>> loadable modules. Other frameworks (Eg: clock) just don't handle
>>>> this due to the lack of a clear signal for when they can turn off
>>>> resources. This leads to downstream hacks to handle cases like this
>>>> that can easily be solved in the upstream kernel.
>>>>
>>>> By linking devices before they are probed, we give suppliers a clear
>>>> count of the number of dependent consumers. Once all of the
>>>> consumers are active, the suppliers can turn off the unused
>>>> resources without making assumptions about the number of consumers.
>>>>
>>>> By default we just add device-links to track "driver presence" (probe
>>>> succeeded) of the supplier device. If any other functionality provided
>>>> by device-links are needed, it is left to the consumer/supplier
>>>> devices to change the link when they probe.
>>>>
>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>> ---
>>>> drivers/of/Kconfig | 9 ++++++++
>>>> drivers/of/platform.c | 52 +++++++++++++++++++++++++++++++++++++++++++
>>>> 2 files changed, 61 insertions(+)
>>>>
>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>>>> index 37c2ccbefecd..7c7fa7394b4c 100644
>>>> --- a/drivers/of/Kconfig
>>>> +++ b/drivers/of/Kconfig
>>>> @@ -103,4 +103,13 @@ config OF_OVERLAY
>>>> config OF_NUMA
>>>> bool
>>>>
>>>> +config OF_DEVLINKS
>>>
>>> I'd prefer this not be a config option. After all, we want one kernel
>>> build that works for all platforms.
>>
>> We need a lot more changes before one kernel build can work for all
>> platforms. At least until then, I think we need this. Lot less chance
>> of breaking existing platforms before all the missing pieces are
>> created.
>>
>>> A kernel command line option to disable might be useful for debugging.
>>
>> Or we can have a command line to enable this for platforms that want
>> to use it and have it default off.
>
> Given the fragility of the current boot sequence (without this patch set)
> and the potential breakage of existing systems, I think that if we choose
> to accept this patch set that it should first bake in the -next tree for
> at least one major release cycle. Maybe even two major release cycles.
I probably didn't state that very well. I was trying to not sound like
I was accusing this patch series of being fragile. The issue is that
adding the patches to systems that weren't expecting the new ordering
may cause boot problems for some systems. I'm not concerned with
pointing fingers, just want to make sure that we proceed cautiously
until we know that the resulting system is robust.
-Frank
>
> -Frank
>
>
>>
>>>> + bool "Device links from DT bindings"
>>>> + help
>>>> + Common DT bindings like clocks, interconnects, etc represent a
>>>> + consumer device's dependency on suppliers devices. This option
>>>> + creates device links from these common bindings so that consumers are
>>>> + probed only after all their suppliers are active and suppliers can
>>>> + tell when all their consumers are active.
>>>> +
>>>> endif # OF
>>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>>> index 04ad312fd85b..a53717168aca 100644
>>>> --- a/drivers/of/platform.c
>>>> +++ b/drivers/of/platform.c
>>>> @@ -61,6 +61,57 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
>>>> EXPORT_SYMBOL(of_find_device_by_node);
>>>>
>>>> #ifdef CONFIG_OF_ADDRESS
>>>> +static int of_link_binding(struct device *dev, char *binding, char *cell)
>>>
>>> Under CONFIG_OF_ADDRESS seems like a strange location.
>>
>> Yeah, but the rest of the file seems to be under this. So I'm not
>> touching that. I can probably move this function further down (close
>> to platform populate) if you want that.
>>>
>>>> +{
>>>> + struct of_phandle_args sup_args;
>>>> + struct platform_device *sup_dev;
>>>> + unsigned int i = 0, links = 0;
>>>> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
>>>> +
>>>> + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
>>>> + &sup_args)) {
>>>> + i++;
>>>> + sup_dev = of_find_device_by_node(sup_args.np);
>>>> + if (!sup_dev)
>>>> + continue;
>>>> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
>>>> + links++;
>>>> + put_device(&sup_dev->dev);
>>>> + }
>>>> + if (links < i)
>>>> + return -ENODEV;
>>>> + return 0;
>>>> +}
>>>> +
>>>> +/*
>>>> + * List of bindings and their cell names (use NULL if no cell names) from which
>>>> + * device links need to be created.
>>>> + */
>>>> +static char *link_bindings[] = {
>>>
>>> const
>>
>> Ack
>>
>>>
>>>> +#ifdef CONFIG_OF_DEVLINKS
>>>> + "clocks", "#clock-cells",
>>>> + "interconnects", "#interconnect-cells",
>>>
>>> Planning to add others?
>>
>> Not in this patch.
>>
>> Regulators are the other big missing piece that I'm aware of now but
>> they need a lot of discussion (see email from David and my reply).
>>
>> Not sure what other resources are shared where they can be "turned
>> off" and cause devices set up at boot to fail. For example, I don't
>> think interrupts need functional dependency tracking because they
>> aren't really turned off by consumer 1 in a way that breaks things for
>> consumer 2. Just masked and the consumer 2 can unmask and use it once
>> it probes.
>>
>> I'm only intimately familiar with clocks, interconnects and regulators
>> (to some extent). I'm open to adding other supplier categories in
>> future patches as I educate myself of those or if other people want to
>> add support for more categories.
>>
>> -Saravana
>>
>>>> +#endif
>>>> +};
>>>> +
>>>> +static int of_link_to_suppliers(struct device *dev)
>>>> +{
>>>> + unsigned int i = 0;
>>>> + bool done = true;
>>>> +
>>>> + if (unlikely(!dev->of_node))
>>>> + return 0;
>>>> +
>>>> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
>>>> + if (of_link_binding(dev, link_bindings[i * 2],
>>>> + link_bindings[i * 2 + 1]))
>>>> + done = false;
>>>> +
>>>> + if (!done)
>>>> + return -ENODEV;
>>>> + return 0;
>>>> +}
>>>> +
>>>> /*
>>>> * The following routines scan a subtree and registers a device for
>>>> * each applicable node.
>>>> @@ -524,6 +575,7 @@ static int __init of_platform_default_populate_init(void)
>>>> if (!of_have_populated_dt())
>>>> return -ENODEV;
>>>>
>>>> + platform_bus_type.add_links = of_link_to_suppliers;
>>>> /*
>>>> * Handle certain compatibles explicitly, since we don't want to create
>>>> * platform_devices for every node in /reserved-memory with a
>>>> --
>>>> 2.22.0.410.gd8fdbe21b5-goog
>>>>
>>
>
>
^ permalink raw reply
* Re: [PATCH v3 2/4] of/platform: Add functional dependency link from DT bindings
From: Frank Rowand @ 2019-07-15 14:26 UTC (permalink / raw)
To: Saravana Kannan, Rob Herring
Cc: Mark Rutland, Greg Kroah-Hartman, Rafael J. Wysocki, devicetree,
linux-kernel@vger.kernel.org, David Collins, Android Kernel Team
In-Reply-To: <CAGETcx_i9353aRFbJXNS78EvqwmU-2-xSBJ+ySZX1gjjHpz_cg@mail.gmail.com>
HiRob,
Sorry for such a late reply...
On 7/1/19 8:25 PM, Saravana Kannan wrote:
> On Mon, Jul 1, 2019 at 6:32 PM Rob Herring <robh+dt@kernel.org> wrote:
>>
>> On Mon, Jul 1, 2019 at 6:48 PM Saravana Kannan <saravanak@google.com> wrote:
>>>
>>> Add device-links after the devices are created (but before they are
>>> probed) by looking at common DT bindings like clocks and
>>> interconnects.
>>>
>>> Automatically adding device-links for functional dependencies at the
>>> framework level provides the following benefits:
>>>
>>> - Optimizes device probe order and avoids the useless work of
>>> attempting probes of devices that will not probe successfully
>>> (because their suppliers aren't present or haven't probed yet).
>>>
>>> For example, in a commonly available mobile SoC, registering just
>>> one consumer device's driver at an initcall level earlier than the
>>> supplier device's driver causes 11 failed probe attempts before the
>>> consumer device probes successfully. This was with a kernel with all
>>> the drivers statically compiled in. This problem gets a lot worse if
>>> all the drivers are loaded as modules without direct symbol
>>> dependencies.
>>>
>>> - Supplier devices like clock providers, interconnect providers, etc
>>> need to keep the resources they provide active and at a particular
>>> state(s) during boot up even if their current set of consumers don't
>>> request the resource to be active. This is because the rest of the
>>> consumers might not have probed yet and turning off the resource
>>> before all the consumers have probed could lead to a hang or
>>> undesired user experience.
>>>
>>> Some frameworks (Eg: regulator) handle this today by turning off
>>> "unused" resources at late_initcall_sync and hoping all the devices
>>> have probed by then. This is not a valid assumption for systems with
>>> loadable modules. Other frameworks (Eg: clock) just don't handle
>>> this due to the lack of a clear signal for when they can turn off
>>> resources. This leads to downstream hacks to handle cases like this
>>> that can easily be solved in the upstream kernel.
>>>
>>> By linking devices before they are probed, we give suppliers a clear
>>> count of the number of dependent consumers. Once all of the
>>> consumers are active, the suppliers can turn off the unused
>>> resources without making assumptions about the number of consumers.
>>>
>>> By default we just add device-links to track "driver presence" (probe
>>> succeeded) of the supplier device. If any other functionality provided
>>> by device-links are needed, it is left to the consumer/supplier
>>> devices to change the link when they probe.
>>>
>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>> ---
>>> drivers/of/Kconfig | 9 ++++++++
>>> drivers/of/platform.c | 52 +++++++++++++++++++++++++++++++++++++++++++
>>> 2 files changed, 61 insertions(+)
>>>
>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>>> index 37c2ccbefecd..7c7fa7394b4c 100644
>>> --- a/drivers/of/Kconfig
>>> +++ b/drivers/of/Kconfig
>>> @@ -103,4 +103,13 @@ config OF_OVERLAY
>>> config OF_NUMA
>>> bool
>>>
>>> +config OF_DEVLINKS
>>
>> I'd prefer this not be a config option. After all, we want one kernel
>> build that works for all platforms.
>
> We need a lot more changes before one kernel build can work for all
> platforms. At least until then, I think we need this. Lot less chance
> of breaking existing platforms before all the missing pieces are
> created.
>
>> A kernel command line option to disable might be useful for debugging.
>
> Or we can have a command line to enable this for platforms that want
> to use it and have it default off.
Given the fragility of the current boot sequence (without this patch set)
and the potential breakage of existing systems, I think that if we choose
to accept this patch set that it should first bake in the -next tree for
at least one major release cycle. Maybe even two major release cycles.
-Frank
>
>>> + bool "Device links from DT bindings"
>>> + help
>>> + Common DT bindings like clocks, interconnects, etc represent a
>>> + consumer device's dependency on suppliers devices. This option
>>> + creates device links from these common bindings so that consumers are
>>> + probed only after all their suppliers are active and suppliers can
>>> + tell when all their consumers are active.
>>> +
>>> endif # OF
>>> diff --git a/drivers/of/platform.c b/drivers/of/platform.c
>>> index 04ad312fd85b..a53717168aca 100644
>>> --- a/drivers/of/platform.c
>>> +++ b/drivers/of/platform.c
>>> @@ -61,6 +61,57 @@ struct platform_device *of_find_device_by_node(struct device_node *np)
>>> EXPORT_SYMBOL(of_find_device_by_node);
>>>
>>> #ifdef CONFIG_OF_ADDRESS
>>> +static int of_link_binding(struct device *dev, char *binding, char *cell)
>>
>> Under CONFIG_OF_ADDRESS seems like a strange location.
>
> Yeah, but the rest of the file seems to be under this. So I'm not
> touching that. I can probably move this function further down (close
> to platform populate) if you want that.
>>
>>> +{
>>> + struct of_phandle_args sup_args;
>>> + struct platform_device *sup_dev;
>>> + unsigned int i = 0, links = 0;
>>> + u32 dl_flags = DL_FLAG_AUTOPROBE_CONSUMER;
>>> +
>>> + while (!of_parse_phandle_with_args(dev->of_node, binding, cell, i,
>>> + &sup_args)) {
>>> + i++;
>>> + sup_dev = of_find_device_by_node(sup_args.np);
>>> + if (!sup_dev)
>>> + continue;
>>> + if (device_link_add(dev, &sup_dev->dev, dl_flags))
>>> + links++;
>>> + put_device(&sup_dev->dev);
>>> + }
>>> + if (links < i)
>>> + return -ENODEV;
>>> + return 0;
>>> +}
>>> +
>>> +/*
>>> + * List of bindings and their cell names (use NULL if no cell names) from which
>>> + * device links need to be created.
>>> + */
>>> +static char *link_bindings[] = {
>>
>> const
>
> Ack
>
>>
>>> +#ifdef CONFIG_OF_DEVLINKS
>>> + "clocks", "#clock-cells",
>>> + "interconnects", "#interconnect-cells",
>>
>> Planning to add others?
>
> Not in this patch.
>
> Regulators are the other big missing piece that I'm aware of now but
> they need a lot of discussion (see email from David and my reply).
>
> Not sure what other resources are shared where they can be "turned
> off" and cause devices set up at boot to fail. For example, I don't
> think interrupts need functional dependency tracking because they
> aren't really turned off by consumer 1 in a way that breaks things for
> consumer 2. Just masked and the consumer 2 can unmask and use it once
> it probes.
>
> I'm only intimately familiar with clocks, interconnects and regulators
> (to some extent). I'm open to adding other supplier categories in
> future patches as I educate myself of those or if other people want to
> add support for more categories.
>
> -Saravana
>
>>> +#endif
>>> +};
>>> +
>>> +static int of_link_to_suppliers(struct device *dev)
>>> +{
>>> + unsigned int i = 0;
>>> + bool done = true;
>>> +
>>> + if (unlikely(!dev->of_node))
>>> + return 0;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(link_bindings) / 2; i++)
>>> + if (of_link_binding(dev, link_bindings[i * 2],
>>> + link_bindings[i * 2 + 1]))
>>> + done = false;
>>> +
>>> + if (!done)
>>> + return -ENODEV;
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * The following routines scan a subtree and registers a device for
>>> * each applicable node.
>>> @@ -524,6 +575,7 @@ static int __init of_platform_default_populate_init(void)
>>> if (!of_have_populated_dt())
>>> return -ENODEV;
>>>
>>> + platform_bus_type.add_links = of_link_to_suppliers;
>>> /*
>>> * Handle certain compatibles explicitly, since we don't want to create
>>> * platform_devices for every node in /reserved-memory with a
>>> --
>>> 2.22.0.410.gd8fdbe21b5-goog
>>>
>
^ permalink raw reply
* Re: [RFC PATCH v2 00/15] tracing: of: Boot time tracing using devicetree
From: Frank Rowand @ 2019-07-15 14:21 UTC (permalink / raw)
To: Masami Hiramatsu, Steven Rostedt, Rob Herring, Tim Bird
Cc: Ingo Molnar, Namhyung Kim, Jiri Olsa, Arnaldo Carvalho de Melo,
Tom Zanussi, linux-kernel, devicetree
In-Reply-To: <156316746861.23477.5815110570539190650.stgit@devnote2>
Hi Masami,
After receiving this email, I replied to one email on the v1 thread,
so there will be a little bit of overlap in the ordering of the two
threads. Feel free to reply to my comments in the v1 thread in this
thread instead.
More comments below.
On 7/14/19 10:11 PM, Masami Hiramatsu wrote:> Hello,
>
> Here is the 2nd version of RFC series to add boot-time tracing using
> devicetree. Previous thread is here.
>
> https://lkml.kernel.org/r/156113387975.28344.16009584175308192243.stgit@devnote2
>
> In this version, I moved the ftrace node under /chosen/linux,ftrace
> and remove compatible property, because it must be in fixed place.
> Also this version has following features;
>
> - Introduce "instance" node, which can have events nodes for setting
> events filters and actions for the trace instance.
> - Introduce "cpumask" property
> - Introduce "ftrace-filters" and "ftrace-notraces"
> - Introduce "fgraph-filters", "fgraph-notraces" and "fgraph-max-depth"
>
> At this moment, this feature is only available on the architecutre which
> supports devicetree. For x86, we can use it on qemu with --dtb option,
> or apply below patch on grub to add devicetree support on grub-x86.
>
> https://github.com/mhiramat/grub/commit/644c35bfd2d18c772cc353b74215344f8264923a
>
> Note that the devicetree for x86 must contain the nodes only under
> /chosen/, or it may cause a problem if it conflicts with ACPI.
> (Maybe we need to disable the FDT nodes except for nodes under /chosen
> on boot if ACPI exists.)
>
> This series can be applied on Steve's tracing tree (ftrace/core) or
> available on below
>
> https://github.com/mhiramat/linux.git ftrace-devicetree-v2
>
> Usage
> ======
> With this series, we can setup new kprobe and synthetic events, more
> complicated event filters and trigger actions including histogram
> via devicetree.
>
> For example, following kernel parameters
>
> trace_options=sym-addr trace_event=initcall:* tp_printk trace_buf_size=1M ftrace=function ftrace_filter="vfs*"
>
> it can be written in devicetree like below.
>
> /{
> chosen {
> ...
> ftrace {
> options = "sym-addr";
> events = "initcall:*";
> tp-printk;
> buffer-size-kb = <0x400>; // 1024KB == 1MB
> ftrace-filters = "vfs*";
> };
>
> Moreover, now we can expand it to add filters for events, kprobe events,
> and synthetic events with histogram like below.
>
> ftrace {
> ...
> event0 {
> event = "task:task_newtask";
> filter = "pid < 128"; // adding filters
> enable;
> };
> event1 {
> event = "kprobes:vfs_read";
> probes = "vfs_read $arg1 $arg2"; // add kprobes
> filter = "common_pid < 200";
> enable;
> };
> event2 {
> event = "initcall_latency"; // add synth event
> fields = "unsigned long func", "u64 lat";
> // with histogram
> actions = "hist:keys=func.sym,lat:vals=lat:sort=lat";
> };
> // and synthetic event callers
> event3 {
> event = "initcall:initcall_start";
> actions = "hist:keys=func:ts0=common_timestamp.usecs";
> };
> event4 {
> event = "initcall:initcall_finish";
> actions = "hist:keys=func:lat=common_timestamp.usecs-$ts0:onmatch(initcall.initcall_start).initcall_latency(func,$lat)";
> };
> };
>
> Also, this version supports "instance" node, which allows us to
> run several tracers for different purpose at once. For example,
> one tracer is for tracing functions in module alpha, and others
> tracing module beta, you can write followings.
>
> ftrace {
> instance0 {
> instance = "foo";
> tracer = "function";
> ftrace-filters = "*:mod:alpha";
> };
> instance1 {
> instance = "bar";
> tracer = "function";
> ftrace-filters = "*:mod:beta";
>
> };
> };
>
> The instance node also accepts event nodes so that each instance
> can customize its event tracing.
>
> Discussion
> =====
> On the previous thread, we discussed that the this devicetree usage
> itself was acceptable or not. Fortunately, I had a chance to discuss
> it in a F2F meeting with Frank and Tim last week.
Thanks for writing up some of what we discussed.
Let me add a problem statement and use case. I'll probably get it at least
a little bit wrong, so please update as needed.
(1) You feel the ftrace kernel command line syntax is not sufficiently user
friendly.
(2) The kernel command line is too small to contain the full set of desired
ftrace commands and options.
(3) There is a desire to change the boot time ftrace commands and options
without re-compiling or re-linking the Linux kernel.
>
> I think the advantages of using devicetree are,
>
> - reuse devicetree's structured syntax for complicated tracefs settings
> - reuse OF-APIs in linux kernel to accept and parse it
> - reuse dtc complier to compile it and validate syntax. (with yaml schema,
> we can enhance it)
> - reuse current bootloader (and qemu) to load it
Devicetree is not a universal data structure and communication channel.
Devicetree is a description of the hardware and also conveys bootloader
specific information that is need by the kernel to boot.
> And we talked about some other ideas to avoid using devicetree.
>
> - expand kernel command line (ascii command strings)
> - expand kernel command line with base64 encoded comressed ascii command
> strings
Base64 being one of possibly many ways to convert arbitrary binary data to
ascii safe data _if_ you want to transfer the ftrace options and commands
in a binary format.
> - load (compressed) ascii command strings to somewhere on memory and pass
> the address via kernel cmdline
Similar to the way initrd is handled, if I understand correctly. (I am not
up to date on how initrd location is passed to the kernel for a non-devicetree
kernel.)
Compressed or not compressed would be an ftrace design choice.
> - load (compressed) ascii command strings to somewhere on memory and pass
> the address via /chosen node (as same as initrd)
Compressed or not compressed would be an ftrace design choice.
> - load binary C data and point it from kernel cmdline
> - load binary C data and point it from /chosen node (as same as initrd)
> - load binary C data as a section of kernel image
For the three options above:
Binary data if ftrace prefers structured data.
A list of strings if ftrace wants to use the existing kernel command line
syntax.
For the third of the above three options, the linker would provide the start
and end address of the ftrace options and commands section.
> The first 2 ideas expand the kernel's cmdline to pass some "magic" command
> to setup ftrace. In both case, the problems are the maximal size of cmdline
> and the issues related to the complexity of commands.
Not a "magic" command. Either continue using the existing ftrace syntax or
add something like: ftrace_cmd="whatever format ftrace desires".
Why can the maximum size of the cmdline not be increased?
> My example showed that the ftrace settings becomes long even if making one
> histogram, which can be longer than 256 bytes. The long and complex data
> can easily lead mis-typing, but cmdline has no syntax validator, it just
> ignores the mis-typed commands.
Hand typing a kernel command line is already not a fun exercise, even
before adding ftrace commands. If you are hand typing kernel command
lines then I suggest you improve your tools (eg bootloader or whatever
is not allowing you to edit and store command lines).
> (Of course even with the devicetree, it must be smaller than 2 pages)
>
> Next 2 ideas are similar, but load the commands on some other memory area
> and pass only address via cmdline. This solves the size limitation issue,
> but still no syntax validation. Of course we can make a new structured
> syntax validator similar to (or just forked from) dt-validate.
> The problem (or disadvantage) of these (and following) ideas, is to change
> the kernel and boot loaders to load another binary blobs on memory.
>
> Maybe if we introduce a generic structured kernel boot arguments, which is
> a kind of /chosen node of devicetree. (But if there is already such hook,
> why we make another one...?)
I got lost in the next sentence, so for my benefit:
GSKBA == generic structured kernel boot arguments
> Also, this "GSKBA" may introduce a parser and access APIs which will be
> very similar to OF-APIs. This also seems redundant to me.
> So the last 3 ideas will avoid introducing new parser and APIs, we just
> compile the data as C data and point it from cmdline or somewhere else.
Or if in a kernel data section then the linker can provide the begin and
end address of the blob. This is already implemented for some other data
structures.
> With these ideas, we still need to expand boot loaders to support
> loading new binary blobs. (And the last one requires to add elf header
> parser/modifier to boot loader too)
Why would the boot loader need to access the elf header? The linker
can provide the location of the new kernel data section via kernel
variables.
>
>>From the above reasons, I think using devicetree's /chosen node is
> the least intrusive way to introduce this boot-time tracing feature.
This is still mis-use of the devicetree data structure. This data
does not belong in the devicetree.
>
> Any suggestions, thoughts?
>
> Thank you,
>
> ---
>
> Masami Hiramatsu (15):
> tracing: Apply soft-disabled and filter to tracepoints printk
> tracing: kprobes: Output kprobe event to printk buffer
> tracing: Expose EXPORT_SYMBOL_GPL symbol
> tracing: kprobes: Register to dynevent earlier stage
> tracing: Accept different type for synthetic event fields
> tracing: Add NULL trace-array check in print_synth_event()
> dt-bindings: tracing: Add ftrace binding document
> tracing: of: Add setup tracing by devicetree support
> tracing: of: Add trace event settings
> tracing: of: Add kprobe event support
> tracing: of: Add synthetic event support
> tracing: of: Add instance node support
> tracing: of: Add cpumask property support
> tracing: of: Add function tracer filter properties
> tracing: of: Add function-graph tracer option properties
>
>
> .../devicetree/bindings/chosen/linux,ftrace.yaml | 306 ++++++++++++
> include/linux/trace_events.h | 1
> kernel/trace/Kconfig | 10
> kernel/trace/Makefile | 1
> kernel/trace/ftrace.c | 85 ++-
> kernel/trace/trace.c | 90 ++--
> kernel/trace/trace_events.c | 3
> kernel/trace/trace_events_hist.c | 14 -
> kernel/trace/trace_events_trigger.c | 2
> kernel/trace/trace_kprobe.c | 81 ++-
> kernel/trace/trace_of.c | 507 ++++++++++++++++++++
> 11 files changed, 1004 insertions(+), 96 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/chosen/linux,ftrace.yaml
> create mode 100644 kernel/trace/trace_of.c
>
> --
> Masami Hiramatsu (Linaro) <mhiramat@kernel.org>
>
^ permalink raw reply
* Re: [PATCH v4 1/5] dt-bindings: interconnect: Add Qualcomm QCS404 DT bindings
From: Georgi Djakov @ 2019-07-15 13:56 UTC (permalink / raw)
To: Bjorn Andersson
Cc: robh+dt, agross, vkoul, evgreen, daidavid1, linux-pm, devicetree,
linux-kernel, linux-arm-msm
In-Reply-To: <20190708192612.GE30636@minitux>
Hi Bjorn,
On 7/8/19 22:26, Bjorn Andersson wrote:
> On Thu 13 Jun 08:13 PDT 2019, Georgi Djakov wrote:
>
>> The Qualcomm QCS404 platform has several buses that could be controlled
>> and tuned according to the bandwidth demand.
>>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>>
>> v4:
>> - Add the DT header into this patch.
>> - Pick Bjorn's r-b.
>>
>> v3:
>> - Add a reg property and move the interconnect nodes under the "soc" node.
>>
>> v2:
>> - No changes.
>>
>> .../bindings/interconnect/qcom,qcs404.txt | 46 ++++++++++
>> .../dt-bindings/interconnect/qcom,qcs404.h | 88 +++++++++++++++++++
>> 2 files changed, 134 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>> create mode 100644 include/dt-bindings/interconnect/qcom,qcs404.h
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>> new file mode 100644
>> index 000000000000..14a827268dda
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,qcs404.txt
>> @@ -0,0 +1,46 @@
>> +Qualcomm QCS404 Network-On-Chip interconnect driver binding
>> +-----------------------------------------------------------
>> +
>> +Required properties :
>> +- compatible : shall contain only one of the following:
>> + "qcom,qcs404-bimc"
>> + "qcom,qcs404-pcnoc"
>> + "qcom,qcs404-snoc"
>> +- #interconnect-cells : should contain 1
>> +
>> +Optional properties :
>> +reg : specifies the physical base address and size of registers
>> +clocks : list of phandles and specifiers to all interconnect bus clocks
>> +clock-names : clock names should include both "bus_clk" and "bus_a_clk"
>
> Spoke to Rob about this patch, and I don't think these properties should
> not be described as optional.
>
> The reg isn't used unless we're implementing support for QoS, but let's
> include them in the binding as required anyways.
Ok, will do it!
>
> Iirc the two clocks are required with the current implementation, but
> shouldn't there be an iface clock as well, for accessing the QoS
> register space?
Actually i expect this to be a list of AXI clocks of the IP blocks, whose QoS
ports we configure. For this platform, according to downstream, it would be just
<&gcc GCC_SYS_NOC_USB3_CLK>. For other platforms i see these are a few usb/ufs
clocks that should be enabled while we set QoS for usb/ufs.
>
> PS. As I read this again, please drop _clk from the two clocks names -
> we know they are clocks...
Sure, will do it!
Thanks,
Georgi
>
> Regards,
> Bjorn
>
>> +
>> +Example:
>> +
>> +soc {
>> + ...
>> + bimc: interconnect@400000 {
>> + reg = <0x00400000 0x80000>;
>> + compatible = "qcom,qcs404-bimc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_BIMC_CLK>,
>> + <&rpmcc RPM_SMD_BIMC_A_CLK>;
>> + };
>> +
>> + pnoc: interconnect@500000 {
>> + reg = <0x00500000 0x15080>;
>> + compatible = "qcom,qcs404-pcnoc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_PNOC_CLK>,
>> + <&rpmcc RPM_SMD_PNOC_A_CLK>;
>> + };
>> +
>> + snoc: interconnect@580000 {
>> + reg = <0x00580000 0x23080>;
>> + compatible = "qcom,qcs404-snoc";
>> + #interconnect-cells = <1>;
>> + clock-names = "bus_clk", "bus_a_clk";
>> + clocks = <&rpmcc RPM_SMD_SNOC_CLK>,
>> + <&rpmcc RPM_SMD_SNOC_A_CLK>;
>> + };
>> +};
>> diff --git a/include/dt-bindings/interconnect/qcom,qcs404.h b/include/dt-bindings/interconnect/qcom,qcs404.h
>> new file mode 100644
>> index 000000000000..960f6e39c5f2
>> --- /dev/null
>> +++ b/include/dt-bindings/interconnect/qcom,qcs404.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Qualcomm interconnect IDs
>> + *
>> + * Copyright (c) 2019, Linaro Ltd.
>> + * Author: Georgi Djakov <georgi.djakov@linaro.org>
>> + */
>> +
>> +#ifndef __DT_BINDINGS_INTERCONNECT_QCOM_QCS404_H
>> +#define __DT_BINDINGS_INTERCONNECT_QCOM_QCS404_H
>> +
>> +#define MASTER_AMPSS_M0 0
>> +#define MASTER_OXILI 1
>> +#define MASTER_MDP_PORT0 2
>> +#define MASTER_SNOC_BIMC_1 3
>> +#define MASTER_TCU_0 4
>> +#define SLAVE_EBI_CH0 5
>> +#define SLAVE_BIMC_SNOC 6
>> +
>> +#define MASTER_SPDM 0
>> +#define MASTER_BLSP_1 1
>> +#define MASTER_BLSP_2 2
>> +#define MASTER_XI_USB_HS1 3
>> +#define MASTER_CRYPT0 4
>> +#define MASTER_SDCC_1 5
>> +#define MASTER_SDCC_2 6
>> +#define MASTER_SNOC_PCNOC 7
>> +#define MASTER_QPIC 8
>> +#define PCNOC_INT_0 9
>> +#define PCNOC_INT_2 10
>> +#define PCNOC_INT_3 11
>> +#define PCNOC_S_0 12
>> +#define PCNOC_S_1 13
>> +#define PCNOC_S_2 14
>> +#define PCNOC_S_3 15
>> +#define PCNOC_S_4 16
>> +#define PCNOC_S_6 17
>> +#define PCNOC_S_7 18
>> +#define PCNOC_S_8 19
>> +#define PCNOC_S_9 20
>> +#define PCNOC_S_10 21
>> +#define PCNOC_S_11 22
>> +#define SLAVE_SPDM 23
>> +#define SLAVE_PDM 24
>> +#define SLAVE_PRNG 25
>> +#define SLAVE_TCSR 26
>> +#define SLAVE_SNOC_CFG 27
>> +#define SLAVE_MESSAGE_RAM 28
>> +#define SLAVE_DISP_SS_CFG 29
>> +#define SLAVE_GPU_CFG 30
>> +#define SLAVE_BLSP_1 31
>> +#define SLAVE_BLSP_2 32
>> +#define SLAVE_TLMM_NORTH 33
>> +#define SLAVE_PCIE 34
>> +#define SLAVE_ETHERNET 35
>> +#define SLAVE_TLMM_EAST 36
>> +#define SLAVE_TCU 37
>> +#define SLAVE_PMIC_ARB 38
>> +#define SLAVE_SDCC_1 39
>> +#define SLAVE_SDCC_2 40
>> +#define SLAVE_TLMM_SOUTH 41
>> +#define SLAVE_USB_HS 42
>> +#define SLAVE_USB3 43
>> +#define SLAVE_CRYPTO_0_CFG 44
>> +#define SLAVE_PCNOC_SNOC 45
>> +
>> +#define MASTER_QDSS_BAM 0
>> +#define MASTER_BIMC_SNOC 1
>> +#define MASTER_PCNOC_SNOC 2
>> +#define MASTER_QDSS_ETR 3
>> +#define MASTER_EMAC 4
>> +#define MASTER_PCIE 5
>> +#define MASTER_USB3 6
>> +#define QDSS_INT 7
>> +#define SNOC_INT_0 8
>> +#define SNOC_INT_1 9
>> +#define SNOC_INT_2 10
>> +#define SLAVE_KPSS_AHB 11
>> +#define SLAVE_WCSS 12
>> +#define SLAVE_SNOC_BIMC_1 13
>> +#define SLAVE_IMEM 14
>> +#define SLAVE_SNOC_PCNOC 15
>> +#define SLAVE_QDSS_STM 16
>> +#define SLAVE_CATS_0 17
>> +#define SLAVE_CATS_1 18
>> +#define SLAVE_LPASS 19
>> +
>> +#endif
^ permalink raw reply
* Re: [RFC PATCH 00/11] tracing: of: Boot time tracing using devicetree
From: Frank Rowand @ 2019-07-15 13:55 UTC (permalink / raw)
To: Masami Hiramatsu
Cc: Steven Rostedt, Rob Herring, Tom Zanussi, Ingo Molnar,
Namhyung Kim, Jiri Olsa, Arnaldo Carvalho de Melo, linux-kernel,
devicetree
In-Reply-To: <20190625140004.a74443238596b297a558a66f@kernel.org>
Hi Masami,
Note that Masami has submitted v2 of the patch with further discussion. I
noticed that I had left some comments unanswered in this thread, so I am
doing this reply before moving on to reply to the v2 thread.
Feel free to move the discussion to v2 instead of replying to this email
if you find that easier.
On 6/24/19 10:00 PM, Masami Hiramatsu wrote:
> Hi Frank,
>
> On Mon, 24 Jun 2019 15:31:07 -0700
> Frank Rowand <frowand.list@gmail.com> wrote:
>>>>> Currently, kernel support boot-time tracing using kernel command-line
>>>>> parameters. But that is very limited because of limited expressions
>>>>> and limited length of command line. Recently, useful features like
>>>>> histogram, synthetic events, etc. are being added to ftrace, but it is
>>>>> clear that we can not expand command-line options to support these
>>>>> features.
>>>>
>>>> "it is clear that we can not expand command-line options" needs a fuller
>>>> explanation. And maybe further exploration.
>>>
>>> Indeed. As an example of tracing settings in the first mail, even for simple
>>> use-case, the trace command is long and complicated. I think it is hard to
>>> express that as 1-liner kernel command line. But devicetree looks very good
>>> for expressing structured data. That is great and I like it :)
>>
>> But you could extend the command line paradigm to meet your needs.
>
> But the kernel command line is a command line. Would you mean encoding the
> structured setting in binary format with base64 and pass it? :(
If you want to put structured data in the command line, then yes, you could use
an ascii safe form, such as base64, to contain it. If that is what you want.
>
>>>> Devicetree is NOT for configuration information. This has been discussed
>>>> over and over again in mail lists, at various conferences, and was also an
>>>> entire session at plumbers a few years ago:
>>>>
>>>> https://elinux.org/Device_tree_future#Linux_Plumbers_2016_Device_Tree_Track
>>>
>>> Thanks, I'll check that.
>
> I found following discussion in etherpad log, https://elinux.org/Device_tree_plumbers_2016_etherpad
> ----
> If you have data that the kernel does not have a good way to get, that's OK to put into DT.
>
> Operating points are OK - but should still be structured well.
> ----
>
> This sounds like if it is structured well, and there are no other way,
> we will be able to use DT as a channel.
>
>>>>
>>>> There is one part of device tree that does allow non-hardware description,
>>>> which is the "chosen" node which is provided to allow communication between
>>>> the bootloader and the kernel.
>>>
>>> Ah, "chosen" will be suit for me :)
>>
>> No. This is not communicating boot loader information.
>
> Hmm, it's a kind of communication with the operator of the boot loader, since there
> is an admin or developer behind it. I think the comminication is to communicate
> with that human. Then if they intend to trace boot process, that is a kind of
> communication.
The quote from the plumbers 2016 devicetree etherpad ("Operating points are OK ...)
conveniently ignores a sentence just a few lines later:
"If firmware is deciding the operating point, then it's OK to convey it via DT."
The operating points example is clearly communicating boot loader information to
the kernel.
Something the admin or developer wants to communicate is not boot loader
information.
>
> [...]
>>>>> - Can we use devicetree for configuring kernel dynamically?
>>>>
>>>> No. Sorry.
>>>>
>>>> My understanding of this proposal is that it is intended to better
>>>> support boot time kernel and driver debugging. As an alternate
>>>> implementation, could you compile the ftrace configuration information
>>>> directly into a kernel data structure? It seems like it would not be
>>>> very difficult to populate the data structure data via a few macros.
>>>
>>> No, that is not what I intended. My intention was to trace boot up
>>> process "without recompiling kernel", but with a structured data.
>>
>> That is debugging. Or if you want to be pedantic, a complex performance
>> measurement of the boot process (more than holding a stopwatch in your
>> hand).
>
> Yeah, that's right.
>
>> Recompiling a single object file (containing the ftrace command data)
>> and re-linking the kernel is not a big price in that context).
>
> No, if I can use DT, I can choose one of them while boot up.
> That will be a big difference.
> (Of course for that purpose, I should work on boot loader to support
> DT overlay)
This is debugging kernel drivers. I do not think that it is a big cost for
a kernel developer to re-link the kernel. On any reasonable modern
development system this should be a matter of seconds, not minutes.
Compiling a devicetree source is not significantly less time. (To be
fair, you imply that you would have various compiled devicetrees to
choose from at boot time. It may be realistic to have a library of
ftrace commands, or it may be more realistic that someone debugging
a kernel driver will create a unique ftrace command set for the
particular case they are debugging.)
>
>> Or if
>> you create a new communication channel, you will have the cost of
>> creating that data object (certainly not much different than compiling
>> a devicetree) and have the bootloader provide the ftrace data object
>> to the kernel.
>
> Yes, and for me, that sounds like just a reinvention of the wheel.
> If I can reuse devicetree infrastructure, it is easily done (as I
> implemented in this series. It's just about 500LOC (and YAML document)
Or you can just use the existing ftrace boot command line syntax.
>
> I can clone drivers/of/ code only for that new communication channel,
> but that makes no one happy. :(
>
>>> For such purpose, we have to implement a tool to parse and pack the
>>> data and a channel to load it at earlier stage in bootloader. And
>>> those are already done by devicetree. Thus I thought I could get a
>>> piggyback on devicetree.
>>
>> Devicetree is not the universal dumping ground for communicating
>> information to a booting kernel. Please create another communication
>> channel.
>
> Why should we so limit the availability of even a small corner of existing
> open source software...?
Because the proposal is a mis-use of devicetree.
>
> Thank you,
>
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: mmc: Document Aspeed SD controller
From: Rob Herring @ 2019-07-15 13:26 UTC (permalink / raw)
To: Andrew Jeffery
Cc: Maxime Ripard, linux-mmc, Ulf Hansson, Mark Rutland, Joel Stanley,
Adrian Hunter, devicetree,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-aspeed, linux-kernel@vger.kernel.org, Ryan Chen
In-Reply-To: <5c831fd3-d0e2-474b-8a6e-8f51f92fbdf8@www.fastmail.com>
On Sun, Jul 14, 2019 at 8:30 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Fri, 12 Jul 2019, at 22:41, Maxime Ripard wrote:
> > Hi,
> >
> > On Fri, Jul 12, 2019 at 01:02:13PM +0930, Andrew Jeffery wrote:
> > > The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the
> > > SDIO Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit
> > > data bus if only a single slot is enabled.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > > In v2:
> > >
> > > * Rename to aspeed,sdhci.yaml
> > > * Rename sd-controller compatible
> > > * Add `maxItems: 1` for reg properties
> > > * Move sdhci subnode description to patternProperties
> > > * Drop sdhci compatible requirement
> > > * #address-cells and #size-cells are required
> > > * Prevent additional properties
> > > * Implement explicit ranges in example
> > > * Remove slot property
> > >
> > > .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 90 +++++++++++++++++++
> > > 1 file changed, 90 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > new file mode 100644
> > > index 000000000000..67a691c3348c
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -0,0 +1,90 @@
> > > +# SPDX-License-Identifier: GPL-2.0-or-later
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mmc/aspeed,sdhci.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ASPEED SD/SDIO/eMMC Controller
> > > +
> > > +maintainers:
> > > + - Andrew Jeffery <andrew@aj.id.au>
> > > + - Ryan Chen <ryanchen.aspeed@gmail.com>
> > > +
> > > +description: |+
> > > + The ASPEED SD/SDIO/eMMC controller exposes two slots implementing the SDIO
> > > + Host Specification v2.00, with 1 or 4 bit data buses, or an 8 bit data bus if
> > > + only a single slot is enabled.
> > > +
> > > + The two slots are supported by a common configuration area. As the SDHCIs for
> > > + the slots are dependent on the common configuration area, they are described
> > > + as child nodes.
> > > +
> > > +properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sd-controller, aspeed,ast2500-sd-controller ]
> > > + reg:
> > > + maxItems: 1
> > > + description: Common configuration registers
> > > + ranges: true
> > > + clocks:
> > > + maxItems: 1
> > > + description: The SD/SDIO controller clock gate
> >
> > #address-cells and #size-cells have not been described here.
> >
> > > +patternProperties:
> > > + "^sdhci@[0-9a-f]+$":
> > > + type: object
> > > + properties:
> > > + compatible:
> > > + enum: [ aspeed,ast2400-sdhci, aspeed,ast2500-sdhci ]
> > > + reg:
> > > + maxItems: 1
> > > + description: The SDHCI registers
> > > + clocks:
> > > + maxItems: 1
> > > + description: The SD bus clock
> > > + interrupts:
> > > + maxItems: 1
> > > + description: The SD interrupt shared between both slots
> > > + required:
> > > + - compatible
> > > + - reg
> > > + - clocks
> > > + - interrupts
> > > +
> > > +additionalProperties: false
> >
> > But that means that it will generate a warning in your DT if you ever
> > use them.
> >
> > > +required:
> > > + - compatible
> > > + - reg
> > > + - "#address-cells"
> > > + - "#size-cells"
> > > + - ranges
> > > + - clocks
> > > +
> > > +examples:
> > > + - |
> > > + #include <dt-bindings/clock/aspeed-clock.h>
> > > + sdc@1e740000 {
> > > + compatible = "aspeed,ast2500-sd-controller";
> > > + reg = <0x1e740000 0x100>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> >
> > Starting with your example.
>
> Heh, right. Thanks. I was inspecting the output of the `dt_binding_check` and
> `dtbs_check` make targets, though maybe I overlooked this. The aspeed dtsis
> do generate a quite a number of warnings which make it hard to parse, so I'm
> going to send a series to clean that up too.
FYI, This will run checks with only the schema file you specify:
make dtbs_check DT_SCHEMA_FILES=path/to/schema/file
Rob
^ permalink raw reply
* [PATCH v3 4/4] edac: Add support for Amazon's Annapurna Labs L2 EDAC
From: Hanna Hawa @ 2019-07-15 13:24 UTC (permalink / raw)
To: robh+dt, mark.rutland, bp, mchehab, james.morse, davem, gregkh,
linus.walleij, Jonathan.Cameron, nicolas.ferre, paulmck
Cc: dwmw, benh, ronenk, talel, jonnyc, hanochu, devicetree,
linux-kernel, linux-edac, hhhawa
In-Reply-To: <1563197049-12679-1-git-send-email-hhhawa@amazon.com>
Adds support for Amazon's Annapurna Labs L2 EDAC driver to detect and
report L2 errors.
Signed-off-by: Hanna Hawa <hhhawa@amazon.com>
---
MAINTAINERS | 6 ++
drivers/edac/Kconfig | 8 ++
drivers/edac/Makefile | 1 +
drivers/edac/al_l2_edac.c | 187 ++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 202 insertions(+)
create mode 100644 drivers/edac/al_l2_edac.c
diff --git a/MAINTAINERS b/MAINTAINERS
index fd29ea6..a6dcf3d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -749,6 +749,12 @@ S: Maintained
F: drivers/edac/al_l1_edac.c
F: Documentation/devicetree/bindings/edac/amazon,al-l1-edac.txt
+AMAZON ANNAPURNA LABS L2 EDAC
+M: Hanna Hawa <hhhawa@amazon.com>
+S: Maintained
+F: drivers/edac/al_l2_edac.c
+F: Documentation/devicetree/bindings/edac/amazon,al-l2-edac.txt
+
AMAZON ANNAPURNA LABS THERMAL MMIO DRIVER
M: Talel Shenhar <talel@amazon.com>
S: Maintained
diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 58b92bc..8bbb745 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -82,6 +82,14 @@ config EDAC_AL_L1
for Amazon's Annapurna Labs SoCs.
This driver detects errors of L1 caches.
+config EDAC_AL_L2
+ bool "Amazon's Annapurna Labs L2 EDAC"
+ depends on ARCH_ALPINE
+ help
+ Support for L2 error detection and correction
+ for Amazon's Annapurna Labs SoCs.
+ This driver detects errors of L2 caches.
+
config EDAC_AMD64
tristate "AMD64 (Opteron, Athlon64)"
depends on AMD_NB && EDAC_DECODE_MCE
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index caa2dc9..60a6b8b 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -23,6 +23,7 @@ edac_mce_amd-y := mce_amd.o
obj-$(CONFIG_EDAC_DECODE_MCE) += edac_mce_amd.o
obj-$(CONFIG_EDAC_AL_L1) += al_l1_edac.o
+obj-$(CONFIG_EDAC_AL_L2) += al_l2_edac.o
obj-$(CONFIG_EDAC_AMD76X) += amd76x_edac.o
obj-$(CONFIG_EDAC_CPC925) += cpc925_edac.o
obj-$(CONFIG_EDAC_I5000) += i5000_edac.o
diff --git a/drivers/edac/al_l2_edac.c b/drivers/edac/al_l2_edac.c
new file mode 100644
index 0000000..bae1ea9
--- /dev/null
+++ b/drivers/edac/al_l2_edac.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/of.h>
+
+#include "edac_device.h"
+#include "edac_module.h"
+
+#define DRV_NAME "al_l2_edac"
+
+/* Same bit assignments of L2MERRSR_EL1 in ARM CA57/CA72 */
+#define ARM_CA57_L2MERRSR_EL1 sys_reg(3, 1, 15, 2, 3)
+#define ARM_CA57_L2MERRSR_RAMID GENMASK(30, 24)
+#define ARM_CA57_L2_TAG_RAM 0x10
+#define ARM_CA57_L2_DATA_RAM 0x11
+#define ARM_CA57_L2_SNOOP_RAM 0x12
+#define ARM_CA57_L2_DIRTY_RAM 0x14
+#define ARM_CA57_L2_INC_PF_RAM 0x18
+#define ARM_CA57_L2MERRSR_VALID BIT(31)
+#define ARM_CA57_L2MERRSR_REPEAT GENMASK_ULL(39, 32)
+#define ARM_CA57_L2MERRSR_OTHER GENMASK_ULL(47, 40)
+#define ARM_CA57_L2MERRSR_FATAL BIT_ULL(63)
+
+#define AL_L2_EDAC_MSG_MAX 256
+
+struct al_l2_edac {
+ cpumask_t cluster_cpus;
+};
+
+static void al_l2_edac_l2merrsr(void *arg)
+{
+ struct edac_device_ctl_info *edac_dev = arg;
+ int cpu, i;
+ u32 ramid, repeat, other, fatal;
+ u64 val = read_sysreg_s(ARM_CA57_L2MERRSR_EL1);
+ char msg[AL_L2_EDAC_MSG_MAX];
+ int space, count;
+ char *p;
+
+ if (!(FIELD_GET(ARM_CA57_L2MERRSR_VALID, val)))
+ return;
+
+ cpu = smp_processor_id();
+ ramid = FIELD_GET(ARM_CA57_L2MERRSR_RAMID, val);
+ repeat = FIELD_GET(ARM_CA57_L2MERRSR_REPEAT, val);
+ other = FIELD_GET(ARM_CA57_L2MERRSR_OTHER, val);
+ fatal = FIELD_GET(ARM_CA57_L2MERRSR_FATAL, val);
+
+ space = sizeof(msg);
+ p = msg;
+ count = snprintf(p, space, "CPU%d L2 %serror detected", cpu,
+ (fatal) ? "Fatal " : "");
+ p += count;
+ space -= count;
+
+ switch (ramid) {
+ case ARM_CA57_L2_TAG_RAM:
+ count = snprintf(p, space, " RAMID='L2 Tag RAM'");
+ break;
+ case ARM_CA57_L2_DATA_RAM:
+ count = snprintf(p, space, " RAMID='L2 Data RAM'");
+ break;
+ case ARM_CA57_L2_SNOOP_RAM:
+ count = snprintf(p, space, " RAMID='L2 Snoop RAM'");
+ break;
+ case ARM_CA57_L2_DIRTY_RAM:
+ count = snprintf(p, space, " RAMID='L2 Dirty RAM'");
+ break;
+ case ARM_CA57_L2_INC_PF_RAM:
+ count = snprintf(p, space, " RAMID='L2 internal metadat'");
+ break;
+ default:
+ count = snprintf(p, space, " RAMID='unknown'");
+ break;
+ }
+
+ p += count;
+ space -= count;
+
+ count = snprintf(p, space,
+ " repeat=%d, other=%d (L2MERRSR_EL1=0x%llx)",
+ repeat, other, val);
+
+ for (i = 0; i < repeat; i++) {
+ if (fatal)
+ edac_device_handle_ue(edac_dev, 0, 0, msg);
+ else
+ edac_device_handle_ce(edac_dev, 0, 0, msg);
+ }
+
+ write_sysreg_s(0, ARM_CA57_L2MERRSR_EL1);
+}
+
+static void al_l2_edac_check(struct edac_device_ctl_info *edac_dev)
+{
+ struct al_l2_edac *al_l2 = edac_dev->pvt_info;
+
+ smp_call_function_any(&al_l2->cluster_cpus, al_l2_edac_l2merrsr,
+ edac_dev, 1);
+}
+
+static int al_l2_edac_probe(struct platform_device *pdev)
+{
+ struct edac_device_ctl_info *edac_dev;
+ struct al_l2_edac *al_l2;
+ struct device *dev = &pdev->dev;
+ int ret, i;
+
+ edac_dev = edac_device_alloc_ctl_info(sizeof(*al_l2),
+ (char *)dev_name(dev), 1, "L", 1,
+ 2, NULL, 0,
+ edac_device_alloc_index());
+ if (IS_ERR(edac_dev))
+ return -ENOMEM;
+
+ al_l2 = edac_dev->pvt_info;
+ edac_dev->edac_check = al_l2_edac_check;
+ edac_dev->dev = dev;
+ edac_dev->mod_name = DRV_NAME;
+ edac_dev->dev_name = dev_name(dev);
+ edac_dev->ctl_name = "L2 cache";
+ platform_set_drvdata(pdev, edac_dev);
+
+ for_each_online_cpu(i) {
+ struct device_node *cpu;
+ struct device_node *cpu_cache, *l2_cache;
+
+ cpu = of_get_cpu_node(i, NULL);
+ cpu_cache = of_find_next_cache_node(cpu);
+ l2_cache = of_parse_phandle(dev->of_node, "l2-cache", 0);
+
+ if (cpu_cache == l2_cache)
+ cpumask_set_cpu(i, &al_l2->cluster_cpus);
+ }
+
+ if (cpumask_empty(&al_l2->cluster_cpus)) {
+ dev_err(dev, "CPU mask is empty for this L2 cache\n");
+ ret = -EINVAL;
+ goto err;
+ }
+
+ ret = edac_device_add_device(edac_dev);
+ if (ret) {
+ dev_err(dev, "Failed to add L2 edac device\n");
+ goto err;
+ }
+
+ return 0;
+
+err:
+ edac_device_free_ctl_info(edac_dev);
+
+ return ret;
+}
+
+static int al_l2_edac_remove(struct platform_device *pdev)
+{
+ struct edac_device_ctl_info *edac_dev = platform_get_drvdata(pdev);
+
+ edac_device_del_device(edac_dev->dev);
+ edac_device_free_ctl_info(edac_dev);
+
+ return 0;
+}
+
+static const struct of_device_id al_l2_edac_of_match[] = {
+ { .compatible = "amazon,al-l2-edac" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, al_l2_edac_of_match);
+
+static struct platform_driver al_l2_edac_driver = {
+ .probe = al_l2_edac_probe,
+ .remove = al_l2_edac_remove,
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = al_l2_edac_of_match,
+ },
+};
+module_platform_driver(al_l2_edac_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Hanna Hawa <hhhawa@amazon.com>");
+MODULE_DESCRIPTION("Amazon's Annapurna Lab's L2 EDAC Driver");
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox