[U-Boot] [PATCH V2] i.MX: fsl_esdhc: allow use with cache enabled.
Signed-off-by: Eric Nelson eric.nelson@boundarydevices.com --- V2 fixes checkpatch errors and typecasts as pointed out on the ML.
drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++- 1 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c index a2f35e3..539d848 100644 --- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else { + flush_dcache_range((ulong)data->src, + (ulong)data->src+data->blocks + *data->blocksize); + if (wml_value > WML_WR_WML_MAX) wml_value = WML_WR_WML_MAX_VAL; if ((esdhc_read32(®s->prsstat) & PRSSTAT_WPSPL) == 0) { @@ -249,7 +253,15 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) return 0; }
- +static void check_and_invalidate_dcache_range + (struct mmc_cmd *cmd, + struct mmc_data *data) { + unsigned start = (unsigned)data->dest ; + unsigned size = roundup(ARCH_DMA_MINALIGN, + data->blocks*data->blocksize); + unsigned end = start+size ; + invalidate_dcache_range(start, end); +} /* * Sends a command out on the bus. Takes the mmc pointer, * a command pointer, and an optional data pointer. @@ -311,6 +323,9 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data) while (!(esdhc_read32(®s->irqstat) & IRQSTAT_CC)) ;
+ if (data && (data->flags & MMC_DATA_READ)) + check_and_invalidate_dcache_range(cmd, data); + irqstat = esdhc_read32(®s->irqstat); esdhc_write32(®s->irqstat, irqstat);
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
- flush_dcache_range((ulong)data->src,
- (ulong)data->src+data->blocks
- *data->blocksize);
This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
if (wml_value > WML_WR_WML_MAX) wml_value = WML_WR_WML_MAX_VAL; if ((esdhc_read32(®s->prsstat) & PRSSTAT_WPSPL) == 0) { @@ -249,7 +253,15 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) return 0; }
+static void check_and_invalidate_dcache_range
- (struct mmc_cmd *cmd,
- struct mmc_data *data) {
This is non-standard formatting in U-Boot.
Andy
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
flush_dcache_range((ulong)data->src,(ulong)data->src+data->blocks*data->blocksize);This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Do you know how many other peripherals are shared between ARM and PPC that might be broken by cache operations?
if (wml_value> WML_WR_WML_MAX) wml_value = WML_WR_WML_MAX_VAL; if ((esdhc_read32(®s->prsstat)& PRSSTAT_WPSPL) == 0) {@@ -249,7 +253,15 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) return 0; }
+static void check_and_invalidate_dcache_range
(struct mmc_cmd *cmd,struct mmc_data *data) {This is non-standard formatting in U-Boot.
Ok. I can address that in a V3 once I know the direction.
Regards,
Eric
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
flush_dcache_range((ulong)data->src,(ulong)data->src+data->blocks*data->blocksize);This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
Best regards, Stefano
On 09.05.2012 07:45, Stefano Babic wrote:
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
flush_dcache_range((ulong)data->src,(ulong)data->src+data->blocks*data->blocksize);This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
Many thanks
Dirk
Hi Dirk,
On Tue, 15 May 2012 14:58:54 +0200 Dirk Behme dirk.behme@de.bosch.com wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
...
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
Stefano is on vacation; please don't expect any quick reply from him.
Thanks, Anatolij
On 05/15/2012 06:09 AM, Anatolij Gustschin wrote:
Hi Dirk,
On Tue, 15 May 2012 14:58:54 +0200 Dirk Behmedirk.behme@de.bosch.com wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
...
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
Stefano is on vacation; please don't expect any quick reply from him.
Vacation? Is that one of those European words I should really know? ;)
I don't have the same excuse. I'm just struggling to find a few hours to take a stab at this.
I'm without a PPC device and compiler, so I'll need some assistance in validating things.
Regards,
Eric
Dear Eric Nelson,
On 05/15/2012 06:09 AM, Anatolij Gustschin wrote:
Hi Dirk,
On Tue, 15 May 2012 14:58:54 +0200
Dirk Behmedirk.behme@de.bosch.com wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
...
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
Stefano is on vacation; please don't expect any quick reply from him.
Vacation? Is that one of those European words I should really know? ;)
I don't have the same excuse. I'm just struggling to find a few hours to take a stab at this.
I'm without a PPC device and compiler, so I'll need some assistance in validating things.
Well if you put up the tree somewhere, I can run it for you ;-)
Regards,
Eric
Best regards, Marek Vasut
On 15/05/2012 14:58, Dirk Behme wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
flush_dcache_range((ulong)data->src,(ulong)data->src+data->blocks*data->blocksize);This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
This is my turn to look back if there are some news. Has anyone taken a look at it ?
Best regards, Stefano Babic
On 06/12/2012 10:07 AM, Stefano Babic wrote:
On 15/05/2012 14:58, Dirk Behme wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
--- a/drivers/mmc/fsl_esdhc.c +++ b/drivers/mmc/fsl_esdhc.c @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data) esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, wml_value); esdhc_write32(®s->dsaddr, (u32)data->dest); } else {
flush_dcache_range((ulong)data->src,(ulong)data->src+data->blocks*data->blocksize);This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
This is my turn to look back if there are some news. Has anyone taken a look at it ?
Best regards, Stefano Babic
Sorry Stefano,
I've been meaning to generate an update but seem to be starving for time.
Some other messages on the list regarding PPC seem to indicate that the real solution here is a set of cache stubs for PPC, not an SDHC patch though.
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
Please advise,
Eric
Dear Eric Nelson,
On 06/12/2012 10:07 AM, Stefano Babic wrote:
On 15/05/2012 14:58, Dirk Behme wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote:
> --- a/drivers/mmc/fsl_esdhc.c > +++ b/drivers/mmc/fsl_esdhc.c > @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, > struct mmc_data *data) > > esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, > > wml_value); > > esdhc_write32(®s->dsaddr, (u32)data->dest); > > } else { > > + flush_dcache_range((ulong)data->src, > + (ulong)data->src+data->blocks > + *data->blocksize); > +
This still won't work. I don't believe this is implemented at all on the FSL PowerPC parts that use this controller.
At the very least, it needs to be protected by an ifdef.
It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
This is my turn to look back if there are some news. Has anyone taken a look at it ?
Best regards, Stefano Babic
Sorry Stefano,
I've been meaning to generate an update but seem to be starving for time.
Some other messages on the list regarding PPC seem to indicate that the real solution here is a set of cache stubs for PPC, not an SDHC patch though.
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
I think WD applied the cache stub patch already. Can you try now please?
Please advise,
Eric
Best regards, Marek Vasut
On 12.06.2012 19:23, Marek Vasut wrote:
Dear Eric Nelson,
On 06/12/2012 10:07 AM, Stefano Babic wrote:
On 15/05/2012 14:58, Dirk Behme wrote:
On 09.05.2012 07:45, Stefano Babic wrote:
On 09/05/2012 01:31, Eric Nelson wrote:
Thanks Andy,
On 05/08/2012 03:59 PM, Andy Fleming wrote: >> --- a/drivers/mmc/fsl_esdhc.c >> +++ b/drivers/mmc/fsl_esdhc.c >> @@ -190,6 +190,10 @@ static int esdhc_setup_data(struct mmc *mmc, >> struct mmc_data *data) >> >> esdhc_clrsetbits32(®s->wml, WML_RD_WML_MASK, >> >> wml_value); >> >> esdhc_write32(®s->dsaddr, (u32)data->dest); >> >> } else { >> >> + flush_dcache_range((ulong)data->src, >> + (ulong)data->src+data->blocks >> + *data->blocksize); >> + > This still won't work. I don't believe this is implemented at all on > the FSL PowerPC parts that use this controller. > > At the very least, it needs to be protected by an ifdef. It seems more generally useful to implement a PowerPC cache layer than to instrument each driver that supports cache.
Some PowerPC (MPC86xx, arch/powerpc/cpu/mpc86xx/cache.S) have this layer, some other not. This driver is used by MPC85xx and flush_dcache_range is not implemented. The reason is that this SOC uses its internal snooping mechanism and does not need to explicitely flush the buffers in the drivers.
The problem with an #ifdef is that it is not very generic - we should add some #if (defined(CONFIG_MX51) || defined(CONFIG_MX53) || ... and update the driver for each new SOCs. I think also that maybe the best way is to add an empty flush_dcache_range() to the MPC85xx, maybe as weak function.
You might have noticed that I'm looking through my list of open patches. So: Any news regarding V3 of this patch? ;)
This is my turn to look back if there are some news. Has anyone taken a look at it ?
Best regards, Stefano Babic
Sorry Stefano,
I've been meaning to generate an update but seem to be starving for time.
Some other messages on the list regarding PPC seem to indicate that the real solution here is a set of cache stubs for PPC, not an SDHC patch though.
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
I think WD applied the cache stub patch already. Can you try now please?
Ping ;)
Thanks
Dirk
Dear Dirk Behme,
[...]
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
I think WD applied the cache stub patch already. Can you try now please?
Ping ;)
Thanks
Adding our human interactions expert into CC :)
Best regards, Marek Vasut
Dear Dirk Behme,
[...]
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
I think WD applied the cache stub patch already. Can you try now please?
Ping ;)
Thanks
Dirk
I just noticed this with latest mainline: Configuring for MPC8569MDS_ATM - Board: MPC8569MDS, Options: ATM make: *** [u-boot] Error 1 powerpc-linux-size: './u-boot': No such file drivers/mmc/libmmc.o: In function `esdhc_setup_data': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:193: undefined reference to `flush_dcache_range' drivers/mmc/libmmc.o: In function `check_and_invalidate_dcache_range': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:263: undefined reference to `invalidate_dcache_range' make: *** [u-boot] Error 1 Configuring for MPC8569MDS_NAND - Board: MPC8569MDS, Options: NAND make: *** [u-boot] Error 1 powerpc-linux-size: './u-boot': No such file drivers/mmc/libmmc.o: In function `esdhc_setup_data': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:193: undefined reference to `flush_dcache_range' drivers/mmc/libmmc.o: In function `check_and_invalidate_dcache_range': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:263: undefined reference to `invalidate_dcache_range' make: *** [u-boot] Error 1
Can this be caused by this patch?
Best regards, Marek Vasut
On 20.07.2012 04:32, Marek Vasut wrote:
Dear Dirk Behme,
[...]
I'm not likely to have any time until after FTF next week.
Any chance that a PPC maintainer can chime in here?
I think WD applied the cache stub patch already. Can you try now please?
Ping ;)
Thanks
Dirk
I just noticed this with latest mainline: Configuring for MPC8569MDS_ATM - Board: MPC8569MDS, Options: ATM make: *** [u-boot] Error 1 powerpc-linux-size: './u-boot': No such file drivers/mmc/libmmc.o: In function `esdhc_setup_data': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:193: undefined reference to `flush_dcache_range' drivers/mmc/libmmc.o: In function `check_and_invalidate_dcache_range': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:263: undefined reference to `invalidate_dcache_range' make: *** [u-boot] Error 1 Configuring for MPC8569MDS_NAND - Board: MPC8569MDS, Options: NAND make: *** [u-boot] Error 1 powerpc-linux-size: './u-boot': No such file drivers/mmc/libmmc.o: In function `esdhc_setup_data': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:193: undefined reference to `flush_dcache_range' drivers/mmc/libmmc.o: In function `check_and_invalidate_dcache_range': /home/marex/testing/u-boot.git/drivers/mmc/fsl_esdhc.c:263: undefined reference to `invalidate_dcache_range' make: *** [u-boot] Error 1
Can this be caused by this patch?
Yes, I think so.
Reading the logs, to my understanding the conclusion in
http://lists.denx.de/pipermail/u-boot/2012-May/124036.html
was to "add an empty flush_dcache_range() to the MPC85xx, maybe as weak function".
Then
http://lists.denx.de/pipermail/u-boot/2012-June/126097.html
mentions "WD applied the cache stub patch already".
So do we have this "cache stub patch" somewhere which fixes the issue mentioned above? I'm not sure I've seen it ...
Best regards
Dirk
On 20/07/2012 07:35, Dirk Behme wrote:
Then
http://lists.denx.de/pipermail/u-boot/2012-June/126097.html
mentions "WD applied the cache stub patch already".
So do we have this "cache stub patch" somewhere which fixes the issue mentioned above? I'm not sure I've seen it ...
It is done, but I see it is compiled only if USB is activated. We need a fix like:
diff --git a/arch/powerpc/cpu/mpc85xx/Makefile b/arch/powerpc/cpu/mpc85xx/Makefile index 7d65aa7..34f6c54 100644 --- a/arch/powerpc/cpu/mpc85xx/Makefile +++ b/arch/powerpc/cpu/mpc85xx/Makefile @@ -131,7 +131,7 @@ COBJS += tlb.o COBJS += traps.o
# Stub implementations of cache management functions for USB -COBJS-$(CONFIG_USB_EHCI) += cache.o +COBJS += cache.o
SRCS := $(START:.o=.S) $(SOBJS:.o=.S) $(COBJS:.o=.c) OBJS := $(addprefix $(obj),$(SOBJS) $(COBJS))
I send a proper patch.
Stefano
participants (6)
-
Anatolij Gustschin -
Andy Fleming -
Dirk Behme -
Eric Nelson -
Marek Vasut -
Stefano Babic