Dear Rob Herring,
On 13 June 2011 21:45, Rob Herring <robherri...@gmail.com> wrote:
> On 06/13/2011 01:59 AM, Minkyu Kang wrote:
>>
>> Dear Rob Herring,
>>
>> On 12 June 2011 06:46, Rob Herring<robherri...@gmail.com> wrote:
>>> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
>>> index 280738f..c82bde0 100644
>>> --- a/drivers/mmc/sdhci.c
>>> +++ b/drivers/mmc/sdhci.c
>>> @@ -21,18 +21,56 @@
>>> #include<common.h>
>>> #include<mmc.h>
>>> #include<asm/io.h>
>>> -#include<asm/arch/mmc.h>
>>> #include<asm/arch/clk.h>
>>>
>>> +struct sdhci_mmc {
>>> + unsigned int sysad;
>>> + unsigned short blksize;
>>> + unsigned short blkcnt;
>>> + unsigned int argument;
>>> + unsigned short trnmod;
>>> + unsigned short cmdreg;
>>> + unsigned int rspreg0;
>>> + unsigned int rspreg1;
>>> + unsigned int rspreg2;
>>> + unsigned int rspreg3;
>>> + unsigned int bdata;
>>> + unsigned int prnsts;
>>> + unsigned char hostctl;
>>> + unsigned char pwrcon;
>>> + unsigned char blkgap;
>>> + unsigned char wakcon;
>>> + unsigned short clkcon;
>>> + unsigned char timeoutcon;
>>> + unsigned char swrst;
>>> + unsigned int norintsts; /* errintsts */
>>> + unsigned int norintstsen; /* errintstsen */
>>> + unsigned int norintsigen; /* errintsigen */
>>> + unsigned short acmd12errsts;
>>> + unsigned char res1[2];
>>> + unsigned int capareg;
>>> + unsigned char res2[4];
>>> + unsigned int maxcurr;
>>> + unsigned char res3[0x34];
>>> + unsigned int control2;
>>> + unsigned int control3;
>>> + unsigned char res4[4];
>>> + unsigned int control4;
>>> + unsigned char res5[0x6e];
>>> + unsigned short hcver;
>>> +};
>>> +
>>> +struct mmc_host {
>>> + struct sdhci_mmc *reg;
>>> + unsigned int version; /* SDHCI spec. version */
>>> + unsigned int clock; /* Current clock (MHz) */
>>> + int dev_index;
>>> +};
>>
>> Please move this structures to header file.
>
> Why? There is no header anymore. These structures are only used within the
> .c file, so there is no point in having a header. They were almost the same
> before and just duplicated code. The only difference was padding added to
> the end of the struct used to calculate base address and the 2nd controller.
> Using the size of the struct to calculate base addresses doesn't work for
> all platforms. The fact that there were already 2 structs for 2 platforms is
> evidence of that.
I mean.. move it to include/sdhci.h.
There are possibility to use at other files.
>>> -static int s5p_mmc_initialize(int dev_index, int bus_width)
>>> +static int sdhci_mmc_initialize(void *base, int bus_width)
>>> {
>>> struct mmc *mmc;
>>> + struct mmc_host *host;
>>> +
>>> + if (dev_count>= 4)
>>> + return -1;
>>>
>>> - mmc =&mmc_dev[dev_index];
>>> + mmc =&mmc_dev[dev_count];
>>> + host =&mmc_host[dev_count];
>>> + host->dev_index = dev_count;
>>> + dev_count++;
>>
>> NAK.
>> dev_index is not a count number, it's a h/w device number.
>> This change is wrong. (Maybe you misunderstood this concept)
>> Please keep the dev_index.
>>
>
> What have I broken? The only thing the index was used for was getting the
> base address and indexing to driver struct. I've simply pulled out the base
> address from the driver so the board code can specify it. You still have an
> index that is controlled by order the board code initializes each
> controller.
No.
Please see the code in detail.
dev_index is used by set_mmc_clk in mmc_chage_clock function (line 295).
set_mmc_clk need h/w device number.
universal_c210 board have 2 mmc device.
device 0 is eMMC and device 2 is SD card.
According to your patch, we should register the dummy device for device 1.
I can't agree it.
Please don't modify unrelated part of patch.
Thanks
Minkyu Kang
--
from. prom.
www.promsoft.net
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
