Re: [PATCH net-next v3 2/7] net: phy: microchip_t1s: update new initial settings for LAN865X Rev.B0

[Parthiban.Veerasooran]

Hi Jakub,

Thanks for your review comments.

On 05/10/24 12:20 am, Jakub Kicinski wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Tue, 1 Oct 2024 18:07:29 +0530 Parthiban Veerasooran wrote:
>> +     cfg_results[0] = FIELD_PREP(GENMASK(15, 10), (9 + offsets[0]) & 0x3F) |
>> +                      FIELD_PREP(GENMASK(15, 4), (14 + offsets[0]) & 0x3F) |
>> +                      0x03;
>> +     cfg_results[1] = FIELD_PREP(GENMASK(15, 10), (40 + offsets[1]) & 0x3F);
> 
> It's really strange to OR together FIELD_PREP()s with overlapping
> fields. What's going on here? 15:10 and 15:4 ranges overlap, then
> there is 0x3 hardcoded, with no fields size definition.
This calculation has been implemented based on the logic provided in the 
configuration application note (AN1760) released with the product. 
Please refer the link [1] below for more info.

As mentioned in the AN1760 document, "it provides guidance on how to 
configure the LAN8650/1 internal PHY for optimal performance in 
10BASE-T1S networks." Unfortunately we don't have any other information 
on those each and every parameters and constants used for the 
calculation. They are all derived by design team to bring up the device 
to the nominal state.

It is also mentioned as, "The following parameters must be calculated 
from the device configuration parameters mentioned above to use for the
configuration of the registers."

uint16 cfgparam1 = (uint16) (((9 + offset1) & 0x3F) << 10) | (uint16) 
(((14 + offset1) & 0x3F) << 4) | 0x03
uint16 cfgparam2 = (uint16) (((40 + offset2) & 0x3F) << 10)

This is the reason why the above logic has been implemented.

> Could you clarify and preferably name as many of the constants
> as possible?
I would like to do that but as I mentioned above there is no info on 
those constants in the application note.
> 
> Also why are you masking the result of the sum with 0x3f?
> Can the result not fit? Is that safe or should we error out?
Hope the above info clarifies this as well.
> 
>> +             ret &= GENMASK(4, 0);
> ?               if (ret & BIT(4))
> 
> GENMASK() is nice but naming the fields would be even nicer..
> What's 3:0, what's 4:4 ?
As per the information provided in the application note, the offset 
value expected range is from -5 to 15. Offsets are stored as signed 
5-bit values in the addresses 0x04 and 0x08. So 0x1F is used to mask the 
5-bit value and if the 4th bit is set then the value from 27 to 31 will 
be considered as -ve value from -5 to -1.

I think adding the above comment in the above code snippet will clarify 
the need. What do you think?

Link:
[1]: 
https://ww1.microchip.com/downloads/aemDocuments/documents/AIS/ApplicationNotes/ApplicationNotes/LAN8650-1-Configuration-Appnote-60001760.pdf

Best regards,
Parthiban V
> --
> pw-bot: cr