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]
Message-ID: <52127CC1.7030005@elopez.com.ar>
Date:	Mon, 19 Aug 2013 17:14:57 -0300
From:	Emilio López <emilio@...pez.com.ar>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>
CC:	Mike Turquette <mturquette@...aro.org>, kevin.z.m.zh@...il.com,
	sunny@...winnertech.com, shuge@...winnertech.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3 3/4] clk: sunxi: Add A31 clocks support

Hi Maxime,

El 05/08/13 17:43, Maxime Ripard escribió:
> The A31 has a mostly different clock set compared to the other older
> SoCs currently supported in the Allwinner clock driver.
>
> Add support for the basic useful clocks. The other ones will come in
> eventually.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@...e-electrons.com>

I had another quick look at it and overall it looks good to go,

Reviewed-by: Emilio López <emilio@...pez.com.ar>

> ---
>   Documentation/devicetree/bindings/clock/sunxi.txt  |   6 +
>   .../bindings/clock/sunxi/sun6i-a31-gates.txt       |  83 ++++++++++++++
>   drivers/clk/sunxi/clk-sunxi.c                      | 124 +++++++++++++++++++++
>   3 files changed, 213 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/clock/sunxi/sun6i-a31-gates.txt
>
> diff --git a/Documentation/devicetree/bindings/clock/sunxi.txt b/Documentation/devicetree/bindings/clock/sunxi.txt
> index b24de10..c383d12 100644
> --- a/Documentation/devicetree/bindings/clock/sunxi.txt
> +++ b/Documentation/devicetree/bindings/clock/sunxi.txt
> @@ -8,6 +8,7 @@ Required properties:
>   - compatible : shall be one of the following:
>   	"allwinner,sun4i-osc-clk" - for a gatable oscillator
>   	"allwinner,sun4i-pll1-clk" - for the main PLL clock
> +	"allwinner,sun6i-a31-pll1-clk" - for the main PLL clock on A31
>   	"allwinner,sun4i-cpu-clk" - for the CPU multiplexer clock
>   	"allwinner,sun4i-axi-clk" - for the AXI clock
>   	"allwinner,sun4i-axi-gates-clk" - for the AXI gates
> @@ -15,6 +16,8 @@ Required properties:
>   	"allwinner,sun4i-ahb-gates-clk" - for the AHB gates on A10
>   	"allwinner,sun5i-a13-ahb-gates-clk" - for the AHB gates on A13
>   	"allwinner,sun5i-a10s-ahb-gates-clk" - for the AHB gates on A10s
> +	"allwinner,sun6i-a31-ahb1-mux-clk" - for the AHB1 multiplexer on A31
> +	"allwinner,sun6i-a31-ahb1-gates-clk" - for the AHB1 gates on A31
>   	"allwinner,sun4i-apb0-clk" - for the APB0 clock
>   	"allwinner,sun4i-apb0-gates-clk" - for the APB0 gates on A10
>   	"allwinner,sun5i-a13-apb0-gates-clk" - for the APB0 gates on A13
> @@ -24,6 +27,9 @@ Required properties:
>   	"allwinner,sun4i-apb1-gates-clk" - for the APB1 gates on A10
>   	"allwinner,sun5i-a13-apb1-gates-clk" - for the APB1 gates on A13
>   	"allwinner,sun5i-a10s-apb1-gates-clk" - for the APB1 gates on A10s
> +	"allwinner,sun6i-a31-apb1-gates-clk" - for the APB1 gates on A31
> +	"allwinner,sun6i-a31-apb2-div-clk" - for the APB2 gates on A31
> +	"allwinner,sun6i-a31-apb2-gates-clk" - for the APB2 gates on A31
>
>   Required properties for all clocks:
>   - reg : shall be the control register address for the clock.
> diff --git a/Documentation/devicetree/bindings/clock/sunxi/sun6i-a31-gates.txt b/Documentation/devicetree/bindings/clock/sunxi/sun6i-a31-gates.txt
> new file mode 100644
> index 0000000..fe44932
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sunxi/sun6i-a31-gates.txt
> @@ -0,0 +1,83 @@
> +Gate clock outputs
> +------------------
> +
> +  * AHB1 gates ("allwinner,sun6i-a31-ahb1-gates-clk")
> +
> +    MIPI DSI					1
> +
> +    SS						5
> +    DMA						6
> +
> +    MMC0					8
> +    MMC1					9
> +    MMC2					10
> +    MMC3					11
> +
> +    NAND1					12
> +    NAND0					13
> +    SDRAM					14
> +
> +    GMAC					17
> +    TS						18
> +    HSTIMER					19
> +    SPI0					20
> +    SPI1					21
> +    SPI2					22
> +    SPI3					23
> +    USB_OTG					24
> +
> +    EHCI0					26
> +    EHCI1					27
> +
> +    OHCI0					29
> +    OHCI1					30
> +    OHCI2					31
> +    VE						32
> +
> +    LCD0					36
> +    LCD1					37
> +
> +    CSI						40
> +
> +    HDMI					43
> +    DE_BE0					44
> +    DE_BE1					45
> +    DE_FE1					46
> +    DE_FE1					47
> +
> +    MP						50
> +
> +    GPU						52
> +
> +    DEU0					55
> +    DEU1					56
> +    DRC0					57
> +    DRC1					58
> +
> +  * APB1 gates ("allwinner,sun6i-a31-apb1-gates-clk")
> +
> +    CODEC					0
> +
> +    DIGITAL MIC					4
> +    PIO						5
> +
> +    DAUDIO0					12
> +    DAUDIO1					13
> +
> +  * APB2 gates ("allwinner,sun6i-a31-apb2-gates-clk")
> +
> +    I2C0					0
> +    I2C1					1
> +    I2C2					2
> +    I2C3					3
> +
> +    UART0					16
> +    UART1					17
> +    UART2					18
> +    UART3					19
> +    UART4					20
> +    UART5					21
> +
> +Notation:
> + [*]:  The datasheet didn't mention these, but they are present on AW code
> + [**]: The datasheet had this marked as "NC" but they are used on AW code

If you have to respin this, I suppose you could drop the notation block, 
as it's not being used. But don't worry otherwise.

> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
> index cd07169..bd01a02 100644
> --- a/drivers/clk/sunxi/clk-sunxi.c
> +++ b/drivers/clk/sunxi/clk-sunxi.c
> @@ -125,7 +125,89 @@ static void sun4i_get_pll1_factors(u32 *freq, u32 parent_rate,
>   	*n = div / 4;
>   }
>
> +/**
> + * sun6i_a31_get_pll1_factors() - calculates n, k and m factors for PLL1
> + * PLL1 rate is calculated as follows
> + * rate = parent_rate * (n + 1) * (k + 1) / (m + 1);
> + * parent_rate should always be 24MHz
> + */
> +static void sun6i_a31_get_pll1_factors(u32 *freq, u32 parent_rate,
> +				       u8 *n, u8 *k, u8 *m, u8 *p)
> +{
> +	/*
> +	 * We can operate only on MHz, this will make our life easier
> +	 * later.
> +	 */
> +	u32 freq_mhz = *freq / 1000000;
> +	u32 parent_freq_mhz = parent_rate / 1000000;
> +
> +	/*
> +	 * Round down the frequency to the closest multiple of either
> +	 * 6 or 16
> +	 */
> +	u32 round_freq_6 = round_down(freq_mhz, 6);
> +	u32 round_freq_16 = round_down(freq_mhz, 16);
> +
> +	if (round_freq_6 > round_freq_16)
> +		freq_mhz = round_freq_6;
> +	else
> +		freq_mhz = round_freq_16;
>
> +	*freq = freq_mhz * 1000000;
> +
> +	/*
> +	 * If the factors pointer are null, we were just called to
> +	 * round down the frequency.
> +	 * Exit.
> +	 */
> +	if (n == NULL)
> +		return;
> +
> +	/* If the frequency is a multiple of 32 MHz, k is always 3 */
> +	if (!(freq_mhz % 32))
> +		*k = 3;
> +	/* If the frequency is a multiple of 9 MHz, k is always 2 */
> +	else if (!(freq_mhz % 9))
> +		*k = 2;
> +	/* If the frequency is a multiple of 8 MHz, k is always 1 */
> +	else if (!(freq_mhz % 8))
> +		*k = 1;
> +	/* Otherwise, we don't use the k factor */
> +	else
> +		*k = 0;
> +
> +	/*
> +	 * If the frequency is a multiple of 2 but not a multiple of
> +	 * 3, m is 3. This is the first time we use 6 here, yet we
> +	 * will use it on several other places.
> +	 * We use this number because it's the lowest frequency we can
> +	 * generate (with n = 0, k = 0, m = 3), so every other frequency
> +	 * somehow relates to this frequency.
> +	 */
> +	if ((freq_mhz % 6) == 2 || (freq_mhz % 6) == 4)
> +		*m = 2;
> +	/*
> +	 * If the frequency is a multiple of 6MHz, but the factor is
> +	 * odd, m will be 3
> +	 */
> +	else if ((freq_mhz / 6) & 1)
> +		*m = 3;
> +	/* Otherwise, we end up with m = 1 */
> +	else
> +		*m = 1;
> +
> +	/* Calculate n thanks to the above factors we already got */
> +	*n = freq_mhz * (*m + 1) / ((*k + 1) * parent_freq_mhz) - 1;
> +
> +	/*
> +	 * If n end up being outbound, and that we can still decrease
> +	 * m, do it.
> +	 */
> +	if ((*n + 1) > 31 && (*m + 1) > 1) {
> +		*n = (*n + 1) / 2 - 1;
> +		*m = (*m + 1) / 2 - 1;
> +	}
> +}
>

And again, I'm purely nitpicking, but it'd be good to keep the space 
between functions consistent.

>   /**
>    * sun4i_get_apb1_factors() - calculates m, p factors for APB1
> @@ -190,6 +272,15 @@ static struct clk_factors_config sun4i_pll1_config = {
>   	.pwidth = 2,
>   };
>
> +static struct clk_factors_config sun6i_a31_pll1_config = {
> +	.nshift	= 8,
> +	.nwidth = 5,
> +	.kshift = 4,
> +	.kwidth = 2,
> +	.mshift = 0,
> +	.mwidth = 2,
> +};
> +
>   static struct clk_factors_config sun4i_apb1_config = {
>   	.mshift = 0,
>   	.mwidth = 5,
> @@ -202,6 +293,11 @@ static const __initconst struct factors_data sun4i_pll1_data = {
>   	.getter = sun4i_get_pll1_factors,
>   };
>
> +static const __initconst struct factors_data sun6i_a31_pll1_data = {
> +	.table = &sun6i_a31_pll1_config,
> +	.getter = sun6i_a31_get_pll1_factors,
> +};
> +
>   static const __initconst struct factors_data sun4i_apb1_data = {
>   	.table = &sun4i_apb1_config,
>   	.getter = sun4i_get_apb1_factors,
> @@ -244,6 +340,10 @@ static const __initconst struct mux_data sun4i_cpu_mux_data = {
>   	.shift = 16,
>   };
>
> +static const __initconst struct mux_data sun6i_a31_ahb1_mux_data = {
> +	.shift = 12,
> +};
> +
>   static const __initconst struct mux_data sun4i_apb1_mux_data = {
>   	.shift = 24,
>   };
> @@ -302,6 +402,12 @@ static const __initconst struct div_data sun4i_apb0_data = {
>   	.width	= 2,
>   };
>
> +static const __initconst struct div_data sun6i_a31_apb2_div_data = {
> +	.shift	= 0,
> +	.pow	= 0,
> +	.width	= 4,
> +};
> +
>   static void __init sunxi_divider_clk_setup(struct device_node *node,
>   					   struct div_data *data)
>   {
> @@ -352,6 +458,10 @@ static const __initconst struct gates_data sun5i_a13_ahb_gates_data = {
>   	.mask = {0x107067e7, 0x185111},
>   };
>
> +static const __initconst struct gates_data sun6i_a31_ahb1_gates_data = {
> +	.mask = {0xEDFE7F62, 0x794F931},
> +};
> +
>   static const __initconst struct gates_data sun4i_apb0_gates_data = {
>   	.mask = {0x4EF},
>   };
> @@ -376,6 +486,14 @@ static const __initconst struct gates_data sun5i_a13_apb1_gates_data = {
>   	.mask = {0xa0007},
>   };
>
> +static const __initconst struct gates_data sun6i_a31_apb1_gates_data = {
> +	.mask = {0x3031},
> +};
> +
> +static const __initconst struct gates_data sun6i_a31_apb2_gates_data = {
> +	.mask = {0x3F000F},
> +};
> +
>   static void __init sunxi_gates_clk_setup(struct device_node *node,
>   					 struct gates_data *data)
>   {
> @@ -428,6 +546,7 @@ static void __init sunxi_gates_clk_setup(struct device_node *node,
>   /* Matches for factors clocks */
>   static const __initconst struct of_device_id clk_factors_match[] = {
>   	{.compatible = "allwinner,sun4i-pll1-clk", .data = &sun4i_pll1_data,},
> +	{.compatible = "allwinner,sun6i-a31-pll1-clk", .data = &sun6i_a31_pll1_data,},
>   	{.compatible = "allwinner,sun4i-apb1-clk", .data = &sun4i_apb1_data,},
>   	{}
>   };
> @@ -437,6 +556,7 @@ static const __initconst struct of_device_id clk_div_match[] = {
>   	{.compatible = "allwinner,sun4i-axi-clk", .data = &sun4i_axi_data,},
>   	{.compatible = "allwinner,sun4i-ahb-clk", .data = &sun4i_ahb_data,},
>   	{.compatible = "allwinner,sun4i-apb0-clk", .data = &sun4i_apb0_data,},
> +	{.compatible = "allwinner,sun6i-a31-apb2-div-clk", .data = &sun6i_a31_apb2_div_data,},
>   	{}
>   };
>
> @@ -444,6 +564,7 @@ static const __initconst struct of_device_id clk_div_match[] = {
>   static const __initconst struct of_device_id clk_mux_match[] = {
>   	{.compatible = "allwinner,sun4i-cpu-clk", .data = &sun4i_cpu_mux_data,},
>   	{.compatible = "allwinner,sun4i-apb1-mux-clk", .data = &sun4i_apb1_mux_data,},
> +	{.compatible = "allwinner,sun6i-a31-ahb1-mux-clk", .data = &sun6i_a31_ahb1_mux_data,},
>   	{}
>   };
>
> @@ -453,12 +574,15 @@ static const __initconst struct of_device_id clk_gates_match[] = {
>   	{.compatible = "allwinner,sun4i-ahb-gates-clk", .data = &sun4i_ahb_gates_data,},
>   	{.compatible = "allwinner,sun5i-a10s-ahb-gates-clk", .data = &sun5i_a10s_ahb_gates_data,},
>   	{.compatible = "allwinner,sun5i-a13-ahb-gates-clk", .data = &sun5i_a13_ahb_gates_data,},
> +	{.compatible = "allwinner,sun6i-a31-ahb1-gates-clk", .data = &sun6i_a31_ahb1_gates_data,},
>   	{.compatible = "allwinner,sun4i-apb0-gates-clk", .data = &sun4i_apb0_gates_data,},
>   	{.compatible = "allwinner,sun5i-a10s-apb0-gates-clk", .data = &sun5i_a10s_apb0_gates_data,},
>   	{.compatible = "allwinner,sun5i-a13-apb0-gates-clk", .data = &sun5i_a13_apb0_gates_data,},
>   	{.compatible = "allwinner,sun4i-apb1-gates-clk", .data = &sun4i_apb1_gates_data,},
>   	{.compatible = "allwinner,sun5i-a10s-apb1-gates-clk", .data = &sun5i_a10s_apb1_gates_data,},
>   	{.compatible = "allwinner,sun5i-a13-apb1-gates-clk", .data = &sun5i_a13_apb1_gates_data,},
> +	{.compatible = "allwinner,sun6i-a31-apb1-gates-clk", .data = &sun6i_a31_apb1_gates_data,},
> +	{.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,},
>   	{}
>   };
>
>

Thanks for working on this!

Emilio
--
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/

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ