On 09.02.2021 17:40, Michael Walle wrote:
> Registers >= 16 are paged. Be sure to set the page. It seems this was
> working for now, because the default is correct for the registers used
> in the driver at the moment. But this will also assume, nobody will
> change the page select register before linux is started. The page select
> register is _not_ reset with a soft reset of the PHY.
>
> Add read_page()/write_page() support for the IP101G and use it
> accordingly.
>
> Signed-off-by: Michael Walle <mich...@walle.cc>
> ---
> drivers/net/phy/icplus.c | 50 +++++++++++++++++++++++++++++++---------
> 1 file changed, 39 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
> index a6e1c7611f15..858b9326a72d 100644
> --- a/drivers/net/phy/icplus.c
> +++ b/drivers/net/phy/icplus.c
> @@ -49,6 +49,8 @@ MODULE_LICENSE("GPL");
> #define IP101G_DIGITAL_IO_SPEC_CTRL 0x1d
> #define IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32 BIT(2)
>
> +#define IP101G_DEFAULT_PAGE 16
> +
> #define IP175C_PHY_ID 0x02430d80
> #define IP1001_PHY_ID 0x02430d90
> #define IP101A_PHY_ID 0x02430c54
> @@ -250,23 +252,25 @@ static int ip101a_g_probe(struct phy_device *phydev)
> static int ip101a_g_config_init(struct phy_device *phydev)
> {
> struct ip101a_g_phy_priv *priv = phydev->priv;
> - int err;
> + int oldpage, err;
> +
> + oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
>
> /* configure the RXER/INTR_32 pin of the 32-pin IP101GR if needed: */
> switch (priv->sel_intr32) {
> case IP101GR_SEL_INTR32_RXER:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32, 0);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> case IP101GR_SEL_INTR32_INTR:
> - err = phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> - IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> + err = __phy_modify(phydev, IP101G_DIGITAL_IO_SPEC_CTRL,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32,
> + IP101G_DIGITAL_IO_SPEC_CTRL_SEL_INTR32);
> if (err < 0)
> - return err;
> + goto out;
> break;
>
> default:
> @@ -284,12 +288,14 @@ static int ip101a_g_config_init(struct phy_device
> *phydev)
> * reserved as 'write-one'.
> */
> if (priv->model == IP101A) {
> - err = phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
> IP101A_G_APS_ON);
> + err = __phy_set_bits(phydev, IP10XX_SPEC_CTRL_STATUS,
> + IP101A_G_APS_ON);
> if (err)
> - return err;
> + goto out;
> }
>
> - return 0;
> +out:
> + return phy_restore_page(phydev, oldpage, err);
If a random page was set before entering config_init, do we actually want
to restore it? Or wouldn't it be better to set the default page as part
of initialization?
> }
>
> static int ip101a_g_ack_interrupt(struct phy_device *phydev)
> @@ -347,6 +353,26 @@ static irqreturn_t ip101a_g_handle_interrupt(struct
> phy_device *phydev)
> return IRQ_HANDLED;
> }
>
> +static int ip101a_g_read_page(struct phy_device *phydev)
> +{
> + struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> + if (priv->model == IP101A)
> + return 0;
> +
> + return __phy_read(phydev, IP101G_PAGE_CONTROL);
> +}
> +
> +static int ip101a_g_write_page(struct phy_device *phydev, int page)
> +{
> + struct ip101a_g_phy_priv *priv = phydev->priv;
> +
> + if (priv->model == IP101A)
> + return 0;
> +
> + return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
> +}
> +
> static struct phy_driver icplus_driver[] = {
> {
> PHY_ID_MATCH_MODEL(IP175C_PHY_ID),
> @@ -373,6 +399,8 @@ static struct phy_driver icplus_driver[] = {
> .config_intr = ip101a_g_config_intr,
> .handle_interrupt = ip101a_g_handle_interrupt,
> .config_init = ip101a_g_config_init,
> + .read_page = ip101a_g_read_page,
> + .write_page = ip101a_g_write_page,
> .soft_reset = genphy_soft_reset,
> .suspend = genphy_suspend,
> .resume = genphy_resume,
>
