[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