| Message ID | 1330692928-30330-3-git-send-email-deepak.sikri@st.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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@st.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/7/2012 12:49 PM, Giuseppe CAVALLARO wrote: > 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. Sure, I will do the need full. >>>> +/* 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 >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 + +/* 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 */ + /* Platfrom data for platform device structure's platform_data field */ struct stmmac_mdio_bus_data {
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@st.com> --- include/linux/stmmac.h | 23 +++++++++++++++++++++++ 1 files changed, 23 insertions(+), 0 deletions(-)