Message-ID: <d0ec7d22-9479-82cb-9e13-cdc247c07311@microchip.com>
Date:   Fri, 21 Apr 2023 07:53:44 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <ramon.nordin.rodriguez@...roamp.se>
CC:     <andrew@...n.ch>, <hkallweit1@...il.com>, <linux@...linux.org.uk>,
        <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <netdev@...r.kernel.org>, <olteanv@...il.com>,
        <Jan.Huber@...rochip.com>
Subject: Re: [PATCH v4] drivers/net/phy: add driver for Microchip LAN867x
 10BASE-T1S PHY

Hi Ramon,

Thanks for the clarification.

Best Regards,
Parthiban V

On 21/04/23 12:59 pm, Ramón Nordin Rodriguez wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Fri, Apr 21, 2023 at 06:44:02AM +0000, Parthiban.Veerasooran@...rochip.com wrote:
>> Hi Ramon,
>>
>> Sorry for the delay. Please find my comments below.
>>
>> Best Regards,
>> Parthiban V
>>
>> On 20/04/23 10:06 pm, Ramón Nordin Rodriguez wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> This patch adds support for the Microchip LAN867x 10BASE-T1S family
>>> (LAN8670/1/2). The driver supports P2MP with PLCA.
>>>
>>> Signed-off-by: Ramón Nordin Rodriguez <ramon.nordin.rodriguez@...roamp.se>
>>> ---
>>>    drivers/net/phy/Kconfig         |   5 ++
>>>    drivers/net/phy/Makefile        |   1 +
>>>    drivers/net/phy/microchip_t1s.c | 138 ++++++++++++++++++++++++++++++++
>>>    3 files changed, 144 insertions(+)
>>>    create mode 100644 drivers/net/phy/microchip_t1s.c
>>>
>>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>>> index 54874555c921..15a0bd95f12c 100644
>>> --- a/drivers/net/phy/Kconfig
>>> +++ b/drivers/net/phy/Kconfig
>>> @@ -235,6 +235,11 @@ config MICREL_PHY
>>>           help
>>>             Supports the KSZ9021, VSC8201, KS8001 PHYs.
>>>
>>> +config MICROCHIP_T1S_PHY
>>> +       tristate "Microchip 10BASE-T1S Ethernet PHY"
>>> +       help
>>> +         Currently supports the LAN8670, LAN8671, LAN8672
>>> +
>>>    config MICROCHIP_PHY
>>>           tristate "Microchip PHYs"
>>>           help
>>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>>> index b5138066ba04..64f649f2f62f 100644
>>> --- a/drivers/net/phy/Makefile
>>> +++ b/drivers/net/phy/Makefile
>>> @@ -74,6 +74,7 @@ obj-$(CONFIG_MICREL_KS8995MA) += spi_ks8995.o
>>>    obj-$(CONFIG_MICREL_PHY)       += micrel.o
>>>    obj-$(CONFIG_MICROCHIP_PHY)    += microchip.o
>>>    obj-$(CONFIG_MICROCHIP_T1_PHY) += microchip_t1.o
>>> +obj-$(CONFIG_MICROCHIP_T1S_PHY) += microchip_t1s.o
>>>    obj-$(CONFIG_MICROSEMI_PHY)    += mscc/
>>>    obj-$(CONFIG_MOTORCOMM_PHY)    += motorcomm.o
>>>    obj-$(CONFIG_NATIONAL_PHY)     += national.o
>>> diff --git a/drivers/net/phy/microchip_t1s.c b/drivers/net/phy/microchip_t1s.c
>>> new file mode 100644
>>> index 000000000000..b7fd5141a117
>>> --- /dev/null
>>> +++ b/drivers/net/phy/microchip_t1s.c
>>> @@ -0,0 +1,138 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Driver for Microchip 10BASE-T1S LAN867X PHY
>>> + *
>>> + * Support: Microchip Phys:
>>> + *  lan8670, lan8671, lan8672
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/phy.h>
>>> +
>>> +#define PHY_ID_LAN867X 0x0007C160
>> Do you think it makes sense to probe the driver based on the Revision
>> number as well? because we have two more revisions in the pipeline and
>> each revision has different settings for the initial configuration. If
>> you agree with this, as per the current datasheet this PHY revision is 2
>> means Rev.B1. If you like, I would suggest to use the PHY_ID define as
>> below,
>> #define PHY_ID_LAN867X_REVB1 0x0007C162
> 
> Personally I'd save that for a later patch that also implements the
> initial config for the different revisions.
> Can't speak for what's common practice in netdev though.
> 
>>> +
>>> +#define LAN867X_REG_IRQ_1_CTL 0x001C
>>> +#define LAN867X_REG_IRQ_2_CTL 0x001D
>>> +
>>> +/* The arrays below are pulled from the following table from AN1699
>>> + * Access MMD Address Value Mask
>>> + * RMW 0x1F 0x00D0 0x0002 0x0E03
>>> + * RMW 0x1F 0x00D1 0x0000 0x0300
>>> + * RMW 0x1F 0x0084 0x3380 0xFFC0
>>> + * RMW 0x1F 0x0085 0x0006 0x000F
>>> + * RMW 0x1F 0x008A 0xC000 0xF800
>>> + * RMW 0x1F 0x0087 0x801C 0x801C
>>> + * RMW 0x1F 0x0088 0x033F 0x1FFF
>>> + * W   0x1F 0x008B 0x0404 ------
>>> + * RMW 0x1F 0x0080 0x0600 0x0600
>>> + * RMW 0x1F 0x00F1 0x2400 0x7F00
>>> + * RMW 0x1F 0x0096 0x2000 0x2000
>>> + * W   0x1F 0x0099 0x7F80 ------
>>> + */
>>> +
>>> +static const int lan867x_fixup_registers[12] = {
>>> +       0x00D0, 0x00D1, 0x0084, 0x0085,
>>> +       0x008A, 0x0087, 0x0088, 0x008B,
>>> +       0x0080, 0x00F1, 0x0096, 0x0099,
>>> +};
>>> +
>>> +static const int lan867x_fixup_values[12] = {
>>> +       0x0002, 0x0000, 0x3380, 0x0006,
>>> +       0xC000, 0x801C, 0x033F, 0x0404,
>>> +       0x0600, 0x2400, 0x2000, 0x7F80,
>>> +};
>>> +
>>> +static const int lan867x_fixup_masks[12] = {
>>> +       0x0E03, 0x0300, 0xFFC0, 0x000F,
>>> +       0xF800, 0x801C, 0x1FFF, 0xFFFF,
>>> +       0x0600, 0x7F00, 0x2000, 0xFFFF,
>>> +};
>>> +
>>> +static int lan867x_config_init(struct phy_device *phydev)
>>> +{
>>> +       /* HW quirk: Microchip states in the application note (AN1699) for the phy
>>> +        * that a set of read-modify-write (rmw) operations has to be performed
>>> +        * on a set of seemingly magic registers.
>>> +        * The result of these operations is just described as 'optimal performance'
>>> +        * Microchip gives no explanation as to what these mmd regs do,
>>> +        * in fact they are marked as reserved in the datasheet.
>>> +        * It is unclear if phy_modify_mmd would be safe to use or if a write
>>> +        * really has to happen to each register.
>>> +        * In order to exacly conform to what is stated in the AN phy_write_mmd is
>>> +        * used, which might then write the same value back as read + modified.
>>> +        */
>>> +
>>> +       int reg_value;
>>> +       int err;
>>> +       int reg;
>>> +
>>> +       /* Read-Modified Write Pseudocode (from AN1699)
>>> +        * current_val = read_register(mmd, addr) // Read current register value
>>> +        * new_val = current_val AND (NOT mask) // Clear bit fields to be written
>>> +        * new_val = new_val OR value // Set bits
>>> +        * write_register(mmd, addr, new_val) // Write back updated register value
>>> +        */
>>> +       for (int i = 0; i < ARRAY_SIZE(lan867x_fixup_registers); i++) {
>>> +               reg = lan867x_fixup_registers[i];
>>> +               reg_value = phy_read_mmd(phydev, MDIO_MMD_VEND2, reg);
>>> +               reg_value &= ~lan867x_fixup_masks[i];
>>> +               reg_value |= lan867x_fixup_values[i];
>>> +               err = phy_write_mmd(phydev, MDIO_MMD_VEND2, reg, reg_value);
>>> +               if (err != 0)
>>> +                       return err;
>>> +       }
>>> +
>>> +       /* None of the interrupts in the lan867x phy seem relevant.
>>> +        * Other phys inspect the link status and call phy_trigger_machine
>>> +        * in the interrupt handler.
>>> +        * This phy does not support link status, and thus has no interrupt
>>> +        * for it either.
>>> +        * So we'll just disable all interrupts on the chip.
>>> +        */
>> I could see the below in the datasheet section 4.7.
>>
>> When the device is in a reset state, the IRQ_N interrupt pin is
>> high-impedance and will be pulled high through an
>> external pull-up resistor. Once all device reset sources are deasserted,
>> the device will begin its internal initialization.
>> The device will assert the Reset Complete (RESETC) bit in the Status 2
>> (STS2) register to indicate that it has
>> completed its internal initialization and is ready for configuration. As
>> the Reset Complete status is non-maskable, the
>> IRQ_N pin will always be asserted and driven low following a device reset
>>
>> Do you think it makes sense to clear/acknowledge the "reset_done"
>> interrupt by reading the Status 2 register in the interrupt routine?
>>
> 
> I don't think it's necessary when not populating the handle_interrupt
> member in the phy_driver struct.
> If there were any other interrupts we could act upon I'd say
> differently.
> 
> IMHO the interruts on the chip are interesting, but it's elusive to me
> how to handle any of them gracefully in kernelspace. Say the 'unexpected beacon'
> interrupt for example, meaning two or more nodes have id 0 (i.e. PLCA
> coordinator), any of them can detect the collision, but which one
> yields. The desired result here would be different in different
> products/applications.
> Then this is probably just useful for debugging and it's easy to spot
> with an oscilloscope or signal analyser, so probably overkill to support
> debugging of the network from the driver.
> 
>>> +       err = phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_1_CTL, 0xFFFF);
>>> +       if (err != 0)
>>> +               return err;
>>> +       return phy_write_mmd(phydev, MDIO_MMD_VEND2, LAN867X_REG_IRQ_2_CTL, 0xFFFF);
>>> +}
>>> +
>>> +static int lan867x_read_status(struct phy_device *phydev)
>>> +{
>>> +       /* The phy has some limitations, namely:
>>> +        *  - always reports link up
>>> +        *  - only supports 10MBit half duplex
>>> +        *  - does not support auto negotiate
>>> +        */
>>> +       phydev->link = 1;
>>> +       phydev->duplex = DUPLEX_HALF;
>>> +       phydev->speed = SPEED_10;
>>> +       phydev->autoneg = AUTONEG_DISABLE;
>>> +
>>> +       return 0;
>>> +}
>>> +
>>> +static struct phy_driver lan867x_driver[] = {
>>> +       {
>>> +               PHY_ID_MATCH_MODEL(PHY_ID_LAN867X),
>> If you agree with the above PHY_ID comment then this can be below,
>> PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1),
> 
> As stated earlier I think this can be added in a later patch with more
> functionality tied together.
> 
>>> +               .name               = "LAN867X",
>>> +               .features           = PHY_BASIC_T1S_P2MP_FEATURES,
>>> +               .config_init        = lan867x_config_init,
>>> +               .read_status        = lan867x_read_status,
>>> +               .get_plca_cfg       = genphy_c45_plca_get_cfg,
>>> +               .set_plca_cfg       = genphy_c45_plca_set_cfg,
>>> +               .get_plca_status    = genphy_c45_plca_get_status,
>>> +       }
>>> +};
>>> +
>>> +module_phy_driver(lan867x_driver);
>>> +
>>> +static struct mdio_device_id __maybe_unused tbl[] = {
>>> +       { PHY_ID_MATCH_MODEL(PHY_ID_LAN867X) },
>> If you agree with the above PHY_ID comment then this can be below,
>> PHY_ID_MATCH_EXACT(PHY_ID_LAN867X_REVB1)
> 
> See the comment above
> 
> Let me know if you disagree with my reasoning
> R
> 
>>> +       { }
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(mdio, tbl);
>>> +
>>> +MODULE_DESCRIPTION("Microchip 10BASE-T1S lan867x Phy driver");
>>> +MODULE_AUTHOR("Ramón Nordin Rodriguez");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 2.39.2
>>>
>>> Changes:
>>>       v2:
>>> - Removed mentioning of not supporting auto-negotiation from commit
>>>     message
>>> - Renamed file drivers/net/phy/lan867x.c ->
>>>     drivers/net/phy/microchip_t1s.c
>>> - Renamed Kconfig option to reflect implementation filename (from
>>>     LAN867X_PHY to MICROCHIP_T1S_PHY)
>>> - Moved entry in drivers/net/phy/KConfig to correct sort order
>>> - Moved entry in drivers/net/phy/Makefile to correct sort order
>>> - Moved variable declarations to conform to reverse christmas tree order
>>>     (in func lan867x_config_init)
>>> - Moved register write to disable chip interrupts to func lan867x_config_init, when omitting the irq disable all togheter I got null pointer dereference, see the call trace below:
>>>
>>>       Call Trace:
>>>        <TASK>
>>>        phy_interrupt+0xa8/0xf0 [libphy]
>>>        irq_thread_fn+0x1c/0x60
>>>        irq_thread+0xf7/0x1c0
>>>        ? __pfx_irq_thread_dtor+0x10/0x10
>>>        ? __pfx_irq_thread+0x10/0x10
>>>        kthread+0xe6/0x110
>>>        ? __pfx_kthread+0x10/0x10
>>>        ret_from_fork+0x29/0x50
>>>        </TASK>
>>>
>>> - Removed func lan867x_config_interrupt and removed the member
>>>     .config_intr from the phy_driver struct
>>>
>>>       v3:
>>> - Indentation level in drivers/net/phy/Kconfig
>>> - Moved const arrays into global scope and made them static in order to have
>>>     them placed in the .rodata section
>>> - Renamed array variables, since they are no longer as closely scoped as
>>>     earlier
>>> - Added comment about why phy_write_mmd is used over phy_modify_mmd
>>>     (this should have been addressed in the V2 change since it was brought
>>>     up in the V1 review)
>>> - Return result of last call instead of saving it in a var and then
>>>     returning the var (in lan867x_config_init)
>>>
>>>       v4:
>>> - Moved history out of commit message
>>>
>>> Testing:
>>> This has been tested with ethtool --set/get-plca-cfg and verified on an
>>> oscilloscope where it was observed that:
>>> - The PLCA beacon was enabled/disabled when setting the node-id to 0/not
>>>     0
>>> - The PLCA beacon is transmitted with the expected frequency when
>>>     changing max nodes
>>> - Two devices using the evaluation board EVB-LAN8670-USB could ping each
>>>     other
>>

Powered by blists - more mailing lists