Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752099AbbEFKSR (ORCPT ); Wed, 6 May 2015 06:18:17 -0400 Received: from mail-wi0-f177.google.com ([209.85.212.177]:38232 "EHLO mail-wi0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751101AbbEFKSP (ORCPT ); Wed, 6 May 2015 06:18:15 -0400 Message-ID: <5549EA63.5060602@gmail.com> Date: Wed, 06 May 2015 12:18:11 +0200 From: Jens Kuske User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Chen-Yu Tsai CC: Maxime Ripard , =?UTF-8?B?RW1pbGlvIA==?= =?UTF-8?B?TMOzcGV6?= , Mike Turquette , Linus Walleij , Vinod Koul , Rob Herring , devicetree , linux-arm-kernel , linux-kernel , linux-sunxi Subject: Re: [PATCH 2/6] clk: sunxi: Add H3 clocks support References: <1430904693-1404-1-git-send-email-jenskuske@gmail.com> <1430904693-1404-3-git-send-email-jenskuske@gmail.com> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8613 Lines: 204 On 06/05/15 11:47, Chen-Yu Tsai wrote: > Hi, > > On Wed, May 6, 2015 at 5:31 PM, Jens Kuske wrote: >> The H3 clock control unit is similar to the those of other sun8i family >> members like the A23. >> >> The AHB1 gates got split up into AHB1 and AHB2, with AHB2 clock source >> being muxable between AHB1 and PLL6/2, but still sharing gate registers. >> The documentation isn't totally clear about which devices belong to >> AHB2 now, especially USB EHIC/OHIC, so it is mostly based on Allwinner >> kernel source code. >> >> Signed-off-by: Jens Kuske >> --- >> Documentation/devicetree/bindings/clock/sunxi.txt | 7 ++++ >> drivers/clk/sunxi/clk-sunxi.c | 46 ++++++++++++++++++++++- >> 2 files changed, 52 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt >> index 4fa11af..4eeb893 100644 >> --- a/Documentation/devicetree/bindings/clock/sunxi.txt >> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt >> @@ -14,6 +14,8 @@ Required properties: >> "allwinner,sun4i-a10-pll5-clk" - for the PLL5 clock >> "allwinner,sun4i-a10-pll6-clk" - for the PLL6 clock >> "allwinner,sun6i-a31-pll6-clk" - for the PLL6 clock on A31 >> + "allwinner,sun8i-h3-pll6-clk" - for the PLL6 clock on H3 >> + "allwinner,sun8i-h3-pll8-clk" - for the PLL8 clock on H3 >> "allwinner,sun9i-a80-gt-clk" - for the GT bus clock on A80 >> "allwinner,sun4i-a10-cpu-clk" - for the CPU multiplexer clock >> "allwinner,sun4i-a10-axi-clk" - for the AXI clock >> @@ -28,8 +30,11 @@ Required properties: >> "allwinner,sun7i-a20-ahb-gates-clk" - for the AHB gates on A20 >> "allwinner,sun6i-a31-ar100-clk" - for the AR100 on A31 >> "allwinner,sun6i-a31-ahb1-clk" - for the AHB1 clock on A31 >> + "allwinner,sun8i-h3-ahb2-clk" - for the AHB2 clock on H3 >> "allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31 >> "allwinner,sun8i-a23-ahb1-gates-clk" - for the AHB1 gates on A23 >> + "allwinner,sun8i-h3-ahb1-gates-clk" - for the AHB1 gates on H3 >> + "allwinner,sun8i-h3-ahb2-gates-clk" - for the AHB2 gates on H3 >> "allwinner,sun9i-a80-ahb0-gates-clk" - for the AHB0 gates on A80 >> "allwinner,sun9i-a80-ahb1-gates-clk" - for the AHB1 gates on A80 >> "allwinner,sun9i-a80-ahb2-gates-clk" - for the AHB2 gates on A80 >> @@ -52,8 +57,10 @@ Required properties: >> "allwinner,sun6i-a31-apb1-gates-clk" - for the APB1 gates on A31 >> "allwinner,sun7i-a20-apb1-gates-clk" - for the APB1 gates on A20 >> "allwinner,sun8i-a23-apb1-gates-clk" - for the APB1 gates on A23 >> + "allwinner,sun8i-h3-apb1-gates-clk" - for the APB1 gates on H3 >> "allwinner,sun9i-a80-apb1-gates-clk" - for the APB1 gates on A80 >> "allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31 >> + "allwinner,sun8i-h3-apb2-gates-clk" - for the APB2 gates on H3 >> "allwinner,sun8i-a23-apb2-gates-clk" - for the APB2 gates on A23 >> "allwinner,sun5i-a13-mbus-clk" - for the MBUS clock on A13 >> "allwinner,sun4i-a10-mmc-clk" - for the MMC clock >> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c >> index 7e1e2bd..152a1f7 100644 >> --- a/drivers/clk/sunxi/clk-sunxi.c >> +++ b/drivers/clk/sunxi/clk-sunxi.c >> @@ -727,6 +727,12 @@ static const struct factors_data sun5i_a13_ahb_data __initconst = { >> .getter = sun5i_a13_get_ahb_factors, >> }; >> >> +static const struct factors_data sun8i_h3_pll8_data __initconst = { >> + .enable = 31, >> + .table = &sun6i_a31_pll6_config, >> + .getter = sun6i_a31_get_pll6_factors, >> +}; >> + > > If it's fully compatible with sun6i-a31-pll6, please just use it. > > On second thought, maybe it's not working because of the .name field? > If so, you're missing one here. It complained about the .name field. But I notice now, the name was correct, it gives us a pll8 running twice as fast as it should. pll8 doesn't have the x2 output. So, self-NACK > >> static const struct factors_data sun4i_apb1_data __initconst = { >> .mux = 24, >> .muxmask = BIT(1) | BIT(0), >> @@ -777,6 +783,10 @@ static const struct mux_data sun6i_a31_ahb1_mux_data __initconst = { >> .shift = 12, >> }; >> >> +static const struct mux_data sun8i_h3_ahb2_mux_data __initconst = { >> + .shift = 0, >> +}; >> + >> static void __init sunxi_mux_clk_setup(struct device_node *node, >> struct mux_data *data) >> { >> @@ -892,7 +902,7 @@ static void __init sunxi_divider_clk_setup(struct device_node *node, >> * sunxi_gates_clk_setup() - Setup function for leaf gates on clocks >> */ >> >> -#define SUNXI_GATES_MAX_SIZE 64 >> +#define SUNXI_GATES_MAX_SIZE 160 >> >> struct gates_data { >> DECLARE_BITMAP(mask, SUNXI_GATES_MAX_SIZE); >> @@ -926,6 +936,10 @@ static const struct gates_data sun8i_a23_ahb1_gates_data __initconst = { >> .mask = {0x25386742, 0x2505111}, >> }; >> >> +static const struct gates_data sun8i_h3_ahb1_gates_data __initconst = { >> + .mask = {0x1fbc6760, 0x00701b39, 0x00000000, 0x00000000, 0x00000081}, >> +}; >> + > > Wow, what's with the hardware design... :| Yeah, I don't like that too, but Allwinner's kernel source says EPHY is on AHB1. If anyone has a better idea how to implement this I'm happy to hear it. Maybe we should simply add a ahb1_gates2 for the new register. > >> static const struct gates_data sun9i_a80_ahb0_gates_data __initconst = { >> .mask = {0xF5F12B}, >> }; >> @@ -938,6 +952,10 @@ static const struct gates_data sun9i_a80_ahb2_gates_data __initconst = { >> .mask = {0x9B7}, >> }; >> >> +static const struct gates_data sun8i_h3_ahb2_gates_data __initconst = { >> + .mask = {0xe0020000}, >> +}; >> + >> static const struct gates_data sun4i_apb0_gates_data __initconst = { >> .mask = {0x4EF}, >> }; >> @@ -978,6 +996,10 @@ static const struct gates_data sun8i_a23_apb1_gates_data __initconst = { >> .mask = {0x3021}, >> }; >> >> +static const struct gates_data sun8i_h3_apb1_gates_data __initconst = { >> + .mask = {0x7123}, >> +}; >> + >> static const struct gates_data sun6i_a31_apb2_gates_data __initconst = { >> .mask = {0x3F000F}, >> }; >> @@ -994,6 +1016,10 @@ static const struct gates_data sun8i_a23_apb2_gates_data __initconst = { >> .mask = {0x1F0007}, >> }; >> >> +static const struct gates_data sun8i_h3_apb2_gates_data __initconst = { >> + .mask = {0x1F0007}, >> +}; >> + >> static void __init sunxi_gates_clk_setup(struct device_node *node, >> struct gates_data *data) >> { >> @@ -1106,6 +1132,16 @@ static const struct divs_data sun6i_a31_pll6_divs_data __initconst = { >> } >> }; >> >> +static const struct divs_data sun8i_h3_pll6_divs_data __initconst = { >> + .factors = &sun6i_a31_pll6_data, >> + .ndivs = 3, >> + .div = { >> + { .fixed = 2 }, /* normal output, pll6 */ >> + { .self = 1 }, /* base factor clock, pll6 x2 */ >> + { .fixed = 4 }, /* divided output, pll6 /2 */ > > Since you have the luxury of starting a new binding, maybe you could > put the ".self" clock first? I wanted to keep it compatible to the older SoCs, and it felt logical to have the normal output at first position. > >> + } >> +}; >> + >> /** >> * sunxi_divs_clk_setup() - Setup function for leaf divisors on clocks >> * >> @@ -1252,6 +1288,7 @@ static const struct of_device_id clk_factors_match[] __initconst = { >> {.compatible = "allwinner,sun5i-a13-ahb-clk", .data = &sun5i_a13_ahb_data,}, >> {.compatible = "allwinner,sun4i-a10-apb1-clk", .data = &sun4i_apb1_data,}, >> {.compatible = "allwinner,sun7i-a20-out-clk", .data = &sun7i_a20_out_data,}, >> + {.compatible = "allwinner,sun8i-h3-pll8-clk", .data = &sun8i_h3_pll8_data,}, > > Matching comment above, no need to add a new compatible for something that's > the same. PLL8 doesn't have a x2 output according to User Manual, but I did it wrong anyway, see first comment. This will need some more work I think. Jens -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/