Hi everyone,
the OPP table on am625 currently uses a syscon node to get required
information from efuse registers. As efuse registers contain many
different information, this series adds nvmem support for the TI OPP
table and cpufreq driver. This way just the specific information can be
referenced in the devicetree without the need to use a syscon reference.
The nvmem layout is added in my previous series, links are below.
This series is based on
https://lore.kernel.org/linux-arm-kernel/[email protected]/
Which is also available on my public git:
https://gitlab.baylibre.com/msp8/linux/-/tree/topic/ti-chipid-nvmem/v6.8?ref_type=heads
This series is available on git as well:
https://gitlab.baylibre.com/msp8/linux/-/tree/topic/ti-cpufreq-nvmem/v6.8?ref_type=heads
Best,
Markus
Markus Schneider-Pargmann (3):
dt-bindings: cpufreq: Add nvmem-cells for chip information
cpufreq: ti-cpufreq: Support nvmem for chip version
arm64: dts: ti: k3-am625: Use nvmem-cells for opp
.../opp/operating-points-v2-ti-cpu.yaml | 16 ++-
arch/arm64/boot/dts/ti/k3-am625.dtsi | 2 +
drivers/cpufreq/ti-cpufreq.c | 105 +++++++++++-------
3 files changed, 83 insertions(+), 40 deletions(-)
--
2.43.0
Support using nvmem-cells 'chipvariant' and 'chipspeed' instead of
syscon. This makes it more flexible and moves more configuration into
the devicetree.
If nvmem-cells are present, probing will fail if the configuration of
these cells is broken. If nvmem-cells is not present syscon will be
used.
Signed-off-by: Markus Schneider-Pargmann <[email protected]>
---
drivers/cpufreq/ti-cpufreq.c | 105 ++++++++++++++++++++++-------------
1 file changed, 66 insertions(+), 39 deletions(-)
diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
index 46c41e2ca727..3ee72b1309f0 100644
--- a/drivers/cpufreq/ti-cpufreq.c
+++ b/drivers/cpufreq/ti-cpufreq.c
@@ -10,6 +10,7 @@
#include <linux/io.h>
#include <linux/mfd/syscon.h>
#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
#include <linux/init.h>
#include <linux/of.h>
#include <linux/platform_device.h>
@@ -65,6 +66,7 @@ struct ti_cpufreq_soc_data {
struct ti_cpufreq_data {
struct device *cpu_dev;
+ struct device *dev;
struct device_node *opp_node;
struct regmap *syscon;
const struct ti_cpufreq_soc_data *soc_data;
@@ -244,31 +246,40 @@ static struct ti_cpufreq_soc_data am625_soc_data = {
static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
u32 *efuse_value)
{
+ struct device_node *np = opp_data->opp_node;
struct device *dev = opp_data->cpu_dev;
u32 efuse;
int ret;
- ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
- &efuse);
- if (ret == -EIO) {
- /* not a syscon register! */
- void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
- opp_data->soc_data->efuse_offset, 4);
-
- if (!regs)
- return -ENOMEM;
- efuse = readl(regs);
- iounmap(regs);
+ ret = nvmem_cell_read_u32(opp_data->dev, "chipspeed", &efuse);
+ if (ret && (ret != -ENOENT || !opp_data->syscon))
+ return dev_err_probe(dev, ret,
+ "Failed to read nvmem cell 'chipspeed': %pe",
+ ERR_PTR(ret));
+
+ if (ret) {
+ ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
+ &efuse);
+ if (ret == -EIO) {
+ /* not a syscon register! */
+ void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
+ opp_data->soc_data->efuse_offset, 4);
+
+ if (!regs)
+ return -ENOMEM;
+ efuse = readl(regs);
+ iounmap(regs);
+ }
+ else if (ret) {
+ dev_err(dev,
+ "Failed to read the efuse value from syscon: %d\n",
+ ret);
+ return ret;
}
- else if (ret) {
- dev_err(dev,
- "Failed to read the efuse value from syscon: %d\n",
- ret);
- return ret;
- }
- efuse = (efuse & opp_data->soc_data->efuse_mask);
- efuse >>= opp_data->soc_data->efuse_shift;
+ efuse = (efuse & opp_data->soc_data->efuse_mask);
+ efuse >>= opp_data->soc_data->efuse_shift;
+ }
*efuse_value = opp_data->soc_data->efuse_xlate(opp_data, efuse);
@@ -285,30 +296,41 @@ static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
static int ti_cpufreq_get_rev(struct ti_cpufreq_data *opp_data,
u32 *revision_value)
{
+ struct device_node *np = opp_data->opp_node;
struct device *dev = opp_data->cpu_dev;
u32 revision;
int ret;
- ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
- &revision);
- if (ret == -EIO) {
- /* not a syscon register! */
- void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
- opp_data->soc_data->rev_offset, 4);
-
- if (!regs)
- return -ENOMEM;
- revision = readl(regs);
- iounmap(regs);
+ ret = nvmem_cell_read_u32(opp_data->dev, "chipvariant", &revision);
+ if (ret && (ret != -ENOENT || !opp_data->syscon))
+ return dev_err_probe(dev, ret,
+ "Failed to read nvmem cell 'chipvariant': %pe",
+ ERR_PTR(ret));
+
+ if (ret) {
+ ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
+ &revision);
+ if (ret == -EIO) {
+ /* not a syscon register! */
+ void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
+ opp_data->soc_data->rev_offset, 4);
+
+ if (!regs)
+ return -ENOMEM;
+ revision = readl(regs);
+ iounmap(regs);
+ }
+ else if (ret) {
+ dev_err(dev,
+ "Failed to read the revision number from syscon: %d\n",
+ ret);
+ return ret;
}
- else if (ret) {
- dev_err(dev,
- "Failed to read the revision number from syscon: %d\n",
- ret);
- return ret;
+
+ revision = (revision >> REVISION_SHIFT) & REVISION_MASK;
}
- *revision_value = BIT((revision >> REVISION_SHIFT) & REVISION_MASK);
+ *revision_value = BIT(revision);
return 0;
}
@@ -392,9 +414,14 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
goto register_cpufreq_dt;
}
- ret = ti_cpufreq_setup_syscon_register(opp_data);
- if (ret)
- goto fail_put_node;
+ opp_data->dev = &pdev->dev;
+ opp_data->dev->of_node = opp_data->opp_node;
+
+ if (!of_property_read_bool(opp_data->opp_node, "nvmem-cells")) {
+ ret = ti_cpufreq_setup_syscon_register(opp_data);
+ if (ret)
+ goto fail_put_node;
+ }
/*
* OPPs determine whether or not they are supported based on
--
2.43.0
Add nvmem-cells to describe chip information like chipvariant and
chipspeed. If nvmem-cells are used, the syscon property is not necessary
anymore.
Signed-off-by: Markus Schneider-Pargmann <[email protected]>
Acked-by: Andrew Davis <[email protected]>
---
.../bindings/opp/operating-points-v2-ti-cpu.yaml | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
index 02d1d2c17129..b1881a0834fe 100644
--- a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
+++ b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
@@ -34,6 +34,14 @@ properties:
points to syscon node representing the control module
register space of the SoC.
+ nvmem-cells:
+ $ref: /schemas/types.yaml#/definitions/phandle-array
+
+ nvmem-cell-names:
+ items:
+ - const: chipvariant
+ - const: chipspeed
+
opp-shared: true
patternProperties:
@@ -55,7 +63,13 @@ patternProperties:
required:
- compatible
- - syscon
+
+oneOf:
+ - required:
+ - syscon
+ - required:
+ - nvmem-cells
+ - nvmem-cell-names
additionalProperties: false
--
2.43.0
Use nvmem cells referring to chip variant and speed grade for the
operating points.
Signed-off-by: Markus Schneider-Pargmann <[email protected]>
---
arch/arm64/boot/dts/ti/k3-am625.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am625.dtsi b/arch/arm64/boot/dts/ti/k3-am625.dtsi
index 4193c2b3eed6..d60e1be9eb89 100644
--- a/arch/arm64/boot/dts/ti/k3-am625.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am625.dtsi
@@ -105,6 +105,8 @@ a53_opp_table: opp-table {
compatible = "operating-points-v2-ti-cpu";
opp-shared;
syscon = <&wkup_conf>;
+ nvmem-cells = <&chip_variant>, <&chip_speed>;
+ nvmem-cell-names = "chipvariant", "chipspeed";
opp-200000000 {
opp-hz = /bits/ 64 <200000000>;
--
2.43.0
On 06-02-24, 15:57, Markus Schneider-Pargmann wrote:
> Add nvmem-cells to describe chip information like chipvariant and
> chipspeed. If nvmem-cells are used, the syscon property is not necessary
> anymore.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Acked-by: Andrew Davis <[email protected]>
Rob,
Can you please review / Ack this one ?
--
viresh
On 06/02/2024 15:57, Markus Schneider-Pargmann wrote:
> Add nvmem-cells to describe chip information like chipvariant and
> chipspeed. If nvmem-cells are used, the syscon property is not necessary
> anymore.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Acked-by: Andrew Davis <[email protected]>
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 06/02/2024 15:57, Markus Schneider-Pargmann wrote:
> Use nvmem cells referring to chip variant and speed grade for the
> operating points.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> ---
> arch/arm64/boot/dts/ti/k3-am625.dtsi | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/ti/k3-am625.dtsi b/arch/arm64/boot/dts/ti/k3-am625.dtsi
> index 4193c2b3eed6..d60e1be9eb89 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am625.dtsi
> @@ -105,6 +105,8 @@ a53_opp_table: opp-table {
> compatible = "operating-points-v2-ti-cpu";
> opp-shared;
> syscon = <&wkup_conf>;
> + nvmem-cells = <&chip_variant>, <&chip_speed>;
> + nvmem-cell-names = "chipvariant", "chipspeed";
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Best regards,
Krzysztof
On 15/02/2024 08:26, Viresh Kumar wrote:
> On 06-02-24, 15:57, Markus Schneider-Pargmann wrote:
>> Add nvmem-cells to describe chip information like chipvariant and
>> chipspeed. If nvmem-cells are used, the syscon property is not necessary
>> anymore.
>>
>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>> Acked-by: Andrew Davis <[email protected]>
>
> Rob,
>
> Can you please review / Ack this one ?
Done now, although it is not aligned with DTS in this patchset, so this
does not look tested...
Best regards,
Krzysztof
On 15-02-24, 09:25, Krzysztof Kozlowski wrote:
> On 15/02/2024 08:26, Viresh Kumar wrote:
> > On 06-02-24, 15:57, Markus Schneider-Pargmann wrote:
> >> Add nvmem-cells to describe chip information like chipvariant and
> >> chipspeed. If nvmem-cells are used, the syscon property is not necessary
> >> anymore.
> >>
> >> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> >> Acked-by: Andrew Davis <[email protected]>
> >
> > Rob,
> >
> > Can you please review / Ack this one ?
>
> Done now, although it is not aligned with DTS in this patchset, so this
> does not look tested...
I will wait for them to be fixed then.
--
viresh
Hi,
On Feb 06, 2024 at 15:57:19 +0100, Markus Schneider-Pargmann wrote:
> Add nvmem-cells to describe chip information like chipvariant and
> chipspeed. If nvmem-cells are used, the syscon property is not necessary
> anymore.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Acked-by: Andrew Davis <[email protected]>
> ---
> .../bindings/opp/operating-points-v2-ti-cpu.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> index 02d1d2c17129..b1881a0834fe 100644
> --- a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> +++ b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> @@ -34,6 +34,14 @@ properties:
> points to syscon node representing the control module
> register space of the SoC.
>
> + nvmem-cells:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> +
> + nvmem-cell-names:
> + items:
> + - const: chipvariant
> + - const: chipspeed
> +
> opp-shared: true
>
> patternProperties:
> @@ -55,7 +63,13 @@ patternProperties:
>
> required:
> - compatible
> - - syscon
> +
> +oneOf:
> + - required:
> + - syscon
> + - required:
> + - nvmem-cells
> + - nvmem-cell-names
Reviewed-by: Dhruva Gole <[email protected]>
--
Best regards,
Dhruva Gole <[email protected]>
Hi,
On Feb 06, 2024 at 15:57:20 +0100, Markus Schneider-Pargmann wrote:
> Support using nvmem-cells 'chipvariant' and 'chipspeed' instead of
> syscon. This makes it more flexible and moves more configuration into
> the devicetree.
>
> If nvmem-cells are present, probing will fail if the configuration of
> these cells is broken. If nvmem-cells is not present syscon will be
> used.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> ---
> drivers/cpufreq/ti-cpufreq.c | 105 ++++++++++++++++++++++-------------
> 1 file changed, 66 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/cpufreq/ti-cpufreq.c b/drivers/cpufreq/ti-cpufreq.c
> index 46c41e2ca727..3ee72b1309f0 100644
> --- a/drivers/cpufreq/ti-cpufreq.c
> +++ b/drivers/cpufreq/ti-cpufreq.c
> @@ -10,6 +10,7 @@
> #include <linux/io.h>
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> +#include <linux/nvmem-consumer.h>
> #include <linux/init.h>
> #include <linux/of.h>
> #include <linux/platform_device.h>
> @@ -65,6 +66,7 @@ struct ti_cpufreq_soc_data {
>
> struct ti_cpufreq_data {
> struct device *cpu_dev;
> + struct device *dev;
> struct device_node *opp_node;
> struct regmap *syscon;
> const struct ti_cpufreq_soc_data *soc_data;
> @@ -244,31 +246,40 @@ static struct ti_cpufreq_soc_data am625_soc_data = {
> static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
> u32 *efuse_value)
> {
> + struct device_node *np = opp_data->opp_node;
Umm.. slightly confused, where is *np being used?
> struct device *dev = opp_data->cpu_dev;
> u32 efuse;
> int ret;
>
> - ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
> - &efuse);
> - if (ret == -EIO) {
> - /* not a syscon register! */
> - void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> - opp_data->soc_data->efuse_offset, 4);
> -
> - if (!regs)
> - return -ENOMEM;
> - efuse = readl(regs);
> - iounmap(regs);
> + ret = nvmem_cell_read_u32(opp_data->dev, "chipspeed", &efuse);
> + if (ret && (ret != -ENOENT || !opp_data->syscon))
> + return dev_err_probe(dev, ret,
> + "Failed to read nvmem cell 'chipspeed': %pe",
> + ERR_PTR(ret));
> +
> + if (ret) {
> + ret = regmap_read(opp_data->syscon, opp_data->soc_data->efuse_offset,
> + &efuse);
> + if (ret == -EIO) {
> + /* not a syscon register! */
> + void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> + opp_data->soc_data->efuse_offset, 4);
> +
> + if (!regs)
> + return -ENOMEM;
> + efuse = readl(regs);
> + iounmap(regs);
> + }
> + else if (ret) {
else should be enough I guess, no need of elif?
> + dev_err(dev,
> + "Failed to read the efuse value from syscon: %d\n",
> + ret);
> + return ret;
> }
> - else if (ret) {
> - dev_err(dev,
> - "Failed to read the efuse value from syscon: %d\n",
> - ret);
> - return ret;
> - }
>
> - efuse = (efuse & opp_data->soc_data->efuse_mask);
> - efuse >>= opp_data->soc_data->efuse_shift;
> + efuse = (efuse & opp_data->soc_data->efuse_mask);
> + efuse >>= opp_data->soc_data->efuse_shift;
> + }
>
> *efuse_value = opp_data->soc_data->efuse_xlate(opp_data, efuse);
>
> @@ -285,30 +296,41 @@ static int ti_cpufreq_get_efuse(struct ti_cpufreq_data *opp_data,
> static int ti_cpufreq_get_rev(struct ti_cpufreq_data *opp_data,
> u32 *revision_value)
> {
> + struct device_node *np = opp_data->opp_node;
where is this used? Atleast, in this patch I don't see it...
> struct device *dev = opp_data->cpu_dev;
> u32 revision;
> int ret;
>
> - ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
> - &revision);
> - if (ret == -EIO) {
> - /* not a syscon register! */
> - void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> - opp_data->soc_data->rev_offset, 4);
> -
> - if (!regs)
> - return -ENOMEM;
> - revision = readl(regs);
> - iounmap(regs);
> + ret = nvmem_cell_read_u32(opp_data->dev, "chipvariant", &revision);
> + if (ret && (ret != -ENOENT || !opp_data->syscon))
> + return dev_err_probe(dev, ret,
> + "Failed to read nvmem cell 'chipvariant': %pe",
> + ERR_PTR(ret));
> +
> + if (ret) {
> + ret = regmap_read(opp_data->syscon, opp_data->soc_data->rev_offset,
> + &revision);
> + if (ret == -EIO) {
> + /* not a syscon register! */
> + void __iomem *regs = ioremap(OMAP3_SYSCON_BASE +
> + opp_data->soc_data->rev_offset, 4);
> +
> + if (!regs)
> + return -ENOMEM;
> + revision = readl(regs);
> + iounmap(regs);
> + }
> + else if (ret) {
Do you really have to? This code will reach only if(ret) is satisfied,
the elif feels redundant. Else should be fine
> + dev_err(dev,
> + "Failed to read the revision number from syscon: %d\n",
> + ret);
> + return ret;
> }
> - else if (ret) {
> - dev_err(dev,
> - "Failed to read the revision number from syscon: %d\n",
> - ret);
> - return ret;
> +
> + revision = (revision >> REVISION_SHIFT) & REVISION_MASK;
> }
>
> - *revision_value = BIT((revision >> REVISION_SHIFT) & REVISION_MASK);
> + *revision_value = BIT(revision);
>
> return 0;
> }
> @@ -392,9 +414,14 @@ static int ti_cpufreq_probe(struct platform_device *pdev)
> goto register_cpufreq_dt;
> }
>
> - ret = ti_cpufreq_setup_syscon_register(opp_data);
> - if (ret)
> - goto fail_put_node;
> + opp_data->dev = &pdev->dev;
> + opp_data->dev->of_node = opp_data->opp_node;
> +
> + if (!of_property_read_bool(opp_data->opp_node, "nvmem-cells")) {
> + ret = ti_cpufreq_setup_syscon_register(opp_data);
> + if (ret)
> + goto fail_put_node;
> + }
Mostly looks okay, with above comments addressed:
Reviewed-by: Dhruva Gole <[email protected]>
--
Best regards,
Dhruva Gole <[email protected]>
On 15/02/2024 09:24, Krzysztof Kozlowski wrote:
> On 06/02/2024 15:57, Markus Schneider-Pargmann wrote:
>> Add nvmem-cells to describe chip information like chipvariant and
>> chipspeed. If nvmem-cells are used, the syscon property is not necessary
>> anymore.
>>
>> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
>> Acked-by: Andrew Davis <[email protected]>
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
NAK
And unreviewed. Please carry on:
Nacked-by: Krzysztof Kozlowski <[email protected]>
I found older discussion with Rob, who pointed out that one device
cannot have entirely different programming models, so this patchset has
the same errors. None of the previous issues were fixed here.
Best regards,
Krzysztof
On 06/02/2024 15:57, Markus Schneider-Pargmann wrote:
> Add nvmem-cells to describe chip information like chipvariant and
> chipspeed. If nvmem-cells are used, the syscon property is not necessary
> anymore.
>
> Signed-off-by: Markus Schneider-Pargmann <[email protected]>
> Acked-by: Andrew Davis <[email protected]>
> ---
> .../bindings/opp/operating-points-v2-ti-cpu.yaml | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> index 02d1d2c17129..b1881a0834fe 100644
> --- a/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> +++ b/Documentation/devicetree/bindings/opp/operating-points-v2-ti-cpu.yaml
> @@ -34,6 +34,14 @@ properties:
> points to syscon node representing the control module
> register space of the SoC.
>
> + nvmem-cells:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
Why redefining the type?
Best regards,
Krzysztof