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 <[email protected]>
Cc: Stephen Boyd <[email protected]>
Cc: [email protected]
Signed-off-by: Maxime Ripard <[email protected]>
---
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
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
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
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
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 <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Cc: [email protected]
> Signed-off-by: Maxime Ripard <[email protected]>
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
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 <[email protected]>
> > Cc: Stephen Boyd <[email protected]>
> > Cc: [email protected]
> > Signed-off-by: Maxime Ripard <[email protected]>
>
> 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