Re: [PATCH linux dev-4.17 3/7] mmc: Aspeed: Add Aspeed sdhci core driver

[Ryan Chen <ryanchen.aspeed@gmail.com>]

On Thu, Jul 26, 2018 at 11:41:49AM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2018-07-17 at 15:26 +0800, Ryan Chen wrote:
> > In Aspeed SoC's sdhci have two slots and have a interrupt status and
> > general information in top for register. add sdhci core driver
> > defore sdhci driver probe
> > 
> > V0->V1
> > move irqchip/irq-aspeed-sdhci-ic.c to drivers/mmc/host/aspeed-sdhci-core.c
> > 
> > Signed-off-by: Ryan Chen <ryanchen.aspeed@gmail.com>
> > ---
> >  drivers/irqchip/Makefile              |   2 +-
> >  drivers/mmc/host/Makefile             |   1 +
> >  drivers/mmc/host/aspeed-sdhci-core.c  | 147 ++++++++++++++++++++++++++++++++++
> >  include/linux/mmc/sdhci-aspeed-data.h |  28 +++++++
> >  4 files changed, 177 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/mmc/host/aspeed-sdhci-core.c
> >  create mode 100644 include/linux/mmc/sdhci-aspeed-data.h
> > 
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index 5ed465a..d99cb1d 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -78,7 +78,7 @@ obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
> >  obj-$(CONFIG_MVEBU_PIC)			+= irq-mvebu-pic.o
> >  obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
> >  obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
> > -obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o
> > +obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o irq-aspeed-i2c-ic.o 
> >  obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
> >  obj-$(CONFIG_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> >  obj-$(CONFIG_IRQ_UNIPHIER_AIDET)	+= irq-uniphier-aidet.o
> > diff --git a/drivers/mmc/host/Makefile b/drivers/mmc/host/Makefile
> > index 6aead24..c3e5a1f 100644
> > --- a/drivers/mmc/host/Makefile
> > +++ b/drivers/mmc/host/Makefile
> > @@ -78,6 +78,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)	+= aspeed-sdhci-core.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/aspeed-sdhci-core.c b/drivers/mmc/host/aspeed-sdhci-core.c
> > new file mode 100644
> > index 0000000..c3d9d45
> > --- /dev/null
> > +++ b/drivers/mmc/host/aspeed-sdhci-core.c
> > @@ -0,0 +1,147 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * SDHCI IRQCHIP driver for the Aspeed SoC
> > + *
> > + * Copyright (C) ASPEED Technology Inc.
> > + * Ryan Chen <ryan_chen@aspeedtech.com>
> > + *
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/mmc/sdhci-aspeed-data.h>
> > +
> > +#define ASPEED_SDHCI_SLOT_NUM			2
> > 
> 
> Change the "irq" prefixes on the functions.. Dont' do a chained
> handler, that will be too much overhead for no benefit. Your SoC aren't
> very fast and you want to avoid that overhead.
> 
> Just do a normal interrupt handler, and have it call each port
> interrupts.
> 
> This isn't really an interrupt controller, it has no
> enable/disable/masking ability, I would just create the ports directly
> from the same driver and process the interrupts.
> 
> I don't think there's benefit in keeping the ports as separate drivers.
> 
> > +static void aspeed_sdhci_irq_handler(struct irq_desc *desc)
> > +{
> > +	struct aspeed_sdhci_irq *sdhci_irq = irq_desc_get_handler_data(desc);
> > +	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	unsigned long bit, status;
> > +	unsigned int slot_irq;
> > +
> > +	chained_irq_enter(chip, desc);
> > +	status = readl(sdhci_irq->regs + ASPEED_SDHCI_ISR);
> > +	for_each_set_bit(bit, &status, ASPEED_SDHCI_SLOT_NUM) {
> > +		slot_irq = irq_find_mapping(sdhci_irq->irq_domain, bit);
> > +		generic_handle_irq(slot_irq);
> > +	}
> > +	chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void noop(struct irq_data *data) { }
> > +
> > +static unsigned int noop_ret(struct irq_data *data)
> > +{
> > +	return 0;
> > +}
> > +
> > +struct irq_chip sdhci_irq_chip = {
> > +	.name		= "sdhci-ic",
> > +	.irq_startup	= noop_ret,
> > +	.irq_shutdown	= noop,
> > +	.irq_enable	= noop,
> > +	.irq_disable	= noop,
> > +	.irq_ack	= noop,
> > +	.irq_mask	= noop,
> > +	.irq_unmask	= noop,
> > +	.flags		= IRQCHIP_SKIP_SET_WAKE,
> > +};
> > +
> > +static int ast_sdhci_map_irq_domain(struct irq_domain *domain,
> > +				    unsigned int irq, irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_and_handler(irq, &sdhci_irq_chip, handle_simple_irq);
> > +	irq_set_chip_data(irq, domain->host_data);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops aspeed_sdhci_irq_domain_ops = {
> > +	.map = ast_sdhci_map_irq_domain,
> > +};
> > +
> > +static int irq_aspeed_sdhci_probe(struct platform_device *pdev)
> > +{
> > +	struct aspeed_sdhci_irq *sdhci_irq;
> > +	struct clk *sdclk;
> > +
> > +	sdhci_irq = kzalloc(sizeof(*sdhci_irq), GFP_KERNEL);
> > +	if (!sdhci_irq)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, sdhci_irq);
> > +	//node->data = sdhci_irq;
> > +	pdev->dev.of_node->data = sdhci_irq;
> > +
> > +	sdhci_irq->regs = of_iomap(pdev->dev.of_node, 0);
> > +	if (IS_ERR(sdhci_irq->regs))
> > +		return PTR_ERR(sdhci_irq->regs);
> > +
> > +	sdclk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(sdclk)) {
> > +		dev_err(&pdev->dev, "no sdclk clock defined\n");
> > +		return PTR_ERR(sdclk);
> > +	}
> > +
> > +	clk_prepare_enable(sdclk);
> > +
> > +	sdhci_irq->parent_irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> > +	if (sdhci_irq->parent_irq < 0)
> > +		return sdhci_irq->parent_irq;
> > +
> > +	sdhci_irq->irq_domain = irq_domain_add_linear(
> > +					pdev->dev.of_node, ASPEED_SDHCI_SLOT_NUM,
> > +					&aspeed_sdhci_irq_domain_ops, NULL);
> > +	if (!sdhci_irq->irq_domain)
> > +		return -ENOMEM;
> > +
> > +	sdhci_irq->irq_domain->name = "aspeed-sdhci-irq";
> > +
> > +	irq_set_chained_handler_and_data(sdhci_irq->parent_irq,
> > +					 aspeed_sdhci_irq_handler, sdhci_irq);
> > +
> > +	pr_info("sdhci irq controller registered, irq %d\n", sdhci_irq->parent_irq);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id irq_aspeed_sdhci_dt_ids[] = {
> > +	{ .compatible = "aspeed,aspeed-sdhci-irq", },
> > +	{},
> > +};
> 
> You should call this aspeed,ast2{4,5}00-sdhci-core. I would change your
> device-tree representation as follow (add back all the reg &
> properties):
> 
> 	sdhci-core {
> 		reg = <common regs>
> 		ranges = either empty prop or the 2 ranges goign to the children
> 		compatible = <aspeed,ast2400-sdhci-core>;
> 
> 		sdhci-slot@xxx {
> 			compatible = <aspeed,ast2400-sdhci-slot>;
> 		};
> 
> 		sdhci-slot@yyy {
> 			compatible = <aspeed,ast2400-sdhci-slot>;
> 		};
> 	}
> 

If i keep the interrupt controller for dispatch slot. 
Is the following is compromise way for dts? 
Each slot need interrupt-parent.

&sdhci {

	sdhci_core: interrupt-controller@0 {
		#interrupt-cells = <1>;
		compatible = "aspeed,ast2500-sdhci-core";
		reg = <0x0 0x100>;
		interrupts = <26>;
		interrupt-controller;
		clocks = <&syscon ASPEED_CLK_GATE_SDCLKCLK>;
	};

	sdhci_slot0: sdhci_slot@100 {
		compatible = "aspeed,ast2500-sdhci-slot";
		reg = <0x100 0x100>;
		interrupts = <0>;
		interrupt-parent = <&sdhci_core>;
		sdhci,auto-cmd12;
		clocks = <&syscon ASPEED_CLK_SDIO>;
		status = "disabled";
	};

	sdhci_slot1: sdhci_slot@200 {
		compatible = "aspeed,ast2500-sdhci-slot";
		reg = <0x200 0x100>;
		interrupts = <1>;
		interrupt-parent = <&sdhci_core>;
		sdhci,auto-cmd12;
		clocks = <&syscon ASPEED_CLK_SDIO>;
		status = "disabled";
	};

};



> > +MODULE_DEVICE_TABLE(of, irq_aspeed_sdhci_dt_ids);
> > +
> > +static struct platform_driver irq_aspeed_sdhci_device_driver = {
> > +	.probe		= irq_aspeed_sdhci_probe,
> > +	.driver		= {
> > +		.name   = KBUILD_MODNAME,
> > +		.of_match_table	= irq_aspeed_sdhci_dt_ids,
> > +	}
> > +};
> > +
> > +static int __init irq_aspeed_sdhci_init(void)
> > +{
> > +	return platform_driver_register(&irq_aspeed_sdhci_device_driver);
> > +}
> > +core_initcall(irq_aspeed_sdhci_init);
> 
> This should be a normal device init call, thus module_platform_driver
> is all you need here.
> 
> > +
> > +MODULE_AUTHOR("Ryan Chen");
> > +MODULE_DESCRIPTION("ASPEED SOC SDHCI IRQ Driver");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/mmc/sdhci-aspeed-data.h b/include/linux/mmc/sdhci-aspeed-data.h
> > new file mode 100644
> > index 0000000..fba2bf2
> > --- /dev/null
> > +++ b/include/linux/mmc/sdhci-aspeed-data.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef LINUX_MMC_SDHCI_ASPEED_DATA_H
> > +#define LINUX_MMC_SDHCI_ASPEED_DATA_H
> > +
> > +#include <linux/io.h>
> > +
> > +#define ASPEED_SDHCI_INFO			0x00
> > +#define  ASPEED_SDHCI_S1MMC8			BIT(25)
> > +#define  ASPEED_SDHCI_S0MMC8			BIT(24)
> > +#define ASPEED_SDHCI_BLOCK			0x04
> > +#define ASPEED_SDHCI_CTRL			0xF0
> > +#define ASPEED_SDHCI_ISR			0xFC
> > +
> > +struct aspeed_sdhci_irq {
> > +	void __iomem *regs;
> > +	int parent_irq;
> > +	struct irq_domain *irq_domain;
> > +};
> > +
> > +static inline void aspeed_sdhci_set_8bit_mode(struct aspeed_sdhci_irq *sdhci_irq, int mode)
> > +{
> > +	if (mode)
> > +		writel(ASPEED_SDHCI_S0MMC8 | readl(sdhci_irq->regs), sdhci_irq->regs);
> > +	else
> > +		writel(~ASPEED_SDHCI_S0MMC8 & readl(sdhci_irq->regs), sdhci_irq->regs);
> > +}
> > +
> > +#endif
>