[U-Boot] [PATCH 7/9] fsl_esdhc: add support for mx51 processor

[Wolfgang Denk]

Dear Stefano Babic,

In message <4B542184.6060706 at 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(&regs->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(&regs->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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Doubt isn't the opposite of faith; it is an element of faith.
- Paul Tillich, German theologian and historian