[PATCH 05/89] clk: Return error code when of provider pointer is NULL

[PATCH 05/89] clk: Return error code when of provider pointer is NULL

[Maxime Ripard]

The clock framework DT provider helpers don't check the pointers in the
array registered by the clock provider before returning it.

This means that if the array is sparse, we will end up returning a NULL
pointer while the caller expects an error pointer, resulting in a crash.

Let's test the pointer returned and properly return an error if the pointer
is NULL.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/clk.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index f0f2b599fd7e..8532b5ed1060 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -4318,13 +4318,18 @@ struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data)
 {
 	struct clk_onecell_data *clk_data = data;
 	unsigned int idx = clkspec->args[0];
+	struct clk *clk;
 
 	if (idx >= clk_data->clk_num) {
 		pr_err("%s: invalid clock index %u\n", __func__, idx);
 		return ERR_PTR(-EINVAL);
 	}
 
-	return clk_data->clks[idx];
+	clk = clk_data->clks[idx];
+	if (!clk)
+		return ERR_PTR(-ENODEV);
+
+	return clk;
 }
 EXPORT_SYMBOL_GPL(of_clk_src_onecell_get);
 
@@ -4333,13 +4338,18 @@ of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data)
 {
 	struct clk_hw_onecell_data *hw_data = data;
 	unsigned int idx = clkspec->args[0];
+	struct clk_hw *hw;
 
 	if (idx >= hw_data->num) {
 		pr_err("%s: invalid index %u\n", __func__, idx);
 		return ERR_PTR(-EINVAL);
 	}
 
-	return hw_data->hws[idx];
+	hw = hw_data->hws[idx];
+	if (!hw)
+		return ERR_PTR(-ENODEV);
+
+	return hw;
 }
 EXPORT_SYMBOL_GPL(of_clk_hw_onecell_get);
 
-- 
git-series 0.9.1

Re: [PATCH 05/89] clk: Return error code when of provider pointer is NULL

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:07)
> The clock framework DT provider helpers don't check the pointers in the
> array registered by the clock provider before returning it.
> 
> This means that if the array is sparse, we will end up returning a NULL
> pointer while the caller expects an error pointer, resulting in a crash.
> 
> Let's test the pointer returned and properly return an error if the pointer
> is NULL.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/clk.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index f0f2b599fd7e..8532b5ed1060 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -4318,13 +4318,18 @@ struct clk *of_clk_src_onecell_get(struct of_phandle_args *clkspec, void *data)
>  {
>         struct clk_onecell_data *clk_data = data;
>         unsigned int idx = clkspec->args[0];
> +       struct clk *clk;
>  
>         if (idx >= clk_data->clk_num) {
>                 pr_err("%s: invalid clock index %u\n", __func__, idx);
>                 return ERR_PTR(-EINVAL);
>         }
>  
> -       return clk_data->clks[idx];
> +       clk = clk_data->clks[idx];
> +       if (!clk)

NULL is a valid clk. That should keep working and not be overriden with
an error pointer. If you want to return an error pointer either fill it
in with an error pointer or write your own version of this.

> +               return ERR_PTR(-ENODEV);
> +
> +       return clk;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_src_onecell_get);
>  
> @@ -4333,13 +4338,18 @@ of_clk_hw_onecell_get(struct of_phandle_args *clkspec, void *data)
>  {
>         struct clk_hw_onecell_data *hw_data = data;
>         unsigned int idx = clkspec->args[0];
> +       struct clk_hw *hw;
>  
>         if (idx >= hw_data->num) {
>                 pr_err("%s: invalid index %u\n", __func__, idx);
>                 return ERR_PTR(-EINVAL);
>         }
>  
> -       return hw_data->hws[idx];
> +       hw = hw_data->hws[idx];
> +       if (!hw)

And this one is the same. We let NULL be returned so that it can be
returned as a NULL pointer to the caller if desired. That indicates a
clk that does nothing when used.

> +               return ERR_PTR(-ENODEV);
> +
> +       return hw;
>  }
>  EXPORT_SYMBOL_GPL(of_clk_hw_onecell_get);

[PATCH 06/89] dt-bindings: clock: Add a binding for the RPi Firmware clocks

[Maxime Ripard]

The firmare running on the RPi VideoCore can be used to discover and
change the various clocks running in the BCM2711. Since devices will
need to use them through the DT, let's add a pretty simple binding.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml

diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
new file mode 100644
index 000000000000..d37bc311321d
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/raspberrypi,firmware-clocks.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RaspberryPi Firmware Clocks Device Tree Bindings
+
+maintainers:
+  - Maxime Ripard <mripard@kernel.org>
+
+properties:
+  "#clock-cells":
+    const: 1
+
+  compatible:
+    const: raspberrypi,firmware-clocks
+
+  raspberrypi,firmware:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: >
+      Phandle to the mailbox node to communicate with the firmware.
+
+required:
+  - "#clock-cells"
+  - compatible
+  - raspberrypi,firmware
+
+additionalProperties: false
+
+examples:
+  - |
+    firmware_clocks: firmware-clocks {
+        compatible = "raspberrypi,firmware-clocks";
+        raspberrypi,firmware = <&firmware>;
+        #clock-cells = <1>;
+    };
+
+...
-- 
git-series 0.9.1

Re: [PATCH 06/89] dt-bindings: clock: Add a binding for the RPi Firmware clocks

[Rob Herring]

On Mon, Feb 24, 2020 at 10:06:08AM +0100, Maxime Ripard wrote:
> The firmare running on the RPi VideoCore can be used to discover and
> change the various clocks running in the BCM2711. Since devices will
> need to use them through the DT, let's add a pretty simple binding.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> 
> diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> new file mode 100644
> index 000000000000..d37bc311321d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/raspberrypi,firmware-clocks.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RaspberryPi Firmware Clocks Device Tree Bindings
> +
> +maintainers:
> +  - Maxime Ripard <mripard@kernel.org>
> +
> +properties:
> +  "#clock-cells":
> +    const: 1
> +
> +  compatible:
> +    const: raspberrypi,firmware-clocks
> +
> +  raspberrypi,firmware:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description: >
> +      Phandle to the mailbox node to communicate with the firmware.

Can't this be a child node of the phandle instead? Or just add 
'#clock-cells' to the firmware node.

> +
> +required:
> +  - "#clock-cells"
> +  - compatible
> +  - raspberrypi,firmware
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    firmware_clocks: firmware-clocks {
> +        compatible = "raspberrypi,firmware-clocks";
> +        raspberrypi,firmware = <&firmware>;
> +        #clock-cells = <1>;
> +    };
> +
> +...
> -- 
> git-series 0.9.1
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/89] dt-bindings: clock: Add a binding for the RPi Firmware clocks

[Stephen Boyd]

Quoting Rob Herring (2020-02-25 10:16:54)
> On Mon, Feb 24, 2020 at 10:06:08AM +0100, Maxime Ripard wrote:
> > The firmare running on the RPi VideoCore can be used to discover and
> > change the various clocks running in the BCM2711. Since devices will
> > need to use them through the DT, let's add a pretty simple binding.
> > 
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > ---
> >  Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml | 39 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> > new file mode 100644
> > index 000000000000..d37bc311321d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/clock/raspberrypi,firmware-clocks.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/clock/raspberrypi,firmware-clocks.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RaspberryPi Firmware Clocks Device Tree Bindings
> > +
> > +maintainers:
> > +  - Maxime Ripard <mripard@kernel.org>
> > +
> > +properties:
> > +  "#clock-cells":
> > +    const: 1
> > +
> > +  compatible:
> > +    const: raspberrypi,firmware-clocks
> > +
> > +  raspberrypi,firmware:
> > +    $ref: /schemas/types.yaml#/definitions/phandle
> > +    description: >
> > +      Phandle to the mailbox node to communicate with the firmware.
> 
> Can't this be a child node of the phandle instead? Or just add 
> '#clock-cells' to the firmware node.

Yeah, just add the clock-cells to the firmware node unless that doesn't
work for some reason?

[PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Maxime Ripard]

The current firmware clock driver for the RaspberryPi can only be probed by
manually registering an associated platform_device.

While this works fine for cpufreq where the device gets attached a clkdev
lookup, it would be tedious to maintain a table of all the devices using
one of the clocks exposed by the firmware.

Since the DT on the other hand is the perfect place to store those
associations, make the firmware clocks driver probe-able through the device
tree so that we can represent it as a node.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 1654fd0eedc9..94870234824c 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -255,15 +255,13 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	struct raspberrypi_clk *rpi;
 	int ret;
 
-	firmware_node = of_find_compatible_node(NULL, NULL,
-					"raspberrypi,bcm2835-firmware");
+	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware", 0);
 	if (!firmware_node) {
 		dev_err(dev, "Missing firmware node\n");
 		return -ENOENT;
 	}
 
 	firmware = rpi_firmware_get(firmware_node);
-	of_node_put(firmware_node);
 	if (!firmware)
 		return -EPROBE_DEFER;
 
@@ -300,9 +298,16 @@ static int raspberrypi_clk_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id raspberrypi_clk_match[] = {
+        { .compatible = "raspberrypi,firmware-clocks" },
+        { },
+};
+MODULE_DEVICE_TABLE(of, raspberrypi_clk_match);
+
 static struct platform_driver raspberrypi_clk_driver = {
 	.driver = {
 		.name = "raspberrypi-clk",
+		.of_match_table = raspberrypi_clk_match,
 	},
 	.probe          = raspberrypi_clk_probe,
 	.remove		= raspberrypi_clk_remove,
-- 
git-series 0.9.1

Re: [PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 3327 bytes --]

Hi Maxime,

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The current firmware clock driver for the RaspberryPi can only be probed by
> manually registering an associated platform_device.
> 
> While this works fine for cpufreq where the device gets attached a clkdev
> lookup, it would be tedious to maintain a table of all the devices using
> one of the clocks exposed by the firmware.
> 
> Since the DT on the other hand is the perfect place to store those
> associations, make the firmware clocks driver probe-able through the device
> tree so that we can represent it as a node.

I'm not convinced this is the right approach, and if we decide to go this way,
there are more changes to take into account.

For one, if we create a dt node for this driver, we'd have to delete the
platform device creation in firmware/raspberrypi.c and then we'd be even able
to bypass raspberrypi-cpufreq altogether by creating opp tables in dt. But
there are reasons we didn't go that way at the time.

We've made an effort to avoid using dt for firmware interfaces whenever
possible as, on one hand, it's arguable they don't fit device-tree's hardware
description paradigm and, on the other, the lack of flexibility they impose
once the binding is defined. VC4's firmware interfaces are not set in stone,
nor standardized like SCMI, so the more flexible we are to future changes the
better.

Another thing I'm not all that happy about it's how dynamic clock registering
is handled in patch #22 (but I'll keep it here as relevant to the discussion):

- Some of those fw managed clocks you're creating have their mmio counterpart
  being registered by clk-bcm238. IMO either register one or the other, giving
  precedence to the mmio counterpart. Note that for pllb, we deleted the
  relevant code from clk-bcm2385.

- The same way we were able to map the fw CPU clock into the clk tree
  (pllb/pllb_arm) there are no reasons we shouldn't be able to do the same for
  the VPU clocks. It's way nicer and less opaque to users (this being a
  learning platform adds to the argument).

- On top of that, having a special case for the CPU clock registration is
  nasty. Lets settle for one solution and make everyone follow it.

- I don't see what's so bad about creating clock lookups. IIUC there are only
  two clocks that need this special handling CPU & HDMI, It's manageable. You
  don't even have to mess with the consumer driver, if there was ever to be a
  dt provided mmio option to this clock.

>  drivers/clk/bcm/clk-raspberrypi.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-
> raspberrypi.c
> index 1654fd0eedc9..94870234824c 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -255,15 +255,13 @@ static int raspberrypi_clk_probe(struct platform_device
> *pdev)
>  	struct raspberrypi_clk *rpi;
>  	int ret;
>  
> -	firmware_node = of_find_compatible_node(NULL, NULL,
> -					"raspberrypi,bcm2835-firmware");
> +	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware",
> 0);

There is no such phandle in the upstream device tree. Maybe this was aimed at
the downstream dt?

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6024 bytes --]

Hi Nicolas,

On Tue, Feb 25, 2020 at 05:00:56PM +0100, Nicolas Saenz Julienne wrote:
> On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> > The current firmware clock driver for the RaspberryPi can only be probed by
> > manually registering an associated platform_device.
> >
> > While this works fine for cpufreq where the device gets attached a clkdev
> > lookup, it would be tedious to maintain a table of all the devices using
> > one of the clocks exposed by the firmware.
> >
> > Since the DT on the other hand is the perfect place to store those
> > associations, make the firmware clocks driver probe-able through the device
> > tree so that we can represent it as a node.
>
> I'm not convinced this is the right approach, and if we decide to go this way,
> there are more changes to take into account.

This was actually a shameless bait to start that discussion, so I'm
glad it worked ;)

> For one, if we create a dt node for this driver, we'd have to delete the
> platform device creation in firmware/raspberrypi.c and then we'd be even able
> to bypass raspberrypi-cpufreq altogether by creating opp tables in dt. But
> there are reasons we didn't go that way at the time.

Right, I missed that one since the check for the firmware phandle was
preventing the double-probe to happen, but it's bad indeed.

> We've made an effort to avoid using dt for firmware interfaces whenever
> possible as, on one hand, it's arguable they don't fit device-tree's hardware
> description paradigm and, on the other, the lack of flexibility they impose
> once the binding is defined. VC4's firmware interfaces are not set in stone,
> nor standardized like SCMI, so the more flexible we are to future changes the
> better.

The device tree isn't just about the hardware though, but also
contains the state the bootloader / firmware left the hardware
in. You're mentionning SCMI, and SCMI clocks IDs are stored in the
device tree. Just like pen release addresses, PSCI function ids, etc.

The firmware IDs of these clocks shouldn't change too.

But you also raise a valid point with the lack of flexibility,
especially since the clock tree isn't that well understood.

> Another thing I'm not all that happy about it's how dynamic clock registering
> is handled in patch #22 (but I'll keep it here as relevant to the discussion):
>
> - Some of those fw managed clocks you're creating have their mmio counterpart
>   being registered by clk-bcm238. IMO either register one or the other, giving
>   precedence to the mmio counterpart. Note that for pllb, we deleted the
>   relevant code from clk-bcm2385.

Indeed, and it's really that part of the discussion I wanted to
start. For some reason, it looks like a good chunk of those clocks are
non-functional at the moment (they all report 0). If we're going to
use the firmware clocks as I did here, we'd have to modify most of the
device clocks used so far (UART, especially) to derive from the core
clock.

I wasn't really sure of the implications though, since it's my first
experience with the RPi clock tree.

> - The same way we were able to map the fw CPU clock into the clk tree
>   (pllb/pllb_arm) there are no reasons we shouldn't be able to do the same for
>   the VPU clocks. It's way nicer and less opaque to users (this being a
>   learning platform adds to the argument).

This would make the Linux clock tree match the one in hardware, which
would indeed be more readable to a beginner, but I see three main
drawbacks with this:

  - The parent / child relationship is already encoded in the firmware
    discovery mechanism. It's not used yet by the driver, because the
    firmware reports all of them as root clocks, but that's pretty
    easy to fix.

  - It would make the code far more complicated and confusing than it
    could, especially to beginners. And as far as I know, only the RPi
    is doing that, while pretty much all the other platforms either
    have the clock tree entirely defined, or rely on the firmware, but
    don't have an hybrid. So they would learn something that cannot
    really be applied to anywhere else.

  - I have no idea what the clock tree is supposed to look like :)

> - On top of that, having a special case for the CPU clock registration is
>   nasty. Lets settle for one solution and make everyone follow it.

It seemed to me that the CPU clock had a factor between the actual CPU
frequency and its clock? If not, then yeah we should definitely get
rid of it.

> - I don't see what's so bad about creating clock lookups. IIUC there are only
>   two clocks that need this special handling CPU & HDMI, It's manageable. You
>   don't even have to mess with the consumer driver, if there was ever to be a
>   dt provided mmio option to this clock.

V3D needs one too, and I might have missed a bunch of them in that
series given how the current debugging of the remaining issues turn
out to be. And clk_lookups are local to devices, so you need to factor
that by the number of devices you have.

Sure, it works, but it feels to me like that's going to be an issue
pretty fast, especially with the lookups on the way out?

> >  drivers/clk/bcm/clk-raspberrypi.c | 11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-
> > raspberrypi.c
> > index 1654fd0eedc9..94870234824c 100644
> > --- a/drivers/clk/bcm/clk-raspberrypi.c
> > +++ b/drivers/clk/bcm/clk-raspberrypi.c
> > @@ -255,15 +255,13 @@ static int raspberrypi_clk_probe(struct platform_device
> > *pdev)
> >  	struct raspberrypi_clk *rpi;
> >  	int ret;
> >
> > -	firmware_node = of_find_compatible_node(NULL, NULL,
> > -					"raspberrypi,bcm2835-firmware");
> > +	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware",
> > 0);
>
> There is no such phandle in the upstream device tree. Maybe this was aimed at
> the downstream dt?

raspberrypi,firmware is the property, it points to the /soc/firmware
node that is defined in bcm2835-rpi.dtsi

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 6537 bytes --]

Hi Maxime,

On Wed, 2020-02-26 at 16:01 +0100, Maxime Ripard wrote:

[...]

> This was actually a shameless bait to start that discussion, so I'm
> glad it worked ;)

:)

[...]

> > - Some of those fw managed clocks you're creating have their mmio
> > counterpart
> >   being registered by clk-bcm238. IMO either register one or the other,
> > giving
> >   precedence to the mmio counterpart. Note that for pllb, we deleted the
> >   relevant code from clk-bcm2385.
> 
> Indeed, and it's really that part of the discussion I wanted to
> start. For some reason, it looks like a good chunk of those clocks are
> non-functional at the moment (they all report 0).

Yes, although they should be alright. I think it's just a matter of passing the
right flags to the clk framework (disable caching and so on), but never found
the time to investigate further.

> If we're going to
> use the firmware clocks as I did here, we'd have to modify most of the
> device clocks used so far (UART, especially) to derive from the core
> clock. I wasn't really sure of the implications though, since it's my first
> experience with the RPi clock tree.

That's something I'm confused about. I played around with your code and the HSM
clock changes seem to be completely unrelated to the VPU clock. Actually it
seems it's derived from 'plld_per' (here Florian can maybe contradict me). I
found out by feeding the mmio HSM clock to your driver, which actually seemed
to work (albeit maybe just out of luck since the FW already set up everything).

Bare in mind, we disable turbo mode upstream so as for the firmware not to
change the VPU frequencies on par with CPU changes (controlled by a special bit
in the CPU clock mailbox property). So, if I'm not wrong, this simplifies
things. As we don't have to worry about re-clocking all peripherals with every
resolution change.

This even opens up another question. Which clocks is the firmware interface
monitoring for DVFS? If it's just the VPU and CPU we could be over-complicating
things here, and MMIO clks could be an option, isn't it?

On the subject of re-clocking, I had a word with the clk maintainers on how to
properly implement it, see the two last paragraphs here if curious:
https://www.spinics.net/lists/linux-clk/msg36937.html

> > - The same way we were able to map the fw CPU clock into the clk tree
> >   (pllb/pllb_arm) there are no reasons we shouldn't be able to do the same
> > for
> >   the VPU clocks. It's way nicer and less opaque to users (this being a
> >   learning platform adds to the argument).
> 
> This would make the Linux clock tree match the one in hardware, which
> would indeed be more readable to a beginner, but I see three main
> drawbacks with this:
> 
>   - The parent / child relationship is already encoded in the firmware
>     discovery mechanism. It's not used yet by the driver, because the
>     firmware reports all of them as root clocks, but that's pretty
>     easy to fix.

Had a look at this, they all return root as their parent. Which is somewhat
true from the fw interface perspective (only leaves are represented), but not
too endearing.

>   - It would make the code far more complicated and confusing than it
>     could, especially to beginners. And as far as I know, only the RPi
>     is doing that, while pretty much all the other platforms either
>     have the clock tree entirely defined, or rely on the firmware, but
>     don't have an hybrid. So they would learn something that cannot
>     really be applied to anywhere else.

Fair enough. Still, for now, I think I prefer a hybrid clk tree approach.

>   - I have no idea what the clock tree is supposed to look like :)

I don't have access to the official clock tree either. The closest we have is
whatever the mmio clk driver exposes.

> > - On top of that, having a special case for the CPU clock registration is
> >   nasty. Lets settle for one solution and make everyone follow it.
> 
> It seemed to me that the CPU clock had a factor between the actual CPU
> frequency and its clock? If not, then yeah we should definitely get
> rid of it.

Yes, IIRC, there is a factor because the CPU clock firmware interface actually
controls the underlying PLL frequency which is then divided by 2 before
reaching the CPU. Which kind of breaks the FW interface design if you ask
me (alongside this turbo mode thing).

> > - I don't see what's so bad about creating clock lookups. IIUC there are
> > only
> >   two clocks that need this special handling CPU & HDMI, It's manageable.
> > You
> >   don't even have to mess with the consumer driver, if there was ever to be
> > a
> >   dt provided mmio option to this clock.
> 
> V3D needs one too, and I might have missed a bunch of them in that
> series given how the current debugging of the remaining issues turn
> out to be.

Would be nice to see if V3D is also affected by DVFS, and the rest of clocks
for that matter.

> And clk_lookups are local to devices, so you need to factor that by the
> number of devices you have. Sure, it works, but it feels to me like that's
> going to be an issue pretty fast, especially with the lookups on the way out?

I see your point, TBH I don't mind moving it into the device-tree if things are
going to get nasty.


> > >  drivers/clk/bcm/clk-raspberrypi.c | 11 ++++++++---
> > >  1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-
> > > raspberrypi.c
> > > index 1654fd0eedc9..94870234824c 100644
> > > --- a/drivers/clk/bcm/clk-raspberrypi.c
> > > +++ b/drivers/clk/bcm/clk-raspberrypi.c
> > > @@ -255,15 +255,13 @@ static int raspberrypi_clk_probe(struct
> > > platform_device
> > > *pdev)
> > >  	struct raspberrypi_clk *rpi;
> > >  	int ret;
> > > 
> > > -	firmware_node = of_find_compatible_node(NULL, NULL,
> > > -					"raspberrypi,bcm2835-firmware");
> > > +	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware",
> > > 0);
> > 
> > There is no such phandle in the upstream device tree. Maybe this was aimed
> > at
> > the downstream dt?
> 
> raspberrypi,firmware is the property, it points to the /soc/firmware
> node that is defined in bcm2835-rpi.dtsi

Yes, my bad. On that topic, I kind of like Robh's suggestion of making this
driver a child of the firmware node (see an example in
input/touchscreen/raspberrypi-ts.c).

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Stefan Wahren]

Hi Maxime,

Am 24.02.20 um 10:06 schrieb Maxime Ripard:
> The current firmware clock driver for the RaspberryPi can only be probed by
> manually registering an associated platform_device.
>
> While this works fine for cpufreq where the device gets attached a clkdev
> lookup, it would be tedious to maintain a table of all the devices using
> one of the clocks exposed by the firmware.
>
> Since the DT on the other hand is the perfect place to store those
> associations, make the firmware clocks driver probe-able through the device
> tree so that we can represent it as a node.
>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

FWIW i want to mention that starting with this commit, X doesn't start
on my Raspberry Pi 3A (applied on top of linux-next using
multi_v7_defconfig).

Regards
Stefan

Re: [PATCH 07/89] clk: bcm: rpi: Allow the driver to be probed by DT

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1127 bytes --]

Hi Stefan,

On Sun, Mar 01, 2020 at 01:16:28PM +0100, Stefan Wahren wrote:
> Hi Maxime,
>
> Am 24.02.20 um 10:06 schrieb Maxime Ripard:
> > The current firmware clock driver for the RaspberryPi can only be probed by
> > manually registering an associated platform_device.
> >
> > While this works fine for cpufreq where the device gets attached a clkdev
> > lookup, it would be tedious to maintain a table of all the devices using
> > one of the clocks exposed by the firmware.
> >
> > Since the DT on the other hand is the perfect place to store those
> > associations, make the firmware clocks driver probe-able through the device
> > tree so that we can represent it as a node.
> >
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> FWIW i want to mention that starting with this commit, X doesn't start
> on my Raspberry Pi 3A (applied on top of linux-next using
> multi_v7_defconfig).

Was this the same issue you reported with the HSM clock rate, or truly
an issue with my series?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 08/89] clk: bcm: rpi: Statically init clk_init_data

[Maxime Ripard]

Instead of declaring the clk_init_data and then calling memset on it, just
initialise properly.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 94870234824c..64fd91b5ffe9 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -175,11 +175,10 @@ static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
 
 static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 {
+	struct clk_init_data init = {};
 	u32 min_rate = 0, max_rate = 0;
-	struct clk_init_data init;
 	int ret;
 
-	memset(&init, 0, sizeof(init));
 
 	/* All of the PLLs derive from the external oscillator. */
 	init.parent_names = (const char *[]){ "osc" };
-- 
git-series 0.9.1

Re: [PATCH 08/89] clk: bcm: rpi: Statically init clk_init_data

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 428 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> Instead of declaring the clk_init_data and then calling memset on it, just
> initialise properly.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 09/89] clk: bcm: rpi: Use clk_hw_register for pllb_arm

[Maxime Ripard]

The pllb_arm clock is defined as a fixed factor clock with the pllb clock
as a parent. However, all its configuration is entirely static, and thus we
don't really need to call clk_hw_register_fixed_factor but can simply call
clk_hw_register with a static clk_fixed_factor structure.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 64fd91b5ffe9..48cffa44db64 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -225,16 +225,28 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	return devm_clk_hw_register(rpi->dev, &rpi->pllb);
 }
 
+static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
+	.mult = 1,
+	.div = 2,
+	.hw.init = &(struct clk_init_data) {
+		.name		= "pllb_arm",
+		.parent_names	= (const char *[]){ "pllb" },
+		.num_parents	= 1,
+		.ops		= &clk_fixed_factor_ops,
+		.flags		= CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
+	},
+};
+
 static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 {
-	rpi->pllb_arm = clk_hw_register_fixed_factor(rpi->dev,
-				"pllb_arm", "pllb",
-				CLK_SET_RATE_PARENT | CLK_GET_RATE_NOCACHE,
-				1, 2);
-	if (IS_ERR(rpi->pllb_arm)) {
+	int ret;
+
+	ret = clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
+	if (ret) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
-		return PTR_ERR(rpi->pllb_arm);
+		return ret;
 	}
+	rpi->pllb_arm = &raspberrypi_clk_pllb_arm.hw;
 
 	rpi->pllb_arm_lookup = clkdev_hw_create(rpi->pllb_arm, NULL, "cpu0");
 	if (!rpi->pllb_arm_lookup) {
-- 
git-series 0.9.1

Re: [PATCH 09/89] clk: bcm: rpi: Use clk_hw_register for pllb_arm

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The pllb_arm clock is defined as a fixed factor clock with the pllb clock
> as a parent. However, all its configuration is entirely static, and thus we
> don't really need to call clk_hw_register_fixed_factor but can simply call
> clk_hw_register with a static clk_fixed_factor structure.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 09/89] clk: bcm: rpi: Use clk_hw_register for pllb_arm

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:11)
> The pllb_arm clock is defined as a fixed factor clock with the pllb clock
> as a parent. However, all its configuration is entirely static, and thus we
> don't really need to call clk_hw_register_fixed_factor but can simply call
> clk_hw_register with a static clk_fixed_factor structure.

Please add () to things like clk_hw_register_fixed_factor() and
clk_hw_register().

> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 10/89] clk: bcm: rpi: Remove global pllb_arm clock pointer

[Maxime Ripard]

The pllb_arm clk_hw pointer in the raspberry_clk structure isn't used
anywhere but in the raspberrypi_register_pllb_arm.

Let's remove it, this will make our lives easier in future patches.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 48cffa44db64..61a942f9a6a2 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -40,7 +40,6 @@ struct raspberrypi_clk {
 	unsigned long max_rate;
 
 	struct clk_hw pllb;
-	struct clk_hw *pllb_arm;
 	struct clk_lookup *pllb_arm_lookup;
 };
 
@@ -246,12 +245,12 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 		dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
 		return ret;
 	}
-	rpi->pllb_arm = &raspberrypi_clk_pllb_arm.hw;
 
-	rpi->pllb_arm_lookup = clkdev_hw_create(rpi->pllb_arm, NULL, "cpu0");
+	rpi->pllb_arm_lookup = clkdev_hw_create(&raspberrypi_clk_pllb_arm.hw,
+						NULL, "cpu0");
 	if (!rpi->pllb_arm_lookup) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
-		clk_hw_unregister_fixed_factor(rpi->pllb_arm);
+		clk_hw_unregister_fixed_factor(&raspberrypi_clk_pllb_arm.hw);
 		return -ENOMEM;
 	}
 
-- 
git-series 0.9.1

Re: [PATCH 10/89] clk: bcm: rpi: Remove global pllb_arm clock pointer

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 526 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The pllb_arm clk_hw pointer in the raspberry_clk structure isn't used
> anywhere but in the raspberrypi_register_pllb_arm.
> 
> Let's remove it, this will make our lives easier in future patches.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 10/89] clk: bcm: rpi: Remove global pllb_arm clock pointer

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 647 bytes --]

Hi Nicolas,

On Tue, Feb 25, 2020 at 05:13:33PM +0100, Nicolas Saenz Julienne wrote:
> On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> > The pllb_arm clk_hw pointer in the raspberry_clk structure isn't used
> > anywhere but in the raspberrypi_register_pllb_arm.
> >
> > Let's remove it, this will make our lives easier in future patches.
> >
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

I guess you meant Acked or Reviewed-by?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 10/89] clk: bcm: rpi: Remove global pllb_arm clock pointer

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 877 bytes --]

On Wed, 2020-02-26 at 15:26 +0100, Maxime Ripard wrote:
> Hi Nicolas,
> 
> On Tue, Feb 25, 2020 at 05:13:33PM +0100, Nicolas Saenz Julienne wrote:
> > On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> > > The pllb_arm clk_hw pointer in the raspberry_clk structure isn't used
> > > anywhere but in the raspberrypi_register_pllb_arm.
> > > 
> > > Let's remove it, this will make our lives easier in future patches.
> > > 
> > > Cc: Michael Turquette <mturquette@baylibre.com>
> > > Cc: Stephen Boyd <sboyd@kernel.org>
> > > Cc: linux-clk@vger.kernel.org
> > > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> 
> I guess you meant Acked or Reviewed-by?

Yes sorry, I ran the wrong macro in vim.

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 11/89] clk: bcm: rpi: Make sure pllb_arm is removed

[Maxime Ripard]

The pllb_arm clock was created at probe time, but was never removed if
something went wrong later in probe, or if the driver was ever removed from
the system.

Now that we are using clk_hw_register, we can just use its managed variant
to take care of that for us.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 61a942f9a6a2..783c8c5e5373 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -240,7 +240,7 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 {
 	int ret;
 
-	ret = clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
+	ret = devm_clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
 		return ret;
@@ -250,7 +250,6 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 						NULL, "cpu0");
 	if (!rpi->pllb_arm_lookup) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
-		clk_hw_unregister_fixed_factor(&raspberrypi_clk_pllb_arm.hw);
 		return -ENOMEM;
 	}
 
-- 
git-series 0.9.1

Re: [PATCH 11/89] clk: bcm: rpi: Make sure pllb_arm is removed

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The pllb_arm clock was created at probe time, but was never removed if
> something went wrong later in probe, or if the driver was ever removed from
> the system.
> 
> Now that we are using clk_hw_register, we can just use its managed variant
> to take care of that for us.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 11/89] clk: bcm: rpi: Make sure pllb_arm is removed

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:13)
> The pllb_arm clock was created at probe time, but was never removed if
> something went wrong later in probe, or if the driver was ever removed from
> the system.
> 
> Now that we are using clk_hw_register, we can just use its managed variant
> to take care of that for us.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 12/89] clk: bcm: rpi: Remove pllb_arm_lookup global pointer

[Maxime Ripard]

The pllb_arm_lookup pointer in the struct raspberrypi_clk is not used for
anything but to store the returned pointer to clkdev_hw_create, and is not
used anywhere else in the driver.

Let's remove that global pointer from the structure.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 783c8c5e5373..0c1d6c292302 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -40,7 +40,6 @@ struct raspberrypi_clk {
 	unsigned long max_rate;
 
 	struct clk_hw pllb;
-	struct clk_lookup *pllb_arm_lookup;
 };
 
 /*
@@ -238,6 +237,7 @@ static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
 
 static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 {
+	struct clk_lookup *pllb_arm_lookup;
 	int ret;
 
 	ret = devm_clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
@@ -246,9 +246,9 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 		return ret;
 	}
 
-	rpi->pllb_arm_lookup = clkdev_hw_create(&raspberrypi_clk_pllb_arm.hw,
-						NULL, "cpu0");
-	if (!rpi->pllb_arm_lookup) {
+	pllb_arm_lookup = clkdev_hw_create(&raspberrypi_clk_pllb_arm.hw,
+					   NULL, "cpu0");
+	if (!pllb_arm_lookup) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
 		return -ENOMEM;
 	}
-- 
git-series 0.9.1

Re: [PATCH 12/89] clk: bcm: rpi: Remove pllb_arm_lookup global pointer

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The pllb_arm_lookup pointer in the struct raspberrypi_clk is not used for
> anything but to store the returned pointer to clkdev_hw_create, and is not
> used anywhere else in the driver.
> 
> Let's remove that global pointer from the structure.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 12/89] clk: bcm: rpi: Remove pllb_arm_lookup global pointer

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:14)
> The pllb_arm_lookup pointer in the struct raspberrypi_clk is not used for
> anything but to store the returned pointer to clkdev_hw_create, and is not
> used anywhere else in the driver.
> 
> Let's remove that global pointer from the structure.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

Re: [PATCH 12/89] clk: bcm: rpi: Remove pllb_arm_lookup global pointer

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:14)
> The pllb_arm_lookup pointer in the struct raspberrypi_clk is not used for
> anything but to store the returned pointer to clkdev_hw_create, and is not
> used anywhere else in the driver.
> 
> Let's remove that global pointer from the structure.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 13/89] clk: bcm: rpi: Switch to clk_hw_register_clkdev

[Maxime Ripard]

Since we don't care about retrieving the clk_lookup structure pointer
returned by clkdev_hw_create, we can just use the clk_hw_register_clkdev
function.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 0c1d6c292302..b97c7ec61376 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -237,7 +237,6 @@ static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
 
 static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 {
-	struct clk_lookup *pllb_arm_lookup;
 	int ret;
 
 	ret = devm_clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
@@ -246,11 +245,11 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 		return ret;
 	}
 
-	pllb_arm_lookup = clkdev_hw_create(&raspberrypi_clk_pllb_arm.hw,
-					   NULL, "cpu0");
-	if (!pllb_arm_lookup) {
-		dev_err(rpi->dev, "Failed to initialize pllb_arm_lookup\n");
-		return -ENOMEM;
+	ret = clk_hw_register_clkdev(&raspberrypi_clk_pllb_arm.hw,
+				     NULL, "cpu0");
+	if (ret) {
+		dev_err(rpi->dev, "Failed to initialize clkdev\n");
+		return ret;
 	}
 
 	return 0;
-- 
git-series 0.9.1

Re: [PATCH 13/89] clk: bcm: rpi: Switch to clk_hw_register_clkdev

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 481 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> Since we don't care about retrieving the clk_lookup structure pointer
> returned by clkdev_hw_create, we can just use the clk_hw_register_clkdev
> function.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 13/89] clk: bcm: rpi: Switch to clk_hw_register_clkdev

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:15)
> Since we don't care about retrieving the clk_lookup structure pointer
> returned by clkdev_hw_create, we can just use the clk_hw_register_clkdev
> function.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 14/89] clk: bcm: rpi: Make sure the clkdev lookup is removed

[Maxime Ripard]

The clkdev lookup created for the cpufreq device is never removed if
there's an issue later in probe or at module removal time.

Let's convert to the managed variant of the clk_hw_register_clkdev function
to make sure it happens.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index b97c7ec61376..b8b55134ba3f 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -245,8 +245,9 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 		return ret;
 	}
 
-	ret = clk_hw_register_clkdev(&raspberrypi_clk_pllb_arm.hw,
-				     NULL, "cpu0");
+	ret = devm_clk_hw_register_clkdev(rpi->dev,
+					  &raspberrypi_clk_pllb_arm.hw,
+					  NULL, "cpu0");
 	if (ret) {
 		dev_err(rpi->dev, "Failed to initialize clkdev\n");
 		return ret;
-- 
git-series 0.9.1

Re: [PATCH 14/89] clk: bcm: rpi: Make sure the clkdev lookup is removed

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The clkdev lookup created for the cpufreq device is never removed if
> there's an issue later in probe or at module removal time.
> 
> Let's convert to the managed variant of the clk_hw_register_clkdev function
> to make sure it happens.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 14/89] clk: bcm: rpi: Make sure the clkdev lookup is removed

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:16)
> The clkdev lookup created for the cpufreq device is never removed if
> there's an issue later in probe or at module removal time.
> 
> Let's convert to the managed variant of the clk_hw_register_clkdev function
> to make sure it happens.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 15/89] clk: bcm: rpi: Create a data structure for the clocks

[Maxime Ripard]

So far the driver has really only been providing a single clock, and stored
both the data associated to that clock in particular with the data
associated to the "controller".

Since we will change that in the future, let's decouple the clock data from
the provider data.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 40 ++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index b8b55134ba3f..964fc8f792cc 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -35,11 +35,15 @@ struct raspberrypi_clk {
 	struct device *dev;
 	struct rpi_firmware *firmware;
 	struct platform_device *cpufreq;
+};
+
+struct raspberrypi_clk_data {
+	struct clk_hw hw;
 
 	unsigned long min_rate;
 	unsigned long max_rate;
 
-	struct clk_hw pllb;
+	struct raspberrypi_clk *rpi;
 };
 
 /*
@@ -83,8 +87,9 @@ static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32 tag,
 
 static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
 {
-	struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
-						   pllb);
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
+	struct raspberrypi_clk *rpi = data->rpi;
 	u32 val = 0;
 	int ret;
 
@@ -101,8 +106,9 @@ static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
 static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
 						 unsigned long parent_rate)
 {
-	struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
-						   pllb);
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
+	struct raspberrypi_clk *rpi = data->rpi;
 	u32 val = 0;
 	int ret;
 
@@ -119,8 +125,9 @@ static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
 static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 				       unsigned long parent_rate)
 {
-	struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
-						   pllb);
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
+	struct raspberrypi_clk *rpi = data->rpi;
 	u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
 	int ret;
 
@@ -142,13 +149,13 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 static int raspberrypi_pll_determine_rate(struct clk_hw *hw,
 					  struct clk_rate_request *req)
 {
-	struct raspberrypi_clk *rpi = container_of(hw, struct raspberrypi_clk,
-						   pllb);
+	struct raspberrypi_clk_data *data =
+		container_of(hw, struct raspberrypi_clk_data, hw);
 	u64 div, final_rate;
 	u32 ndiv, fdiv;
 
 	/* We can't use req->rate directly as it would overflow */
-	final_rate = clamp(req->rate, rpi->min_rate, rpi->max_rate);
+	final_rate = clamp(req->rate, data->min_rate, data->max_rate);
 
 	div = (u64)final_rate << A2W_PLL_FRAC_BITS;
 	do_div(div, req->best_parent_rate);
@@ -173,10 +180,15 @@ static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
 
 static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 {
+	struct raspberrypi_clk_data *data;
 	struct clk_init_data init = {};
 	u32 min_rate = 0, max_rate = 0;
 	int ret;
 
+	data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	data->rpi = rpi;
 
 	/* All of the PLLs derive from the external oscillator. */
 	init.parent_names = (const char *[]){ "osc" };
@@ -215,12 +227,12 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n",
 		 min_rate, max_rate);
 
-	rpi->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
-	rpi->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+	data->min_rate = min_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+	data->max_rate = max_rate * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
 
-	rpi->pllb.init = &init;
+	data->hw.init = &init;
 
-	return devm_clk_hw_register(rpi->dev, &rpi->pllb);
+	return devm_clk_hw_register(rpi->dev, &data->hw);
 }
 
 static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
-- 
git-series 0.9.1

Re: [PATCH 15/89] clk: bcm: rpi: Create a data structure for the clocks

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 608 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> So far the driver has really only been providing a single clock, and stored
> both the data associated to that clock in particular with the data
> associated to the "controller".
> 
> Since we will change that in the future, let's decouple the clock data from
> the provider data.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 15/89] clk: bcm: rpi: Create a data structure for the clocks

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:17)
> So far the driver has really only been providing a single clock, and stored
> both the data associated to that clock in particular with the data
> associated to the "controller".
> 
> Since we will change that in the future, let's decouple the clock data from
> the provider data.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Maxime Ripard]

The driver has really only supported one clock so far and has hardcoded the
ID used in communications with the firmware in all the functions
implementing the clock framework hooks. Let's store that in the clock data
structure so that we can support more clocks later on.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 964fc8f792cc..e796dabbc641 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -39,6 +39,7 @@ struct raspberrypi_clk {
 
 struct raspberrypi_clk_data {
 	struct clk_hw hw;
+	unsigned id;
 
 	unsigned long min_rate;
 	unsigned long max_rate;
@@ -95,7 +96,7 @@ static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
 
 	ret = raspberrypi_clock_property(rpi->firmware,
 					 RPI_FIRMWARE_GET_CLOCK_STATE,
-					 RPI_FIRMWARE_ARM_CLK_ID, &val);
+					 data->id, &val);
 	if (ret)
 		return 0;
 
@@ -114,8 +115,7 @@ static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
 
 	ret = raspberrypi_clock_property(rpi->firmware,
 					 RPI_FIRMWARE_GET_CLOCK_RATE,
-					 RPI_FIRMWARE_ARM_CLK_ID,
-					 &val);
+					 data->id, &val);
 	if (ret)
 		return ret;
 
@@ -133,8 +133,7 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	ret = raspberrypi_clock_property(rpi->firmware,
 					 RPI_FIRMWARE_SET_CLOCK_RATE,
-					 RPI_FIRMWARE_ARM_CLK_ID,
-					 &new_rate);
+					 data->id, &new_rate);
 	if (ret)
 		dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d",
 				    clk_hw_get_name(hw), ret);
@@ -189,6 +188,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	if (!data)
 		return -ENOMEM;
 	data->rpi = rpi;
+	data->id = RPI_FIRMWARE_ARM_CLK_ID;
 
 	/* All of the PLLs derive from the external oscillator. */
 	init.parent_names = (const char *[]){ "osc" };
@@ -200,8 +200,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	/* Get min & max rates set by the firmware */
 	ret = raspberrypi_clock_property(rpi->firmware,
 					 RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
-					 RPI_FIRMWARE_ARM_CLK_ID,
-					 &min_rate);
+					 data->id, &min_rate);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s min freq: %d\n",
 			init.name, ret);
@@ -210,8 +209,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 
 	ret = raspberrypi_clock_property(rpi->firmware,
 					 RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
-					 RPI_FIRMWARE_ARM_CLK_ID,
-					 &max_rate);
+					 data->id, &max_rate);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s max freq: %d\n",
 			init.name, ret);
-- 
git-series 0.9.1

Re: [PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Stefan Wahren]

Hi Maxime,

Am 24.02.20 um 10:06 schrieb Maxime Ripard:
> The driver has really only supported one clock so far and has hardcoded the
> ID used in communications with the firmware in all the functions
> implementing the clock framework hooks. Let's store that in the clock data
> structure so that we can support more clocks later on.

thank you for this series. I looked through it but i couldn't find an
explanation why we need to expose firmware clocks via DT instead of
extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an
exception to get cpufreq working. I prefer to keep it an exception.

Regards
Stefan

Re: [PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1847 bytes --]

Hi Stefan,

On Mon, Feb 24, 2020 at 08:25:46PM +0100, Stefan Wahren wrote:
> Hi Maxime,
>
> Am 24.02.20 um 10:06 schrieb Maxime Ripard:
> > The driver has really only supported one clock so far and has hardcoded the
> > ID used in communications with the firmware in all the functions
> > implementing the clock framework hooks. Let's store that in the clock data
> > structure so that we can support more clocks later on.
>
> thank you for this series. I looked through it but i couldn't find an
> explanation why we need to expose firmware clocks via DT instead of
> extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an
> exception to get cpufreq working. I prefer to keep it an exception.

Thanks for pointing this out, I indeed forgot to address it in my
cover letter or my commit log.

I'm not quite sure what the situation was with the previous
RaspberryPi, but the RPi4 firmware does a bunch of things under the
hood to make sure that everything works as expected:

 - The HSM (and V3D) clocks will be reparented to multiple PLLs
   depending on the rate being asked for.
 - Still depending on the rate, the firmware will adjust the voltage
   of the various PLLs.
 - Depending on the temperature of the CPU and GPU, the firmware will
   change the rate of clocks to throttle in case of the cores
   overheating, with all the fallout that might happen to clocks
   deriving from it.
 - No matter what we choose to do in Linux, this will happen so
   whether or not we want to do it, so doing it behind the firmware's
   back (or the firmware doing it behind Linux's back) will only
   result in troubles, with voltages too low, or the firmware trying
   to access the same register at the same time than the Linux driver
   would, etc.

So all in all, it just seems much easier and safer to use the firmware
clocks.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 2364 bytes --]

Hi Maxime,

On Tue, 2020-02-25 at 10:54 +0100, Maxime Ripard wrote:
> Hi Stefan,
> 
> On Mon, Feb 24, 2020 at 08:25:46PM +0100, Stefan Wahren wrote:
> > Hi Maxime,
> > 
> > Am 24.02.20 um 10:06 schrieb Maxime Ripard:
> > > The driver has really only supported one clock so far and has hardcoded
> > > the
> > > ID used in communications with the firmware in all the functions
> > > implementing the clock framework hooks. Let's store that in the clock data
> > > structure so that we can support more clocks later on.
> > 
> > thank you for this series. I looked through it but i couldn't find an
> > explanation why we need to expose firmware clocks via DT instead of
> > extending clk-bcm2835. The whole pllb / clk-raspberrypi stuff was an
> > exception to get cpufreq working. I prefer to keep it an exception.
> 
> Thanks for pointing this out, I indeed forgot to address it in my
> cover letter or my commit log.
> 
> I'm not quite sure what the situation was with the previous
> RaspberryPi, but the RPi4 firmware does a bunch of things under the
> hood to make sure that everything works as expected:
> 
>  - The HSM (and V3D) clocks will be reparented to multiple PLLs
>    depending on the rate being asked for.
>  - Still depending on the rate, the firmware will adjust the voltage
>    of the various PLLs.
>  - Depending on the temperature of the CPU and GPU, the firmware will
>    change the rate of clocks to throttle in case of the cores
>    overheating, with all the fallout that might happen to clocks
>    deriving from it.
>  - No matter what we choose to do in Linux, this will happen so
>    whether or not we want to do it, so doing it behind the firmware's
>    back (or the firmware doing it behind Linux's back) will only
>    result in troubles, with voltages too low, or the firmware trying
>    to access the same register at the same time than the Linux driver
>    would, etc.
> 
> So all in all, it just seems much easier and safer to use the firmware
> clocks.

I agree with your assesment. Both DVFS and overheating/overvoltage protections
will cause trouble, if not, make a Linux solution impossible while using the
Foundation's firmware.

Please note that, as Stefan says, it'd be nice to keep track of those arguments
somewhere in the commit messages.

Regards,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The driver has really only supported one clock so far and has hardcoded the
> ID used in communications with the firmware in all the functions
> implementing the clock framework hooks. Let's store that in the clock data
> structure so that we can support more clocks later on.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 16/89] clk: bcm: rpi: Add clock id to data

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:18)
> The driver has really only supported one clock so far and has hardcoded the
> ID used in communications with the firmware in all the functions
> implementing the clock framework hooks. Let's store that in the clock data
> structure so that we can support more clocks later on.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 17/89] clk: bcm: rpi: Pass the clocks data to the firmware function

[Maxime Ripard]

The raspberry_clock_property only takes the clock ID as an argument, but
now that we have a clock data structure it makes more sense to just pass
that structure instead.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index e796dabbc641..3b2da62a72f5 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -67,11 +67,12 @@ struct raspberrypi_firmware_prop {
 	__le32 disable_turbo;
 } __packed;
 
-static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32 tag,
-				      u32 clk, u32 *val)
+static int raspberrypi_clock_property(struct rpi_firmware *firmware,
+				      struct raspberrypi_clk_data *data,
+				      u32 tag, u32 *val)
 {
 	struct raspberrypi_firmware_prop msg = {
-		.id = cpu_to_le32(clk),
+		.id = cpu_to_le32(data->id),
 		.val = cpu_to_le32(*val),
 		.disable_turbo = cpu_to_le32(1),
 	};
@@ -94,9 +95,8 @@ static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
 	u32 val = 0;
 	int ret;
 
-	ret = raspberrypi_clock_property(rpi->firmware,
-					 RPI_FIRMWARE_GET_CLOCK_STATE,
-					 data->id, &val);
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_GET_CLOCK_STATE, &val);
 	if (ret)
 		return 0;
 
@@ -113,9 +113,8 @@ static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
 	u32 val = 0;
 	int ret;
 
-	ret = raspberrypi_clock_property(rpi->firmware,
-					 RPI_FIRMWARE_GET_CLOCK_RATE,
-					 data->id, &val);
+	ret = raspberrypi_clock_property(rpi->firmware, data,
+					 RPI_FIRMWARE_GET_CLOCK_RATE, &val);
 	if (ret)
 		return ret;
 
@@ -131,9 +130,9 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
 	int ret;
 
-	ret = raspberrypi_clock_property(rpi->firmware,
+	ret = raspberrypi_clock_property(rpi->firmware, data,
 					 RPI_FIRMWARE_SET_CLOCK_RATE,
-					 data->id, &new_rate);
+					 &new_rate);
 	if (ret)
 		dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d",
 				    clk_hw_get_name(hw), ret);
@@ -198,18 +197,18 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;
 
 	/* Get min & max rates set by the firmware */
-	ret = raspberrypi_clock_property(rpi->firmware,
+	ret = raspberrypi_clock_property(rpi->firmware, data,
 					 RPI_FIRMWARE_GET_MIN_CLOCK_RATE,
-					 data->id, &min_rate);
+					 &min_rate);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s min freq: %d\n",
 			init.name, ret);
 		return ret;
 	}
 
-	ret = raspberrypi_clock_property(rpi->firmware,
+	ret = raspberrypi_clock_property(rpi->firmware, data,
 					 RPI_FIRMWARE_GET_MAX_CLOCK_RATE,
-					 data->id, &max_rate);
+					 &max_rate);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s max freq: %d\n",
 			init.name, ret);
-- 
git-series 0.9.1

Re: [PATCH 17/89] clk: bcm: rpi: Pass the clocks data to the firmware function

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The raspberry_clock_property only takes the clock ID as an argument, but
> now that we have a clock data structure it makes more sense to just pass
> that structure instead.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 17/89] clk: bcm: rpi: Pass the clocks data to the firmware function

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:19)
> The raspberry_clock_property only takes the clock ID as an argument, but
> now that we have a clock data structure it makes more sense to just pass
> that structure instead.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---
>  drivers/clk/bcm/clk-raspberrypi.c | 29 ++++++++++++++---------------
>  1 file changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index e796dabbc641..3b2da62a72f5 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -67,11 +67,12 @@ struct raspberrypi_firmware_prop {
>         __le32 disable_turbo;
>  } __packed;
>  
> -static int raspberrypi_clock_property(struct rpi_firmware *firmware, u32 tag,
> -                                     u32 clk, u32 *val)
> +static int raspberrypi_clock_property(struct rpi_firmware *firmware,
> +                                     struct raspberrypi_clk_data *data,

Can data be const?

> +                                     u32 tag, u32 *val)
>  {
>         struct raspberrypi_firmware_prop msg = {
> -               .id = cpu_to_le32(clk),
> +               .id = cpu_to_le32(data->id),
>                 .val = cpu_to_le32(*val),
>                 .disable_turbo = cpu_to_le32(1),
>         };

[PATCH 18/89] clk: bcm: rpi: Rename is_prepared function

[Maxime Ripard]

The raspberrypi_fw_pll_is_on function doesn't only apply to PLL
registered in the driver, but any clock exposed by the firmware.

Since we also implement the is_prepared hook, make the function
consistent with the other function names, and drop the fw from the
function name.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 3b2da62a72f5..13b7ee148824 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -87,7 +87,7 @@ static int raspberrypi_clock_property(struct rpi_firmware *firmware,
 	return 0;
 }
 
-static int raspberrypi_fw_pll_is_on(struct clk_hw *hw)
+static int raspberrypi_fw_is_prepared(struct clk_hw *hw)
 {
 	struct raspberrypi_clk_data *data =
 		container_of(hw, struct raspberrypi_clk_data, hw);
@@ -170,7 +170,7 @@ static int raspberrypi_pll_determine_rate(struct clk_hw *hw,
 }
 
 static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
-	.is_prepared = raspberrypi_fw_pll_is_on,
+	.is_prepared = raspberrypi_fw_is_prepared,
 	.recalc_rate = raspberrypi_fw_pll_get_rate,
 	.set_rate = raspberrypi_fw_pll_set_rate,
 	.determine_rate = raspberrypi_pll_determine_rate,
-- 
git-series 0.9.1

Re: [PATCH 18/89] clk: bcm: rpi: Rename is_prepared function

[Nicolas Saenz Julienne]

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On Mon, 2020-02-24 at 10:06 +0100, Maxime Ripard wrote:
> The raspberrypi_fw_pll_is_on function doesn't only apply to PLL
> registered in the driver, but any clock exposed by the firmware.
> 
> Since we also implement the is_prepared hook, make the function
> consistent with the other function names, and drop the fw from the
> function name.

It seems you didn't :)

As it does use the fw interface I'd say keep it in the name, with that:

Acked-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>

Thanks!
Nicolas

> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 18/89] clk: bcm: rpi: Rename is_prepared function

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:20)
> The raspberrypi_fw_pll_is_on function doesn't only apply to PLL
> registered in the driver, but any clock exposed by the firmware.
> 
> Since we also implement the is_prepared hook, make the function
> consistent with the other function names, and drop the fw from the
> function name.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 19/89] clk: bcm: rpi: Split pllb clock hooks

[Maxime Ripard]

The driver only supports the pllb for now and all the clock framework hooks
are a mix of the generic firmware interface and the specifics of the pllb.
Since we will support more clocks in the future let's split the generic and
specific hooks

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 13b7ee148824..6ee2c6639604 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -104,8 +104,8 @@ static int raspberrypi_fw_is_prepared(struct clk_hw *hw)
 }
 
 
-static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
-						 unsigned long parent_rate)
+static unsigned long raspberrypi_fw_get_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
 {
 	struct raspberrypi_clk_data *data =
 		container_of(hw, struct raspberrypi_clk_data, hw);
@@ -118,21 +118,27 @@ static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
 	if (ret)
 		return ret;
 
-	return val * RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+	return val;
 }
 
-static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
-				       unsigned long parent_rate)
+static unsigned long raspberrypi_fw_pll_get_rate(struct clk_hw *hw,
+						 unsigned long parent_rate)
+{
+	return raspberrypi_fw_get_rate(hw, parent_rate) *
+		RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+}
+
+static int raspberrypi_fw_set_rate(struct clk_hw *hw, unsigned long rate,
+				   unsigned long parent_rate)
 {
 	struct raspberrypi_clk_data *data =
 		container_of(hw, struct raspberrypi_clk_data, hw);
 	struct raspberrypi_clk *rpi = data->rpi;
-	u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+	u32 _rate = rate;
 	int ret;
 
 	ret = raspberrypi_clock_property(rpi->firmware, data,
-					 RPI_FIRMWARE_SET_CLOCK_RATE,
-					 &new_rate);
+					 RPI_FIRMWARE_SET_CLOCK_RATE, &_rate);
 	if (ret)
 		dev_err_ratelimited(rpi->dev, "Failed to change %s frequency: %d",
 				    clk_hw_get_name(hw), ret);
@@ -140,6 +146,14 @@ static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
 	return ret;
 }
 
+static int raspberrypi_fw_pll_set_rate(struct clk_hw *hw, unsigned long rate,
+				       unsigned long parent_rate)
+{
+	u32 new_rate = rate / RPI_FIRMWARE_PLLB_ARM_DIV_RATE;
+
+	return raspberrypi_fw_set_rate(hw, new_rate, parent_rate);
+}
+
 /*
  * Sadly there is no firmware rate rounding interface. We borrowed it from
  * clk-bcm2835.
-- 
git-series 0.9.1

Re: [PATCH 19/89] clk: bcm: rpi: Split pllb clock hooks

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:21)
> The driver only supports the pllb for now and all the clock framework hooks
> are a mix of the generic firmware interface and the specifics of the pllb.
> Since we will support more clocks in the future let's split the generic and
> specific hooks
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 20/89] clk: bcm: rpi: Make the PLLB registration function return a clk_hw

[Maxime Ripard]

The raspberrypi_register_pllb has been returning an integer so far to
notify whether the functions has exited successfully or not.

However, the OF provider functions in the clock framework require access to
the clk_hw structure so that we can expose those clocks to device tree
consumers.

Since we'll want that for the future clocks, let's return a clk_hw pointer
instead of the return code.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 40 +++++++++++++++++---------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 6ee2c6639604..db5de9073930 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -190,7 +190,7 @@ static const struct clk_ops raspberrypi_firmware_pll_clk_ops = {
 	.determine_rate = raspberrypi_pll_determine_rate,
 };
 
-static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
+static struct clk_hw *raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 {
 	struct raspberrypi_clk_data *data;
 	struct clk_init_data init = {};
@@ -199,7 +199,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 
 	data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	data->rpi = rpi;
 	data->id = RPI_FIRMWARE_ARM_CLK_ID;
 
@@ -217,7 +217,7 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s min freq: %d\n",
 			init.name, ret);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
 	ret = raspberrypi_clock_property(rpi->firmware, data,
@@ -226,13 +226,13 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 	if (ret) {
 		dev_err(rpi->dev, "Failed to get %s max freq: %d\n",
 			init.name, ret);
-		return ret;
+		return ERR_PTR(ret);
 	}
 
 	if (!min_rate || !max_rate) {
 		dev_err(rpi->dev, "Unexpected frequency range: min %u, max %u\n",
 			min_rate, max_rate);
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 	}
 
 	dev_info(rpi->dev, "CPU frequency range: min %u, max %u\n",
@@ -243,7 +243,11 @@ static int raspberrypi_register_pllb(struct raspberrypi_clk *rpi)
 
 	data->hw.init = &init;
 
-	return devm_clk_hw_register(rpi->dev, &data->hw);
+	ret = devm_clk_hw_register(rpi->dev, &data->hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &data->hw;
 }
 
 static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
@@ -258,14 +262,14 @@ static struct clk_fixed_factor raspberrypi_clk_pllb_arm = {
 	},
 };
 
-static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
+static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 {
 	int ret;
 
 	ret = devm_clk_hw_register(rpi->dev, &raspberrypi_clk_pllb_arm.hw);
 	if (ret) {
 		dev_err(rpi->dev, "Failed to initialize pllb_arm\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
 	ret = devm_clk_hw_register_clkdev(rpi->dev,
@@ -273,10 +277,10 @@ static int raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 					  NULL, "cpu0");
 	if (ret) {
 		dev_err(rpi->dev, "Failed to initialize clkdev\n");
-		return ret;
+		return ERR_PTR(ret);
 	}
 
-	return 0;
+	return &raspberrypi_clk_pllb_arm.hw;
 }
 
 static int raspberrypi_clk_probe(struct platform_device *pdev)
@@ -285,7 +289,7 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rpi_firmware *firmware;
 	struct raspberrypi_clk *rpi;
-	int ret;
+	struct clk_hw *hw;
 
 	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware", 0);
 	if (!firmware_node) {
@@ -305,15 +309,15 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	rpi->firmware = firmware;
 	platform_set_drvdata(pdev, rpi);
 
-	ret = raspberrypi_register_pllb(rpi);
-	if (ret) {
-		dev_err(dev, "Failed to initialize pllb, %d\n", ret);
-		return ret;
+	hw = raspberrypi_register_pllb(rpi);
+	if (IS_ERR(hw)) {
+		dev_err(dev, "Failed to initialize pllb, %ld\n", PTR_ERR(hw));
+		return PTR_ERR(hw);
 	}
 
-	ret = raspberrypi_register_pllb_arm(rpi);
-	if (ret)
-		return ret;
+	hw = raspberrypi_register_pllb_arm(rpi);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
 
 	rpi->cpufreq = platform_device_register_data(dev, "raspberrypi-cpufreq",
 						     -1, NULL, 0);
-- 
git-series 0.9.1

Re: [PATCH 20/89] clk: bcm: rpi: Make the PLLB registration function return a clk_hw

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:22)
> The raspberrypi_register_pllb has been returning an integer so far to
> notify whether the functions has exited successfully or not.
> 
> However, the OF provider functions in the clock framework require access to
> the clk_hw structure so that we can expose those clocks to device tree
> consumers.
> 
> Since we'll want that for the future clocks, let's return a clk_hw pointer
> instead of the return code.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 21/89] clk: bcm: rpi: Add DT provider for the clocks

[Maxime Ripard]

For the upcoming registration of the clocks provided by the firmware, make
sure it's exposed to the device tree providers.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index db5de9073930..3f21888a3e3e 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -31,6 +31,8 @@
 
 #define A2W_PLL_FRAC_BITS		20
 
+#define NUM_FW_CLKS			16
+
 struct raspberrypi_clk {
 	struct device *dev;
 	struct rpi_firmware *firmware;
@@ -285,6 +287,7 @@ static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 
 static int raspberrypi_clk_probe(struct platform_device *pdev)
 {
+	struct clk_hw_onecell_data *clk_data;
 	struct device_node *firmware_node;
 	struct device *dev = &pdev->dev;
 	struct rpi_firmware *firmware;
@@ -309,6 +312,11 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	rpi->firmware = firmware;
 	platform_set_drvdata(pdev, rpi);
 
+	clk_data = devm_kzalloc(dev, struct_size(clk_data, hws, NUM_FW_CLKS),
+				GFP_KERNEL);
+	if (!clk_data)
+		return -ENOMEM;
+
 	hw = raspberrypi_register_pllb(rpi);
 	if (IS_ERR(hw)) {
 		dev_err(dev, "Failed to initialize pllb, %ld\n", PTR_ERR(hw));
@@ -318,6 +326,13 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	hw = raspberrypi_register_pllb_arm(rpi);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
+	clk_data->hws[RPI_FIRMWARE_ARM_CLK_ID] = hw;
+	clk_data->num = RPI_FIRMWARE_ARM_CLK_ID + 1;
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
+					  clk_data);
+	if (ret)
+		return ret;
 
 	rpi->cpufreq = platform_device_register_data(dev, "raspberrypi-cpufreq",
 						     -1, NULL, 0);
-- 
git-series 0.9.1

Re: [PATCH 21/89] clk: bcm: rpi: Add DT provider for the clocks

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:23)
> For the upcoming registration of the clocks provided by the firmware, make
> sure it's exposed to the device tree providers.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Stephen Boyd <sboyd@kernel.org>

[PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[Maxime Ripard]

The firmware has an interface to discover the clocks it exposes.

Let's use it to discover, register the clocks in the clocks framework and
then expose them through the device tree for consumers to use them.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: linux-clk@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/clk-raspberrypi.c          | 105 +++++++++++++++++++---
 include/soc/bcm2835/raspberrypi-firmware.h |   5 +-
 2 files changed, 98 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 3f21888a3e3e..bf6a1e2dc099 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -285,6 +285,95 @@ static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
 	return &raspberrypi_clk_pllb_arm.hw;
 }
 
+static long raspberrypi_fw_dumb_round_rate(struct clk_hw *hw,
+					   unsigned long rate,
+					   unsigned long *parent_rate)
+{
+	return rate;
+}
+
+static const struct clk_ops raspberrypi_firmware_clk_ops = {
+	.is_prepared	= raspberrypi_fw_is_prepared,
+	.recalc_rate	= raspberrypi_fw_get_rate,
+	.round_rate	= raspberrypi_fw_dumb_round_rate,
+	.set_rate	= raspberrypi_fw_set_rate,
+};
+
+static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
+					       unsigned int parent,
+					       unsigned int id)
+{
+	struct raspberrypi_clk_data *data;
+	struct clk_init_data init = {};
+	int ret;
+
+	if (id == RPI_FIRMWARE_ARM_CLK_ID) {
+		struct clk_hw *hw;
+
+		hw = raspberrypi_register_pllb(rpi);
+		if (IS_ERR(hw)) {
+			dev_err(rpi->dev, "Failed to initialize pllb, %ld\n",
+				PTR_ERR(hw));
+			return hw;
+		}
+
+		hw = raspberrypi_register_pllb_arm(rpi);
+		if (IS_ERR(hw))
+			return hw;
+
+		return hw;
+	}
+
+	data = devm_kzalloc(rpi->dev, sizeof(data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+	data->rpi = rpi;
+	data->id = id;
+
+	init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id);
+	init.ops = &raspberrypi_firmware_clk_ops;
+	init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;
+
+	data->hw.init = &init;
+
+	ret = devm_clk_hw_register(rpi->dev, &data->hw);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &data->hw;
+}
+
+static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
+				       struct clk_hw_onecell_data *data)
+{
+	struct rpi_firmware_get_clocks_response *clks;
+	size_t clks_size = NUM_FW_CLKS * sizeof(*clks);
+	int ret;
+
+	clks = devm_kzalloc(rpi->dev, clks_size, GFP_KERNEL);
+	if (!clks)
+		return -ENOMEM;
+
+	ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS,
+				    clks, clks_size);
+	if (ret)
+		return ret;
+
+	while (clks->id) {
+		struct clk_hw *hw;
+
+		hw = raspberrypi_clk_register(rpi, clks->parent, clks->id);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+
+		data->hws[clks->id] = hw;
+		data->num = clks->id + 1;
+		clks++;
+	}
+
+	return 0;
+}
+
 static int raspberrypi_clk_probe(struct platform_device *pdev)
 {
 	struct clk_hw_onecell_data *clk_data;
@@ -292,7 +381,7 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct rpi_firmware *firmware;
 	struct raspberrypi_clk *rpi;
-	struct clk_hw *hw;
+	int ret;
 
 	firmware_node = of_parse_phandle(dev->of_node, "raspberrypi,firmware", 0);
 	if (!firmware_node) {
@@ -317,17 +406,9 @@ static int raspberrypi_clk_probe(struct platform_device *pdev)
 	if (!clk_data)
 		return -ENOMEM;
 
-	hw = raspberrypi_register_pllb(rpi);
-	if (IS_ERR(hw)) {
-		dev_err(dev, "Failed to initialize pllb, %ld\n", PTR_ERR(hw));
-		return PTR_ERR(hw);
-	}
-
-	hw = raspberrypi_register_pllb_arm(rpi);
-	if (IS_ERR(hw))
-		return PTR_ERR(hw);
-	clk_data->hws[RPI_FIRMWARE_ARM_CLK_ID] = hw;
-	clk_data->num = RPI_FIRMWARE_ARM_CLK_ID + 1;
+	ret = raspberrypi_discover_clocks(rpi, clk_data);
+	if (ret)
+		return ret;
 
 	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get,
 					  clk_data);
diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 7800e12ee042..e5b7a41bba6b 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -135,6 +135,11 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
 };
 
+struct rpi_firmware_get_clocks_response {
+	__le32	parent;
+	__le32	id;
+};
+
 #if IS_ENABLED(CONFIG_RASPBERRYPI_FIRMWARE)
 int rpi_firmware_property(struct rpi_firmware *fw,
 			  u32 tag, void *data, size_t len);
-- 
git-series 0.9.1

Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[kbuild test robot]

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next anholt/for-next v5.6-rc3 next-20200224]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-vc4-Support-BCM2711-Display-Pipeline/20200224-172730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/clk/bcm/clk-raspberrypi.c:327:31-37: ERROR: application of sizeof to pointer

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[PATCH] clk: bcm: rpi: fix noderef.cocci warnings

[kbuild test robot]

From: kbuild test robot <lkp@intel.com>

drivers/clk/bcm/clk-raspberrypi.c:327:31-37: ERROR: application of sizeof to pointer

 sizeof when applied to a pointer typed expression gives the size of
 the pointer

Generated by: scripts/coccinelle/misc/noderef.cocci

Fixes: 56ccc5cfbb47 ("clk: bcm: rpi: Discover the firmware clocks")
CC: Maxime Ripard <maxime@cerno.tech>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-vc4-Support-BCM2711-Display-Pipeline/20200224-172730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next

 clk-raspberrypi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -324,7 +324,7 @@ static struct clk_hw *raspberrypi_clk_re
 		return hw;
 	}
 
-	data = devm_kzalloc(rpi->dev, sizeof(data), GFP_KERNEL);
+	data = devm_kzalloc(rpi->dev, sizeof(*data), GFP_KERNEL);
 	if (!data)
 		return ERR_PTR(-ENOMEM);
 	data->rpi = rpi;

Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[Florian Fainelli]

On 2/24/20 1:06 AM, Maxime Ripard wrote:
> The firmware has an interface to discover the clocks it exposes.
> 
> Let's use it to discover, register the clocks in the clocks framework and
> then expose them through the device tree for consumers to use them.
> 
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-clk@vger.kernel.org
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

That seems like a re-implementaiton of SCMI without all of its
protocols, without being able to use the existing drivers, maybe a
firmware update should be considered so standard drivers can be leveraged?
-- 
Florian

Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1042 bytes --]

Hi Florian,

On Mon, Feb 24, 2020 at 10:15:32AM -0800, Florian Fainelli wrote:
> On 2/24/20 1:06 AM, Maxime Ripard wrote:
> > The firmware has an interface to discover the clocks it exposes.
> >
> > Let's use it to discover, register the clocks in the clocks framework and
> > then expose them through the device tree for consumers to use them.
> >
> > Cc: Michael Turquette <mturquette@baylibre.com>
> > Cc: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-clk@vger.kernel.org
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
>
> That seems like a re-implementaiton of SCMI without all of its
> protocols, without being able to use the existing drivers, maybe a
> firmware update should be considered so standard drivers can be leveraged?

I'm not really qualified to talk about how the firmware will evolve in
the future, but you're right that it looks a lot like what SCMI can
do.

Even if a firmware update was to support SCMI at some point, since the
firmware is flashed in an EEPROM, we'd still have to support that
interface.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[kbuild test robot]

Hi Maxime,

I love your patch! Perhaps something to improve:

[auto build test WARNING on clk/clk-next]
[also build test WARNING on robh/for-next anholt/for-next v5.6-rc3 next-20200224]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Maxime-Ripard/drm-vc4-Support-BCM2711-Display-Pipeline/20200224-172730
base:   https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git clk-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-173-ge0787745-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/clk/bcm/clk-raspberrypi.c:365:56: sparse: sparse: incorrect type in argument 2 (different base types)
>> drivers/clk/bcm/clk-raspberrypi.c:365:56: sparse:    expected unsigned int parent
>> drivers/clk/bcm/clk-raspberrypi.c:365:56: sparse:    got restricted __le32 [usertype] parent
   drivers/clk/bcm/clk-raspberrypi.c:365:70: sparse: sparse: incorrect type in argument 3 (different base types)
>> drivers/clk/bcm/clk-raspberrypi.c:365:70: sparse:    expected unsigned int id
>> drivers/clk/bcm/clk-raspberrypi.c:365:70: sparse:    got restricted __le32 [usertype] id
>> drivers/clk/bcm/clk-raspberrypi.c:369:31: sparse: sparse: restricted __le32 degrades to integer
   drivers/clk/bcm/clk-raspberrypi.c:370:33: sparse: sparse: restricted __le32 degrades to integer

vim +365 drivers/clk/bcm/clk-raspberrypi.c

   345	
   346	static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
   347					       struct clk_hw_onecell_data *data)
   348	{
   349		struct rpi_firmware_get_clocks_response *clks;
   350		size_t clks_size = NUM_FW_CLKS * sizeof(*clks);
   351		int ret;
   352	
   353		clks = devm_kzalloc(rpi->dev, clks_size, GFP_KERNEL);
   354		if (!clks)
   355			return -ENOMEM;
   356	
   357		ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS,
   358					    clks, clks_size);
   359		if (ret)
   360			return ret;
   361	
   362		while (clks->id) {
   363			struct clk_hw *hw;
   364	
 > 365			hw = raspberrypi_clk_register(rpi, clks->parent, clks->id);
   366			if (IS_ERR(hw))
   367				return PTR_ERR(hw);
   368	
 > 369			data->hws[clks->id] = hw;
   370			data->num = clks->id + 1;
   371			clks++;
   372		}
   373	
   374		return 0;
   375	}
   376	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Re: [PATCH 22/89] clk: bcm: rpi: Discover the firmware clocks

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:24)
> The firmware has an interface to discover the clocks it exposes.
> 
> Let's use it to discover, register the clocks in the clocks framework and
> then expose them through the device tree for consumers to use them.

Everyone's doing it! :)

> diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
> index 3f21888a3e3e..bf6a1e2dc099 100644
> --- a/drivers/clk/bcm/clk-raspberrypi.c
> +++ b/drivers/clk/bcm/clk-raspberrypi.c
> @@ -285,6 +285,95 @@ static struct clk_hw *raspberrypi_register_pllb_arm(struct raspberrypi_clk *rpi)
>         return &raspberrypi_clk_pllb_arm.hw;
>  }
>  
> +static long raspberrypi_fw_dumb_round_rate(struct clk_hw *hw,
> +                                          unsigned long rate,
> +                                          unsigned long *parent_rate)
> +{
> +       return rate;

Can we get a comment here like "The firmware does the rounding but
doesn't tell us so we have to assume anything goes!"

> +}
> +
> +static const struct clk_ops raspberrypi_firmware_clk_ops = {
> +       .is_prepared    = raspberrypi_fw_is_prepared,
> +       .recalc_rate    = raspberrypi_fw_get_rate,
> +       .round_rate     = raspberrypi_fw_dumb_round_rate,
> +       .set_rate       = raspberrypi_fw_set_rate,
> +};
> +
> +static struct clk_hw *raspberrypi_clk_register(struct raspberrypi_clk *rpi,
> +                                              unsigned int parent,
> +                                              unsigned int id)
> +{
> +       struct raspberrypi_clk_data *data;
> +       struct clk_init_data init = {};
> +       int ret;
> +
> +       if (id == RPI_FIRMWARE_ARM_CLK_ID) {
> +               struct clk_hw *hw;
> +
> +               hw = raspberrypi_register_pllb(rpi);
> +               if (IS_ERR(hw)) {
> +                       dev_err(rpi->dev, "Failed to initialize pllb, %ld\n",
> +                               PTR_ERR(hw));
> +                       return hw;
> +               }
> +
> +               hw = raspberrypi_register_pllb_arm(rpi);
> +               if (IS_ERR(hw))
> +                       return hw;
> +
> +               return hw;

Why the double return? Not just

	return raspberrypi_register_pllb_arm(rpi);

> +       }
> +
> +       data = devm_kzalloc(rpi->dev, sizeof(data), GFP_KERNEL);
> +       if (!data)
> +               return ERR_PTR(-ENOMEM);
> +       data->rpi = rpi;
> +       data->id = id;
> +
> +       init.name = devm_kasprintf(rpi->dev, GFP_KERNEL, "fw-clk-%u", id);
> +       init.ops = &raspberrypi_firmware_clk_ops;
> +       init.flags = CLK_GET_RATE_NOCACHE | CLK_IGNORE_UNUSED;

Why ignore unused? Doesn't seem to support enable/disable anyway so not
sure this flag adds any value.

> +
> +       data->hw.init = &init;
> +
> +       ret = devm_clk_hw_register(rpi->dev, &data->hw);
> +       if (ret)
> +               return ERR_PTR(ret);
> +
> +       return &data->hw;
> +}
> +
> +static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
> +                                      struct clk_hw_onecell_data *data)
> +{
> +       struct rpi_firmware_get_clocks_response *clks;
> +       size_t clks_size = NUM_FW_CLKS * sizeof(*clks);
> +       int ret;
> +
> +       clks = devm_kzalloc(rpi->dev, clks_size, GFP_KERNEL);

Use devm_kcalloc to avoid overflow and indicate it's an array.

> +       if (!clks)
> +               return -ENOMEM;
> +
> +       ret = rpi_firmware_property(rpi->firmware, RPI_FIRMWARE_GET_CLOCKS,
> +                                   clks, clks_size);
> +       if (ret)
> +               return ret;
> +
> +       while (clks->id) {
> +               struct clk_hw *hw;
> +
> +               hw = raspberrypi_clk_register(rpi, clks->parent, clks->id);
> +               if (IS_ERR(hw))
> +                       return PTR_ERR(hw);
> +
> +               data->hws[clks->id] = hw;
> +               data->num = clks->id + 1;
> +               clks++;
> +       }
> +
> +       return 0;
> +}
> +
>  static int raspberrypi_clk_probe(struct platform_device *pdev)
>  {
>         struct clk_hw_onecell_data *clk_data;
> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
> index 7800e12ee042..e5b7a41bba6b 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -135,6 +135,11 @@ enum rpi_firmware_property_tag {
>         RPI_FIRMWARE_GET_DMA_CHANNELS =                       0x00060001,
>  };
>  
> +struct rpi_firmware_get_clocks_response {
> +       __le32  parent;
> +       __le32  id;

Why double spaced or tabbed out?

> +};
> +
>  #if IS_ENABLED(CONFIG_RASPBERRYPI_FIRMWARE)
>  int rpi_firmware_property(struct rpi_firmware *fw,

[PATCH 27/89] clk: bcm: Add BCM2711 DVP driver

[Maxime Ripard]

The HDMI block has a block that controls clocks and reset signals to the
HDMI0 and HDMI1 controllers.

Let's expose that through a clock driver implementing a clock and reset
provider.

Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-clk@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/clk/bcm/Kconfig           |   1 +-
 drivers/clk/bcm/Makefile          |   1 +-
 drivers/clk/bcm/clk-bcm2711-dvp.c | 113 +++++++++++++++++++++++++++++++-
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/clk/bcm/clk-bcm2711-dvp.c

diff --git a/drivers/clk/bcm/Kconfig b/drivers/clk/bcm/Kconfig
index 8c83977a7dc4..03bbd8040451 100644
--- a/drivers/clk/bcm/Kconfig
+++ b/drivers/clk/bcm/Kconfig
@@ -4,6 +4,7 @@ config CLK_BCM2835
 	depends on ARCH_BCM2835 || ARCH_BRCMSTB || COMPILE_TEST
 	depends on COMMON_CLK
 	default ARCH_BCM2835 || ARCH_BRCMSTB
+	select RESET_SIMPLE
 	help
 	  Enable common clock framework support for Broadcom BCM2835
 	  SoCs.
diff --git a/drivers/clk/bcm/Makefile b/drivers/clk/bcm/Makefile
index 0070ddf6cdd2..2c1349062147 100644
--- a/drivers/clk/bcm/Makefile
+++ b/drivers/clk/bcm/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_CLK_BCM_KONA)	+= clk-kona-setup.o
 obj-$(CONFIG_CLK_BCM_KONA)	+= clk-bcm281xx.o
 obj-$(CONFIG_CLK_BCM_KONA)	+= clk-bcm21664.o
 obj-$(CONFIG_COMMON_CLK_IPROC)	+= clk-iproc-armpll.o clk-iproc-pll.o clk-iproc-asiu.o
+obj-$(CONFIG_CLK_BCM2835)	+= clk-bcm2711-dvp.o
 obj-$(CONFIG_CLK_BCM2835)	+= clk-bcm2835.o
 obj-$(CONFIG_CLK_BCM2835)	+= clk-bcm2835-aux.o
 obj-$(CONFIG_CLK_RASPBERRYPI)	+= clk-raspberrypi.o
diff --git a/drivers/clk/bcm/clk-bcm2711-dvp.c b/drivers/clk/bcm/clk-bcm2711-dvp.c
new file mode 100644
index 000000000000..f4773cc92724
--- /dev/null
+++ b/drivers/clk/bcm/clk-bcm2711-dvp.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright 2020 Cerno
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/reset-controller.h>
+#include <linux/reset/reset-simple.h>
+
+#define DVP_HT_RPI_SW_INIT	0x04
+#define DVP_HT_RPI_MISC_CONFIG	0x08
+
+#define NR_CLOCKS	2
+#define NR_RESETS	6
+
+struct clk_dvp {
+	struct clk			*clks[NR_CLOCKS];
+	struct clk_onecell_data		clk_data;
+	struct reset_simple_data	reset;
+};
+
+static int clk_dvp_probe(struct platform_device *pdev)
+{
+	struct resource *res;
+	struct clk_dvp *dvp;
+	void __iomem *base;
+	const char *parent;
+	int ret;
+
+	dvp = devm_kzalloc(&pdev->dev, sizeof(*dvp), GFP_KERNEL);
+	if (!dvp)
+		return -ENOMEM;
+	platform_set_drvdata(pdev, dvp);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
+
+	dvp->reset.rcdev.owner = THIS_MODULE;
+	dvp->reset.rcdev.nr_resets = NR_RESETS;
+	dvp->reset.rcdev.ops = &reset_simple_ops;
+	dvp->reset.rcdev.of_node = pdev->dev.of_node;
+	dvp->reset.membase = base + DVP_HT_RPI_SW_INIT;
+	spin_lock_init(&dvp->reset.lock);
+
+	ret = reset_controller_register(&dvp->reset.rcdev);
+	if (ret)
+		return ret;
+
+	parent = of_clk_get_parent_name(pdev->dev.of_node, 0);
+	if (!parent)
+		goto unregister_reset;
+
+	dvp->clks[0] = clk_register_gate(&pdev->dev, "hdmi0-108MHz",
+					 parent, CLK_IS_CRITICAL,
+					 base + DVP_HT_RPI_MISC_CONFIG, 3,
+					 CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);
+	if (IS_ERR(dvp->clks[0])) {
+		ret = PTR_ERR(dvp->clks[0]);
+		goto unregister_reset;
+	}
+
+	dvp->clks[1] = clk_register_gate(&pdev->dev, "hdmi1-108MHz",
+					 parent, CLK_IS_CRITICAL,
+					 base + DVP_HT_RPI_MISC_CONFIG, 4,
+					 CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);
+	if (IS_ERR(dvp->clks[1])) {
+		ret = PTR_ERR(dvp->clks[1]);
+		goto unregister_clk0;
+	}
+
+	dvp->clk_data.clks = dvp->clks;
+	dvp->clk_data.clk_num = NR_CLOCKS;
+	of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
+			    &dvp->clk_data);
+
+	return 0;
+
+
+unregister_clk0:
+	clk_unregister_gate(dvp->clks[0]);
+
+unregister_reset:
+	reset_controller_unregister(&dvp->reset.rcdev);
+	return ret;
+};
+
+static int clk_dvp_remove(struct platform_device *pdev)
+{
+	struct clk_dvp *dvp = platform_get_drvdata(pdev);
+
+	clk_unregister_gate(dvp->clks[1]);
+	clk_unregister_gate(dvp->clks[0]);
+	reset_controller_unregister(&dvp->reset.rcdev);
+
+	return 0;
+}
+
+static const struct of_device_id clk_dvp_dt_ids[] = {
+	{ .compatible = "brcm,brcm2711-dvp", },
+	{ /* sentinel */ },
+};
+
+static struct platform_driver clk_dvp_driver = {
+	.probe	= clk_dvp_probe,
+	.remove	= clk_dvp_remove,
+	.driver	= {
+		.name		= "brcm2711-dvp",
+		.of_match_table	= clk_dvp_dt_ids,
+	},
+};
+module_platform_driver(clk_dvp_driver);
-- 
git-series 0.9.1

Re: [PATCH 27/89] clk: bcm: Add BCM2711 DVP driver

[Stephen Boyd]

Quoting Maxime Ripard (2020-02-24 01:06:29)
> diff --git a/drivers/clk/bcm/clk-bcm2711-dvp.c b/drivers/clk/bcm/clk-bcm2711-dvp.c
> new file mode 100644
> index 000000000000..f4773cc92724
> --- /dev/null
> +++ b/drivers/clk/bcm/clk-bcm2711-dvp.c
> @@ -0,0 +1,113 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2020 Cerno
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset-controller.h>
> +#include <linux/reset/reset-simple.h>
> +
> +#define DVP_HT_RPI_SW_INIT     0x04
> +#define DVP_HT_RPI_MISC_CONFIG 0x08
> +
> +#define NR_CLOCKS      2
> +#define NR_RESETS      6
> +
> +struct clk_dvp {
> +       struct clk                      *clks[NR_CLOCKS];
> +       struct clk_onecell_data         clk_data;
> +       struct reset_simple_data        reset;
> +};
> +
> +static int clk_dvp_probe(struct platform_device *pdev)
> +{
> +       struct resource *res;
> +       struct clk_dvp *dvp;
> +       void __iomem *base;
> +       const char *parent;
> +       int ret;
> +
> +       dvp = devm_kzalloc(&pdev->dev, sizeof(*dvp), GFP_KERNEL);
> +       if (!dvp)
> +               return -ENOMEM;
> +       platform_set_drvdata(pdev, dvp);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       base = devm_ioremap_resource(&pdev->dev, res);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
> +
> +       dvp->reset.rcdev.owner = THIS_MODULE;
> +       dvp->reset.rcdev.nr_resets = NR_RESETS;
> +       dvp->reset.rcdev.ops = &reset_simple_ops;
> +       dvp->reset.rcdev.of_node = pdev->dev.of_node;
> +       dvp->reset.membase = base + DVP_HT_RPI_SW_INIT;
> +       spin_lock_init(&dvp->reset.lock);
> +
> +       ret = reset_controller_register(&dvp->reset.rcdev);
> +       if (ret)
> +               return ret;
> +
> +       parent = of_clk_get_parent_name(pdev->dev.of_node, 0);
> +       if (!parent)
> +               goto unregister_reset;
> +
> +       dvp->clks[0] = clk_register_gate(&pdev->dev, "hdmi0-108MHz",
> +                                        parent, CLK_IS_CRITICAL,
> +                                        base + DVP_HT_RPI_MISC_CONFIG, 3,
> +                                        CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);
> +       if (IS_ERR(dvp->clks[0])) {
> +               ret = PTR_ERR(dvp->clks[0]);
> +               goto unregister_reset;
> +       }
> +
> +       dvp->clks[1] = clk_register_gate(&pdev->dev, "hdmi1-108MHz",
> +                                        parent, CLK_IS_CRITICAL,
> +                                        base + DVP_HT_RPI_MISC_CONFIG, 4,
> +                                        CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);

Can we use clk_hw APIs, document why CLK_IS_CRITICAL, and use something
like clk_hw_register_gate_parent_data() so that we don't have to use
of_clk_get_parent_name() above?


> +       if (IS_ERR(dvp->clks[1])) {
> +               ret = PTR_ERR(dvp->clks[1]);
> +               goto unregister_clk0;
> +       }
> +
> +       dvp->clk_data.clks = dvp->clks;
> +       dvp->clk_data.clk_num = NR_CLOCKS;
> +       of_clk_add_provider(pdev->dev.of_node, of_clk_src_onecell_get,
> +                           &dvp->clk_data);

This can fail. Please recover. Also, please use clk_hw based APIs.

> +
> +       return 0;
> +
> +
> +unregister_clk0:
> +       clk_unregister_gate(dvp->clks[0]);
> +
> +unregister_reset:
> +       reset_controller_unregister(&dvp->reset.rcdev);
> +       return ret;
> +};
> +
> +static int clk_dvp_remove(struct platform_device *pdev)
> +{
> +       struct clk_dvp *dvp = platform_get_drvdata(pdev);
> +
> +       clk_unregister_gate(dvp->clks[1]);
> +       clk_unregister_gate(dvp->clks[0]);
> +       reset_controller_unregister(&dvp->reset.rcdev);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id clk_dvp_dt_ids[] = {
> +       { .compatible = "brcm,brcm2711-dvp", },
> +       { /* sentinel */ },

Please drop comma after this so we can't put anything after lest it
cause a compiler error.

> +};
> +
> +static struct platform_driver clk_dvp_driver = {
> +       .probe  = clk_dvp_probe,
> +       .remove = clk_dvp_remove,
> +       .driver = {
> +               .name           = "brcm2711-dvp",
> +               .of_match_table = clk_dvp_dt_ids,
> +       },
> +};
> +module_platform_driver(clk_dvp_driver);
> -- 
> git-series 0.9.1

Re: [PATCH 27/89] clk: bcm: Add BCM2711 DVP driver

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1081 bytes --]

Hi Stephen,

On Thu, Mar 12, 2020 at 06:00:59PM -0700, Stephen Boyd wrote:
> > +       dvp->clks[1] = clk_register_gate(&pdev->dev, "hdmi1-108MHz",
> > +                                        parent, CLK_IS_CRITICAL,
> > +                                        base + DVP_HT_RPI_MISC_CONFIG, 4,
> > +                                        CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);
>
> Can we use clk_hw APIs, document why CLK_IS_CRITICAL, and use something
> like clk_hw_register_gate_parent_data() so that we don't have to use
> of_clk_get_parent_name() above?

That function is new to me, and I'm not sure how I'm supposed to use it?

It looks like clk_hw_register_gate, clk_hw_register_gate_parent_hw and
clk_hw_register_gate_parent_data all call __clk_hw_register_gate with
the same arguments, each expecting the parent_name, so they look
equivalent?

It looks like the original intent was to have the parent name, clk_hw
or clk_parent_data as argument, but the macro itself was copy pasted
without changing the arguments it's calling __clk_hw_register_gate
with?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 27/89] clk: bcm: Add BCM2711 DVP driver

[Stephen Boyd]

Quoting Maxime Ripard (2020-03-23 03:56:16)
> Hi Stephen,
> 
> On Thu, Mar 12, 2020 at 06:00:59PM -0700, Stephen Boyd wrote:
> > > +       dvp->clks[1] = clk_register_gate(&pdev->dev, "hdmi1-108MHz",
> > > +                                        parent, CLK_IS_CRITICAL,
> > > +                                        base + DVP_HT_RPI_MISC_CONFIG, 4,
> > > +                                        CLK_GATE_SET_TO_DISABLE, &dvp->reset.lock);
> >
> > Can we use clk_hw APIs, document why CLK_IS_CRITICAL, and use something
> > like clk_hw_register_gate_parent_data() so that we don't have to use
> > of_clk_get_parent_name() above?
> 
> That function is new to me, and I'm not sure how I'm supposed to use it?
> 
> It looks like clk_hw_register_gate, clk_hw_register_gate_parent_hw and
> clk_hw_register_gate_parent_data all call __clk_hw_register_gate with
> the same arguments, each expecting the parent_name, so they look
> equivalent?
> 
> It looks like the original intent was to have the parent name, clk_hw
> or clk_parent_data as argument, but the macro itself was copy pasted
> without changing the arguments it's calling __clk_hw_register_gate
> with?
> 

Yeah! It looks like nobody has tried to use it yet so you've come across
that problem where nobody reviews things and I just merge it anyway.
I'll send a fix shortly.