Re: [PATCH V5 1/2] mmc: add OpenPiton mmc support
On 6/6/21 12:57 PM, Tianrui Wei wrote: Dear Sean, Many thanks for taking the time and energy to review our patches ( again ) And sorry about not responding or not implementing them. I'll go through them one by one and try to doing so in the future :P On 6/7/21 12:34 AM, Sean Anderson wrote: On 6/4/21 12:52 AM, Tianrui Wei wrote: From: Tianrui Wei Date: Fri, 4 June 2021 12:45:29 +0800 Subject: [PATCH v5 1/2] mmc: add OpenPiton mmc support This patch adds mmc support for OpenPiton board. Changelog: v5: move definitions around, changed some incompatible type information Signed-off-by: Tianrui Wei Signed-off-by: Jonathan Balkind --- drivers/mmc/Kconfig | 7 + drivers/mmc/Makefile| 1 + drivers/mmc/piton_mmc.c | 174 + 3 files changed, 182 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 We already know it's for U-Boot ;) :P +OpenPiton SoC Please add a bit more than this. In particular, it would be useful to note that this is an "emulated" device where the host does all of the work. Will do :P + 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 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 ..2b2d5756 --- /dev/null +++ b/drivers/mmc/piton_mmc.c @@ -0,0 +1,174 @@ +// 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 + * This file is adapted from tegra_mmc.c + * Tianrui Wei + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct piton_mmc_plat { +struct mmc_config cfg; +struct mmc mmc; +}; + +struct piton_mmc_priv { +u64 piton_mmc_base_addr; /* peripheral id */ Again, please use void __iomem *. Sorry, forgot this change :( +}; + +/* + * see mmc_read_blocks to see how it is used. + * start block is hidden at cmd->arg Hidden? I wrote this comment a long time ago, I'll comment it out. + * also, initialize the block size at init + */ +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, + struct mmc_data *data) Nit: alignment I'm not sure how to align this? Should I align the struct in the first line and the second line? It appears so in my email client :P In your original email, this second line has 7 tabs, whereas the line above it has 29 characters before the `(`. I'm not sure why it's aligned in my reply. You can see the misaligned issue if you view your patch in patchwork. +{ +/* check first if this is a pure command */ +if (!data) +return 0; Variable declarations come before code. Will do :P + +u64 byte_cnt, start_block, start_addr; I think these should be size_t, since you use it for pointer arithmetic. Will do :P + +struct piton_mmc_priv *priv = dev_get_priv(dev); +unsigned long *buff = (unsigned long *)data->dest; + +start_addr = priv->piton_mmc_base_addr + (start_block); +byte_cnt = data->blocks * data->blocksize; What should happen if (byte_cnt & 3)? The 1th or 3rd byte will be read into buf, Ah, disregard this; presumably blocksize will always be a (large enough) power of two. +start_block = cmd->cmdarg; + +/* if there is a read */ +if (data->flags & MMC_DATA_READ) { +for (u64 i = 0; i < byte_cnt; i += 4) { Declare i with the rest of the variables. +*(buff) = readl((void *)(start_addr + i)); I think this could be rewritten as size_t byte_cnt; u32 *buff, *start_addr; /* ... */ for (byte_cnt = /* ... */; byte_cnt; byte_cnt -= sizeof(u32)); *buff++ = readl(start_addr++); Which more closely follow
Re: [PATCH V5 1/2] mmc: add OpenPiton mmc support
Dear Sean, Many thanks for taking the time and energy to review our patches ( again ) And sorry about not responding or not implementing them. I'll go through them one by one and try to doing so in the future :P On 6/7/21 12:34 AM, Sean Anderson wrote: > On 6/4/21 12:52 AM, Tianrui Wei wrote: >> From: Tianrui Wei >> Date: Fri, 4 June 2021 12:45:29 +0800 >> Subject: [PATCH v5 1/2] mmc: add OpenPiton mmc support >> >> This patch adds mmc support for OpenPiton board. >> >> Changelog: >> >> v5: move definitions around, changed some incompatible type information >> >> Signed-off-by: Tianrui Wei >> Signed-off-by: Jonathan Balkind >> --- >> >> drivers/mmc/Kconfig | 7 + >> drivers/mmc/Makefile | 1 + >> drivers/mmc/piton_mmc.c | 174 + >> 3 files changed, 182 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 > > We already know it's for U-Boot ;) :P > >> + OpenPiton SoC > > Please add a bit more than this. In particular, it would be useful to > note that this is an "emulated" device where the host does all of the > work. Will do :P > >> + >> 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 >> 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 ..2b2d5756 >> --- /dev/null >> +++ b/drivers/mmc/piton_mmc.c >> @@ -0,0 +1,174 @@ >> +// 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 >> + * This file is adapted from tegra_mmc.c >> + * Tianrui Wei >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +struct piton_mmc_plat { >> + struct mmc_config cfg; >> + struct mmc mmc; >> +}; >> + >> +struct piton_mmc_priv { >> + u64 piton_mmc_base_addr; /* peripheral id */ > > Again, please use void __iomem *. Sorry, forgot this change :( > >> +}; >> + >> +/* >> + * see mmc_read_blocks to see how it is used. >> + * start block is hidden at cmd->arg > > Hidden? I wrote this comment a long time ago, I'll comment it out. > >> + * also, initialize the block size at init >> + */ >> +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, >> + struct mmc_data *data) > > Nit: alignment I'm not sure how to align this? Should I align the struct in the first line and the second line? It appears so in my email client :P > >> +{ >> + /* check first if this is a pure command */ >> + if (!data) >> + return 0; > > Variable declarations come before code. Will do :P > >> + >> + u64 byte_cnt, start_block, start_addr; > > I think these should be size_t, since you use it for pointer > arithmetic. Will do :P > >> + >> + struct piton_mmc_priv *priv = dev_get_priv(dev); >> + unsigned long *buff = (unsigned long *)data->dest; >> + >> + start_addr = priv->pi
Re: [PATCH V5 1/2] mmc: add OpenPiton mmc support
On 6/4/21 12:52 AM, Tianrui Wei wrote: From: Tianrui Wei Date: Fri, 4 June 2021 12:45:29 +0800 Subject: [PATCH v5 1/2] mmc: add OpenPiton mmc support This patch adds mmc support for OpenPiton board. Changelog: v5: move definitions around, changed some incompatible type information Signed-off-by: Tianrui Wei Signed-off-by: Jonathan Balkind --- drivers/mmc/Kconfig | 7 + drivers/mmc/Makefile| 1 + drivers/mmc/piton_mmc.c | 174 + 3 files changed, 182 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 We already know it's for U-Boot ;) +OpenPiton SoC Please add a bit more than this. In particular, it would be useful to note that this is an "emulated" device where the host does all of the work. + 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 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 ..2b2d5756 --- /dev/null +++ b/drivers/mmc/piton_mmc.c @@ -0,0 +1,174 @@ +// 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 + * This file is adapted from tegra_mmc.c + * Tianrui Wei + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct piton_mmc_plat { + struct mmc_config cfg; + struct mmc mmc; +}; + +struct piton_mmc_priv { + u64 piton_mmc_base_addr; /* peripheral id */ Again, please use void __iomem *. +}; + +/* + * see mmc_read_blocks to see how it is used. + * start block is hidden at cmd->arg Hidden? + * also, initialize the block size at init + */ +static int piton_mmc_send_cmd(struct udevice *dev, struct mmc_cmd *cmd, + struct mmc_data *data) Nit: alignment +{ + /* check first if this is a pure command */ + if (!data) + return 0; Variable declarations come before code. + + u64 byte_cnt, start_block, start_addr; I think these should be size_t, since you use it for pointer arithmetic. + + struct piton_mmc_priv *priv = dev_get_priv(dev); + unsigned long *buff = (unsigned long *)data->dest; + + start_addr = priv->piton_mmc_base_addr + (start_block); + byte_cnt = data->blocks * data->blocksize; What should happen if (byte_cnt & 3)? + start_block = cmd->cmdarg; + + /* if there is a read */ + if (data->flags & MMC_DATA_READ) { + for (u64 i = 0; i < byte_cnt; i += 4) { Declare i with the rest of the variables. + *(buff) = readl((void *)(start_addr + i)); I think this could be rewritten as size_t byte_cnt; u32 *buff, *start_addr; /* ... */ for (byte_cnt = /* ... */; byte_cnt; byte_cnt -= sizeof(u32)); *buff++ = readl(start_addr++); Which more closely follows the model of memcpy. Speaking of which, I thought you were going to use it: Scratch the previous email, I stand corrected. You're right about memcpy could work, thanks for the great suggestion :P What happened to that? + buff++; + } + } else { + /* else there is a write +* we don't handle write, so error right away +*/ + return -ENODEV; Please use -ENOSYS. See [1] for details. In general, several of my comments on your previous patch are still unaddressed. I would like if you could either respond to them with your reasoning for not addressing them, or implement them. --Sean [1] https://u-boot.readthedocs.io/en/latest/develop/driver-model/design.html#error-codes + } + + return 0; +} + +static int pi
[PATCH V5 1/2] mmc: add OpenPiton mmc support
From: Tianrui Wei Date: Fri, 4 June 2021 12:45:29 +0800 Subject: [PATCH v5 1/2] mmc: add OpenPiton mmc support This patch adds mmc support for OpenPiton board. Changelog: v5: move definitions around, changed some incompatible type information Signed-off-by: Tianrui Wei Signed-off-by: Jonathan Balkind --- drivers/mmc/Kconfig | 7 + drivers/mmc/Makefile| 1 + drivers/mmc/piton_mmc.c | 174 + 3 files changed, 182 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 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 ..2b2d5756 --- /dev/null +++ b/drivers/mmc/piton_mmc.c @@ -0,0 +1,174 @@ +// 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 + * This file is adapted from tegra_mmc.c + * Tianrui Wei + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +struct piton_mmc_plat { + struct mmc_config cfg; + struct mmc mmc; +}; + +struct piton_mmc_priv { + u64 piton_mmc_base_addr; /* peripheral id */ +}; + +/* + * 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; + + u64 byte_cnt, start_block, start_addr; + + struct piton_mmc_priv *priv = dev_get_priv(dev); + unsigned long *buff = (unsigned long *)data->dest; + + start_addr = priv->piton_mmc_base_addr + (start_block); + byte_cnt = data->blocks * data->blocksize; + start_block = cmd->cmdarg; + + /* 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++; + } + } else { + /* else there is a write +* we don't handle write, so error right away +*/ + return -ENODEV; + } + + 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 = >cfg; + cfg->name = "PITON MMC"; + cfg->host_caps = MMC_MODE_8BIT; + cfg->f_max = 10; + cfg->f_min = 40; + cfg->voltages = MMC_VDD_21_22; + + mmc = >mmc; + mmc->read_bl_len = MMC_MAX_BLOCK_LEN; + mmc->capacity_user = 0x1; + 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 = 0x20ULL; + 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); +