Re: [PATCH v2 12/14] mmc: sdhci-pic32: Add PIC32 SDHCI host controller driver

From: Andy Green
Date: Mon Dec 14 2015 - 19:32:31 EST

Next message: Jianqun Xu: "Re: [PATCH v1 1/3] i2c: rk3x: add calc_divs ops for new version"
Previous message: Stephen Rothwell: "linux-next: manual merge of the net-next tree with the net tree"
In reply to: Joshua Henderson: "[PATCH v2 12/14] mmc: sdhci-pic32: Add PIC32 SDHCI host controller driver"
Next in thread: Joshua Henderson: "[PATCH v2 11/14] DEVICETREE: Add bindings for PIC32 SDHCI host controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi... looks good, just some small general comments.

On 15 December 2015 at 06:42, Joshua Henderson
<joshua.henderson@xxxxxxxxxxxxx> wrote:
> From: Andrei Pistirica <andrei.pistirica@xxxxxxxxxxxxx>
>
> This driver supports the SDHCI host controller found on a PIC32.
>
> Signed-off-by: Andrei Pistirica <andrei.pistirica@xxxxxxxxxxxxx>
> Signed-off-by: Joshua Henderson <joshua.henderson@xxxxxxxxxxxxx>
> Cc: Ralf Baechle <ralf@xxxxxxxxxxxxxx>
> ---
> drivers/mmc/host/Kconfig | 11 ++
> drivers/mmc/host/Makefile | 1 +
> drivers/mmc/host/sdhci-pic32.c | 291 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 303 insertions(+)
> create mode 100644 drivers/mmc/host/sdhci-pic32.c
>
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 1dee533..1a3a42b 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -785,3 +785,14 @@ config MMC_MTK
> If you have a machine with a integrated SD/MMC card reader, say Y or M here.
> This is needed if support for any SD/SDIO/MMC devices is required.
> If unsure, say N.
> +
> +config MMC_SDHCI_MICROCHIP_PIC32
> + tristate "Microchip PIC32MZDA SDHCI support"
> + depends on MMC_SDHCI && PIC32MZDA
> + help
> + This selects the Secure Digital Host Controller Interface (SDHCI)
> + for PIC32MZDA platform.
> +
> + If you have a controller with this interface, say Y or M here.
> +
> + If unsure, say N.
> diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> index 3595f83..af918d2 100644
> --- a/drivers/mmc/host/Makefile
> +++ b/drivers/mmc/host/Makefile
> @@ -75,6 +75,7 @@ obj-$(CONFIG_MMC_SDHCI_BCM2835) += sdhci-bcm2835.o
> obj-$(CONFIG_MMC_SDHCI_IPROC) += sdhci-iproc.o
> obj-$(CONFIG_MMC_SDHCI_MSM) += sdhci-msm.o
> obj-$(CONFIG_MMC_SDHCI_ST) += sdhci-st.o
> +obj-$(CONFIG_MMC_SDHCI_MICROCHIP_PIC32) += sdhci-pic32.o
>
> ifeq ($(CONFIG_CB710_DEBUG),y)
> CFLAGS-cb710-mmc += -DDEBUG
> diff --git a/drivers/mmc/host/sdhci-pic32.c b/drivers/mmc/host/sdhci-pic32.c
> new file mode 100644
> index 0000000..b7d7da2
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-pic32.c
> @@ -0,0 +1,291 @@
> +/*
> + * Support of SDHCI platform devices for Microchip PIC32.
> + *
> + * Copyright (C) 2015 Microchip
> + * Andrei Pistirica, Paul Thacker
> + *
> + * Inspired by sdhci-pltfm.c
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/highmem.h>
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/mmc/host.h>
> +#include <linux/io.h>
> +#include "sdhci.h"
> +#include <linux/platform_data/sdhci-pic32.h>
> +
> +#define SDH_SHARED_BUS_CTRL 0x000000E0
> +#define SDH_SHARED_BUS_NR_CLK_PINS_MASK 0x7
> +#define SDH_SHARED_BUS_NR_IRQ_PINS_MASK 0x30
> +#define SDH_SHARED_BUS_CLK_PINS 0x10
> +#define SDH_SHARED_BUS_IRQ_PINS 0x14
> +#define SDH_CAPS_SDH_SLOT_TYPE_MASK 0xC0000000
> +#define SDH_SLOT_TYPE_REMOVABLE 0x0
> +#define SDH_SLOT_TYPE_EMBEDDED 0x1
> +#define SDH_SLOT_TYPE_SHARED_BUS 0x2
> +#define SDHCI_CTRL_CDSSEL 0x80
> +#define SDHCI_CTRL_CDTLVL 0x40
> +
> +#define ADMA_FIFO_RD_THSHLD 512
> +#define ADMA_FIFO_WR_THSHLD 512
> +
> +#define DEV_NAME "pic32-sdhci"

Is there any point defining this when it only has one use in the driver?

> +struct pic32_sdhci_pdata {
> + struct platform_device *pdev;
> + struct clk *sys_clk;
> + struct clk *base_clk;
> +};
> +
> +static unsigned int pic32_sdhci_get_max_clock(struct sdhci_host *host)
> +{
> + struct pic32_sdhci_pdata *sdhci_pdata = sdhci_priv(host);
> +
> + return clk_get_rate(sdhci_pdata->base_clk);
> +}
> +
> +static void pic32_sdhci_set_bus_width(struct sdhci_host *host, int width)
> +{
> + u8 ctrl;
> +
> + ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> + if (width == MMC_BUS_WIDTH_8) {
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl |= SDHCI_CTRL_8BITBUS;
> + } else {
> + if (host->version >= SDHCI_SPEC_300)
> + ctrl &= ~SDHCI_CTRL_8BITBUS;
> + if (width == MMC_BUS_WIDTH_4)
> + ctrl |= SDHCI_CTRL_4BITBUS;
> + else
> + ctrl &= ~SDHCI_CTRL_4BITBUS;
> + }
> + /*
> + * SDHCI will not work if JTAG is not Connected.As a workaround fix,
> + * set Card Detect Signal Selection bit in SDHCI Host Control
> + * register and clear Card Detect Test Level bit in SDHCI Host
> + * Control register.
> + */

Isn't this a clearer explanation, if I understood?

"Without setting CD select and test bits now, SDHCI only works with
JTAG connected."

> + ctrl &= ~SDHCI_CTRL_CDTLVL;
> + ctrl |= SDHCI_CTRL_CDSSEL;
> + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);

Also... is that a feature of the SDHCI IP or this particular chip's
implementation of it? I guess if there is only one implementation
right now that has that restriction worry about making it selectable
later. But if the implementation, it might make sense to also put the
affected implementation name in the comment to make it clear.

> +}
> +
> +static unsigned int pic32_sdhci_get_ro(struct sdhci_host *host)
> +{
> + /*
> + * The SDHCI_WRITE_PROTECT bit is unstable on current hardware so we
> + * can't depend on its value in any way.
> + */
> + return 0;
> +}
> +
> +static const struct sdhci_ops pic32_sdhci_ops = {
> + .get_max_clock = pic32_sdhci_get_max_clock,
> + .set_clock = sdhci_set_clock,
> + .set_bus_width = pic32_sdhci_set_bus_width,
> + .reset = sdhci_reset,
> + .set_uhs_signaling = sdhci_set_uhs_signaling,
> + .get_ro = pic32_sdhci_get_ro,
> +};
> +
> +static void pic32_sdhci_shared_bus(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + u32 bus = readl(host->ioaddr + SDH_SHARED_BUS_CTRL);
> + u32 clk_pins = (bus & SDH_SHARED_BUS_NR_CLK_PINS_MASK) >> 0;
> + u32 irq_pins = (bus & SDH_SHARED_BUS_NR_IRQ_PINS_MASK) >> 4;
> +
> + /* select first clock */
> + if (clk_pins & 0x1)

BIT(0)? Also a couple of lines down.

> + bus |= (0x1 << SDH_SHARED_BUS_CLK_PINS);

I know it's popular but there is no meaning or use in "0x1" where you
could just say "1".

> + /* select first interrupt */
> + if (irq_pins & 0x1)
> + bus |= (0x1 << SDH_SHARED_BUS_IRQ_PINS);

As above.

> + writel(bus, host->ioaddr + SDH_SHARED_BUS_CTRL);
> +}
> +
> +static int pic32_sdhci_probe_platform(struct platform_device *pdev,
> + struct pic32_sdhci_pdata *pdata)
> +{
> + int ret = 0;
> + u32 caps_slot_type;
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> +
> + /* Check card slot connected on shared bus. */
> + host->caps = readl(host->ioaddr + SDHCI_CAPABILITIES);
> + caps_slot_type = (host->caps & SDH_CAPS_SDH_SLOT_TYPE_MASK) >> 30;
> + if (caps_slot_type == SDH_SLOT_TYPE_SHARED_BUS)
> + pic32_sdhci_shared_bus(pdev);
> +
> + return ret;
> +}
> +
> +static int pic32_sdhci_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sdhci_host *host;
> + struct resource *iomem;
> + struct pic32_sdhci_pdata *sdhci_pdata;
> + struct pic32_sdhci_platform_data *plat_data;
> + unsigned int clk_rate = 0;
> + int ret;
> + struct pinctrl *pinctrl;

It's hardly critical but for extra gold star arranging local vars in
length order (longest first) is nice.

> +
> + host = sdhci_alloc_host(dev, sizeof(*sdhci_pdata));
> + if (IS_ERR(host)) {
> + ret = PTR_ERR(host);
> + dev_err(&pdev->dev, "cannot allocate memory for sdhci\n");
> + goto err;
> + }
> +
> + sdhci_pdata = sdhci_priv(host);
> + sdhci_pdata->pdev = pdev;
> + platform_set_drvdata(pdev, host);
> +
> + iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + host->ioaddr = devm_ioremap_resource(&pdev->dev, iomem);
> + if (IS_ERR(host->ioaddr)) {
> + ret = PTR_ERR(host->ioaddr);
> + dev_err(&pdev->dev, "unable to map iomem: %d\n", ret);
> + goto err_host;
> + }
> +
> + plat_data = pdev->dev.platform_data;
> + if (plat_data && plat_data->setup_dma) {
> + ret = plat_data->setup_dma(ADMA_FIFO_RD_THSHLD,
> + ADMA_FIFO_WR_THSHLD);
> + if (ret)
> + goto err_host;
> + }
> +
> + pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> + if (IS_ERR(pinctrl)) {
> + ret = PTR_ERR(pinctrl);
> + dev_warn(&pdev->dev, "No pinctrl provided %d\n", ret);
> + if (ret == -EPROBE_DEFER)
> + goto err_host;
> + }
> +
> + host->ops = &pic32_sdhci_ops;
> + host->irq = platform_get_irq(pdev, 0);
> +
> + sdhci_pdata->sys_clk = devm_clk_get(&pdev->dev, "sys_clk");
> + if (IS_ERR(sdhci_pdata->sys_clk)) {
> + ret = PTR_ERR(sdhci_pdata->sys_clk);
> + dev_err(&pdev->dev, "Error getting clock\n");
> + goto err_host;
> + }
> +
> + /* Enable clock when available! */
> + ret = clk_prepare_enable(sdhci_pdata->sys_clk);
> + if (ret) {
> + dev_dbg(&pdev->dev, "Error enabling clock\n");

Shouldn't this be dev_err()? You don't survive not having the clock
in the stanza above. So if you have the clock, you would want to know
if it didn't deal with the clk_prepare_enable().

The comment 4 lines above is also wrong if so.

> + goto err_host;
> + }
> +
> + /* SDH CLK enable */
> + sdhci_pdata->base_clk = devm_clk_get(&pdev->dev, "base_clk");
> + if (IS_ERR(sdhci_pdata->base_clk)) {
> + ret = PTR_ERR(sdhci_pdata->base_clk);
> + dev_err(&pdev->dev, "Error getting clock\n");
> + goto err_host;
> + }
> +
> + /* Enable clock when available! */
> + ret = clk_prepare_enable(sdhci_pdata->base_clk);

Again the comment seems wrong.

> + if (ret) {
> + dev_dbg(&pdev->dev, "Error enabling clock\n");

And if I understood it, dev_err() needed.

> + goto err_host;
> + }
> +
> + clk_rate = clk_get_rate(sdhci_pdata->base_clk);
> + dev_dbg(&pdev->dev, "base clock at: %u\n", clk_rate);
> + clk_rate = clk_get_rate(sdhci_pdata->sys_clk);
> + dev_dbg(&pdev->dev, "sys clock at: %u\n", clk_rate);
> +
> + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
> +

Probably can lose the blank line.

> + host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT;
> +
> + ret = mmc_of_parse(host->mmc);
> + if (ret)
> + goto err_host;
> +
> + ret = pic32_sdhci_probe_platform(pdev, sdhci_pdata);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to probe platform!\n");
> + goto err_host;
> + }
> +
> + ret = sdhci_add_host(host);
> + if (ret) {
> + dev_dbg(&pdev->dev, "error adding host\n");
> + goto err_host;
> + }
> +
> + dev_info(&pdev->dev, "Successfully added sdhci host\n");
> + return 0;
> +
> +err_host:
> + sdhci_free_host(host);
> +err:
> + dev_err(&pdev->dev, "pic32-sdhci probe failed: %d\n", ret);
> + return ret;
> +}
> +
> +static int pic32_sdhci_remove(struct platform_device *pdev)
> +{
> + struct sdhci_host *host = platform_get_drvdata(pdev);
> + struct pic32_sdhci_pdata *sdhci_pdata = sdhci_priv(host);
> + int dead = 0;
> + u32 scratch;
> +
> + scratch = readl(host->ioaddr + SDHCI_INT_STATUS);
> + if (scratch == (u32)-1)

Since it's not actually related to signed, (u32)~0 might be clearer.

> + dead = 1;
> +
> + sdhci_remove_host(host, dead);

You could get rid of "dead" and have

sdhci_remove_host(host, scratch == (u32)~0);

> + clk_disable_unprepare(sdhci_pdata->base_clk);
> + clk_disable_unprepare(sdhci_pdata->sys_clk);
> + sdhci_free_host(host);
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pic32_sdhci_id_table[] = {
> + { .compatible = "microchip,pic32mzda-sdhci" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, pic32_sdhci_id_table);
> +
> +static struct platform_driver pic32_sdhci_driver = {
> + .driver = {
> + .name = DEV_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(pic32_sdhci_id_table),
> + },
> + .probe = pic32_sdhci_probe,
> + .remove = pic32_sdhci_remove,
> +};
> +
> +module_platform_driver(pic32_sdhci_driver);
> +
> +MODULE_DESCRIPTION("Microchip PIC32 SDHCI driver");
> +MODULE_AUTHOR("Pistirica Sorin Andrei & Sandeep Sheriker");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Jianqun Xu: "Re: [PATCH v1 1/3] i2c: rk3x: add calc_divs ops for new version"
Previous message: Stephen Rothwell: "linux-next: manual merge of the net-next tree with the net tree"
In reply to: Joshua Henderson: "[PATCH v2 12/14] mmc: sdhci-pic32: Add PIC32 SDHCI host controller driver"
Next in thread: Joshua Henderson: "[PATCH v2 11/14] DEVICETREE: Add bindings for PIC32 SDHCI host controller"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]