| Message ID | 1531812378-14316-4-git-send-email-ryanchen.aspeed@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | mmc/host: Add Aspeed SDIO driver | expand |
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>; }; } > +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
On Thu, 2018-07-26 at 11:41 +1000, Benjamin Herrenschmidt wrote: > 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. Look at how shdci-pci does it, sdhci-acpi as well, they have multiple slots. You can just register multiple slots from a single driver I think. You can still use device-tree sub-nodes and iterate them from the core driver to create the slots if you want, in case you may add slots in the future, but it's not even that a big deal. Ben.
On Thu, Jul 26, 2018 at 11:47:40AM +1000, Benjamin Herrenschmidt wrote: > On Thu, 2018-07-26 at 11:41 +1000, Benjamin Herrenschmidt wrote: > > 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. > > Look at how shdci-pci does it, sdhci-acpi as well, they have multiple > slots. You can just register multiple slots from a single driver > I think. You can still use device-tree sub-nodes and iterate them > from the core driver to create the slots if you want, in case you may > add slots in the future, but it's not even that a big deal. > > Ben. > > Thanks, after look into sdhci-pci, it should only request one pci_irq, But, i am wondering where is the slot dispatch. could you help me point out it?
On Thu, 2018-07-26 at 11:07 +0800, Ryan Chen wrote: > On Thu, Jul 26, 2018 at 11:47:40AM +1000, Benjamin Herrenschmidt wrote: > > On Thu, 2018-07-26 at 11:41 +1000, Benjamin Herrenschmidt wrote: > > > 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. > > > > Look at how shdci-pci does it, sdhci-acpi as well, they have multiple > > slots. You can just register multiple slots from a single driver > > I think. You can still use device-tree sub-nodes and iterate them > > from the core driver to create the slots if you want, in case you may > > add slots in the future, but it's not even that a big deal. > > > > Ben. > > > > > > Thanks, after look into sdhci-pci, it should only request one pci_irq, > But, i am wondering where is the slot dispatch. could you help me point out it? So the PCI one just uses the same IRQ number for all the slots in sdhci_pci_probe_slot(), so the IRQ ends up shared. So on every irq it will jsut check all slots. Not sure if that's a problem. Due to how sdhci is organized, if you really want to make sure it only accesses the one slot, then you indeed do have to use a cascaded interrupt controller as you are doing, but I'm not sure it's really worthwhile. Cheers, Ben.
On Thu, Jul 26, 2018 at 01:14:58PM +1000, Benjamin Herrenschmidt wrote: > On Thu, 2018-07-26 at 11:07 +0800, Ryan Chen wrote: > > On Thu, Jul 26, 2018 at 11:47:40AM +1000, Benjamin Herrenschmidt wrote: > > > On Thu, 2018-07-26 at 11:41 +1000, Benjamin Herrenschmidt wrote: > > > > 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. > > > > > > Look at how shdci-pci does it, sdhci-acpi as well, they have multiple > > > slots. You can just register multiple slots from a single driver > > > I think. You can still use device-tree sub-nodes and iterate them > > > from the core driver to create the slots if you want, in case you may > > > add slots in the future, but it's not even that a big deal. > > > > > > Ben. > > > > > > > > > > Thanks, after look into sdhci-pci, it should only request one pci_irq, > > But, i am wondering where is the slot dispatch. could you help me point out it? > > So the PCI one just uses the same IRQ number for all the slots in > sdhci_pci_probe_slot(), so the IRQ ends up shared. > > So on every irq it will jsut check all slots. > > Not sure if that's a problem. Due to how sdhci is organized, if you > really want to make sure it only accesses the one slot, then you indeed > do have to use a cascaded interrupt controller as you are doing, but > I'm not sure it's really worthwhile. > > Cheers, > Ben. > > Yes, there is two way, to handle irq. One is use share irq, so my sdhci driver have following outcome when share irq, cat /proc/interrupt ---------- 34: 82 AVIC 26 Edge mmc0, mmc1 --------- when use cascaded interrupt. ......... 292: 35 sdhci-ic 0 Edge mmc0 293: 42 sdhci-ic 1 Edge mmc1 --------- use separate slot (actually it is) interrupt, it have advantage for each slot debug. if use share. the debug will not easy.
On Thu, 2018-07-26 at 11:26 +0800, Ryan Chen wrote: > > > > > > Yes, there is two way, to handle irq. > One is use share irq, so my sdhci driver have following outcome when share > irq, > cat /proc/interrupt > ---------- > 34: 82 AVIC 26 Edge mmc0, mmc1 > --------- > when use cascaded interrupt. > ......... > 292: 35 sdhci-ic 0 Edge mmc0 > 293: 42 sdhci-ic 1 Edge mmc1 > --------- > use separate slot (actually it is) interrupt, it have advantage for each slot > debug. if use share. the debug will not easy. I see ... well, you can then continue doing a cascaded controller if you wish then. I sitll don't think you need to have separate drivers however, but that's up to you. You definitely don't want to use core initcalls. Also having a single driver might make it easier to implement the card detect stuff which is done by the "common" part, isn't it ? Cheers, Ben.
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 >
On Thu, 2018-07-26 at 17:10 +0800, Ryan Chen wrote: > On > > 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"; > }; I think the "core" is more than just the interrupt controller. The slots should be the children of it. You don't necessarily need to treat the slots as being matched by a separate driver, it complicates things imho, but up to you. Ben. > }; > > > > > > +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
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 + +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", }, + {}, +}; +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); + +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
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