IronPort-SDR: xj9ZNRJgXumkXCLBCBXSwEdWc7eKkoQlSk3FPLw26j7+23PaiwMXz4hIyteRAYZn+us1tPsTi3
 tA8JNEsA4iN71fyowmRw7HVcBltqy2LY0uNVnpsgxZM+9wfVe69xH2DF34aJI+7EHvQLI/h8/V
 yEaFUsHToOPERRmcsS1PBh+HVdAEeego2u+B4xcUNZiVVuNDrzZRWsQkrrVbxpEvmAONpw4PGN
 XcnkgnRTQtExZr8pF729W6IMEXI1x1haaM4sV3zqHJMDn/tHGDOmUNM0KxbPJACZE/hBD518BU
 Ld/V8c646J6eQ+7F3eYJid8X
Date:   Mon, 29 Nov 2021 09:29:58 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Heiner Kallweit <hkallweit1@gmail.com>
CC:     <andrew@lunn.ch>, <linux@armlinux.org.uk>, <davem@davemloft.net>,
        <kuba@kernel.org>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next] net: phy: micrel: Add config_init for LAN8814
Message-ID: <20211129082958.ap6xtsb6jad3os4x@soft-dev3-1.localhost>
References: <20211126103833.3609945-1-horatiu.vultur@microchip.com>
 <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <402780af-9d12-45dd-e435-e7279f1b9263@gmail.com>
Precedence: bulk

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.

> 
> > +     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,
> >
> 

-- 
/Horatiu