| Message ID | 1257830147.2411446.1344976418653.JavaMail.root@advansee.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On 14/08/2012 22:33, Benoît Thébaudeau wrote: > Each eSDHC instance has a dedicated clock. > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > Cc: Stefano Babic <sbabic@denx.de> > --- Hi Benoît, > .../arch/arm/cpu/arm1136/mx35/generic.c | 14 ++++++++++++-- > .../arch/arm/include/asm/arch-mx35/clock.h | 4 +++- > 2 files changed, 15 insertions(+), 3 deletions(-) > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c > index 4af052c..15a0098 100644 > --- u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c > +++ u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c > @@ -368,8 +368,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) > return get_ipg_per_clk(); > case MXC_UART_CLK: > return imx_get_uartclk(); > - case MXC_ESDHC_CLK: > + case MXC_ESDHC1_CLK: > return mxc_get_peri_clock(ESDHC1_CLK); > + case MXC_ESDHC2_CLK: > + return mxc_get_peri_clock(ESDHC2_CLK); > + case MXC_ESDHC3_CLK: > + return mxc_get_peri_clock(ESDHC3_CLK); > case MXC_USB_CLK: > return mxc_get_main_clock(USB_CLK); > case MXC_FEC_CLK: Your change let understand that we can have different clocks among the ESDHC controllers. One thing is not clear to me is that the MX35 have two ESDHC controllers, and you define here a thitd one. Where is it ? Even if the two controllers can have different clocks, this is not supported by the driver. In fact, in drivers/mmc/fsl_esdhc.c: int sdhc_clk = gd->sdhc_clk; The driver uses always the same clock, stored in the global structure. Before extending the code as in this patch, the driver should be modified to handle separate clocks. Currently the driver supports multiple controller, but they share the same clock or at least the same frequency. Best regards, Stefano
Hi Stefano, > On 14/08/2012 22:33, Benoît Thébaudeau wrote: > > Each eSDHC instance has a dedicated clock. > > > > Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> > > Cc: Stefano Babic <sbabic@denx.de> > > --- > > Hi Benoît, > > > .../arch/arm/cpu/arm1136/mx35/generic.c | 14 > > ++++++++++++-- > > .../arch/arm/include/asm/arch-mx35/clock.h | 4 +++- > > 2 files changed, 15 insertions(+), 3 deletions(-) > > > > diff --git u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c > > u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c > > index 4af052c..15a0098 100644 > > --- u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c > > +++ u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c > > @@ -368,8 +368,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) > > return get_ipg_per_clk(); > > case MXC_UART_CLK: > > return imx_get_uartclk(); > > - case MXC_ESDHC_CLK: > > + case MXC_ESDHC1_CLK: > > return mxc_get_peri_clock(ESDHC1_CLK); > > + case MXC_ESDHC2_CLK: > > + return mxc_get_peri_clock(ESDHC2_CLK); > > + case MXC_ESDHC3_CLK: > > + return mxc_get_peri_clock(ESDHC3_CLK); > > case MXC_USB_CLK: > > return mxc_get_main_clock(USB_CLK); > > case MXC_FEC_CLK: > > Your change let understand that we can have different clocks among > the > ESDHC controllers. Indeed. > One thing is not clear to me is that the MX35 have > two ESDHC controllers, and you define here a thitd one. Where is it ? No, there are 3. See the reference manual. It's referenced in the memory map, in the interrupts, in the pin multiplexing and in the clocks. > Even if the two controllers can have different clocks, this is not > supported by the driver. In fact, in drivers/mmc/fsl_esdhc.c: > > int sdhc_clk = gd->sdhc_clk; > > The driver uses always the same clock, stored in the global > structure. > Before extending the code as in this patch, the driver should be > modified to handle separate clocks. Currently the driver supports > multiple controller, but they share the same clock or at least the > same > frequency. Indeed, I had seen that. I didn't know what to decide as to the driver clocks, so I made this change to select the correct clock if a single clock or frequency is used. If several clock frequencies are to be supported at once, what kind of API would you like? gd->sdhc_clk could be changed to an array, then the corresponding index could be passed to the init function through the fsl_esdhc_cfg struct. Best regards, Benoît
Hi Stefano, > > Even if the two controllers can have different clocks, this is not > > supported by the driver. In fact, in drivers/mmc/fsl_esdhc.c: > > > > int sdhc_clk = gd->sdhc_clk; > > > > The driver uses always the same clock, stored in the global > > structure. > > Before extending the code as in this patch, the driver should be > > modified to handle separate clocks. Currently the driver supports > > multiple controller, but they share the same clock or at least the > > same > > frequency. > > Indeed, I had seen that. I didn't know what to decide as to the > driver clocks, > so I made this change to select the correct clock if a single clock > or frequency > is used. > > If several clock frequencies are to be supported at once, what kind > of API would > you like? gd->sdhc_clk could be changed to an array, then the > corresponding > index could be passed to the init function through the fsl_esdhc_cfg > struct. But there is also the issue of fsl_esdhc_mmc_init() that would need a new config just to pass this index. I don't like that. Any suggestion? Best regards, Benoît
On 20/08/2012 14:34, Benoît Thébaudeau wrote: > Hi Stefano, > Hi Benoît, >>> Even if the two controllers can have different clocks, this is not >>> supported by the driver. In fact, in drivers/mmc/fsl_esdhc.c: >>> >>> int sdhc_clk = gd->sdhc_clk; >>> >>> The driver uses always the same clock, stored in the global >>> structure. >>> Before extending the code as in this patch, the driver should be >>> modified to handle separate clocks. Currently the driver supports >>> multiple controller, but they share the same clock or at least the >>> same >>> frequency. >> >> Indeed, I had seen that. I didn't know what to decide as to the >> driver clocks, >> so I made this change to select the correct clock if a single clock >> or frequency >> is used. >> >> If several clock frequencies are to be supported at once, what kind >> of API would >> you like? gd->sdhc_clk could be changed to an array, then the >> corresponding >> index could be passed to the init function through the fsl_esdhc_cfg >> struct. > > But there is also the issue of fsl_esdhc_mmc_init() that would need a new config > just to pass this index. I don't like that. Any suggestion? There is another issue. The driver is used by both ARM (i.MX) and PowerPCs (PowerQuickIII, ...). fsl_esdhc_mmc_init() is already the interface when one parameter is enough. If we need more than one controller, we should already call fsl_esdhc_initialize() with the cfg structure. Then adding a field to the struct fsl_esdhc_cfg is maybe not too bad. Instead of an index, we can add directly the frequency - reading the driver this is all that the driver needs. Best regards, Stefano
On 20/08/2012 14:55, Benoît Thébaudeau wrote: > > OK, then this patch does the job for the single eSDHC instance use case, which > will still use gd->sdhc_clk. Yes, but then we do not need ESDHC1, ESDHC2 and ESDHC3, because the driver does not support different clocks. > I will make another patch before or after this one > for the multi-instance use case. Fine. > I will do the same in the v2 of my mx5 clock > series (for gd->sdhc_clk). I think I also have the same stuff for mx25. Ok - I read the series for MX5, but I need to check it with the manuals to understand the changes. I need some more time... Best regards, Stefano
Hi Stefano, > >>> Even if the two controllers can have different clocks, this is > >>> not > >>> supported by the driver. In fact, in drivers/mmc/fsl_esdhc.c: > >>> > >>> int sdhc_clk = gd->sdhc_clk; > >>> > >>> The driver uses always the same clock, stored in the global > >>> structure. > >>> Before extending the code as in this patch, the driver should be > >>> modified to handle separate clocks. Currently the driver supports > >>> multiple controller, but they share the same clock or at least > >>> the > >>> same > >>> frequency. > >> > >> Indeed, I had seen that. I didn't know what to decide as to the > >> driver clocks, > >> so I made this change to select the correct clock if a single > >> clock > >> or frequency > >> is used. > >> > >> If several clock frequencies are to be supported at once, what > >> kind > >> of API would > >> you like? gd->sdhc_clk could be changed to an array, then the > >> corresponding > >> index could be passed to the init function through the > >> fsl_esdhc_cfg > >> struct. > > > > But there is also the issue of fsl_esdhc_mmc_init() that would need > > a new config > > just to pass this index. I don't like that. Any suggestion? > > There is another issue. The driver is used by both ARM (i.MX) and > PowerPCs (PowerQuickIII, ...). > > fsl_esdhc_mmc_init() is already the interface when one parameter is > enough. If we need more than one controller, we should already call > fsl_esdhc_initialize() with the cfg structure. > > Then adding a field to the struct fsl_esdhc_cfg is maybe not too bad. > Instead of an index, we can add directly the frequency - reading the > driver this is all that the driver needs. OK, then this patch does the job for the single eSDHC instance use case, which will still use gd->sdhc_clk. I will make another patch before or after this one for the multi-instance use case. I will do the same in the v2 of my mx5 clock series (for gd->sdhc_clk). I think I also have the same stuff for mx25. Best regards, Benoît
Hi Stefano, > > > > OK, then this patch does the job for the single eSDHC instance use > > case, which > > will still use gd->sdhc_clk. > > Yes, but then we do not need ESDHC1, ESDHC2 and ESDHC3, because the > driver does not support different clocks. We need them: Yes, the driver supports a single eSDHC instance and a single clock with gd->sdhc_clk and fsl_esdhc_mmc_init(), but there is a choice to determine at compile time depending on CONFIG_SYS_FSL_ESDHC_ADDR. You don't want to use the clock of eSDHC1 if CONFIG_SYS_FSL_ESDHC_ADDR selects eSDHC2. This is use case exists on i.MX with cpu_mmc_init() if a single eSDHC instance is needed. > > I will make another patch before or after this one > > for the multi-instance use case. > > Fine. OK. > > I will do the same in the v2 of my mx5 clock > > series (for gd->sdhc_clk). I think I also have the same stuff for > > mx25. > > Ok - I read the series for MX5, but I need to check it with the > manuals > to understand the changes. I need some more time... No problem. Best regards, Benoît
diff --git u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c index 4af052c..15a0098 100644 --- u-boot-4d3c95f.orig/arch/arm/cpu/arm1136/mx35/generic.c +++ u-boot-4d3c95f/arch/arm/cpu/arm1136/mx35/generic.c @@ -368,8 +368,12 @@ unsigned int mxc_get_clock(enum mxc_clock clk) return get_ipg_per_clk(); case MXC_UART_CLK: return imx_get_uartclk(); - case MXC_ESDHC_CLK: + case MXC_ESDHC1_CLK: return mxc_get_peri_clock(ESDHC1_CLK); + case MXC_ESDHC2_CLK: + return mxc_get_peri_clock(ESDHC2_CLK); + case MXC_ESDHC3_CLK: + return mxc_get_peri_clock(ESDHC3_CLK); case MXC_USB_CLK: return mxc_get_main_clock(USB_CLK); case MXC_FEC_CLK: @@ -469,7 +473,13 @@ int cpu_eth_init(bd_t *bis) int get_clocks(void) { #ifdef CONFIG_FSL_ESDHC - gd->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK); +#if CONFIG_SYS_FSL_ESDHC_ADDR == MMC_SDHC1_BASE_ADDR + gd->sdhc_clk = mxc_get_clock(MXC_ESDHC1_CLK); +#elif CONFIG_SYS_FSL_ESDHC_ADDR == MMC_SDHC2_BASE_ADDR + gd->sdhc_clk = mxc_get_clock(MXC_ESDHC2_CLK); +#elif CONFIG_SYS_FSL_ESDHC_ADDR == MMC_SDHC3_BASE_ADDR + gd->sdhc_clk = mxc_get_clock(MXC_ESDHC3_CLK); +#endif #endif return 0; } diff --git u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx35/clock.h u-boot-4d3c95f/arch/arm/include/asm/arch-mx35/clock.h index e94f124..0575dad 100644 --- u-boot-4d3c95f.orig/arch/arm/include/asm/arch-mx35/clock.h +++ u-boot-4d3c95f/arch/arm/include/asm/arch-mx35/clock.h @@ -30,7 +30,9 @@ enum mxc_clock { MXC_IPG_CLK, MXC_IPG_PERCLK, MXC_UART_CLK, - MXC_ESDHC_CLK, + MXC_ESDHC1_CLK, + MXC_ESDHC2_CLK, + MXC_ESDHC3_CLK, MXC_USB_CLK, MXC_CSPI_CLK, MXC_FEC_CLK,
Each eSDHC instance has a dedicated clock. Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com> Cc: Stefano Babic <sbabic@denx.de> --- .../arch/arm/cpu/arm1136/mx35/generic.c | 14 ++++++++++++-- .../arch/arm/include/asm/arch-mx35/clock.h | 4 +++- 2 files changed, 15 insertions(+), 3 deletions(-)