Re: [PATCH net-next v3 7/9] net: phy: icplus: fix paged register access
From: Michael Walle
Date: Wed Feb 10 2021 - 18:56:54 EST
Am 2021-02-10 22:08, schrieb Michael Walle:
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.
To ease the function reuse between the non-paged register space of the
IP101A and the IP101G, add noop read_page()/write_page() callbacks so
the IP101G functions can also be used for the IP101A.
Signed-off-by: Michael Walle <michael@xxxxxxxx>
---
Changes since v2:
- none
Changes since v1:
- introduce a noop read/write_page() for the IP101A
- also use phy_*_paged() for the interrupt status register
Andrew, I've dropped your Reviewed-by because of this.
drivers/net/phy/icplus.c | 65 ++++++++++++++++++++++++++++++++--------
1 file changed, 52 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/icplus.c b/drivers/net/phy/icplus.c
index bc2b58061507..7e0ef05b1cae 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
@@ -211,23 +213,25 @@ static int ip101a_g_probe(struct phy_device
*phydev)
static int ip101a_g_config_intr_pin(struct phy_device *phydev)
{
struct ip101a_g_phy_priv *priv = phydev->priv;
- int err;
+ int oldpage, err;
besides this being uninitialized
+
+ oldpage = phy_select_page(phydev, IP101G_DEFAULT_PAGE);
this is also missing a check for negative return values
will be fixed in v4
-michael
/* 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:
@@ -241,7 +245,8 @@ static int ip101a_g_config_intr_pin(struct
phy_device *phydev)
break;
}
- return 0;
+out:
+ return phy_restore_page(phydev, oldpage, err);
}
static int ip101a_config_init(struct phy_device *phydev)
@@ -263,8 +268,10 @@ static int ip101g_config_init(struct phy_device
*phydev)
static int ip101a_g_ack_interrupt(struct phy_device *phydev)
{
- int err = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+ int err;
+ err = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
+ IP101A_G_IRQ_CONF_STATUS);
if (err < 0)
return err;
@@ -283,10 +290,12 @@ static int ip101a_g_config_intr(struct phy_device
*phydev)
/* INTR pin used: Speed/link/duplex will cause an interrupt */
val = IP101A_G_IRQ_PIN_USED;
- err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
+ err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
+ IP101A_G_IRQ_CONF_STATUS, val);
} else {
val = IP101A_G_IRQ_ALL_MASK;
- err = phy_write(phydev, IP101A_G_IRQ_CONF_STATUS, val);
+ err = phy_write_paged(phydev, IP101G_DEFAULT_PAGE,
+ IP101A_G_IRQ_CONF_STATUS, val);
if (err)
return err;
@@ -300,7 +309,8 @@ static irqreturn_t
ip101a_g_handle_interrupt(struct phy_device *phydev)
{
int irq_status;
- irq_status = phy_read(phydev, IP101A_G_IRQ_CONF_STATUS);
+ irq_status = phy_read_paged(phydev, IP101G_DEFAULT_PAGE,
+ IP101A_G_IRQ_CONF_STATUS);
if (irq_status < 0) {
phy_error(phydev);
return IRQ_NONE;
@@ -316,6 +326,31 @@ static irqreturn_t
ip101a_g_handle_interrupt(struct phy_device *phydev)
return IRQ_HANDLED;
}
+/* The IP101A doesn't really have a page register. We just pretend to
have one
+ * so we can use the paged versions of the callbacks of the IP101G.
+ */
+static int ip101a_read_page(struct phy_device *phydev)
+{
+ return IP101G_DEFAULT_PAGE;
+}
+
+static int ip101a_write_page(struct phy_device *phydev, int page)
+{
+ WARN_ONCE(page != IP101G_DEFAULT_PAGE, "wrong page selected\n");
+
+ return 0;
+}
+
+static int ip101g_read_page(struct phy_device *phydev)
+{
+ return __phy_read(phydev, IP101G_PAGE_CONTROL);
+}
+
+static int ip101g_write_page(struct phy_device *phydev, int page)
+{
+ return __phy_write(phydev, IP101G_PAGE_CONTROL, page);
+}
+
static int ip101a_g_has_page_register(struct phy_device *phydev)
{
int oldval, val, ret;
@@ -390,6 +425,8 @@ static struct phy_driver icplus_driver[] = {
.name = "ICPlus IP101A",
.match_phy_device = ip101a_match_phy_device,
.probe = ip101a_g_probe,
+ .read_page = ip101a_read_page,
+ .write_page = ip101a_write_page,
.config_intr = ip101a_g_config_intr,
.handle_interrupt = ip101a_g_handle_interrupt,
.config_init = ip101a_config_init,
@@ -400,6 +437,8 @@ static struct phy_driver icplus_driver[] = {
.name = "ICPlus IP101G",
.match_phy_device = ip101g_match_phy_device,
.probe = ip101a_g_probe,
+ .read_page = ip101g_read_page,
+ .write_page = ip101g_write_page,
.config_intr = ip101a_g_config_intr,
.handle_interrupt = ip101a_g_handle_interrupt,
.config_init = ip101g_config_init,
--
-michael