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

2014-06-04 Thread Ulf Hansson
On 4 June 2014 10:48, Peter Griffin  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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-04 Thread Peter Griffin
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(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-04 Thread Peter Griffin
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 (_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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-04 Thread Peter Griffin
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-04 Thread Peter Griffin
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-04 Thread Ulf Hansson
On 4 June 2014 10:48, Peter Griffin peter.grif...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-02 Thread Peter Griffin
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(>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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-06-02 Thread Peter Griffin
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.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-23 Thread Ulf Hansson
On 22 May 2014 17:18, 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 
> Signed-off-by: Giuseppe Cavallaro 
> ---
>  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 000..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 
> + *
> + * 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 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#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 = _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 

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

2014-05-23 Thread Srinivas Kandagatla

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 000..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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 

Do you need this header?


+#include 
+#include 
+
+#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(>dev, "SDHCI ST platform driver\n");
+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return -ENOMEM;
+
+   pdata->rstc = devm_reset_control_get(>dev, NULL);
+   if (IS_ERR(pdata->rstc))
+   pdata->rstc = NULL;
+   else
+   reset_control_deassert(pdata->rstc);
+
+   clk =  devm_clk_get(>dev, "mmc");
+   if (IS_ERR(clk)) {
+   dev_err(>dev, "Perpheral clk not found\n");
+   return PTR_ERR(clk);
+   }
+
+   host = sdhci_pltfm_init(pdev, _st_pdata, 0);
+   if (IS_ERR(host)) {
+   dev_err(>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(>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(>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, 

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

2014-05-23 Thread Srinivas Kandagatla

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 000..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.cavall...@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 

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

2014-05-23 Thread Ulf Hansson
On 22 May 2014 17:18, Peter Griffin peter.grif...@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.grif...@linaro.org
 Signed-off-by: Giuseppe Cavallaro peppe.cavall...@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 000..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.cavall...@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 

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

2014-05-22 Thread Lee Jones

> 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 
> Signed-off-by: Giuseppe Cavallaro 
> ---
>  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(>dev, "SDHCI ST platform driver\n");
> +
> + pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return -ENOMEM;
> +
> + pdata->rstc = devm_reset_control_get(>dev, NULL);
> + if (IS_ERR(pdata->rstc))
> + pdata->rstc = NULL;
> + else
> + reset_control_deassert(pdata->rstc);
> +
> + clk =  devm_clk_get(>dev, "mmc");
> + if (IS_ERR(clk)) {
> + dev_err(>dev, "Perpheral clk not found\n");
> + return PTR_ERR(clk);
> + }
> +
> + host = sdhci_pltfm_init(pdev, _st_pdata, 0);
> + if (IS_ERR(host)) {
> + dev_err(>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(>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 ");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:st-sdhci");

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/


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

2014-05-22 Thread Maxime Coquelin

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 
Signed-off-by: Giuseppe Cavallaro 
---
  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 000..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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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 = _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(>dev, "SDHCI ST platform driver\n");

You can remove this I think.


+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return -ENOMEM;
+
+   pdata->rstc = devm_reset_control_get(>dev, NULL);
+   if (IS_ERR(pdata->rstc))
+   pdata->rstc = NULL;
+   else
+   reset_control_deassert(pdata->rstc);
+
+   clk =  devm_clk_get(>dev, "mmc");
+   if (IS_ERR(clk)) {
+   dev_err(>dev, "Perpheral clk not found\n");
+   return PTR_ERR(clk);
+   }
+
+   host = sdhci_pltfm_init(pdev, _st_pdata, 0);
+   if (IS_ERR(host)) {
+   dev_err(>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(>dev, "Failed sdhci_add_host\n");
+   goto err_out;
+   }
+
+   pltfm_host->priv = pdata;
+
+   

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

2014-05-22 Thread Peter Griffin
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 
Signed-off-by: Giuseppe Cavallaro 
---
 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 000..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 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#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 = _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(>dev, "SDHCI ST platform driver\n");
+
+   pdata = devm_kzalloc(>dev, sizeof(*pdata), GFP_KERNEL);
+   if (!pdata)
+   return -ENOMEM;
+
+   pdata->rstc = devm_reset_control_get(>dev, NULL);
+   if (IS_ERR(pdata->rstc))
+   pdata->rstc = NULL;
+   

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

2014-05-22 Thread Peter Griffin
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.grif...@linaro.org
Signed-off-by: Giuseppe Cavallaro peppe.cavall...@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 000..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.cavall...@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)
+  

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

2014-05-22 Thread Maxime Coquelin

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.grif...@linaro.org
Signed-off-by: Giuseppe Cavallaro peppe.cavall...@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 000..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.cavall...@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 = 

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

2014-05-22 Thread Lee Jones

 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.grif...@linaro.org
 Signed-off-by: Giuseppe Cavallaro peppe.cavall...@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.cavall...@st.com);
 +MODULE_LICENSE(GPL v2);
 +MODULE_ALIAS(platform:st-sdhci);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/