Tom, > On Sep 29, 2020, at 11:21 AM, Tom Rini <tr...@konsulko.com> wrote: > > On Tue, Sep 29, 2020 at 06:17:26PM +0000, Alex Nemirovsky wrote: > >> Tom, >> Yes the intent is to allow overwrite the functionality elsewhere. I agree >> that the name is too generic and should be more unique to avoid name space >> conflicts. > > Any sort of board specific tweaking should be done via device tree > properties, I strongly suspect.
You maybe right. That might be a design possibility. I personally don’t know enough about this to comment either way. I’ll pass this on to the team for consideration and review of the overall approach. > >> >> Abbie, >> Please Review and update per Tom’s request. >>>> On Sep 29, 2020, at 10:57 AM, Tom Rini <tr...@konsulko.com> wrote: >>> >>> On Wed, Sep 23, 2020 at 05:59:13PM -0700, Alex Nemirovsky wrote: >>>> From: Abbie Chang <abbie.ch...@cortina-access.com> >>>> >>>> Add phy driver support for MACs embedded inside Cortina Access SoCs >>>> >>>> Signed-off-by: Abbie Chang <abbie.ch...@cortina-access.com> >>>> Signed-off-by: Alex Nemirovsky <alex.nemirov...@cortina-access.com> >>>> >>>> CC: Joe Hershberger <joe.hershber...@ni.com> >>>> CC: Tom Rini <tr...@konsulko.com> >>>> CC: Aaron Tseng <aaron.ts...@cortina-access.com> >>>> >>>> Moved out PHY specific code out of Cortina NI Ethernet driver >>>> and into a Cortina Access PHY interface driver >>> [snip] >>>> +__weak void __internal_phy_init(struct phy_device *phydev, int reset_phy) >>>> +{ >>>> + u8 phy_addr; >>>> + u16 data; >>>> + >>>> + /* should initialize 4 GPHYs at once */ >>>> + for (phy_addr = 4; phy_addr > 0; phy_addr--) { >>>> + phydev->addr = phy_addr; >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0BC6); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 16, 0x0053); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 18, 0x4003); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 22, 0x7e01); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0A42); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0A40); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 0, 0x1140); >>>> + } >>>> + >>>> + /* workaround to fix GPHY fail */ >>>> + for (phy_addr = 1; phy_addr < 5; phy_addr++) { >>>> + /* Clear clock fail interrupt */ >>>> + phydev->addr = phy_addr; >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0xB90); >>>> + data = phy_read(phydev, MDIO_DEVAD_NONE, 19); >>>> + if (data == 0x10) { >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0xB90); >>>> + data = phy_read(phydev, MDIO_DEVAD_NONE, 19); >>>> + printf("%s: read again.\n", __func__); >>>> + } >>>> + >>>> + printf("%s: phy_addr=%d, read register 19, value=0x%x\n", >>>> + __func__, phy_addr, data); >>>> + } >>>> +} >>>> + >>>> +__weak void __external_phy_init(struct phy_device *phydev, int reset_phy) >>>> +{ >>>> + u16 val; >>>> + >>>> + /* Disable response PHYAD=0 function of RTL8211 series PHY */ >>>> + /* REG31 write 0x0007, set to extension page */ >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0007); >>>> + >>>> + /* REG30 write 0x002C, set to extension page 44 */ >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 30, 0x002C); >>>> + >>>> + /* >>>> + * REG27 write bit[2] = 0 disable response PHYAD = 0 function. >>>> + * we should read REG27 and clear bit[2], and write back >>>> + */ >>>> + val = phy_read(phydev, MDIO_DEVAD_NONE, 27); >>>> + val &= ~(1 << 2); >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 27, val); >>>> + >>>> + /* REG31 write 0X0000, back to page0 */ >>>> + phy_write(phydev, MDIO_DEVAD_NONE, 31, 0x0000); >>>> +} >>>> + >>>> +static int rtl8211_external_config(struct phy_device *phydev) >>>> +{ >>>> + __external_phy_init(phydev, 0); >>>> + printf("%s: initialize RTL8211 external done.\n", __func__); >>>> + return 0; >>>> +} >>>> + >>>> +static int rtl8211_internal_config(struct phy_device *phydev) >>>> +{ >>>> + struct phy_device phydev_init; >>>> + >>>> + memcpy(&phydev_init, phydev, sizeof(struct phy_device)); >>>> + /* should initialize 4 GPHYs at once */ >>>> + __internal_phy_init(&phydev_init, 0); >>>> + printf("%s: initialize RTL8211 internal done.\n", __func__); >>>> + return 0; >>>> +} >>> >>> Why do we have two weak functions (which have very generic names and >>> could get overwritten by accident) and not just inlining them to the >>> functions? >>> >>> -- >>> Tom > > -- > Tom