linux-kernel - Re: [PATCH v2 3/4] clk: stm32: Add clock driver for STM32F4[23]xxx devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 30 May 2015 11:15:58 +0200
From:	Maxime Coquelin <mcoquelin.stm32@...il.com>
To:	Daniel Thompson <daniel.thompson@...aro.org>,
	Mike Turquette <mturquette@...aro.org>,
	Stephen Boyd <sboyd@...eaurora.org>
CC:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Russell King <linux@....linux.org.uk>,
	Kamil Lulko <rev13@...pl>, Andreas Farber <afaerber@...e.de>,
	linux-clk@...r.kernel.org, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	patches@...aro.org, linaro-kernel@...ts.linaro.org
Subject: Re: [PATCH v2 3/4] clk: stm32: Add clock driver for STM32F4[23]xxx
 devices

Hi Daniel,

     Nice driver, please find my comments below.

     Once fixed, you can add:
Reviewed-by: Maxime Coquelin <mcoquelin.stm32@...il.com>


On 05/30/2015 09:54 AM, Daniel Thompson wrote:
> The driver supports decoding and statically modelling PLL state (i.e.
> we inherit state from bootloader) and provides support for all
> peripherals that support simple one-bit gated clocks. The covers all
> peripherals whose clocks come from the AHB, APB1 or APB2 buses.
>
> It has been tested (for non-regression only) on an STM32F429I-Discovery
> boards. The clock counts for TIM2, USART1 and SYSTICK are all set correctly
> and time and the wall clock looks OK when checked with a stopwatch.
>
> Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
> ---
>   drivers/clk/Makefile      |   1 +
>   drivers/clk/clk-stm32f4.c | 364 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 365 insertions(+)
>   create mode 100644 drivers/clk/clk-stm32f4.c
...
> diff --git a/drivers/clk/clk-stm32f4.c b/drivers/clk/clk-stm32f4.c
> new file mode 100644
> index 0000000..68f9962
> --- /dev/null
> +++ b/drivers/clk/clk-stm32f4.c
> @@ -0,0 +1,364 @@
...
> +/*
> + * Converts the primary and secondary indices (as they appear in DT) to an
> + * offset into our struct clock array.
> + */
> +static unsigned int stm32f4_rcc_lookup_clk_idx(u8 primary, u8 secondary)
> +{
> +	u64 table[ARRAY_SIZE(stm32f42xx_gate_map)];
> +
> +	if (primary == 1) {
> +		BUG_ON(secondary > FCLK);
Maybe the function could return a signed int, an propagate errors 
instead of using BUG_ON?
> +		return secondary;
> +	}
> +
> +	memcpy(table, stm32f42xx_gate_map, sizeof(table));
> +
> +	/* only bits set in table can be used as indices */
> +	BUG_ON(secondary > 8 * sizeof(table) ||
> +	       0 == (table[BIT_ULL_WORD(secondary)] & BIT_ULL_MASK(secondary)));
Ditto.
> +
> +	/* mask out bits above our current index */
> +	table[BIT_ULL_WORD(secondary)] &=
> +	    GENMASK_ULL(secondary % BITS_PER_LONG_LONG, 0);
> +
> +	return FCLK + hweight64(table[0]) +
> +	       (BIT_ULL_WORD(secondary) >= 1 ? hweight64(table[1]) : 0) +
> +	       (BIT_ULL_WORD(secondary) >= 2 ? hweight64(table[2]) : 0);
> +}
> +
> +struct clk *stm32f4_rcc_lookup_clk(struct of_phandle_args *clkspec, void *data)
> +{
> +	return clks[stm32f4_rcc_lookup_clk_idx(clkspec->args[0],
> +					       clkspec->args[1])];
If stm32f4_rcc_lookup_clk_idx() returns an error, you could propagate it 
using ERR_PTR().
> +}
> +
> +static const char __initdata *sys_parents[] =   { "hsi", NULL, "pll" };
> +
> +static struct clk_div_table ahb_div_table[] = {
Should be const.
> +	{ 0x0,   1 }, { 0x1,   1 }, { 0x2,   1 }, { 0x3,   1 },
> +	{ 0x4,   1 }, { 0x5,   1 }, { 0x6,   1 }, { 0x7,   1 },
> +	{ 0x8,   2 }, { 0x9,   4 }, { 0xa,   8 }, { 0xb,  16 },
> +	{ 0xc,  64 }, { 0xd, 128 }, { 0xe, 256 }, { 0xf, 512 },
> +	{ 0 },
> +};
> +
> +static struct clk_div_table apb_div_table[] = {
Ditto.
> +	{ 0,  1 }, { 0,  1 }, { 0,  1 }, { 0,  1 },
> +	{ 4,  2 }, { 5,  4 }, { 6,  8 }, { 7, 16 },
> +	{ 0 },
> +};
>

Thanks!
Maxime
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/