[PATCH v3 2/3] net: phy: ca_phy: Add driver for CAxxxx SoCs

[Alex Nemirovsky]

Tom,

> On Sep 29, 2020, at 11:21 AM, Tom Rini <trini at 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 <trini at konsulko.com> wrote:
>>> 
>>> On Wed, Sep 23, 2020 at 05:59:13PM -0700, Alex Nemirovsky wrote:
>>>> From: Abbie Chang <abbie.chang at cortina-access.com>
>>>> 
>>>> Add phy driver support for MACs embedded inside Cortina Access SoCs
>>>> 
>>>> Signed-off-by: Abbie Chang <abbie.chang at cortina-access.com>
>>>> Signed-off-by: Alex Nemirovsky <alex.nemirovsky at cortina-access.com>
>>>> 
>>>> CC: Joe Hershberger <joe.hershberger at ni.com>
>>>> CC: Tom Rini <trini at konsulko.com>
>>>> CC: Aaron Tseng <aaron.tseng at 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