Andy Fleming wrote:
Wolfgang has already covered most of this, but I have a few other comments (plus a couple of redundant ones)
Hi Andy,
I know why you did this, but I really think it's a bad idea to "trick" the driver into doing the right thing.
I agree, I wanted only to point out why I need to do this ;)
It's more painful, but we need to change all of the out_be32/in_be32 commands into something generic, and then add support for big and little endian accesses. This is just a hack. :(
Ok, let's see if I have really understood and I can proceed with the modifications. I start to change the driver replacing all _be32 functions with a more neutral name (write_register/read_register or something like that). I will define these functions then in asm-ppc/io.h and asm-arm/io.h, setting them to the desired function.
Let's try to use #ifdefs sparingly, and definitely have them trigger on constants that are directly related to the difference. This isn't a PPC/ARM problem. I see you've already agreed to use gd->sdhc_clk, though, so that's good.
I agree, using global data is a good idea.
You have two choices, here. Either create a new CONFIG_SYS option that declares the existence of this register for platforms that have it,
In this case we will probably get a lot of CONFIG_SYS options when Freescale changes version or uses this controller on other processors. There are some different registers, not only for snooping. In most cases, some further bits are added. At least I need to know if the SDCLK_EN bit is present in the control register (present in the MX.51 implementation, absent in PowerQuick).
OR (my preference) design a configuration mechanism which allows board/cpu code to declare such things.
I agree with you. I have already changed the initialization procedure and I can pass (as you suggest later) a configuration structure that describe the feature of the controller. The board can set this structure in board_mmc_init(). If it is not set, the driver should work as now (powerpc), so the structure must be filled only by arm boards.
Ideally, the driver could detect this based on a version register, but I'm well aware that FSL's hardware designers frequently forget to increment their version numbers for such "small" changes.
I know, the version register is not reliable enough...
What is certain is that CONFIG_PPC is wrong.
Absolutely agree, I get rid of it.
const char *compat = "fsl,esdhc";@@ -365,3 +414,4 @@ out: do_fixup_by_compat(blob, compat, "status", status, strlen(status) + 1, 1); } +#endif
Use the OF config option, here.
Ok, thanks.
+#define clrbits_be32(addr, clear) clrbits(l, addr, clear) +#define setbits_be32(addr, set) setbits(l, addr, set) +#define clrsetbits_be32(addr, clear, set) clrsetbits(l, addr, clear, set)
This should be part of a completely different patch. Also, I'm positive that it's completely wrong. setbits_be32 is big endian, writel is little endian.
Agree. I will set a separate patch to setup the newer functions to access the registers.
#ifdef CONFIG_FSL_ESDHC int fsl_esdhc_mmc_init(bd_t *bis); +int fsl_esdhc_initialize(bd_t *bis, uint32_t esdhc_addr,
int (*getcd)(struct mmc *mmc));Hmmm....this doesn't scale well. Rather than pass in an address and a function pointer, create a structure with that information, and then pass *that* in. That way, when we discover we want some other information/functions, we can add them without having to modify the API.
You are right. I will create a structure that I can use to describe the feature of the controller, as you already suggested.
I think getcd needs more discussion, but even if it doesn't, this clearly belongs in a separate patch. You are modifying the U-Boot MMC API, here, not just the fsl_esdhc driver.
Probably I do not need anymore. I can implement as you suggest a weakly defined function, something like:
int board_mmc_getcd(u8 *cd, struct mmc *mmc)__attribute__((weak, alias("__board_mmc_getcd")))
If this failed, I can use the internal bits of the controller to check the presence of the card.
Stefano