Re: [linux-sunxi] [PATCH v4 5/6] ARM: sun7i: Convert to CCU

[Andre Przywara]

Hi,

On 29/06/17 12:49, Maxime Ripard wrote:
> On Thu, Jun 29, 2017 at 11:57:05AM +0100, Andre Przywara wrote:
>> Hi,
>>
>> On 25/06/17 21:45, Priit Laes wrote:
>>> Convert sun7i-a20.dtsi to new CCU driver.
>>
>> I know that some people hat^Wget annoyed by me asking this, but anyway:
>>
>> Why do we actually need this?
>>
>> This ultimately makes the DT incompatible with older kernels (as
>> actually shipped by distros today).
>>
>> So if we for instance use UEFI boot or otherwise just use "one golden
>> DT" to drive all kernels (like using the DT from U-Boot), we now don't
>> have one good DT that fits all.
> 
> What is broken exactly?

Having *one* DT (in U-Boot, for instance), and just passing this one via
UEFI to the kernel - without knowing *which* exact kernel this will be
(older Linux, newer Linux, some *BSD). Using this we would need to have
the current DT in there (because we would break older kernels
otherwise). Which means we can't benefit from newer DTs and clocks in
there. Which would counteract the actual purpose of this change
(improved clock support) - hence my question.
In UEFI boot land we generally don't consider per-kernel DTs, as the DT
is referenced in the UEFI system table. Yes, grub can pass on a specific
DT, but this is considered a hacking and development vehicle (for people
like you and me) and not meant for public consumption.
So booting via UEFI mandates *one* valid DT for a particular system.

>> This is really a showstopper for boards which ship a DT in firmware
>> (in SPI flash, for instance, or on some eMMC).
>>
>> So:
>> - Do we actually need to change the .dtsi? The old .dtsi should still work.
> 
> It does.
> 
>> - Is there anything that the new and fancy clocks gives us over the
>> existing clocks? If yes, that should be a  stated in the commit message
>> or cover letter.
> 
> Yes, support for all the clocks used in the SoC, and not a single
> fraction of them

Fair enough, but this should be mentioned somewhere.

> (which will reduce the number of additions of new
> bindings and drivers, which will in turn make the DT have less
> changes, which will make it far more easier for distros and / or
> firmwares to ship an immutable DT).

Well, as far as we only talk about *additions* (or compatible changes) I
don't see much of an issue. There will always be feature updates, and
this would just boil down to update the DT to make use of them. Older
kernels would just ignore any new and fancy nodes.

>> - Why do we change the clocks for those older SoCs in the first place?
>> Can't we just keep on using what worked for years?
> 
> Please tell me where the displays clocks, CSI or HDMI clocks are in
> the old code.

Fair enough, but as mentioned above that would have been a good
rationale in some commit message or cover letter. As it stands now it
reads to me as: "Let's break stuff because sunxi-ng is much cooler" ;-)

>> I think we really can't remove the old code anyway.
> 
> We don't.
> 
>> The new clock driver moves information from the DT into the kernel. That
>> means it is no longer available for a DT consumer and the SoC details
>> (which clocks is located where, for instance), have to be replicated to
>> other DT users (U-Boot, *BSD, you-name-it). We already came across this
>> issue when looking at converting U-Boot over to use DT clocks.
>> Also it ultimately requires kernel changes for each new SoC, even if it
>> only differs in some detail which could be perfectly modelled in DT
>> (think of H3 vs. H5).
> 
> Doing otherwise would also assume that you have a perfect
> understanding of all the clocks interactions, relationship and
> computations from day one, which is something the old code proved that
> it was unreasonable.

I completely understand the reasoning from a point of view like five
years ago. But in the meantime I think we have a pretty good
understanding of how the clocks work - sunxi-ng having almost complete
support proves this. And we also know how to abstract this properly, as
this is now done inside the driver. What's just unfortunate is that
those abstraction are modelled inside the *Linux* *driver* file, and not
inside DT. I understand that it's easier to just model this with some
linked C structs, but this now has the unfortunate side effect of
requiring kernel changes for slightly different SoCs. If those details
would be described in the DT, a vendor could just provide a .dtb (for
instance in some on-board memory) and we could boot existing
distributions (or installers) without further ado.

> The new binding also makes it easier to add SCPI that you're
> interested in iirc, where you basically just have to change the CCU
> compatible, and be done with it as long as you use the same IDs.

That's an interesting argument, but I wonder if it's that hard to change
all clock references instead.

> It also lowers the barrier of entry for people that would want to
> write new drivers, since the first thing you'd need to do otherwise
> would be to 

Re: [linux-sunxi] [PATCH v4 5/6] ARM: sun7i: Convert to CCU

[Maxime Ripard]

On Thu, Jun 29, 2017 at 01:28:12PM +0200, Emmanuel Vadot wrote:
> On Thu, 29 Jun 2017 11:57:05 +0100
> Andre Przywara  wrote:
> 
> > Hi,
> > 
> > On 25/06/17 21:45, Priit Laes wrote:
> > > Convert sun7i-a20.dtsi to new CCU driver.
> > 
> > I know that some people hat^Wget annoyed by me asking this, but anyway:
> > 
> > Why do we actually need this?
> 
>  No. I can understand the need for clkng/sunxi-ng/whatever you call it,
> it's not that bad (but see below) to add a new SoC on FreeBSD now that
> I've added the framework, but breaking old SoC that were perfectly fine
> isn't acceptable.

We haven't broken it.

>  It also mean that, on FreeBSD, we still have patches for sun7i dts to
> add hdmi support (which we have since a year or so) because last time
> someone (I think plaes) wanted to add clock node for it, it was said
> that it was needed to move to clkng first.

This is a circular argument. It wouldn't have been the case with
sunxi-ng, since we would have had that clock from the start...

> > This ultimately makes the DT incompatible with older kernels (as
> > actually shipped by distros today).
> 
>  Yes, right now sun5i support is broken in FreeBSD because I couldn't
> find the time to make a driver for it yet.

Probably because you merged new DTs without updating the code. That
has nothing to do with backward compatibility, the old DT would still
work fine.

> > So if we for instance use UEFI boot or otherwise just use "one golden
> > DT" to drive all kernels (like using the DT from U-Boot), we now don't
> > have one good DT that fits all. This is really a showstopper for boards
> > which ship a DT in firmware (in SPI flash, for instance, or on some eMMC).
> > So:
> > - Do we actually need to change the .dtsi? The old .dtsi should still work.
> > - Is there anything that the new and fancy clocks gives us over the
> > existing clocks? If yes, that should be a  stated in the commit message
> > or cover letter.
> > - Why do we change the clocks for those older SoCs in the first place?
> > Can't we just keep on using what worked for years? I think we really
> > can't remove the old code anyway.
> > 
> > The new clock driver moves information from the DT into the kernel. That
> > means it is no longer available for a DT consumer and the SoC details
> > (which clocks is located where, for instance), have to be replicated to
> > other DT users (U-Boot, *BSD, you-name-it). We already came across this
> > issue when looking at converting U-Boot over to use DT clocks.
> > Also it ultimately requires kernel changes for each new SoC, even if it
> > only differs in some detail which could be perfectly modelled in DT
> > (think of H3 vs. H5).
> 
>  The last point is very interesting, before adding a new Allwinner SoC
> was just a matter of maybe handling one/two new clocks (at least to
> have something that 'just boots'), now it's a whole new big boring file
> to write while reading datasheet.

You can definitely do that with sunxi-ng bindings if you want. You
just have to populate only the IDs that are of interest to you.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH v4 5/6] ARM: sun7i: Convert to CCU

[Maxime Ripard]

On Thu, Jun 29, 2017 at 11:57:05AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 25/06/17 21:45, Priit Laes wrote:
> > Convert sun7i-a20.dtsi to new CCU driver.
> 
> I know that some people hat^Wget annoyed by me asking this, but anyway:
> 
> Why do we actually need this?
> 
> This ultimately makes the DT incompatible with older kernels (as
> actually shipped by distros today).
>
> So if we for instance use UEFI boot or otherwise just use "one golden
> DT" to drive all kernels (like using the DT from U-Boot), we now don't
> have one good DT that fits all.

What is broken exactly?

> This is really a showstopper for boards which ship a DT in firmware
> (in SPI flash, for instance, or on some eMMC).
>
> So:
> - Do we actually need to change the .dtsi? The old .dtsi should still work.

It does.

> - Is there anything that the new and fancy clocks gives us over the
> existing clocks? If yes, that should be a  stated in the commit message
> or cover letter.

Yes, support for all the clocks used in the SoC, and not a single
fraction of them (which will reduce the number of additions of new
bindings and drivers, which will in turn make the DT have less
changes, which will make it far more easier for distros and / or
firmwares to ship an immutable DT).

> - Why do we change the clocks for those older SoCs in the first place?
> Can't we just keep on using what worked for years?

Please tell me where the displays clocks, CSI or HDMI clocks are in
the old code.

> I think we really can't remove the old code anyway.

We don't.

> The new clock driver moves information from the DT into the kernel. That
> means it is no longer available for a DT consumer and the SoC details
> (which clocks is located where, for instance), have to be replicated to
> other DT users (U-Boot, *BSD, you-name-it). We already came across this
> issue when looking at converting U-Boot over to use DT clocks.
> Also it ultimately requires kernel changes for each new SoC, even if it
> only differs in some detail which could be perfectly modelled in DT
> (think of H3 vs. H5).

Doing otherwise would also assume that you have a perfect
understanding of all the clocks interactions, relationship and
computations from day one, which is something the old code proved that
it was unreasonable.

The new binding also makes it easier to add SCPI that you're
interested in iirc, where you basically just have to change the CCU
compatible, and be done with it as long as you use the same IDs.

It also lowers the barrier of entry for people that would want to
write new drivers, since the first thing you'd need to do otherwise
would be to create a clock driver for that, which is yet another thing
to learn.

The reduced duplication is also neat and reduces the number of similar
bugs in each and every clock, even though it's not related to clocks.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups 
"linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.


signature.asc
Description: PGP signature

Re: [linux-sunxi] [PATCH v4 5/6] ARM: sun7i: Convert to CCU

[Andre Przywara]

Hi,

On 25/06/17 21:45, Priit Laes wrote:
> Convert sun7i-a20.dtsi to new CCU driver.

I know that some people hat^Wget annoyed by me asking this, but anyway:

Why do we actually need this?

This ultimately makes the DT incompatible with older kernels (as
actually shipped by distros today).
So if we for instance use UEFI boot or otherwise just use "one golden
DT" to drive all kernels (like using the DT from U-Boot), we now don't
have one good DT that fits all. This is really a showstopper for boards
which ship a DT in firmware (in SPI flash, for instance, or on some eMMC).
So:
- Do we actually need to change the .dtsi? The old .dtsi should still work.
- Is there anything that the new and fancy clocks gives us over the
existing clocks? If yes, that should be a  stated in the commit message
or cover letter.
- Why do we change the clocks for those older SoCs in the first place?
Can't we just keep on using what worked for years? I think we really
can't remove the old code anyway.

The new clock driver moves information from the DT into the kernel. That
means it is no longer available for a DT consumer and the SoC details
(which clocks is located where, for instance), have to be replicated to
other DT users (U-Boot, *BSD, you-name-it). We already came across this
issue when looking at converting U-Boot over to use DT clocks.
Also it ultimately requires kernel changes for each new SoC, even if it
only differs in some detail which could be perfectly modelled in DT
(think of H3 vs. H5).

Cheers,
Andre.

> Signed-off-by: Priit Laes 
> ---
>  arch/arm/boot/dts/sun7i-a20.dtsi | 719 +++-
>  1 file changed, 84 insertions(+), 635 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi 
> b/arch/arm/boot/dts/sun7i-a20.dtsi
> index 96bee77..a5ca5a8 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -46,8 +46,6 @@
>  
>  #include 
>  #include 
> -
> -#include 
>  #include 
>  
>  / {
> @@ -66,9 +64,10 @@
>   compatible = "allwinner,simple-framebuffer",
>"simple-framebuffer";
>   allwinner,pipeline = "de_be0-lcd0-hdmi";
> - clocks = <_gates 36>, <_gates 43>,
> -  <_gates 44>, <_be0_clk>,
> -  <_ch1_clk>, <_gates 26>;
> + clocks = < 56>, < 60>,
> +  < 62>, < 144>,
> +  < 155>, < 140>,
> +  < 164>;
>   status = "disabled";
>   };
>  
> @@ -76,9 +75,9 @@
>   compatible = "allwinner,simple-framebuffer",
>"simple-framebuffer";
>   allwinner,pipeline = "de_be0-lcd0";
> - clocks = <_gates 36>, <_gates 44>,
> -  <_be0_clk>, <_ch0_clk>,
> -  <_gates 26>;
> + clocks = < 56>, < 62>,
> +  < 144>, < 149>,
> +  < 140>;
>   status = "disabled";
>   };
>  
> @@ -86,10 +85,10 @@
>   compatible = "allwinner,simple-framebuffer",
>"simple-framebuffer";
>   allwinner,pipeline = "de_be0-lcd0-tve0";
> - clocks = <_gates 34>, <_gates 36>,
> -  <_gates 44>,
> -  <_be0_clk>, <_ch1_clk>,
> -  <_gates 5>, <_gates 26>;
> + clocks = < 54>, < 56>,
> +  < 62>,
> +  < 144>, < 155>,
> +  < 135>, < 140>;
>   status = "disabled";
>   };
>   };
> @@ -102,7 +101,7 @@
>   compatible = "arm,cortex-a7";
>   device_type = "cpu";
>   reg = <0>;
> - clocks = <>;
> + clocks = < 20>;
>   clock-latency = <244144>; /* 8 32k periods */
>   operating-points = <
>   /* kHzuV */
> @@ -183,21 +182,11 @@
>  
>   osc24M: clk@01c20050 {
>   #clock-cells = <0>;
> - compatible = "allwinner,sun4i-a10-osc-clk";
> - reg = <0x01c20050 0x4>;
> + compatible = "fixed-clock";
>   clock-frequency = <2400>;
>   clock-output-names = "osc24M";
>   };
>  
> - osc3M: osc3M_clk {
> - #clock-cells = <0>;
> - compatible = "fixed-factor-clock";
> - clock-div = <8>;
> - clock-mult = <1>;
> - clocks = <>;
> - 

[linux-sunxi] [PATCH v4 5/6] ARM: sun7i: Convert to CCU

[Priit Laes]

Convert sun7i-a20.dtsi to new CCU driver.

Tested on Cubietruck.

Signed-off-by: Priit Laes 
---
 arch/arm/boot/dts/sun7i-a20.dtsi | 719 +++-
 1 file changed, 84 insertions(+), 635 deletions(-)

diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
index 96bee77..a5ca5a8 100644
--- a/arch/arm/boot/dts/sun7i-a20.dtsi
+++ b/arch/arm/boot/dts/sun7i-a20.dtsi
@@ -46,8 +46,6 @@
 
 #include 
 #include 
-
-#include 
 #include 
 
 / {
@@ -66,9 +64,10 @@
compatible = "allwinner,simple-framebuffer",
 "simple-framebuffer";
allwinner,pipeline = "de_be0-lcd0-hdmi";
-   clocks = <_gates 36>, <_gates 43>,
-<_gates 44>, <_be0_clk>,
-<_ch1_clk>, <_gates 26>;
+   clocks = < 56>, < 60>,
+< 62>, < 144>,
+< 155>, < 140>,
+< 164>;
status = "disabled";
};
 
@@ -76,9 +75,9 @@
compatible = "allwinner,simple-framebuffer",
 "simple-framebuffer";
allwinner,pipeline = "de_be0-lcd0";
-   clocks = <_gates 36>, <_gates 44>,
-<_be0_clk>, <_ch0_clk>,
-<_gates 26>;
+   clocks = < 56>, < 62>,
+< 144>, < 149>,
+< 140>;
status = "disabled";
};
 
@@ -86,10 +85,10 @@
compatible = "allwinner,simple-framebuffer",
 "simple-framebuffer";
allwinner,pipeline = "de_be0-lcd0-tve0";
-   clocks = <_gates 34>, <_gates 36>,
-<_gates 44>,
-<_be0_clk>, <_ch1_clk>,
-<_gates 5>, <_gates 26>;
+   clocks = < 54>, < 56>,
+< 62>,
+< 144>, < 155>,
+< 135>, < 140>;
status = "disabled";
};
};
@@ -102,7 +101,7 @@
compatible = "arm,cortex-a7";
device_type = "cpu";
reg = <0>;
-   clocks = <>;
+   clocks = < 20>;
clock-latency = <244144>; /* 8 32k periods */
operating-points = <
/* kHzuV */
@@ -183,21 +182,11 @@
 
osc24M: clk@01c20050 {
#clock-cells = <0>;
-   compatible = "allwinner,sun4i-a10-osc-clk";
-   reg = <0x01c20050 0x4>;
+   compatible = "fixed-clock";
clock-frequency = <2400>;
clock-output-names = "osc24M";
};
 
-   osc3M: osc3M_clk {
-   #clock-cells = <0>;
-   compatible = "fixed-factor-clock";
-   clock-div = <8>;
-   clock-mult = <1>;
-   clocks = <>;
-   clock-output-names = "osc3M";
-   };
-
osc32k: clk@0 {
#clock-cells = <0>;
compatible = "fixed-clock";
@@ -205,528 +194,6 @@
clock-output-names = "osc32k";
};
 
-   pll1: clk@01c2 {
-   #clock-cells = <0>;
-   compatible = "allwinner,sun4i-a10-pll1-clk";
-   reg = <0x01c2 0x4>;
-   clocks = <>;
-   clock-output-names = "pll1";
-   };
-
-   pll2: clk@01c20008 {
-   #clock-cells = <1>;
-   compatible = "allwinner,sun4i-a10-pll2-clk";
-   reg = <0x01c20008 0x8>;
-   clocks = <>;
-   clock-output-names = "pll2-1x", "pll2-2x",
-"pll2-4x", "pll2-8x";
-   };
-
-   pll3: clk@01c20010 {
-   #clock-cells = <0>;
-   compatible = "allwinner,sun4i-a10-pll3-clk";
-   reg = <0x01c20010 0x4>;
-   clocks = <>;
-   clock-output-names = "pll3";
-   };
-
-   pll3x2: pll3x2_clk {
-   #clock-cells = <0>;
-   compatible = "fixed-factor-clock";
-   clocks = <>;
-   clock-div = <1>;
-