Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752496AbbK2UUA (ORCPT ); Sun, 29 Nov 2015 15:20:00 -0500 Received: from exsmtp01.microchip.com ([198.175.253.37]:60265 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752126AbbK2UT6 (ORCPT ); Sun, 29 Nov 2015 15:19:58 -0500 X-Greylist: delayed 308 seconds by postgrey-1.27 at vger.kernel.org; Sun, 29 Nov 2015 15:19:58 EST From: To: , CC: , , , , , , , , , , , , , , , , , , , , Subject: RE: [PATCH 11/14] mmc: sdhci-pic32: Add PIC32 SDHC host controller driver Thread-Topic: [PATCH 11/14] mmc: sdhci-pic32: Add PIC32 SDHC host controller driver Thread-Index: AQHRI/J3e4h013qRbkq0Pb+oZL4CxJ6tYM+AgAYW5iA= Date: Sun, 29 Nov 2015 20:14:48 +0000 Message-ID: References: <1448065205-15762-1-git-send-email-joshua.henderson@microchip.com> <1448065205-15762-12-git-send-email-joshua.henderson@microchip.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.10.215.90] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tATKKqS8014824 Content-Length: 17511 Lines: 494 On 11/25/2015 8:55 AM, Ulf Hansson wrote: > On 21 November 2015 at 01:17, Joshua Henderson > wrote: > > From: Andrei Pistirica > > > > This driver supports the SDHCI host controller found on the PIC32 in > > DMA or PIO mode. > > > > Signed-off-by: Andrei Pistirica > > Signed-off-by: Joshua Henderson > > --- > > drivers/mmc/host/Kconfig | 11 ++ > > drivers/mmc/host/Makefile | 1 + > > drivers/mmc/host/sdhci-pic32.c | 354 > > ++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 366 insertions(+) > > create mode 100644 drivers/mmc/host/sdhci-pic32.c > > > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig index > > af71de5..75aaed1 100644 > > --- a/drivers/mmc/host/Kconfig > > +++ b/drivers/mmc/host/Kconfig > > @@ -784,3 +784,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..7ff23a9 > > --- /dev/null > > +++ b/drivers/mmc/host/sdhci-pic32.c > > @@ -0,0 +1,354 @@ > > +/* > > + * 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include "sdhci.h" > > +#include > > Where does this include come from, any why do you need it? It's included in patch 6/14 of this patch set, which adds platform support for PIC32. It's needed to provide safe access to a register that's shared by this driver and another driver. > > > + > > +#define PIC32_MMC_OCR (MMC_VDD_32_33 | MMC_VDD_33_34) > > Why these values? These are not needed, and will be removed. > > Does the SDHCI controller manages the power to the card or do have an > external regulator available? The SDHCI controller manages power to the card. > > > + > > +#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 > > Is all the above registers/bits, specific for sdhci-pic32? The shared bus control register is in the v3.0 spec, but I don't see a define for it in the kernel. The FIFO threshold registers are specific to PIC32. > > > + > > +#define DEV_NAME "pic32-sdhci" > > + > > +struct pic32_sdhci_pdata { > > + struct platform_device *pdev; > > + struct clk *sys_clk; > > + struct clk *base_clk; > > + bool support_vsel; > > + bool piomode; > > +}; > > + > > +unsigned int pic32_sdhci_get_max_clock(struct sdhci_host *host) { > > + struct pic32_sdhci_pdata *sdhci_pdata = sdhci_priv(host); > > + unsigned int clk_rate = clk_get_rate(sdhci_pdata->base_clk); > > + struct platform_device *pdev = sdhci_pdata->pdev; > > + > > + dev_dbg(&pdev->dev, "Sdhc max clock rate: %u\n", clk_rate); > > You may want to remove "Sdhc", I think it isn't needed. > > I find the similar at some more places in the driver, please remove all of them. Ack. > > > + return clk_rate; > > +} > > + > > +unsigned int pic32_sdhci_get_min_clock(struct sdhci_host *host) { > > + struct pic32_sdhci_pdata *sdhci_pdata = sdhci_priv(host); > > + unsigned int clk_rate = clk_get_rate(sdhci_pdata->base_clk); > > + struct platform_device *pdev = sdhci_pdata->pdev; > > + > > + dev_dbg(&pdev->dev, "Sdhc min clock rate: %u\n", clk_rate); > > + return clk_rate; > > +} > > + > > +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; > > + } > > + /* > > + * SDHC will not work if JTAG is not Connected.As a workaround fix, > > + * set Card Detect Signal Selection bit in SDHC Host Control > > + * register and clear Card Detect Test Level bit in SDHC Host > > + * Control register. > > /s/SDHC/SDHCI > > If there are more places, please fix them too. Ack. > > > + */ > > + ctrl &= ~SDHCI_CTRL_CDTLVL; > > + ctrl |= SDHCI_CTRL_CDSSEL; > > + sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL); } > > + > > +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, > > + .get_min_clock = pic32_sdhci_get_min_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, > > +}; > > + > > +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) > > + bus |= (0x1 << SDH_SHARED_BUS_CLK_PINS); > > + > > + /* select first interrupt */ > > + if (irq_pins & 0x1) > > + bus |= (0x1 << SDH_SHARED_BUS_IRQ_PINS); > > + > > + 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; > > +} > > + > > +#ifdef CONFIG_OF > > +static inline int > > Why inline? This function will be removed in favor of mmc_of_parse(). > > > +sdhci_pic32_probe_dts(struct platform_device *pdev, > > + struct pic32_sdhci_pdata *boarddata) { > > + struct device_node *np = pdev->dev.of_node; > > + > > + if (!np) > > + return -ENODEV; > > + > > + if (of_find_property(np, "no-1-8-v", NULL)) > > Please don't use this property as it's broken. It has two different purposes and > those are conflicting as discussed here[1]. Ack. > > You have two options, either invent a new binding which provides information > about which signal voltage that *is* supported, or use the current existing > common MMC dt bindings to override the SDHCI capabilities register. > We will use existing common mmc DT bindings to override the SDHCI capabilities register. > > + boarddata->support_vsel = true; > > + else > > + boarddata->support_vsel = false; > > + > > + if (of_find_property(np, "piomode", NULL)) > > What's this? Are there different versions of the controller, one supporting > "piomode" another not? > > Moreover, where is this being documented? This will be removed. We won't be supporting pio mode. > > > + boarddata->piomode = true; > > + else > > + boarddata->piomode = false; > > + > > + return 0; > > +} > > +#else > > +static inline int > > +sdhci_pic32_probe_dts(struct platform_device *pdev, > > + struct pic32_sdhci_pdata *boarddata) { > > + return -ENODEV; > > +} > > +#endif > > I don't think you need this for the CONFIG_OF unset case. There should be stub > functions for of_find_property() etc. This function will be removed. > > > + > > +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; > > + > > + 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); > > + > > + if (sdhci_pic32_probe_dts(pdev, sdhci_pdata) < 0) { > > + ret = -EINVAL; > > + dev_err(&pdev->dev, "no device tree information %d\n", ret); > > + goto err_host1; > > + } > > + > > + 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; > > + } > > + > > + if (!sdhci_pdata->piomode) { > > + 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"); > > + 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); > > + if (ret) { > > + dev_dbg(&pdev->dev, "Error enabling clock\n"); > > + 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); > > + > > + if (sdhci_pdata->support_vsel) > > + host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V; > > + > > + if (sdhci_pdata->piomode) > > + host->quirks |= SDHCI_QUIRK_BROKEN_ADMA | > > + SDHCI_QUIRK_BROKEN_DMA; > > + > > + host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT; > > + > > + host->mmc->ocr_avail = PIC32_MMC_OCR; > > + > > + ret = pic32_sdhci_probe_platform(pdev, sdhci_pdata); > > + if (ret) { > > + dev_err(&pdev->dev, "failed to probe platform!\n"); > > + goto err_host; > > + } > > Any reason why you don't want to use mmc_of_parse() to parse for common > mmc DT bindings? We will update the driver to use mmc_of_parse(). > > > + > > + 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: > > + devm_iounmap(&pdev->dev, host->ioaddr); > > +err_host1: > > + 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) > > + dead = 1; > > + > > + sdhci_remove_host(host, dead); > > + clk_disable_unprepare(sdhci_pdata->base_clk); > > + clk_disable_unprepare(sdhci_pdata->sys_clk); > > + devm_iounmap(&pdev->dev, host->ioaddr); > > + sdhci_free_host(host); > > + > > + return 0; > > +} > > + > > +static const struct of_device_id pic32_sdhci_id_table[] = { > > + { .compatible = "microchip,pic32-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 > > > > Kind regards > Uffe Thanks, Paul ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?