netdev - Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for Microchip LAN865x Rev.B0 PHYs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <25927897-ad2f-c933-795d-cc9b80e6a0c6@microchip.com>
Date:   Thu, 27 Apr 2023 12:28:43 +0000
From:   <Parthiban.Veerasooran@...rochip.com>
To:     <andrew@...n.ch>, <ramon.nordin.rodriguez@...roamp.se>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <Jan.Huber@...rochip.com>, <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [PATCH net-next 2/2] net: phy: microchip_t1s: add support for
 Microchip LAN865x Rev.B0 PHYs

Hi Andrew,

Thanks for reviewing the patches. Please see my comments below.

Best Regards,
Parthiban V
On 27/04/23 3:13 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>> +/* indirect read pseudocode from AN1760
>>> + * write_register(0x4, 0x00D8, addr)
>>> + * write_register(0x4, 0x00DA, 0x2)
>>> + * return (int8)(read_register(0x4, 0x00D9))
>>> + */
>>
>> I suggest this comment block is slightly changed to
>>
>> /* Pulled from AN1760 describing 'indirect read'
>>   *
>>   * write_register(0x4, 0x00D8, addr)
>>   * write_register(0x4, 0x00DA, 0x2)
>>   * return (int8)(read_register(0x4, 0x00D9))
>>   *
>>   * 0x4 refers to memory map selector 4, which maps to MDIO_MMD_VEND2
>>   */
>>
>>> +static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
>>> +{
>>> +   int ret;
>>> +
>>> +   ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xD8, addr);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>> +   ret = phy_write_mmd(phydev, MDIO_MMD_VEND2, 0xDA, 0x0002);
>>> +   if (ret)
>>> +           return ret;
>>> +
>>> +   ret = phy_read_mmd(phydev, MDIO_MMD_VEND2, 0xD9);
>>> +   if (ret < 0)
>>> +           return ret;
>>> +
>>> +   return ret;
>>> +}
> 
> It is unclear to me how 0x4 maps to MDIO_MMD_VEND2, which is 31.
> 
> Why not just describe it in terms of MMD read/write?
This is an internal 10BASE-T1S PHY of LAN865x MAC-PHY. LAN865x has SPI 
to interface with Host. The integrated PHY vendor specific registers are 
located within Memory Map Selector 4 (MMS4). This MMS4 will be used as a 
base if the MAC driver wants to access the integrated PHY vendor 
specific registers directly through SPI without PHY driver.

Is it clear? or do you need more information?
> 
>> The func itself might get a bit more readable by naming the magic regs
>> and value, below is a suggestion.
>>
>> static int lan865x_revb0_indirect_read(struct phy_device *phydev, u16 addr)
>> {
>>        int ret;
>>        static const u16 fixup_w0_reg = 0x00D8;
>>        static const u16 fixup_r0_reg = 0x00D9;
>>        static const u16 fixup_w1_val = 0x0002;
>>        static const u16 fixup_w1_reg = 0x00DA;
> 
> #defines would be normal, not variables.
> 
> And i guess 0x0002 is actually BIT(1). And it probably means something
> like START? 0xD8 is some sort of address register, so i would put ADDR
> in the name. 0xD9 appears to be a control register, so CTRL. And 0xDA
> is a data register? So these could be give more descriptive names,
> just by my reverse engineering it. With the actual data sheet, i'm
> expect somebody could do better.
> 
>>> +static int lan865x_revb0_config_init(struct phy_device *phydev)
>>> +{
>>> +   int addr;
>>> +   int value;
>>> +   int ret;
>>> +
>>> +   /* As per AN1760, the below configuration applies only to the LAN8650/1
>>> +    * hardware revision Rev.B0.
>>> +    */
>>
>> I think this is implied by having it the device specific init func, you
>> can probably drop this comment.
> 
> A reference to AN1760 somewhere in the driver would be good, to help
Do you mean to give a web link to that document like below?

https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ProductDocuments/SupportingCollateral/AN-LAN8650-1-Configuration-60001760.pdf
> people understand why this magic is needed. Does AN1760 actually
> explain the magic, or just say you need to do this to make it work, Trust Us(tm).
Unfortunately it doesn't explain what those init settings but we need to 
do this to work as expected. As you said, Trust Us.
> 
>              Andrew