netdev - Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

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: <4F54CF00.6030005@st.com>
Date:	Mon, 05 Mar 2012 15:34:40 +0100
From:	Giuseppe CAVALLARO <peppe.cavallaro@...com>
To:	Deepak SIKRI <deepak.sikri@...com>
Cc:	spear-devel@...t.st.com, netdev@...r.kernel.org
Subject: Re: [PATCH 2/6] stmmac: Define MDC clock selection macros.

Hello Deepak

On 3/2/2012 1:55 PM, Deepak SIKRI wrote:
> The patch adds the macros to be used for MDC clock selection. The MDC clock
> frequency is based on scaled system clock, and has to be confined to a range
> of 1-2.5 MHz. Based on the input CSR clock, the scaling factor has to be
> selected.
> The platform specific code will provide the default value of this scaling
> factor, based on the input CSR clock.
> 
> Signed-off-by: Deepak Sikri <deepak.sikri@...com>
> ---
>  include/linux/stmmac.h |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index aa0d99e..7332ed8 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -40,6 +40,29 @@
>  #define STMMAC_CSUM_T1	1
>  #define STMMAC_CSUM_T2	2
>  
> +/*
> + * Define the macros for CSR clock range parameters to be passed by
> + * platform code.
> + * This could also be configured at run time using CPU freq framework.
> + */
> +
> +/* CSR Frequency Access Defines*/
> +#define CSR_F_20M	20000000
> +#define CSR_F_35M	35000000
> +#define CSR_F_60M	60000000
> +#define CSR_F_100M	100000000
> +#define CSR_F_150M	150000000
> +#define CSR_F_250M	50000000
> +#define CSR_F_300M	300000000


I have some concerns about this patch.

We want to have some defines to help on setting the clk_csr (that is is
a clk divisor).

When you program the "CSR Clock Range" bits in the GMII Address Register
you can also set the bit 5 (not supported in older devices e.g. 3.41a).
In this case, the defines below do not cover all the cases, I mean:

1000 clk_csr_i/4
1001 clk_csr_i/6
1010 clk_csr_i/8
1011 clk_csr_i/10
1100 clk_csr_i/12
1101 clk_csr_i/14
1110 clk_csr_i/16
1111 clk_csr_i/18

> +/* MDC Clock Selection define*/
> +#define	STMMAC_CLK_RANGE_60_100M	0	/* MDC = Clk/42 */
> +#define	STMMAC_CLK_RANGE_100_150M	1	/* MDC = Clk/62 */
> +#define	STMMAC_CLK_RANGE_20_35M		2	/* MDC = Clk/16 */
> +#define	STMMAC_CLK_RANGE_35_60M		3	/* MDC = Clk/26 */
> +#define	STMMAC_CLK_RANGE_150_250M	4	/* MDC = Clk/102 */
> +#define	STMMAC_CLK_RANGE_250_300M	5	/* MDC = Clk/122 */

I suggest you to rename these macros as:

#define STMMAC_CSR_60_100M	0	/* MDC = Clk/42 */
...

Also, macros CSR_F_20M should be totally removed.

Peppe

> +
>  /* Platfrom data for platform device structure's platform_data field */
>  
>  struct stmmac_mdio_bus_data {

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html