Messages in this thread | | | Subject | Re: [PATCH net-next v2] net: phy: dp83867: Add speed optimization feature | From | Heiner Kallweit <> | Date | Fri, 7 Feb 2020 00:04:59 +0100 |
| |
On 06.02.2020 23:36, Dan Murphy wrote: > Heiner > > On 2/6/20 4:28 PM, Heiner Kallweit wrote: >> On 06.02.2020 23:13, Dan Murphy wrote: >>> Heiner >>> >>> On 2/5/20 3:16 PM, Heiner Kallweit wrote: >>>> On 04.02.2020 19:13, Dan Murphy wrote: >>>>> Set the speed optimization bit on the DP83867 PHY. >>>>> This feature can also be strapped on the 64 pin PHY devices >>>>> but the 48 pin devices do not have the strap pin available to enable >>>>> this feature in the hardware. PHY team suggests to have this bit set. >>>>> >>>>> With this bit set the PHY will auto negotiate and report the link >>>>> parameters in the PHYSTS register. This register provides a single >>>>> location within the register set for quick access to commonly accessed >>>>> information. >>>>> >>>>> In this case when auto negotiation is on the PHY core reads the bits >>>>> that have been configured or if auto negotiation is off the PHY core >>>>> reads the BMCR register and sets the phydev parameters accordingly. >>>>> >>>>> This Giga bit PHY can throttle the speed to 100Mbps or 10Mbps to accomodate a >>>>> 4-wire cable. If this should occur the PHYSTS register contains the >>>>> current negotiated speed and duplex mode. >>>>> >>>>> In overriding the genphy_read_status the dp83867_read_status will do a >>>>> genphy_read_status to setup the LP and pause bits. And then the PHYSTS >>>>> register is read and the phydev speed and duplex mode settings are >>>>> updated. >>>>> >>>>> Signed-off-by: Dan Murphy <dmurphy@ti.com> >>>>> --- >>>>> v2 - Updated read status to call genphy_read_status first, added link_change >>>>> callback to notify of speed change and use phy_set_bits - https://lore.kernel.org/patchwork/patch/1188348/ >>>>> >>>> As stated in the first review, it would be appreciated if you implement >>>> also the downshift tunable. This could be a separate patch in this series. >>>> Most of the implementation would be boilerplate code. >>> >>> I looked at this today and there are no registers that allow tuning the downshift attempts. There is only a RO register that tells you how many attempts it took to achieve a link. So at the very least we could put in the get_tunable but there will be no set. >>> >> The get operation for the downshift tunable should return after how many failed >> attempts the PHY starts a downshift. This doesn't match with your description of >> this register, so yes: Implementing the tunable for this PHY doesn't make sense. > True. This register is only going to return 1,2,4 and 8. And it is defaulted to 4 attempts. >> >> However this register may be useful in the link_change_notify() callback to >> figure out whether a downshift happened, to trigger the info message you had in >> your first version. > > Thats a good idea but.. The register is defaulted to always report 4 attempts were made. It never reports 0 attempts so we would never know the truth behind the reporting. Kinda like matching the speeds. >
I just had a brief look at the datasheet here: http://www.ti.com/lit/ds/symlink/dp83867ir.pdf It says: The number of failed link attempts before falling back to 100-M operation is configurable. (p.45) Description of SPEED_OPT_ATTEMPT_CNT in CFG2 says "select attempt count", so it sounds like it's an RW register. It's marked as RO however, maybe it's a typo in the datasheet. Did you test whether register is writable? Last but not least this register is exactly what's needed for the downshift tunable.
Checking whether a downshift occurred should be possible by reading SPEED_OPT_EVENT_INT in ISR. In interrupt mode however this may require a custom interrupt handler (implementation of handle_interrupt callback).
Alternatively you could check SPEED_OPT_STATUS in PHYSTS. It says "valid only during aneg" but that sounds a little bit weird. Would need to be tested whether bit remains set after downshifted aneg is finished.
> Dan > Heiner
| |