Am 23.01.2017 um 07:06 schrieb Jaehoon Chung: > On 01/20/2017 04:18 PM, Heiner Kallweit wrote: >> This driver implements MMC support on Meson GX (S905) based systems. >> It's based on Carlo Caione's work, changes: >> - BLK support added >> - minor general refactoring >> >> Original author: Carlo Caione <[email protected]> >> Signed-off-by: Heiner Kallweit <[email protected]> >> --- >> arch/arm/include/asm/arch-meson/gxbb.h | 2 + >> arch/arm/include/asm/arch-meson/sd_emmc.h | 77 ++++++++ >> drivers/mmc/Kconfig | 11 ++ >> drivers/mmc/Makefile | 1 + >> drivers/mmc/meson_gx_mmc.c | 281 >> ++++++++++++++++++++++++++++++ >> include/configs/meson-gxbb-common.h | 4 + >> 6 files changed, 376 insertions(+) >> create mode 100644 arch/arm/include/asm/arch-meson/sd_emmc.h >> create mode 100644 drivers/mmc/meson_gx_mmc.c >> >> diff --git a/arch/arm/include/asm/arch-meson/gxbb.h >> b/arch/arm/include/asm/arch-meson/gxbb.h >> index ce41349..af21222 100644 >> --- a/arch/arm/include/asm/arch-meson/gxbb.h >> +++ b/arch/arm/include/asm/arch-meson/gxbb.h >> @@ -20,6 +20,8 @@ >> #define GXBB_GPIO_IN(n) GXBB_PERIPHS_ADDR(_GXBB_GPIO_OFF(n) + 1) >> #define GXBB_GPIO_OUT(n) GXBB_PERIPHS_ADDR(_GXBB_GPIO_OFF(n) + 2) >> >> +#define GXBB_PINMUX(n) GXBB_PERIPHS_ADDR(0x2c + (n)) >> + > > It seems that this code is not related to MMC. > >> #define GXBB_ETH_REG_0 GXBB_PERIPHS_ADDR(0x50) >> #define GXBB_ETH_REG_1 GXBB_PERIPHS_ADDR(0x51) >> >> diff --git a/arch/arm/include/asm/arch-meson/sd_emmc.h >> b/arch/arm/include/asm/arch-meson/sd_emmc.h >> new file mode 100644 >> index 0000000..f07ec52 >> --- /dev/null >> +++ b/arch/arm/include/asm/arch-meson/sd_emmc.h >> @@ -0,0 +1,77 @@ >> +/* >> + * (C) Copyright 2016 Carlo Caione <[email protected]> > > 2017? > >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef __SD_EMMC_H__ >> +#define __SD_EMMC_H__ >> + >> +#include <mmc.h> >> + >> +#define SDIO_PORT_A 0 >> +#define SDIO_PORT_B 1 >> +#define SDIO_PORT_C 2 >> + >> +#define SD_IRQ_ALL 0x3fff >> + >> +#define SD_EMMC_CLKSRC_24M 24000000 >> +#define SD_EMMC_CLKSRC_DIV2 1000000000 >> + >> +#define CLK_DIV 0 >> +#define CLK_SRC 6 >> +#define CLK_CO_PHASE 8 >> +#define CLK_ALWAYS_ON 24 >> + >> +#define ADDR_USE_PING_BUF BIT(1) >> + >> +#define CFG_BUS_WIDTH 0 >> +#define CFG_BUS_WIDTH_MASK (0x3 << 0) > > Don't need to shift 0. > >> +#define CFG_BL_LEN 4 >> +#define CFG_BL_LEN_MASK (0xf << 4) >> +#define CFG_RESP_TIMEOUT 8 >> +#define CFG_RESP_TIMEOUT_MASK (0xf << 8) >> +#define CFG_RC_CC 12 >> +#define CFG_RC_CC_MASK (0xf << 12) >> + >> +#define STATUS_ERR_MASK GENMASK(12, 0) >> +#define STATUS_RXD_ERR_MASK 0xff >> +#define STATUS_TXD_ERR BIT(8) >> +#define STATUS_DESC_ERR BIT(9) >> +#define STATUS_RESP_ERR BIT(10) >> +#define STATUS_RESP_TIMEOUT BIT(11) >> +#define STATUS_DESC_TIMEOUT BIT(12) >> +#define STATUS_END_OF_CHAIN BIT(13) >> + >> +#define CMD_CFG_LENGTH_MASK 0x1ff >> +#define CMD_CFG_CMD_INDEX 24 >> +#define CMD_CFG_BLOCK_MODE BIT(9) >> +#define CMD_CFG_R1B BIT(10) >> +#define CMD_CFG_END_OF_CHAIN BIT(11) >> +#define CMD_CFG_NO_RESP BIT(16) >> +#define CMD_CFG_DATA_IO BIT(18) >> +#define CMD_CFG_DATA_WR BIT(19) >> +#define CMD_CFG_RESP_NOCRC BIT(20) >> +#define CMD_CFG_RESP_128 BIT(21) >> +#define CMD_CFG_OWNER BIT(31) > > Adds the comments about thess bits are related to which register. > e.g) /* CFG regiters */ > ....#define CMD_CFG.... > ... > > >> + >> +#define MESON_SD_EMMC_CLOCK 0x000 >> +#define MESON_SD_EMMC_CFG 0x044 >> +#define MESON_SD_EMMC_STATUS 0x048 >> +#define MESON_SD_EMMC_CMD_CFG 0x050 >> +#define MESON_SD_EMMC_CMD_ARG 0x054 >> +#define MESON_SD_EMMC_CMD_DAT 0x058 >> +#define MESON_SD_EMMC_CMD_RSP 0x05c >> +#define MESON_SD_EMMC_CMD_RSP1 0x060 >> +#define MESON_SD_EMMC_CMD_RSP2 0x064 >> +#define MESON_SD_EMMC_CMD_RSP3 0x068 >> +#define MESON_SD_EMMC_PING 0x400 >> + >> +struct meson_mmc_platdata { >> + struct mmc_config cfg; >> + struct mmc mmc; >> + void *regbase; >> + char *w_buf; >> +}; >> + >> +#endif >> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig >> index 9ed8da3..03416ff 100644 >> --- a/drivers/mmc/Kconfig >> +++ b/drivers/mmc/Kconfig >> @@ -168,6 +168,17 @@ config ZYNQ_SDHCI >> help >> Support for Arasan SDHCI host controller on Zynq/ZynqMP ARM SoCs >> platform >> >> +config MMC_MESON_GX >> + bool "Meson GX EMMC controller support" >> + depends on DM_MMC && BLK && DM_MMC_OPS && ARCH_MESON >> + help >> + Support for EMMC host controller on Meson GX ARM SoCs platform (S905) >> + >> +config MMC_MESON_GX_SD_PORT >> + int "Meson SD port selection" >> + range 0 2 >> + depends on MMC_MESON_GX >> + >> config ROCKCHIP_SDHCI >> bool "Arasan SDHCI controller for Rockchip support" >> depends on DM_MMC && BLK && DM_MMC_OPS >> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile >> index 4dca09c..6e951a0 100644 >> --- a/drivers/mmc/Makefile >> +++ b/drivers/mmc/Makefile >> @@ -31,6 +31,7 @@ ifdef CONFIG_SUPPORT_EMMC_BOOT >> obj-$(CONFIG_GENERIC_MMC) += mmc_boot.o >> endif >> obj-$(CONFIG_GENERIC_ATMEL_MCI) += gen_atmel_mci.o >> +obj-$(CONFIG_MMC_MESON_GX) += meson_gx_mmc.o >> obj-$(CONFIG_MMC_SPI) += mmc_spi.o >> obj-$(CONFIG_MVEBU_MMC) += mvebu_mmc.o >> obj-$(CONFIG_MMC_OMAP_HS) += omap_hsmmc.o >> diff --git a/drivers/mmc/meson_gx_mmc.c b/drivers/mmc/meson_gx_mmc.c >> new file mode 100644 >> index 0000000..69aa15f >> --- /dev/null >> +++ b/drivers/mmc/meson_gx_mmc.c >> @@ -0,0 +1,281 @@ >> +/* >> + * (C) Copyright 2016 Carlo Caione <[email protected]> > > Ditto. waht is "[email protected]"? "ca..." is right? > >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#include <common.h> >> +#include <fdtdec.h> >> +#include <malloc.h> >> +#include <mmc.h> >> +#include <asm/io.h> >> +#include <asm/arch/sd_emmc.h> >> +#include <dm/device.h> >> + >> +static inline void *get_regbase(const struct mmc *mmc) > > This function is really need? > >> +{ >> + struct meson_mmc_platdata *pdata = mmc->priv; >> + >> + return pdata->regbase; >> +} >> + >> +static inline uint32_t meson_read(struct mmc *mmc, int offset) >> +{ >> + return readl(get_regbase(mmc) + offset); >> +} >> + >> +static inline void meson_write(struct mmc *mmc, uint32_t val, int offset) >> +{ >> + writel(val, get_regbase(mmc) + offset); >> +} > > Why do you pass the mmc structure? Isn't this host controller? > >> + >> +static void meson_mmc_config_clock(struct mmc *mmc) >> +{ >> + uint32_t meson_mmc_clk = 0; >> + unsigned int clk, clk_src, clk_div; >> + >> + if (mmc->clock > 12000000) { >> + clk = SD_EMMC_CLKSRC_DIV2; >> + clk_src = 1; >> + } else { >> + clk = SD_EMMC_CLKSRC_24M; >> + clk_src = 0; >> + } >> + clk_div = clk / mmc->clock; >> + >> + if (mmc->clock < mmc->cfg->f_min) >> + mmc->clock = mmc->cfg->f_min; >> + if (mmc->clock > mmc->cfg->f_max) >> + mmc->clock = mmc->cfg->f_max; > > I don't understand why mmc->clock value is assigned at here? > mmc->clock value is assigned at mmc.c > >> + >> + /* Keep the clock always on */ >> + meson_mmc_clk |= (1 << CLK_ALWAYS_ON); >> + >> + /* 180 phase */ >> + meson_mmc_clk |= (2 << CLK_CO_PHASE); > > Use the macro about 1 and 2. > >> + >> + /* clock settings */ >> + meson_mmc_clk |= (clk_src << CLK_SRC); >> + meson_mmc_clk |= (clk_div << CLK_DIV); >> + >> + meson_write(mmc, meson_mmc_clk, MESON_SD_EMMC_CLOCK); >> +} >> + >> +static int meson_dm_mmc_set_ios(struct udevice *dev) >> +{ >> + struct mmc *mmc = mmc_get_mmc_dev(dev); >> + uint32_t meson_mmc_cfg = 0; >> + >> + meson_mmc_config_clock(mmc); >> + >> + meson_mmc_cfg = meson_read(mmc, MESON_SD_EMMC_CFG); >> + >> + /* 1-bit mode */ >> + meson_mmc_cfg &= ~CFG_BUS_WIDTH_MASK; >> + meson_mmc_cfg |= (mmc->bus_width / 4 << CFG_BUS_WIDTH); >> + >> + /* 512 bytes block length */ >> + meson_mmc_cfg &= ~CFG_BL_LEN_MASK; >> + meson_mmc_cfg |= (9 << CFG_BL_LEN); >> + >> + /* Response timeout 256 clk */ >> + meson_mmc_cfg &= ~CFG_RESP_TIMEOUT_MASK; >> + meson_mmc_cfg |= (7 << CFG_RESP_TIMEOUT); >> + >> + /* Command-command gap 1024 clk */ >> + meson_mmc_cfg &= ~CFG_RC_CC_MASK; >> + meson_mmc_cfg |= (4 << CFG_RC_CC); > > Ditto about 9, 7, 3. > >> + >> + meson_write(mmc, meson_mmc_cfg, MESON_SD_EMMC_CFG); >> + >> + return 0; >> +} >> + >> +static void meson_mmc_setup_cmd(struct mmc *mmc, struct mmc_data *data, >> + struct mmc_cmd *cmd) >> +{ >> + uint32_t meson_mmc_cmd = 0; >> + >> + meson_mmc_cmd = ((0x80 | cmd->cmdidx) << CMD_CFG_CMD_INDEX); > > don't use the magic number likes 0x80. > >> + >> + if (cmd->resp_type & MMC_RSP_PRESENT) { >> + if (cmd->resp_type & MMC_RSP_136) >> + meson_mmc_cmd |= CMD_CFG_RESP_128; >> + >> + if (cmd->resp_type & MMC_RSP_BUSY) >> + meson_mmc_cmd |= CMD_CFG_R1B; >> + >> + if (!(cmd->resp_type & MMC_RSP_CRC)) >> + meson_mmc_cmd |= CMD_CFG_RESP_NOCRC; >> + } else { >> + meson_mmc_cmd |= CMD_CFG_NO_RESP; >> + } >> + >> + if (data) { >> + meson_mmc_cmd |= CMD_CFG_DATA_IO; >> + >> + if (data->flags == MMC_DATA_WRITE) >> + meson_mmc_cmd |= CMD_CFG_DATA_WR; >> + >> + if (data->blocks > 1) { >> + meson_mmc_cmd |= CMD_CFG_BLOCK_MODE; >> + meson_mmc_cmd |= data->blocks; >> + } else { >> + meson_mmc_cmd |= (data->blocksize & >> CMD_CFG_LENGTH_MASK); >> + } >> + } >> + >> + meson_mmc_cmd |= CMD_CFG_OWNER; >> + meson_mmc_cmd |= CMD_CFG_END_OF_CHAIN; > > Make one line. > >> + >> + meson_write(mmc, meson_mmc_cmd, MESON_SD_EMMC_CMD_CFG); >> +} >> + >> +static void meson_mmc_setup_addr(struct mmc *mmc, struct mmc_data *data) >> +{ >> + struct meson_mmc_platdata *pdata = mmc->priv; >> + unsigned int data_size = 0; >> + uint32_t meson_mmc_data_addr = 0; > > make more simple variable name. > >> + >> + if (data) { >> + data_size = data->blocks * data->blocksize; >> + >> + if (data->flags == MMC_DATA_READ) { >> + if (data_size < 0x200) { > > What is 0x200? > >> + meson_mmc_data_addr = >> + (ulong) (get_regbase(mmc) + >> MESON_SD_EMMC_PING); >> + meson_mmc_data_addr |= ADDR_USE_PING_BUF; >> + } else { >> + invalidate_dcache_range((ulong) data->dest, >> + (ulong) (data->dest + >> data_size)); >> + meson_mmc_data_addr = (ulong) data->dest; >> + } >> + } else if (data->flags == MMC_DATA_WRITE) { >> + pdata->w_buf = calloc(data_size, sizeof(char)); >> + memcpy(pdata->w_buf, data->src, data_size); >> + flush_dcache_range((ulong) pdata->w_buf, >> + (ulong) (pdata->w_buf + data_size)); >> + meson_mmc_data_addr = (ulong) pdata->w_buf; >> + } >> + } >> + >> + meson_write(mmc, meson_mmc_data_addr, MESON_SD_EMMC_CMD_DAT); >> +} >> + >> +static void meson_mmc_read_response(struct mmc *mmc, struct mmc_data *data, >> + struct mmc_cmd *cmd) >> +{ >> + unsigned int data_size = 0; >> + >> + if (data) { >> + data_size = data->blocks * data->blocksize; >> + if ((data_size < 0x200) && (data->flags == MMC_DATA_READ)) >> + memcpy(data->dest, >> + get_regbase(mmc) + MESON_SD_EMMC_PING, >> + data_size); >> + } >> + >> + if (cmd->resp_type & MMC_RSP_136) { >> + cmd->response[0] = meson_read(mmc, MESON_SD_EMMC_CMD_RSP3); >> + cmd->response[1] = meson_read(mmc, MESON_SD_EMMC_CMD_RSP2); >> + cmd->response[2] = meson_read(mmc, MESON_SD_EMMC_CMD_RSP1); >> + cmd->response[3] = meson_read(mmc, MESON_SD_EMMC_CMD_RSP); >> + } else { >> + cmd->response[0] = meson_read(mmc, MESON_SD_EMMC_CMD_RSP); >> + } >> +} >> + >> +static int meson_dm_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, >> + struct mmc_data *data) >> +{ >> + struct mmc *mmc = mmc_get_mmc_dev(dev); >> + struct meson_mmc_platdata *pdata = mmc->priv; >> + uint32_t meson_mmc_irq = 0; >> + int ret = 0; >> + >> + meson_mmc_setup_cmd(mmc, data, cmd); >> + meson_mmc_setup_addr(mmc, data); >> + >> + meson_write(mmc, SD_IRQ_ALL, MESON_SD_EMMC_STATUS); >> + meson_write(mmc, cmd->cmdarg, MESON_SD_EMMC_CMD_ARG); >> + >> + do { >> + meson_mmc_irq = meson_read(mmc, MESON_SD_EMMC_STATUS); >> + } while(!(meson_mmc_irq & STATUS_END_OF_CHAIN)); > > I don't want to use this loop..because it might have the potential infinite > loop. > >> + >> + if (meson_mmc_irq & STATUS_RESP_TIMEOUT) >> + ret = -ETIMEDOUT; >> + else if (meson_mmc_irq & STATUS_ERR_MASK) >> + ret = -EIO; >> + >> + meson_mmc_read_response(mmc, data, cmd); >> + >> + if (data && data->flags == MMC_DATA_WRITE) >> + free(pdata->w_buf); >> + >> + return ret; >> +} >> + >> +static const struct dm_mmc_ops meson_dm_mmc_ops = { >> + .send_cmd = meson_dm_mmc_send_cmd, >> + .set_ios = meson_dm_mmc_set_ios, >> +}; >> + >> +static int meson_mmc_ofdata_to_platdata(struct udevice *dev) >> +{ >> + struct meson_mmc_platdata *pdata = dev_get_platdata(dev); >> + fdt_addr_t addr; >> + >> + addr = dev_get_addr(dev); >> + if (addr == FDT_ADDR_T_NONE) >> + return -EINVAL; >> + >> + pdata->regbase = (void *)addr; >> + >> + return 0; >> +} >> + >> +static int meson_mmc_probe(struct udevice *dev) >> +{ >> + struct meson_mmc_platdata *pdata = dev_get_platdata(dev); >> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev); >> + struct mmc_config *cfg = &pdata->cfg; >> + >> + cfg->voltages = MMC_VDD_33_34 | MMC_VDD_32_33 | >> + MMC_VDD_31_32 | MMC_VDD_165_195; >> + cfg->host_caps = MMC_MODE_8BIT | MMC_MODE_4BIT | >> + MMC_MODE_HS_52MHz | MMC_MODE_HS; >> + cfg->f_min = 400000; >> + cfg->f_max = 50000000; >> + cfg->b_max = 256; > > Max block count is 256, right? > >> + cfg->name = dev->name; >> + >> + pdata->mmc.priv = pdata; >> + pdata->mmc.clock = 400000; >> + upriv->mmc = &pdata->mmc; >> + >> + return 0; >> +} >> + >> +int meson_mmc_bind(struct udevice *dev) >> +{ >> + struct meson_mmc_platdata *pdata = dev_get_platdata(dev); >> + >> + return mmc_bind(dev, &pdata->mmc, &pdata->cfg); >> +} >> + >> +static const struct udevice_id meson_mmc_match[] = { >> + { .compatible = "amlogic,meson-gx-mmc" }, >> + { /* sentinel */ } >> +}; >> + >> +U_BOOT_DRIVER(meson_mmc) = { >> + .name = "meson_gx_mmc", >> + .id = UCLASS_MMC, >> + .of_match = meson_mmc_match, >> + .ops = &meson_dm_mmc_ops, >> + .probe = meson_mmc_probe, >> + .bind = meson_mmc_bind, >> + .ofdata_to_platdata = meson_mmc_ofdata_to_platdata, >> + .platdata_auto_alloc_size = sizeof(struct meson_mmc_platdata), >> +}; >> diff --git a/include/configs/meson-gxbb-common.h >> b/include/configs/meson-gxbb-common.h >> index 3bba2e6..3e54255 100644 >> --- a/include/configs/meson-gxbb-common.h >> +++ b/include/configs/meson-gxbb-common.h >> @@ -24,6 +24,10 @@ >> #define CONFIG_SYS_INIT_SP_ADDR 0x20000000 >> #define CONFIG_SYS_LOAD_ADDR CONFIG_SYS_TEXT_BASE >> >> +#ifdef CONFIG_MMC >> +#define CONFIG_GENERIC_MMC 1 >> +#endif > > doesn't need to put 1. > >> + >> /* Generic Interrupt Controller Definitions */ >> #define GICD_BASE 0xc4301000 >> #define GICC_BASE 0xc4302000 >> > > Thanks for the review comments. I will address them, refactor the code a little more and send a v2.
Regards, Heiner _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
