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

[deepaksi <deepak.sikri@st.com>]

Hello Peppe,

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

I agree that these macros have been missed. But lets take the change 
suggested as a separate patch,
as this would then be integrated along with the driver.
The driver by default is considering the 2.5MHz MDIO clock option only. 
In this case we require an extra
variable to differentiate specification which is higher than IEEE spec 
of 2.5MHz.

>> +/* 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 */
> ...

Ok

>
> Also, macros CSR_F_20M should be totally removed.

ok

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