Re: [PATCH net-next 04/10] net: dsa: microchip: add PTP interrupt handling for KSZ8463

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

Hi all,

On 7/9/26 8:42 AM, Bastien Curutchet (Schneider Electric) wrote:
> KSZ8463 PTP interrupts aren't handled by the driver.
> The interrupt layout in KSZ8463 has nothing to do with the other
> switches:
> - Its global interrupt enable register is 16-bits long and follow an
>    'enable' logic, instead of a 'mask' one
> - all the interrupts of all ports are grouped into one status register
>    while others have one interrupt register per port
> - xdelay_req and pdresp timestamps share one single interrupt bit on the
>    KSZ8463 while each of them has its own interrupt bit on other switches
> 
> Create a KSZ8463-specific set of interrupt domain operations to handle
> the global IRQ layer. To limit code duplication, it uses the same
> interrupt handler than the other switches. Since other switches have
> 8-bits registers, only the high-byte of the interrupt status/enable
> registers are used. This high-byte is where the PTP interrupts are
> located. The low-byte contains the wake-up detection interrupts so if at
> some points these interrupts are needed we'll need a bit of rework here.
> 
> Create KSZ8463-specific functions to setup the PTP interrupts. The
> created IRQ domain is tied to the first port of the KSZ8463. Again,
> the same PTP interrupt handler than the others switches is used.
> 
> Implement the teardown callback to release the interrupts.
> 
> Signed-off-by: Bastien Curutchet (Schneider Electric) <bastien.curutchet@bootlin.com>
> ---
>   drivers/net/dsa/microchip/ksz8.c        |  93 ++++++++++++++++++++++-
>   drivers/net/dsa/microchip/ksz_ptp.c     | 129 ++++++++++++++++++++++++++++++++
>   drivers/net/dsa/microchip/ksz_ptp.h     |   9 +++
>   drivers/net/dsa/microchip/ksz_ptp_reg.h |   6 ++
>   4 files changed, 235 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz8.c b/drivers/net/dsa/microchip/ksz8.c
> index 3bbca6f9cfc5..c099a7005808 100644
> --- a/drivers/net/dsa/microchip/ksz8.c
> +++ b/drivers/net/dsa/microchip/ksz8.c
> @@ -36,6 +36,13 @@
>   #include "ksz8_reg.h"
>   #include "ksz8.h"
>   
> +/*
> + * We use only the high-byte (so odd addresses) of the 16-bits registers to fit
> + * in the common IRQ framework
> + */
> +#define KSZ8463_REG_ISR			0x191
> +#define KSZ8463_REG_IER			0x193
> +
>   /* ksz88x3_drive_strengths - Drive strength mapping for KSZ8863, KSZ8873, ..
>    *			     variants.
>    * This values are documented in KSZ8873 and KSZ8863 datasheets.
> @@ -181,6 +188,58 @@ static int ksz8_pme_pwrite8(struct ksz_device *dev, int port, int offset, u8 dat
>   	return ksz8_ind_write8(dev, table, (u8)(offset), data);
>   }
>   
> +static void ksz8463_irq_mask(struct irq_data *d)
> +{
> +	struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
> +
> +	kirq->masked &= ~BIT(d->hwirq);
> +}
> +
> +static void ksz8463_irq_unmask(struct irq_data *d)
> +{
> +	struct ksz_irq *kirq = irq_data_get_irq_chip_data(d);
> +
> +	kirq->masked |= BIT(d->hwirq);
> +}
> +
> +static const struct irq_chip ksz8463_irq_chip = {
> +	.name			= "ksz8463-irq",
> +	.irq_mask		= ksz8463_irq_mask,
> +	.irq_unmask		= ksz8463_irq_unmask,
> +	.irq_bus_lock		= ksz_irq_bus_lock,
> +	.irq_bus_sync_unlock	= ksz_irq_bus_sync_unlock,
> +};
> +
> +static int ksz8463_irq_domain_map(struct irq_domain *d,
> +				  unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(irq, d->host_data);
> +	irq_set_chip_and_handler(irq, &ksz8463_irq_chip, handle_level_irq);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops ksz8463_irq_domain_ops = {
> +	.map	= ksz8463_irq_domain_map,
> +	.xlate	= irq_domain_xlate_twocell,
> +};
> +
> +static int ksz8463_girq_setup(struct ksz_device *dev)
> +{
> +	struct ksz_irq *girq = &dev->girq;
> +
> +	girq->nirqs = 8;
> +	girq->reg_mask = KSZ8463_REG_IER;
> +	girq->reg_status = KSZ8463_REG_ISR;
> +	girq->masked = 0;
> +	snprintf(girq->name, sizeof(girq->name), "ksz8463-girq");
> +
> +	girq->irq_num = dev->irq;
> +
> +	return ksz_irq_common_setup(dev, girq, &ksz8463_irq_domain_ops);
> +}
> +
>   static int ksz8463_reset_switch(struct ksz_device *dev)
>   {
>   	ksz_cfg(dev, KSZ8463_REG_SW_RESET, KSZ8463_GLOBAL_SOFTWARE_RESET, true);
> @@ -2407,21 +2466,50 @@ static int ksz8463_setup(struct dsa_switch *ds)
>   	p = &dev->ports[dev->cpu_port];
>   	p->learning = true;
>   
> +	if (dev->irq > 0) {
> +		ret = ksz8463_girq_setup(dev);
> +		if (ret)
> +			return ret;
> +
> +		ret = ksz8463_ptp_irq_setup(ds);
> +		if (ret)
> +			goto free_girq;
> +	}
> +
>   	ret = ksz_mdio_register(dev);
>   	if (ret < 0) {
>   		dev_err(dev->dev, "failed to register the mdio");
> -		return ret;
> +		goto free_ptp_irq;
>   	}
>   
>   	ret = ksz_dcb_init(dev);
>   	if (ret)
> -		return ret;
> +		goto free_ptp_irq;
>   
>   	/* start switch */
>   	regmap_update_bits(ksz_regmap_8(dev), regs[S_START_CTRL],
>   			   SW_START, SW_START);
>   
>   	return 0;
> +
> +free_ptp_irq:
> +	if (dev->irq > 0)
> +		ksz8463_ptp_irq_free(ds);
> +free_girq:
> +	if (dev->irq > 0)
> +		ksz_irq_free(&dev->girq);
> +
> +	return ret;
> +}
> +
> +static void ksz8463_teardown(struct dsa_switch *ds)
> +{
> +	struct ksz_device *dev = ds->priv;
> +
> +	if (dev->irq > 0) {
> +		ksz8463_ptp_irq_free(ds);
> +		ksz_irq_free(&dev->girq);
> +	}
>   }
>   
>   /**
> @@ -3010,6 +3098,7 @@ const struct dsa_switch_ops ksz8463_switch_ops = {
>   	.get_tag_protocol	= ksz8463_get_tag_protocol,
>   	.connect_tag_protocol   = ksz8463_connect_tag_protocol,
>   	.setup			= ksz8463_setup,
> +	.teardown		= ksz8463_teardown,
>   	.phy_read		= ksz8463_phy_read16,
>   	.phy_write		= ksz8463_phy_write16,
>   	.phylink_get_caps	= ksz8_phylink_get_caps,
> diff --git a/drivers/net/dsa/microchip/ksz_ptp.c b/drivers/net/dsa/microchip/ksz_ptp.c
> index 8b98039320ad..7a74befda9ad 100644
> --- a/drivers/net/dsa/microchip/ksz_ptp.c
> +++ b/drivers/net/dsa/microchip/ksz_ptp.c
> @@ -32,6 +32,15 @@
>   
>   #define KSZ_PTP_INT_START 13
>   
> +/*
> + * PTP interrupt bit is the bit 12 of the 16-bits ISR/IER. But ksz_common.c only
> + * accesses the high-byte of these registers so the PTP interrupt bit becomes 4.
> + */
> +#define KSZ8463_SRC_PTP_INT		4
> +#define KSZ8463_PTP_PORT1_INT_START	12
> +#define KSZ8463_PTP_PORT2_INT_START	14
> +#define KSZ8463_PTP_INT_START		KSZ8463_PTP_PORT1_INT_START
> +
>   static int ksz_ptp_tou_gpio(struct ksz_device *dev)
>   {
>   	int ret;
> @@ -1129,6 +1138,126 @@ static int ksz_ptp_msg_irq_setup(struct ksz_port *port, u8 n)
>   	return ret;
>   }
>   
> +static int ksz8463_ptp_port_irq_setup(struct ksz_irq *ptpirq,
> +				      struct ksz_port *port, int hw_irq)
> +{
> +	u16 ts_reg[] = {KSZ8463_REG_PORT_SYNC_TS, KSZ8463_REG_PORT_DREQ_TS};
> +	static const char * const name[] = {"sync-msg", "delay-msg"};
> +	const struct ksz_dev_ops *ops = port->ksz_dev->dev_ops;
> +	struct ksz_ptp_irq *ptpmsg_irq;
> +	int ret;
> +	int i;
> +
> +	init_completion(&port->tstamp_msg_comp);
> +
> +	for (i = 0; i < 2; i++) {
> +		ptpmsg_irq = &port->ptpmsg_irq[i];
> +		ptpmsg_irq->num = irq_create_mapping(ptpirq->domain,
> +						     hw_irq + i);
> +		if (!ptpmsg_irq->num)
> +			goto release_msg_irq;
> +

Sashiko says : "Does this code return an uninitialized or incorrect 
error code on failure?"

Yes it does, I'll fix it in next iteration


Best regards,
Bastien