| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170323085812.GA2250@hardcore>
Date: Thu, 23 Mar 2017 09:58:12 +0100
From: Jan Glauber <jan.glauber@...iumnetworks.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
David Daney <ddaney@...iumnetworks.com>,
"Steven J . Hill" <Steven.Hill@...ium.com>
Subject: Re: [PATCH v12 6/9] mmc: cavium: Add MMC PCI driver for ThunderX SOCs
On Fri, Mar 17, 2017 at 03:58:26PM +0100, Ulf Hansson wrote:
> On 10 March 2017 at 14:25, Jan Glauber <jglauber@...ium.com> wrote:
> > Add a platform driver for ThunderX ARM SOCs.
> >
> > Signed-off-by: Jan Glauber <jglauber@...ium.com>
> > ---
> > drivers/mmc/host/Kconfig | 10 ++
> > drivers/mmc/host/Makefile | 2 +
> > drivers/mmc/host/cavium-mmc.h | 10 +-
> > drivers/mmc/host/cavium-pci-thunderx.c | 198 +++++++++++++++++++++++++++++++++
> > 4 files changed, 218 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/mmc/host/cavium-pci-thunderx.c
> >
> > diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> > index 68cc811..3983dee 100644
> > --- a/drivers/mmc/host/Kconfig
> > +++ b/drivers/mmc/host/Kconfig
> > @@ -632,6 +632,16 @@ config MMC_CAVIUM_OCTEON
> >
> > If unsure, say N.
> >
> > +config MMC_CAVIUM_THUNDERX
> > + tristate "Cavium ThunderX SD/MMC Card Interface support"
> > + depends on PCI && 64BIT && (ARM64 || COMPILE_TEST)
> > + select GPIO_THUNDERX
>
> Do you really need to select GPIO_THUNDERX? What is the relationship?
I don't know much about gpio, but in the end despite all these layers
there must be a gpio set function called doing the writeq on our SOC
to enable/disable the power gpio, right?
GPIO_THUNDERX implements this gpio set function for Cavium's SOC.
> Maybe "depends on GPIOLIB" instead?
>
> > + help
> > + This selects Cavium ThunderX SD/MMC Card Interface.
> > + If you have an Cavium ARM64 board with a Multimedia Card slot
> > + or builtin eMMC chip say Y or M here. If built as a module
> > + the module will be called thunderx_mmc.ko.
> > +
> > config MMC_DW
> > tristate "Synopsys DesignWare Memory Card Interface"
> > depends on HAS_DMA
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index c7f0ccf..0068610 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -44,6 +44,8 @@ obj-$(CONFIG_MMC_VIA_SDMMC) += via-sdmmc.o
> > obj-$(CONFIG_SDH_BFIN) += bfin_sdh.o
> > octeon-mmc-objs := cavium-mmc.o cavium-pltfm-octeon.o
> > obj-$(CONFIG_MMC_CAVIUM_OCTEON) += octeon-mmc.o
> > +thunderx-mmc-objs := cavium-mmc.o cavium-pci-thunderx.o
> > +obj-$(CONFIG_MMC_CAVIUM_THUNDERX) += thunderx-mmc.o
> > obj-$(CONFIG_MMC_DW) += dw_mmc.o
> > obj-$(CONFIG_MMC_DW_PLTFM) += dw_mmc-pltfm.o
> > obj-$(CONFIG_MMC_DW_EXYNOS) += dw_mmc-exynos.o
> > diff --git a/drivers/mmc/host/cavium-mmc.h b/drivers/mmc/host/cavium-mmc.h
> > index 4b22432..fb82aee 100644
> > --- a/drivers/mmc/host/cavium-mmc.h
> > +++ b/drivers/mmc/host/cavium-mmc.h
> > @@ -22,8 +22,12 @@
> > #define CAVIUM_MAX_MMC 4
> >
> > /* DMA register addresses */
> > -#define MIO_EMM_DMA_CFG(x) (0x00 + x->reg_off_dma)
> > -#define MIO_EMM_DMA_ADR(x) (0x08 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_CFG(x) (0x20 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_ADR(x) (0x28 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_INT(x) (0x30 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_INT_W1S(x) (0x38 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_INT_ENA_W1S(x) (0x40 + x->reg_off_dma)
> > +#define MIO_EMM_DMA_INT_ENA_W1C(x) (0x48 + x->reg_off_dma)
> >
> > /* register addresses */
> > #define MIO_EMM_CFG(x) (0x00 + x->reg_off)
> > @@ -39,6 +43,8 @@
> > #define MIO_EMM_SAMPLE(x) (0x90 + x->reg_off)
> > #define MIO_EMM_STS_MASK(x) (0x98 + x->reg_off)
> > #define MIO_EMM_RCA(x) (0xa0 + x->reg_off)
> > +#define MIO_EMM_INT_EN_SET(x) (0xb0 + x->reg_off)
> > +#define MIO_EMM_INT_EN_CLR(x) (0xb8 + x->reg_off)
> > #define MIO_EMM_BUF_IDX(x) (0xe0 + x->reg_off)
> > #define MIO_EMM_BUF_DAT(x) (0xe8 + x->reg_off)
> >
> > diff --git a/drivers/mmc/host/cavium-pci-thunderx.c b/drivers/mmc/host/cavium-pci-thunderx.c
> > new file mode 100644
> > index 0000000..6ad36b4
> > --- /dev/null
> > +++ b/drivers/mmc/host/cavium-pci-thunderx.c
> > @@ -0,0 +1,198 @@
> > +/*
> > + * Driver for MMC and SSD cards for Cavium ThunderX SOCs.
> > + *
> > + * This file is subject to the terms and conditions of the GNU General Public
> > + * License. See the file "COPYING" in the main directory of this archive
> > + * for more details.
> > + *
> > + * Copyright (C) 2016 Cavium Inc.
> > + */
> > +#include <linux/dma-mapping.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/mmc/mmc.h>
> > +#include <linux/mmc/slot-gpio.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pci.h>
> > +#include "cavium-mmc.h"
> > +
> > +struct platform_device *slot_pdev[2];
>
> Let's not be lazy. We don't want global arrays of platform devices,
> whatever the reason.
OK, I'll move this into the host struct.
> > +
> > +static void thunder_mmc_acquire_bus(struct cvm_mmc_host *host)
> > +{
> > + down(&host->mmc_serializer);
> > +}
> > +
> > +static void thunder_mmc_release_bus(struct cvm_mmc_host *host)
> > +{
> > + up(&host->mmc_serializer);
> > +}
> > +
> > +static void thunder_mmc_int_enable(struct cvm_mmc_host *host, u64 val)
> > +{
> > + writeq(val, host->base + MIO_EMM_INT(host));
> > + writeq(val, host->base + MIO_EMM_INT_EN_SET(host));
> > +}
> > +
> > +static int thunder_mmc_register_interrupts(struct cvm_mmc_host *host,
> > + struct pci_dev *pdev)
> > +{
> > + int nvec, ret, i;
> > +
> > + nvec = pci_alloc_irq_vectors(pdev, 1, 9, PCI_IRQ_MSIX);
> > + if (nvec < 0)
> > + return nvec;
> > +
> > + /* register interrupts */
> > + for (i = 0; i < nvec; i++) {
> > + ret = devm_request_irq(&pdev->dev, pci_irq_vector(pdev, i),
> > + cvm_mmc_interrupt,
> > + 0, cvm_mmc_irq_names[i], host);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
> > +
> > +static int thunder_mmc_probe(struct pci_dev *pdev,
> > + const struct pci_device_id *id)
> > +{
> > + struct device_node *node = pdev->dev.of_node;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *child_node;
> > + struct cvm_mmc_host *host;
> > + int ret, i = 0;
> > +
> > + host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
> > + if (!host)
> > + return -ENOMEM;
> > +
> > + pci_set_drvdata(pdev, host);
> > + ret = pcim_enable_device(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + ret = pci_request_regions(pdev, KBUILD_MODNAME);
> > + if (ret)
> > + return ret;
> > +
> > + host->base = pcim_iomap(pdev, 0, pci_resource_len(pdev, 0));
> > + if (!host->base)
> > + return -EINVAL;
> > +
> > + /* On ThunderX these are identical */
> > + host->dma_base = host->base;
> > +
> > + host->reg_off = 0x2000;
> > + host->reg_off_dma = 0x160;
> > +
> > + host->clk = devm_clk_get(dev, NULL);
> > + if (IS_ERR(host->clk))
> > + return PTR_ERR(host->clk);
> > +
> > + ret = clk_prepare_enable(host->clk);
> > + if (ret)
> > + return ret;
> > + host->sys_freq = clk_get_rate(host->clk);
> > +
> > + spin_lock_init(&host->irq_handler_lock);
> > + sema_init(&host->mmc_serializer, 1);
> > +
> > + host->dev = dev;
> > + host->acquire_bus = thunder_mmc_acquire_bus;
> > + host->release_bus = thunder_mmc_release_bus;
> > + host->int_enable = thunder_mmc_int_enable;
> > +
> > + host->big_dma_addr = true;
> > + host->need_irq_handler_lock = true;
> > + host->last_slot = -1;
> > +
> > + ret = dma_set_mask(dev, DMA_BIT_MASK(48));
> > + if (ret)
> > + goto error;
> > +
> > + /*
> > + * Clear out any pending interrupts that may be left over from
> > + * bootloader. Writing 1 to the bits clears them.
> > + */
> > + writeq(127, host->base + MIO_EMM_INT_EN(host));
> > + writeq(3, host->base + MIO_EMM_DMA_INT_ENA_W1C(host));
> > +
> > + ret = thunder_mmc_register_interrupts(host, pdev);
> > + if (ret)
> > + goto error;
> > +
> > + for_each_child_of_node(node, child_node) {
> > + /*
> > + * TODO: mmc_of_parse and devm* require one device per slot.
>
> I guess the TODO is about fixing this behavior in the mmc core, such
> we can use mmc_of_parse() in more flexible manner. You may want to
> remove the "TODO", but please keep the comment.
OK.
> > + * Create a dummy device per slot and set the node pointer to
> > + * the slot. The easiest way to get this is using
> > + * of_platform_device_create.
> > + */
> > + if (!slot_pdev[i])
> > + slot_pdev[i] = of_platform_device_create(child_node, NULL,
> > + &pdev->dev);
>
> Seems like we should verify that this is a slot node, by checking the
> compatible, before creating a platform device for it. No?
Not sure I understand you correctly. Before creating the platform device
I don't have a device for the slot. Looking at functions I could use to
check the compatible all of these need a device, which I'm just going to
create. Can you point me to a function I can use here?
> > + if (!slot_pdev[i])
> > + continue;
> > + ret = cvm_mmc_of_slot_probe(&slot_pdev[i]->dev, host);
> > + if (ret)
> > + goto error;
> > + i++;
> > + }
> > + dev_info(dev, "probed\n");
> > + return 0;
> > +
> > +error:
> > + clk_disable_unprepare(host->clk);
> > + return ret;
> > +}
> > +
> > +static void thunder_mmc_remove(struct pci_dev *pdev)
> > +{
> > + struct cvm_mmc_host *host = pci_get_drvdata(pdev);
> > + u64 dma_cfg;
> > + int i;
> > +
> > + for (i = 0; i < CAVIUM_MAX_MMC; i++)
> > + if (host->slot[i]) {
> > + cvm_mmc_of_slot_remove(host->slot[i]);
> > + platform_device_del(slot_pdev[i]);
> > + }
> > +
> > + dma_cfg = readq(host->dma_base + MIO_EMM_DMA_CFG(host));
> > + dma_cfg &= ~MIO_EMM_DMA_CFG_EN;
> > + writeq(dma_cfg, host->dma_base + MIO_EMM_DMA_CFG(host));
> > +
> > + clk_disable_unprepare(host->clk);
> > +}
> > +
> > +static const struct pci_device_id thunder_mmc_id_table[] = {
> > + { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, 0xa010) },
> > + { 0, } /* end of table */
> > +};
> > +
> > +static struct pci_driver thunder_mmc_driver = {
> > + .name = KBUILD_MODNAME,
> > + .id_table = thunder_mmc_id_table,
> > + .probe = thunder_mmc_probe,
> > + .remove = thunder_mmc_remove,
> > +};
> > +
> > +static int __init thunder_mmc_init_module(void)
> > +{
> > + return pci_register_driver(&thunder_mmc_driver);
> > +}
> > +
> > +static void __exit thunder_mmc_exit_module(void)
> > +{
> > + pci_unregister_driver(&thunder_mmc_driver);
> > +}
> > +
> > +module_init(thunder_mmc_init_module);
> > +module_exit(thunder_mmc_exit_module);
> > +
> > +MODULE_AUTHOR("Cavium Inc.");
> > +MODULE_DESCRIPTION("Cavium ThunderX eMMC Driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_DEVICE_TABLE(pci, thunder_mmc_id_table);
> > --
> > 2.9.0.rc0.21.g7777322
> >
>
> Kind regards
> Uffe
Powered by blists - more mailing lists