diff mbox

[1/8] mmc: sdhci-st: Intial support for ST SDHCI controller

Message ID 1400771902-26553-2-git-send-email-peter.griffin@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Griffin May 22, 2014, 3:18 p.m. UTC 
This platform driver adds initial support for the SDHCI host controller
found on STMicroelectronics SoCs.

It has been tested on STiH41x b2020 platforms currently.

Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/mmc/host/Kconfig    |  12 +++
 drivers/mmc/host/Makefile   |   1 +
 drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/mmc/host/sdhci-st.c

Comments

Maxime Coquelin May 22, 2014, 3:41 p.m. UTC | #1 
Hi Peter

On 05/22/2014 05:18 PM, Peter Griffin wrote:
> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
>
> It has been tested on STiH41x b2020 platforms currently.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>   drivers/mmc/host/Kconfig    |  12 +++
>   drivers/mmc/host/Makefile   |   1 +
>   drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 257 insertions(+)
>   create mode 100644 drivers/mmc/host/sdhci-st.c
>

[...]

> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +	struct  reset_control	*rstc;
> +};
Since it uses the generic reset framework, could we imagine moving the 
reset to the sdhci_pltfm_host struct?
Doing this, we could get rid of st_mmc_platform_data.


> +
> +static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
> +{
> +	u8 ctrl;
> +
> +	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +
> +	switch (width) {
> +	case MMC_BUS_WIDTH_8:
> +		ctrl |= SDHCI_CTRL_8BITBUS;
> +		ctrl &= ~SDHCI_CTRL_4BITBUS;
> +		break;
> +	case MMC_BUS_WIDTH_4:
> +		ctrl |= SDHCI_CTRL_4BITBUS;
> +		ctrl &= ~SDHCI_CTRL_8BITBUS;
> +		break;
> +	default:
> +		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> +		break;
You can remove the break here.

> +	}
> +
> +	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +
> +	return 0;
> +}
> +
> +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +	return clk_get_rate(pltfm_host->clk);
> +}
> +
> +static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
> +{
> +	u32 ret;
> +
> +	switch (reg) {
> +	case SDHCI_CAPABILITIES:
> +		ret = readl(host->ioaddr + reg);
> +		/* Support 3.3V and 1.8V */
> +		ret &= ~SDHCI_CAN_VDD_300;
> +		break;
> +	default:
> +		ret = readl(host->ioaddr + reg);

Could we use readl_relaxed?

> +	}
> +	return ret;
> +}
> +
> +static const struct sdhci_ops sdhci_st_ops = {
> +	.get_max_clock = sdhci_st_get_max_clk,
> +	.platform_bus_width = sdhci_st_8bit_width,
> +	.read_l = sdhci_st_readl,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_st_pdata = {
> +	.ops = &sdhci_st_ops,
> +	.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> +	    SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
You can remove this I think.

> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);
> +
> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;
> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
> +		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> +			       SDHCI_VENDOR_VER_SHIFT));
Maybe you could change to dev_info here. It might be interresting to 
always have IP version.

> +
> +	return 0;
> +
> +err_out:
> +	clk_disable_unprepare(clk);
> +	sdhci_pltfm_free(pdev);
> +
> +	return ret;
> +}

[...]

Regards,
Maxime
Lee Jones May 22, 2014, 4:50 p.m. UTC | #2 
> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
> 
> It has been tested on STiH41x b2020 platforms currently.
> 
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/host/Kconfig    |  12 +++
>  drivers/mmc/host/Makefile   |   1 +
>  drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-st.c

[...]

> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);

Move this down as far as it will go.  When do you _need_ the clock
running by?

> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;

If it's possible to move the clk_prepare enable down past here, then
you only need to do sdhci_pltfm_free() and you can remove all of the
err_out error path.

> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));

Do we want to be doing any error checking for unsupported devices
here?

[...]

> +static struct platform_driver sdhci_st_driver = {
> +	.probe = sdhci_st_probe,
> +	.remove = sdhci_st_remove,
> +	.driver = {
> +		   .name = "sdhci-st",
> +		   .owner = THIS_MODULE,
> +		   .pm = SDHCI_ST_PMOPS,
> +		   .of_match_table = of_match_ptr(st_sdhci_match),
> +		  },

Tabbing issue here.

> +};
> +
> +module_platform_driver(sdhci_st_driver);
> +
> +MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
> +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:st-sdhci");
Srinivas Kandagatla May 23, 2014, 6:18 a.m. UTC | #3 
Hi Pete,

On 22/05/14 16:18, Peter Griffin wrote:

>
>   ifeq ($(CONFIG_CB710_DEBUG),y)
>   	CFLAGS-cb710-mmc	+= -DDEBUG
> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
Do you need this header?

> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +	struct  reset_control	*rstc;
> +};

st_mmc_platform_data name is bit missleading as this data is not part of 
platform data. Probably you could rename it to struct sdhci_st_data.
...
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct sdhci_host *host;
> +	struct st_mmc_platform_data *pdata;
> +	struct sdhci_pltfm_host *pltfm_host;
> +	struct clk *clk;
> +	int ret = 0;
> +	u16 host_version;
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +	if (IS_ERR(pdata->rstc))
> +		pdata->rstc = NULL;
> +	else
> +		reset_control_deassert(pdata->rstc);
> +
> +	clk =  devm_clk_get(&pdev->dev, "mmc");
> +	if (IS_ERR(clk)) {
> +		dev_err(&pdev->dev, "Perpheral clk not found\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +	if (IS_ERR(host)) {
> +		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +		return PTR_ERR(host);
> +	}
> +
> +	clk_prepare_enable(clk);
> +
> +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +			| MMC_CAP_1_8V_DDR;
> +
> +	if (of_property_read_bool(np, "non-removable"))
> +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> +
> +	pltfm_host = sdhci_priv(host);
> +	pltfm_host->clk = clk;
> +
> +	ret = sdhci_add_host(host);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> +		goto err_out;
> +	}
> +
> +	pltfm_host->priv = pdata;
> +
> +	platform_set_drvdata(pdev, host);

Why not platform_set_drvdata(pdev, priv_host);
As you are not using sdhci_host directly, this will reduced few 
indirections in the driver.


> +
> +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> +
> +	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
> +		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> +			       SDHCI_VENDOR_VER_SHIFT));
> +
> +	return 0;
> +
> +err_out:
> +	clk_disable_unprepare(clk);
> +	sdhci_pltfm_free(pdev);
> +
IP could be left out of reset in this path.
> +	return ret;
> +}
> +
> +static int sdhci_st_remove(struct platform_device *pdev)
> +{
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +	int ret;
> +
> +	clk_disable_unprepare(pltfm_host->clk);
> +
> +	ret = sdhci_pltfm_unregister(pdev);
> +
> +	if (pdata->rstc)
> +		reset_control_assert(pdata->rstc);
> +
> +	return ret;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int sdhci_st_suspend(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +	int ret = sdhci_suspend_host(host);
> +
> +	if (ret)
> +		goto out;
> +
> +	if (pdata->rstc)
> +		reset_control_assert(pdata->rstc);
> +
> +	clk_disable_unprepare(pltfm_host->clk);
> +out:
> +	return ret;
> +}
> +
> +static int sdhci_st_resume(struct device *dev)
> +{
> +	struct sdhci_host *host = dev_get_drvdata(dev);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct st_mmc_platform_data *pdata = pltfm_host->priv;
> +
> +	clk_prepare_enable(pltfm_host->clk);
> +
> +	if (pdata->rstc)
> +		reset_control_deassert(pdata->rstc);
> +
> +	return sdhci_resume_host(host);
> +}
> +

replace:
[...
> +static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
> +#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
> +#else
> +#define SDHCI_ST_PMOPS NULL
> +#endif
...]

with :

#endif

static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);

	
> +
> +static const struct of_device_id st_sdhci_match[] = {
> +	{ .compatible = "st,sdhci" },
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, st_sdhci_match);
> +
> +static struct platform_driver sdhci_st_driver = {
> +	.probe = sdhci_st_probe,
> +	.remove = sdhci_st_remove,
> +	.driver = {
> +		   .name = "sdhci-st",
> +		   .owner = THIS_MODULE,
> +		   .pm = SDHCI_ST_PMOPS,
use .pm = sdhci_st_pmops,

> +		   .of_match_table = of_match_ptr(st_sdhci_match),
> +		  },
> +};
> +
> +module_platform_driver(sdhci_st_driver);
> +
> +MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
> +MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:st-sdhci");
>
Ulf Hansson May 23, 2014, 7:34 a.m. UTC | #4 
On 22 May 2014 17:18, Peter Griffin <peter.griffin@linaro.org> wrote:
> This platform driver adds initial support for the SDHCI host controller
> found on STMicroelectronics SoCs.
>
> It has been tested on STiH41x b2020 platforms currently.
>
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
>  drivers/mmc/host/Kconfig    |  12 +++
>  drivers/mmc/host/Makefile   |   1 +
>  drivers/mmc/host/sdhci-st.c | 244 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 257 insertions(+)
>  create mode 100644 drivers/mmc/host/sdhci-st.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 8aaf8c1..b62c40d 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -283,6 +283,18 @@ config MMC_SDHCI_BCM2835
>
>           If unsure, say N.
>
> +config MMC_SDHCI_ST
> +       tristate "SDHCI support on STMicroelectronics SoC"
> +       depends on ARCH_STI
> +       depends on MMC_SDHCI_PLTFM
> +       select MMC_SDHCI_IO_ACCESSORS
> +       help
> +         This selects the Secure Digital Host Controller Interface in
> +         STMicroelectronics SoCs.
> +
> +         If you have a controller with this interface, say Y or M here.
> +         If unsure, say N.
> +
>  config MMC_OMAP
>         tristate "TI OMAP Multimedia Card Interface support"
>         depends on ARCH_OMAP
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 0c8aa5e..6e0acf7 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_MMC_SDHCI_OF_HLWD)               += sdhci-of-hlwd.o
>  obj-$(CONFIG_MMC_SDHCI_BCM_KONA)       += sdhci-bcm-kona.o
>  obj-$(CONFIG_MMC_SDHCI_BCM2835)                += sdhci-bcm2835.o
>  obj-$(CONFIG_MMC_SDHCI_MSM)            += sdhci-msm.o
> +obj-$(CONFIG_MMC_SDHCI_ST)             += sdhci-st.o
>
>  ifeq ($(CONFIG_CB710_DEBUG),y)
>         CFLAGS-cb710-mmc        += -DDEBUG
> diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
> new file mode 100644
> index 0000000..1790fa7
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-st.c
> @@ -0,0 +1,244 @@
> +/*
> + * Support for SDHCI on STMicroelectronics SoCs
> + *
> + * Copyright (C) 2014 STMicroelectronics Ltd
> + * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> + *
> + * Based on sdhci-cns3xxx.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/of_gpio.h>
> +#include <linux/mmc/host.h>
> +#include <linux/reset.h>
> +
> +#include "sdhci-pltfm.h"
> +
> +struct st_mmc_platform_data {
> +       struct  reset_control   *rstc;
> +};
> +
> +static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
> +{
> +       u8 ctrl;
> +
> +       ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +
> +       switch (width) {
> +       case MMC_BUS_WIDTH_8:
> +               ctrl |= SDHCI_CTRL_8BITBUS;
> +               ctrl &= ~SDHCI_CTRL_4BITBUS;
> +               break;
> +       case MMC_BUS_WIDTH_4:
> +               ctrl |= SDHCI_CTRL_4BITBUS;
> +               ctrl &= ~SDHCI_CTRL_8BITBUS;
> +               break;
> +       default:
> +               ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> +               break;
> +       }
> +
> +       sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +
> +       return 0;
> +}
> +
> +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +       return clk_get_rate(pltfm_host->clk);
> +}

There are sdhci library function for the above:
sdhci_pltfm_clk_get_max_clock()

> +
> +static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
> +{
> +       u32 ret;
> +
> +       switch (reg) {
> +       case SDHCI_CAPABILITIES:
> +               ret = readl(host->ioaddr + reg);
> +               /* Support 3.3V and 1.8V */
> +               ret &= ~SDHCI_CAN_VDD_300;
> +               break;
> +       default:
> +               ret = readl(host->ioaddr + reg);
> +       }
> +       return ret;
> +}
> +
> +static const struct sdhci_ops sdhci_st_ops = {
> +       .get_max_clock = sdhci_st_get_max_clk,
> +       .platform_bus_width = sdhci_st_8bit_width,
> +       .read_l = sdhci_st_readl,
> +};
> +
> +static const struct sdhci_pltfm_data sdhci_st_pdata = {
> +       .ops = &sdhci_st_ops,
> +       .quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
> +           SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> +};
> +
> +
> +static int sdhci_st_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct sdhci_host *host;
> +       struct st_mmc_platform_data *pdata;
> +       struct sdhci_pltfm_host *pltfm_host;
> +       struct clk *clk;
> +       int ret = 0;
> +       u16 host_version;
> +
> +       dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> +
> +       pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> +       if (!pdata)
> +               return -ENOMEM;
> +
> +       pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
> +       if (IS_ERR(pdata->rstc))
> +               pdata->rstc = NULL;
> +       else
> +               reset_control_deassert(pdata->rstc);
> +
> +       clk =  devm_clk_get(&pdev->dev, "mmc");
> +       if (IS_ERR(clk)) {
> +               dev_err(&pdev->dev, "Perpheral clk not found\n");
> +               return PTR_ERR(clk);
> +       }
> +
> +       host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
> +       if (IS_ERR(host)) {
> +               dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
> +               return PTR_ERR(host);
> +       }
> +
> +       clk_prepare_enable(clk);
> +
> +       host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> +                       | MMC_CAP_1_8V_DDR;

Comment below.

> +
> +       if (of_property_read_bool(np, "non-removable"))
> +               host->mmc->caps |= MMC_CAP_NONREMOVABLE;

MMC_CAP_1_8V_DDR, MMC_CAP_8_BIT_DATA, MMC_CAP_NONREMOVABLE aren’t
those board specific capabilities?

Unless there are something that prevents you from using the common mmc
DT parser, I would suggest you to use it. mmc_of_parse(). Thus you can
provide these capabilities through DT instead.

Kind regards
Ulf Hansson
Peter Griffin June 2, 2014, 11:06 a.m. UTC | #5 
Hi Lee,

Thanks for your feedback, all your other comments will be fixed in v2. 
However see comments below for this patch

> > +	clk_prepare_enable(clk);
> 
> Move this down as far as it will go.  When do you _need_ the clock
> running by?
> 
> > +	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
> > +			| MMC_CAP_1_8V_DDR;
> > +
> > +	if (of_property_read_bool(np, "non-removable"))
> > +		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
> > +
> > +	pltfm_host = sdhci_priv(host);
> > +	pltfm_host->clk = clk;
> > +
> > +	ret = sdhci_add_host(host);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
> > +		goto err_out;
> 
> If it's possible to move the clk_prepare enable down past here, then
> you only need to do sdhci_pltfm_free() and you can remove all of the
> err_out error path.

No its not possible. sdhci_add_host() reads registers on
the IP, if the clock isn't enabled the system can hang.

> 
> > +	}
> > +
> > +	pltfm_host->priv = pdata;
> > +
> > +	platform_set_drvdata(pdev, host);
> > +
> > +	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
> 
> Do we want to be doing any error checking for unsupported devices
> here?

Not that I'm aware of. This is just a debug print I added as it is useful
for debugging. Its not unheard for software folks not to be told that the
IP version has changed in a new SoC, so comparing dmesg traces of working
kernels with non working ones which include IP versions etc can often
shed some light on whats happening.

Arguably if the maintainers think its helpful then it could be added in 
sdhci_add_host(), and every platform would benefit. Or if its deemed 
not helpful then it can be removed!

Cheers,

Peter.
Peter Griffin June 4, 2014, 8:37 a.m. UTC | #6 
Hi Srini,

Thanks for reviewing, see my comments below: -

On Fri, 23 May 2014, Srinivas Kandagatla wrote:

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> 
> st_mmc_platform_data name is bit missleading as this data is not
> part of platform data. Probably you could rename it to struct
> sdhci_st_data.

I've now removed this reset controller code for v2, as based on Maxime's feedback
I think this would be better off going in the generic code, so all 
platforms could benefit if they have a reset controller implementation.

I plan to send some RFC patches which implement this seperately to this series.

> >+	pltfm_host->priv = pdata;
> >+
> >+	platform_set_drvdata(pdev, host);
> 
> Why not platform_set_drvdata(pdev, priv_host);
> As you are not using sdhci_host directly, this will reduced few
> indirections in the driver.

Your right, and I was going to change this, but with the re-think on the reset
controller code above I will now need sdhci_host so would prefer to leave it as
is for now.

> >+err_out:
> >+	clk_disable_unprepare(clk);
> >+	sdhci_pltfm_free(pdev);
> >+
> IP could be left out of reset in this path.

Good spot, I'll remember this when I add the reset code
back in :-)

> 
> replace:
> [...
> >+static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
> >+#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
> >+#else
> >+#define SDHCI_ST_PMOPS NULL
> >+#endif
> ...]
> 
> with :
> 
> #endif
> 
> static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);

Fixed in v2

regards,

Peter.
Peter Griffin June 4, 2014, 11:05 a.m. UTC | #7 
Hi Maxime,

Thanks fo reviewing, see my comments below: -

> >+struct st_mmc_platform_data {
> >+	struct  reset_control	*rstc;
> >+};
> Since it uses the generic reset framework, could we imagine moving
> the reset to the sdhci_pltfm_host struct?
> Doing this, we could get rid of st_mmc_platform_data.

Yes I agree, I will send some RFC patches which moves the reset controller
stuff into the generic code.

For v2 of this series I've removed the reset code, as stih416 doesn't have
seperate reset line each controller, so it will mainly be 
useful for stih407 SoC, which needs additional patches on top of this 
set.

> >+	switch (width) {
> >+	case MMC_BUS_WIDTH_8:
> >+		ctrl |= SDHCI_CTRL_8BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_4BITBUS;
> >+		break;
> >+	case MMC_BUS_WIDTH_4:
> >+		ctrl |= SDHCI_CTRL_4BITBUS;
> >+		ctrl &= ~SDHCI_CTRL_8BITBUS;
> >+		break;
> >+	default:
> >+		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
> >+		break;
> You can remove the break here.

Ok, removed in v2

> >+	switch (reg) {
> >+	case SDHCI_CAPABILITIES:
> >+		ret = readl(host->ioaddr + reg);
> >+		/* Support 3.3V and 1.8V */
> >+		ret &= ~SDHCI_CAN_VDD_300;
> >+		break;
> >+	default:
> >+		ret = readl(host->ioaddr + reg);
> 
> Could we use readl_relaxed?

Yes, I've updated to use readl_relaxed in v2

> >+	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
> You can remove this I think.

Removed in v2

> >+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
> >+			       SDHCI_VENDOR_VER_SHIFT));
> Maybe you could change to dev_info here. It might be interresting to
> always have IP version.

Changed to dev_info in v2

Regards,

Peter.
Ulf Hansson June 4, 2014, 11:06 a.m. UTC | #8 
On 4 June 2014 10:48, Peter Griffin <peter.griffin@linaro.org> wrote:
> Hi Ulf,
>
> Thanks for reviewing, see my comments below: -
>
> On Fri, 23 May 2014, Ulf Hansson wrote:
>> > +static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
>> > +{
>> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> > +
>> > +       return clk_get_rate(pltfm_host->clk);
>> > +}
>>
>> There are sdhci library function for the above:
>> sdhci_pltfm_clk_get_max_clock()
>
> I've fixed in v2 to use the library function
>
>> > +       host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
>> > +                       | MMC_CAP_1_8V_DDR;
>>
>> Comment below.
>>
>> > +
>> > +       if (of_property_read_bool(np, "non-removable"))
>> > +               host->mmc->caps |= MMC_CAP_NONREMOVABLE;
>>
>> MMC_CAP_1_8V_DDR, MMC_CAP_8_BIT_DATA, MMC_CAP_NONREMOVABLE aren’t
>> those board specific capabilities?
>
> Yep
>>
>> Unless there are something that prevents you from using the common mmc
>> DT parser, I would suggest you to use it. mmc_of_parse(). Thus you can
>> provide these capabilities through DT instead.
>
> Thanks for pointing that out, I've switched over to using mmc_of_parse,
> and everything works as expected.
>
> Also as an added bonus this will simplify support for the next SoC which
> needs access to the high speed ddr / sdr bindings which
> mmc_of_parse already supports :-)
>
> In v2 I've also removed the reset controller code from this platform driver
> with the intention of adding it back in, in the generic code. The idea
> would be that an additional 'reset' binding could be provided, which if
> present would be used to deassert the IP reset line of the controller at
> probe / resume, and assert reset at remove / sleep.
>
> Is this something you view as useful, if so I will prepare some RFC patches?

Sounds useful. Please go ahead and send patches! :-)

Kind regards
Uffe
diff mbox

Patch

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 8aaf8c1..b62c40d 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -283,6 +283,18 @@  config MMC_SDHCI_BCM2835
 
 	  If unsure, say N.
 
+config MMC_SDHCI_ST
+	tristate "SDHCI support on STMicroelectronics SoC"
+	depends on ARCH_STI
+	depends on MMC_SDHCI_PLTFM
+	select MMC_SDHCI_IO_ACCESSORS
+	help
+	  This selects the Secure Digital Host Controller Interface in
+	  STMicroelectronics SoCs.
+
+	  If you have a controller with this interface, say Y or M here.
+	  If unsure, say N.
+
 config MMC_OMAP
 	tristate "TI OMAP Multimedia Card Interface support"
 	depends on ARCH_OMAP
diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
index 0c8aa5e..6e0acf7 100644
--- a/drivers/mmc/host/Makefile
+++ b/drivers/mmc/host/Makefile
@@ -64,6 +64,7 @@  obj-$(CONFIG_MMC_SDHCI_OF_HLWD)		+= sdhci-of-hlwd.o
 obj-$(CONFIG_MMC_SDHCI_BCM_KONA)	+= sdhci-bcm-kona.o
 obj-$(CONFIG_MMC_SDHCI_BCM2835)		+= sdhci-bcm2835.o
 obj-$(CONFIG_MMC_SDHCI_MSM)		+= sdhci-msm.o
+obj-$(CONFIG_MMC_SDHCI_ST)		+= sdhci-st.o
 
 ifeq ($(CONFIG_CB710_DEBUG),y)
 	CFLAGS-cb710-mmc	+= -DDEBUG
diff --git a/drivers/mmc/host/sdhci-st.c b/drivers/mmc/host/sdhci-st.c
new file mode 100644
index 0000000..1790fa7
--- /dev/null
+++ b/drivers/mmc/host/sdhci-st.c
@@ -0,0 +1,244 @@ 
+/*
+ * Support for SDHCI on STMicroelectronics SoCs
+ *
+ * Copyright (C) 2014 STMicroelectronics Ltd
+ * Author: Giuseppe Cavallaro <peppe.cavallaro@st.com>
+ *
+ * Based on sdhci-cns3xxx.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/of_gpio.h>
+#include <linux/mmc/host.h>
+#include <linux/reset.h>
+
+#include "sdhci-pltfm.h"
+
+struct st_mmc_platform_data {
+	struct  reset_control	*rstc;
+};
+
+static int sdhci_st_8bit_width(struct sdhci_host *host, int width)
+{
+	u8 ctrl;
+
+	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+
+	switch (width) {
+	case MMC_BUS_WIDTH_8:
+		ctrl |= SDHCI_CTRL_8BITBUS;
+		ctrl &= ~SDHCI_CTRL_4BITBUS;
+		break;
+	case MMC_BUS_WIDTH_4:
+		ctrl |= SDHCI_CTRL_4BITBUS;
+		ctrl &= ~SDHCI_CTRL_8BITBUS;
+		break;
+	default:
+		ctrl &= ~(SDHCI_CTRL_8BITBUS | SDHCI_CTRL_4BITBUS);
+		break;
+	}
+
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+
+	return 0;
+}
+
+static unsigned int sdhci_st_get_max_clk(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+
+	return clk_get_rate(pltfm_host->clk);
+}
+
+static u32 sdhci_st_readl(struct sdhci_host *host, int reg)
+{
+	u32 ret;
+
+	switch (reg) {
+	case SDHCI_CAPABILITIES:
+		ret = readl(host->ioaddr + reg);
+		/* Support 3.3V and 1.8V */
+		ret &= ~SDHCI_CAN_VDD_300;
+		break;
+	default:
+		ret = readl(host->ioaddr + reg);
+	}
+	return ret;
+}
+
+static const struct sdhci_ops sdhci_st_ops = {
+	.get_max_clock = sdhci_st_get_max_clk,
+	.platform_bus_width = sdhci_st_8bit_width,
+	.read_l = sdhci_st_readl,
+};
+
+static const struct sdhci_pltfm_data sdhci_st_pdata = {
+	.ops = &sdhci_st_ops,
+	.quirks = SDHCI_QUIRK_NO_ENDATTR_IN_NOPDESC |
+	    SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
+};
+
+
+static int sdhci_st_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct sdhci_host *host;
+	struct st_mmc_platform_data *pdata;
+	struct sdhci_pltfm_host *pltfm_host;
+	struct clk *clk;
+	int ret = 0;
+	u16 host_version;
+
+	dev_dbg(&pdev->dev, "SDHCI ST platform driver\n");
+
+	pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->rstc = devm_reset_control_get(&pdev->dev, NULL);
+	if (IS_ERR(pdata->rstc))
+		pdata->rstc = NULL;
+	else
+		reset_control_deassert(pdata->rstc);
+
+	clk =  devm_clk_get(&pdev->dev, "mmc");
+	if (IS_ERR(clk)) {
+		dev_err(&pdev->dev, "Perpheral clk not found\n");
+		return PTR_ERR(clk);
+	}
+
+	host = sdhci_pltfm_init(pdev, &sdhci_st_pdata, 0);
+	if (IS_ERR(host)) {
+		dev_err(&pdev->dev, "Failed sdhci_pltfm_init\n");
+		return PTR_ERR(host);
+	}
+
+	clk_prepare_enable(clk);
+
+	host->mmc->caps |= MMC_CAP_8_BIT_DATA | MMC_CAP_BUS_WIDTH_TEST
+			| MMC_CAP_1_8V_DDR;
+
+	if (of_property_read_bool(np, "non-removable"))
+		host->mmc->caps |= MMC_CAP_NONREMOVABLE;
+
+	pltfm_host = sdhci_priv(host);
+	pltfm_host->clk = clk;
+
+	ret = sdhci_add_host(host);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed sdhci_add_host\n");
+		goto err_out;
+	}
+
+	pltfm_host->priv = pdata;
+
+	platform_set_drvdata(pdev, host);
+
+	host_version = readw_relaxed((host->ioaddr + SDHCI_HOST_VERSION));
+
+	dev_dbg(&pdev->dev, "SDHCI ST Initialised: Host Version: 0x%x Vendor Version 0x%x\n",
+		host_version, ((host_version & SDHCI_VENDOR_VER_MASK) >>
+			       SDHCI_VENDOR_VER_SHIFT));
+
+	return 0;
+
+err_out:
+	clk_disable_unprepare(clk);
+	sdhci_pltfm_free(pdev);
+
+	return ret;
+}
+
+static int sdhci_st_remove(struct platform_device *pdev)
+{
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+	int ret;
+
+	clk_disable_unprepare(pltfm_host->clk);
+
+	ret = sdhci_pltfm_unregister(pdev);
+
+	if (pdata->rstc)
+		reset_control_assert(pdata->rstc);
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int sdhci_st_suspend(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+	int ret = sdhci_suspend_host(host);
+
+	if (ret)
+		goto out;
+
+	if (pdata->rstc)
+		reset_control_assert(pdata->rstc);
+
+	clk_disable_unprepare(pltfm_host->clk);
+out:
+	return ret;
+}
+
+static int sdhci_st_resume(struct device *dev)
+{
+	struct sdhci_host *host = dev_get_drvdata(dev);
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct st_mmc_platform_data *pdata = pltfm_host->priv;
+
+	clk_prepare_enable(pltfm_host->clk);
+
+	if (pdata->rstc)
+		reset_control_deassert(pdata->rstc);
+
+	return sdhci_resume_host(host);
+}
+
+static SIMPLE_DEV_PM_OPS(sdhci_st_pmops, sdhci_st_suspend, sdhci_st_resume);
+#define SDHCI_ST_PMOPS (&sdhci_st_pmops)
+#else
+#define SDHCI_ST_PMOPS NULL
+#endif
+
+static const struct of_device_id st_sdhci_match[] = {
+	{ .compatible = "st,sdhci" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, st_sdhci_match);
+
+static struct platform_driver sdhci_st_driver = {
+	.probe = sdhci_st_probe,
+	.remove = sdhci_st_remove,
+	.driver = {
+		   .name = "sdhci-st",
+		   .owner = THIS_MODULE,
+		   .pm = SDHCI_ST_PMOPS,
+		   .of_match_table = of_match_ptr(st_sdhci_match),
+		  },
+};
+
+module_platform_driver(sdhci_st_driver);
+
+MODULE_DESCRIPTION("SDHCI driver for STMicroelectronics SoCs");
+MODULE_AUTHOR("Giuseppe Cavallaro <peppe.cavallaro@st.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:st-sdhci");