Message-ID: <a85b5463-7742-af34-2f51-8a828d6fe866@gmail.com>
Date:   Mon, 29 Nov 2021 18:07:03 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.2
Content-Language: en-US
To:     Horatiu Vultur <horatiu.vultur@microchip.com>
Cc:     andrew@lunn.ch, linux@armlinux.org.uk, davem@davemloft.net,
        kuba@kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20211126103833.3609945-1-horatiu.vultur@microchip.com>
 <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
 <20211129082958.ap6xtsb6jad3os4x@soft-dev3-1.localhost>
From:   Heiner Kallweit <hkallweit1@gmail.com>
Subject: Re: [PATCH net-next] net: phy: micrel: Add config_init for LAN8814
In-Reply-To: <20211129082958.ap6xtsb6jad3os4x@soft-dev3-1.localhost>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 29.11.2021 09:29, Horatiu Vultur wrote:
> The 11/26/2021 12:57, Heiner Kallweit wrote:
> 
> Hi Heiner,
> 
>>
>> On 26.11.2021 11:38, Horatiu Vultur wrote:
>>>
>>> +static int lan8814_config_init(struct phy_device *phydev)
>>> +{
>>> +     int val;
>>> +
>>> +     /* Reset the PHY */
>>> +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
>>> +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
>>> +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
>>> +
>>> +     /* Disable ANEG with QSGMII PCS Host side */
>>> +     val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
>>> +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
>>> +     lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG, val);
>>> +
>>> +     /* MDI-X setting for swap A,B transmit */
>>> +     val = lanphy_read_page_reg(phydev, 2, LAN8814_ALIGN_SWAP);
>>> +     val &= ~LAN8814_ALIGN_TX_A_B_SWAP_MASK;
>>> +     val |= LAN8814_ALIGN_TX_A_B_SWAP;
>>> +     lanphy_write_page_reg(phydev, 2, LAN8814_ALIGN_SWAP, val);
>>> +
>>
>> Not directly related to just this patch:
>> Did you consider implementing the read_page and write_page PHY driver
>> callbacks? Then you could use phylib functions like phy_modify_paged et al
>> and you wouldn't have to open-code the paged register operations.
>>
>> I think write_page would just be
>> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, page);
>> phy_write(phydev, LAN_EXT_PAGE_ACCESS_ADDRESS_DATA, addr);
>> phy_write(phydev, LAN_EXT_PAGE_ACCESS_CONTROL, (page | LAN_EXT_PAGE_ACCESS_CTRL_EP_FUNC));
>>
>> and read_page
>> phy_read(phydev, LAN_EXT_PAGE_ACCESS_CONTROL);
> 
> Thanks for the suggestion, but unfortunately it would not work.
> The reason is that in the callback 'write_page' I don't actually get
> also the address in the page that is needed to be read/write.
> 
> If this issue would be fixed, then there is another problem.
> To read/write the data in the extended page is required to access the
> register LAN_EXT_PAGE_ACCESS_ADDRESS_DATA. But that would not happen
> when using the phy_read_paged, it would read actually the register in
> page 0.
> 
> If I have missed something, please let me know.
> 

Right, after reading the sequence in lanphy_read_page_reg() more
carefully I agree. This PHY re-uses the more complex MMD access
mechanism for paged access.

>>
>>> +     return 0;
>>> +}
>>> +
>>>  static int lan8804_config_init(struct phy_device *phydev)
>>>  {
>>>       int val;
>>> @@ -1793,6 +1824,7 @@ static struct phy_driver ksphy_driver[] = {
>>>       .phy_id         = PHY_ID_LAN8814,
>>>       .phy_id_mask    = MICREL_PHY_ID_MASK,
>>>       .name           = "Microchip INDY Gigabit Quad PHY",
>>> +     .config_init    = lan8814_config_init,
>>>       .driver_data    = &ksz9021_type,
>>>       .probe          = kszphy_probe,
>>>       .soft_reset     = genphy_soft_reset,
>>>
>>
>