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

Minkyu Kang
2011-06-14T08:14:59Z

Dear Rob Herring,

On 13 June 2011 21:45, Rob Herring <robherring2@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@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.