Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller
On Fri, 26 Jul 2019, at 15:27, Adrian Hunter wrote: > On 26/07/19 3:52 AM, Andrew Jeffery wrote: > > On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: > >> On 12/07/19 6:32 AM, Andrew Jeffery wrote: > >>> +static int aspeed_sdhci_probe(struct platform_device *pdev) > >>> +{ > >>> + struct sdhci_pltfm_host *pltfm_host; > >>> + struct aspeed_sdhci *dev; > >>> + struct sdhci_host *host; > >>> + struct resource *res; > >>> + int slot; > >>> + int ret; > >>> + > >>> + host = sdhci_pltfm_init(pdev, _sdc_pdata, sizeof(*dev)); > >>> + if (IS_ERR(host)) > >>> + return PTR_ERR(host); > >>> + > >>> + pltfm_host = sdhci_priv(host); > >>> + dev = sdhci_pltfm_priv(pltfm_host); > >>> + dev->parent = dev_get_drvdata(pdev->dev.parent); > >>> + > >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >>> + slot = aspeed_sdhci_calculate_slot(dev, res); > >>> + if (slot < 0) > >>> + return slot; > >>> + dev_info(>dev, "Configuring for slot %d\n", slot); > >>> + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; > >> > >> That implies that you only support 2 slots which begs the question why > >> you don't validate slot. > > > > I'm not sure what you mean here, but I'll dig into it. > > I just meant, if you only support 2 slots: > > if (slot > 1) > return -EINVAL; > Oh, sure.
Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller
On 26/07/19 3:52 AM, Andrew Jeffery wrote: > On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: >> On 12/07/19 6:32 AM, Andrew Jeffery wrote: >>> +static int aspeed_sdhci_probe(struct platform_device *pdev) >>> +{ >>> + struct sdhci_pltfm_host *pltfm_host; >>> + struct aspeed_sdhci *dev; >>> + struct sdhci_host *host; >>> + struct resource *res; >>> + int slot; >>> + int ret; >>> + >>> + host = sdhci_pltfm_init(pdev, _sdc_pdata, sizeof(*dev)); >>> + if (IS_ERR(host)) >>> + return PTR_ERR(host); >>> + >>> + pltfm_host = sdhci_priv(host); >>> + dev = sdhci_pltfm_priv(pltfm_host); >>> + dev->parent = dev_get_drvdata(pdev->dev.parent); >>> + >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + slot = aspeed_sdhci_calculate_slot(dev, res); >>> + if (slot < 0) >>> + return slot; >>> + dev_info(>dev, "Configuring for slot %d\n", slot); >>> + dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8; >> >> That implies that you only support 2 slots which begs the question why >> you don't validate slot. > > I'm not sure what you mean here, but I'll dig into it. I just meant, if you only support 2 slots: if (slot > 1) return -EINVAL;
Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller
On Thu, 25 Jul 2019, at 22:49, Adrian Hunter wrote: > On 12/07/19 6:32 AM, Andrew Jeffery wrote: > > Add a minimal driver for ASPEED's SD controller, which exposes two > > SDHCIs. > > > > The ASPEED design implements a common register set for the SDHCIs, and > > moves some of the standard configuration elements out to this common > > area (e.g. 8-bit mode, and card detect configuration which is not > > currently supported). > > > > The SD controller has a dedicated hardware interrupt that is shared > > between the slots. The common register set exposes information on which > > slot triggered the interrupt; early revisions of the patch introduced an > > irqchip for the register, but reality is it doesn't behave as an > > irqchip, and the result fits awkwardly into the irqchip APIs. Instead > > I've taken the simple approach of using the IRQ as a shared IRQ with > > some minor performance impact for the second slot. > > > > Ryan was the original author of the patch - I've taken his work and > > massaged it to drop the irqchip support and rework the devicetree > > integration. The driver has been smoke tested under qemu against a > > minimal SD controller model and lightly tested on an ast2500-evb. > > > > Signed-off-by: Ryan Chen > > Signed-off-by: Andrew Jeffery > > Looks fine. Few minor comments below. > > > --- > > In v2: > > > > * Drop unnecesasry MODULE_DEVICE_TABLE() > > * Rename sd-controller compatible > > * Add IBM copyright > > * Drop unnecesary data assignment in of match table entries > > * Derive the slot from the SDHCI offset > > > > drivers/mmc/host/Kconfig | 12 ++ > > drivers/mmc/host/Makefile | 1 + > > drivers/mmc/host/sdhci-of-aspeed.c | 326 + > > 3 files changed, 339 insertions(+) > > create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c > > > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > > index 931770f17087..2bb5e1264b3d 100644 > > --- a/drivers/mmc/host/Kconfig > > +++ b/drivers/mmc/host/Kconfig > > @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN > > > > If unsure, say N. > > > > +config MMC_SDHCI_OF_ASPEED > > + tristate "SDHCI OF support for the ASPEED SDHCI controller" > > + depends on MMC_SDHCI_PLTFM > > + depends on OF > > + help > > + This selects the ASPEED Secure Digital Host Controller Interface. > > + > > + If you have a controller with this interface, say Y or M here. You > > + also need to enable an appropriate bus interface. > > + > > + If unsure, say N. > > + > > config MMC_SDHCI_OF_AT91 > > tristate "SDHCI OF support for the Atmel SDMMC controller" > > depends on MMC_SDHCI_PLTFM > > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > > index 73578718f119..390ee162fe71 100644 > > --- a/drivers/mmc/host/Makefile > > +++ b/drivers/mmc/host/Makefile > > @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o > > obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o > > obj-$(CONFIG_MMC_SDHCI_TEGRA) += sdhci-tegra.o > > obj-$(CONFIG_MMC_SDHCI_OF_ARASAN) += sdhci-of-arasan.o > > +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED) += sdhci-of-aspeed.o > > obj-$(CONFIG_MMC_SDHCI_OF_AT91)+= sdhci-of-at91.o > > obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o > > obj-$(CONFIG_MMC_SDHCI_OF_HLWD)+= sdhci-of-hlwd.o > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > > b/drivers/mmc/host/sdhci-of-aspeed.c > > new file mode 100644 > > index ..9528e43c257d > > --- /dev/null > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > > @@ -0,0 +1,326 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > +/* Copyright (C) 2019 ASPEED Technology Inc. */ > > +/* Copyright (C) 2019 IBM Corp. */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#include "sdhci-pltfm.h" > > + > > +#define ASPEED_SDC_INFO0x00 > > +#define ASPEED_SDC_S1MMC8BIT(25) > > +#define ASPEED_SDC_S0MMC8BIT(24) > > + > > +struct aspeed_sdc { > > + struct clk *clk; > > + struct resource *res; > > + > > + spinlock_t lock; > > + void __iomem *regs; > > +}; > > + > > +struct aspeed_sdhci { > > + struct aspeed_sdc *parent; > > + u32 width_mask; > > +}; > > + > > +static void aspeed_sdc_bus_width(struct aspeed_sdc *sdc, > > +struct aspeed_sdhci *sdhci, bool bus8) > > The function name threw me at first. I suggest: > > static void aspeed_sdhci_set_clr_8_bit_mode(struct aspeed_sdhci *aspeed_sdhci, > bool bus8) I wasn't real happy with the name I picked, so was hoping to some degree to be bike-shedded on it. I'll fix it up. > { > struct aspeed_sdc *aspeed_sdc = aspeed_sdhci->parent; > > > +{ > > + u32 info; > > + > > + /* Set/clear 8 bit mode */ > > +
Re: [PATCH v2 2/2] mmc: Add support for the ASPEED SD controller
On 12/07/19 6:32 AM, Andrew Jeffery wrote: > Add a minimal driver for ASPEED's SD controller, which exposes two > SDHCIs. > > The ASPEED design implements a common register set for the SDHCIs, and > moves some of the standard configuration elements out to this common > area (e.g. 8-bit mode, and card detect configuration which is not > currently supported). > > The SD controller has a dedicated hardware interrupt that is shared > between the slots. The common register set exposes information on which > slot triggered the interrupt; early revisions of the patch introduced an > irqchip for the register, but reality is it doesn't behave as an > irqchip, and the result fits awkwardly into the irqchip APIs. Instead > I've taken the simple approach of using the IRQ as a shared IRQ with > some minor performance impact for the second slot. > > Ryan was the original author of the patch - I've taken his work and > massaged it to drop the irqchip support and rework the devicetree > integration. The driver has been smoke tested under qemu against a > minimal SD controller model and lightly tested on an ast2500-evb. > > Signed-off-by: Ryan Chen > Signed-off-by: Andrew Jeffery Looks fine. Few minor comments below. > --- > In v2: > > * Drop unnecesasry MODULE_DEVICE_TABLE() > * Rename sd-controller compatible > * Add IBM copyright > * Drop unnecesary data assignment in of match table entries > * Derive the slot from the SDHCI offset > > drivers/mmc/host/Kconfig | 12 ++ > drivers/mmc/host/Makefile | 1 + > drivers/mmc/host/sdhci-of-aspeed.c | 326 + > 3 files changed, 339 insertions(+) > create mode 100644 drivers/mmc/host/sdhci-of-aspeed.c > > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig > index 931770f17087..2bb5e1264b3d 100644 > --- a/drivers/mmc/host/Kconfig > +++ b/drivers/mmc/host/Kconfig > @@ -154,6 +154,18 @@ config MMC_SDHCI_OF_ARASAN > > If unsure, say N. > > +config MMC_SDHCI_OF_ASPEED > + tristate "SDHCI OF support for the ASPEED SDHCI controller" > + depends on MMC_SDHCI_PLTFM > + depends on OF > + help > + This selects the ASPEED Secure Digital Host Controller Interface. > + > + If you have a controller with this interface, say Y or M here. You > + also need to enable an appropriate bus interface. > + > + If unsure, say N. > + > config MMC_SDHCI_OF_AT91 > tristate "SDHCI OF support for the Atmel SDMMC controller" > depends on MMC_SDHCI_PLTFM > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile > index 73578718f119..390ee162fe71 100644 > --- a/drivers/mmc/host/Makefile > +++ b/drivers/mmc/host/Makefile > @@ -84,6 +84,7 @@ obj-$(CONFIG_MMC_SDHCI_ESDHC_IMX) += sdhci-esdhc-imx.o > obj-$(CONFIG_MMC_SDHCI_DOVE) += sdhci-dove.o > obj-$(CONFIG_MMC_SDHCI_TEGRA)+= sdhci-tegra.o > obj-$(CONFIG_MMC_SDHCI_OF_ARASAN)+= sdhci-of-arasan.o > +obj-$(CONFIG_MMC_SDHCI_OF_ASPEED)+= sdhci-of-aspeed.o > obj-$(CONFIG_MMC_SDHCI_OF_AT91) += sdhci-of-at91.o > obj-$(CONFIG_MMC_SDHCI_OF_ESDHC) += sdhci-of-esdhc.o > obj-$(CONFIG_MMC_SDHCI_OF_HLWD) += sdhci-of-hlwd.o > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c > b/drivers/mmc/host/sdhci-of-aspeed.c > new file mode 100644 > index ..9528e43c257d > --- /dev/null > +++ b/drivers/mmc/host/sdhci-of-aspeed.c > @@ -0,0 +1,326 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* Copyright (C) 2019 ASPEED Technology Inc. */ > +/* Copyright (C) 2019 IBM Corp. */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "sdhci-pltfm.h" > + > +#define ASPEED_SDC_INFO 0x00 > +#define ASPEED_SDC_S1MMC8 BIT(25) > +#define ASPEED_SDC_S0MMC8 BIT(24) > + > +struct aspeed_sdc { > + struct clk *clk; > + struct resource *res; > + > + spinlock_t lock; > + void __iomem *regs; > +}; > + > +struct aspeed_sdhci { > + struct aspeed_sdc *parent; > + u32 width_mask; > +}; > + > +static void aspeed_sdc_bus_width(struct aspeed_sdc *sdc, > + struct aspeed_sdhci *sdhci, bool bus8) The function name threw me at first. I suggest: static void aspeed_sdhci_set_clr_8_bit_mode(struct aspeed_sdhci *aspeed_sdhci, bool bus8) { struct aspeed_sdc *aspeed_sdc = aspeed_sdhci->parent; > +{ > + u32 info; > + > + /* Set/clear 8 bit mode */ > + spin_lock(>lock); > + info = readl(sdc->regs + ASPEED_SDC_INFO); > + if (bus8) > + info |= sdhci->width_mask; > + else > + info &= ~sdhci->width_mask; > + writel(info, sdc->regs + ASPEED_SDC_INFO); > + spin_unlock(>lock); > +} > + > +static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int > clock) > +{ > + unsigned long timeout; > +