[PATCH 5/6] mmc: actions: add MMC driver for Actions OWL S700

Amit Tomar atomar25opensource at gmail.com
Mon Dec 14 15:42:46 CET 2020


Hi Jaehoon

Thanks for having a look and providing the comments

Order to the bit number.

Ok, Sure

>
> > +
> > +#define OWL_MMC_OCR                     (MMC_VDD_32_33 | MMC_VDD_33_34
> | \
> > +                                      MMC_VDD_165_195)
> > +
> > +#define DATA_TRANSFER_TIMEOUT                (3 * (100 * 1000))
>
> Really need to set 300000?
>

I should have used the timeout value used in the Mainline Linux driver
which is 30000.
I would fix it to 30000.


>
> > +
> > +/*
> > + * Simple DMA transfer operations defines for MMC/SD card
> > + */
> > +#define DMA0_CHANNEL_BASE(x)         (0xe0230100 + 0x100 * (x))
>
> What is 0xe0230100?
>

This is the base address of external DMA channel 0 used for MMC/SD
controllers. Linux has
has separate full blown DMA driver for it but here we chose not to have
separate driver
instead use DMA bits only needed for MMC/SD.

>
> > +
> > +#define DMA_MODE                     0x0000
> > +#define DMA_SOURCE                   0x0004
> > +#define DMA_DESTINATION                      0x0008
> > +#define DMA_FRAME_LEN                        0x000C
> > +#define DMA_FRAME_CNT                        0x0010
> > +#define DMA_START                    0x0024
> > +
> > +/* DMAx_MODE */
> > +#define DMA_MODE_ST(x)                  (((x) & 0x3) << 8)
> > +#define DMA_MODE_ST_DEV                 DMA_MODE_ST(0)
> > +#define DMA_MODE_DT(x)                  (((x) & 0x3) << 10)
> > +#define DMA_MODE_DT_DCU                 DMA_MODE_DT(2)
> > +#define DMA_MODE_SAM(x)                 (((x) & 0x3) << 16)
> > +#define DMA_MODE_SAM_CONST              DMA_MODE_SAM(0)
> > +#define DMA_MODE_DAM(x)                 (((x) & 0x3) << 18)
> > +#define DMA_MODE_DAM_INC                DMA_MODE_DAM(1)
> > +
> > +
> > +struct owl_mmc_plat {
> > +     struct mmc_config cfg;
> > +     struct mmc mmc;
> > +};
> > +
> > +struct owl_mmc_priv {
> > +     void *reg_base;
> > +     struct clk clk;
> > +     unsigned int clock;       /* Current clock */
> > +     unsigned int dma_drq;     /* Trigger Source */
> > +};
> > +
> > +static void owl_dma_config(void *dma_base, unsigned int drq,
> > +             unsigned int src, unsigned int dst,
> > +             unsigned int len)
> > +{
> > +     unsigned int mode = drq;
> > +     u8 frame_cnt = 0x1;
>
> Does it need to use frame_cnt variable?
>


> Ok, would remove it and use fixed value 0x1 for it.
> > +
> > +     /* Set Source and Destination adderess mode */
> > +     mode |= (DMA_MODE_ST_DEV | DMA_MODE_SAM_CONST | DMA_MODE_DT_DCU |
> > +                     DMA_MODE_DAM_INC);
> > +
> > +     writel(mode, dma_base + DMA_MODE);
> > +     writel(src, dma_base + DMA_SOURCE);
> > +     writel(dst, dma_base + DMA_DESTINATION);
> > +     writel(len, dma_base + DMA_FRAME_LEN);
> > +     writel(frame_cnt, dma_base + DMA_FRAME_CNT);
> > +}
> > +
> > +static void owl_mmc_prepare_data(struct owl_mmc_priv *priv,
> > +             struct mmc_data *data)
> > +{
> > +     unsigned int reg, total;
> > +     uint32_t buf = 0;
> > +
> > +     reg = readl(priv->reg_base + OWL_REG_SD_EN);
> > +     reg |= OWL_SD_EN_BSEL;
> > +     writel(reg, priv->reg_base + OWL_REG_SD_EN);
> > +
> > +     writel(data->blocks, priv->reg_base + OWL_REG_SD_BLK_NUM);
> > +     writel(data->blocksize, priv->reg_base + OWL_REG_SD_BLK_SIZE);
> > +     total = data->blocksize * data->blocks;
> > +
> > +     if (data->blocksize < 512)
> > +             writel(total, priv->reg_base + OWL_REG_SD_BUF_SIZE);
> > +     else
> > +             writel(512, priv->reg_base + OWL_REG_SD_BUF_SIZE);
>
> Is it right to set to 512?
>


> Actally, this is something picked from Linux driver as it.
>


> > +
> > +     /* DMA STOP */
> > +     writel(0x0, (DMA0_CHANNEL_BASE(2) + DMA_START));
> > +
> > +     if (data) {
> > +             if (data->flags == MMC_DATA_READ) {
> > +                     buf = (ulong) (data->dest);
> > +                     owl_dma_config((void *) DMA0_CHANNEL_BASE(2),
> > +                                     priv->dma_drq,
> > +                                     (ulong) priv->reg_base +
> > +                                     OWL_REG_SD_DAT,
> > +                                     buf, total);
> > +                     invalidate_dcache_range(buf, buf + total);
> > +             } else if (data->flags == MMC_DATA_WRITE) {
>
> Not need to use "else if", because it shoud be MMC_DATA_WRITE.
>

Ok, I would remove this "else if" statement.

>
> > +                     buf = (ulong) (data->src);
> > +                     owl_dma_config((void *) DMA0_CHANNEL_BASE(2),
> > +                                     priv->dma_drq,
> > +                                     buf,
> > +                                     (ulong) priv->reg_base +
> > +                                     OWL_REG_SD_DAT,
> > +                                     total);
> > +                     flush_dcache_range(buf, buf + total);
> > +             }
> > +             /* DMA START */
> > +             writel(0x1, (DMA0_CHANNEL_BASE(2) + DMA_START));
> > +     }
> > +}
> > +
> > +static int owl_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
> > +             struct mmc_data *data)
> > +{
> > +     struct owl_mmc_priv *priv = dev_get_priv(dev);
> > +     unsigned int cmd_rsp_mask, mode, reg;
> > +     int timeout = DATA_TRANSFER_TIMEOUT;
> > +
> > +     reg = readl(priv->reg_base + OWL_REG_SD_EN);
> > +     reg |= OWL_SD_ENABLE;
> > +     writel(reg, priv->reg_base + OWL_REG_SD_EN);
> > +
> > +     /* setup response */
> > +     switch (cmd->resp_type) {
> > +     case MMC_RSP_NONE:
> > +             break;
> > +     case MMC_RSP_R1:
> > +             if (data) {
> > +                     if (data->flags == MMC_DATA_READ)
> > +                             mode = OWL_SD_CTL_TM(4);
> > +                     else
> > +                             mode = OWL_SD_CTL_TM(5);
> > +             } else {
> > +                     mode = OWL_SD_CTL_TM(1);
> > +             }
> > +             cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;
> > +             break;
> > +     case MMC_RSP_R1b:
> > +             mode = OWL_SD_CTL_TM(3);
> > +             cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;
> > +             break;
> > +     case MMC_RSP_R2:
> > +             mode = OWL_SD_CTL_TM(2);
> > +             cmd_rsp_mask = OWL_SD_STATE_CLNR | OWL_SD_STATE_CRC7ER;
> > +             break;
> > +     case MMC_RSP_R3:
> > +             cmd->cmdarg = 0x40ff8000;
>
> I don't know why set to cmdarg at here?
>

Sure, this is not needed and is a leftover from debugging.

>
> > +             mode = OWL_SD_CTL_TM(1);
> > +             cmd_rsp_mask = OWL_SD_STATE_CLNR;
> > +             break;
> > +     default:
> > +             printf("error: no math command RSP flag %x\n",
> cmd->cmdarg);
> > +             return -1;
>
> Not use -1, instead use error number.
>

Ok

>
> > +     }
> > +
> > +     mode |= (readl(priv->reg_base + OWL_REG_SD_CTL) & (0xff << 16));
> > +
> > +     /* setup command */
> > +     writel(cmd->cmdidx, priv->reg_base + OWL_REG_SD_CMD);
> > +     writel(cmd->cmdarg, priv->reg_base + OWL_REG_SD_ARG);
> > +
> > +     /* Set LBE to send clk at the end of last read block */
> > +     if (data)
> > +             mode |= (OWL_SD_CTL_TS | OWL_SD_CTL_LBE | 0xE4000000);
> > +     else
> > +             mode |= OWL_SD_CTL_TS;
> > +
> > +     if (data)
> > +             owl_mmc_prepare_data(priv, data);
> > +
> > +     /* Start transfer */
> > +     writel(mode, priv->reg_base + OWL_REG_SD_CTL);
> > +
> > +     while ((readl(priv->reg_base + OWL_REG_SD_CTL) & OWL_SD_CTL_TS)
> > +                     && timeout--)
> > +             udelay(20);
>
> Use readl_poll_timeout()?
>
> > +
> > +     if (!timeout) {
> > +             printf("error: transferred data timeout\n");
> > +             return -ETIMEDOUT;
> > +     }
> > +
> > +     if (cmd->resp_type & MMC_RSP_136) {
> > +             cmd->response[3] = readl(priv->reg_base +
> OWL_REG_SD_RSPBUF0);
> > +             cmd->response[2] = readl(priv->reg_base +
> OWL_REG_SD_RSPBUF1);
> > +             cmd->response[1] = readl(priv->reg_base +
> OWL_REG_SD_RSPBUF2);
> > +             cmd->response[0] = readl(priv->reg_base +
> OWL_REG_SD_RSPBUF3);
> > +     } else {
> > +             u32 rsp[2];
> > +
> > +             rsp[0] = readl(priv->reg_base + OWL_REG_SD_RSPBUF0);
> > +             rsp[1] = readl(priv->reg_base + OWL_REG_SD_RSPBUF1);
> > +             cmd->response[0] = rsp[1] << 24 | rsp[0] >> 8;
> > +             cmd->response[1] = rsp[1] >> 8;
> > +     }
> > +
> > +     if (data) {
> > +             /* DMA STOP */
> > +             writel(0x0, (DMA0_CHANNEL_BASE(2) + DMA_START));
> > +             /* Transmission STOP */
> > +             while (readl(priv->reg_base + OWL_REG_SD_CTL) &
> OWL_SD_CTL_TS)
> > +                     clrbits_le32(priv->reg_base + OWL_REG_SD_CTL,
> > +                                     OWL_SD_CTL_TS);
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void owl_mmc_clk_set(struct owl_mmc_priv *priv, int rate)
> > +{
> > +     u32 reg;
> > +
> > +     reg = readl(priv->reg_base + OWL_REG_SD_CTL);
> > +     reg &= ~OWL_SD_CTL_DELAY_MSK;
> > +
> > +     /* Set RDELAY and WDELAY based on the clock */
> > +     if (rate <= 1000000) {
> > +             writel(reg | OWL_SD_CTL_RDELAY(OWL_SD_DELAY_LOW_CLK) |
> > +                     OWL_SD_CTL_WDELAY(OWL_SD_DELAY_LOW_CLK),
> > +                     priv->reg_base + OWL_REG_SD_CTL);
> > +     } else if ((rate > 1000000) && (rate <= 26000000)) {
> > +             writel(reg | OWL_SD_CTL_RDELAY(OWL_SD_DELAY_MID_CLK) |
> > +                     OWL_SD_CTL_WDELAY(OWL_SD_DELAY_MID_CLK),
> > +                     priv->reg_base + OWL_REG_SD_CTL);
> > +     } else if ((rate > 26000000) && (rate <= 52000000)) {
> > +             writel(reg | OWL_SD_CTL_RDELAY(OWL_SD_RDELAY_HIGH) |
> > +                     OWL_SD_CTL_WDELAY(OWL_SD_WDELAY_HIGH),
> > +                     priv->reg_base + OWL_REG_SD_CTL);
> > +     } else {
> > +             printf("SD clock rate not supported\n");
> > +     }
>
> I think that you can make more readable than now.
>
> if (rate <= 1000000) {
>         delay = OWL_SD_DELAY_LOW_CLK;
> } else if ( ...) {
>         delay = OWL_SD_DELAY_MID_CLK;
> } else if (....) {
>         delay = OWL_SD_DELAY_HIGH_CLK;
> }
>
> writel(reg | OWL_SD_CTRL_RDELAY(delay) | OWL_SD_CTL_WDELAY(delay)...);
>

Sure, would make this change.

>
>
>
> > +}
> > +
> > +static int owl_mmc_set_ios(struct udevice *dev)
> > +{
> > +     struct owl_mmc_priv *priv = dev_get_priv(dev);
> > +     struct owl_mmc_plat *plat = dev_get_platdata(dev);
> > +     struct mmc *mmc = &plat->mmc;
> > +     u32 reg, ret;
> > +
> > +     if (mmc->clock != priv->clock) {
> > +             priv->clock = mmc->clock;
> > +             owl_mmc_clk_set(priv, mmc->clock);
> > +     }
> > +
> > +     ret = clk_set_rate(&priv->clk, mmc->clock);
> > +     if (IS_ERR_VALUE(ret))
> > +             return ret;
> > +
> > +     ret = clk_enable(&priv->clk);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Set the Bus width */
> > +     reg = readl(priv->reg_base + OWL_REG_SD_EN);
> > +     reg &= ~0x03;
>
> Use macro, not 0x03.
>
> > +     if (mmc->bus_width == 8)
> > +             reg |= OWL_SD_EN_DATAWID(2);
> > +     else if (mmc->bus_width == 4)
> > +             reg |= OWL_SD_EN_DATAWID(1);
> > +
> > +     writel(reg, priv->reg_base + OWL_REG_SD_EN);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct dm_mmc_ops owl_mmc_ops = {
> > +     .send_cmd       = owl_mmc_send_cmd,
> > +     .set_ios        = owl_mmc_set_ios,
> > +};
> > +
> > +static int owl_mmc_probe(struct udevice *dev)
> > +{
> > +     struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
> > +     struct owl_mmc_plat *plat = dev_get_platdata(dev);
> > +     struct owl_mmc_priv *priv = dev_get_priv(dev);
> > +     struct mmc_config *cfg = &plat->cfg;
> > +     int bus_width, ret;
> > +     fdt_addr_t addr;
> > +
> > +     cfg->name = dev->name;
> > +     cfg->voltages = OWL_MMC_OCR;
> > +     cfg->f_min = 400000;
> > +     cfg->f_max = 52000000;
> > +     cfg->b_max = CONFIG_SYS_MMC_MAX_BLK_COUNT;
> > +
> > +     bus_width = dev_read_u32_default(dev, "bus-width", 1);
>
> Use mmc_of_parse(). It's generic parser.
>

Ok, Sure.

>
> Best Regards,
> Jaehoon Chung
>
>
> Thanks,
Amit


More information about the U-Boot mailing list