From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752309AbdC0Ism (ORCPT ); Mon, 27 Mar 2017 04:48:42 -0400 Received: from mail.free-electrons.com ([62.4.15.54]:55565 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752113AbdC0IsC (ORCPT ); Mon, 27 Mar 2017 04:48:02 -0400 Date: Mon, 27 Mar 2017 09:54:38 +0200 From: Maxime Ripard To: Priit Laes Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-clk@vger.kernel.org, linux-sunxi@googlegroups.com, Icenowy Zheng , Russell King , Chen-Yu Tsai , Mark Rutland , Rob Herring , Stephen Boyd , Michael Turquette , Philipp Zabel Subject: Re: [PATCH v2 1/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver Message-ID: <20170327075438.cw3d6s7zyeemenwr@lukather> References:

MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gsmomqsq6zdsfons" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --gsmomqsq6zdsfons Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, Thanks a lot for working on this. On Sun, Mar 26, 2017 at 08:20:16PM +0300, Priit Laes wrote: > Introduce a clock controller driver for sun4i A10 and sun7i A20 > series SoCs. >=20 > Signed-off-by: Priit Laes > --- > drivers/clk/sunxi-ng/Kconfig | 13 +- > drivers/clk/sunxi-ng/Makefile | 1 +- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.c | 1532 ++++++++++++++++++- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.h | 59 +- > include/dt-bindings/clock/sunxi-a10-a20-ccu.h | 208 ++- > include/dt-bindings/reset/sunxi-a10-a20-ccu.h | 66 +- I'm not too fond of those sunxi-. We're not doing that for any other driver, I don't really know why this has became a trend lately. You can call them ccu-sun4i-a10.h, and it will work just fine. > +/* Not documented on A10 */ > +static SUNXI_CCU_GATE(pll_periph_sata_clk, "pll-periph-sata", "pll-perip= h", > + 0x028, BIT(14), 0); The rate doesn't come from pll-periph directly, does it? > +#define SUN4I_AHB_REG 0x054 > +static struct ccu_mux cpu_clk =3D { > + .mux =3D { > + .shift =3D 16, > + .width =3D 2, > + .fixed_predivs =3D cpu_predivs, > + .n_predivs =3D ARRAY_SIZE(cpu_predivs), > + }, > + .common =3D { > + .reg =3D 0x054, Why did you define this one, even though you don't seem to be using it anywhere? > +static const char *const ahb_parents[] =3D { "axi", "pll-periph", > + "pll-periph-2x" }; > +static const struct ccu_mux_fixed_prediv ahb_predivs[] =3D { > + { .index =3D 2, .div =3D 2, }, > +}; This seems to be only true for the A20, and not the A10. Are you sure here? The pll-periph-2x seem to be only used in the MBUS clock in our current code. And then, using pll-periph-2x, and then dividing it by 2 just gives us pll-periph, which is also our previous parent :) > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_output_clk, "mmc0_output", "mmc0", > + 0x088, 8, 3, 0); > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_sample_clk, "mmc0_sample", "mmc0", > + 0x088, 20, 3, 0); The A10 doesn't have them. > +/* TODO: Check whether A10 actually supports osc32k as 4th parent? */ > +static const char *const ir_parents_sun4i[] =3D { "hosc", "pll-periph", > + "pll-ddr-other" }; What does the BSP say about this? > +/* Undocumented on A10 */ > +static SUNXI_CCU_MUX_WITH_GATE(spdif_clk, "spdif", audio_parents, > + 0x0c0, 16, 2, BIT(31), CLK_SET_RATE_PARENT); This doesn't seem to exist at all on the A10 > +/* > + * TODO: SATA clock also supports external clock as parent via BIT(24) > + * The external clock is probably an optional crystal or oscillator > + * that can be connected to the SATA-CLKM / SATA-CLKP pins. > + */ > +static SUNXI_CCU_GATE(sata_clk, "sata", "pll-periph-sata", > + 0x0c8, BIT(31), 0); The rate won't be good here either. This is supposed to be 100MHz. > +static const char *const csi_isp_parents[] =3D { "pll-video0", "pll-ve", > + "pll-ddr-other", "pll-sata" }; > + > +static SUNXI_CCU_M_WITH_MUX_GATE(csi_isp_clk, "csi-isp", > + csi_isp_parents, > + 0x120, 0, 4, 24, 2, BIT(31), 0); We've been calling it sclk in the other SoC iirc. Any particular reason to call it differently? > +static const char *const out_parents[] =3D { "hosc", "osc32k", "hosc" }; > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_a_clk, "out-a", out_parents, > + 0x1f0, 8, 5, 20, 2, 24, 2, BIT(31), 0); > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_b_clk, "out-b", out_parents, > + 0x1f4, 8, 5, 20, 2, 24, 2, BIT(31), 0); There's a fixed pre-divider on the first hosc of 750. > +static void init_clocks(void __iomem *reg) > +{ > + u32 val; > + > + /* Force the PLL-Audio-1x divider to 4 */ > + val =3D readl(reg + SUN4I_PLL_AUDIO_REG); > + val &=3D ~GENMASK(19, 16); > + writel(val | (3 << 16), reg + SUN4I_PLL_AUDIO_REG); > + > + /* Use PLL6 as parent for AHB */ > + val =3D readl(reg + SUN4I_AHB_REG); > + val &=3D ~GENMASK(7, 6); > + writel(val | (2 << 6), reg + SUN4I_AHB_REG); Keeping some kind of comment similar to what was in the DT would be great, otherwise we lose *why* we need to do so. > +} > + > +static void __init sun4i_a10_ccu_setup(struct device_node *node) > +{ > + void __iomem *reg; > + > + reg =3D of_io_request_and_map(node, 0, of_node_full_name(node)); > + if (IS_ERR(reg)) { > + pr_err("%s: Could not map the clock registers\n", > + of_node_full_name(node)); > + return; > + } > + > + init_clocks(reg); > + > + sunxi_ccu_probe(node, reg, &sun4i_a10_ccu_desc); Can't you move the request_and_map / probe in the common function? > +#ifndef _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > +#define _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > + > +#define CLK_HOSC 1 > +#define CLK_PLL_PERIPH_SATA 16 That one looks suspicious. I don't see why we would need the PLL, while we have a perfectly functional SATA clock below. Have you tried gating the bit31 of the register 0xc8 to see if it has any impact? Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --gsmomqsq6zdsfons Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJY2MU5AAoJEBx+YmzsjxAgqb0QAKiR1+1eCWBWSrAKS5Ze43k+ bA2sGN/avTZvFUQHks3zTekgDwtwj2qS7k14bbRabR53samBJdS+iUU0WOFFd+YK r1VxXvsTOhIQshCh6qfYqZq6pkMHLFoT+zM+pJyzHICYwOEkfQfJGGsW6Wf3N4qO Eac12Q1Ou4IXYZJnoTSu4zb8VTBcL6zIwsSg6TYWKCyFoFFVuZHNecWL6GUeqFq5 MmrRsSQ8DcdQcM346VxWROYGl442ceynHKzouBMUN84OixoBcQdoF7sLHVbeBox2 DP17h71gyZet8oun9vk/RQlDVnxPKONwCB4E9sDc8ctbxcLfs86aIF43v0o9WUZT z96+Iaou0U53Uvhf10PjxO0esTRLcbDdZGXu+H4j7gTG4MqMhbW3RDuNQAv212OY xWlpUuGhOq0ZaegTE8Ukq1Xf0bj9w7oHkMjvdXLTI8AzBZex48mAlJMQYKJXjTSa gt5kYJOIxjDTKG5JTfT4H+TAeonyt0XapHJtNjDkx7YC5JVC+VvvfWS5GD3uKU9k Xaa0T22KELYv3rFeWrGgKbcjo2D7qyVBFIioMUOvktH5hQZuMQYYTfLrq0UKoxSL BTXcNSPLiOdrU2wI8FsSkMpaiIAwWARF5PBkS4rHshpcp6+a5VUh9VzHHSQWrXiA Mn1xNwPSUHWHJY+2OZwa =aP+b -----END PGP SIGNATURE----- --gsmomqsq6zdsfons-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v2 1/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver Date: Mon, 27 Mar 2017 09:54:38 +0200 Message-ID: <20170327075438.cw3d6s7zyeemenwr@lukather> References:

Reply-To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="gsmomqsq6zdsfons" Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Priit Laes Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-clk-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org, Icenowy Zheng , Russell King , Chen-Yu Tsai , Mark Rutland , Rob Herring , Stephen Boyd , Michael Turquette , Philipp Zabel List-Id: devicetree@vger.kernel.org --gsmomqsq6zdsfons Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline Hi, Thanks a lot for working on this. On Sun, Mar 26, 2017 at 08:20:16PM +0300, Priit Laes wrote: > Introduce a clock controller driver for sun4i A10 and sun7i A20 > series SoCs. > > Signed-off-by: Priit Laes > --- > drivers/clk/sunxi-ng/Kconfig | 13 +- > drivers/clk/sunxi-ng/Makefile | 1 +- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.c | 1532 ++++++++++++++++++- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.h | 59 +- > include/dt-bindings/clock/sunxi-a10-a20-ccu.h | 208 ++- > include/dt-bindings/reset/sunxi-a10-a20-ccu.h | 66 +- I'm not too fond of those sunxi-. We're not doing that for any other driver, I don't really know why this has became a trend lately. You can call them ccu-sun4i-a10.h, and it will work just fine. > +/* Not documented on A10 */ > +static SUNXI_CCU_GATE(pll_periph_sata_clk, "pll-periph-sata", "pll-periph", > + 0x028, BIT(14), 0); The rate doesn't come from pll-periph directly, does it? > +#define SUN4I_AHB_REG 0x054 > +static struct ccu_mux cpu_clk = { > + .mux = { > + .shift = 16, > + .width = 2, > + .fixed_predivs = cpu_predivs, > + .n_predivs = ARRAY_SIZE(cpu_predivs), > + }, > + .common = { > + .reg = 0x054, Why did you define this one, even though you don't seem to be using it anywhere? > +static const char *const ahb_parents[] = { "axi", "pll-periph", > + "pll-periph-2x" }; > +static const struct ccu_mux_fixed_prediv ahb_predivs[] = { > + { .index = 2, .div = 2, }, > +}; This seems to be only true for the A20, and not the A10. Are you sure here? The pll-periph-2x seem to be only used in the MBUS clock in our current code. And then, using pll-periph-2x, and then dividing it by 2 just gives us pll-periph, which is also our previous parent :) > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_output_clk, "mmc0_output", "mmc0", > + 0x088, 8, 3, 0); > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_sample_clk, "mmc0_sample", "mmc0", > + 0x088, 20, 3, 0); The A10 doesn't have them. > +/* TODO: Check whether A10 actually supports osc32k as 4th parent? */ > +static const char *const ir_parents_sun4i[] = { "hosc", "pll-periph", > + "pll-ddr-other" }; What does the BSP say about this? > +/* Undocumented on A10 */ > +static SUNXI_CCU_MUX_WITH_GATE(spdif_clk, "spdif", audio_parents, > + 0x0c0, 16, 2, BIT(31), CLK_SET_RATE_PARENT); This doesn't seem to exist at all on the A10 > +/* > + * TODO: SATA clock also supports external clock as parent via BIT(24) > + * The external clock is probably an optional crystal or oscillator > + * that can be connected to the SATA-CLKM / SATA-CLKP pins. > + */ > +static SUNXI_CCU_GATE(sata_clk, "sata", "pll-periph-sata", > + 0x0c8, BIT(31), 0); The rate won't be good here either. This is supposed to be 100MHz. > +static const char *const csi_isp_parents[] = { "pll-video0", "pll-ve", > + "pll-ddr-other", "pll-sata" }; > + > +static SUNXI_CCU_M_WITH_MUX_GATE(csi_isp_clk, "csi-isp", > + csi_isp_parents, > + 0x120, 0, 4, 24, 2, BIT(31), 0); We've been calling it sclk in the other SoC iirc. Any particular reason to call it differently? > +static const char *const out_parents[] = { "hosc", "osc32k", "hosc" }; > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_a_clk, "out-a", out_parents, > + 0x1f0, 8, 5, 20, 2, 24, 2, BIT(31), 0); > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_b_clk, "out-b", out_parents, > + 0x1f4, 8, 5, 20, 2, 24, 2, BIT(31), 0); There's a fixed pre-divider on the first hosc of 750. > +static void init_clocks(void __iomem *reg) > +{ > + u32 val; > + > + /* Force the PLL-Audio-1x divider to 4 */ > + val = readl(reg + SUN4I_PLL_AUDIO_REG); > + val &= ~GENMASK(19, 16); > + writel(val | (3 << 16), reg + SUN4I_PLL_AUDIO_REG); > + > + /* Use PLL6 as parent for AHB */ > + val = readl(reg + SUN4I_AHB_REG); > + val &= ~GENMASK(7, 6); > + writel(val | (2 << 6), reg + SUN4I_AHB_REG); Keeping some kind of comment similar to what was in the DT would be great, otherwise we lose *why* we need to do so. > +} > + > +static void __init sun4i_a10_ccu_setup(struct device_node *node) > +{ > + void __iomem *reg; > + > + reg = of_io_request_and_map(node, 0, of_node_full_name(node)); > + if (IS_ERR(reg)) { > + pr_err("%s: Could not map the clock registers\n", > + of_node_full_name(node)); > + return; > + } > + > + init_clocks(reg); > + > + sunxi_ccu_probe(node, reg, &sun4i_a10_ccu_desc); Can't you move the request_and_map / probe in the common function? > +#ifndef _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > +#define _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > + > +#define CLK_HOSC 1 > +#define CLK_PLL_PERIPH_SATA 16 That one looks suspicious. I don't see why we would need the PLL, while we have a perfectly functional SATA clock below. Have you tried gating the bit31 of the register 0xc8 to see if it has any impact? Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --gsmomqsq6zdsfons-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Mon, 27 Mar 2017 09:54:38 +0200 Subject: [PATCH v2 1/6] clk: sunxi-ng: Add sun4i/sun7i CCU driver In-Reply-To: References:

Message-ID: <20170327075438.cw3d6s7zyeemenwr@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi, Thanks a lot for working on this. On Sun, Mar 26, 2017 at 08:20:16PM +0300, Priit Laes wrote: > Introduce a clock controller driver for sun4i A10 and sun7i A20 > series SoCs. > > Signed-off-by: Priit Laes > --- > drivers/clk/sunxi-ng/Kconfig | 13 +- > drivers/clk/sunxi-ng/Makefile | 1 +- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.c | 1532 ++++++++++++++++++- > drivers/clk/sunxi-ng/ccu-sunxi-a10-a20.h | 59 +- > include/dt-bindings/clock/sunxi-a10-a20-ccu.h | 208 ++- > include/dt-bindings/reset/sunxi-a10-a20-ccu.h | 66 +- I'm not too fond of those sunxi-. We're not doing that for any other driver, I don't really know why this has became a trend lately. You can call them ccu-sun4i-a10.h, and it will work just fine. > +/* Not documented on A10 */ > +static SUNXI_CCU_GATE(pll_periph_sata_clk, "pll-periph-sata", "pll-periph", > + 0x028, BIT(14), 0); The rate doesn't come from pll-periph directly, does it? > +#define SUN4I_AHB_REG 0x054 > +static struct ccu_mux cpu_clk = { > + .mux = { > + .shift = 16, > + .width = 2, > + .fixed_predivs = cpu_predivs, > + .n_predivs = ARRAY_SIZE(cpu_predivs), > + }, > + .common = { > + .reg = 0x054, Why did you define this one, even though you don't seem to be using it anywhere? > +static const char *const ahb_parents[] = { "axi", "pll-periph", > + "pll-periph-2x" }; > +static const struct ccu_mux_fixed_prediv ahb_predivs[] = { > + { .index = 2, .div = 2, }, > +}; This seems to be only true for the A20, and not the A10. Are you sure here? The pll-periph-2x seem to be only used in the MBUS clock in our current code. And then, using pll-periph-2x, and then dividing it by 2 just gives us pll-periph, which is also our previous parent :) > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_output_clk, "mmc0_output", "mmc0", > + 0x088, 8, 3, 0); > +/* Undocumented on A10 */ > +static SUNXI_CCU_PHASE(mmc0_sample_clk, "mmc0_sample", "mmc0", > + 0x088, 20, 3, 0); The A10 doesn't have them. > +/* TODO: Check whether A10 actually supports osc32k as 4th parent? */ > +static const char *const ir_parents_sun4i[] = { "hosc", "pll-periph", > + "pll-ddr-other" }; What does the BSP say about this? > +/* Undocumented on A10 */ > +static SUNXI_CCU_MUX_WITH_GATE(spdif_clk, "spdif", audio_parents, > + 0x0c0, 16, 2, BIT(31), CLK_SET_RATE_PARENT); This doesn't seem to exist at all on the A10 > +/* > + * TODO: SATA clock also supports external clock as parent via BIT(24) > + * The external clock is probably an optional crystal or oscillator > + * that can be connected to the SATA-CLKM / SATA-CLKP pins. > + */ > +static SUNXI_CCU_GATE(sata_clk, "sata", "pll-periph-sata", > + 0x0c8, BIT(31), 0); The rate won't be good here either. This is supposed to be 100MHz. > +static const char *const csi_isp_parents[] = { "pll-video0", "pll-ve", > + "pll-ddr-other", "pll-sata" }; > + > +static SUNXI_CCU_M_WITH_MUX_GATE(csi_isp_clk, "csi-isp", > + csi_isp_parents, > + 0x120, 0, 4, 24, 2, BIT(31), 0); We've been calling it sclk in the other SoC iirc. Any particular reason to call it differently? > +static const char *const out_parents[] = { "hosc", "osc32k", "hosc" }; > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_a_clk, "out-a", out_parents, > + 0x1f0, 8, 5, 20, 2, 24, 2, BIT(31), 0); > +static SUNXI_CCU_MP_WITH_MUX_GATE(out_b_clk, "out-b", out_parents, > + 0x1f4, 8, 5, 20, 2, 24, 2, BIT(31), 0); There's a fixed pre-divider on the first hosc of 750. > +static void init_clocks(void __iomem *reg) > +{ > + u32 val; > + > + /* Force the PLL-Audio-1x divider to 4 */ > + val = readl(reg + SUN4I_PLL_AUDIO_REG); > + val &= ~GENMASK(19, 16); > + writel(val | (3 << 16), reg + SUN4I_PLL_AUDIO_REG); > + > + /* Use PLL6 as parent for AHB */ > + val = readl(reg + SUN4I_AHB_REG); > + val &= ~GENMASK(7, 6); > + writel(val | (2 << 6), reg + SUN4I_AHB_REG); Keeping some kind of comment similar to what was in the DT would be great, otherwise we lose *why* we need to do so. > +} > + > +static void __init sun4i_a10_ccu_setup(struct device_node *node) > +{ > + void __iomem *reg; > + > + reg = of_io_request_and_map(node, 0, of_node_full_name(node)); > + if (IS_ERR(reg)) { > + pr_err("%s: Could not map the clock registers\n", > + of_node_full_name(node)); > + return; > + } > + > + init_clocks(reg); > + > + sunxi_ccu_probe(node, reg, &sun4i_a10_ccu_desc); Can't you move the request_and_map / probe in the common function? > +#ifndef _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > +#define _DT_BINDINGS_CLK_SUNXI_A10_A20_H_ > + > +#define CLK_HOSC 1 > +#define CLK_PLL_PERIPH_SATA 16 That one looks suspicious. I don't see why we would need the PLL, while we have a perfectly functional SATA clock below. Have you tried gating the bit31 of the register 0xc8 to see if it has any impact? Thanks! Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 801 bytes Desc: not available URL: