| Message ID | 20190613170636.6647-1-tbogendoerfer@suse.de (mailing list archive) |
|---|---|
| Headers | show |
| Series | Use MFD framework for SGI IOC3 drivers | expand |
On Thu, Jun 13, 2019 at 07:06:31PM +0200, Thomas Bogendoerfer wrote: > SGI IOC3 chip has integrated ethernet, keyboard and mouse interface. > It also supports connecting a SuperIO chip for serial and parallel > interfaces. IOC3 is used inside various SGI systemboards and add-on > cards with different equipped external interfaces. > > Support for ethernet and serial interfaces were implemented inside > the network driver. This patchset moves out the not network related > parts to a new MFD driver, which takes care of card detection, > setup of platform devices and interrupt distribution for the subdevices. > > Serial portion: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > arch/mips/include/asm/sn/ioc3.h | 345 +++---- > arch/mips/sgi-ip27/ip27-timer.c | 20 - > drivers/mfd/Kconfig | 13 + > drivers/mfd/Makefile | 1 + > drivers/mfd/ioc3.c | 683 +++++++++++++ Lee, can you give me an indication, if the MFD changes are ok now or if I need to improve it further. Thanks, Thomas.
On Tue, 25 Jun 2019, Thomas Bogendoerfer wrote: > On Thu, Jun 13, 2019 at 07:06:31PM +0200, Thomas Bogendoerfer wrote: > > SGI IOC3 chip has integrated ethernet, keyboard and mouse interface. > > It also supports connecting a SuperIO chip for serial and parallel > > interfaces. IOC3 is used inside various SGI systemboards and add-on > > cards with different equipped external interfaces. > > > > Support for ethernet and serial interfaces were implemented inside > > the network driver. This patchset moves out the not network related > > parts to a new MFD driver, which takes care of card detection, > > setup of platform devices and interrupt distribution for the subdevices. > > > > Serial portion: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > > --- > > arch/mips/include/asm/sn/ioc3.h | 345 +++---- > > arch/mips/sgi-ip27/ip27-timer.c | 20 - > > drivers/mfd/Kconfig | 13 + > > drivers/mfd/Makefile | 1 + > > drivers/mfd/ioc3.c | 683 +++++++++++++ > > Lee, > > can you give me an indication, if the MFD changes are ok now > or if I need to improve it further. I will do, when I get to them. My review list currently runs into the 50s.
On Thu, 13 Jun 2019, Thomas Bogendoerfer wrote: > SGI IOC3 chip has integrated ethernet, keyboard and mouse interface. > It also supports connecting a SuperIO chip for serial and parallel > interfaces. IOC3 is used inside various SGI systemboards and add-on > cards with different equipped external interfaces. > > Support for ethernet and serial interfaces were implemented inside > the network driver. This patchset moves out the not network related > parts to a new MFD driver, which takes care of card detection, > setup of platform devices and interrupt distribution for the subdevices. > > Serial portion: Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de> > --- > arch/mips/include/asm/sn/ioc3.h | 345 +++---- > arch/mips/sgi-ip27/ip27-timer.c | 20 - > drivers/mfd/Kconfig | 13 + > drivers/mfd/Makefile | 1 + > drivers/mfd/ioc3.c | 683 +++++++++++++ A vast improvement, but still a little work to do. > drivers/net/ethernet/sgi/Kconfig | 4 +- > drivers/net/ethernet/sgi/ioc3-eth.c | 1932 ++++++++++++++--------------------- > drivers/tty/serial/8250/8250_ioc3.c | 98 ++ > drivers/tty/serial/8250/Kconfig | 11 + > drivers/tty/serial/8250/Makefile | 1 + > 10 files changed, 1693 insertions(+), 1415 deletions(-) > create mode 100644 drivers/mfd/ioc3.c > create mode 100644 drivers/tty/serial/8250/8250_ioc3.c [...] > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > index a17d275bf1d4..5c9f1bd9bd0a 100644 > --- a/drivers/mfd/Kconfig > +++ b/drivers/mfd/Kconfig > @@ -1989,5 +1989,18 @@ config RAVE_SP_CORE > Select this to get support for the Supervisory Processor > device found on several devices in RAVE line of hardware. > > +config SGI_MFD_IOC3 > + tristate "SGI IOC3 core driver" > + depends on PCI && MIPS > + select MFD_CORE > + help > + This option enables basic support for the SGI IOC3-based > + controller cards. This option does not enable any specific > + functions on such a card, but provides necessary infrastructure > + for other drivers to utilize. > + > + If you have an SGI Origin, Octane, or a PCI IOC3 card, > + then say Y. Otherwise say N. > + > endmenu > endif > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > index 52b1a90ff515..bba0d9eb0b18 100644 > --- a/drivers/mfd/Makefile > +++ b/drivers/mfd/Makefile > @@ -249,3 +249,4 @@ obj-$(CONFIG_MFD_SC27XX_PMIC) += sprd-sc27xx-spi.o > obj-$(CONFIG_RAVE_SP_CORE) += rave-sp.o > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o > obj-$(CONFIG_MFD_STMFX) += stmfx.o > +obj-$(CONFIG_SGI_MFD_IOC3) += ioc3.o > diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c > new file mode 100644 > index 000000000000..0c0d1b3475d0 > --- /dev/null > +++ b/drivers/mfd/ioc3.c > @@ -0,0 +1,683 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * SGI IOC3 multifunction device driver > + * > + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@suse.de> > + * > + * Based on work by: > + * Stanislaw Skowronek <skylark@unaligned.org> > + * Joshua Kinard <kumba@gentoo.org> > + * Brent Casavant <bcasavan@sgi.com> - IOC4 master driver > + * Pat Gefre <pfg@sgi.com> - IOC3 serial port IRQ demuxer > + */ > + > +#include <linux/delay.h> > +#include <linux/errno.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/core.h> > +#include <linux/module.h> > +#include <linux/pci.h> > +#include <linux/platform_device.h> > +#include <linux/platform_data/sgi-w1.h> > +#include <linux/rtc/ds1685.h> > + > +#include <asm/pci/bridge.h> > +#include <asm/sn/ioc3.h> > + > +#define IOC3_IRQ_SERIAL_A 6 > +#define IOC3_IRQ_SERIAL_B 15 > +#define IOC3_IRQ_KBD 22 > +#define IOC3_IRQ_ETH_DOMAIN 23 > + > +static int ioc3_serial_id; > +static int ioc3_eth_id; > +static int ioc3_kbd_id; > + > +struct ioc3_priv_data { > + struct irq_domain *domain; > + struct ioc3 __iomem *regs; > + struct pci_dev *pdev; > + int domain_irq; > +}; > + > +static void ioc3_irq_ack(struct irq_data *d) > +{ > + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d); > + unsigned int hwirq = irqd_to_hwirq(d); > + > + writel(BIT(hwirq), &ipd->regs->sio_ir); > +} > + > +static void ioc3_irq_mask(struct irq_data *d) > +{ > + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d); > + unsigned int hwirq = irqd_to_hwirq(d); > + > + writel(BIT(hwirq), &ipd->regs->sio_iec); > +} > + > +static void ioc3_irq_unmask(struct irq_data *d) > +{ > + struct ioc3_priv_data *ipd = irq_data_get_irq_chip_data(d); > + unsigned int hwirq = irqd_to_hwirq(d); > + > + writel(BIT(hwirq), &ipd->regs->sio_ies); > +} > + > +static struct irq_chip ioc3_irq_chip = { > + .name = "IOC3", > + .irq_ack = ioc3_irq_ack, > + .irq_mask = ioc3_irq_mask, > + .irq_unmask = ioc3_irq_unmask, > +}; > + > +#define IOC3_LVL_MASK (BIT(IOC3_IRQ_SERIAL_A) | BIT(IOC3_IRQ_SERIAL_B)) > + > +static int ioc3_irq_domain_map(struct irq_domain *d, unsigned int irq, > + irq_hw_number_t hwirq) > +{ > + /* use level irqs for every interrupt contained in IOC3_LVL_MASK */ Nit: Could you use proper grammar (less the full stops) in the comments please? Start with an uppercase character and things like "irq" should be "IRQ", etc. > + if (BIT(hwirq) & IOC3_LVL_MASK) > + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_level_irq); > + else > + irq_set_chip_and_handler(irq, &ioc3_irq_chip, handle_edge_irq); > + > + irq_set_chip_data(irq, d->host_data); > + return 0; > +} > + > +static const struct irq_domain_ops ioc3_irq_domain_ops = { > + .map = ioc3_irq_domain_map, > +}; > + > +static void ioc3_irq_handler(struct irq_desc *desc) > +{ > + struct irq_domain *domain = irq_desc_get_handler_data(desc); > + struct ioc3_priv_data *ipd = domain->host_data; > + struct ioc3 __iomem *regs = ipd->regs; > + u32 pending, mask; > + unsigned int irq; > + > + pending = readl(®s->sio_ir); > + mask = readl(®s->sio_ies); > + pending &= mask; /* mask off not enabled but pending irqs */ > + > + if (mask & BIT(IOC3_IRQ_ETH_DOMAIN)) > + /* if eth irq is enabled we need to check in eth irq regs */ > + if (readl(®s->eth.eisr) & readl(®s->eth.eier)) > + pending |= IOC3_IRQ_ETH_DOMAIN; > + > + if (pending) { > + irq = irq_find_mapping(domain, __ffs(pending)); > + if (irq) > + generic_handle_irq(irq); > + } else { > + spurious_interrupt(); > + } > +} > + > +/* > + * System boards/BaseIOs use more interrupt pins of the bridge asic "ASIC" > + * to which the IOC3 is connected. Since the IOC3 MFD driver > + * knows wiring of these extra pins, we use the map_irq function > + * to get interrupts activated > + */ > +static int ioc3_map_irq(struct pci_dev *pdev, int pin) > +{ > + struct pci_host_bridge *hbrg = pci_find_host_bridge(pdev->bus); > + > + return hbrg->map_irq(pdev, pin, 0); > +} > + > +static int ioc3_irq_domain_setup(struct ioc3_priv_data *ipd, int irq) > +{ > + struct irq_domain *domain; > + struct fwnode_handle *fn; > + > + fn = irq_domain_alloc_named_fwnode("IOC3"); > + if (!fn) > + goto err; > + > + domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd); > + if (!domain) > + goto err; > + > + irq_domain_free_fwnode(fn); > + ipd->domain = domain; > + > + irq_set_chained_handler_and_data(irq, ioc3_irq_handler, domain); > + ipd->domain_irq = irq; > + return 0; > +err: > + dev_err(&ipd->pdev->dev, "irq domain setup failed\n"); > + return -ENOMEM; > +} > + > +static struct resource ioc3_uarta_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta), > + sizeof_field(struct ioc3, sregs.uarta)), > + DEFINE_RES_IRQ(IOC3_IRQ_SERIAL_A) > +}; > + > +static struct resource ioc3_uartb_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uartb), > + sizeof_field(struct ioc3, sregs.uartb)), > + DEFINE_RES_IRQ(IOC3_IRQ_SERIAL_B) > +}; > + > +static struct mfd_cell ioc3_serial_cells[] = { > + { > + .name = "ioc3-serial8250", > + .resources = ioc3_uarta_resources, > + .num_resources = ARRAY_SIZE(ioc3_uarta_resources), > + .id = 0, > + }, > + { > + .name = "ioc3-serial8250", > + .resources = ioc3_uartb_resources, > + .num_resources = ARRAY_SIZE(ioc3_uartb_resources), > + .id = 1, Any reason for not using PLATFORM_DEVID_AUTO. > + } > +}; > + > +static int ioc3_serial_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + /* set gpio pins for RS232/RS422 mode selection */ > + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL, > + &ipd->regs->gpcr_s); > + /* select RS232 mode for uart a */ > + writel(0, &ipd->regs->gppr[6]); > + /* select RS232 mode for uart b */ > + writel(0, &ipd->regs->gppr[7]); > + > + /* switch both ports to 16650 mode */ > + writel(readl(&ipd->regs->port_a.sscr) & ~SSCR_DMA_EN, > + &ipd->regs->port_a.sscr); > + writel(readl(&ipd->regs->port_b.sscr) & ~SSCR_DMA_EN, > + &ipd->regs->port_b.sscr); > + udelay(1000); /* wait until mode switch is done */ > + > + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_serial_id, Any reason for not using PLATFORM_DEVID_AUTO. > + ioc3_serial_cells, ARRAY_SIZE(ioc3_serial_cells), > + &ipd->pdev->resource[0], 0, ipd->domain); > + if (ret) { > + dev_err(&ipd->pdev->dev, "Failed to add 16550 subdevs\n"); > + return ret; > + } > + ioc3_serial_id += 2; When is this likely to be re-read? > + return 0; > +} > + > +static struct resource ioc3_kbd_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, serio), > + sizeof_field(struct ioc3, serio)), > + DEFINE_RES_IRQ(IOC3_IRQ_KBD) > +}; > + > +static struct mfd_cell ioc3_kbd_cells[] = { > + { > + .name = "ioc3-kbd", > + .resources = ioc3_kbd_resources, > + .num_resources = ARRAY_SIZE(ioc3_kbd_resources), > + } > +}; > + > +static int ioc3_kbd_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_kbd_id, ioc3_kbd_cells, > + ARRAY_SIZE(ioc3_kbd_cells), > + &ipd->pdev->resource[0], 0, ipd->domain); > + if (ret) { > + dev_err(&ipd->pdev->dev, "Failed to add 16550 subdevs\n"); > + return ret; > + } > + ioc3_kbd_id++; Nit: '\n' here > + return 0; > +} > + > +static struct resource ioc3_eth_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, eth), > + sizeof_field(struct ioc3, eth)), > + DEFINE_RES_MEM(offsetof(struct ioc3, ssram), > + sizeof_field(struct ioc3, ssram)), > + DEFINE_RES_IRQ(0) > +}; > + > +static struct resource ioc3_w1_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, mcr), > + sizeof_field(struct ioc3, mcr)), > +}; > +static struct sgi_w1_platform_data ioc3_w1_platform_data; > + > +static struct mfd_cell ioc3_eth_cells[] = { > + { > + .name = "ioc3-eth", > + .resources = ioc3_eth_resources, > + .num_resources = ARRAY_SIZE(ioc3_eth_resources), > + }, > + { > + .name = "sgi_w1", > + .resources = ioc3_w1_resources, > + .num_resources = ARRAY_SIZE(ioc3_w1_resources), > + .platform_data = &ioc3_w1_platform_data, > + .pdata_size = sizeof(ioc3_w1_platform_data), > + } > +}; > + > +static int ioc3_eth_setup(struct ioc3_priv_data *ipd, bool use_domain) > +{ > + int irq = ipd->pdev->irq; > + int ret; > + > + /* enable One-Wire bus */ > + writel(GPCR_MLAN_EN, &ipd->regs->gpcr_s); > + > + /* generate unique identifier */ > + snprintf(ioc3_w1_platform_data.dev_id, > + sizeof(ioc3_w1_platform_data.dev_id), "ioc3-%012llx", > + ipd->pdev->resource->start); > + > + if (use_domain) > + irq = irq_create_mapping(ipd->domain, IOC3_IRQ_ETH_DOMAIN); > + > + ret = mfd_add_devices(&ipd->pdev->dev, ioc3_eth_id, ioc3_eth_cells, > + ARRAY_SIZE(ioc3_eth_cells), > + &ipd->pdev->resource[0], irq, NULL); > + if (ret) { > + dev_err(&ipd->pdev->dev, "Failed to add ETH/W1 subdev\n"); > + return ret; > + } > + Nit: Remove this line and place it below: > + ioc3_eth_id++; Nit: '\n' here > + return 0; > +} > + > +#define M48T35_REG_SIZE 32768 /* size of m48t35 registers */ > + > +static struct resource ioc3_m48t35_resources[] = { > + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV0, M48T35_REG_SIZE) > +}; > + > +static struct mfd_cell ioc3_m48t35_cells[] = { > + { > + .name = "rtc-m48t35", > + .resources = ioc3_m48t35_resources, > + .num_resources = ARRAY_SIZE(ioc3_m48t35_resources), > + } > +}; > + > +static int ioc3_m48t35_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + ret = mfd_add_devices(&ipd->pdev->dev, PLATFORM_DEVID_AUTO, > + ioc3_m48t35_cells, ARRAY_SIZE(ioc3_m48t35_cells), > + &ipd->pdev->resource[0], 0, ipd->domain); > + if (ret) > + dev_err(&ipd->pdev->dev, "Failed to add M48T35 subdev\n"); > + > + return ret; > +} > + > +/* > + * On IP30 the RTC (a DS1687) is behind the IOC3 on the generic > + * ByteBus regions. We have to write the RTC address of interest to > + * IOC3_BYTEBUS_DEV1, then read the data from IOC3_BYTEBUS_DEV2. > + * rtc->regs already points to IOC3_BYTEBUS_DEV1. > + */ > +#define IP30_RTC_ADDR(rtc) (rtc->regs) > +#define IP30_RTC_DATA(rtc) ((rtc->regs) + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1) > + > +static u8 ip30_rtc_read(struct ds1685_priv *rtc, int reg) > +{ > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > + return readb(IP30_RTC_DATA(rtc)); > +} > + > +static void ip30_rtc_write(struct ds1685_priv *rtc, int reg, u8 value) > +{ > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > + writeb(value, IP30_RTC_DATA(rtc)); > +} Why is this not in the RTC driver? > +static struct ds1685_rtc_platform_data ip30_rtc_platform_data = { > + .bcd_mode = false, > + .no_irq = false, > + .uie_unsupported = true, > + .alloc_io_resources = true, > + .plat_read = ip30_rtc_read, > + .plat_write = ip30_rtc_write, Call-backs in a non-subsystem API is pretty ugly IMHO. Where are these called from? > +}; > + > +static struct resource ioc3_rtc_ds1685_resources[] = { > + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV1, > + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1 + 1), > + DEFINE_RES_IRQ(0) > +}; > + > +static struct mfd_cell ioc3_ds1685_cells[] = { > + { > + .name = "rtc-ds1685", > + .resources = ioc3_rtc_ds1685_resources, > + .num_resources = ARRAY_SIZE(ioc3_rtc_ds1685_resources), > + .platform_data = &ip30_rtc_platform_data, > + .pdata_size = sizeof(ip30_rtc_platform_data), > + .id = -1, Use the #define. Same goes for below. > + } > +}; > + > +static int ioc3_ds1685_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, irq; > + > + irq = ioc3_map_irq(ipd->pdev, 6); Nit: '\n' here > + ret = mfd_add_devices(&ipd->pdev->dev, 0, ioc3_ds1685_cells, > + ARRAY_SIZE(ioc3_ds1685_cells), > + &ipd->pdev->resource[0], irq, NULL); > + if (ret) > + dev_err(&ipd->pdev->dev, "Failed to add DS1685 subdev\n"); > + > + return ret; > +}; > + > + > +static struct resource ioc3_leds_resources[] = { > + DEFINE_RES_MEM(offsetof(struct ioc3, gppr[0]), > + sizeof_field(struct ioc3, gppr[0])), > + DEFINE_RES_MEM(offsetof(struct ioc3, gppr[1]), > + sizeof_field(struct ioc3, gppr[1])), > +}; > + > +static struct mfd_cell ioc3_led_cells[] = { > + { > + .name = "ip30-leds", > + .resources = ioc3_leds_resources, > + .num_resources = ARRAY_SIZE(ioc3_leds_resources), > + .id = -1, > + } > +}; > + > +static int ioc3_led_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + ret = mfd_add_devices(&ipd->pdev->dev, 0, ioc3_led_cells, > + ARRAY_SIZE(ioc3_led_cells), > + &ipd->pdev->resource[0], 0, ipd->domain); > + if (ret) > + dev_err(&ipd->pdev->dev, "Failed to add LED subdev\n"); Nit: '\n' here > + return ret; > +} > + > +static int ip27_baseio_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, io_irq; > + > + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2); > + ret = ioc3_irq_domain_setup(ipd, io_irq); > + if (ret) > + return ret; Nit: '\n' here > + ret = ioc3_eth_setup(ipd, false); > + if (ret) > + return ret; Nit: '\n' here > + ret = ioc3_serial_setup(ipd); > + if (ret) > + return ret; Nit: '\n' here Etc, etc, etc ... same for all of the instances below. > + return ioc3_m48t35_setup(ipd); > +} > + > +static int ip27_baseio6g_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, io_irq; > + > + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2); > + ret = ioc3_irq_domain_setup(ipd, io_irq); > + if (ret) > + return ret; > + ret = ioc3_eth_setup(ipd, false); > + if (ret) > + return ret; > + ret = ioc3_serial_setup(ipd); > + if (ret) > + return ret; > + ret = ioc3_m48t35_setup(ipd); > + if (ret) > + return ret; > + return ioc3_kbd_setup(ipd); > +} > + > +static int ip27_mio_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + ret = ioc3_irq_domain_setup(ipd, ipd->pdev->irq); > + if (ret) > + return ret; > + ret = ioc3_serial_setup(ipd); > + if (ret) > + return ret; > + return ioc3_kbd_setup(ipd); > +} > + > +static int ip29_sysboard_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, io_irq; > + > + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 1); > + ret = ioc3_irq_domain_setup(ipd, io_irq); > + if (ret) > + return ret; > + ret = ioc3_eth_setup(ipd, false); > + if (ret) > + return ret; > + ret = ioc3_serial_setup(ipd); > + if (ret) > + return ret; > + ret = ioc3_m48t35_setup(ipd); > + if (ret) > + return ret; > + return ioc3_kbd_setup(ipd); > +} > + > +static int ip30_sysboard_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, io_irq; > + > + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 2); > + ret = ioc3_irq_domain_setup(ipd, io_irq); > + if (ret) > + return ret; > + ret = ioc3_eth_setup(ipd, false); > + if (ret) > + return ret; > + ret = ioc3_serial_setup(ipd); > + if (ret) > + return ret; > + ret = ioc3_kbd_setup(ipd); > + if (ret) > + return ret; > + ret = ioc3_ds1685_setup(ipd); > + if (ret) > + return ret; > + return ioc3_led_setup(ipd); > +} > + > +static int ioc3_menet_setup(struct ioc3_priv_data *ipd) > +{ > + int ret, io_irq; > + > + io_irq = ioc3_map_irq(ipd->pdev, PCI_SLOT(ipd->pdev->devfn) + 4); > + ret = ioc3_irq_domain_setup(ipd, io_irq); > + if (ret) > + return ret; > + ret = ioc3_eth_setup(ipd, false); > + if (ret) > + return ret; > + return ioc3_serial_setup(ipd); > +} > + > +static int ioc3_menet4_setup(struct ioc3_priv_data *ipd) > +{ > + return ioc3_eth_setup(ipd, false); > +} > + > +static int ioc3_cad_duo_setup(struct ioc3_priv_data *ipd) > +{ > + int ret; > + > + ret = ioc3_irq_domain_setup(ipd, ipd->pdev->irq); > + if (ret) > + return ret; > + ret = ioc3_eth_setup(ipd, true); > + if (ret) > + return ret; > + return ioc3_kbd_setup(ipd); > +} > + > +#define IOC3_SID(_name, _sid, _setup) \ > + { \ > + .name = _name, \ > + .sid = (PCI_VENDOR_ID_SGI << 16) | IOC3_SUBSYS_ ## _sid, \ > + .setup = _setup, \ > + } > + > +static struct { > + const char *name; > + u32 sid; > + int (*setup)(struct ioc3_priv_data *ipd); > +} ioc3_infos[] = { IMHO it's neater if you separate the definition and static data part. > + IOC3_SID("IP27 BaseIO6G", IP27_BASEIO6G, &ip27_baseio6g_setup), > + IOC3_SID("IP27 MIO", IP27_MIO, &ip27_mio_setup), > + IOC3_SID("IP27 BaseIO", IP27_BASEIO, &ip27_baseio_setup), > + IOC3_SID("IP29 System Board", IP29_SYSBOARD, &ip29_sysboard_setup), > + IOC3_SID("IP30 System Board", IP30_SYSBOARD, &ip30_sysboard_setup), > + IOC3_SID("MENET", MENET, &ioc3_menet_setup), > + IOC3_SID("MENET4", MENET4, &ioc3_menet4_setup) > +}; > + > +static int ioc3_setup(struct ioc3_priv_data *ipd) > +{ > + u32 sid; > + int i; > + > + /* Clear IRQs */ > + writel(~0, &ipd->regs->sio_iec); > + writel(~0, &ipd->regs->sio_ir); > + writel(0, &ipd->regs->eth.eier); > + writel(~0, &ipd->regs->eth.eisr); > + > + /* read subsystem vendor id and subsystem id */ > + pci_read_config_dword(ipd->pdev, PCI_SUBSYSTEM_VENDOR_ID, &sid); > + > + for (i = 0; i < ARRAY_SIZE(ioc3_infos); i++) > + if (sid == ioc3_infos[i].sid) { > + pr_info("ioc3: %s\n", ioc3_infos[i].name); > + return ioc3_infos[i].setup(ipd); > + } > + > + /* treat everything not identified by PCI subid as CAD DUO */ > + pr_info("ioc3: CAD DUO\n"); > + return ioc3_cad_duo_setup(ipd); > +} > + > +static int ioc3_mfd_probe(struct pci_dev *pdev, > + const struct pci_device_id *pci_id) > +{ > + struct ioc3_priv_data *ipd; > + struct ioc3 __iomem *regs; > + int ret; > + > + ret = pci_enable_device(pdev); > + if (ret) > + return ret; > + > + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64); What does 64 mean here? Define it perhaps? > + pci_set_master(pdev); > + > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); > + if (ret) { > + dev_warn(&pdev->dev, "Warning: couldn_t set 64-bit DMA mask\n"); Remove the "Warning:" part please. "Failed to set 64-bit DMA mask - trying 32-bit" ? > + ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > + if (ret) { > + dev_err(&pdev->dev, "Can't set DMA mask, aborting\n"); > + return ret; > + } > + } > + > + /* Set up per-IOC3 data */ > + ipd = devm_kzalloc(&pdev->dev, sizeof(struct ioc3_priv_data), > + GFP_KERNEL); > + if (!ipd) { > + ret = -ENOMEM; > + goto out_disable_device; > + } > + ipd->pdev = pdev; > + > + /* > + * Map all IOC3 registers. These are shared between subdevices > + * so the main IOC3 module manages them. > + */ > + regs = devm_ioremap(&pdev->dev, pci_resource_start(pdev, 0), > + pci_resource_len(pdev, 0)); > + if (!regs) { > + pr_warn("ioc3: Unable to remap PCI BAR for %s.\n", > + pci_name(pdev)); Why are you using pr_warn() here? > + ret = -ENOMEM; > + goto out_disable_device; > + } > + ipd->regs = regs; > + > + /* Track PCI-device specific data */ > + pci_set_drvdata(pdev, ipd); > + > + ret = ioc3_setup(ipd); > + if (ret) > + goto out_disable_device; > + > + return 0; > + > +out_disable_device: > + pci_disable_device(pdev); > + return ret; > +} > + > +static void ioc3_mfd_remove(struct pci_dev *pdev) > +{ > + struct ioc3_priv_data *ipd; > + > + ipd = pci_get_drvdata(pdev); > + > + /* Clear and disable all IRQs */ > + writel(~0, &ipd->regs->sio_iec); > + writel(~0, &ipd->regs->sio_ir); > + > + /* Release resources */ > + if (ipd->domain) { > + irq_domain_remove(ipd->domain); > + free_irq(ipd->domain_irq, (void *)ipd); > + } > + pci_disable_device(pdev); > +} > + > +static struct pci_device_id ioc3_mfd_id_table[] = { > + { PCI_VENDOR_ID_SGI, PCI_DEVICE_ID_SGI_IOC3, PCI_ANY_ID, PCI_ANY_ID }, > + { 0, }, > +}; > +MODULE_DEVICE_TABLE(pci, ioc3_mfd_id_table); > + > +static struct pci_driver ioc3_mfd_driver = { > + .name = "IOC3", > + .id_table = ioc3_mfd_id_table, > + .probe = ioc3_mfd_probe, > + .remove = ioc3_mfd_remove, > +}; > + > +module_pci_driver(ioc3_mfd_driver); > + > +MODULE_AUTHOR("Thomas Bogendoerfer <tbogendoerfer@suse.de>"); > +MODULE_DESCRIPTION("SGI IOC3 MFD driver"); > +MODULE_LICENSE("GPL"); GPL v2 ?
On Thu, 25 Jul 2019 12:47:16 +0100 Lee Jones <lee.jones@linaro.org> wrote: > On Thu, 13 Jun 2019, Thomas Bogendoerfer wrote: > > +/* > > + * On IP30 the RTC (a DS1687) is behind the IOC3 on the generic > > + * ByteBus regions. We have to write the RTC address of interest to > > + * IOC3_BYTEBUS_DEV1, then read the data from IOC3_BYTEBUS_DEV2. > > + * rtc->regs already points to IOC3_BYTEBUS_DEV1. > > + */ > > +#define IP30_RTC_ADDR(rtc) (rtc->regs) > > +#define IP30_RTC_DATA(rtc) ((rtc->regs) + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1) > > + > > +static u8 ip30_rtc_read(struct ds1685_priv *rtc, int reg) > > +{ > > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > > + return readb(IP30_RTC_DATA(rtc)); > > +} > > + > > +static void ip30_rtc_write(struct ds1685_priv *rtc, int reg, u8 value) > > +{ > > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > > + writeb(value, IP30_RTC_DATA(rtc)); > > +} > > Why is this not in the RTC driver? because rtc1685 is used in different systems and accessing the chip differs between those systems. > > +static struct ds1685_rtc_platform_data ip30_rtc_platform_data = { > > + .bcd_mode = false, > > + .no_irq = false, > > + .uie_unsupported = true, > > + .alloc_io_resources = true, > > > + .plat_read = ip30_rtc_read, > > + .plat_write = ip30_rtc_write, > > Call-backs in a non-subsystem API is pretty ugly IMHO. I agree > Where are these called from? drivers/rtc/rtc-ds1685.c I could do the same as done for serial8250 and add an additional .c file in drivers/rtc which handles this for SGI-IP30. Alexandre would this work for you as well ? > > +#define IOC3_SID(_name, _sid, _setup) \ > > + { \ > > + .name = _name, \ > > + .sid = (PCI_VENDOR_ID_SGI << 16) | IOC3_SUBSYS_ ## _sid, \ > > + .setup = _setup, \ > > + } > > + > > +static struct { > > + const char *name; > > + u32 sid; > > + int (*setup)(struct ioc3_priv_data *ipd); > > +} ioc3_infos[] = { > > IMHO it's neater if you separate the definition and static data part. I don't quite understand what you mean here. Should I move the #define at the beginning of the file ? Why is it neater ? Thomas.
On 29/07/2019 20:45:57+0200, Thomas Bogendoerfer wrote: > On Thu, 25 Jul 2019 12:47:16 +0100 > Lee Jones <lee.jones@linaro.org> wrote: > > > On Thu, 13 Jun 2019, Thomas Bogendoerfer wrote: > > > +/* > > > + * On IP30 the RTC (a DS1687) is behind the IOC3 on the generic > > > + * ByteBus regions. We have to write the RTC address of interest to > > > + * IOC3_BYTEBUS_DEV1, then read the data from IOC3_BYTEBUS_DEV2. > > > + * rtc->regs already points to IOC3_BYTEBUS_DEV1. > > > + */ > > > +#define IP30_RTC_ADDR(rtc) (rtc->regs) > > > +#define IP30_RTC_DATA(rtc) ((rtc->regs) + IOC3_BYTEBUS_DEV2 - IOC3_BYTEBUS_DEV1) > > > + > > > +static u8 ip30_rtc_read(struct ds1685_priv *rtc, int reg) > > > +{ > > > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > > > + return readb(IP30_RTC_DATA(rtc)); > > > +} > > > + > > > +static void ip30_rtc_write(struct ds1685_priv *rtc, int reg, u8 value) > > > +{ > > > + writeb((reg & 0x7f), IP30_RTC_ADDR(rtc)); > > > + writeb(value, IP30_RTC_DATA(rtc)); > > > +} > > > > Why is this not in the RTC driver? > > because rtc1685 is used in different systems and accessing the chip > differs between those systems. > > > > +static struct ds1685_rtc_platform_data ip30_rtc_platform_data = { > > > + .bcd_mode = false, > > > + .no_irq = false, > > > + .uie_unsupported = true, > > > + .alloc_io_resources = true, > > > > > + .plat_read = ip30_rtc_read, > > > + .plat_write = ip30_rtc_write, > > > > Call-backs in a non-subsystem API is pretty ugly IMHO. > > I agree > > > Where are these called from? > > drivers/rtc/rtc-ds1685.c > > I could do the same as done for serial8250 and add an additional .c file > in drivers/rtc which handles this for SGI-IP30. Alexandre would this work > for you as well ? > As it is not particularly big, you could put that directly in rtc-ds1685.c.