[U-Boot] [PATCH v5 4/8] MMC: sdhci: Add support for dove sdhci

[Jagan Teki]

Hi,

I have a few comments.

I myself not convenient with the capital letters on commit header, IMHO
"MMC: dove_sdhci: "  ==> "mmc: dove_sdhci: "

perhaps not much an issue we are taking about.

On Wed, Jun 26, 2013 at 2:57 AM, Sascha Silbe <t-uboot at infra-silbe.de> wrote:
> This adds a driver for the sdhci controller found on Dove SoCs.
>
> Signed-off-by: Sascha Silbe <t-uboot at infra-silbe.de>
> ---
>  v4->v5: no changes
>
>  arch/arm/include/asm/arch-dove/mmc.h |  27 ++++++++++
>  drivers/mmc/Makefile                 |   1 +
>  drivers/mmc/dove_sdhci.c             | 101 +++++++++++++++++++++++++++++++++++
>  3 files changed, 129 insertions(+)
>
> diff --git a/arch/arm/include/asm/arch-dove/mmc.h b/arch/arm/include/asm/arch-dove/mmc.h
> new file mode 100644
> index 0000000..579396c
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-dove/mmc.h
> @@ -0,0 +1,27 @@
> +/*
> + * Marvell Dove SoC SDHCI
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#ifndef _DOVEMMC_H
> +#define _DOVEMMC_H
> +
> +int dove_sdhci_init(int num);
> +#endif /* _DOVEMMC_H */

Do you really require this file just for a single func deceleration.
I have a another thought like..just directly call dove_sdhci_init from
board file.

> diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile
> index 24648a2..074af75 100644
> --- a/drivers/mmc/Makefile
> +++ b/drivers/mmc/Makefile
> @@ -28,6 +28,7 @@ LIB   := $(obj)libmmc.o
>
>  COBJS-$(CONFIG_BFIN_SDH) += bfin_sdh.o
>  COBJS-$(CONFIG_DAVINCI_MMC) += davinci_mmc.o
> +COBJS-$(CONFIG_DOVE_SDHCI) += dove_sdhci.o
>  COBJS-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o
>  COBJS-$(CONFIG_FTSDC010) += ftsdc010_mci.o
>  COBJS-$(CONFIG_GENERIC_MMC) += mmc.o
> diff --git a/drivers/mmc/dove_sdhci.c b/drivers/mmc/dove_sdhci.c
> new file mode 100644
> index 0000000..ac15fd7
> --- /dev/null
> +++ b/drivers/mmc/dove_sdhci.c
> @@ -0,0 +1,101 @@
> +/*
> + *
> + * Marvell Dove SDHCI driver
> + *
> + * Sebastian Hesselbarth <sebastian.hesselbarth at gmail.com>
> + *
> + * Based on linux drivers/mmc/host/sdhci-dove.c
> + * by: Saeed Bishara <saeed at marvell.com>
> + *     Mike Rapoport <mike at compulab.co.il>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#include <common.h>
> +#include <malloc.h>
> +#include <sdhci.h>
> +#include <asm/arch/dove.h>
> +
> +static u16 dove_sdhci_readw(struct sdhci_host *host, int reg)
> +{
> +       u16 ret;
> +
> +       switch (reg) {
> +       case SDHCI_HOST_VERSION:
> +       case SDHCI_SLOT_INT_STATUS:
> +               /* those registers don't exist */
> +               return 0;
> +       default:
> +               ret = readw(host->ioaddr + reg);
> +       }
> +
> +       return ret;
> +}
> +
> +static u32 dove_sdhci_readl(struct sdhci_host *host, int reg)
> +{
> +       u32 ret;
> +
> +       switch (reg) {
> +       case SDHCI_CAPABILITIES:
> +               ret = readl(host->ioaddr + reg);
> +               /* Mask the support for 3.0V */
> +               ret &= ~SDHCI_CAN_VDD_300;
> +               break;
> +       default:
> +               ret = readl(host->ioaddr + reg);
> +       }
> +
> +       return ret;
> +}
> +
> +static struct sdhci_ops dove_sdhci_ops = {
> +       .read_w = dove_sdhci_readw,
> +       .read_l = dove_sdhci_readl,
> +};
> +
> +static struct sdhci_host hosts[2] = {
> +       {
> +               .name = "Dove SDHCI0",
> +               .ioaddr = (void *)DOVE_SDIO0_BASE,
> +       },
> +       {
> +               .name = "Dove SDHCI1",
> +               .ioaddr = (void *)DOVE_SDIO1_BASE,
> +       },
> +};
> +
> +int dove_sdhci_init(int num)
> +{
> +       struct sdhci_host *host;
> +
> +       if (num < 0 || num > 1)
> +               return 1;

What exactly this logic is for, are you able to detect the sdhci host
at runtime out of 2.
if not so may be you directly call dove_sdhci_init from board file
with respective base.

> +
> +       host = &hosts[num];
> +
> +       if (host->version)
> +               return 1;
> +
> +       host->quirks =
> +               SDHCI_QUIRK_NO_HISPD_BIT |
> +               SDHCI_QUIRK_BROKEN_R1B |
> +               SDHCI_QUIRK_32BIT_DMA_ADDR;
> +       host->version = SDHCI_SPEC_200;

Better to read the version using sdhci_readw() instead of direct assignment.

--
Thanks,
Jagan.

> +       host->ops = &dove_sdhci_ops;
> +
> +       add_sdhci(host, 50000000, 4000000);
> +       return 0;
> +}
> --
> 1.8.2.1
>