Re: [PATCH net-next 6/6 v2] net: dsa: microchip: Setup fiber ports for KSZ8463

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Tristram

On Tue, 8 Jul 2025 19:45:44 +0000
<Tristram.Ha@microchip.com> wrote:

> > Hi Tristram,
> > 
> > On Mon, 7 Jul 2025 20:16:48 -0700
> > <Tristram.Ha@microchip.com> wrote:
> >   
> > > From: Tristram Ha <tristram.ha@microchip.com>
> > >
> > > The fiber ports in KSZ8463 cannot be detected internally, so it requires
> > > specifying that condition in the device tree.  Like the one used in
> > > Micrel PHY the port link can only be read and there is no write to the
> > > PHY.  The driver programs registers to operate fiber ports correctly.
> > >
> > > The PTP function of the switch is also turned off as it may interfere the
> > > normal operation of the MAC.
> > >
> > > Signed-off-by: Tristram Ha <tristram.ha@microchip.com>
> > > ---
> > >  drivers/net/dsa/microchip/ksz8.c       | 26 ++++++++++++++++++++++++++
> > >  drivers/net/dsa/microchip/ksz_common.c |  3 +++
> > >  2 files changed, 29 insertions(+)
> > >
> > > diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> > > index 904db68e11f3..1207879ef80c 100644
> > > --- a/drivers/net/dsa/microchip/ksz8.c
> > > +++ b/drivers/net/dsa/microchip/ksz8.c
> > > @@ -1715,6 +1715,7 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
> > >       const u32 *masks;
> > >       const u16 *regs;
> > >       u8 remote;
> > > +     u8 fiber_ports = 0;
> > >       int i;
> > >
> > >       masks = dev->info->masks;
> > > @@ -1745,6 +1746,31 @@ void ksz8_config_cpu_port(struct dsa_switch *ds)
> > >               else
> > >                       ksz_port_cfg(dev, i, regs[P_STP_CTRL],
> > >                                    PORT_FORCE_FLOW_CTRL, false);
> > > +             if (p->fiber)
> > > +                     fiber_ports |= (1 << i);
> > > +     }
> > > +     if (ksz_is_ksz8463(dev)) {
> > > +             /* Setup fiber ports. */  
> > 
> > What does fiber port mean ? Is it 100BaseFX ? As this configuration is
> > done only for the CPU port (it seems), looks like this mode is planned
> > to be used as the MAC to MAC mode on the DSA conduit. So, instead of
> > using this property maybe you should implement that as handling the
> > "100base-x" phy-mode ?
> >   
> > > +             if (fiber_ports) {
> > > +                     regmap_update_bits(ksz_regmap_16(dev),
> > > +                                        reg16(dev, KSZ8463_REG_CFG_CTRL),
> > > +                                        fiber_ports << PORT_COPPER_MODE_S,
> > > +                                        0);
> > > +                     regmap_update_bits(ksz_regmap_16(dev),
> > > +                                        reg16(dev, KSZ8463_REG_DSP_CTRL_6),
> > > +                                        COPPER_RECEIVE_ADJUSTMENT, 0);
> > > +             }
> > > +
> > > +             /* Turn off PTP function as the switch's proprietary way of
> > > +              * handling timestamp is not supported in current Linux PTP
> > > +              * stack implementation.
> > > +              */
> > > +             regmap_update_bits(ksz_regmap_16(dev),
> > > +                                reg16(dev, KSZ8463_PTP_MSG_CONF1),
> > > +                                PTP_ENABLE, 0);
> > > +             regmap_update_bits(ksz_regmap_16(dev),
> > > +                                reg16(dev, KSZ8463_PTP_CLK_CTRL),
> > > +                                PTP_CLK_ENABLE, 0);
> > >       }
> > >  }
> > >
> > > diff --git a/drivers/net/dsa/microchip/ksz_common.c  
> > b/drivers/net/dsa/microchip/ksz_common.c  
> > > index c08e6578a0df..b3153b45ced9 100644
> > > --- a/drivers/net/dsa/microchip/ksz_common.c
> > > +++ b/drivers/net/dsa/microchip/ksz_common.c
> > > @@ -5441,6 +5441,9 @@ int ksz_switch_register(struct ksz_device *dev)
> > >                                               &dev->ports[port_num].interface);
> > >
> > >                               ksz_parse_rgmii_delay(dev, port_num, port);
> > > +                             dev->ports[port_num].fiber =
> > > +                                     of_property_read_bool(port,
> > > +                                                           "micrel,fiber-mode");  
> > 
> > Shouldn't this be described in the binding ?
> >   
> > >                       }
> > >                       of_node_put(ports);
> > >               }  
> 
> The "micrel,fiber-mode" is described in Documentation/devicetree/
> bindings/net/micrel.txt.

Yes but that's for PHYs right ? Yours is under the DSA "ports"
node.

> 
> Some old KSZ88XX switches have option of using fiber in a port running
> 100base-fx.  Typically they have a register indicating that configuration
> and the driver just treats the port as having a PHY and reads the link
> status and speed as normal except there is no write to those PHY related
> registers.  KSZ8463 does not have that option so the driver needs to be
> told.

That's what I understood from your comments indeed, what thew me off
guard is that all ports's fiber mode is configured in the
config_cpu_port() callback.

I'd like to one day be able to deprecate these
micrel,fiber-mode/ti,fiber-mode properties in favor of the ports API
that's being worked on, but I guess we can roll with it for now.

Maxime