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

Rob Herring robherring2 at gmail.com
Mon Jun 13 14:45:25 CEST 2011


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:
>> From: Rob Herring<rob.herring at calxeda.com>
>>
>> Move the register definitions into the sdhci.c file. Set the base
>> address from the board init code.
>>
>> The Samsung SDHCI controller has extra registers. Make them conditional
>> on CONFIG_MMC_S5P.
>>
>> Signed-off-by: Rob Herring<rob.herring at calxeda.com>
>> ---
>>   arch/arm/include/asm/arch-s5pc1xx/mmc.h  |   73 --------------------------
>>   arch/arm/include/asm/arch-s5pc2xx/mmc.h  |   73 --------------------------
>>   board/samsung/goni/goni.c                |    4 +-
>>   board/samsung/universal_c210/universal.c |    8 ++-
>>   drivers/mmc/sdhci.c                      |   82 +++++++++++++++++++++++-------
>>   include/sdhci.h                          |   18 +++++++
>>   6 files changed, 89 insertions(+), 169 deletions(-)
>>   delete mode 100644 arch/arm/include/asm/arch-s5pc1xx/mmc.h
>>   delete mode 100644 arch/arm/include/asm/arch-s5pc2xx/mmc.h
>>   create mode 100644 include/sdhci.h
>>
>> diff --git a/board/samsung/universal_c210/universal.c b/board/samsung/universal_c210/universal.c
>> index b65bc6e..b0e76e8 100644
>> --- a/board/samsung/universal_c210/universal.c
>> +++ b/board/samsung/universal_c210/universal.c
>> @@ -23,10 +23,10 @@
>>   */
>>
>>   #include<common.h>
>> +#include<sdhci.h>
>>   #include<asm/io.h>
>>   #include<asm/arch/adc.h>
>>   #include<asm/arch/gpio.h>
>> -#include<asm/arch/mmc.h>
>>
>>   DECLARE_GLOBAL_DATA_PTR;
>>
>> @@ -151,6 +151,7 @@ int checkboard(void)
>>   int board_mmc_init(bd_t *bis)
>>   {
>>         int i, err;
>> +       u32 base;
>>
>>         switch (get_hwrev()) {
>>         case 0:
>> @@ -217,7 +218,8 @@ int board_mmc_init(bd_t *bis)
>>          * mmc0  : eMMC (8-bit buswidth)
>>          * mmc2  : SD card (4-bit buswidth)
>>          */
>> -       err = s5p_mmc_init(0, 8);
>> +       base = samsung_get_base_mmc();
>> +       err = sdhci_mmc_init((void *)base, 8);
>>
>>         /*
>>          * Check the T-flash  detect pin
>> @@ -241,7 +243,7 @@ int board_mmc_init(bd_t *bis)
>>                         /* GPK2[0:6] drv 4x */
>>                         gpio_set_drv(&gpio2->k2, i, GPIO_DRV_4X);
>>                 }
>> -               err = s5p_mmc_init(2, 4);
>> +               err = sdhci_mmc_init((void *)(base + 0x20000), 4);
>
> No, please use the macro instead of constant. (pass the device number to macro)
>
>>         }
>>
>>         return err;
>> 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.

>
>> +
>>   /* support 4 mmc hosts */
>>   struct mmc mmc_dev[4];
>>   struct mmc_host mmc_host[4];
>> -
>> -static inline struct s5p_mmc *s5p_get_base_mmc(int dev_index)
>> -{
>> -       unsigned long offset = dev_index * sizeof(struct s5p_mmc);
>> -       return (struct s5p_mmc *)(samsung_get_base_mmc() + offset);
>> -}
>> +int dev_count;
>>
>>   static void mmc_prepare_data(struct mmc_host *host, struct mmc_data *data)
>>   {
>> @@ -255,6 +293,7 @@ static void mmc_change_clock(struct mmc_host *host, uint clock)
>>         unsigned long timeout;
>>         unsigned long ctrl2;
>>
>> +#ifdef CONFIG_S5P_MMC
>>         /*
>>          * SELBASECLK[5:4]
>>          * 00/01 = HCLK
>> @@ -265,7 +304,7 @@ static void mmc_change_clock(struct mmc_host *host, uint clock)
>>         ctrl2&= ~(3<<  4);
>>         ctrl2 |= (2<<  4);
>>         writel(ctrl2,&host->reg->control2);
>> -
>> +#endif
>>         writew(0,&host->reg->clkcon);
>>
>>         /* XXX: we assume that clock is between 40MHz and 50MHz */
>> @@ -320,6 +359,7 @@ static void mmc_set_ios(struct mmc *mmc)
>>
>>         debug("bus_width: %x, clock: %d\n", mmc->bus_width, mmc->clock);
>>
>> +#ifdef CONFIG_S5P_MMC
>>         /*
>>          * SELCLKPADDS[17:16]
>>          * 00 = 2mA
>> @@ -349,7 +389,7 @@ static void mmc_set_ios(struct mmc *mmc)
>>          *      10 = Delay4 (inverter delay + 2ns)
>>          */
>>         writel(0x8080,&host->reg->control3);
>> -
>> +#endif
>>         mmc_change_clock(host, mmc->clock);
>>
>>         ctrl = readb(&host->reg->hostctl);
>> @@ -445,14 +485,21 @@ static int mmc_core_init(struct mmc *mmc)
>>         return 0;
>>   }
>>
>> -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.

Rob


More information about the U-Boot mailing list