Messages in this thread | | | From | <> | Subject | Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link | Date | Thu, 1 Sep 2022 09:02:51 +0000 |
| |
On Tue, 2022-08-30 at 15:25 +0200, Andrew Lunn wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > > @@ -85,6 +94,7 @@ struct ksz_port { > > u32 rgmii_tx_val; > > u32 rgmii_rx_val; > > struct ksz_device *ksz_dev; > > + struct ksz_irq irq; > > Here irq is of type ksz_irq. > > > u8 num; > > }; > > > > @@ -103,6 +113,7 @@ struct ksz_device { > > struct regmap *regmap[3]; > > > > void *priv; > > + int irq; > > Here it is of type int. > > > > > struct gpio_desc *reset_gpio; /* Optional reset GPIO */ > > > > @@ -124,6 +135,8 @@ struct ksz_device { > > u16 mirror_tx; > > u32 features; /* chip specific features */ > > u16 port_mask; > > + struct mutex lock_irq; /* IRQ Access */ > > + struct ksz_irq girq; > > And here you have the type ksz_irq called qirq. This is going to be > confusing. > > I suggest you make the first one called pirq, for port, and then we > have girq for global, and irq is just a number.
Ok. I will update variable names as per the context it is used.
> > > static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits, > > bool set) > > { > > return regmap_update_bits(dev->regmap[0], addr, bits, set ? > > bits : 0); > > @@ -171,6 +175,7 @@ static int lan937x_mdio_register(struct > > ksz_device *dev) > > struct device_node *mdio_np; > > struct mii_bus *bus; > > int ret; > > + int p; > > > > mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio"); > > if (!mdio_np) { > > @@ -194,6 +199,16 @@ static int lan937x_mdio_register(struct > > ksz_device *dev) > > > > ds->slave_mii_bus = bus; > > > > + for (p = 0; p < KSZ_MAX_NUM_PORTS; p++) { > > + if (BIT(p) & ds->phys_mii_mask) { > > + unsigned int irq; > > + > > + irq = irq_find_mapping(dev- > > >ports[p].irq.domain, > > + PORT_SRC_PHY_INT); > > This could return an error code. You really should check for it, the > irq subsystem is not going to be happy with a negative irq number.
Ok. I will check the return value for irq_find_mapping.
> > > + ds->slave_mii_bus->irq[p] = irq; > > + } > > + } > > + > > ret = devm_of_mdiobus_register(ds->dev, bus, mdio_np); > > if (ret) { > > dev_err(ds->dev, "unable to register MDIO bus %s\n", > > I don't see anywhere you destroy the mappings you created above when > the MDIO bus is unregistered. The equivalent of > mv88e6xxx_g2_irq_mdio_free().
I missed to destroy it. I will add.
> > > > +static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id) > > +{ > > + struct ksz_device *dev = dev_id; > > + unsigned int nhandled = 0; > > + unsigned int sub_irq; > > + unsigned int n; > > + u32 data; > > + int ret; > > + > > + ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data); > > + if (ret) > > + goto out; > > + > > + if (data & POR_READY_INT) { > > + ret = ksz_write32(dev, REG_SW_INT_STATUS__4, > > POR_READY_INT); > > + if (ret) > > + goto out; > > + } > > What do these two read/writes do? It seems like you are discarding an > interrupt?
This interrupt in Power on reset interrupt. It is enabled by default in the chip. I am not performing any operation based on POR. So I just cleared the interrupt. Do I need to disable the POR interrupt in the setup function, since no operation is performed based on it?
> > > > + > > + /* Read global interrupt status register */ > > + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data); > > + if (ret) > > + goto out; > > + > > + for (n = 0; n < dev->girq.nirqs; ++n) { > > + if (data & (1 << n)) { > > + sub_irq = irq_find_mapping(dev->girq.domain, > > n); > > + handle_nested_irq(sub_irq); > > + ++nhandled; > > + } > > + } > > +out: > > + return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE); > > +} > > + > > +static irqreturn_t lan937x_pirq_thread_fn(int irq, void *dev_id) > > +{ > > + struct ksz_port *port = dev_id; > > + unsigned int nhandled = 0; > > + struct ksz_device *dev; > > + unsigned int sub_irq; > > + unsigned int n; > > + u8 data; > > + > > + dev = port->ksz_dev; > > + > > + /* Read global interrupt status register */ > > + ksz_pread8(dev, port->num, REG_PORT_INT_STATUS, &data); > > I think global here should be port?
Yes. Copy paste problem. I will update it.
> > > + > > + for (n = 0; n < port->irq.nirqs; ++n) { > > + if (data & (1 << n)) { > > + sub_irq = irq_find_mapping(port->irq.domain, > > n); > > + handle_nested_irq(sub_irq); > > + ++nhandled; > > + } > > + } > > + > > + return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE); > > +} > > + > > > > void lan937x_switch_exit(struct ksz_device *dev) > > { > > + struct dsa_port *dp; > > + > > lan937x_reset_switch(dev); > > + > > + if (dev->irq > 0) { > > + dsa_switch_for_each_user_port(dp, dev->ds) { > > + lan937x_pirq_free(&dev->ports[dp->index], dp- > > >index); > > + } > > + > > + lan937x_girq_free(dev); > > This is where your problem with exit vs reset is coming from. You > setup all the interrupt code in setup(). But currently there is no > function which is the opposite of setup(). Generally, it is called > tairdown. Add such a function.
Yes I was looking for opposite function for setup(). Thanks for suggestion, I will update the code accordingly and remove the patch 1/3 from the series which replaces the exit with reset.
> > Andrew
| |