[U-Boot] [PATCH 2/8] mmc: sdhci: rework Samsung specfic code

[Minkyu Kang]

Dear Rob Herring,

On 13 June 2011 21:45, Rob Herring <robherring2 at gmail.com> wrote:
> On 06/13/2011 01:59 AM, Minkyu Kang wrote:
>>
>> Dear Rob Herring,
>>
>> On 12 June 2011 06:46, Rob Herring<robherring2 at 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