Dear Stefano Babic,
In message 4B542184.6060706@denx.de you wrote:
+#ifndef CONFIG_PPC +#define out_be32(a,v) writel(v,a) +#define in_be32(a) __raw_readl(a) +#endif
Are you sure these are correct definitions for all architectures? If so, they should go into a global header file, not here.
The main reason for that is to have the same common way to access esdhc registers on the powerpc and on the arm processors. The driver is already implemented for powerpc and use the out_be32() function to access the internal registers (all registers are 32 bit wide). As counterpart on i.MX51, we have the writel() function.
But my understanding is that writel() is a little endian I/O operation?
I am not sure which is the best way to do it and I ask you for help. Maybe changing in all code with a "neutral" function (but this is an additional I/O accessor, there are already a lot of them...) and setting it to point to out_be32() in asm-ppc/io.h and to writel() in asm-arm/io.h. I do not know, but it sounds to me pretty bad.
Do you have a hint to manage that ?
In the long term we indeed want to convert the code (all code, yes) to generic I/O accessors, i. e. ioreadX(), iowriteX().
But that's a bigger task.
My concern here is that defining out_be32() [=explicitely big endian] as writel() [=explicitly little endian] might be plain wrong and thus confusing.
+#ifdef CONFIG_PPC /* Enable cache snooping */ out_be32(®s->scr, 0x00000040); +#endif
Why only on PPC? [I'm asking again because I don;t want to see so many #ifdef's in such code.]
As I said, the controllers are quite the same but they are not identical. There is not this register on the i.MX51 implementation. Really this has nothing to do with PPC and ARM.
Then we should not make this depend on CONFIG_PPC.
I would prefer to have a general method to ask the controller for its capabilities and setting the bit fields according to the query. However, I have not found a way to to this and I use '#ifdef PPC' to distinguish between the two implementations of the hardware controller.
This is not a good idea. We should probably try to find out how the controller versions are named within Freescale (probably they do have internal version IDs attached - we see the same for some NAND controllers) and use these ID's to enable / disable features.
Using CONFIG_PPC here looks wrong to me.
- /* Wait until the controller is available */
- while ((in_be32(®s->sysctl) & SYSCTL_RSTA) && --timeout)
udelay(1000);Has this been tested on non-i.MX51 systems as well?
No, I could not test on powerpc, but the method is described in powerpc manual, too. In both manuals, the setting of this bit performs a reset of the controller. There is nothing special related to the ARM implementation.
I see. Please add the respective PPC custodians on the Cc: list for this patch, then.
These are a lot of changes - how mu of this has actually been tested on non-i.MX51 ?
Well, as I said I could not test on PowerQuick, but changes are not related to the architecture.
Let's me explain. The driver up now uses only the internal controller to detect if a SD card is present in the slot. However, this is not a general way. In a lot of cases the pins responsible for this function and connected to the ESDHC controller are required for another peripheral and cannot be used. In these cases, a GPIO can be used for Card Detection. The change adds only a callback in the case a GPIO is required. If the internal controller can be used (!mmc->getcd), the bits defined in PRSSTAT_CINS are still used as before. Agree this code was not tested outside the mx51evk, but changes are smaller as we can think.
OK - but please let's at least trigger the respective code maintainers so they get aware of the changes and test it on their platforms.
sprintf(mmc->name, "FSL_ESDHC");
- if (!esdhc_addr) {
regs = (struct fsl_esdhc *)CONFIG_SYS_FSL_ESDHC_ADDR;- } else {
regs = (struct fsl_esdhc *)esdhc_addr;- }
Argh... Please don't. Use a common way for configuration, please.
Well, the main reason for that is to support more than one controller. The MX51 has two ESDHC controllers and both of them are well supported in hardware on the mx51evk. The powerpc implementation supports clearly only one instance. The reason here is not to break the actual implementation on the powerpc boards, allowing in the same time more than one instance on the mx51evk board.
Personally I would prefer to change the powerpc code (but probably should be done in another patch ?), calling fsl_esdhc_mmc_init() in cpu/mpc85xx/cpu.c, cpu/mpc83xx/cpu.c (and in the board related functions, if any) and passing there the HW address of the controller. Something like that (for example, in cpu/mpc85xx/cpu.c:)
Looks OK to me. Let's ping the respective custodians...
+#ifdef CONFIG_PPC void fdt_fixup_esdhc(void *blob, bd_t *bd) { const char *compat = "fsl,esdhc"; @@ -365,3 +414,4 @@ out: do_fixup_by_compat(blob, compat, "status", status, strlen(status) + 1, 1); } +#endif
Can we drop this #ifdef ?
Really replacing it with another #ifdef...
I think the code shold be protect with CONFIG_OF_LIBFDT instead of CONFIG_PPC.
OK.
+#define clrbits(type, addr, clear) \
- write##type(__raw_read##type(addr) & ~(clear), (addr))
+#define setbits(type, addr, set) \
- write##type(__raw_read##type(addr) | (set), (addr))
+#define clrsetbits(type, addr, clear, set) \
- write##type((__raw_read##type(addr) & ~(clear)) | (set), (addr))
+#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)
Are these macros really generally applicaple to all ARM systems, including both big and little endian configurations?
See above, first issue, it is related. The reason here is to have a general way to add a "native" access method to internal registers for both architectures.
I understand your intentions, but I wonder if the implementation is correct, or if we are mixing big endian and little endian code here in a most confusing way.
Best regards,
Wolfgang Denk