Re: [PATCH net-next v6 1/9] net: dsa: microchip: Add support for KSZ8463 global irq

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

Hi,

On 3/6/26 2:10 AM, Tristram.Ha@microchip.com wrote:
>> On 3/5/26 1:51 PM, Vladimir Oltean wrote:
>>> On Thu, Mar 05, 2026 at 01:39:29PM +0100, Bastien Curutchet wrote:
>>>> Hi Vladimir,
>>>>
>>>> On 3/5/26 10:56 AM, Vladimir Oltean wrote:
>>>>> On Wed, Mar 04, 2026 at 11:18:52AM +0100, Bastien Curutchet (Schneider
>> Electric) wrote:
>>>>>> @@ -2890,14 +2899,18 @@ static irqreturn_t ksz_irq_thread_fn(int irq,
>> void *dev_id)
>>>>>>             unsigned int nhandled = 0;
>>>>>>             struct ksz_device *dev;
>>>>>>             unsigned int sub_irq;
>>>>>> -  u8 data;
>>>>>> +  u16 data;
>>>>>>             int ret;
>>>>>>             u8 n;
>>>>>>             dev = kirq->dev;
>>>>>> -  /* Read interrupt status register */
>>>>>> -  ret = ksz_read8(dev, kirq->reg_status, &data);
>>>>>> +  /*
>>>>>> +   * Most of the KSZ switches have a 8-bits long interrupt status
>>>>>> +   * register, but the KSZ8463 has a 16-bits long one. The overread here
>>>>>> +   * is safe because we only iterate over kirq->nirqs in the below loop.
>>>>>
>>>>> FWIW, this isn't the only thing making an overread "safe".
>>>>> If the adjacent register also has "clear on read" semantics, that's not
>>>>> good.
>>>>>
>>>>> I can't tell whether that's the case here, though. There are just too
>>>>> many hardware variations to check for. I just wanted to point out that
>>>>> the reasoning is incomplete.
>>>>>
>>>> You're right, I hadn't thought about the 'clear on read' case.
>>>>
>>>> I'll use ksz_read16() only for the KSZ8463 to be 100% sure it won't break
>>>> anything for other switches.
>>>
>>> I'm still on the fence on whether to say this or not, because I don't
>>> really want to get so involved in internal driver bookkeeping, but...
>>> this driver is just becoming a hell to review, even if I want to
>>> concentrate exclusively on correct API use, like I try to do.
>>>> Linus Walleij has added a new ks8995 driver, which has some overlap with
>>> the common ksz driver for the KSZ8 family. Now he wants to remove the
>>> overlapping device support:
>>> https://lore.kernel.org/netdev/20260219-ks8995-fixups-v3-0-
>> a7fc63fe1916@kernel.org/
>>>
>>> Maybe we should go the other way around, migrate KSZ8 support to the
>>> ks8995 driver instead? The common ksz driver is becoming just extremely
>>> convoluted to handle all hardware variations. Would it help in any way
>>> to maintain cleaner code paths, what do you think?
>>
>> I agree, this driver is extremely convoluted because of all the
>> different hardware it handles. It wasn't easy to fit PTP support for the
>> KSZ8463 into it. And I encountered the same kind of difficulties when
>> adding periodic output support (I have another series ready for this
>> once this PTP support will be merged).
>>
>> Regarding migrating the KSZ8 support into the ksz8995, I think we would
>> quickly hit the same pain points than in ksz_common. Even inside the
>> KSZ8 family we can find a quite big amount of differences between the
>> switches. For instance, both KSZ8463 and KSZ8563 support PTP, they share
>> lots of common registers but their interrupt scheme is very different.
>>
>> I've added Tristram in Cc:, who works at Microchip. Maybe, Tristram, you
>> have some insights about which switches could share code if we decide to
>> split the big ksz_common into several drivers ?
>   
> Although KSZ8463 uses 16-bit register for its interrupt operation, the
> bits that are necessary for actual operation are all in the high byte, so
> it is possible to simulate using 8-bit register for interrupt operation
> just like the other switches.
> 

I hadn't though about using only the high byte, I like this idea. I'll 
implement it in the next iteration with a comment explaining why it 
works for the KSZ8463.

> Note also the previous interrupt code only works for KSZ9477 and LAN937X
> families of switches, which have interrupts for each port.  The older
> switches like KSZ8863, KSZ8895, and KSZ8795 all have only one global
> interrupt and need change to work in this framework.

Indeed, that's what patches 1 to 4 are here for.

> 
> These switches have link change interrupt bits for external ports, so it
> is easy to simulate them as port interrupts.  But KSZ8463 only has 1 bit
> for link, so it is necessary to add some special code to return a value of
> 3 to indicate both ports have link change.
> 

This series only use interrupt for PTP means. TBH I haven't looked at 
the link change interrupt mechanism.

> KSZ8463 is based from KSZ8863 so it should have about the same 8-bit
> register definitions as KSZ8863.  But when it was designed it had a
> companion chip that operates as a 16-bit network controller.  Because of
> that all register definitions are described in 16-bit.
> 
> I need to try your patches to see how it works regarding to the PTP
> interrupts.
> 
> Regarding to the old ks8995 driver I think it was a simple SPI driver to
> start the KSZ8995 switch, which is an older version of KSZ8895.  It was
> used to locate under the PHY drivers.  I did not know it was upgraded to
> become a full DSA driver and is now trying to expand to the other
> switches.

Ok, thanks for these information

Best regards,
Bastien