netdev - Re: [PATCH v4] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 25 Apr 2023 05:18:44 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>
CC:     <ramon.nordin.rodriguez@...roamp.se>, <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 Andrew,

On 21/04/23 6:09 pm, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> 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,
> 
> Do the current settings not work at all on future silicon? The words
> 'optimal performance' have been used when describing what this magic
> does. Which suggest it should work, but not optimally?
Unfortunately the current settings in the driver works only for Rev.B1 
and will not work for future silicon revisions. The word "optimal 
performance" is used here to tune the performance of Rev.B1 silicon 
only. Please refer the below link for the Application Notes released 
along with this Rev.B1 silicon in our product page which describes the 
configuration settings for product revision B1.

https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8670-1-2-config-60001699.pdf#G1.1102673

If you can't open the above link please find the document attached here 
for your reference. In this please refer the section "CONFIGURATION 
SETTINGS FOR PRODUCT REVISION B1".
> 
> Unless this is going to cause the magic smoke to escape, i say leave
> it as it is until you add explicit support for those revisions.
> 
>>> +       /* 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?
> 
> When interrupt support is added, you should do this. It is normal to
> clear all pending interrupt before enabling interrupts, since you
> don't want to handle stale interrupts.
> 
> The only potential issue here is when somebody has a board which mixes
> multiple different PHYs using a shared interrupt. This PHY is going to
> block all other PHYs, which maybe do have working interrupt
> support. But there is no regression here, it has never worked. So it
> is not something which the first version of the driver needs to
> handle.
Ok thanks for the clarification.

Best Regards,
Parthiban V
> 
>          Andrew

Download attachment "AN-LAN8670-1-2-config-60001699.pdf" of type "application/pdf" (677223 bytes)