On Tue, 09 Apr 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.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> ---
> 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 | 802 ++++++++++++++
> drivers/net/ethernet/sgi/Kconfig | 4 +-
> drivers/net/ethernet/sgi/ioc3-eth.c | 1867 ++++++++++++---------------------
> drivers/tty/serial/8250/8250_ioc3.c | 98 ++
> drivers/tty/serial/8250/Kconfig | 11 +
> drivers/tty/serial/8250/Makefile | 1 +
> include/linux/platform_data/ioc3eth.h | 15 +
> 11 files changed, 1762 insertions(+), 1415 deletions(-)
> create mode 100644 drivers/mfd/ioc3.c
> create mode 100644 drivers/tty/serial/8250/8250_ioc3.c
> create mode 100644 include/linux/platform_data/ioc3eth.h
[...]
> +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 b4569ed7f3f3..07255e499129 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -246,4 +246,5 @@ obj-$(CONFIG_MFD_MXS_LRADC) += mxs-lradc.o
> 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_SGI_MFD_IOC3) += ioc3.o
>
> diff --git a/drivers/mfd/ioc3.c b/drivers/mfd/ioc3.c
> new file mode 100644
> index 000000000000..ad715805b16e
> --- /dev/null
> +++ b/drivers/mfd/ioc3.c
> @@ -0,0 +1,802 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SGI IOC3 multifunction device driver
> + *
> + * Copyright (C) 2018, 2019 Thomas Bogendoerfer <tbogendoerfer@xxxxxxx>
> + *
> + * Based on work by:
> + * Stanislaw Skowronek <skylark@xxxxxxxxxxxxx>
> + * Joshua Kinard <kumba@xxxxxxxxxx>
> + * Brent Casavant <bcasavan@xxxxxxx> - IOC4 master driver
> + * Pat Gefre <pfg@xxxxxxx> - IOC3 serial port IRQ demuxer
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/ioc3eth.h>
These should be in alphabetical order.
> +#include <asm/sn/ioc3.h>
> +#include <asm/pci/bridge.h>
> +
> +#define IOC3_ETH BIT(0)
> +#define IOC3_SER BIT(1)
> +#define IOC3_PAR BIT(2)
> +#define IOC3_KBD BIT(3)
> +#define IOC3_M48T35 BIT(4)
> +
> +static int ioc3_serial_id = 1;
> +static int ioc3_eth_id = 1;
> +static int ioc3_kbd_id = 1;
> +static struct mfd_cell ioc3_mfd_cells[5];
> +
> +struct ioc3_board_info {
> + const char *name;
> + int irq_offset;
> + int funcs;
> +};
> +
> +struct ioc3_priv_data {
> + struct ioc3_board_info *info;
> + struct irq_domain *domain;
> + struct ioc3 __iomem *regs;
> + struct pci_dev *pdev;
> + char nic_part[32];
> + char nic_mac[6];
> + int irq_io;
> +};
> +
> +#define MCR_PACK(pulse, sample) (((pulse) << 10) | ((sample) << 2))
> +
> +static int ioc3_nic_wait(u32 __iomem *mcr)
> +{
> + u32 mcr_val;
> +
> + do {
> + mcr_val = readl(mcr);
> + } while (!(mcr_val & 2));
Why 2? Is bit 2 always set on a successful read?
Either way, you should provide a comment to improve readability.
> + return (mcr_val & 1);
Reads just a bit at a time? Again, a comment please.
> +}
> +
> +static int ioc3_nic_reset(u32 __iomem *mcr)
> +{
> + int presence;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(520, 65), mcr);
What do these magic values mean? Best to define them.
> + presence = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(500);
What are you waiting for? Why 500us?
> + return presence;
> +}
> +
> +static int ioc3_nic_read_bit(u32 __iomem *mcr)
> +{
> + int result;
> + unsigned long flags;
> +
> + local_irq_save(flags);
> + writel(MCR_PACK(6, 13), mcr);
Why 6 and 13?
Define all magic values please.
> + result = ioc3_nic_wait(mcr);
> + local_irq_restore(flags);
> +
> + udelay(100);
Why 100?
> + return result;
> +}
> +
> +static u32 ioc3_nic_read_byte(u32 __iomem *mcr)
> +{
> + u32 result = 0;
> + int i;
> +
> + for (i = 0; i < 8; i++)
> + result = ((result >> 1) | (ioc3_nic_read_bit(mcr) << 7));
> +
> + return result;
> +}
> +
> +static void ioc3_nic_write_bit(u32 __iomem *mcr, int bit)
> +{
> + if (bit)
> + writel(MCR_PACK(6, 110), mcr);
> + else
> + writel(MCR_PACK(80, 30), mcr);
> +
> + ioc3_nic_wait(mcr);
> +}
> +
> +static void ioc3_nic_write_byte(u32 __iomem *mcr, int byte)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + ioc3_nic_write_bit(mcr, byte & 1);
> + byte >>= 1;
> + }
> +}
> +
> +static u64 ioc3_nic_find(u32 __iomem *mcr, int *last, u64 addr)
> +{
> + int a, b, index, disc;
> +
> + ioc3_nic_reset(mcr);
> +
> + /* Search ROM. */
> + ioc3_nic_write_byte(mcr, 0xf0);
What does 0xf0 mean?
Please define it and all magic numbers that follow.
> + /* Algorithm from ``Book of iButton Standards''. */
That's great, but what is it actually doing. Please provide suitable
comments such that the reader doesn't have to painstakingly work it
all out.
> + for (index = 0, disc = 0; index < 64; index++) {
> + a = ioc3_nic_read_bit(mcr);
> + b = ioc3_nic_read_bit(mcr);
> +
> + if (unlikely(a && b)) {
> + pr_warn("ioc3: NIC search failed.\n");
> + *last = 0;
> + return 0;
> + }
> +
> + if (!a && !b) {
> + if (index == *last)
> + addr |= 1UL << index;
> + else if (index > *last) {
> + addr &= ~(1UL << index);
> + disc = index;
> + } else if ((addr & (1UL << index)) == 0)
> + disc = index;
> + ioc3_nic_write_bit(mcr, (addr >> index) & 1);
> + continue;
> + } else {
> + if (a)
> + addr |= (1UL << index);
> + else
> + addr &= ~(1UL << index);
> + ioc3_nic_write_bit(mcr, a);
> + continue;
> + }
> + }
> + *last = disc;
> + return addr;
> +}
> +
> +static void ioc3_nic_addr(u32 __iomem *mcr, u64 addr)
> +{
> + int index;
> +
> + ioc3_nic_reset(mcr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> +
> + for (index = 0; index < 64; index++) {
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_read_bit(mcr);
> + ioc3_nic_write_bit(mcr, ((addr >> index) & 1));
> + }
What is this function doing? Setting the NIC address?
Why are the 2 sequential reads required before each bit write?
> +}
> +
> +static void crc16_byte(u32 *crc, u8 db)
> +{
> + int i;
> +
> + for (i = 0; i < 8; i++) {
> + *crc <<= 1;
> + if ((db ^ (*crc >> 16)) & 1)
> + *crc ^= 0x8005;
> + db >>= 1;
> + }
> + *crc &= 0xffff;
> +}
> +
> +static u32 crc16_area(u8 *dbs, int size, u32 crc)
> +{
> + while (size--)
> + crc16_byte(&crc, *(dbs++));
> +
> + return crc;
> +}
> +
> +static void crc8_byte(u32 *crc, u8 db)
> +{
> + int i, f;
> +
> + for (i = 0; i < 8; i++) {
> + f = ((*crc ^ db) & 1);
> + *crc >>= 1;
> + db >>= 1;
> + if (f)
> + *crc ^= 0x8c;
> + }
> + *crc &= 0xff;
> +}
> +
> +static u32 crc8_addr(u64 addr)
> +{
> + u32 crc = 0;
> + int i;
> +
> + for (i = 0; i < 64; i += 8)
> + crc8_byte(&crc, addr >> i);
> + return crc;
> +}
Not looked into these in any detail, but are you not able to use the
CRC functions already provided by the kernel?
> +static void ioc3_read_redir_page(u32 __iomem *mcr, u64 addr, int page,
What is a redir page?
> + u8 *redir, u8 *data)
> +{
> + int loops = 16, i;
> +
> + while (redir[page] != 0xff) {
> + page = (redir[page] ^ 0xff);
> + loops--;
> + if (unlikely(loops < 0)) {
> + pr_err("ioc3: NIC circular redirection\n");
> + return;
> + }
> + }
> +
> + loops = 3;
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, (page << 5) & 0xe0);
> + ioc3_nic_write_byte(mcr, (page >> 3) & 0x1f);
> +
> + for (i = 0; i < 0x20; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 0x20, 0) == 0x800d)
> + return;
> +
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in data page\n");
> + for (i = 0; i < 0x20; i++)
> + data[i] = 0x00;
Comments required throughout.
> +}
> +
> +static void ioc3_read_redir_map(u32 __iomem *mcr, u64 addr, u8 *redir)
> +{
> + int i, j, crc_ok, loops = 3;
> + u32 crc;
> +
> + while (loops > 0) {
> + crc_ok = 1;
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xaa);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x01);
> +
> + for (i = 0; i < 64; i += 8) {
> + for (j = 0; j < 8; j++)
> + redir[i + j] = ioc3_nic_read_byte(mcr);
> +
> + crc = crc16_area(redir + i, 8, i == 0 ? 0x8707 : 0);
> +
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> + crc16_byte(&crc, ioc3_nic_read_byte(mcr));
> +
> + if (crc != 0x800d)
> + crc_ok = 0;
> + }
> + if (crc_ok)
> + return;
> + loops--;
> + }
> + pr_err("ioc3: CRC error in redirection page\n");
> + for (i = 0; i < 64; i++)
> + redir[i] = 0xff;
As above.
> +}
> +
> +static void ioc3_read_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr,
> + u64 addr)
> +{
> + u8 redir[64];
> + u8 data[64], part[32];
> + int i, j;
> +
> + /* Read redirections */
> + ioc3_read_redir_map(mcr, addr, redir);
> +
> + /* Read data pages */
> + ioc3_read_redir_page(mcr, addr, 0, redir, data);
> + ioc3_read_redir_page(mcr, addr, 1, redir, (data + 32));
> +
> + /* Assemble the part # */
> + j = 0;
> + for (i = 0; i < 19; i++)
> + if (data[i + 11] != ' ')
> + part[j++] = data[i + 11];
> +
> + for (i = 0; i < 6; i++)
> + if (data[i + 32] != ' ')
> + part[j++] = data[i + 32];
> +
> + part[j] = 0;
> +
> + /* Skip Octane (IP30) power supplies */
> + if (!(strncmp(part, "060-0035-", 9)) ||
> + !(strncmp(part, "060-0038-", 9)) ||
> + !(strncmp(part, "060-0028-", 9)))
> + return;
> +
> + strlcpy(ipd->nic_part, part, sizeof(ipd->nic_part));
> +}
> +
> +static void ioc3_read_mac(struct ioc3_priv_data *ipd, u64 addr)
> +{
> + int i, loops = 3;
> + u8 data[13];
> + u32 __iomem *mcr = &ipd->regs->mcr;
> +
> + while (loops > 0) {
> + ioc3_nic_addr(mcr, addr);
> + ioc3_nic_write_byte(mcr, 0xf0);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_write_byte(mcr, 0x00);
> + ioc3_nic_read_byte(mcr);
> +
> + for (i = 0; i < 13; i++)
> + data[i] = ioc3_nic_read_byte(mcr);
> +
> + if (crc16_area(data, 13, 0) == 0x800d) {
> + for (i = 10; i > 4; i--)
> + ipd->nic_mac[10 - i] = data[i];
> + return;
> + }
> + loops--;
> + }
> +
> + pr_err("ioc3: CRC error in MAC address\n");
> + for (i = 0; i < 6; i++)
> + ipd->nic_mac[i] = 0x00;
> +}
> +
> +static void ioc3_probe_nic(struct ioc3_priv_data *ipd, u32 __iomem *mcr)
> +{
> + int save = 0, loops = 3;
> + u64 first, addr;
> +
> + while (loops > 0) {
> + ipd->nic_part[0] = 0;
> + first = ioc3_nic_find(mcr, &save, 0);
> + addr = first;
> +
> + if (unlikely(!first))
> + return;
> +
> + while (1) {
> + if (crc8_addr(addr))
> + break;
> +
> + switch (addr & 0xff) {
> + case 0x0b:
> + ioc3_read_nic(ipd, mcr, addr);
> + break;
> + case 0x09:
> + case 0x89:
> + case 0x91:
> + ioc3_read_mac(ipd, addr);
> + break;
> + }
> +
> + addr = ioc3_nic_find(mcr, &save, addr);
> + if (addr == first)
> + return;
> + }
> + loops--;
> + }
> + pr_err("ioc3: CRC error in NIC address\n");
> +}
This all looks like networking code. If this is the case, it should
be moved to drivers/networking or similar.
> +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(0) | BIT(2) | BIT(6) | BIT(9) | BIT(11) | BIT(15))
You need to define what each of these bits are. BIT(2) doesn't tell
the reader anything, meaning the code is unreadable.
> +static int ioc3_irq_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + if (BIT(hwirq) & IOC3_LVL_MASK)
Comment needed.
> + 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;
> +}
> +
> +
Drop the superfluous '\n'.
> +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;
> + unsigned int irq = 0;
Why does these need to be pre-assigned?
> + u32 pending;
> +
> + pending = readl(®s->sio_ir);
> + pending &= readl(®s->sio_ies);
Comment required.
> + if (pending)
> + irq = irq_find_mapping(domain, __ffs(pending));
> + else if (!ipd->info->irq_offset &&
> + (readl(®s->eth.eisr) & readl(®s->eth.eier)))
Comment required.
> + irq = irq_find_mapping(domain, 23);
> +
> + if (irq)
> + generic_handle_irq(irq);
> + else
> + spurious_interrupt();
> +}
> +
> +static struct resource ioc3_uarta_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
You are the first user of offsetof() in MFD. Could you tell me why
it's required please?
> + sizeof_field(struct ioc3, sregs.uarta)),
> + DEFINE_RES_IRQ(6)
Please define all magic numbers.
> +};
> +
> +static struct resource ioc3_uartb_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uartb),
> + sizeof_field(struct ioc3, sregs.uartb)),
> + DEFINE_RES_IRQ(15)
> +};
> +
> +static struct resource ioc3_kbd_resources[] = {
> + DEFINE_RES_MEM(offsetof(struct ioc3, serio),
> + sizeof_field(struct ioc3, serio)),
> + DEFINE_RES_IRQ(22)
> +};
> +
> +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 ioc3eth_platform_data ioc3_eth_platform_data;
I doubt you'll need this in here. The MAC address info will need to
be moved out to a networking driver of some sort.
> +#ifdef CONFIG_SGI_IP27
#ifery in C files is highly discouraged. Please remove them.
> +static struct resource ioc3_rtc_resources[] = {
> + DEFINE_RES_MEM(IOC3_BYTEBUS_DEV0, 32768)
Define all magic numbers please.
> +};
> +
> +static struct ioc3_board_info ip27_baseio_info = {
> + .name = "IP27 BaseIO",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_baseio6g_info = {
> + .name = "IP27 BaseIO6G",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 2
> +};
> +
> +static struct ioc3_board_info ip27_mio_info = {
> + .name = "MIO",
> + .funcs = IOC3_SER | IOC3_PAR | IOC3_KBD,
> + .irq_offset = 0
> +};
> +
> +static struct ioc3_board_info ip29_baseio_info = {
> + .name = "IP29 System Board",
> + .funcs = IOC3_ETH | IOC3_SER | IOC3_PAR | IOC3_KBD | IOC3_M48T35,
> + .irq_offset = 1
> +};
> +
> +#endif /* CONFIG_SGI_IP27 */
> +
> +static struct ioc3_board_info ioc3_menet_info = {
> + .name = "MENET",
> + .funcs = IOC3_ETH | IOC3_SER,
> + .irq_offset = 4
> +};
> +
> +static struct ioc3_board_info ioc3_cad_duo_info = {
> + .name = "CAD DUO",
> + .funcs = IOC3_ETH | IOC3_KBD,
> + .irq_offset = 0
> +};
Please drop all of these and statically create the MFD cells like
almost all other MFD drivers do.
> +#define IOC3_BOARD(_partno, _info) { .info = _info, .match = _partno }
> +
> +static struct {
> + struct ioc3_board_info *info;
> + const char *match;
> +} ioc3_boards[] = {
> +#ifdef CONFIG_SGI_IP27
> + IOC3_BOARD("030-0734-", &ip27_baseio6g_info),
> + IOC3_BOARD("030-1023-", &ip27_baseio_info),
> + IOC3_BOARD("030-1124-", &ip27_baseio_info),
> + IOC3_BOARD("030-1025-", &ip29_baseio_info),
> + IOC3_BOARD("030-1244-", &ip29_baseio_info),
> + IOC3_BOARD("030-1389-", &ip29_baseio_info),
> + IOC3_BOARD("030-0880-", &ip27_mio_info),
> +#endif
> + IOC3_BOARD("030-0873-", &ioc3_menet_info),
> + IOC3_BOARD("030-1155-", &ioc3_cad_duo_info),
> +};
> +
> +static int ioc3_identify(struct ioc3_priv_data *idp)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(ioc3_boards); i++)
> + if (!strncmp(idp->nic_part, ioc3_boards[i].match,
> + strlen(ioc3_boards[i].match))) {
> + idp->info = ioc3_boards[i].info;
> + return 0;
> + }
> +
> + return -1;
Please return a proper Linux error code.
> +}
> +
> +static void ioc3_create_devices(struct ioc3_priv_data *ipd)
> +{
> + struct mfd_cell *cell;
> +
> + memset(ioc3_mfd_cells, 0, sizeof(ioc3_mfd_cells));
> + cell = ioc3_mfd_cells;
> +
> + if (ipd->info->funcs & IOC3_ETH) {
> + memcpy(ioc3_eth_platform_data.mac_addr, ipd->nic_mac,
> + sizeof(ioc3_eth_platform_data.mac_addr));
Better to pull the MAC address from within the Ethernet driver.
> + cell->name = "ioc3-eth";
> + cell->id = ioc3_eth_id++;
> + cell->resources = ioc3_eth_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_eth_resources);
> + cell->platform_data = &ioc3_eth_platform_data;
> + cell->pdata_size = sizeof(ioc3_eth_platform_data);
Please define all of this in a static struct, external to this
function.
> + if (ipd->info->irq_offset) {
> + /*
> + * Ethernet interrupt is on an extra interrupt
> + * not inside the irq domain, so we need an
> + * extra mfd_add_devices without the domain
> + * argument
> + */
> + ioc3_eth_resources[2].start = ipd->pdev->irq;
> + ioc3_eth_resources[2].end = ipd->pdev->irq;
Using '2' is fragile.
In this case, seeing as you're using the parent's IRQ, you can obtain
the IRQ using the usual platform_*() handlers, using pdev->parent.
> + mfd_add_devices(&ipd->pdev->dev, -1, cell, 1,
Don't use -1, use the defines instead.
Instead of 1, use ARRAY_SIZE() once the cells are defined statically.
> + &ipd->pdev->resource[0], 0, NULL);
> + memset(cell, 0, sizeof(*cell));
> + } else {
> + /* fake hwirq in domain */
What does this mean?
> + ioc3_eth_resources[2].start = 23;
> + ioc3_eth_resources[2].end = 23;
> + cell++;
> + }
> + }
> + if (ipd->info->funcs & IOC3_SER) {
> + writel(GPCR_UARTA_MODESEL | GPCR_UARTB_MODESEL,
> + &ipd->regs->gpcr_s);
> + writel(0, &ipd->regs->gppr[6]);
> + writel(0, &ipd->regs->gppr[7]);
> + udelay(100);
> + 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);
No idea what any of this does.
It looks like it belongs in the serial driver (and needs comments).
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uarta_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
> + cell++;
> + cell->name = "ioc3-serial8250";
> + cell->id = ioc3_serial_id++;
> + cell->resources = ioc3_uartb_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_uartb_resources);
> + cell++;
Please define this statically.
> + }
> + if (ipd->info->funcs & IOC3_KBD) {
> + cell->name = "ioc3-kbd",
> + cell->id = ioc3_kbd_id++;
> + cell->resources = ioc3_kbd_resources,
> + cell->num_resources = ARRAY_SIZE(ioc3_kbd_resources),
> + cell++;
As above.
> + }
> +#if defined(CONFIG_SGI_IP27)
What is this? Can't you obtain this dynamically by probing the H/W?
> + if (ipd->info->funcs & IOC3_M48T35) {
> + cell->name = "rtc-m48t35";
> + cell->id = -1;
> + cell->resources = ioc3_rtc_resources;
> + cell->num_resources = ARRAY_SIZE(ioc3_rtc_resources);
> + cell++;
Static please.
> + }
> +#endif
> + mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
Use the defines.
> + cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
?
> + 0, ipd->domain);
> +}
> +
> +static int ioc3_mfd_probe(struct pci_dev *pdev,
> + const struct pci_device_id *pci_id)
> +{
> + struct ioc3_priv_data *ipd;
> + int err, ret = -ENODEV, io_irqno;
> + struct ioc3 __iomem *regs;
> + struct irq_domain *domain;
> + struct fwnode_handle *fn;
> +
> + err = pci_enable_device(pdev);
> + if (err)
> + return err;
> +
> + pci_write_config_byte(pdev, PCI_LATENCY_TIMER, 64);
> + 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");
> + 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 = kzalloc(sizeof(struct ioc3_priv_data), GFP_KERNEL);
Does PCI not provide managed (devm_*() like) helpers?
> + if (!ipd) {
> + ret = -ENOMEM;
> + goto out_disable_device;
> + }
> + ipd->pdev = pdev;
'\n'
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
> + regs = pci_ioremap_bar(pdev, 0);
> + if (!regs) {
> + pr_warn("ioc3: Unable to remap PCI BAR for %s.\n",
> + pci_name(pdev));
> + goto out_free_ipd;
> + }
> + ipd->regs = regs;
> +
> + /* Track PCI-device specific data */
> + pci_set_drvdata(pdev, ipd);
Do you don't need 'ipd->pdev = pdev' then.
> + writel(GPCR_MLAN_EN, &ipd->regs->gpcr_s);
> + ioc3_probe_nic(ipd, ®s->mcr);
This should probably be moved to the networking driver.
> +#ifdef CONFIG_SGI_IP27
No #ifery in MFD c-files please.
> + /* BaseIO NIC is attached to bridge */
> + if (!ipd->nic_part[0]) {
> + struct bridge_controller *bc = BRIDGE_CONTROLLER(pdev->bus);
> +
> + ioc3_probe_nic(ipd, &bc->base->b_nic);
> + }
> +#endif
> +
> + if (ioc3_identify(ipd)) {
> + pr_err("ioc3: part: [%s] unknown card\n", ipd->nic_part);
> + goto out_iounmap;
> + }
> +
> + pr_info("ioc3: part: [%s] %s\n", ipd->nic_part, ipd->info->name);
> +
> + /* Clear IRQs */
A comment! I just fell off my chair! =;-)
> + writel(~0, ®s->sio_iec);
> + writel(~0, ®s->sio_ir);
> + writel(0, ®s->eth.eier);
> + writel(~0, ®s->eth.eisr);
> +
> + if (ipd->info->irq_offset) {
What does this really signify?
> + struct pci_host_bridge *hbrg = pci_find_host_bridge(pdev->bus);
> +
> + io_irqno = hbrg->map_irq(pdev,
> + PCI_SLOT(pdev->devfn) + ipd->info->irq_offset, 0);
> + } else {
> + io_irqno = pdev->irq;
> + }
> + ipd->irq_io = io_irqno;
> +
> + fn = irq_domain_alloc_named_fwnode("IOC3");
> + if (!fn)
> + goto out_iounmap;
> +
> + domain = irq_domain_create_linear(fn, 24, &ioc3_irq_domain_ops, ipd);
> + irq_domain_free_fwnode(fn);
> + if (!domain)
> + goto out_iounmap;
> + ipd->domain = domain;
> +
> + irq_set_chained_handler_and_data(io_irqno, ioc3_irq_handler, domain);
> +
> + ioc3_create_devices(ipd);
> +
> + return 0;
> +
> +out_iounmap:
> + iounmap(ipd->regs);
> +
> +out_free_ipd:
> + kfree(ipd);
> +
> +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 */
> + irq_domain_remove(ipd->domain);
> + free_irq(ipd->irq_io, (void *)ipd);
> + iounmap(ipd->regs);
> +
> + pci_disable_device(pdev);
> +
> + kfree(ipd);
> +}
> +
> +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@xxxxxxx>");
> +MODULE_DESCRIPTION("SGI IOC3 MFD driver");
> +MODULE_LICENSE("GPL");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
