On 23/12/2020 04:25, Jaehoon Chung wrote:
> On 12/23/20 11:22 AM, Amit Tomer wrote:
>> On Wed, Dec 23, 2020 at 5:57 AM André Przywara <andre.przyw...@arm.com>
>> wrote:
>>>
>>> On 19/12/2020 14:51, Amit Singh Tomar wrote:
>>>> From: Amit Singh Tomar <amittome...@gmail.com>
>>>>
>>>> This commit adds support for MMC controllers found on Actions OWL
>>>> S700 SoC platform.
>>>>
>>>> Signed-off-by: Amit Singh Tomar <amittome...@gmail.com>
>>>> ---
>>>> Changes since previous version
>>>> * Corrected block count to 512.
>>>> * Changed the command timeout value to 30ms.
>>>> * Used readl_poll_timeout.
>>>> * Read DMA parameters from DT instead of hardcoding it.
>>>> * Reduced number of arguments passed to own_dma_cofig.
>>>> * Removed debug leftover.
>>>> * Used mmc_of_parse().
>>>> ---
>>>> drivers/mmc/Kconfig | 7 +
>>>> drivers/mmc/Makefile | 1 +
>>>> drivers/mmc/owl_mmc.c | 399
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 407 insertions(+)
>>>> create mode 100644 drivers/mmc/owl_mmc.c
>>>>
>>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>>> index 14d7913..61f9c67 100644
>>>> --- a/drivers/mmc/Kconfig
>>>> +++ b/drivers/mmc/Kconfig
>>>> @@ -289,6 +289,13 @@ config MMC_MXC
>>>>
>>>> If unsure, say N.
>>>>
>>>> +config MMC_OWL
>>>> + bool "Actions OWL Multimedia Card Interface support"
>>>> + depends on ARCH_OWL && DM_MMC && BLK
>>>> + help
>>>> + This selects the OWL SD/MMC host controller found on board
>>>> + based on Actions S700 SoC.
>>>
>>> And S900 as well?
>>>
>>>> +
>>>> config MMC_MXS
>>>> bool "Freescale MXS Multimedia Card Interface support"
>>>> depends on MX23 || MX28 || MX6 || MX7
>>>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>>>> index 1c849cb..f270f6c 100644
>>>> --- a/drivers/mmc/Makefile
>>>> +++ b/drivers/mmc/Makefile
>>>> @@ -38,6 +38,7 @@ obj-$(CONFIG_MMC_OMAP_HS) += omap_hsmmc.o
>>>> obj-$(CONFIG_MMC_MXC) += mxcmmc.o
>>>> obj-$(CONFIG_MMC_MXS) += mxsmmc.o
>>>> obj-$(CONFIG_MMC_OCTEONTX) += octeontx_hsmmc.o
>>>> +obj-$(CONFIG_MMC_OWL) += owl_mmc.o
>>>> obj-$(CONFIG_MMC_PCI) += pci_mmc.o
>>>> obj-$(CONFIG_PXA_MMC_GENERIC) += pxa_mmc_gen.o
>>>> obj-$(CONFIG_$(SPL_TPL_)SUPPORT_EMMC_RPMB) += rpmb.o
>>>> diff --git a/drivers/mmc/owl_mmc.c b/drivers/mmc/owl_mmc.c
>>>> new file mode 100644
>>>> index 0000000..5c48307
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/owl_mmc.c
>>>> @@ -0,0 +1,399 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> +/*
>>>> + * Copyright (C) 2020 Amit Singh Tomar <amittome...@gmail.com>
>>>> + *
>>>> + * Driver for SD/MMC controller present on Actions Semi S700 SoC, based
>>>> + * on Linux Driver "drivers/mmc/host/owl-mmc.c".
>>>> + *
>>>> + * Though, there is a bit (BSEL, BUS or DMA Special Channel Selection)
>>>> that
>>>> + * controls the data transfer from SDx_DAT register either using CPU AHB
>>>> Bus
>>>> + * or DMA channel, but seems like, it only works correctly using external
>>>> DMA
>>>> + * channel, and those special bits used in this driver is picked from
>>>> vendor
>>>> + * source exclusively for MMC/SD.
>>>> + */
>>>> +#include <common.h>
>>>> +#include <clk.h>
>>>> +#include <cpu_func.h>
>>>> +#include <dm.h>
>>>> +#include <errno.h>
>>>> +#include <log.h>
>>>> +#include <mmc.h>
>>>> +#include <asm/io.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/iopoll.h>
>>>> +
>>>> +/*
>>>> + * SDC registers
>>>> + */
>>>> +#define OWL_REG_SD_EN 0x0000
>>>> +#define OWL_REG_SD_CTL 0x0004
>>>> +#define OWL_REG_SD_STATE 0x0008
>>>> +#define OWL_REG_SD_CMD 0x000c
>>>> +#define OWL_REG_SD_ARG 0x0010
>>>> +#define OWL_REG_SD_RSPBUF0 0x0014
>>>> +#define OWL_REG_SD_RSPBUF1 0x0018
>>>> +#define OWL_REG_SD_RSPBUF2 0x001c
>>>> +#define OWL_REG_SD_RSPBUF3 0x0020
>>>> +#define OWL_REG_SD_RSPBUF4 0x0024
>>>> +#define OWL_REG_SD_DAT 0x0028
>>>> +#define OWL_REG_SD_BLK_SIZE 0x002c
>>>> +#define OWL_REG_SD_BLK_NUM 0x0030
>>>> +#define OWL_REG_SD_BUF_SIZE 0x0034
>>>> +
>>>> +/* SD_EN Bits */
>>>> +#define OWL_SD_EN_RANE BIT(31)
>>>> +#define OWL_SD_EN_RESE BIT(10)
>>>> +#define OWL_SD_ENABLE BIT(7)
>>>> +#define OWL_SD_EN_BSEL BIT(6)
>>>> +#define OWL_SD_EN_DATAWID(x) (((x) & 0x3) << 0)
>>>> +#define OWL_SD_EN_DATAWID_MASK 0x03
>>>> +
>>>> +/* SD_CTL Bits */
>>>> +#define OWL_SD_CTL_TOUTEN BIT(31)
>>>> +#define OWL_SD_CTL_DELAY_MSK GENMASK(23, 16)
>>>> +#define OWL_SD_CTL_RDELAY(x) (((x) & 0xf) << 20)
>>>> +#define OWL_SD_CTL_WDELAY(x) (((x) & 0xf) << 16)
>>>> +#define OWL_SD_CTL_TS BIT(7)
>>>> +#define OWL_SD_CTL_LBE BIT(6)
>>>> +#define OWL_SD_CTL_TM(x) (((x) & 0xf) << 0)
>>>> +
>>>> +#define OWL_SD_DELAY_LOW_CLK 0x0f
>>>> +#define OWL_SD_DELAY_MID_CLK 0x0a
>>>> +#define OWL_SD_RDELAY_HIGH 0x08
>>>> +#define OWL_SD_WDELAY_HIGH 0x09
>>>
>>> w/s? Here and elsewhere. I would use tabs everywhere instead.
>>>
>>>> +
>>>> +/* SD_STATE Bits */
>>>> +#define OWL_SD_STATE_DAT0S BIT(7)
>>>> +#define OWL_SD_STATE_CLNR BIT(4)
>>>> +#define OWL_SD_STATE_CRC7ER BIT(0)
>>>> +
>>>> +#define OWL_MMC_OCR (MMC_VDD_32_33 | MMC_VDD_33_34 | \
>>>> + MMC_VDD_165_195)
>>>> +
>>>> +#define DATA_TRANSFER_TIMEOUT 30000
>>>> +
>>>> +/*
>>>> + * Simple DMA transfer operations defines for MMC/SD card
>>>> + */
>>>> +#define SD_DMA_CHANNEL(base, channel) ((base) + 0x100 * (channel))
>>>> +
>>>> +#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;
>>>> + void *dma_channel;
>>>> + struct clk clk;
>>>> + unsigned int clock; /* Current clock */
>>>> + unsigned int dma_drq; /* Trigger Source */
>>>> +};
>>>> +
>>>> +static void owl_dma_config(struct owl_mmc_priv *priv, unsigned int src,
>>>> + unsigned int dst, unsigned int len)
>>>> +{
>>>> + unsigned int mode = priv->dma_drq;
>>>> +
>>>> + /* 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, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_MODE);
>>>> + writel(src, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_SOURCE);
>>>> + writel(dst, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_DESTINATION);
>>>> + writel(len, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_FRAME_LEN);
>>>> + writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_FRAME_CNT);
>>>> +}
>>>> +
>>>> +static void owl_mmc_prepare_data(struct owl_mmc_priv *priv,
>>>> + struct mmc_data *data)
>>>> +{
>>>> + unsigned int reg, total;
>>>> + u32 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);
>>>> +
>>>> + /* DMA STOP */
>>>> + writel(0x0, SD_DMA_CHANNEL(priv->dma_channel, 0) + DMA_START);
>>>> +
>>>> + if (data) {
>>>> + if (data->flags == MMC_DATA_READ) {
>>>> + buf = (ulong) (data->dest);
>>>> + owl_dma_config(priv, (ulong) priv->reg_base +
>>>> + OWL_REG_SD_DAT, buf, total);
>>>> + invalidate_dcache_range(buf, buf + total);
>>>> + } else {
>>>> + buf = (ulong) (data->src);
>>>> + owl_dma_config(priv, buf, (ulong) priv->reg_base +
>>>> + OWL_REG_SD_DAT, total);
>>>> + flush_dcache_range(buf, buf + total);
>>>> + }
>>>> + /* DMA START */
>>>> + writel(0x1, SD_DMA_CHANNEL(priv->dma_channel, 0) +
>>>> 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 ret;
>>>> +
>>>> + 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:
>>>> + mode = OWL_SD_CTL_TM(1);
>>>> + cmd_rsp_mask = OWL_SD_STATE_CLNR;
>>>> + break;
>>>> + default:
>>>> + debug("Unknown MMC command\n");
>>>
>>> "Unsupported MMC response type"
>>> And I wonder if you should define MMC_RSP_R7 as well.
>>>
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + 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);
>>>> +
>>>> + ret = readl_poll_timeout(priv->reg_base + OWL_REG_SD_CTL, reg,
>>>> + !(reg & OWL_SD_CTL_TS),
>>>> DATA_TRANSFER_TIMEOUT);
>>>> +
>>>> + if (ret == -ETIMEDOUT) {
>>>> + debug("error: transferred data timeout\n");
>>>> + return ret;
>>>> + }
>>>> +
>>>> + 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, SD_DMA_CHANNEL(priv->dma_channel, 0) +
>>>> 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);
>>>
>>> Can you please have a variable "delay", and set just that to
>>> OWL_SD_DELAY_{LOW,MID,HIGH}_CLK, respectively? And then have one
>>> writel() call instead?
>>
>> But for HIGH Clock, delay values for read and write are different
>> unlike LOW and MID.
>
> I had already mentioned about making more readable than now.
>
> if (rate <= 1000000) {
> rdelay = wdelay = OWL_SD_DELAY_LOW_CLK;
> } else if ( ...) {
> rdelay = wdelay = OWL_SD_DELAY_MID_CLK;
> } else if (....) {
> rdelay = OWL_SD_RDELAY_HIGH;
> wdelay = OWL_SD_WDELAY_HIGH;
> }
>
> writel(reg | OWL_SD_CTRL_RDELAY(rdelay) | OWL_SD_CTL_WDELAY(wdelay)...);
Yes, this is what I had in mind as well.
> There are many approach to make readable..but Amit mentioned it's using same
> code in Linux kernel driver.
>
> Well, i don't know what's good or not..But i don't understand that "kernel
> drier is using" is a reason of one.
> While last few years, i couldn't check u-boot mailing..So I don't know that
> it's u-boot policy or not.
> If it's u-boot policy, i will also rework kernel driver to change.
I think the idea is to piggy back on the experience and testing levels
of Linux, and to be able to copy fixes. So to not deviate too much from
the kernel driver. But we can't be 100% compatible anyway, because the
U-Boot frameworks are different, we don't use IRQs, higher speed modes,
and can't make use of some fancy Linux features (like asynchronous I/O
handling).
Which means we will never be automatically "patch compatible".
So I appreciate that we try to stay as close to the Linux driver as
possible, but that surely should focus on the structure and ideas. So we
re-use this if-cascade, the clock and delay values. But not making this
more readable because "Linux does it" is not a good excuse, IMHO.
Cheers,
Andre
