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

[Stefano Babic]

Wolfgang Denk wrote:
> Dear Stefano Babic,
> 

Hi Wolfgang,

>> +#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.

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 ?

> 
>> @@ -129,7 +133,7 @@ static int esdhc_setup_data(struct mmc *mmc, struct mmc_data *data)
>>  	out_be32(&regs->blkattr, data->blocks << 16 | data->blocksize);
>>  
>>  	/* Calculate the timeout period for data transactions */
>> -	timeout = __ilog2(mmc->tran_speed/10);
>> +	timeout = fls(mmc->tran_speed/10) - 1;
> 
> What's the reason for this change?

The only reason is that there is no __ilog2 function for the arm
architecture and I preferred to change it in a way that is defined for
all architectures, not only powerpc. ilog2 is defined as (fls() - 1) in
linux if no architecture specific function is provided. So, even if it
is not optimized, the change is suitable for powerpc, too.

> 
>> @@ -236,11 +240,18 @@ esdhc_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, struct mmc_data *data)
>>  
>>  void set_sysctl(struct mmc *mmc, uint clock)
>>  {
>> +#ifdef CONFIG_PPC
>>  	int sdhc_clk = gd->sdhc_clk;
>> +#else
>> +	int sdhc_clk = mxc_get_clock(MXC_IPG_PERCLK);
>> +#endif
> 
> Can we avoid such #ifdef's here? Why don't we use gd->sdhc_clk
> everywhere?

Yes, thanks, good tip. I will add the field sdhc_clk to the global data
for arm and I will get rid of these unneeded #ifdef.

> 
>> @@ -257,11 +268,14 @@ void set_sysctl(struct mmc *mmc, uint clock)
>>  
>>  	clk = (pre_div << 8) | (div << 4);
>>  
>> +	/* On imx the clock must be stopped before changing frequency */
>> +	clrbits_be32(&regs->sysctl, SYSCTL_CKEN);
> 
> Is this compatible with the other architectures?

Zeroing the bit should be ok as described in PowerQuick manual.
The controller in the i.MX51 is (quite) the same as in the powerpc (I
checked in the MPC8560 manual), with the exception of some registers. It
seems that the controller in the i.MX51 is an evolution and some
features (as the possibility to enable/disable the clock when no access
to the card is required) are added later. Really to be sure I have to
protect the setting of this bit (with #ifndef PPC....)

>> +#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.

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.

> 
>> +	/* Reset the entire host controller */
>> +	out_be32(&regs->sysctl, SYSCTL_RSTA);
>> +
>> +	/* 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.

> 
>> @@ -299,24 +324,44 @@ static int esdhc_init(struct mmc *mmc)
>>  	/* Put the PROCTL reg back to the default */
>>  	out_be32(&regs->proctl, PROCTL_INIT);
>>  
>> -	while (!(in_be32(&regs->prsstat) & PRSSTAT_CINS) && --timeout)
>> -		udelay(1000);
>> +	/* Set timout to the maximum value */
>> +	clrsetbits_be32(&regs->sysctl, SYSCTL_TIMEOUT_MASK, 14 << 16);
>>  
>> -	if (timeout <= 0)
>> -		return NO_CARD_ERR;
>> +	/* Check if there is a callback for detecting the card */
>> +	if (!mmc->getcd) {
>> +		timeout = 1000;
>> +		while (!(in_be32(&regs->prsstat) & PRSSTAT_CINS) && --timeout)
>> +			udelay(1000);
>>  
>> -	return 0;
>> +		if (timeout <= 0)
>> +			ret = NO_CARD_ERR;
>> +	} else {
>> +		if (mmc->getcd(mmc))
>> +			ret = NO_CARD_ERR;
>> +	}
>> +
>> +	return ret;
>>  }
> 
> 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.

>> +	struct fsl_esdhc *regs;
>>  	struct mmc *mmc;
>>  	u32 caps;
>>  
>>  	mmc = malloc(sizeof(struct mmc));
>>  
>>  	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:)

--- a/cpu/mpc85xx/cpu.c
+++ b/cpu/mpc85xx/cpu.c
@@ -323,7 +323,7 @@ void upmconfig (uint upm, uint * table, uint size)
 int cpu_mmc_init(bd_t *bis)
 {
 #ifdef CONFIG_FSL_ESDHC
-       return fsl_esdhc_mmc_init(bis);
+       return fsl_esdhc_initialize(bis, CONFIG_SYS_MPC85xx_ESDHC_ADDR,
NULL);
 #else
        return 0;
 #endif

> 
>> +#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.

>> +#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.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================