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

[Giuseppe CAVALLARO <peppe.cavallaro@st.com>]

On 3/7/2012 7:55 AM, Deepak SIKRI wrote:
> 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.

ok, we will have them in a another patch. At any rate, I ask you to add
a FIXME in the code and detail this issue in the patch comment.

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