Dear Stefano Babic,
In message <4b542184.6060...@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
--
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: w...@denx.de
Doubt isn't the opposite of faith; it is an element of faith.
- Paul Tillich, German theologian and historian
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
