diff mbox series

[RESEND,v6,2/2] mmc: openpiton: add piton_mmc driver

Message ID SY4PR01MB6798A8E0E807BF137D39DC12F6339@SY4PR01MB6798.ausprd01.prod.outlook.com
State Superseded
Delegated to: Andes
Headers show
Series [RESEND,v6,1/2] board: riscv: add openpiton-riscv64 SoC support | expand

Commit Message

Tianrui Wei June 12, 2021, 5:16 p.m. UTC
This commit adds support to piton_mmc driver for OpenPiton-riscv64
In particular, this driver

- V6
  . change type of address
  . move declarations ahead
  . loop style update

Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
---
 drivers/mmc/Kconfig     |   9 +++
 drivers/mmc/Makefile    |   1 +
 drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100644 drivers/mmc/piton_mmc.c

Comments

Jaehoon Chung June 13, 2021, 10:09 p.m. UTC | #1
Dear Tianrui,

On 6/13/21 2:16 AM, Tianrui Wei wrote:
> This commit adds support to piton_mmc driver for OpenPiton-riscv64
> In particular, this driver
> 
> - V6
>   . change type of address
>   . move declarations ahead
>   . loop style update
> 
> Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
> Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
> ---
>  drivers/mmc/Kconfig     |   9 +++
>  drivers/mmc/Makefile    |   1 +
>  drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 179 insertions(+)
>  create mode 100644 drivers/mmc/piton_mmc.c
> 
> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
> index 8901456967..096d6a930c 100644
> --- a/drivers/mmc/Kconfig
> +++ b/drivers/mmc/Kconfig
> @@ -727,6 +727,15 @@ 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 selects support for the SD host controller on
> +		OpenPiton SoC. Note that this SD controller directly
> +		exposes the contents of the SD card as memory mapped,
> +		so there is no manual configuration required

Fix indentation. 

> +
>  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 89d6af3db3..cc08b41d0d 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -72,6 +72,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

Ditto.

>  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 0000000000..32671d1a89
> --- /dev/null
> +++ b/drivers/mmc/piton_mmc.c
> @@ -0,0 +1,169 @@
> +// 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
> + * This file is adapted from tegra_mmc.c
> + * Tianrui Wei <tianrui-wei@outlook.com>
> + */
> +
> +#include <asm/gpio.h>
> +#include <asm/io.h>
> +#include <common.h>
> +#include <div64.h>
> +#include <dm.h>
> +#include <errno.h>
> +#include <linux/bitops.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/types.h>
> +#include <log.h>
> +#include <mmc.h>
> +
> +struct piton_mmc_plat {
> +	struct mmc_config cfg;
> +	struct mmc mmc;
> +};
> +
> +struct piton_mmc_priv {
> +	void __iomem *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)

Ditto.

> +{
> +	/* check first if this is a pure command */
> +	if (!data)
> +		return 0;
> +
> +	struct piton_mmc_priv *priv = dev_get_priv(dev);
> +	u32 *buff, *start_addr;
> +	size_t byte_cnt, start_block;
> +
> +	buff = (u32 *)data->dest;
> +	start_block = cmd->cmdarg;
> +	start_addr = priv->piton_mmc_base_addr + start_block;
> +
> +	/* if there is a read */
> +	if (data->flags & MMC_DATA_READ) {
> +		for (byte_cnt = data->blocks * data->blocksize; byte_cnt;
> +				 byte_cnt -= sizeof(u32)) {
> +			*buff++ = readl(start_addr++);
> +		}
> +	} else {
> +		return -ENOSYS;

Is is right to return -ENOSYS? MMC_DATA_WRITE doesn't support?

> +	}
> +
> +	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 = (void *)dev_read_addr(dev);
> +	cfg = &plat->cfg;
> +	cfg->name = "PITON MMC";
> +	cfg->host_caps = MMC_MODE_8BIT;
> +	cfg->f_max = 100000;

f_max is 100K? I don't think so.

> +	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;

Use macro. It's not readable. What's 0x2000000000ULL?

> +	mmc->has_init = 1;

Right? has_init will be set in mmc.c.
Why it's set to 1 in driver?

> +
> +	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 the micro sd card is 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,
> +	.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;

Ditto.

> +	upriv->mmc->capacity = 0x2000000000ULL;

Ditto.

> +	upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN;

readl_blk_len will be set in mmc.c.
Your driver doesn't use mmc.c? Why do you set to some values in your driver?

> +	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;

Use a readable value instead of U32_MAX. CONFIG_SYS_MMC_MAX_BLK_COUNT or other.

Best Regards,
Jaehoon Chung

> +
> +	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),
> +};
>
Tianrui Wei June 14, 2021, 12:28 a.m. UTC | #2
Hi Jaehoon,


Many thanks for taking the time to review our patches :P


On 6/14/2021 6:09 AM, Jaehoon Chung wrote:
> Dear Tianrui,
>
> On 6/13/21 2:16 AM, Tianrui Wei wrote:
>> This commit adds support to piton_mmc driver for OpenPiton-riscv64
>> In particular, this driver
>>
>> - V6
>>    . change type of address
>>    . move declarations ahead
>>    . loop style update
>>
>> Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
>> Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
>> ---
>>   drivers/mmc/Kconfig     |   9 +++
>>   drivers/mmc/Makefile    |   1 +
>>   drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 179 insertions(+)
>>   create mode 100644 drivers/mmc/piton_mmc.c
>>
>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>> index 8901456967..096d6a930c 100644
>> --- a/drivers/mmc/Kconfig
>> +++ b/drivers/mmc/Kconfig
>> @@ -727,6 +727,15 @@ 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 selects support for the SD host controller on
>> +		OpenPiton SoC. Note that this SD controller directly
>> +		exposes the contents of the SD card as memory mapped,
>> +		so there is no manual configuration required
> Fix indentation.


Will do


>
>> +
>>   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 89d6af3db3..cc08b41d0d 100644
>> --- a/drivers/mmc/Makefile
>> +++ b/drivers/mmc/Makefile
>> @@ -72,6 +72,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
> Ditto.
>
>>   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 0000000000..32671d1a89
>> --- /dev/null
>> +++ b/drivers/mmc/piton_mmc.c
>> @@ -0,0 +1,169 @@
>> +// 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
>> + * This file is adapted from tegra_mmc.c
>> + * Tianrui Wei <tianrui-wei@outlook.com>
>> + */
>> +
>> +#include <asm/gpio.h>
>> +#include <asm/io.h>
>> +#include <common.h>
>> +#include <div64.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <linux/bitops.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/types.h>
>> +#include <log.h>
>> +#include <mmc.h>
>> +
>> +struct piton_mmc_plat {
>> +	struct mmc_config cfg;
>> +	struct mmc mmc;
>> +};
>> +
>> +struct piton_mmc_priv {
>> +	void __iomem *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)
> Ditto.


Will do


>
>> +{
>> +	/* check first if this is a pure command */
>> +	if (!data)
>> +		return 0;
>> +
>> +	struct piton_mmc_priv *priv = dev_get_priv(dev);
>> +	u32 *buff, *start_addr;
>> +	size_t byte_cnt, start_block;
>> +
>> +	buff = (u32 *)data->dest;
>> +	start_block = cmd->cmdarg;
>> +	start_addr = priv->piton_mmc_base_addr + start_block;
>> +
>> +	/* if there is a read */
>> +	if (data->flags & MMC_DATA_READ) {
>> +		for (byte_cnt = data->blocks * data->blocksize; byte_cnt;
>> +				 byte_cnt -= sizeof(u32)) {
>> +			*buff++ = readl(start_addr++);
>> +		}
>> +	} else {
>> +		return -ENOSYS;
> Is is right to return -ENOSYS? MMC_DATA_WRITE doesn't support?


Will change to support it


>
>> +	}
>> +
>> +	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 = (void *)dev_read_addr(dev);
>> +	cfg = &plat->cfg;
>> +	cfg->name = "PITON MMC";
>> +	cfg->host_caps = MMC_MODE_8BIT;
>> +	cfg->f_max = 100000;
> f_max is 100K? I don't think so.


We don't use f_max anywhere, so we're setting dummy values :P


>
>> +	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;
> Use macro. It's not readable. What's 0x2000000000ULL?


Will do


>
>> +	mmc->has_init = 1;
> Right? has_init will be set in mmc.c.
> Why it's set to 1 in driver?


Our mmc controller directly maps the contents of the SD card into a

memory region, so there're no configuration registers avaiable. We're

setting these values to avoid problems with mmc.c


>
>> +
>> +	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 the micro sd card is 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,
>> +	.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;
> Ditto.


Ditto


>
>> +	upriv->mmc->capacity = 0x2000000000ULL;
> Ditto.


Ditto


>
>> +	upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN;
> readl_blk_len will be set in mmc.c.
> Your driver doesn't use mmc.c? Why do you set to some values in your driver?


Ditto


>
>> +	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;
> Use a readable value instead of U32_MAX. CONFIG_SYS_MMC_MAX_BLK_COUNT or other.


Will do.


Thanks for your review and helpful comments. We're very keen to get this 
upstream so I

will address your issues ASAP and get a new version submitted. Regarding 
readl_blk_len,

our MMC is a bit of a special case in that it's directly memory mapped 
under the hood.

Can we go ahead with this version or might you have another suggestion?


Best regards,

Tianrui


>
> Best Regards,
> Jaehoon Chung
>
>> +
>> +	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),
>> +};
>>
Jaehoon Chung June 14, 2021, 5:58 a.m. UTC | #3
Hi Tianrui,

On 6/14/21 9:28 AM, Tianrui Wei wrote:
> Hi Jaehoon,
> 
> 
> Many thanks for taking the time to review our patches :P
> 
> 
> On 6/14/2021 6:09 AM, Jaehoon Chung wrote:
>> Dear Tianrui,
>>
>> On 6/13/21 2:16 AM, Tianrui Wei wrote:
>>> This commit adds support to piton_mmc driver for OpenPiton-riscv64
>>> In particular, this driver
>>>
>>> - V6
>>>    . change type of address
>>>    . move declarations ahead
>>>    . loop style update
>>>
>>> Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
>>> Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
>>> ---
>>>   drivers/mmc/Kconfig     |   9 +++
>>>   drivers/mmc/Makefile    |   1 +
>>>   drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 179 insertions(+)
>>>   create mode 100644 drivers/mmc/piton_mmc.c
>>>
>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>> index 8901456967..096d6a930c 100644
>>> --- a/drivers/mmc/Kconfig
>>> +++ b/drivers/mmc/Kconfig
>>> @@ -727,6 +727,15 @@ 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 selects support for the SD host controller on
>>> +        OpenPiton SoC. Note that this SD controller directly
>>> +        exposes the contents of the SD card as memory mapped,
>>> +        so there is no manual configuration required
>> Fix indentation.
> 
> 
> Will do
> 
> 
>>
>>> +
>>>   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 89d6af3db3..cc08b41d0d 100644
>>> --- a/drivers/mmc/Makefile
>>> +++ b/drivers/mmc/Makefile
>>> @@ -72,6 +72,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
>> Ditto.
>>
>>>   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 0000000000..32671d1a89
>>> --- /dev/null
>>> +++ b/drivers/mmc/piton_mmc.c
>>> @@ -0,0 +1,169 @@
>>> +// 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
>>> + * This file is adapted from tegra_mmc.c
>>> + * Tianrui Wei <tianrui-wei@outlook.com>
>>> + */
>>> +
>>> +#include <asm/gpio.h>
>>> +#include <asm/io.h>
>>> +#include <common.h>
>>> +#include <div64.h>
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/err.h>
>>> +#include <linux/types.h>
>>> +#include <log.h>
>>> +#include <mmc.h>
>>> +
>>> +struct piton_mmc_plat {
>>> +    struct mmc_config cfg;
>>> +    struct mmc mmc;
>>> +};
>>> +
>>> +struct piton_mmc_priv {
>>> +    void __iomem *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)
>> Ditto.
> 
> 
> Will do
> 
> 
>>
>>> +{
>>> +    /* check first if this is a pure command */
>>> +    if (!data)
>>> +        return 0;
>>> +
>>> +    struct piton_mmc_priv *priv = dev_get_priv(dev);
>>> +    u32 *buff, *start_addr;
>>> +    size_t byte_cnt, start_block;
>>> +
>>> +    buff = (u32 *)data->dest;
>>> +    start_block = cmd->cmdarg;
>>> +    start_addr = priv->piton_mmc_base_addr + start_block;
>>> +
>>> +    /* if there is a read */
>>> +    if (data->flags & MMC_DATA_READ) {
>>> +        for (byte_cnt = data->blocks * data->blocksize; byte_cnt;
>>> +                 byte_cnt -= sizeof(u32)) {
>>> +            *buff++ = readl(start_addr++);
>>> +        }
>>> +    } else {
>>> +        return -ENOSYS;
>> Is is right to return -ENOSYS? MMC_DATA_WRITE doesn't support?
> 
> 
> Will change to support it
> 
> 
>>
>>> +    }
>>> +
>>> +    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 = (void *)dev_read_addr(dev);
>>> +    cfg = &plat->cfg;
>>> +    cfg->name = "PITON MMC";
>>> +    cfg->host_caps = MMC_MODE_8BIT;
>>> +    cfg->f_max = 100000;
>> f_max is 100K? I don't think so.
> 
> 
> We don't use f_max anywhere, so we're setting dummy values :P
> 
> 
>>
>>> +    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;
>> Use macro. It's not readable. What's 0x2000000000ULL?
> 
> 
> Will do
> 
> 
>>
>>> +    mmc->has_init = 1;
>> Right? has_init will be set in mmc.c.
>> Why it's set to 1 in driver?
> 
> 
> Our mmc controller directly maps the contents of the SD card into a
> 
> memory region, so there're no configuration registers avaiable. We're
> 
> setting these values to avoid problems with mmc.c
> 
> 
>>
>>> +
>>> +    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 the micro sd card is 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,
>>> +    .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;
>> Ditto.
> 
> 
> Ditto
> 
> 
>>
>>> +    upriv->mmc->capacity = 0x2000000000ULL;
>> Ditto.
> 
> 
> Ditto
> 
> 
>>
>>> +    upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN;
>> readl_blk_len will be set in mmc.c.
>> Your driver doesn't use mmc.c? Why do you set to some values in your driver?
> 
> 
> Ditto
> 
> 
>>
>>> +    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;
>> Use a readable value instead of U32_MAX. CONFIG_SYS_MMC_MAX_BLK_COUNT or other.
> 
> 
> Will do.
> 
> 
> Thanks for your review and helpful comments. We're very keen to get this upstream so I
> 
> will address your issues ASAP and get a new version submitted. Regarding readl_blk_len,
> 
> our MMC is a bit of a special case in that it's directly memory mapped under the hood.
> 
> Can we go ahead with this version or might you have another suggestion?

I think that it can be applied after fixing some indentation.
It seems that other things can be updated after applied your patches.

BTW, which board can this driver be used? Is there open board that can i use?

Best Regards,
Jaehoon Chung

> 
> 
> Best regards,
> 
> Tianrui
> 
> 
>>
>> Best Regards,
>> Jaehoon Chung
>>
>>> +
>>> +    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),
>>> +};
>>>
>
Tianrui Wei June 14, 2021, 5:23 p.m. UTC | #4
Dear Jaehoon,

On 6/14/2021 1:58 PM, Jaehoon Chung wrote:
> Hi Tianrui,
>
> On 6/14/21 9:28 AM, Tianrui Wei wrote:
>> Hi Jaehoon,
>>
>>
>> Many thanks for taking the time to review our patches :P
>>
>>
>> On 6/14/2021 6:09 AM, Jaehoon Chung wrote:
>>> Dear Tianrui,
>>>
>>> On 6/13/21 2:16 AM, Tianrui Wei wrote:
>>>> This commit adds support to piton_mmc driver for OpenPiton-riscv64
>>>> In particular, this driver
>>>>
>>>> - V6
>>>>     . change type of address
>>>>     . move declarations ahead
>>>>     . loop style update
>>>>
>>>> Signed-off-by: Tianrui Wei <tianrui-wei@outlook.com>
>>>> Signed-off-by: Jonathan Balkind <jbalkind@ucsb.edu>
>>>> ---
>>>>    drivers/mmc/Kconfig     |   9 +++
>>>>    drivers/mmc/Makefile    |   1 +
>>>>    drivers/mmc/piton_mmc.c | 169 ++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 179 insertions(+)
>>>>    create mode 100644 drivers/mmc/piton_mmc.c
>>>>
>>>> diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
>>>> index 8901456967..096d6a930c 100644
>>>> --- a/drivers/mmc/Kconfig
>>>> +++ b/drivers/mmc/Kconfig
>>>> @@ -727,6 +727,15 @@ 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 selects support for the SD host controller on
>>>> +        OpenPiton SoC. Note that this SD controller directly
>>>> +        exposes the contents of the SD card as memory mapped,
>>>> +        so there is no manual configuration required
>>> Fix indentation.
>>
>> Will do
>>
>>
>>>> +
>>>>    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 89d6af3db3..cc08b41d0d 100644
>>>> --- a/drivers/mmc/Makefile
>>>> +++ b/drivers/mmc/Makefile
>>>> @@ -72,6 +72,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
>>> Ditto.
>>>
>>>>    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 0000000000..32671d1a89
>>>> --- /dev/null
>>>> +++ b/drivers/mmc/piton_mmc.c
>>>> @@ -0,0 +1,169 @@
>>>> +// 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
>>>> + * This file is adapted from tegra_mmc.c
>>>> + * Tianrui Wei <tianrui-wei@outlook.com>
>>>> + */
>>>> +
>>>> +#include <asm/gpio.h>
>>>> +#include <asm/io.h>
>>>> +#include <common.h>
>>>> +#include <div64.h>
>>>> +#include <dm.h>
>>>> +#include <errno.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/types.h>
>>>> +#include <log.h>
>>>> +#include <mmc.h>
>>>> +
>>>> +struct piton_mmc_plat {
>>>> +    struct mmc_config cfg;
>>>> +    struct mmc mmc;
>>>> +};
>>>> +
>>>> +struct piton_mmc_priv {
>>>> +    void __iomem *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)
>>> Ditto.
>>
>> Will do
>>
>>
>>>> +{
>>>> +    /* check first if this is a pure command */
>>>> +    if (!data)
>>>> +        return 0;
>>>> +
>>>> +    struct piton_mmc_priv *priv = dev_get_priv(dev);
>>>> +    u32 *buff, *start_addr;
>>>> +    size_t byte_cnt, start_block;
>>>> +
>>>> +    buff = (u32 *)data->dest;
>>>> +    start_block = cmd->cmdarg;
>>>> +    start_addr = priv->piton_mmc_base_addr + start_block;
>>>> +
>>>> +    /* if there is a read */
>>>> +    if (data->flags & MMC_DATA_READ) {
>>>> +        for (byte_cnt = data->blocks * data->blocksize; byte_cnt;
>>>> +                 byte_cnt -= sizeof(u32)) {
>>>> +            *buff++ = readl(start_addr++);
>>>> +        }
>>>> +    } else {
>>>> +        return -ENOSYS;
>>> Is is right to return -ENOSYS? MMC_DATA_WRITE doesn't support?
>>
>> Will change to support it
>>
>>
>>>> +    }
>>>> +
>>>> +    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 = (void *)dev_read_addr(dev);
>>>> +    cfg = &plat->cfg;
>>>> +    cfg->name = "PITON MMC";
>>>> +    cfg->host_caps = MMC_MODE_8BIT;
>>>> +    cfg->f_max = 100000;
>>> f_max is 100K? I don't think so.
>>
>> We don't use f_max anywhere, so we're setting dummy values :P
>>
>>
>>>> +    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;
>>> Use macro. It's not readable. What's 0x2000000000ULL?
>>
>> Will do
>>
>>
>>>> +    mmc->has_init = 1;
>>> Right? has_init will be set in mmc.c.
>>> Why it's set to 1 in driver?
>>
>> Our mmc controller directly maps the contents of the SD card into a
>>
>> memory region, so there're no configuration registers avaiable. We're
>>
>> setting these values to avoid problems with mmc.c
>>
>>
>>>> +
>>>> +    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 the micro sd card is 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,
>>>> +    .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;
>>> Ditto.
>>
>> Ditto
>>
>>
>>>> +    upriv->mmc->capacity = 0x2000000000ULL;
>>> Ditto.
>>
>> Ditto
>>
>>
>>>> +    upriv->mmc->read_bl_len = MMC_MAX_BLOCK_LEN;
>>> readl_blk_len will be set in mmc.c.
>>> Your driver doesn't use mmc.c? Why do you set to some values in your driver?
>>
>> Ditto
>>
>>
>>>> +    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;
>>> Use a readable value instead of U32_MAX. CONFIG_SYS_MMC_MAX_BLK_COUNT or other.
>>
>> Will do.
>>
>>
>> Thanks for your review and helpful comments. We're very keen to get this upstream so I
>>
>> will address your issues ASAP and get a new version submitted. Regarding readl_blk_len,
>>
>> our MMC is a bit of a special case in that it's directly memory mapped under the hood.
>>
>> Can we go ahead with this version or might you have another suggestion?
> I think that it can be applied after fixing some indentation.
> It seems that other things can be updated after applied your patches.
>
> BTW, which board can this driver be used? Is there open board that can i use?
>
> Best Regards,
> Jaehoon Chung


Many thanks for taking the time and energy to review our code!


If you want to test this driver, you can try Xilinx vc707, digilent 
nexys video and genesys 2.

As our SoC is fpga based, these development boards are the easiest to 
work with.

If you'd like to try it out, you could try the design as documented in 
the other patch's

board documentation :P


Best Regards,

Tianrui

>
>>
>> Best regards,
>>
>> Tianrui
>>
>>
>>> Best Regards,
>>> Jaehoon Chung
>>>
>>>> +
>>>> +    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),
>>>> +};
>>>>
diff mbox series

Patch

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig
index 8901456967..096d6a930c 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -727,6 +727,15 @@  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 selects support for the SD host controller on
+		OpenPiton SoC. Note that this SD controller directly
+		exposes the contents of the SD card as memory mapped,
+		so there is no manual configuration required
+
 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 89d6af3db3..cc08b41d0d 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -72,6 +72,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 0000000000..32671d1a89
--- /dev/null
+++ b/drivers/mmc/piton_mmc.c
@@ -0,0 +1,169 @@ 
+// 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
+ * This file is adapted from tegra_mmc.c
+ * Tianrui Wei <tianrui-wei@outlook.com>
+ */
+
+#include <asm/gpio.h>
+#include <asm/io.h>
+#include <common.h>
+#include <div64.h>
+#include <dm.h>
+#include <errno.h>
+#include <linux/bitops.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/types.h>
+#include <log.h>
+#include <mmc.h>
+
+struct piton_mmc_plat {
+	struct mmc_config cfg;
+	struct mmc mmc;
+};
+
+struct piton_mmc_priv {
+	void __iomem *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;
+
+	struct piton_mmc_priv *priv = dev_get_priv(dev);
+	u32 *buff, *start_addr;
+	size_t byte_cnt, start_block;
+
+	buff = (u32 *)data->dest;
+	start_block = cmd->cmdarg;
+	start_addr = priv->piton_mmc_base_addr + start_block;
+
+	/* if there is a read */
+	if (data->flags & MMC_DATA_READ) {
+		for (byte_cnt = data->blocks * data->blocksize; byte_cnt;
+				 byte_cnt -= sizeof(u32)) {
+			*buff++ = readl(start_addr++);
+		}
+	} else {
+		return -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 = (void *)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 the micro sd card is 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,
+	.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),
+};