| Message ID | SY4PR01MB6798923E35FB6262EEBF2BC0F6589@SY4PR01MB6798.ausprd01.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Andes |
| Headers | show |
| Series | Add OpenPiton board support | expand |
On 5/5/21 11:40 PM, Tianrui Wei wrote: > From: Tianrui Wei <tianrui-wei@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@outlook.com> > Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> > + * Jaehoon Chung <jh80.chung@samsung.com> > + * Portions Copyright 2011-2019 NVIDIA Corporation > + * Portions Copyright 2021 Tianrui Wei > + * Tianrui Wei <tianrui-wei@outlook.com> This is quite the list of authors. Where does this file come from? > + */ > + > +#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
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@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@outlook.com> >> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >> + * Jaehoon Chung <jh80.chung@samsung.com> >> + * Portions Copyright 2011-2019 NVIDIA Corporation >> + * Portions Copyright 2021 Tianrui Wei >> + * Tianrui Wei <tianrui-wei@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
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 <tianrui-wei@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@outlook.com> >>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>> + * Jaehoon Chung <jh80.chung@samsung.com> >>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>> + * Portions Copyright 2021 Tianrui Wei >>> + * Tianrui Wei <tianrui-wei@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. Can you add a note along the lines of "Adapted from tegra_mmc.c" for the curiosity of future readers :) --Sean > >> >>> + */ >>> + >>> +#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
Hi Sean, Thanks for reviewing the patches, I'll fix the initialization thing and the tegra_mmc credit :P On 5/7/2021 10:55 AM, Sean Anderson wrote: > 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 <tianrui-wei@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@outlook.com> >>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>> + * Portions Copyright 2021 Tianrui Wei >>>> + * Tianrui Wei <tianrui-wei@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. > > Can you add a note along the lines of "Adapted from tegra_mmc.c" for the > curiosity of future readers :) > > --Sean > >> >>> >>>> + */ >>>> + >>>> +#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 >
On 5/6/21 10:57 PM, Tianrui Wei wrote: > Hi Sean, > > > Thanks for reviewing the patches, I'll fix the initialization thing and the tegra_mmc credit :P Please also add a changelog to your next revision. It helps reviewers know what you have fixed. --Sean > > On 5/7/2021 10:55 AM, Sean Anderson wrote: >> 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 <tianrui-wei@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@outlook.com> >>>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>>> + * Portions Copyright 2021 Tianrui Wei >>>>> + * Tianrui Wei <tianrui-wei@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. >> >> Can you add a note along the lines of "Adapted from tegra_mmc.c" for the >> curiosity of future readers :) >> >> --Sean >> >>> >>>> >>>>> + */ >>>>> + >>>>> +#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 >>
On 5/7/2021 10:59 AM, Sean Anderson wrote: > On 5/6/21 10:57 PM, Tianrui Wei wrote: >> Hi Sean, >> >> >> Thanks for reviewing the patches, I'll fix the initialization thing >> and the tegra_mmc credit :P > > Please also add a changelog to your next revision. It helps reviewers > know what you have fixed. > > --Sean > >> >> On 5/7/2021 10:55 AM, Sean Anderson wrote: >>> 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 <tianrui-wei@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@outlook.com> >>>>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>>>> + * Portions Copyright 2021 Tianrui Wei >>>>>> + * Tianrui Wei <tianrui-wei@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. >>> >>> Can you add a note along the lines of "Adapted from tegra_mmc.c" for >>> the >>> curiosity of future readers :) >>> >>> --Sean >>> >>>> >>>>> >>>>>> + */ >>>>>> + >>>>>> +#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? I went back and double check, seems that it only applies for board with PCI configured >>>>> >>>>>> + } 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 >>> > Thanks, Tianrui
On 5/8/21 1:27 PM, Tianrui Wei wrote: > > On 5/7/2021 10:59 AM, Sean Anderson wrote: >> On 5/6/21 10:57 PM, Tianrui Wei wrote: >>> Hi Sean, >>> >>> >>> Thanks for reviewing the patches, I'll fix the initialization thing and the tegra_mmc credit :P >> >> Please also add a changelog to your next revision. It helps reviewers >> know what you have fixed. >> >> --Sean >> >>> >>> On 5/7/2021 10:55 AM, Sean Anderson wrote: >>>> 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 <tianrui-wei@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@outlook.com> >>>>>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>>>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>>>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>>>>> + * Portions Copyright 2021 Tianrui Wei >>>>>>> + * Tianrui Wei <tianrui-wei@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. >>>> >>>> Can you add a note along the lines of "Adapted from tegra_mmc.c" for the >>>> curiosity of future readers :) >>>> >>>> --Sean >>>> >>>>> >>>>>> >>>>>>> + */ >>>>>>> + >>>>>>> +#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? > > > I went back and double check, seems that it only applies for board with PCI configured Ok, then does memcpy work? --Sean > > >>>>>> >>>>>>> + } 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 >>>> >> > Thanks, > > Tianrui >
On 5/9/2021 1:39 AM, Sean Anderson wrote: > On 5/8/21 1:27 PM, Tianrui Wei wrote: >> >> On 5/7/2021 10:59 AM, Sean Anderson wrote: >>> On 5/6/21 10:57 PM, Tianrui Wei wrote: >>>> Hi Sean, >>>> >>>> >>>> Thanks for reviewing the patches, I'll fix the initialization thing >>>> and the tegra_mmc credit :P >>> >>> Please also add a changelog to your next revision. It helps reviewers >>> know what you have fixed. >>> >>> --Sean >>> >>>> >>>> On 5/7/2021 10:55 AM, Sean Anderson wrote: >>>>> 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 <tianrui-wei@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@outlook.com> >>>>>>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>>>>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>>>>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>>>>>> + * Portions Copyright 2021 Tianrui Wei >>>>>>>> + * Tianrui Wei <tianrui-wei@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. >>>>> >>>>> Can you add a note along the lines of "Adapted from tegra_mmc.c" >>>>> for the >>>>> curiosity of future readers :) >>>>> >>>>> --Sean >>>>> >>>>>> >>>>>>> >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#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? >> >> >> I went back and double check, seems that it only applies for board >> with PCI configured > > Ok, then does memcpy work? > > --Sean I'm afraid not, I was under the impression that memcpy cannot deal with uncachable region Thanks, Tianrui > > >> >> >>>>>>> >>>>>>>> + } 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 >>>>> >>> >> Thanks, >> >> Tianrui >> > >
On 5/9/2021 1:39 AM, Sean Anderson wrote: > On 5/8/21 1:27 PM, Tianrui Wei wrote: >> >> On 5/7/2021 10:59 AM, Sean Anderson wrote: >>> On 5/6/21 10:57 PM, Tianrui Wei wrote: >>>> Hi Sean, >>>> >>>> >>>> Thanks for reviewing the patches, I'll fix the initialization thing >>>> and the tegra_mmc credit :P >>> >>> Please also add a changelog to your next revision. It helps reviewers >>> know what you have fixed. >>> >>> --Sean >>> >>>> >>>> On 5/7/2021 10:55 AM, Sean Anderson wrote: >>>>> 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 <tianrui-wei@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@outlook.com> >>>>>>>> Signed-off-by: Jonathan Balkind <jbalkind@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@samsung.com> >>>>>>>> + * Jaehoon Chung <jh80.chung@samsung.com> >>>>>>>> + * Portions Copyright 2011-2019 NVIDIA Corporation >>>>>>>> + * Portions Copyright 2021 Tianrui Wei >>>>>>>> + * Tianrui Wei <tianrui-wei@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. >>>>> >>>>> Can you add a note along the lines of "Adapted from tegra_mmc.c" >>>>> for the >>>>> curiosity of future readers :) >>>>> >>>>> --Sean >>>>> >>>>>> >>>>>>> >>>>>>>> + */ >>>>>>>> + >>>>>>>> +#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? >> >> >> I went back and double check, seems that it only applies for board >> with PCI configured > > Ok, then does memcpy work? > > --Sean Scratch the previous email, I stand corrected. You're right about memcpy could work, thanks for the great suggestion :P Many thanks, Tianrui > >> >> >>>>>>> >>>>>>>> + } 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 >>>>> >>> >> Thanks, >> >> Tianrui >> > >
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 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@samsung.com> + * Jaehoon Chung <jh80.chung@samsung.com> + * Portions Copyright 2011-2019 NVIDIA Corporation + * Portions Copyright 2021 Tianrui Wei + * Tianrui Wei <tianrui-wei@outlook.com> + */ + +#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; +}; + +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 = data->blocks * data->blocksize; + u64 start_block = cmd->cmdarg; + unsigned int *buff = (unsigned int *)data->dest; + + 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++; + } + } 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 = &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), +};