[PATCH V4 1/2] mmc: add OpenPiton mmc support
Tianrui Wei
tianrui-wei at outlook.com
Fri May 7 04:50:16 CEST 2021
Hi Sean,
Many thanks for taking your valuable time to review our patch and give
suggestions. I'll improve both our patches accordingly. Just for the
record, what would be the exact command you'd recommend to run
checkpatch.pl? Because it only showed one warning about me not putting
my name on the MAINTAINERS file on this patch.
Many thanks,
Tianrui
On 5/7/2021 10:43 AM, Sean Anderson wrote:
> On 5/5/21 11:40 PM, Tianrui Wei wrote:
>> From: Tianrui Wei <tianrui-wei at outlook.com>
>> Date: Thu, 6 May 2021 11:30:20 +0800
>> Subject: [PATCH V4 1/2] mmc: add OpenPiton mmc support
>>
>> This patch adds mmc support for OpenPiton.
>> Specifically, some dts bindings were not used because
>> our mmc controller doens't have those configuration
>> options, it only exposes a dummy mmap interface for
>> CPU.
>
> Please wrap your commit messages to 75 characterss, not 56 ;)
>
>>
>> Signed-off-by: Tianrui Wei <tianrui-wei at outlook.com>
>> Signed-off-by: Jonathan Balkind <jbalkind at ucsb.edu>
>> ---
>>
>> drivers/mmc/Kconfig | 7 +
>> drivers/mmc/Makefile | 1 +
>> drivers/mmc/piton_mmc.c | 172 +++++
>> 3 files changed, 180 insertions(+)
>> create mode 100644 drivers/mmc/piton_mmc.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index 14d79139..1038800f 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -707,6 +707,13 @@ config MMC_SUNXI_HAS_MODE_SWITCH
>> bool
>> depends on MMC_SUNXI
>> +config MMC_PITON
>> + bool "MMC support for openpiton SoC"
>> + depends on DM_MMC && BLK
>> + help
>> + This driver enables SD card support in U-Boot port for
>> + OpenPiton SoC
>> +
>> config GENERIC_ATMEL_MCI
>> bool "Atmel Multimedia Card Interface support"
>> depends on DM_MMC && BLK && ARCH_AT91
>> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
>> index 1c849cba..698dfe05 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -71,6 +71,7 @@ obj-$(CONFIG_MMC_SDHCI_XENON) += xenon_sdhci.o
>> obj-$(CONFIG_MMC_SDHCI_ZYNQ) += zynq_sdhci.o
>> obj-$(CONFIG_MMC_SUNXI) += sunxi_mmc.o
>> +obj-$(CONFIG_MMC_PITON) += piton_mmc.o
>
> Fix indentation. And please place this object in alphabetical order.
>
>> obj-$(CONFIG_MMC_UNIPHIER) += tmio-common.o uniphier-sd.o
>> obj-$(CONFIG_RENESAS_SDHI) += tmio-common.o renesas-sdhi.o
>> obj-$(CONFIG_MMC_BCM2835) += bcm2835_sdhost.o
>> diff --git a/drivers/mmc/piton_mmc.c b/drivers/mmc/piton_mmc.c
>> new file mode 100644
>> index 00000000..266e26d8
>> --- /dev/null
>> +++ b/drivers/mmc/piton_mmc.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2009 SAMSUNG Electronics
>> + * Minkyu Kang <mk7.kang at samsung.com>
>> + * Jaehoon Chung <jh80.chung at samsung.com>
>> + * Portions Copyright 2011-2019 NVIDIA Corporation
>> + * Portions Copyright 2021 Tianrui Wei
>> + * Tianrui Wei <tianrui-wei at outlook.com>
>
> This is quite the list of authors. Where does this file come from?
This file was adapted from a Samsung mmc file, so I kept the original
authors.
>
>> + */
>> +
>> +#include <asm/gpio.h>
>> +#include <asm/io.h>
>> +#include <common.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <linux/bitops.h>
>> +#include <linux/types.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <log.h>
>> +#include <div64.h>
>> +#include <mmc.h>
>> +
>> +struct piton_mmc_plat {
>> + struct mmc_config cfg;
>> + struct mmc mmc;
>> +};
>
> Is platdata used? I don't see it in 2/2.
>
>> +
>> +struct piton_mmc_priv {
>> + u64 piton_mmc_base_addr; /* peripheral id */
>
> This should be void __iomem *.
>
>> +};
>> +
>> +/*
>> + * see mmc_read_blocks to see how it is used.
>> + * start block is hidden at cmd->arg
>> + * also, initialize the block size at init
>> + */
>> +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd,
>> + struct mmc_data *data)
>> +{
>> + /* check first if this is a pure command */
>> + if (!data)
>> + return 0;
>
> Please place definitions before code (and please run checkpatch).
>
>> +
>> + u64 byte_cnt = data->blocks * data->blocksize;
>> + u64 start_block = cmd->cmdarg;
>> + unsigned int *buff = (unsigned int *)data->dest;
>
> Why is buff an unsigned int? If you are using readl, shouldn't it be a
> long?
>
>> +
>> + struct piton_mmc_priv *priv = dev_get_priv(dev);
>> + u64 start_addr = priv->piton_mmc_base_addr + (start_block);
>> +
>> + /* if there is a read */
>> + if (data->flags & MMC_DATA_READ) {
>> + for (u64 i = 0; i < byte_cnt; i += 4) {
>> + *(buff) = readl((void *)(start_addr + i));
>> + buff++;
>> + }
>
> Can you use memcpy_fromio here?
>
>> + } else {
>> + /* else there is a write
>> + * we don't handle write, so error right away
>> + */
>> + return -ENODEV;
>
> Please use -ENOSYS.
>
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int piton_mmc_ofdata_to_platdata(struct udevice *dev)
>> +{
>> + struct piton_mmc_priv *priv = dev_get_priv(dev);
>> + struct piton_mmc_plat *plat = dev_get_platdata(dev);
>> + struct mmc_config *cfg;
>> + struct mmc *mmc;
>> + /* fill in device description */
>> + struct blk_desc *bdesc;
>> +
>> + priv->piton_mmc_base_addr = dev_read_addr(dev);
>> + cfg = &plat->cfg;
>> + cfg->name = "PITON MMC";
>> + cfg->host_caps = MMC_MODE_8BIT;
>> + cfg->f_max = 100000;
>> + cfg->f_min = 400000;
>> + cfg->voltages = MMC_VDD_21_22;
>> +
>> + mmc = &plat->mmc;
>> + mmc->read_bl_len = MMC_MAX_BLOCK_LEN;
>> + mmc->capacity_user = 0x100000000;
>> + mmc->capacity_user *= mmc->read_bl_len;
>> + mmc->capacity_boot = 0;
>> + mmc->capacity_rpmb = 0;
>> + for (int i = 0; i < 4; i++)
>> + mmc->capacity_gp[i] = 0;
>> + mmc->capacity = 0x2000000000ULL;
>> + mmc->has_init = 1;
>> +
>> + bdesc = mmc_get_blk_desc(mmc);
>> + bdesc->lun = 0;
>> + bdesc->hwpart = 0;
>> + bdesc->type = 0;
>> + bdesc->blksz = mmc->read_bl_len;
>> + bdesc->log2blksz = LOG2(bdesc->blksz);
>> + bdesc->lba = lldiv(mmc->capacity, mmc->read_bl_len);
>> +
>> + return 0;
>> +}
>> +
>> +/* test if piton has the micro mmc card present
>> + * always return 1, which means present
>> + */
>> +static int piton_mmc_getcd(struct udevice *dev)
>> +{
>> + /*
>> + * always return 1
>> + */
>> + return 1;
>> +}
>> +
>> +/* dummy function, piton_mmc don't need initialization
>> + * in hw
>> + */
>> +static const struct dm_mmc_ops piton_mmc_ops = {
>> + .send_cmd = piton_mmc_send_cmd,
>> + .set_ios = piton_mmc_set_ios,
>> + .get_cd = piton_mmc_getcd,
>> +};
>> +
>> +static int piton_mmc_probe(struct udevice *dev)
>> +{
>> + struct mmc_uclass_priv *upriv = dev_get_uclass_priv(dev);
>> + struct piton_mmc_plat *plat = dev_get_platdata(dev);
>> + struct mmc_config *cfg = &plat->cfg;
>> +
>> + cfg->name = dev->name;
>> + upriv->mmc = &plat->mmc;
>> + upriv->mmc->has_init = 1;
>> + upriv->mmc->capacity = 0x2000000000ULL;
>> + upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN;
>> +
>> + return 0;
>> +}
>> +
>> +static int piton_mmc_bind(struct udevice *dev)
>> +{
>> + struct piton_mmc_plat *plat = dev_get_platdata(dev);
>> + struct mmc_config *cfg = &plat->cfg;
>> +
>> + cfg->name = dev->name;
>> + cfg->host_caps = MMC_MODE_HS_52MHz | MMC_MODE_HS | MMC_MODE_8BIT;
>> + cfg->voltages = MMC_VDD_165_195 | MMC_VDD_32_33 | MMC_VDD_33_34;
>> + cfg->f_min = 1000000;
>> + cfg->f_max = 52000000;
>> + cfg->b_max = U32_MAX;
>> +
>> + return mmc_bind(dev, &plat->mmc, cfg);
>> +}
>> +
>> +static const struct udevice_id piton_mmc_ids[] = {
>> + {.compatible = "openpiton,piton-mmc"},
>> + { /* sentinel */ }
>> +};
>> +
>> +U_BOOT_DRIVER(piton_mmc_drv) = {
>> + .name = "piton_mmc",
>> + .id = UCLASS_MMC,
>> + .of_match = piton_mmc_ids,
>> + .ofdata_to_platdata = piton_mmc_ofdata_to_platdata,
>> + .bind = piton_mmc_bind,
>> + .probe = piton_mmc_probe,
>> + .ops = &piton_mmc_ops,
>> + .platdata_auto_alloc_size = sizeof(struct piton_mmc_plat),
>> + .priv_auto_alloc_size = sizeof(struct piton_mmc_priv),
>> +};
>>
>
> --Sean
More information about the U-Boot
mailing list