| Message ID | 20201112153537.22383-7-ceggers@arri.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: dsa: microchip: PTP support for KSZ956x | expand |
On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote: > Interrupts are required for TX time stamping. Probably they could also > be used for PHY connection status. Do the KSZ switches have an internal PHY? And there's a single interrupt line, shared between the PTP timestamping engine, and the internal PHY that is driver by phylib? > This patch only adds the basic infrastructure for interrupts, no > interrupts are actually enabled nor handled. > > ksz9477_reset_switch() must be called before requesting the IRQ (in > ksz9477_init() instead of ksz9477_setup()). A patch can never be "too simple". Maybe you could factor out that code movement into a separate patch. > Signed-off-by: Christian Eggers <ceggers@arri.de> > --- > drivers/net/dsa/microchip/ksz9477_i2c.c | 2 + > drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++-- > drivers/net/dsa/microchip/ksz9477_spi.c | 2 + > include/linux/dsa/ksz_common.h | 1 + > 4 files changed, 100 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c > index 4e053a25d077..4ed1f503044a 100644 > --- a/drivers/net/dsa/microchip/ksz9477_i2c.c > +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c > @@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c, > if (i2c->dev.platform_data) > dev->pdata = i2c->dev.platform_data; > > + dev->irq = i2c->irq; > + > ret = ksz9477_switch_register(dev); > > /* Main DSA driver may not be started yet. */ > diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c > index abfd3802bb51..6b5a981fb21f 100644 > --- a/drivers/net/dsa/microchip/ksz9477_main.c > +++ b/drivers/net/dsa/microchip/ksz9477_main.c > @@ -7,7 +7,9 @@ > > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/interrupt.h> > #include <linux/iopoll.h> > +#include <linux/irq.h> > #include <linux/platform_data/microchip-ksz.h> > #include <linux/phy.h> > #include <linux/if_bridge.h> > @@ -1345,19 +1347,12 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) > static int ksz9477_setup(struct dsa_switch *ds) > { > struct ksz_device *dev = ds->priv; > - int ret = 0; > > dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table), > dev->num_vlans, GFP_KERNEL); > if (!dev->vlan_cache) > return -ENOMEM; > > - ret = ksz9477_reset_switch(dev); > - if (ret) { > - dev_err(ds->dev, "failed to reset switch\n"); > - return ret; > - } > - > /* Required for port partitioning. */ > ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY, > true); > @@ -1535,12 +1530,84 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = { > }, > }; > > +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id) > +{ > + struct ksz_device *dev = dev_id; > + u32 data; > + int port; > + int ret; > + irqreturn_t result = IRQ_NONE; Please keep local variable declaration sorted in the reverse order of line length. But.... > + > + /* Read global port interrupt status register */ > + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data); > + if (ret) > + return result; ...Is there any point at all in keeping the "result" variable? > + > + for (port = 0; port < dev->port_cnt; port++) { > + if (data & BIT(port)) { You can reduce the indentation level by 1 here using: if (!(data & BIT(port))) continue; > + u8 data8; > + > + /* Read port interrupt status register */ > + ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS), > + &data8); > + if (ret) > + return result; > + > + /* ToDo: Add specific handling of port interrupts */ Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to this commit exactly, will emit interrupts and complain that nobody cared. > + } > + } > + > + return result; > +} > + > +static int ksz9477_enable_port_interrupts(struct ksz_device *dev) > +{ > + u32 data; > + int ret; > + > + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); > + if (ret) > + return ret; > + > + /* Enable port interrupts (0 means enabled) */ > + data &= ~((1 << dev->port_cnt) - 1); And what's the " - 1" for? > + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > + if (ret) > + return ret; > + > + return 0; return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > +} > + > +static int ksz9477_disable_port_interrupts(struct ksz_device *dev) > +{ > + u32 data; > + int ret; > + > + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); > + if (ret) > + return ret; > + > + /* Disable port interrupts (1 means disabled) */ > + data |= ((1 << dev->port_cnt) - 1); > + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > + if (ret) > + return ret; > + > + return 0; same comments as above. Also, it's almost as if you want to implement these in the same function, with a "bool enable"? > +} > + > static int ksz9477_switch_init(struct ksz_device *dev) > { > - int i; > + int i, ret; > > dev->ds->ops = &ksz9477_switch_ops; > > + ret = ksz9477_reset_switch(dev); > + if (ret) { > + dev_err(dev->dev, "failed to reset switch\n"); > + return ret; > + } > + > for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) { > const struct ksz_chip_data *chip = &ksz9477_switch_chips[i]; > > @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device *dev) > > /* set the real number of ports */ > dev->ds->num_ports = dev->port_cnt; > + if (dev->irq > 0) { > + unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq)); What is irqd_get_trigger_type and what does it have to do with the "irqflags" argument of request_threaded_irq? Where else have you even seen this? > + > + irqflags |= IRQF_ONESHOT; And shared maybe? > + ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL, > + ksz9477_switch_irq_thread, > + irqflags, > + dev_name(dev->dev), > + dev); > + if (ret) { > + dev_err(dev->dev, "failed to request IRQ.\n"); > + return ret; > + } > + > + ret = ksz9477_enable_port_interrupts(dev); > + if (ret) > + return ret; Could you also clear pending interrupts before enabling the line? > + } > > return 0; > } > > static void ksz9477_switch_exit(struct ksz_device *dev) > { > + if (dev->irq > 0) > + ksz9477_disable_port_interrupts(dev); I think it'd look a bit nicer if you moved this condition into ksz9477_disable_port_interrupts: if (!dev->irq) return; > ksz9477_reset_switch(dev); > } >
On Friday, 13 November 2020, 00:26:17 CET, Vladimir Oltean wrote: > On Thu, Nov 12, 2020 at 04:35:32PM +0100, Christian Eggers wrote: > > Interrupts are required for TX time stamping. Probably they could also > > be used for PHY connection status. > > Do the KSZ switches have an internal PHY? And there's a single interrupt > line, shared between the PTP timestamping engine, and the internal PHY > that is driver by phylib? The device has only one interrupt line (INTRP_N), although there may be applications which use additionally the GPIO (PPS/PEROUT) output as an interrupt. I assume that the PHY driver currently uses polling (as the KSZ9477 driver used to have no interrupt functionality. Maybe this can be changed in future, as the KSZ hardware has hierarchical interrupt enable/status registers. > > This patch only adds the basic infrastructure for interrupts, no > > interrupts are actually enabled nor handled. > > > > ksz9477_reset_switch() must be called before requesting the IRQ (in > > ksz9477_init() instead of ksz9477_setup()). > > A patch can never be "too simple". Maybe you could factor out that code > movement into a separate patch. I haven't checked yet, but I'll try. [...] > > +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id) > > +{ > > + struct ksz_device *dev = dev_id; > > + u32 data; > > + int port; > > + int ret; > > + irqreturn_t result = IRQ_NONE; > > Please keep local variable declaration sorted in the reverse order of > line length. But.... > > > + > > + /* Read global port interrupt status register */ > > + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data); > > + if (ret) > > + return result; > > ...Is there any point at all in keeping the "result" variable? > > > + > > + for (port = 0; port < dev->port_cnt; port++) { > > + if (data & BIT(port)) { > > You can reduce the indentation level by 1 here using: > > if (!(data & BIT(port))) > continue; > > > + u8 data8; > > + > > + /* Read port interrupt status register */ > > + ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS), > > + &data8); > > + if (ret) > > + return result; > > + > > + /* ToDo: Add specific handling of port interrupts */ > > Buggy? Please return IRQ_HANDLED, otherwise the system, when bisected to > this commit exactly, will emit interrupts and complain that nobody cared. Probably this can be kept as it is. The hardware will only emit interrupts if these have been explicitly enabled. Although the *port* interrupts are enabled here (and all bits in the "Port Interrupt Mask Register" (section 5.2.1.12) are active after reset), actually no interrupts should be raised as the ports sub units (PTP, PHY and ACL) don't emit interrupt after reset: - PHY (section 5.2.2.19): All interrupts are disabled after reset - PTP (section 5.2.11.11): dito - ACL (not found): I got never interrupts from here > > > + } > > + } > > + > > + return result; > > +} > > + > > +static int ksz9477_enable_port_interrupts(struct ksz_device *dev) > > +{ > > + u32 data; > > + int ret; > > + > > + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); > > + if (ret) > > + return ret; > > + > > + /* Enable port interrupts (0 means enabled) */ > > + data &= ~((1 << dev->port_cnt) - 1); > > And what's the " - 1" for? I build a bitmask where the bits 0..(dev->port_cnt-1) are set... I'll whether GENMASK() can be used with variable data as argument. > > > + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > > + if (ret) > > + return ret; > > + > > + return 0; > > return ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > > > +} > > + > > +static int ksz9477_disable_port_interrupts(struct ksz_device *dev) > > +{ > > + u32 data; > > + int ret; > > + > > + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); > > + if (ret) > > + return ret; > > + > > + /* Disable port interrupts (1 means disabled) */ > > + data |= ((1 << dev->port_cnt) - 1); > > + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); > > + if (ret) > > + return ret; > > + > > + return 0; > > same comments as above. > > Also, it's almost as if you want to implement these in the same > function, with a "bool enable"? You are right. > > > +} > > + > > > > static int ksz9477_switch_init(struct ksz_device *dev) > > { > > > > - int i; > > + int i, ret; > > > > dev->ds->ops = &ksz9477_switch_ops; > > > > + ret = ksz9477_reset_switch(dev); > > + if (ret) { > > + dev_err(dev->dev, "failed to reset switch\n"); > > + return ret; > > + } > > + > > > > for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) { > > > > const struct ksz_chip_data *chip = &ksz9477_switch_chips[i]; > > > > @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device > > *dev)> > > /* set the real number of ports */ > > dev->ds->num_ports = dev->port_cnt; > > > > + if (dev->irq > 0) { > > + unsigned long irqflags = > > irqd_get_trigger_type(irq_get_irq_data(dev->irq)); > What is irqd_get_trigger_type and what does it have to do with the > "irqflags" argument of request_threaded_irq? Where else have you even > seen this? No idea where I originally found this. It's some time ago when I wrote this. > > > + > > + irqflags |= IRQF_ONESHOT; > > And shared maybe? I don't need it. Is there a rule when to add shared? At least the KSZ should be able to tell whether it has raised an IRQ or not. > > > + ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL, > > + ksz9477_switch_irq_thread, > > + irqflags, > > + dev_name(dev->dev), > > + dev); > > + if (ret) { > > + dev_err(dev->dev, "failed to request IRQ.\n"); > > + return ret; > > + } > > + > > + ret = ksz9477_enable_port_interrupts(dev); > > + if (ret) > > + return ret; > > Could you also clear pending interrupts before enabling the line? As the device has just been reset and no concrete interrupts have been enabled, there should be no need for this. > > > + } > > > > return 0; > > > > } > > > > static void ksz9477_switch_exit(struct ksz_device *dev) > > { > > > > + if (dev->irq > 0) > > + ksz9477_disable_port_interrupts(dev); > > I think it'd look a bit nicer if you moved this condition into > ksz9477_disable_port_interrupts: > > if (!dev->irq) > return; > > > ksz9477_reset_switch(dev); > > > > } regards Christian
diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c index 4e053a25d077..4ed1f503044a 100644 --- a/drivers/net/dsa/microchip/ksz9477_i2c.c +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c @@ -41,6 +41,8 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c, if (i2c->dev.platform_data) dev->pdata = i2c->dev.platform_data; + dev->irq = i2c->irq; + ret = ksz9477_switch_register(dev); /* Main DSA driver may not be started yet. */ diff --git a/drivers/net/dsa/microchip/ksz9477_main.c b/drivers/net/dsa/microchip/ksz9477_main.c index abfd3802bb51..6b5a981fb21f 100644 --- a/drivers/net/dsa/microchip/ksz9477_main.c +++ b/drivers/net/dsa/microchip/ksz9477_main.c @@ -7,7 +7,9 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/interrupt.h> #include <linux/iopoll.h> +#include <linux/irq.h> #include <linux/platform_data/microchip-ksz.h> #include <linux/phy.h> #include <linux/if_bridge.h> @@ -1345,19 +1347,12 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds) static int ksz9477_setup(struct dsa_switch *ds) { struct ksz_device *dev = ds->priv; - int ret = 0; dev->vlan_cache = devm_kcalloc(dev->dev, sizeof(struct vlan_table), dev->num_vlans, GFP_KERNEL); if (!dev->vlan_cache) return -ENOMEM; - ret = ksz9477_reset_switch(dev); - if (ret) { - dev_err(ds->dev, "failed to reset switch\n"); - return ret; - } - /* Required for port partitioning. */ ksz9477_cfg32(dev, REG_SW_QM_CTRL__4, UNICAST_VLAN_BOUNDARY, true); @@ -1535,12 +1530,84 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = { }, }; +static irqreturn_t ksz9477_switch_irq_thread(int irq, void *dev_id) +{ + struct ksz_device *dev = dev_id; + u32 data; + int port; + int ret; + irqreturn_t result = IRQ_NONE; + + /* Read global port interrupt status register */ + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data); + if (ret) + return result; + + for (port = 0; port < dev->port_cnt; port++) { + if (data & BIT(port)) { + u8 data8; + + /* Read port interrupt status register */ + ret = ksz_read8(dev, PORT_CTRL_ADDR(port, REG_PORT_INT_STATUS), + &data8); + if (ret) + return result; + + /* ToDo: Add specific handling of port interrupts */ + } + } + + return result; +} + +static int ksz9477_enable_port_interrupts(struct ksz_device *dev) +{ + u32 data; + int ret; + + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); + if (ret) + return ret; + + /* Enable port interrupts (0 means enabled) */ + data &= ~((1 << dev->port_cnt) - 1); + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); + if (ret) + return ret; + + return 0; +} + +static int ksz9477_disable_port_interrupts(struct ksz_device *dev) +{ + u32 data; + int ret; + + ret = ksz_read32(dev, REG_SW_PORT_INT_MASK__4, &data); + if (ret) + return ret; + + /* Disable port interrupts (1 means disabled) */ + data |= ((1 << dev->port_cnt) - 1); + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, data); + if (ret) + return ret; + + return 0; +} + static int ksz9477_switch_init(struct ksz_device *dev) { - int i; + int i, ret; dev->ds->ops = &ksz9477_switch_ops; + ret = ksz9477_reset_switch(dev); + if (ret) { + dev_err(dev->dev, "failed to reset switch\n"); + return ret; + } + for (i = 0; i < ARRAY_SIZE(ksz9477_switch_chips); i++) { const struct ksz_chip_data *chip = &ksz9477_switch_chips[i]; @@ -1584,12 +1651,32 @@ static int ksz9477_switch_init(struct ksz_device *dev) /* set the real number of ports */ dev->ds->num_ports = dev->port_cnt; + if (dev->irq > 0) { + unsigned long irqflags = irqd_get_trigger_type(irq_get_irq_data(dev->irq)); + + irqflags |= IRQF_ONESHOT; + ret = devm_request_threaded_irq(dev->dev, dev->irq, NULL, + ksz9477_switch_irq_thread, + irqflags, + dev_name(dev->dev), + dev); + if (ret) { + dev_err(dev->dev, "failed to request IRQ.\n"); + return ret; + } + + ret = ksz9477_enable_port_interrupts(dev); + if (ret) + return ret; + } return 0; } static void ksz9477_switch_exit(struct ksz_device *dev) { + if (dev->irq > 0) + ksz9477_disable_port_interrupts(dev); ksz9477_reset_switch(dev); } diff --git a/drivers/net/dsa/microchip/ksz9477_spi.c b/drivers/net/dsa/microchip/ksz9477_spi.c index 1142768969c2..d2eea9596e53 100644 --- a/drivers/net/dsa/microchip/ksz9477_spi.c +++ b/drivers/net/dsa/microchip/ksz9477_spi.c @@ -48,6 +48,8 @@ static int ksz9477_spi_probe(struct spi_device *spi) if (spi->dev.platform_data) dev->pdata = spi->dev.platform_data; + dev->irq = spi->irq; + ret = ksz9477_switch_register(dev); /* Main DSA driver may not be started yet. */ diff --git a/include/linux/dsa/ksz_common.h b/include/linux/dsa/ksz_common.h index 3b22380d85c5..bf57ba4b2132 100644 --- a/include/linux/dsa/ksz_common.h +++ b/include/linux/dsa/ksz_common.h @@ -55,6 +55,7 @@ struct ksz_device { struct device *dev; struct regmap *regmap[3]; + int irq; void *priv;
Interrupts are required for TX time stamping. Probably they could also be used for PHY connection status. This patch only adds the basic infrastructure for interrupts, no interrupts are actually enabled nor handled. ksz9477_reset_switch() must be called before requesting the IRQ (in ksz9477_init() instead of ksz9477_setup()). Signed-off-by: Christian Eggers <ceggers@arri.de> --- drivers/net/dsa/microchip/ksz9477_i2c.c | 2 + drivers/net/dsa/microchip/ksz9477_main.c | 103 +++++++++++++++++++++-- drivers/net/dsa/microchip/ksz9477_spi.c | 2 + include/linux/dsa/ksz_common.h | 1 + 4 files changed, 100 insertions(+), 8 deletions(-)