Re: [PATCH net-next v1 4/6] r8169: add support for RTL8116af

[Andrew Lunn]

> >Can firmware programming be moved into the PHY driver?
> 

> For our PCIe nics, they are single chips. r8169 driver is for the
> entire integrated chip. Some internal phy shared the same PHY id,
> but the required PHY parameters differ depending on the specific
> MAC/Chip it is integrated with. So I think firmware here can not be
> moved.

Can you tell them part only using PHY registers? It is a bad design if
you cannot. You can populate the .match_phy_device call with a
function which can peak into registers to determine if the hardware
matches the entry in the PHY driver list.

> >> >> -     rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) & 0x3fff;
> >> >> -     if (rg_saw_cnt > 0) {
> >> >> -             u16 sw_cnt_1ms_ini;
> >> >> +     if (tp->phydev) {
> >> >> +             rg_saw_cnt = phy_read_paged(tp->phydev, 0x0c42, 0x13) &
> >0x3fff;
> >> >> +             if (rg_saw_cnt > 0) {
> >> >> +                     u16 sw_cnt_1ms_ini;
> >> >>
> >> >> -             sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> >> >> -             r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> >> >> +                     sw_cnt_1ms_ini = (16000000 / rg_saw_cnt) & 0x0fff;
> >> >> +                     r8168_mac_ocp_modify(tp, 0xd412, 0x0fff, sw_cnt_1ms_ini);
> >> >> +             }
> >> >
> >> >Can this move into the PHY driver?
> >>
> >> It reads a counter from PHY, but the calculated value is programmed into
> >MAC OCP register via r8168_mac_ocp_modify, which accesses r8169 through
> >tp->mmio_addr. So I think this can not be moved.
> >
> >So, big picture, what is this doing? What is rg_saw_cnt?
> 
> UPS mode TX-link-pulse timing calibration. We read rg_saw_cnt from
> the GPHY page 0x0C42, register 0x13. This value is the SAW
> calibration result burned during mass production. If rg_saw_cnt ==
> 0, it means the MP did not burn the calibration result, so we do
> nothing. If calibrated (rg_saw_cnt > 0), we calculate the 1ms timer
> counter: sw_cnt_1ms_ini = (16000000 / rg_saw_cnt). And then write it
> into the MAC OCP register (0xD412). This requires reading a
> hardware-specific value from the PHY to configure a timer register
> in the MAC. So I think it can not be moved too.

> How do you recommend handling this kind of SoC-specific tight
> coupling within the phylink framework? Is it acceptable to retain a
> local phydev pointer in the MAC driver solely for these
> hardware-specific initialization quirks, or is there a preferred
> approach for single-chip devices?

Step one is to not tightly couple hardware like this when you design
it. It just causes problems when you need to make layering violations.

However, the hardware is designed like this, so you have to live with
it. At least for this generation. I hope the next generation is better
designed.

So yes, you can reference the phydev.

	Andrew




> 
> Any guidance for the migration would be greatly appreciated!
> 
> Thanks,
> BRs,
> Javen
> >
> >        Andrew