From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sean Anderson Date: Thu, 6 May 2021 22:55:34 -0400 Subject: [PATCH V4 1/2] mmc: add OpenPiton mmc support In-Reply-To: References: <51c6e20e-4ddc-4bd3-b6b7-98e1be659d6d@gmail.com> Message-ID: <1bb02945-322a-2e20-982d-47a9a79ecb21@gmail.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 5/6/21 10:50 PM, Tianrui Wei wrote: > 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. Huh. I would have expected checkpatch to catch the variable initialization thing, but I guess it didn't. > > > 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 >>> 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 >>> Signed-off-by: Jonathan Balkind >>> --- >>> >>> 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 >>> + * Jaehoon Chung >>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>> + * Portions Copyright 2021 Tianrui Wei >>> + * Tianrui Wei >> >> 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. Can you add a note along the lines of "Adapted from tegra_mmc.c" for the curiosity of future readers :) --Sean > >> >>> + */ >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +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