Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add interrupt support for port phy link

[<Arun.Ramadoss@microchip.com>]

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