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

[Stefano Babic <sbabic@denx.de>]

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

-- 
=====================================================================
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
=====================================================================