* Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused [not found] <1454358000-13594-1-git-send-email-maxime.ripard@free-electrons.com> @ 2016-02-05 17:59 ` Andre Przywara 2016-02-10 12:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Andre Przywara 2016-02-10 12:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Maxime Ripard 0 siblings, 2 replies; 23+ messages in thread From: Andre Przywara @ 2016-02-05 17:59 UTC (permalink / raw) To: Maxime Ripard, Chen-Yu Tsai Cc: Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, Jens Kuske, linux-clk, linux-arm-kernel, Mark Rutland, Rob Herring, Frank Rowand, Grant Likely, devicetree Hi Maxime, just found this while looking at your current git branch, so sorry for the late reply. CC:ing DT people, since you touch both existing DTs(!) and the binding doc. On 01/02/16 20:20, Maxime Ripard wrote: > Remove the fixed dividers from the PLL6 driver to be able to have a > reusable driver that can be used across several SoCs that share the same > controller, but don't have the same set of dividers for this clock, and to > also be reused multiple times in the same SoC, since we're droping the > clock name. > > Acked-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > --- > > Changes from v3: > - Fixed the documentation > - Added pll6d2 back > > Changes from v2: > - Rebased and converted over to the new factors refactoring. Fixed the > retrieved rate > > Documentation/devicetree/bindings/clock/sunxi.txt | 2 +- > arch/arm/boot/dts/sun6i-a31.dtsi | 36 +++++++++++----------- > arch/arm/boot/dts/sun8i-a23-a33.dtsi | 25 ++++++++++----- > arch/arm/boot/dts/sun8i-a23.dtsi | 2 +- > arch/arm/boot/dts/sun8i-a33.dtsi | 4 +-- > arch/arm/boot/dts/sun8i-h3.dtsi | 37 ++++++++++++++--------- So are you really breaking all those systems by changing the DT and the driver in an incompatible way? Please correct me if this assessment is wrong, but to me it looks like any user out there is either stuck with 4.5 at best _or_ will only be able to run 4.6 and later (depending on which version of the DT she is using)? And no, switching DTs along with the kernel is _not_ an option. That is not how I understand DT. Also this totally ignores any other DT user (U-Boot, FreeBSD, you name it). I actually appreciate this rework, it's more flexible now and looks better, but you really can't do this in a way to breaks compatibility with existing DTs. Jean-Francois came up with another solution for the pll8 clock [1], so could this be considered at least? I think changing the H3 PLL8 clock from dummy to something real is a different story in terms of compatibility (since it never really worked before and this wouldn't change for any old-DT user). Cheers, Andre. [1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405104.html > drivers/clk/sunxi/clk-sunxi.c | 32 ++++++++++---------- > 7 files changed, 78 insertions(+), 60 deletions(-) > > diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt > index e59f57b24777..f82850c0f6a5 100644 > --- a/Documentation/devicetree/bindings/clock/sunxi.txt > +++ b/Documentation/devicetree/bindings/clock/sunxi.txt > @@ -86,7 +86,7 @@ Required properties for all clocks: > - #clock-cells : from common clock binding; shall be set to 0 except for > the following compatibles where it shall be set to 1: > "allwinner,*-gates-clk", "allwinner,sun4i-pll5-clk", > - "allwinner,sun4i-pll6-clk", "allwinner,sun6i-a31-pll6-clk", > + "allwinner,sun4i-pll6-clk", > "allwinner,*-usb-clk", "allwinner,*-mmc-clk", > "allwinner,*-mmc-config-clk" > - clock-output-names : shall be the corresponding names of the outputs. > diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi > index b6ad7850fac6..05fe3d1aa328 100644 > --- a/arch/arm/boot/dts/sun6i-a31.dtsi > +++ b/arch/arm/boot/dts/sun6i-a31.dtsi > @@ -65,7 +65,7 @@ > compatible = "allwinner,simple-framebuffer", > "simple-framebuffer"; > allwinner,pipeline = "de_be0-lcd0-hdmi"; > - clocks = <&pll6 0>; > + clocks = <&pll6>; > status = "disabled"; > }; > > @@ -73,7 +73,7 @@ > compatible = "allwinner,simple-framebuffer", > "simple-framebuffer"; > allwinner,pipeline = "de_be0-lcd0"; > - clocks = <&pll6 0>; > + clocks = <&pll6>; > status = "disabled"; > }; > }; > @@ -201,11 +201,11 @@ > }; > > pll6: clk@01c20028 { > - #clock-cells = <1>; > + #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-pll6-clk"; > reg = <0x01c20028 0x4>; > clocks = <&osc24M>; > - clock-output-names = "pll6", "pll6x2"; > + clock-output-names = "pll6"; > }; > > cpu: cpu@01c20050 { > @@ -235,7 +235,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-ahb1-clk"; > reg = <0x01c20054 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; > clock-output-names = "ahb1"; > > /* > @@ -244,7 +244,7 @@ > * controller requires AHB1 clocked from PLL6. > */ > assigned-clocks = <&ahb1>; > - assigned-clock-parents = <&pll6 0>; > + assigned-clock-parents = <&pll6>; > }; > > ahb1_gates: clk@01c20060 { > @@ -307,7 +307,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-apb1-clk"; > reg = <0x01c20058 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; > clock-output-names = "apb2"; > }; > > @@ -331,7 +331,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20088 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc0", > "mmc0_output", > "mmc0_sample"; > @@ -341,7 +341,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c2008c 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc1", > "mmc1_output", > "mmc1_sample"; > @@ -351,7 +351,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20090 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc2", > "mmc2_output", > "mmc2_sample"; > @@ -361,7 +361,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20094 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc3", > "mmc3_output", > "mmc3_sample"; > @@ -371,7 +371,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c2009c 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "ss"; > }; > > @@ -379,7 +379,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c200a0 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "spi0"; > }; > > @@ -387,7 +387,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c200a4 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "spi1"; > }; > > @@ -395,7 +395,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c200a8 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "spi2"; > }; > > @@ -403,7 +403,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c200ac 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "spi3"; > }; > > @@ -1042,8 +1042,8 @@ > ar100: ar100_clk { > compatible = "allwinner,sun6i-a31-ar100-clk"; > #clock-cells = <0>; > - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, > - <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&pll6>, > + <&pll6>; > clock-output-names = "ar100"; > }; > > diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi > index 6f88fb0ddbc7..edc85eeaf365 100644 > --- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi > +++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi > @@ -60,7 +60,7 @@ > compatible = "allwinner,simple-framebuffer", > "simple-framebuffer"; > allwinner,pipeline = "de_be0-lcd0"; > - clocks = <&pll6 0>; > + clocks = <&pll6>; > status = "disabled"; > }; > }; > @@ -129,11 +129,20 @@ > }; > > pll6: clk@01c20028 { > - #clock-cells = <1>; > + #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-pll6-clk"; > reg = <0x01c20028 0x4>; > clocks = <&osc24M>; > - clock-output-names = "pll6", "pll6x2"; > + clock-output-names = "pll6"; > + }; > + > + pll6x2: pll6x2_clk { > + compatible = "fixed-factor-clock"; > + #clock-cells = <0>; > + clock-div = <1>; > + clock-mult = <2>; > + clocks = <&pll6>; > + clock-output-names = "pll6-2x"; > }; > > cpu: cpu_clk@01c20050 { > @@ -163,7 +172,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-ahb1-clk"; > reg = <0x01c20054 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; > clock-output-names = "ahb1"; > }; > > @@ -190,7 +199,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-apb1-clk"; > reg = <0x01c20058 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; > clock-output-names = "apb2"; > }; > > @@ -213,7 +222,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20088 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc0", > "mmc0_output", > "mmc0_sample"; > @@ -223,7 +232,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c2008c 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc1", > "mmc1_output", > "mmc1_sample"; > @@ -233,7 +242,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20090 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "mmc2", > "mmc2_output", > "mmc2_sample"; > diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi > index 92e6616979ea..5e589c1ddda9 100644 > --- a/arch/arm/boot/dts/sun8i-a23.dtsi > +++ b/arch/arm/boot/dts/sun8i-a23.dtsi > @@ -79,7 +79,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun8i-a23-mbus-clk"; > reg = <0x01c2015c 0x4>; > - clocks = <&osc24M>, <&pll6 1>, <&pll5>; > + clocks = <&osc24M>, <&pll6x2>, <&pll5>; > clock-output-names = "mbus"; > }; > }; > diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi > index 001d8402ca18..f3eb618bcfa7 100644 > --- a/arch/arm/boot/dts/sun8i-a33.dtsi > +++ b/arch/arm/boot/dts/sun8i-a33.dtsi > @@ -103,7 +103,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-mod0-clk"; > reg = <0x01c2009c 0x4>; > - clocks = <&osc24M>, <&pll6 0>; > + clocks = <&osc24M>, <&pll6>; > clock-output-names = "ss"; > }; > > @@ -111,7 +111,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun8i-a23-mbus-clk"; > reg = <0x01c2015c 0x4>; > - clocks = <&osc24M>, <&pll6 1>, <&pll5>, <&pll11>; > + clocks = <&osc24M>, <&pll6x2>, <&pll5>, <&pll11>; > clock-output-names = "mbus"; > }; > }; > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi > index 1524130e43c9..6f6b4e469ac9 100644 > --- a/arch/arm/boot/dts/sun8i-h3.dtsi > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi > @@ -121,11 +121,11 @@ > }; > > pll6: clk@01c20028 { > - #clock-cells = <1>; > + #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-pll6-clk"; > reg = <0x01c20028 0x4>; > clocks = <&osc24M>; > - clock-output-names = "pll6", "pll6x2"; > + clock-output-names = "pll6"; > }; > > pll6d2: pll6d2_clk { > @@ -133,15 +133,24 @@ > compatible = "fixed-factor-clock"; > clock-div = <2>; > clock-mult = <1>; > - clocks = <&pll6 0>; > - clock-output-names = "pll6d2"; > + clocks = <&pll6>; > + clock-output-names = "pll6-d2"; > }; > > - /* dummy clock until pll6 can be reused */ > - pll8: pll8_clk { > + pll6x2: pll6x2_clk { > #clock-cells = <0>; > - compatible = "fixed-clock"; > - clock-frequency = <1>; > + compatible = "fixed-factor-clock"; > + clock-div = <1>; > + clock-mult = <2>; > + clocks = <&pll6>; > + clock-output-names = "pll6-2x"; > + }; > + > + pll8: clk@01c20044 { > + #clock-cells = <0>; > + compatible = "allwinner,sun6i-a31-pll6-clk"; > + reg = <0x01c20044 0x4>; > + clocks = <&osc24M>; > clock-output-names = "pll8"; > }; > > @@ -165,7 +174,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun6i-a31-ahb1-clk"; > reg = <0x01c20054 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; > clock-output-names = "ahb1"; > }; > > @@ -189,7 +198,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun4i-a10-apb1-clk"; > reg = <0x01c20058 0x4>; > - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; > + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; > clock-output-names = "apb2"; > }; > > @@ -243,7 +252,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20088 0x4>; > - clocks = <&osc24M>, <&pll6 0>, <&pll8>; > + clocks = <&osc24M>, <&pll6>, <&pll8>; > clock-output-names = "mmc0", > "mmc0_output", > "mmc0_sample"; > @@ -253,7 +262,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c2008c 0x4>; > - clocks = <&osc24M>, <&pll6 0>, <&pll8>; > + clocks = <&osc24M>, <&pll6>, <&pll8>; > clock-output-names = "mmc1", > "mmc1_output", > "mmc1_sample"; > @@ -263,7 +272,7 @@ > #clock-cells = <1>; > compatible = "allwinner,sun4i-a10-mmc-clk"; > reg = <0x01c20090 0x4>; > - clocks = <&osc24M>, <&pll6 0>, <&pll8>; > + clocks = <&osc24M>, <&pll6>, <&pll8>; > clock-output-names = "mmc2", > "mmc2_output", > "mmc2_sample"; > @@ -273,7 +282,7 @@ > #clock-cells = <0>; > compatible = "allwinner,sun8i-a23-mbus-clk"; > reg = <0x01c2015c 0x4>; > - clocks = <&osc24M>, <&pll6 1>, <&pll5>; > + clocks = <&osc24M>, <&pll6x2>, <&pll5>; > clock-output-names = "mbus"; > }; > }; > diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c > index da15f2b12ab2..4de800e379d3 100644 > --- a/drivers/clk/sunxi/clk-sunxi.c > +++ b/drivers/clk/sunxi/clk-sunxi.c > @@ -227,9 +227,9 @@ static void sun4i_get_pll5_factors(struct factors_request *req) > } > > /** > - * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6x2 > - * PLL6x2 rate is calculated as follows > - * rate = parent_rate * (n + 1) * (k + 1) > + * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6 > + * PLL6 rate is calculated as follows > + * rate = parent_rate * (n + 1) * (k + 1) / 2 > * parent_rate is always 24Mhz > */ > > @@ -238,8 +238,8 @@ static void sun6i_a31_get_pll6_factors(struct factors_request *req) > u8 div; > > /* Normalize value to a parent_rate multiple (24M) */ > - div = req->rate / req->parent_rate; > - req->rate = req->parent_rate * div; > + div = req->rate / (req->parent_rate / 2); > + req->rate = (req->parent_rate / 2) * div; > > req->k = div / 32; > if (req->k > 3) > @@ -248,6 +248,15 @@ static void sun6i_a31_get_pll6_factors(struct factors_request *req) > req->n = DIV_ROUND_UP(div, (req->k + 1)) - 1; > } > > +static void sun6i_a31_pll6_recalc(struct factors_request *req) > +{ > + req->rate = req->parent_rate; > + > + req->rate *= req->n + 1; > + req->rate *= req->k + 1; > + req->rate /= 2; > +} > + > /** > * sun5i_a13_get_ahb_factors() - calculates m, p factors for AHB > * AHB rate is calculated as follows > @@ -537,7 +546,7 @@ static const struct factors_data sun6i_a31_pll6_data __initconst = { > .enable = 31, > .table = &sun6i_a31_pll6_config, > .getter = sun6i_a31_get_pll6_factors, > - .name = "pll6x2", > + .recalc = sun6i_a31_pll6_recalc, > }; > > static const struct factors_data sun5i_a13_ahb_data __initconst = { > @@ -788,15 +797,6 @@ static const struct divs_data pll6_divs_data __initconst = { > } > }; > > -static const struct divs_data sun6i_a31_pll6_divs_data __initconst = { > - .factors = &sun6i_a31_pll6_data, > - .ndivs = 2, > - .div = { > - { .fixed = 2 }, /* normal output */ > - { .self = 1 }, /* base factor clock, 2x */ > - } > -}; > - > /** > * sunxi_divs_clk_setup() - Setup function for leaf divisors on clocks > * > @@ -938,6 +938,7 @@ free_clkdata: > static const struct of_device_id clk_factors_match[] __initconst = { > {.compatible = "allwinner,sun4i-a10-pll1-clk", .data = &sun4i_pll1_data,}, > {.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,}, > + {.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_data,}, > {.compatible = "allwinner,sun8i-a23-pll1-clk", .data = &sun8i_a23_pll1_data,}, > {.compatible = "allwinner,sun7i-a20-pll4-clk", .data = &sun7i_a20_pll4_data,}, > {.compatible = "allwinner,sun5i-a13-ahb-clk", .data = &sun5i_a13_ahb_data,}, > @@ -959,7 +960,6 @@ static const struct of_device_id clk_div_match[] __initconst = { > static const struct of_device_id clk_divs_match[] __initconst = { > {.compatible = "allwinner,sun4i-a10-pll5-clk", .data = &pll5_divs_data,}, > {.compatible = "allwinner,sun4i-a10-pll6-clk", .data = &pll6_divs_data,}, > - {.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_divs_data,}, > {} > }; > > ^ permalink raw reply [flat|nested] 23+ messages in thread
* breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-05 17:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Andre Przywara @ 2016-02-10 12:30 ` Andre Przywara 2016-02-10 13:42 ` Rob Herring 2016-02-10 12:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Maxime Ripard 1 sibling, 1 reply; 23+ messages in thread From: Andre Przywara @ 2016-02-10 12:30 UTC (permalink / raw) To: Mark Rutland, Rob Herring, Grant Likely, Frank Rowand Cc: Maxime Ripard, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree, Jens Kuske, linux-clk, linux-arm-kernel Hi, just a ping: Are we really OK with breaking existing DTs in 4.6? (per the code in -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). Also I think one needs ACKs from DT maintainers before merging something in the respective directories, which I don't see here. As I am somewhat blocked on that patch, I'd like to have some discussion on the list. Thanks, Andre. On 05/02/16 17:59, Andre Przywara wrote: > Hi Maxime, > > just found this while looking at your current git branch, so sorry for > the late reply. > > CC:ing DT people, since you touch both existing DTs(!) and the binding doc. > > On 01/02/16 20:20, Maxime Ripard wrote: >> Remove the fixed dividers from the PLL6 driver to be able to have a >> reusable driver that can be used across several SoCs that share the same >> controller, but don't have the same set of dividers for this clock, and to >> also be reused multiple times in the same SoC, since we're droping the >> clock name. >> >> Acked-by: Chen-Yu Tsai <wens@csie.org> >> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >> --- >> >> Changes from v3: >> - Fixed the documentation >> - Added pll6d2 back >> >> Changes from v2: >> - Rebased and converted over to the new factors refactoring. Fixed the >> retrieved rate >> >> Documentation/devicetree/bindings/clock/sunxi.txt | 2 +- >> arch/arm/boot/dts/sun6i-a31.dtsi | 36 +++++++++++----------- >> arch/arm/boot/dts/sun8i-a23-a33.dtsi | 25 ++++++++++----- >> arch/arm/boot/dts/sun8i-a23.dtsi | 2 +- >> arch/arm/boot/dts/sun8i-a33.dtsi | 4 +-- >> arch/arm/boot/dts/sun8i-h3.dtsi | 37 ++++++++++++++--------- > > So are you really breaking all those systems by changing the DT and the > driver in an incompatible way? > Please correct me if this assessment is wrong, but to me it looks like > any user out there is either stuck with 4.5 at best _or_ will only be > able to run 4.6 and later (depending on which version of the DT she is > using)? And no, switching DTs along with the kernel is _not_ an option. > That is not how I understand DT. > Also this totally ignores any other DT user (U-Boot, FreeBSD, you name it). > > I actually appreciate this rework, it's more flexible now and looks > better, but you really can't do this in a way to breaks compatibility > with existing DTs. > > Jean-Francois came up with another solution for the pll8 clock [1], so > could this be considered at least? > I think changing the H3 PLL8 clock from dummy to something real is a > different story in terms of compatibility (since it never really worked > before and this wouldn't change for any old-DT user). > > Cheers, > Andre. > > [1] > http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405104.html > >> drivers/clk/sunxi/clk-sunxi.c | 32 ++++++++++---------- >> 7 files changed, 78 insertions(+), 60 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt >> index e59f57b24777..f82850c0f6a5 100644 >> --- a/Documentation/devicetree/bindings/clock/sunxi.txt >> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt >> @@ -86,7 +86,7 @@ Required properties for all clocks: >> - #clock-cells : from common clock binding; shall be set to 0 except for >> the following compatibles where it shall be set to 1: >> "allwinner,*-gates-clk", "allwinner,sun4i-pll5-clk", >> - "allwinner,sun4i-pll6-clk", "allwinner,sun6i-a31-pll6-clk", >> + "allwinner,sun4i-pll6-clk", >> "allwinner,*-usb-clk", "allwinner,*-mmc-clk", >> "allwinner,*-mmc-config-clk" >> - clock-output-names : shall be the corresponding names of the outputs. >> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi >> index b6ad7850fac6..05fe3d1aa328 100644 >> --- a/arch/arm/boot/dts/sun6i-a31.dtsi >> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi >> @@ -65,7 +65,7 @@ >> compatible = "allwinner,simple-framebuffer", >> "simple-framebuffer"; >> allwinner,pipeline = "de_be0-lcd0-hdmi"; >> - clocks = <&pll6 0>; >> + clocks = <&pll6>; >> status = "disabled"; >> }; >> >> @@ -73,7 +73,7 @@ >> compatible = "allwinner,simple-framebuffer", >> "simple-framebuffer"; >> allwinner,pipeline = "de_be0-lcd0"; >> - clocks = <&pll6 0>; >> + clocks = <&pll6>; >> status = "disabled"; >> }; >> }; >> @@ -201,11 +201,11 @@ >> }; >> >> pll6: clk@01c20028 { >> - #clock-cells = <1>; >> + #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-pll6-clk"; >> reg = <0x01c20028 0x4>; >> clocks = <&osc24M>; >> - clock-output-names = "pll6", "pll6x2"; >> + clock-output-names = "pll6"; >> }; >> >> cpu: cpu@01c20050 { >> @@ -235,7 +235,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-ahb1-clk"; >> reg = <0x01c20054 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; >> clock-output-names = "ahb1"; >> >> /* >> @@ -244,7 +244,7 @@ >> * controller requires AHB1 clocked from PLL6. >> */ >> assigned-clocks = <&ahb1>; >> - assigned-clock-parents = <&pll6 0>; >> + assigned-clock-parents = <&pll6>; >> }; >> >> ahb1_gates: clk@01c20060 { >> @@ -307,7 +307,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-apb1-clk"; >> reg = <0x01c20058 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; >> clock-output-names = "apb2"; >> }; >> >> @@ -331,7 +331,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20088 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc0", >> "mmc0_output", >> "mmc0_sample"; >> @@ -341,7 +341,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c2008c 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc1", >> "mmc1_output", >> "mmc1_sample"; >> @@ -351,7 +351,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20090 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc2", >> "mmc2_output", >> "mmc2_sample"; >> @@ -361,7 +361,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20094 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc3", >> "mmc3_output", >> "mmc3_sample"; >> @@ -371,7 +371,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c2009c 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "ss"; >> }; >> >> @@ -379,7 +379,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c200a0 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "spi0"; >> }; >> >> @@ -387,7 +387,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c200a4 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "spi1"; >> }; >> >> @@ -395,7 +395,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c200a8 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "spi2"; >> }; >> >> @@ -403,7 +403,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c200ac 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "spi3"; >> }; >> >> @@ -1042,8 +1042,8 @@ >> ar100: ar100_clk { >> compatible = "allwinner,sun6i-a31-ar100-clk"; >> #clock-cells = <0>; >> - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, >> - <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&pll6>, >> + <&pll6>; >> clock-output-names = "ar100"; >> }; >> >> diff --git a/arch/arm/boot/dts/sun8i-a23-a33.dtsi b/arch/arm/boot/dts/sun8i-a23-a33.dtsi >> index 6f88fb0ddbc7..edc85eeaf365 100644 >> --- a/arch/arm/boot/dts/sun8i-a23-a33.dtsi >> +++ b/arch/arm/boot/dts/sun8i-a23-a33.dtsi >> @@ -60,7 +60,7 @@ >> compatible = "allwinner,simple-framebuffer", >> "simple-framebuffer"; >> allwinner,pipeline = "de_be0-lcd0"; >> - clocks = <&pll6 0>; >> + clocks = <&pll6>; >> status = "disabled"; >> }; >> }; >> @@ -129,11 +129,20 @@ >> }; >> >> pll6: clk@01c20028 { >> - #clock-cells = <1>; >> + #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-pll6-clk"; >> reg = <0x01c20028 0x4>; >> clocks = <&osc24M>; >> - clock-output-names = "pll6", "pll6x2"; >> + clock-output-names = "pll6"; >> + }; >> + >> + pll6x2: pll6x2_clk { >> + compatible = "fixed-factor-clock"; >> + #clock-cells = <0>; >> + clock-div = <1>; >> + clock-mult = <2>; >> + clocks = <&pll6>; >> + clock-output-names = "pll6-2x"; >> }; >> >> cpu: cpu_clk@01c20050 { >> @@ -163,7 +172,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-ahb1-clk"; >> reg = <0x01c20054 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; >> clock-output-names = "ahb1"; >> }; >> >> @@ -190,7 +199,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-apb1-clk"; >> reg = <0x01c20058 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; >> clock-output-names = "apb2"; >> }; >> >> @@ -213,7 +222,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20088 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc0", >> "mmc0_output", >> "mmc0_sample"; >> @@ -223,7 +232,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c2008c 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc1", >> "mmc1_output", >> "mmc1_sample"; >> @@ -233,7 +242,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20090 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "mmc2", >> "mmc2_output", >> "mmc2_sample"; >> diff --git a/arch/arm/boot/dts/sun8i-a23.dtsi b/arch/arm/boot/dts/sun8i-a23.dtsi >> index 92e6616979ea..5e589c1ddda9 100644 >> --- a/arch/arm/boot/dts/sun8i-a23.dtsi >> +++ b/arch/arm/boot/dts/sun8i-a23.dtsi >> @@ -79,7 +79,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun8i-a23-mbus-clk"; >> reg = <0x01c2015c 0x4>; >> - clocks = <&osc24M>, <&pll6 1>, <&pll5>; >> + clocks = <&osc24M>, <&pll6x2>, <&pll5>; >> clock-output-names = "mbus"; >> }; >> }; >> diff --git a/arch/arm/boot/dts/sun8i-a33.dtsi b/arch/arm/boot/dts/sun8i-a33.dtsi >> index 001d8402ca18..f3eb618bcfa7 100644 >> --- a/arch/arm/boot/dts/sun8i-a33.dtsi >> +++ b/arch/arm/boot/dts/sun8i-a33.dtsi >> @@ -103,7 +103,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-mod0-clk"; >> reg = <0x01c2009c 0x4>; >> - clocks = <&osc24M>, <&pll6 0>; >> + clocks = <&osc24M>, <&pll6>; >> clock-output-names = "ss"; >> }; >> >> @@ -111,7 +111,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun8i-a23-mbus-clk"; >> reg = <0x01c2015c 0x4>; >> - clocks = <&osc24M>, <&pll6 1>, <&pll5>, <&pll11>; >> + clocks = <&osc24M>, <&pll6x2>, <&pll5>, <&pll11>; >> clock-output-names = "mbus"; >> }; >> }; >> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi >> index 1524130e43c9..6f6b4e469ac9 100644 >> --- a/arch/arm/boot/dts/sun8i-h3.dtsi >> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi >> @@ -121,11 +121,11 @@ >> }; >> >> pll6: clk@01c20028 { >> - #clock-cells = <1>; >> + #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-pll6-clk"; >> reg = <0x01c20028 0x4>; >> clocks = <&osc24M>; >> - clock-output-names = "pll6", "pll6x2"; >> + clock-output-names = "pll6"; >> }; >> >> pll6d2: pll6d2_clk { >> @@ -133,15 +133,24 @@ >> compatible = "fixed-factor-clock"; >> clock-div = <2>; >> clock-mult = <1>; >> - clocks = <&pll6 0>; >> - clock-output-names = "pll6d2"; >> + clocks = <&pll6>; >> + clock-output-names = "pll6-d2"; >> }; >> >> - /* dummy clock until pll6 can be reused */ >> - pll8: pll8_clk { >> + pll6x2: pll6x2_clk { >> #clock-cells = <0>; >> - compatible = "fixed-clock"; >> - clock-frequency = <1>; >> + compatible = "fixed-factor-clock"; >> + clock-div = <1>; >> + clock-mult = <2>; >> + clocks = <&pll6>; >> + clock-output-names = "pll6-2x"; >> + }; >> + >> + pll8: clk@01c20044 { >> + #clock-cells = <0>; >> + compatible = "allwinner,sun6i-a31-pll6-clk"; >> + reg = <0x01c20044 0x4>; >> + clocks = <&osc24M>; >> clock-output-names = "pll8"; >> }; >> >> @@ -165,7 +174,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun6i-a31-ahb1-clk"; >> reg = <0x01c20054 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>; >> clock-output-names = "ahb1"; >> }; >> >> @@ -189,7 +198,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun4i-a10-apb1-clk"; >> reg = <0x01c20058 0x4>; >> - clocks = <&osc32k>, <&osc24M>, <&pll6 0>, <&pll6 0>; >> + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>; >> clock-output-names = "apb2"; >> }; >> >> @@ -243,7 +252,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20088 0x4>; >> - clocks = <&osc24M>, <&pll6 0>, <&pll8>; >> + clocks = <&osc24M>, <&pll6>, <&pll8>; >> clock-output-names = "mmc0", >> "mmc0_output", >> "mmc0_sample"; >> @@ -253,7 +262,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c2008c 0x4>; >> - clocks = <&osc24M>, <&pll6 0>, <&pll8>; >> + clocks = <&osc24M>, <&pll6>, <&pll8>; >> clock-output-names = "mmc1", >> "mmc1_output", >> "mmc1_sample"; >> @@ -263,7 +272,7 @@ >> #clock-cells = <1>; >> compatible = "allwinner,sun4i-a10-mmc-clk"; >> reg = <0x01c20090 0x4>; >> - clocks = <&osc24M>, <&pll6 0>, <&pll8>; >> + clocks = <&osc24M>, <&pll6>, <&pll8>; >> clock-output-names = "mmc2", >> "mmc2_output", >> "mmc2_sample"; >> @@ -273,7 +282,7 @@ >> #clock-cells = <0>; >> compatible = "allwinner,sun8i-a23-mbus-clk"; >> reg = <0x01c2015c 0x4>; >> - clocks = <&osc24M>, <&pll6 1>, <&pll5>; >> + clocks = <&osc24M>, <&pll6x2>, <&pll5>; >> clock-output-names = "mbus"; >> }; >> }; >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index da15f2b12ab2..4de800e379d3 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -227,9 +227,9 @@ static void sun4i_get_pll5_factors(struct factors_request *req) >> } >> >> /** >> - * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6x2 >> - * PLL6x2 rate is calculated as follows >> - * rate = parent_rate * (n + 1) * (k + 1) >> + * sun6i_a31_get_pll6_factors() - calculates n, k factors for A31 PLL6 >> + * PLL6 rate is calculated as follows >> + * rate = parent_rate * (n + 1) * (k + 1) / 2 >> * parent_rate is always 24Mhz >> */ >> >> @@ -238,8 +238,8 @@ static void sun6i_a31_get_pll6_factors(struct factors_request *req) >> u8 div; >> >> /* Normalize value to a parent_rate multiple (24M) */ >> - div = req->rate / req->parent_rate; >> - req->rate = req->parent_rate * div; >> + div = req->rate / (req->parent_rate / 2); >> + req->rate = (req->parent_rate / 2) * div; >> >> req->k = div / 32; >> if (req->k > 3) >> @@ -248,6 +248,15 @@ static void sun6i_a31_get_pll6_factors(struct factors_request *req) >> req->n = DIV_ROUND_UP(div, (req->k + 1)) - 1; >> } >> >> +static void sun6i_a31_pll6_recalc(struct factors_request *req) >> +{ >> + req->rate = req->parent_rate; >> + >> + req->rate *= req->n + 1; >> + req->rate *= req->k + 1; >> + req->rate /= 2; >> +} >> + >> /** >> * sun5i_a13_get_ahb_factors() - calculates m, p factors for AHB >> * AHB rate is calculated as follows >> @@ -537,7 +546,7 @@ static const struct factors_data sun6i_a31_pll6_data __initconst = { >> .enable = 31, >> .table = &sun6i_a31_pll6_config, >> .getter = sun6i_a31_get_pll6_factors, >> - .name = "pll6x2", >> + .recalc = sun6i_a31_pll6_recalc, >> }; >> >> static const struct factors_data sun5i_a13_ahb_data __initconst = { >> @@ -788,15 +797,6 @@ static const struct divs_data pll6_divs_data __initconst = { >> } >> }; >> >> -static const struct divs_data sun6i_a31_pll6_divs_data __initconst = { >> - .factors = &sun6i_a31_pll6_data, >> - .ndivs = 2, >> - .div = { >> - { .fixed = 2 }, /* normal output */ >> - { .self = 1 }, /* base factor clock, 2x */ >> - } >> -}; >> - >> /** >> * sunxi_divs_clk_setup() - Setup function for leaf divisors on clocks >> * >> @@ -938,6 +938,7 @@ free_clkdata: >> static const struct of_device_id clk_factors_match[] __initconst = { >> {.compatible = "allwinner,sun4i-a10-pll1-clk", .data = &sun4i_pll1_data,}, >> {.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,}, >> + {.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_data,}, >> {.compatible = "allwinner,sun8i-a23-pll1-clk", .data = &sun8i_a23_pll1_data,}, >> {.compatible = "allwinner,sun7i-a20-pll4-clk", .data = &sun7i_a20_pll4_data,}, >> {.compatible = "allwinner,sun5i-a13-ahb-clk", .data = &sun5i_a13_ahb_data,}, >> @@ -959,7 +960,6 @@ static const struct of_device_id clk_div_match[] __initconst = { >> static const struct of_device_id clk_divs_match[] __initconst = { >> {.compatible = "allwinner,sun4i-a10-pll5-clk", .data = &pll5_divs_data,}, >> {.compatible = "allwinner,sun4i-a10-pll6-clk", .data = &pll6_divs_data,}, >> - {.compatible = "allwinner,sun6i-a31-pll6-clk", .data = &sun6i_a31_pll6_divs_data,}, >> {} >> }; >> >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-10 12:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Andre Przywara @ 2016-02-10 13:42 ` Rob Herring 2016-02-10 14:37 ` Maxime Ripard 0 siblings, 1 reply; 23+ messages in thread From: Rob Herring @ 2016-02-10 13:42 UTC (permalink / raw) To: Andre Przywara Cc: Mark Rutland, Grant Likely, Frank Rowand, Maxime Ripard, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > Hi, > > just a ping: > > Are we really OK with breaking existing DTs in 4.6? (per the code in > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). I only warn and make sure people are aware of the issue. I leave that up to platform maintainers to decide. It depends on the maturity of the platform and users. If people complain about it then it's their mess. For platforms supported in distros such as debian or fedora, I would strongly recommend against breaking compatibility. They do ship dtbs, but it's a chicken and egg problem. If dtbs were stable and provided by firmware, then they wouldn't have to provide them. If dtbs are unstable, then they have no choice. > Also I think one needs ACKs from DT maintainers before merging something > in the respective directories, which I don't see here. It can go in with subsystem maintainers ack, but there are problems with this one regardless of compatibility. > As I am somewhat blocked on that patch, I'd like to have some discussion > on the list. > > Thanks, > Andre. > > On 05/02/16 17:59, Andre Przywara wrote: >> Hi Maxime, >> >> just found this while looking at your current git branch, so sorry for >> the late reply. >> >> CC:ing DT people, since you touch both existing DTs(!) and the binding doc. >> >> On 01/02/16 20:20, Maxime Ripard wrote: >>> Remove the fixed dividers from the PLL6 driver to be able to have a >>> reusable driver that can be used across several SoCs that share the same >>> controller, but don't have the same set of dividers for this clock, and to >>> also be reused multiple times in the same SoC, since we're droping the >>> clock name. Removing a compatible name or not has nothing to do with sharing code. >>> >>> Acked-by: Chen-Yu Tsai <wens@csie.org> >>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> >>> --- >>> >>> Changes from v3: >>> - Fixed the documentation >>> - Added pll6d2 back >>> >>> Changes from v2: >>> - Rebased and converted over to the new factors refactoring. Fixed the >>> retrieved rate >>> >>> Documentation/devicetree/bindings/clock/sunxi.txt | 2 +- >>> arch/arm/boot/dts/sun6i-a31.dtsi | 36 +++++++++++----------- >>> arch/arm/boot/dts/sun8i-a23-a33.dtsi | 25 ++++++++++----- >>> arch/arm/boot/dts/sun8i-a23.dtsi | 2 +- >>> arch/arm/boot/dts/sun8i-a33.dtsi | 4 +-- >>> arch/arm/boot/dts/sun8i-h3.dtsi | 37 ++++++++++++++--------- >> >> So are you really breaking all those systems by changing the DT and the >> driver in an incompatible way? >> Please correct me if this assessment is wrong, but to me it looks like >> any user out there is either stuck with 4.5 at best _or_ will only be >> able to run 4.6 and later (depending on which version of the DT she is >> using)? And no, switching DTs along with the kernel is _not_ an option. >> That is not how I understand DT. >> Also this totally ignores any other DT user (U-Boot, FreeBSD, you name it). >> >> I actually appreciate this rework, it's more flexible now and looks >> better, but you really can't do this in a way to breaks compatibility >> with existing DTs. >> >> Jean-Francois came up with another solution for the pll8 clock [1], so >> could this be considered at least? >> I think changing the H3 PLL8 clock from dummy to something real is a >> different story in terms of compatibility (since it never really worked >> before and this wouldn't change for any old-DT user). >> >> Cheers, >> Andre. >> >> [1] >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405104.html >> >>> drivers/clk/sunxi/clk-sunxi.c | 32 ++++++++++---------- >>> 7 files changed, 78 insertions(+), 60 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt >>> index e59f57b24777..f82850c0f6a5 100644 >>> --- a/Documentation/devicetree/bindings/clock/sunxi.txt >>> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt >>> @@ -86,7 +86,7 @@ Required properties for all clocks: >>> - #clock-cells : from common clock binding; shall be set to 0 except for >>> the following compatibles where it shall be set to 1: >>> "allwinner,*-gates-clk", "allwinner,sun4i-pll5-clk", >>> - "allwinner,sun4i-pll6-clk", "allwinner,sun6i-a31-pll6-clk", >>> + "allwinner,sun4i-pll6-clk", >>> "allwinner,*-usb-clk", "allwinner,*-mmc-clk", >>> "allwinner,*-mmc-config-clk" >>> - clock-output-names : shall be the corresponding names of the outputs. >>> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi >>> index b6ad7850fac6..05fe3d1aa328 100644 >>> --- a/arch/arm/boot/dts/sun6i-a31.dtsi >>> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi >>> @@ -65,7 +65,7 @@ >>> compatible = "allwinner,simple-framebuffer", >>> "simple-framebuffer"; >>> allwinner,pipeline = "de_be0-lcd0-hdmi"; >>> - clocks = <&pll6 0>; >>> + clocks = <&pll6>; >>> status = "disabled"; >>> }; >>> >>> @@ -73,7 +73,7 @@ >>> compatible = "allwinner,simple-framebuffer", >>> "simple-framebuffer"; >>> allwinner,pipeline = "de_be0-lcd0"; >>> - clocks = <&pll6 0>; >>> + clocks = <&pll6>; >>> status = "disabled"; >>> }; >>> }; >>> @@ -201,11 +201,11 @@ >>> }; >>> >>> pll6: clk@01c20028 { >>> - #clock-cells = <1>; >>> + #clock-cells = <0>; >>> compatible = "allwinner,sun6i-a31-pll6-clk"; The name is removed, but remains here? ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-10 13:42 ` Rob Herring @ 2016-02-10 14:37 ` Maxime Ripard 2016-02-10 14:45 ` Arnd Bergmann ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Maxime Ripard @ 2016-02-10 14:37 UTC (permalink / raw) To: Rob Herring Cc: Andre Przywara, Mark Rutland, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 3727 bytes --] Hi Rob, On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > Hi, > > > > just a ping: > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > I only warn and make sure people are aware of the issue. I leave that > up to platform maintainers to decide. It depends on the maturity of > the platform and users. The impacted SoCs support is really partial. For the most supported one, big things like display or sound are totally missing, and we still update them on a regular basis to add support for new devices. As such, users are very likely to upgrade the DT from one version to another just because there's new devices available to them. And the newest SoC impacted just got introduced in 4.5, and only has the UART and MMC devices available. > If people complain about it then it's their mess. For platforms > supported in distros such as debian or fedora, I would strongly > recommend against breaking compatibility. None of them are officially supported: https://www.debian.org/releases/stable/armhf/ch02s01.html.en https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 Only the older one are, and they are not affected by this patch. > They do ship dtbs, but it's a chicken and egg problem. If dtbs were > stable and provided by firmware, then they wouldn't have to provide > them. If dtbs are unstable, then they have no choice. And while it might work great on platforms that have all the needed documentation, or a perfect one, which is our case. Almost each release, we discover that something is not working as it was documented, when it was documented in the first place. It also seems that even on well documented platforms, supported by the vendors, the stable ABI dream is not going to happen: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 If it makes things clearer, I can also add such a statement. > > Also I think one needs ACKs from DT maintainers before merging something > > in the respective directories, which I don't see here. > > It can go in with subsystem maintainers ack, but there are problems > with this one regardless of compatibility. > > > As I am somewhat blocked on that patch, I'd like to have some discussion > > on the list. > > > > Thanks, > > Andre. > > > > On 05/02/16 17:59, Andre Przywara wrote: > >> Hi Maxime, > >> > >> just found this while looking at your current git branch, so sorry for > >> the late reply. > >> > >> CC:ing DT people, since you touch both existing DTs(!) and the binding doc. > >> > >> On 01/02/16 20:20, Maxime Ripard wrote: > >>> Remove the fixed dividers from the PLL6 driver to be able to have a > >>> reusable driver that can be used across several SoCs that share the same > >>> controller, but don't have the same set of dividers for this clock, and to > >>> also be reused multiple times in the same SoC, since we're droping the > >>> clock name. > > Removing a compatible name or not has nothing to do with sharing code. This was not about the compatible name, but the hardcoded name in our structure associated to that compatible. And since we can't register two clocks with the same name, we couldn't use the same compatible several times. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-10 14:37 ` Maxime Ripard @ 2016-02-10 14:45 ` Arnd Bergmann 2016-02-10 16:14 ` breaking DT compatibility Andre Przywara 2016-02-10 16:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Mark Rutland 2 siblings, 0 replies; 23+ messages in thread From: Arnd Bergmann @ 2016-02-10 14:45 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Mike Turquette, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org, linux-arm-kernel On Wednesday 10 February 2016 15:37:55 Maxime Ripard wrote: > Hi Rob, > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > > Hi, > > > > > > just a ping: > > > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > > > I only warn and make sure people are aware of the issue. I leave that > > up to platform maintainers to decide. It depends on the maturity of > > the platform and users. > > The impacted SoCs support is really partial. For the most supported > one, big things like display or sound are totally missing, and we > still update them on a regular basis to add support for new > devices. As such, users are very likely to upgrade the DT from one > version to another just because there's new devices available to > them. And the newest SoC impacted just got introduced in 4.5, and only > has the UART and MMC devices available. > > I think the main problems we had were around people building their own boards and maintaining out-of-tree dts files. Arnd ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility 2016-02-10 14:37 ` Maxime Ripard 2016-02-10 14:45 ` Arnd Bergmann @ 2016-02-10 16:14 ` Andre Przywara 2016-02-11 10:16 ` Maxime Ripard 2016-02-10 16:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Mark Rutland 2 siblings, 1 reply; 23+ messages in thread From: Andre Przywara @ 2016-02-10 16:14 UTC (permalink / raw) To: Maxime Ripard, Rob Herring Cc: Mark Rutland, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel Hi, On 10/02/16 14:37, Maxime Ripard wrote: > Hi Rob, > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: >> On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: >>> Hi, >>> >>> just a ping: >>> >>> Are we really OK with breaking existing DTs in 4.6? (per the code in >>> -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). >> >> I only warn and make sure people are aware of the issue. I leave that >> up to platform maintainers to decide. It depends on the maturity of >> the platform and users. > > The impacted SoCs support is really partial. For the most supported > one, big things like display or sound are totally missing, and we > still update them on a regular basis to add support for new > devices. As such, users are very likely to upgrade the DT from one > version to another just because there's new devices available to > them. And the newest SoC impacted just got introduced in 4.5, and only > has the UART and MMC devices available. But I still don't see why we have to break things - can't we just improve support _without_ breaking compatibility? For instance keeping the old behaviour around? I can dig out my old x86 hat and find a _compatible_ solution for this, if you prefer ;-) Actually doesn't Jean-Francois' patch fixes the PLL8 issue without breaking anything? Also were do you want to draw the line for "partial support"? The sun6i-a31.dtsi is around since 3.12, which was released more than 2 years ago. If we want to wait for "stabilization", we will probably wait forever. I find discussions about DT stabilization going back to the very beginning of DT for ARM - as well as the request for moving the DT bindings out of the kernel (which I think we should really eventually do now). Frankly I don't care so much about these particular boards, but just want avoid to set a bad example for the future - in particular sneaking this behaviour into the arm64 world, where DTs _really_ come with the board or are even generated by the (UEFI-)firmware. U-Boot already can embed a device tree, sounds like a perfect place to have it stored for me - as long as there is _one_ best version of it. >> If people complain about it then it's their mess. For platforms >> supported in distros such as debian or fedora, I would strongly >> recommend against breaking compatibility. > > None of them are officially supported: > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > Only the older one are, and they are not affected by this patch. > >> They do ship dtbs, but it's a chicken and egg problem. If dtbs were >> stable and provided by firmware, then they wouldn't have to provide >> them. If dtbs are unstable, then they have no choice. > > And while it might work great on platforms that have all the needed > documentation, or a perfect one, which is our case. Almost each > release, we discover that something is not working as it was > documented, when it was documented in the first place. I understand that it's not an ideal situation for those Allwinner chips - but nevertheless I would like us to at least _try_ to comply with this idea. As said before: this patch is a nice rework/cleanup - but definitely not a good reason to break compatibility. > It also seems that even on well documented platforms, supported by the > vendors, the stable ABI dream is not going to happen: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 I find those two statements really unfortunate ... > If it makes things clearer, I can also add such a statement. and would rather see us referring solely to this document instead: Documentation/devicetree/bindings/ABI.txt Cheers, Andre. >>> Also I think one needs ACKs from DT maintainers before merging something >>> in the respective directories, which I don't see here. >> >> It can go in with subsystem maintainers ack, but there are problems >> with this one regardless of compatibility. >> >>> As I am somewhat blocked on that patch, I'd like to have some discussion >>> on the list. >>> >>> Thanks, >>> Andre. >>> >>> On 05/02/16 17:59, Andre Przywara wrote: >>>> Hi Maxime, >>>> >>>> just found this while looking at your current git branch, so sorry for >>>> the late reply. >>>> >>>> CC:ing DT people, since you touch both existing DTs(!) and the binding doc. >>>> >>>> On 01/02/16 20:20, Maxime Ripard wrote: >>>>> Remove the fixed dividers from the PLL6 driver to be able to have a >>>>> reusable driver that can be used across several SoCs that share the same >>>>> controller, but don't have the same set of dividers for this clock, and to >>>>> also be reused multiple times in the same SoC, since we're droping the >>>>> clock name. >> >> Removing a compatible name or not has nothing to do with sharing code. > > This was not about the compatible name, but the hardcoded name in our > structure associated to that compatible. And since we can't register > two clocks with the same name, we couldn't use the same compatible > several times. > > Maxime > ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility 2016-02-10 16:14 ` breaking DT compatibility Andre Przywara @ 2016-02-11 10:16 ` Maxime Ripard 0 siblings, 0 replies; 23+ messages in thread From: Maxime Ripard @ 2016-02-11 10:16 UTC (permalink / raw) To: Andre Przywara Cc: Rob Herring, Mark Rutland, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 4927 bytes --] On Wed, Feb 10, 2016 at 04:14:34PM +0000, Andre Przywara wrote: > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > >> On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > >>> Hi, > >>> > >>> just a ping: > >>> > >>> Are we really OK with breaking existing DTs in 4.6? (per the code in > >>> -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > >> > >> I only warn and make sure people are aware of the issue. I leave that > >> up to platform maintainers to decide. It depends on the maturity of > >> the platform and users. > > > > The impacted SoCs support is really partial. For the most supported > > one, big things like display or sound are totally missing, and we > > still update them on a regular basis to add support for new > > devices. As such, users are very likely to upgrade the DT from one > > version to another just because there's new devices available to > > them. And the newest SoC impacted just got introduced in 4.5, and only > > has the UART and MMC devices available. > > But I still don't see why we have to break things - can't we just > improve support _without_ breaking compatibility? For instance keeping > the old behaviour around? I can dig out my old x86 hat and find a > _compatible_ solution for this, if you prefer ;-) > Actually doesn't Jean-Francois' patch fixes the PLL8 issue without > breaking anything? Are you convinced that merging code that is not going to be used or tested (and thus maintained) by anyone is a good idea? Using X86 as an example is quite funny, since I remember on a few occasions having to reflash my BIOS/UEFI to fix some issues in Linux (and as it turns out, the last occurence was two weeks ago). > Also were do you want to draw the line for "partial support"? The > sun6i-a31.dtsi is around since 3.12, which was released more than 2 > years ago. > If we want to wait for "stabilization", we will probably wait forever. I > find discussions about DT stabilization going back to the very beginning > of DT for ARM - as well as the request for moving the DT bindings out of > the kernel (which I think we should really eventually do now). I'd say when we have merged everything that makes it work as it was intented to? In this case, it's a multimedia SoCs. So I'd draw the line at once we had display, sound and VPU. Display is coming, sound seems to be gaining traction lately, so that doesn't sound that far off. > Frankly I don't care so much about these particular boards, but just > want avoid to set a bad example for the future - in particular sneaking > this behaviour into the arm64 world, where DTs _really_ come with the > board or are even generated by the (UEFI-)firmware. Ok, that's a good thing to know, and we'll make sure it won't happen. What's the policy when the provided DT comes from a vendor in that case? > U-Boot already can embed a device tree, sounds like a perfect place to > have it stored for me - as long as there is _one_ best version of it. So far, they've been using their own, with some custom properties that were rejected last time they were submitted. http://lists.infradead.org/pipermail/linux-rpi-kernel/2015-August/002122.html So I'm a bit pessimistic about that. > >> If people complain about it then it's their mess. For platforms > >> supported in distros such as debian or fedora, I would strongly > >> recommend against breaking compatibility. > > > > None of them are officially supported: > > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > > > Only the older one are, and they are not affected by this patch. > > > >> They do ship dtbs, but it's a chicken and egg problem. If dtbs were > >> stable and provided by firmware, then they wouldn't have to provide > >> them. If dtbs are unstable, then they have no choice. > > > > And while it might work great on platforms that have all the needed > > documentation, or a perfect one, which is our case. Almost each > > release, we discover that something is not working as it was > > documented, when it was documented in the first place. > > I understand that it's not an ideal situation for those Allwinner > chips - but nevertheless I would like us to at least _try_ to comply > with this idea. As said before: this patch is a nice rework/cleanup > - but definitely not a good reason to break compatibility. To be fair, I really want to tend to keep a DT ABI as stable as possible. I just don't want to commit to it. If it comes in the way of a nice rework/cleanup, then it shouldn't matter. But that doesn't make me want to break everything every kernel release either. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-10 14:37 ` Maxime Ripard 2016-02-10 14:45 ` Arnd Bergmann 2016-02-10 16:14 ` breaking DT compatibility Andre Przywara @ 2016-02-10 16:30 ` Mark Rutland 2016-02-11 10:00 ` Maxime Ripard 2 siblings, 1 reply; 23+ messages in thread From: Mark Rutland @ 2016-02-10 16:30 UTC (permalink / raw) To: Maxime Ripard Cc: Rob Herring, Andre Przywara, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: > Hi Rob, > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > > Hi, > > > > > > just a ping: > > > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > > > I only warn and make sure people are aware of the issue. I leave that > > up to platform maintainers to decide. It depends on the maturity of > > the platform and users. > > The impacted SoCs support is really partial. For the most supported > one, big things like display or sound are totally missing, and we > still update them on a regular basis to add support for new > devices. As such, users are very likely to upgrade the DT from one > version to another just because there's new devices available to > them. And the newest SoC impacted just got introduced in 4.5, and only > has the UART and MMC devices available. > > > If people complain about it then it's their mess. For platforms > > supported in distros such as debian or fedora, I would strongly > > recommend against breaking compatibility. > > None of them are officially supported: > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > Only the older one are, and they are not affected by this patch. > > > They do ship dtbs, but it's a chicken and egg problem. If dtbs were > > stable and provided by firmware, then they wouldn't have to provide > > them. If dtbs are unstable, then they have no choice. > > And while it might work great on platforms that have all the needed > documentation, or a perfect one, which is our case. Almost each > release, we discover that something is not working as it was > documented, when it was documented in the first place. > > It also seems that even on well documented platforms, supported by the > vendors, the stable ABI dream is not going to happen: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 To be quite frank, I completely disagree with that stance. It seems like the only reason DT bindings aren't remaining stable is because people are deliberately ignoring the requirement and reasoning for doing so. I agree that it can be painful, and that we cannot predict the future. There will always be bugs. Having code in mainline comes with responsibilities. One of those is to keep said code working for existing users. Otherwise, why bother having it in mainline at all? Mark. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-10 16:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Mark Rutland @ 2016-02-11 10:00 ` Maxime Ripard 2016-02-11 11:44 ` Mark Rutland ` (2 more replies) 0 siblings, 3 replies; 23+ messages in thread From: Maxime Ripard @ 2016-02-11 10:00 UTC (permalink / raw) To: Mark Rutland Cc: Rob Herring, Andre Przywara, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 3660 bytes --] On Wed, Feb 10, 2016 at 04:30:01PM +0000, Mark Rutland wrote: > On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: > > Hi Rob, > > > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > > > Hi, > > > > > > > > just a ping: > > > > > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > > > > > I only warn and make sure people are aware of the issue. I leave that > > > up to platform maintainers to decide. It depends on the maturity of > > > the platform and users. > > > > The impacted SoCs support is really partial. For the most supported > > one, big things like display or sound are totally missing, and we > > still update them on a regular basis to add support for new > > devices. As such, users are very likely to upgrade the DT from one > > version to another just because there's new devices available to > > them. And the newest SoC impacted just got introduced in 4.5, and only > > has the UART and MMC devices available. > > > > > If people complain about it then it's their mess. For platforms > > > supported in distros such as debian or fedora, I would strongly > > > recommend against breaking compatibility. > > > > None of them are officially supported: > > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > > > Only the older one are, and they are not affected by this patch. > > > > > They do ship dtbs, but it's a chicken and egg problem. If dtbs were > > > stable and provided by firmware, then they wouldn't have to provide > > > them. If dtbs are unstable, then they have no choice. > > > > And while it might work great on platforms that have all the needed > > documentation, or a perfect one, which is our case. Almost each > > release, we discover that something is not working as it was > > documented, when it was documented in the first place. > > > > It also seems that even on well documented platforms, supported by the > > vendors, the stable ABI dream is not going to happen: > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 > > To be quite frank, I completely disagree with that stance. > > It seems like the only reason DT bindings aren't remaining stable is > because people are deliberately ignoring the requirement and reasoning > for doing so. And for DT maintainers saying on multiple occasions that it's bad but ok to break it and / or that they never actually said that it was a stable ABI... I'm guessing it could be a stable ABI if there was bindings reviews. Rob actually started to review a significant amount of bindings lately, and that's really appreciated, but if you don't review all the bindings, then we're going to make mistakes. > I agree that it can be painful, and that we cannot predict the future. > There will always be bugs. In our case, we can't even predict the present. > Having code in mainline comes with responsibilities. One of those is to > keep said code working for existing users. Otherwise, why bother having > it in mainline at all? None of our existing users ever complained. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-11 10:00 ` Maxime Ripard @ 2016-02-11 11:44 ` Mark Rutland 2016-02-11 12:29 ` breaking DT compatibility Andre Przywara 2016-02-11 17:08 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Maxime Ripard 2016-02-11 14:51 ` Richard Cochran 2016-02-11 21:46 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Rob Herring 2 siblings, 2 replies; 23+ messages in thread From: Mark Rutland @ 2016-02-11 11:44 UTC (permalink / raw) To: Maxime Ripard Cc: Rob Herring, Andre Przywara, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel On Thu, Feb 11, 2016 at 11:00:48AM +0100, Maxime Ripard wrote: > On Wed, Feb 10, 2016 at 04:30:01PM +0000, Mark Rutland wrote: > > On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: > > > Hi Rob, > > > > > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > > > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > > > > Hi, > > > > > > > > > > just a ping: > > > > > > > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > > > > > > > I only warn and make sure people are aware of the issue. I leave that > > > > up to platform maintainers to decide. It depends on the maturity of > > > > the platform and users. > > > > > > The impacted SoCs support is really partial. For the most supported > > > one, big things like display or sound are totally missing, and we > > > still update them on a regular basis to add support for new > > > devices. As such, users are very likely to upgrade the DT from one > > > version to another just because there's new devices available to > > > them. And the newest SoC impacted just got introduced in 4.5, and only > > > has the UART and MMC devices available. > > > > > > > If people complain about it then it's their mess. For platforms > > > > supported in distros such as debian or fedora, I would strongly > > > > recommend against breaking compatibility. > > > > > > None of them are officially supported: > > > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > > > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > > > > > Only the older one are, and they are not affected by this patch. > > > > > > > They do ship dtbs, but it's a chicken and egg problem. If dtbs were > > > > stable and provided by firmware, then they wouldn't have to provide > > > > them. If dtbs are unstable, then they have no choice. > > > > > > And while it might work great on platforms that have all the needed > > > documentation, or a perfect one, which is our case. Almost each > > > release, we discover that something is not working as it was > > > documented, when it was documented in the first place. > > > > > > It also seems that even on well documented platforms, supported by the > > > vendors, the stable ABI dream is not going to happen: > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 > > > > To be quite frank, I completely disagree with that stance. > > > > It seems like the only reason DT bindings aren't remaining stable is > > because people are deliberately ignoring the requirement and reasoning > > for doing so. > > And for DT maintainers saying on multiple occasions that it's bad but > ok to break it and / or that they never actually said that it was a > stable ABI... Evidently there is a communication failure. Generally, the statement has been that old DTBs should continue to work. That's even documented, as Andre pointed out: Documentation/devicetree/bindings/ABI.txt There are obviously shades of grey, and _rarely_ it might be necessary to deliberately break a binding. However, that should be the rare last-resort case, rather than a crutch for development. Saying "bad but ok" underplays the "bad" and overplays the "ok". > I'm guessing it could be a stable ABI if there was bindings > reviews. Rob actually started to review a significant amount of > bindings lately, and that's really appreciated, but if you don't > review all the bindings, then we're going to make mistakes. I agree that it is unfortunate that we cannot provide the level of review that we want to. Mistakes will happen even with review; that doesn't necessitate removing support for a binding. > > I agree that it can be painful, and that we cannot predict the future. > > There will always be bugs. > > In our case, we can't even predict the present. > > > Having code in mainline comes with responsibilities. One of those is to > > keep said code working for existing users. Otherwise, why bother having > > it in mainline at all? > > None of our existing users ever complained. I believe that in this case, Andre was complaining about this particular breakage, unless I have misunderstood. To be clear, I'm arguing for the strategy going forward. If no-one has complained about the stuff broken up to this point, let's not waste time restoring that. Going forward we need to keep old DTBs supported. Thanks, Mark. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility 2016-02-11 11:44 ` Mark Rutland @ 2016-02-11 12:29 ` Andre Przywara 2016-02-11 17:08 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Maxime Ripard 1 sibling, 0 replies; 23+ messages in thread From: Andre Przywara @ 2016-02-11 12:29 UTC (permalink / raw) To: Mark Rutland Cc: Maxime Ripard, Rob Herring, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel Hi, On 11/02/16 11:44, Mark Rutland wrote: > On Thu, Feb 11, 2016 at 11:00:48AM +0100, Maxime Ripard wrote: >> On Wed, Feb 10, 2016 at 04:30:01PM +0000, Mark Rutland wrote: >>> On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: >>>> Hi Rob, >>>> >>>> On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: >>>>> On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: >>>>>> Hi, >>>>>> >>>>>> just a ping: >>>>>> >>>>>> Are we really OK with breaking existing DTs in 4.6? (per the code in >>>>>> -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). >>>>> >>>>> I only warn and make sure people are aware of the issue. I leave that >>>>> up to platform maintainers to decide. It depends on the maturity of >>>>> the platform and users. >>>> >>>> The impacted SoCs support is really partial. For the most supported >>>> one, big things like display or sound are totally missing, and we >>>> still update them on a regular basis to add support for new >>>> devices. As such, users are very likely to upgrade the DT from one >>>> version to another just because there's new devices available to >>>> them. And the newest SoC impacted just got introduced in 4.5, and only >>>> has the UART and MMC devices available. >>>> >>>>> If people complain about it then it's their mess. For platforms >>>>> supported in distros such as debian or fedora, I would strongly >>>>> recommend against breaking compatibility. >>>> >>>> None of them are officially supported: >>>> https://www.debian.org/releases/stable/armhf/ch02s01.html.en >>>> https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 >>>> >>>> Only the older one are, and they are not affected by this patch. >>>> >>>>> They do ship dtbs, but it's a chicken and egg problem. If dtbs were >>>>> stable and provided by firmware, then they wouldn't have to provide >>>>> them. If dtbs are unstable, then they have no choice. >>>> >>>> And while it might work great on platforms that have all the needed >>>> documentation, or a perfect one, which is our case. Almost each >>>> release, we discover that something is not working as it was >>>> documented, when it was documented in the first place. >>>> >>>> It also seems that even on well documented platforms, supported by the >>>> vendors, the stable ABI dream is not going to happen: >>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 >>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 >>> >>> To be quite frank, I completely disagree with that stance. >>> >>> It seems like the only reason DT bindings aren't remaining stable is >>> because people are deliberately ignoring the requirement and reasoning >>> for doing so. >> >> And for DT maintainers saying on multiple occasions that it's bad but >> ok to break it and / or that they never actually said that it was a >> stable ABI... > > Evidently there is a communication failure. Generally, the statement has > been that old DTBs should continue to work. That's even documented, as > Andre pointed out: > > Documentation/devicetree/bindings/ABI.txt > > There are obviously shades of grey, and _rarely_ it might be necessary > to deliberately break a binding. However, that should be the rare > last-resort case, rather than a crutch for development. > > Saying "bad but ok" underplays the "bad" and overplays the "ok". > >> I'm guessing it could be a stable ABI if there was bindings >> reviews. Rob actually started to review a significant amount of >> bindings lately, and that's really appreciated, but if you don't >> review all the bindings, then we're going to make mistakes. > > I agree that it is unfortunate that we cannot provide the level of > review that we want to. > > Mistakes will happen even with review; that doesn't necessitate removing > support for a binding. > >>> I agree that it can be painful, and that we cannot predict the future. >>> There will always be bugs. >> >> In our case, we can't even predict the present. >> >>> Having code in mainline comes with responsibilities. One of those is to >>> keep said code working for existing users. Otherwise, why bother having >>> it in mainline at all? >> >> None of our existing users ever complained. > > I believe that in this case, Andre was complaining about this particular > breakage, unless I have misunderstood. > > To be clear, I'm arguing for the strategy going forward. If no-one has > complained about the stuff broken up to this point, let's not waste time > restoring that. > > Going forward we need to keep old DTBs supported. FWIW: I had a chat with Maxime and we exchanged ideas on how to go on from here, by reworking this commit to be both compatible and fixing the issue we are having with the current PLL6 binding. I will look at generating patches for this shortly. So I guess we can move the discussion more to a code level from here on. Cheers, Andre. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-11 11:44 ` Mark Rutland 2016-02-11 12:29 ` breaking DT compatibility Andre Przywara @ 2016-02-11 17:08 ` Maxime Ripard 2016-02-12 9:40 ` Lucas Stach 1 sibling, 1 reply; 23+ messages in thread From: Maxime Ripard @ 2016-02-11 17:08 UTC (permalink / raw) To: Mark Rutland Cc: Rob Herring, Andre Przywara, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree@vger.kernel.org, Jens Kuske, linux-clk, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 5667 bytes --] On Thu, Feb 11, 2016 at 11:44:10AM +0000, Mark Rutland wrote: > On Thu, Feb 11, 2016 at 11:00:48AM +0100, Maxime Ripard wrote: > > On Wed, Feb 10, 2016 at 04:30:01PM +0000, Mark Rutland wrote: > > > On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: > > > > Hi Rob, > > > > > > > > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: > > > > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara@arm.com> wrote: > > > > > > Hi, > > > > > > > > > > > > just a ping: > > > > > > > > > > > > Are we really OK with breaking existing DTs in 4.6? (per the code in > > > > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). > > > > > > > > > > I only warn and make sure people are aware of the issue. I leave that > > > > > up to platform maintainers to decide. It depends on the maturity of > > > > > the platform and users. > > > > > > > > The impacted SoCs support is really partial. For the most supported > > > > one, big things like display or sound are totally missing, and we > > > > still update them on a regular basis to add support for new > > > > devices. As such, users are very likely to upgrade the DT from one > > > > version to another just because there's new devices available to > > > > them. And the newest SoC impacted just got introduced in 4.5, and only > > > > has the UART and MMC devices available. > > > > > > > > > If people complain about it then it's their mess. For platforms > > > > > supported in distros such as debian or fedora, I would strongly > > > > > recommend against breaking compatibility. > > > > > > > > None of them are officially supported: > > > > https://www.debian.org/releases/stable/armhf/ch02s01.html.en > > > > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 > > > > > > > > Only the older one are, and they are not affected by this patch. > > > > > > > > > They do ship dtbs, but it's a chicken and egg problem. If dtbs were > > > > > stable and provided by firmware, then they wouldn't have to provide > > > > > them. If dtbs are unstable, then they have no choice. > > > > > > > > And while it might work great on platforms that have all the needed > > > > documentation, or a perfect one, which is our case. Almost each > > > > release, we discover that something is not working as it was > > > > documented, when it was documented in the first place. > > > > > > > > It also seems that even on well documented platforms, supported by the > > > > vendors, the stable ABI dream is not going to happen: > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 > > > > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 > > > > > > To be quite frank, I completely disagree with that stance. > > > > > > It seems like the only reason DT bindings aren't remaining stable is > > > because people are deliberately ignoring the requirement and reasoning > > > for doing so. > > > > And for DT maintainers saying on multiple occasions that it's bad but > > ok to break it and / or that they never actually said that it was a > > stable ABI... > > Evidently there is a communication failure. Generally, the statement has > been that old DTBs should continue to work. That's even documented, as > Andre pointed out: > > Documentation/devicetree/bindings/ABI.txt > > There are obviously shades of grey, and _rarely_ it might be necessary > to deliberately break a binding. However, that should be the rare > last-resort case, rather than a crutch for development. > > Saying "bad but ok" underplays the "bad" and overplays the "ok". Not all of the DT maintainers have the same position I'm afraid. > > I'm guessing it could be a stable ABI if there was bindings > > reviews. Rob actually started to review a significant amount of > > bindings lately, and that's really appreciated, but if you don't > > review all the bindings, then we're going to make mistakes. > > I agree that it is unfortunate that we cannot provide the level of > review that we want to. > > Mistakes will happen even with review; that doesn't necessitate removing > support for a binding. Yet they will be much more likely to be made if no review is made. > > > I agree that it can be painful, and that we cannot predict the future. > > > There will always be bugs. > > > > In our case, we can't even predict the present. > > > > > Having code in mainline comes with responsibilities. One of those is to > > > keep said code working for existing users. Otherwise, why bother having > > > it in mainline at all? > > > > None of our existing users ever complained. > > I believe that in this case, Andre was complaining about this particular > breakage, unless I have misunderstood. > > To be clear, I'm arguing for the strategy going forward. If no-one has > complained about the stuff broken up to this point, let's not waste time > restoring that. > > Going forward we need to keep old DTBs supported. I find that stand a bit dishonest. You, DT maintainers, admit that you're not doing your job properly, and that burden relies on the platform maintainers? Or should I take it as you volunteering to maintain that code? But ok. Let's do that. Make sure that the other platform maintainers are aware that this is the rule too though. I surely don't want to be alone in that boat. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-11 17:08 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Maxime Ripard @ 2016-02-12 9:40 ` Lucas Stach 2016-02-16 8:44 ` Maxime Ripard 0 siblings, 1 reply; 23+ messages in thread From: Lucas Stach @ 2016-02-12 9:40 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Mike Turquette, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org Am Donnerstag, den 11.02.2016, 18:08 +0100 schrieb Maxime Ripard: [...] > > > > Having code in mainline comes with responsibilities. One of those is to > > > > keep said code working for existing users. Otherwise, why bother having > > > > it in mainline at all? > > > > > > None of our existing users ever complained. > > > > I believe that in this case, Andre was complaining about this particular > > breakage, unless I have misunderstood. > > > > To be clear, I'm arguing for the strategy going forward. If no-one has > > complained about the stuff broken up to this point, let's not waste time > > restoring that. > > > > Going forward we need to keep old DTBs supported. > > I find that stand a bit dishonest. > > You, DT maintainers, admit that you're not doing your job properly, > and that burden relies on the platform maintainers? Or should I take > it as you volunteering to maintain that code? > > But ok. Let's do that. Make sure that the other platform maintainers > are aware that this is the rule too though. I surely don't want to be > alone in that boat. FWIW: I always thought it's the platform maintainers job to enforce a reasonable level of DT stability. I don't see how the DT maintainers could provide the necessary in-depth review with every platform being different in many subtle ways. For the i.MX platform we actually enforced a baseline of DT stability by shooting down patches that break DT stability for the sake of adding new features, or when trying to put "fixes" into the DT, that could be solved entirely inside the kernel. Yes, mistakes happen and and we can not really prevent all breakage, especially when the bindings were not strictly enough defined and board DT writers may have interpreted them differently, but it is definitely possible to keep DTs reasonably stable if the platform maintainers care about that. I strongly disagree with platform maintainers denying that duty, by claiming that DTs won't be completely stable ever, so there is no reason to even care. Regards, Lucas -- Pengutronix e.K. | Lucas Stach | Industrial Linux Solutions | http://www.pengutronix.de/ | ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-12 9:40 ` Lucas Stach @ 2016-02-16 8:44 ` Maxime Ripard 2016-02-16 19:40 ` Michael Turquette 0 siblings, 1 reply; 23+ messages in thread From: Maxime Ripard @ 2016-02-16 8:44 UTC (permalink / raw) To: Lucas Stach Cc: Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Mike Turquette, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2426 bytes --] On Fri, Feb 12, 2016 at 10:40:30AM +0100, Lucas Stach wrote: > Am Donnerstag, den 11.02.2016, 18:08 +0100 schrieb Maxime Ripard: > [...] > > > > > Having code in mainline comes with responsibilities. One of those is to > > > > > keep said code working for existing users. Otherwise, why bother having > > > > > it in mainline at all? > > > > > > > > None of our existing users ever complained. > > > > > > I believe that in this case, Andre was complaining about this particular > > > breakage, unless I have misunderstood. > > > > > > To be clear, I'm arguing for the strategy going forward. If no-one has > > > complained about the stuff broken up to this point, let's not waste time > > > restoring that. > > > > > > Going forward we need to keep old DTBs supported. > > > > I find that stand a bit dishonest. > > > > You, DT maintainers, admit that you're not doing your job properly, > > and that burden relies on the platform maintainers? Or should I take > > it as you volunteering to maintain that code? > > > > But ok. Let's do that. Make sure that the other platform maintainers > > are aware that this is the rule too though. I surely don't want to be > > alone in that boat. > > FWIW: I always thought it's the platform maintainers job to enforce a > reasonable level of DT stability. I don't see how the DT maintainers > could provide the necessary in-depth review with every platform being > different in many subtle ways. > > For the i.MX platform we actually enforced a baseline of DT stability by > shooting down patches that break DT stability for the sake of adding new > features, or when trying to put "fixes" into the DT, that could be > solved entirely inside the kernel. > > Yes, mistakes happen and and we can not really prevent all breakage, > especially when the bindings were not strictly enough defined and board > DT writers may have interpreted them differently, but it is definitely > possible to keep DTs reasonably stable if the platform maintainers care > about that. > > I strongly disagree with platform maintainers denying that duty, by > claiming that DTs won't be completely stable ever, so there is no reason > to even care. A DT is either stable or not. If it is "reasonably" stable, then it's not. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-16 8:44 ` Maxime Ripard @ 2016-02-16 19:40 ` Michael Turquette 2016-02-16 21:11 ` Rob Herring 0 siblings, 1 reply; 23+ messages in thread From: Michael Turquette @ 2016-02-16 19:40 UTC (permalink / raw) To: Maxime Ripard, Lucas Stach Cc: Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org Quoting Maxime Ripard (2016-02-16 00:44:48) > On Fri, Feb 12, 2016 at 10:40:30AM +0100, Lucas Stach wrote: > > Am Donnerstag, den 11.02.2016, 18:08 +0100 schrieb Maxime Ripard: > > [...] > > > > > > Having code in mainline comes with responsibilities. One of those is to > > > > > > keep said code working for existing users. Otherwise, why bother having > > > > > > it in mainline at all? > > > > > > > > > > None of our existing users ever complained. > > > > > > > > I believe that in this case, Andre was complaining about this particular > > > > breakage, unless I have misunderstood. > > > > > > > > To be clear, I'm arguing for the strategy going forward. If no-one has > > > > complained about the stuff broken up to this point, let's not waste time > > > > restoring that. > > > > > > > > Going forward we need to keep old DTBs supported. > > > > > > I find that stand a bit dishonest. > > > > > > You, DT maintainers, admit that you're not doing your job properly, > > > and that burden relies on the platform maintainers? Or should I take > > > it as you volunteering to maintain that code? > > > > > > But ok. Let's do that. Make sure that the other platform maintainers > > > are aware that this is the rule too though. I surely don't want to be > > > alone in that boat. > > > > FWIW: I always thought it's the platform maintainers job to enforce a > > reasonable level of DT stability. I don't see how the DT maintainers > > could provide the necessary in-depth review with every platform being > > different in many subtle ways. > > > > For the i.MX platform we actually enforced a baseline of DT stability by > > shooting down patches that break DT stability for the sake of adding new > > features, or when trying to put "fixes" into the DT, that could be > > solved entirely inside the kernel. > > > > Yes, mistakes happen and and we can not really prevent all breakage, > > especially when the bindings were not strictly enough defined and board > > DT writers may have interpreted them differently, but it is definitely > > possible to keep DTs reasonably stable if the platform maintainers care > > about that. > > > > I strongly disagree with platform maintainers denying that duty, by > > claiming that DTs won't be completely stable ever, so there is no reason > > to even care. > > A DT is either stable or not. If it is "reasonably" stable, then it's > not. Hi all, I thought I'd pour some gasoline on this fire. I'm happy for the one-node-per-clock bindings to be fully converted to a clock-controller style binding, which is a clear compatibility break. In other words, clock-cells = 0 bindings converted to clock-cells >= 1. We really didn't have a clue about good DT bindings for a while, and everyone keeps talking about being reasonable, etc. So with that in mind, I propose that any clock binding written before 2015 should be given amnesty to make these incompatible changes, so long as the platform maintainers consent. Those stakeholders for sunxi are Maxime Ripard, Chen-Yu Tsai and Emilio López. /me puts on flame retardant suit. To be clear, I'm not in the business of telling SoCs to break compatibility. That is thankfully not my choice to make and any platform maintainer should have a long, difficult struggle when making that decision. But I am supportive of merging those changes for clock provider drivers if the platform maintainer decides to do so. Regards, Mike > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux, Kernel and Android engineering > http://free-electrons.com ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-16 19:40 ` Michael Turquette @ 2016-02-16 21:11 ` Rob Herring 0 siblings, 0 replies; 23+ messages in thread From: Rob Herring @ 2016-02-16 21:11 UTC (permalink / raw) To: Michael Turquette Cc: Maxime Ripard, Lucas Stach, Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org On Tue, Feb 16, 2016 at 1:40 PM, Michael Turquette <mturquette@baylibre.com> wrote: > Quoting Maxime Ripard (2016-02-16 00:44:48) >> On Fri, Feb 12, 2016 at 10:40:30AM +0100, Lucas Stach wrote: >> > Am Donnerstag, den 11.02.2016, 18:08 +0100 schrieb Maxime Ripard: >> > [...] >> > > > > > Having code in mainline comes with responsibilities. One of those is to >> > > > > > keep said code working for existing users. Otherwise, why bother having >> > > > > > it in mainline at all? >> > > > > >> > > > > None of our existing users ever complained. >> > > > >> > > > I believe that in this case, Andre was complaining about this particular >> > > > breakage, unless I have misunderstood. >> > > > >> > > > To be clear, I'm arguing for the strategy going forward. If no-one has >> > > > complained about the stuff broken up to this point, let's not waste time >> > > > restoring that. >> > > > >> > > > Going forward we need to keep old DTBs supported. >> > > >> > > I find that stand a bit dishonest. >> > > >> > > You, DT maintainers, admit that you're not doing your job properly, >> > > and that burden relies on the platform maintainers? Or should I take >> > > it as you volunteering to maintain that code? >> > > >> > > But ok. Let's do that. Make sure that the other platform maintainers >> > > are aware that this is the rule too though. I surely don't want to be >> > > alone in that boat. >> > >> > FWIW: I always thought it's the platform maintainers job to enforce a >> > reasonable level of DT stability. I don't see how the DT maintainers >> > could provide the necessary in-depth review with every platform being >> > different in many subtle ways. >> > >> > For the i.MX platform we actually enforced a baseline of DT stability by >> > shooting down patches that break DT stability for the sake of adding new >> > features, or when trying to put "fixes" into the DT, that could be >> > solved entirely inside the kernel. >> > >> > Yes, mistakes happen and and we can not really prevent all breakage, >> > especially when the bindings were not strictly enough defined and board >> > DT writers may have interpreted them differently, but it is definitely >> > possible to keep DTs reasonably stable if the platform maintainers care >> > about that. >> > >> > I strongly disagree with platform maintainers denying that duty, by >> > claiming that DTs won't be completely stable ever, so there is no reason >> > to even care. >> >> A DT is either stable or not. If it is "reasonably" stable, then it's >> not. > > Hi all, > > I thought I'd pour some gasoline on this fire. > > I'm happy for the one-node-per-clock bindings to be fully converted to a > clock-controller style binding, which is a clear compatibility break. In > other words, clock-cells = 0 bindings converted to clock-cells >= 1. In general I agree unless the platform is stable (e.g. highbank). > We really didn't have a clue about good DT bindings for a while, and > everyone keeps talking about being reasonable, etc. So with that in > mind, I propose that any clock binding written before 2015 should be > given amnesty to make these incompatible changes, so long as the > platform maintainers consent. Those stakeholders for sunxi are Maxime > Ripard, Chen-Yu Tsai and Emilio López. > > /me puts on flame retardant suit. > > To be clear, I'm not in the business of telling SoCs to break > compatibility. That is thankfully not my choice to make and any platform > maintainer should have a long, difficult struggle when making that > decision. But I am supportive of merging those changes for clock > provider drivers if the platform maintainer decides to do so. The long difficult struggl > > Regards, > Mike > >> >> Maxime >> >> -- >> Maxime Ripard, Free Electrons >> Embedded Linux, Kernel and Android engineering >> http://free-electrons.com > -- > To unsubscribe from this list: send the line "unsubscribe devicetree" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-11 10:00 ` Maxime Ripard 2016-02-11 11:44 ` Mark Rutland @ 2016-02-11 14:51 ` Richard Cochran 2016-02-11 15:16 ` breaking DT compatibility Andre Przywara 2016-02-11 21:46 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Rob Herring 2 siblings, 1 reply; 23+ messages in thread From: Richard Cochran @ 2016-02-11 14:51 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Andre Przywara, Mike Turquette, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org On Thu, Feb 11, 2016 at 11:00:48AM +0100, Maxime Ripard wrote: > None of our existing users ever complained. Note to self: Avoid choosing this SoC for new projects. The maintainers do not care about the end users. Thanks, Richard ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility 2016-02-11 14:51 ` Richard Cochran @ 2016-02-11 15:16 ` Andre Przywara 0 siblings, 0 replies; 23+ messages in thread From: Andre Przywara @ 2016-02-11 15:16 UTC (permalink / raw) To: Richard Cochran, Maxime Ripard Cc: Mark Rutland, linux-arm-kernel, Rob Herring, Vishnu Patekar, Jean-Francois Moine, Mike Turquette, Stephen Boyd, Hans de Goede, Chen-Yu Tsai, Jens Kuske, Grant Likely, Frank Rowand, linux-clk, devicetree@vger.kernel.org Hi, On 11/02/16 14:51, Richard Cochran wrote: > On Thu, Feb 11, 2016 at 11:00:48AM +0100, Maxime Ripard wrote: >> None of our existing users ever complained. > > Note to self: > > Avoid choosing this SoC for new projects. The maintainers do not > care about the end users. Now that's a bit harsh, I think. Maxime is doing a great job for maintaining this admittedly not very well architected and documented SoC "family" - in his spare time. Also the actual end user experience is probably just fine (despite broken DT compatibility), since on these storage-less boards the distributions usually ship DT, firmware (U-Boot) and kernel bundled together. This does not qualify for breaking the DT deliberately, but we are about to fix this as we speak. Also my concern was just that I wanted to move away from this being the only way of running Linux on those boards. We may think about collecting DTBs and firmware for those boards in a central place without direct connection to a certain Linux version or distribution. This would make the whole idea more visible and would make compatibility breaks more evident. Cheers, Andre. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) 2016-02-11 10:00 ` Maxime Ripard 2016-02-11 11:44 ` Mark Rutland 2016-02-11 14:51 ` Richard Cochran @ 2016-02-11 21:46 ` Rob Herring 2 siblings, 0 replies; 23+ messages in thread From: Rob Herring @ 2016-02-11 21:46 UTC (permalink / raw) To: Maxime Ripard Cc: Mark Rutland, Rob Herring, Andre Przywara, Grant Likely, Frank Rowand, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jens Kuske, linux-clk, linux-arm-kernel On Thu, Feb 11, 2016 at 4:00 AM, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote: > On Wed, Feb 10, 2016 at 04:30:01PM +0000, Mark Rutland wrote: >> On Wed, Feb 10, 2016 at 03:37:55PM +0100, Maxime Ripard wrote: >> > Hi Rob, >> > >> > On Wed, Feb 10, 2016 at 07:42:02AM -0600, Rob Herring wrote: >> > > On Wed, Feb 10, 2016 at 6:30 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote: >> > > > Hi, >> > > > >> > > > just a ping: >> > > > >> > > > Are we really OK with breaking existing DTs in 4.6? (per the code in >> > > > -next: f7d372ba54ea04d528a291b8dbe34716507bb60b, which is this patch). >> > > >> > > I only warn and make sure people are aware of the issue. I leave that >> > > up to platform maintainers to decide. It depends on the maturity of >> > > the platform and users. >> > >> > The impacted SoCs support is really partial. For the most supported >> > one, big things like display or sound are totally missing, and we >> > still update them on a regular basis to add support for new >> > devices. As such, users are very likely to upgrade the DT from one >> > version to another just because there's new devices available to >> > them. And the newest SoC impacted just got introduced in 4.5, and only >> > has the UART and MMC devices available. >> > >> > > If people complain about it then it's their mess. For platforms >> > > supported in distros such as debian or fedora, I would strongly >> > > recommend against breaking compatibility. >> > >> > None of them are officially supported: >> > https://www.debian.org/releases/stable/armhf/ch02s01.html.en >> > https://fedoraproject.org/wiki/Architectures/ARM#Fedora_23 >> > >> > Only the older one are, and they are not affected by this patch. >> > >> > > They do ship dtbs, but it's a chicken and egg problem. If dtbs were >> > > stable and provided by firmware, then they wouldn't have to provide >> > > them. If dtbs are unstable, then they have no choice. >> > >> > And while it might work great on platforms that have all the needed >> > documentation, or a perfect one, which is our case. Almost each >> > release, we discover that something is not working as it was >> > documented, when it was documented in the first place. >> > >> > It also seems that even on well documented platforms, supported by the >> > vendors, the stable ABI dream is not going to happen: >> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/arm/Atmel/README#n105 >> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/arm/marvell,berlin.txt#n4 >> >> To be quite frank, I completely disagree with that stance. >> >> It seems like the only reason DT bindings aren't remaining stable is >> because people are deliberately ignoring the requirement and reasoning >> for doing so. > > And for DT maintainers saying on multiple occasions that it's bad but > ok to break it and / or that they never actually said that it was a > stable ABI... It says so in the documentation, why do we need to repeat it? While I do defer to platform maintainers judgement, if there is complete disregard for the ABI then I won't. This single case alone doesn't bother me so much, but if this is the pattern every kernel cycle I am bothered. Also, I'm inclined to go delete the text in the links above. > I'm guessing it could be a stable ABI if there was bindings > reviews. Rob actually started to review a significant amount of > bindings lately, and that's really appreciated, but if you don't > review all the bindings, then we're going to make mistakes. Everything should be reviewed. Subsystem/platform maintainers are responsible for reviewing and getting DT help if needed. There is documentation about this too. Maybe this system is not working well and needs to be revisited. I know the reviews are better for some subsystems than others. I am trying to be more actively reviewing things, but subsystem maintainers still need to review them. They should know their bindings better than me. Even if we review all bindings, there will still be mistakes. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused 2016-02-05 17:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Andre Przywara 2016-02-10 12:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Andre Przywara @ 2016-02-10 12:59 ` Maxime Ripard 2016-02-10 14:02 ` Rob Herring 2016-02-10 18:41 ` Mark Rutland 1 sibling, 2 replies; 23+ messages in thread From: Maxime Ripard @ 2016-02-10 12:59 UTC (permalink / raw) To: Andre Przywara Cc: Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, Jens Kuske, linux-clk, linux-arm-kernel, Mark Rutland, Rob Herring, Frank Rowand, Grant Likely, devicetree [-- Attachment #1: Type: text/plain, Size: 3264 bytes --] Hi, On Fri, Feb 05, 2016 at 05:59:23PM +0000, Andre Przywara wrote: > Hi Maxime, > > just found this while looking at your current git branch, so sorry for > the late reply. > > CC:ing DT people, since you touch both existing DTs(!) and the binding doc. > > On 01/02/16 20:20, Maxime Ripard wrote: > > Remove the fixed dividers from the PLL6 driver to be able to have a > > reusable driver that can be used across several SoCs that share the same > > controller, but don't have the same set of dividers for this clock, and to > > also be reused multiple times in the same SoC, since we're droping the > > clock name. > > > > Acked-by: Chen-Yu Tsai <wens@csie.org> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com> > > --- > > > > Changes from v3: > > - Fixed the documentation > > - Added pll6d2 back > > > > Changes from v2: > > - Rebased and converted over to the new factors refactoring. Fixed the > > retrieved rate > > > > Documentation/devicetree/bindings/clock/sunxi.txt | 2 +- > > arch/arm/boot/dts/sun6i-a31.dtsi | 36 +++++++++++----------- > > arch/arm/boot/dts/sun8i-a23-a33.dtsi | 25 ++++++++++----- > > arch/arm/boot/dts/sun8i-a23.dtsi | 2 +- > > arch/arm/boot/dts/sun8i-a33.dtsi | 4 +-- > > arch/arm/boot/dts/sun8i-h3.dtsi | 37 ++++++++++++++--------- > > So are you really breaking all those systems by changing the DT and the > driver in an incompatible way? Yes. > Please correct me if this assessment is wrong, but to me it looks like > any user out there is either stuck with 4.5 at best _or_ will only be > able to run 4.6 and later (depending on which version of the DT she is > using)? And no, switching DTs along with the kernel is _not_ an option. It is. And it is one that every other ARM platform chose. And so did every distribution. > That is not how I understand DT. I'm sorry for that. This has never been something we said was happening. > Also this totally ignores any other DT user (U-Boot, FreeBSD, you > name it). Which all have their own DT copies, with their own bindings, that we (ie Linux) never agreed on. By further extending that argument, you're currently looking at the DT from Allwinner, do you want to support theirs as well? > I actually appreciate this rework, it's more flexible now and looks > better, but you really can't do this in a way to breaks compatibility > with existing DTs. > > Jean-Francois came up with another solution for the pll8 clock [1], so > could this be considered at least? > I think changing the H3 PLL8 clock from dummy to something real is a > different story in terms of compatibility (since it never really worked > before and this wouldn't change for any old-DT user). So, one would have to update the DT anyway to benefit from these changes? What's the point of maintaining DT stability if you add every new release new stuff to the DT that users are going to want? Beside adding untested, unmaintained, dead, and possibly broken code, that is. Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused 2016-02-10 12:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Maxime Ripard @ 2016-02-10 14:02 ` Rob Herring 2016-02-11 9:41 ` Maxime Ripard 2016-02-10 18:41 ` Mark Rutland 1 sibling, 1 reply; 23+ messages in thread From: Rob Herring @ 2016-02-10 14:02 UTC (permalink / raw) To: Maxime Ripard Cc: Andre Przywara, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, Jens Kuske, linux-clk, linux-arm-kernel, Mark Rutland, Frank Rowand, Grant Likely, devicetree@vger.kernel.org On Wed, Feb 10, 2016 at 6:59 AM, Maxime Ripard <maxime.ripard@free-electrons.com> wrote: > Hi, > > On Fri, Feb 05, 2016 at 05:59:23PM +0000, Andre Przywara wrote: >> Hi Maxime, >> >> just found this while looking at your current git branch, so sorry for >> the late reply. >> >> CC:ing DT people, since you touch both existing DTs(!) and the binding doc. [...] >> So are you really breaking all those systems by changing the DT and the >> driver in an incompatible way? > > Yes. > >> Please correct me if this assessment is wrong, but to me it looks like >> any user out there is either stuck with 4.5 at best _or_ will only be >> able to run 4.6 and later (depending on which version of the DT she is >> using)? And no, switching DTs along with the kernel is _not_ an option. > > It is. And it is one that every other ARM platform chose. And so did > every distribution. I do leave it to platform maintainers judgement (perhaps that should be reconsidered), but it should still be case by case basis not an all out disregard for the ABI. If you decide to ignore the ABI, then there is no decision for the distros or other users. >> That is not how I understand DT. > > I'm sorry for that. This has never been something we said was > happening. > >> Also this totally ignores any other DT user (U-Boot, FreeBSD, you >> name it). > > Which all have their own DT copies, with their own bindings, that we > (ie Linux) never agreed on. That is not really true. There are some differences ATM, but they are largely derivatives of the kernel copy and the goal is to align them. If we don't maintain an ABI, that will never happen. > By further extending that argument, you're currently looking at the DT > from Allwinner, do you want to support theirs as well? Vendor ones we treat like vendor kernels. However, if the differences come down to bikeshedding, then we will take vendor bindings to maintain the ABI. That's generally only when the compatible string differs though. Rob ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused 2016-02-10 14:02 ` Rob Herring @ 2016-02-11 9:41 ` Maxime Ripard 0 siblings, 0 replies; 23+ messages in thread From: Maxime Ripard @ 2016-02-11 9:41 UTC (permalink / raw) To: Rob Herring Cc: Andre Przywara, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, Jens Kuske, linux-clk, linux-arm-kernel, Mark Rutland, Frank Rowand, Grant Likely, devicetree@vger.kernel.org [-- Attachment #1: Type: text/plain, Size: 2902 bytes --] On Wed, Feb 10, 2016 at 08:02:45AM -0600, Rob Herring wrote: > On Wed, Feb 10, 2016 at 6:59 AM, Maxime Ripard > <maxime.ripard@free-electrons.com> wrote: > > Hi, > > > > On Fri, Feb 05, 2016 at 05:59:23PM +0000, Andre Przywara wrote: > >> Hi Maxime, > >> > >> just found this while looking at your current git branch, so sorry for > >> the late reply. > >> > >> CC:ing DT people, since you touch both existing DTs(!) and the binding doc. > > [...] > > >> So are you really breaking all those systems by changing the DT and the > >> driver in an incompatible way? > > > > Yes. > > > >> Please correct me if this assessment is wrong, but to me it looks like > >> any user out there is either stuck with 4.5 at best _or_ will only be > >> able to run 4.6 and later (depending on which version of the DT she is > >> using)? And no, switching DTs along with the kernel is _not_ an option. > > > > It is. And it is one that every other ARM platform chose. And so did > > every distribution. > > I do leave it to platform maintainers judgement (perhaps that should > be reconsidered), but it should still be case by case basis not an all > out disregard for the ABI. If you decide to ignore the ABI, then there > is no decision for the distros or other users. > > >> That is not how I understand DT. > > > > I'm sorry for that. This has never been something we said was > > happening. > > > >> Also this totally ignores any other DT user (U-Boot, FreeBSD, you > >> name it). > > > > Which all have their own DT copies, with their own bindings, that we > > (ie Linux) never agreed on. > > That is not really true. There are some differences ATM, but they are > largely derivatives of the kernel copy and the goal is to align them. > If we don't maintain an ABI, that will never happen. At least for us, this is definitely true. The SoCs impacted by this change are not supported by FreeBSD at all, and the one that are supported have bindings that do not match the one we have in the kernel: https://github.com/freebsd/freebsd/blob/master/sys/boot/fdt/dts/arm/sun4i-a10.dtsi Over the 10 compatibles used there, 3 are totally different (watchdog, the SRAM controller and the clock hardware block) and two deviates in incompatible ways (timer and pinctrl) and one has property that we'd probably deny (UART, and its current-speed). 6 out of 10 overall. And U-boot never had any issue in the past with that policy. Tom said it on several occasions, and it worked for us too, with some minor compromises we needed, such as keeping some node pathes that U-Boot was using, even though it turned out to be wrong. https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/sun7i-a20.dtsi#n615 Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused 2016-02-10 12:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Maxime Ripard 2016-02-10 14:02 ` Rob Herring @ 2016-02-10 18:41 ` Mark Rutland 1 sibling, 0 replies; 23+ messages in thread From: Mark Rutland @ 2016-02-10 18:41 UTC (permalink / raw) To: Maxime Ripard Cc: Andre Przywara, Chen-Yu Tsai, Jean-Francois Moine, Vishnu Patekar, Mike Turquette, Stephen Boyd, Hans de Goede, Jens Kuske, linux-clk, linux-arm-kernel, Rob Herring, Frank Rowand, Grant Likely, devicetree On Wed, Feb 10, 2016 at 01:59:36PM +0100, Maxime Ripard wrote: > > Also this totally ignores any other DT user (U-Boot, FreeBSD, you > > name it). > > Which all have their own DT copies, with their own bindings, that we > (ie Linux) never agreed on. The FreeBSD guys are actually trying to align with the Linux DTs, which they periodically import (at least for arm64). From everything I've seen the U-Boot guys generally want to align with Linux, and have been attempting to some extent to do so (e.g. submitting bindings). So if anything, Linux DTs are being used by others who didn't have much say on them, rather than the other way around. Regardless, the existece of fragmentation is not a good argument for more fragmentation. Thanks, Mark. ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2016-02-16 21:11 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1454358000-13594-1-git-send-email-maxime.ripard@free-electrons.com>
2016-02-05 17:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Andre Przywara
2016-02-10 12:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Andre Przywara
2016-02-10 13:42 ` Rob Herring
2016-02-10 14:37 ` Maxime Ripard
2016-02-10 14:45 ` Arnd Bergmann
2016-02-10 16:14 ` breaking DT compatibility Andre Przywara
2016-02-11 10:16 ` Maxime Ripard
2016-02-10 16:30 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Mark Rutland
2016-02-11 10:00 ` Maxime Ripard
2016-02-11 11:44 ` Mark Rutland
2016-02-11 12:29 ` breaking DT compatibility Andre Przywara
2016-02-11 17:08 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Maxime Ripard
2016-02-12 9:40 ` Lucas Stach
2016-02-16 8:44 ` Maxime Ripard
2016-02-16 19:40 ` Michael Turquette
2016-02-16 21:11 ` Rob Herring
2016-02-11 14:51 ` Richard Cochran
2016-02-11 15:16 ` breaking DT compatibility Andre Przywara
2016-02-11 21:46 ` breaking DT compatibility (was: Re: [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused) Rob Herring
2016-02-10 12:59 ` [PATCH v4] clk: sunxi: Refactor A31 PLL6 so that it can be reused Maxime Ripard
2016-02-10 14:02 ` Rob Herring
2016-02-11 9:41 ` Maxime Ripard
2016-02-10 18:41 ` Mark Rutland
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).