The patchset is a v2 of the previous single clean-up patch [0] which was
part of larger series. I initially to send these patches separately but
adding helper function without using them did not make much sense after
all. So, here is the complete patchset.
This patchset add defines for the control registers and helpers to access
the banked registers. The goal being to make it easier to understand what
the driver actually does.
Then there is fix for the incorrect sampling of the MII LPA register which
is often breaking the auto-negotiation with this PHY. More details on this
in the related patch
CONFIG_A6 settings is removed since this statement was without effect
Finally interrupt support is added, speeding things up a little
This series has been tested on the libretech-cc and khadas VIM
Jerome Brunet (8):
net: phy: meson-gxl: check phy_write return value
net: phy: meson-gxl: define control registers
net: phy: meson-gxl: add read and write helpers for bank registers
net: phy: meson-gxl: use genphy_config_init
net: phy: meson-gxl: detect LPA corruption
net: phy: meson-gxl: leave CONFIG_A6 untouched
net: phy: meson-gxl: add interrupt support
net: phy: meson-gxl: join the authors
drivers/net/phy/meson-gxl.c | 215 ++++++++++++++++++++++++++++++++++++++++----
1 file changed, 197 insertions(+), 18 deletions(-)
--
2.14.3
Always check phy_write return values. Better to be safe than sorry
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 50 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 38 insertions(+), 12 deletions(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 1ea69b7585d9..7ddb709f69fc 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -25,27 +25,53 @@
static int meson_gxl_config_init(struct phy_device *phydev)
{
+ int ret;
+
/* Enable Analog and DSP register Bank access by */
- phy_write(phydev, 0x14, 0x0000);
- phy_write(phydev, 0x14, 0x0400);
- phy_write(phydev, 0x14, 0x0000);
- phy_write(phydev, 0x14, 0x0400);
+ ret = phy_write(phydev, 0x14, 0x0000);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x0400);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x0000);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x0400);
+ if (ret)
+ return ret;
/* Write Analog register 23 */
- phy_write(phydev, 0x17, 0x8E0D);
- phy_write(phydev, 0x14, 0x4417);
+ ret = phy_write(phydev, 0x17, 0x8E0D);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x4417);
+ if (ret)
+ return ret;
/* Enable fractional PLL */
- phy_write(phydev, 0x17, 0x0005);
- phy_write(phydev, 0x14, 0x5C1B);
+ ret = phy_write(phydev, 0x17, 0x0005);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x5C1B);
+ if (ret)
+ return ret;
/* Program fraction FR_PLL_DIV1 */
- phy_write(phydev, 0x17, 0x029A);
- phy_write(phydev, 0x14, 0x5C1D);
+ ret = phy_write(phydev, 0x17, 0x029A);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x5C1D);
+ if (ret)
+ return ret;
/* Program fraction FR_PLL_DIV1 */
- phy_write(phydev, 0x17, 0xAAAA);
- phy_write(phydev, 0x14, 0x5C1C);
+ ret = phy_write(phydev, 0x17, 0xAAAA);
+ if (ret)
+ return ret;
+ ret = phy_write(phydev, 0x14, 0x5C1C);
+ if (ret)
+ return ret;
return 0;
}
--
2.14.3
Define registers and bits in meson-gxl PHY driver to make a bit
more human friendly. No functional change.
Signed-off-by: Neil Armstrong <[email protected]>
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 64 ++++++++++++++++++++++++++++++++++++---------
1 file changed, 51 insertions(+), 13 deletions(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 7ddb709f69fc..d82aa8cea401 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -22,54 +22,92 @@
#include <linux/ethtool.h>
#include <linux/phy.h>
#include <linux/netdevice.h>
+#include <linux/bitfield.h>
+
+#define TSTCNTL 20
+#define TSTCNTL_READ BIT(15)
+#define TSTCNTL_WRITE BIT(14)
+#define TSTCNTL_REG_BANK_SEL GENMASK(12, 11)
+#define TSTCNTL_TEST_MODE BIT(10)
+#define TSTCNTL_READ_ADDRESS GENMASK(9, 5)
+#define TSTCNTL_WRITE_ADDRESS GENMASK(4, 0)
+#define TSTREAD1 21
+#define TSTWRITE 23
+
+#define BANK_ANALOG_DSP 0
+#define BANK_BIST 3
+
+/* Analog/DSP Registers */
+#define A6_CONFIG_REG 0x17
+
+/* BIST Registers */
+#define FR_PLL_CONTROL 0x1b
+#define FR_PLL_DIV0 0x1c
+#define FR_PLL_DIV1 0x1d
static int meson_gxl_config_init(struct phy_device *phydev)
{
int ret;
/* Enable Analog and DSP register Bank access by */
- ret = phy_write(phydev, 0x14, 0x0000);
+ ret = phy_write(phydev, TSTCNTL, 0);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x0400);
+ ret = phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x0000);
+ ret = phy_write(phydev, TSTCNTL, 0);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x0400);
+ ret = phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE);
if (ret)
return ret;
- /* Write Analog register 23 */
- ret = phy_write(phydev, 0x17, 0x8E0D);
+ /* Write CONFIG_A6*/
+ ret = phy_write(phydev, TSTWRITE, 0x8e0d)
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x4417);
+ ret = phy_write(phydev, TSTCNTL,
+ TSTCNTL_WRITE
+ | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_ANALOG_DSP)
+ | TSTCNTL_TEST_MODE
+ | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, A6_CONFIG_REG));
if (ret)
return ret;
/* Enable fractional PLL */
- ret = phy_write(phydev, 0x17, 0x0005);
+ ret = phy_write(phydev, TSTWRITE, 0x0005);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x5C1B);
+ ret = phy_write(phydev, TSTCNTL,
+ TSTCNTL_WRITE
+ | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
+ | TSTCNTL_TEST_MODE
+ | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_CONTROL));
if (ret)
return ret;
/* Program fraction FR_PLL_DIV1 */
- ret = phy_write(phydev, 0x17, 0x029A);
+ ret = phy_write(phydev, TSTWRITE, 0x029a);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x5C1D);
+ ret = phy_write(phydev, TSTCNTL,
+ TSTCNTL_WRITE
+ | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
+ | TSTCNTL_TEST_MODE
+ | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV1));
if (ret)
return ret;
/* Program fraction FR_PLL_DIV1 */
- ret = phy_write(phydev, 0x17, 0xAAAA);
+ ret = phy_write(phydev, TSTWRITE, 0xaaaa);
if (ret)
return ret;
- ret = phy_write(phydev, 0x14, 0x5C1C);
+ ret = phy_write(phydev, TSTCNTL,
+ TSTCNTL_WRITE
+ | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
+ | TSTCNTL_TEST_MODE
+ | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV0));
if (ret)
return ret;
--
2.14.3
Following previous changes, join the other authors of this driver and
take the blame with them
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 861b021b9758..4cd5b2622ae1 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -256,4 +256,5 @@ MODULE_DEVICE_TABLE(mdio, meson_gxl_tbl);
MODULE_DESCRIPTION("Amlogic Meson GXL Internal PHY driver");
MODULE_AUTHOR("Baoqi wang");
MODULE_AUTHOR("Neil Armstrong <[email protected]>");
+MODULE_AUTHOR("Jerome Brunet <[email protected]>");
MODULE_LICENSE("GPL");
--
2.14.3
Enable interrupt support in meson-gxl PHY driver
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 5325940fe899..861b021b9758 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -33,6 +33,14 @@
#define TSTCNTL_WRITE_ADDRESS GENMASK(4, 0)
#define TSTREAD1 21
#define TSTWRITE 23
+#define INTSRC_FLAG 29
+#define INTSRC_ANEG_PR BIT(1)
+#define INTSRC_PARALLEL_FAULT BIT(2)
+#define INTSRC_ANEG_LP_ACK BIT(3)
+#define INTSRC_LINK_DOWN BIT(4)
+#define INTSRC_REMOTE_FAULT BIT(5)
+#define INTSRC_ANEG_COMPLETE BIT(6)
+#define INTSRC_MASK 30
#define BANK_ANALOG_DSP 0
#define BANK_WOL 1
@@ -193,17 +201,44 @@ int meson_gxl_read_status(struct phy_device *phydev)
return genphy_read_status(phydev);
}
+static int meson_gxl_ack_interrupt(struct phy_device *phydev)
+{
+ int ret = phy_read(phydev, INTSRC_FLAG);
+
+ return ret < 0 ? ret : 0;
+}
+
+static int meson_gxl_config_intr(struct phy_device *phydev)
+{
+ u16 val;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ val = INTSRC_ANEG_PR
+ | INTSRC_PARALLEL_FAULT
+ | INTSRC_ANEG_LP_ACK
+ | INTSRC_LINK_DOWN
+ | INTSRC_REMOTE_FAULT
+ | INTSRC_ANEG_COMPLETE;
+ } else {
+ val = 0;
+ }
+
+ return phy_write(phydev, INTSRC_MASK, val);
+}
+
static struct phy_driver meson_gxl_phy[] = {
{
.phy_id = 0x01814400,
.phy_id_mask = 0xfffffff0,
.name = "Meson GXL Internal PHY",
.features = PHY_BASIC_FEATURES,
- .flags = PHY_IS_INTERNAL,
+ .flags = PHY_IS_INTERNAL | PHY_HAS_INTERRUPT,
.config_init = meson_gxl_config_init,
.config_aneg = genphy_config_aneg,
.aneg_done = genphy_aneg_done,
.read_status = meson_gxl_read_status,
+ .ack_interrupt = meson_gxl_ack_interrupt,
+ .config_intr = meson_gxl_config_intr,
.suspend = genphy_suspend,
.resume = genphy_resume,
},
--
2.14.3
The PHY performs just as well when left in its default configuration and
it makes senses because this poke gets reset just after init.
According to the documentation, all registers in the Analog/DSP bank are
reset when there is a mode switch from 10BT to 100BT.
In the end, we have used the default configuration so far and there is no
reason to change now. Remove CONFIG_A6 poke to make this clear.
Signed-off-by: Jerome Brunet <[email protected]>
---
Out of curiosity, I tried to re-apply the ANALOG/DSP settings on speed
changes (patch available here [0] if someone wants to try) but I did
not notice any change as a result. In the end, I thought it was safer
to keep on using the ANALOG settings we have been actually using so far,
everybody seems to be happy with them
[0]: https://github.com/jeromebrunet/linux/commit/b594288e629a61574e76112497474fd3cf46c781
drivers/net/phy/meson-gxl.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 726e0eeed475..5325940fe899 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -38,9 +38,6 @@
#define BANK_WOL 1
#define BANK_BIST 3
-/* Analog/DSP Registers */
-#define A6_CONFIG_REG 0x17
-
/* WOL Registers */
#define LPI_STATUS 0xc
#define LPI_STATUS_RSV12 BIT(12)
@@ -126,12 +123,6 @@ static int meson_gxl_config_init(struct phy_device *phydev)
{
int ret;
- /* Write CONFIG_A6*/
- ret = meson_gxl_write_reg(phydev, BANK_ANALOG_DSP, A6_CONFIG_REG,
- 0x8e0d);
- if (ret)
- return ret;
-
/* Enable fractional PLL */
ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_CONTROL, 0x5);
if (ret)
--
2.14.3
The purpose of this change is to fix the incorrect detection of the link
partner (LP) advertised capabilities which sometimes happens with this PHY
(roughly 1 time in a dozen)
This issue may cause the link to be negotiated at 10Mbps/Full or
10Mbps/Half when 100MBps/Full is actually possible. In some case, the link
is even completely broken and no communication is possible.
To detect the corruption, we must look for a magic undocumented bit in the
WOL bank (hint given by the SoC vendor kernel) but this is not enough to
cover all cases. We also have to look at the LPA ack. If the LP supports
Aneg but did not ack our base code when aneg is completed, we assume
something went wrong.
The detection of a corrupted LPA triggers a restart of the aneg process.
This solves the problem but may take up to 6 retries to complete.
Fixes: 7334b3e47aee ("net: phy: Add Meson GXL Internal PHY driver")
Signed-off-by: Jerome Brunet <[email protected]>
---
I suppose this patch probably seems a bit hacky, especially the part
about the link partner acknowledge. I'm trying to figure out if the
value in MII_LPA makes sense but I don't have such a deep knowledge
of the ethernet spec.
To me, it does not makes sense for the LP to support ANEG (Bit 1 in
MII_EXPENSION), the aneg to have successfully complete and, at the
same time, LP does not ACK our base code word, which we should have
sent during this aneg.
If you think this may have unintended consequences or if you have
an idea to this differently, feel free to let me know.
drivers/net/phy/meson-gxl.c | 59 ++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 2e8c40df33c2..726e0eeed475 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -35,11 +35,16 @@
#define TSTWRITE 23
#define BANK_ANALOG_DSP 0
+#define BANK_WOL 1
#define BANK_BIST 3
/* Analog/DSP Registers */
#define A6_CONFIG_REG 0x17
+/* WOL Registers */
+#define LPI_STATUS 0xc
+#define LPI_STATUS_RSV12 BIT(12)
+
/* BIST Registers */
#define FR_PLL_CONTROL 0x1b
#define FR_PLL_DIV0 0x1c
@@ -145,6 +150,58 @@ static int meson_gxl_config_init(struct phy_device *phydev)
return genphy_config_init(phydev);
}
+/* This specific function is provided to cope with the possible failures of
+ * this phy during aneg process. When aneg fails, the PHY reports that aneg
+ * is done but the value found in MII_LPA is wrong:
+ * - Early failures: MII_LPA is just 0x0001. if MII_EXPANSION reports that
+ * the link partner (LP) supports aneg but the LP never acked our base
+ * code word, it is likely that we never sent it to begin with.
+ * - Late failures: MII_LPA is filled with a value which seems to make sense
+ * but it actually is not what the LP is advertising. It seems that we
+ * can detect this using a magic bit in the WOL bank (reg 12 - bit 12).
+ * If this particular bit is not set when aneg is reported being done,
+ * it means MII_LPA is likely to be wrong.
+ *
+ * In both case, forcing a restart of the aneg process solve the problem.
+ * When this failure happens, the first retry is usually successful but,
+ * in some cases, it may take up to 6 retries to get a decent result
+ */
+int meson_gxl_read_status(struct phy_device *phydev)
+{
+ int ret, wol, lpa, exp;
+
+ if (phydev->autoneg == AUTONEG_ENABLE) {
+ ret = genphy_aneg_done(phydev);
+ if (ret < 0)
+ return ret;
+ else if (!ret)
+ goto read_status_continue;
+
+ /* Aneg is done, let's check everything is fine */
+ wol = meson_gxl_read_reg(phydev, BANK_WOL, LPI_STATUS);
+ if (wol < 0)
+ return wol;
+
+ lpa = phy_read(phydev, MII_LPA);
+ if (lpa < 0)
+ return lpa;
+
+ exp = phy_read(phydev, MII_EXPANSION);
+ if (exp < 0)
+ return exp;
+
+ if (!(wol & LPI_STATUS_RSV12) ||
+ ((exp & EXPANSION_NWAY) && !(lpa & LPA_LPACK))) {
+ /* Looks like aneg failed after all */
+ phydev_dbg(phydev, "LPA corruption - aneg restart\n");
+ return genphy_restart_aneg(phydev);
+ }
+ }
+
+read_status_continue:
+ return genphy_read_status(phydev);
+}
+
static struct phy_driver meson_gxl_phy[] = {
{
.phy_id = 0x01814400,
@@ -155,7 +212,7 @@ static struct phy_driver meson_gxl_phy[] = {
.config_init = meson_gxl_config_init,
.config_aneg = genphy_config_aneg,
.aneg_done = genphy_aneg_done,
- .read_status = genphy_read_status,
+ .read_status = meson_gxl_read_status,
.suspend = genphy_suspend,
.resume = genphy_resume,
},
--
2.14.3
Use the generic init function to populate some of the phydev
structure fields
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index 05054770aefb..2e8c40df33c2 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -142,7 +142,7 @@ static int meson_gxl_config_init(struct phy_device *phydev)
if (ret)
return ret;
- return 0;
+ return genphy_config_init(phydev);
}
static struct phy_driver meson_gxl_phy[] = {
--
2.14.3
Add read and write helpers to manipulate banked registers on this PHY
This helps clarify the settings applied to these registers in the init
function and upcoming changes.
Signed-off-by: Jerome Brunet <[email protected]>
---
drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++----------------
1 file changed, 67 insertions(+), 36 deletions(-)
diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
index d82aa8cea401..05054770aefb 100644
--- a/drivers/net/phy/meson-gxl.c
+++ b/drivers/net/phy/meson-gxl.c
@@ -45,11 +45,13 @@
#define FR_PLL_DIV0 0x1c
#define FR_PLL_DIV1 0x1d
-static int meson_gxl_config_init(struct phy_device *phydev)
+static int meson_gxl_open_banks(struct phy_device *phydev)
{
int ret;
- /* Enable Analog and DSP register Bank access by */
+ /* Enable Analog and DSP register Bank access by
+ * toggling TSTCNTL_TEST_MODE bit in the TSTCNTL register
+ */
ret = phy_write(phydev, TSTCNTL, 0);
if (ret)
return ret;
@@ -59,55 +61,84 @@ static int meson_gxl_config_init(struct phy_device *phydev)
ret = phy_write(phydev, TSTCNTL, 0);
if (ret)
return ret;
- ret = phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE);
- if (ret)
- return ret;
+ return phy_write(phydev, TSTCNTL, TSTCNTL_TEST_MODE);
+}
- /* Write CONFIG_A6*/
- ret = phy_write(phydev, TSTWRITE, 0x8e0d)
+static void meson_gxl_close_banks(struct phy_device *phydev)
+{
+ phy_write(phydev, TSTCNTL, 0);
+}
+
+static int meson_gxl_read_reg(struct phy_device *phydev,
+ unsigned int bank, unsigned int reg)
+{
+ int ret;
+
+ ret = meson_gxl_open_banks(phydev);
if (ret)
- return ret;
- ret = phy_write(phydev, TSTCNTL,
- TSTCNTL_WRITE
- | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_ANALOG_DSP)
- | TSTCNTL_TEST_MODE
- | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, A6_CONFIG_REG));
+ goto out;
+
+ ret = phy_write(phydev, TSTCNTL, TSTCNTL_READ |
+ FIELD_PREP(TSTCNTL_REG_BANK_SEL, bank) |
+ TSTCNTL_TEST_MODE |
+ FIELD_PREP(TSTCNTL_READ_ADDRESS, reg));
if (ret)
- return ret;
+ goto out;
- /* Enable fractional PLL */
- ret = phy_write(phydev, TSTWRITE, 0x0005);
+ ret = phy_read(phydev, TSTREAD1);
+out:
+ /* Close the bank access on our way out */
+ meson_gxl_close_banks(phydev);
+ return ret;
+}
+
+static int meson_gxl_write_reg(struct phy_device *phydev,
+ unsigned int bank, unsigned int reg,
+ uint16_t value)
+{
+ int ret;
+
+ ret = meson_gxl_open_banks(phydev);
if (ret)
- return ret;
- ret = phy_write(phydev, TSTCNTL,
- TSTCNTL_WRITE
- | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
- | TSTCNTL_TEST_MODE
- | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_CONTROL));
+ goto out;
+
+ ret = phy_write(phydev, TSTWRITE, value);
if (ret)
- return ret;
+ goto out;
- /* Program fraction FR_PLL_DIV1 */
- ret = phy_write(phydev, TSTWRITE, 0x029a);
+ ret = phy_write(phydev, TSTCNTL, TSTCNTL_WRITE |
+ FIELD_PREP(TSTCNTL_REG_BANK_SEL, bank) |
+ TSTCNTL_TEST_MODE |
+ FIELD_PREP(TSTCNTL_WRITE_ADDRESS, reg));
+
+out:
+ /* Close the bank access on our way out */
+ meson_gxl_close_banks(phydev);
+ return ret;
+}
+
+static int meson_gxl_config_init(struct phy_device *phydev)
+{
+ int ret;
+
+ /* Write CONFIG_A6*/
+ ret = meson_gxl_write_reg(phydev, BANK_ANALOG_DSP, A6_CONFIG_REG,
+ 0x8e0d);
if (ret)
return ret;
- ret = phy_write(phydev, TSTCNTL,
- TSTCNTL_WRITE
- | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
- | TSTCNTL_TEST_MODE
- | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV1));
+
+ /* Enable fractional PLL */
+ ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_CONTROL, 0x5);
if (ret)
return ret;
/* Program fraction FR_PLL_DIV1 */
- ret = phy_write(phydev, TSTWRITE, 0xaaaa);
+ ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_DIV1, 0x029a);
if (ret)
return ret;
- ret = phy_write(phydev, TSTCNTL,
- TSTCNTL_WRITE
- | FIELD_PREP(TSTCNTL_REG_BANK_SEL, BANK_BIST)
- | TSTCNTL_TEST_MODE
- | FIELD_PREP(TSTCNTL_WRITE_ADDRESS, FR_PLL_DIV0));
+
+ /* Program fraction FR_PLL_DIV1 */
+ ret = meson_gxl_write_reg(phydev, BANK_BIST, FR_PLL_DIV0, 0xaaaa);
if (ret)
return ret;
--
2.14.3
On Thu, Dec 07, 2017 at 03:27:12PM +0100, Jerome Brunet wrote:
> The purpose of this change is to fix the incorrect detection of the link
> partner (LP) advertised capabilities which sometimes happens with this PHY
> (roughly 1 time in a dozen)
Hi Jerome
Since this is a real fix, please send it alone. Base it on net, not
net-next. David will then get it applied to stable, so it will be
backported.
Thanks
Andrew
On Thu, Dec 07, 2017 at 03:27:08PM +0100, Jerome Brunet wrote:
> Always check phy_write return values. Better to be safe than sorry
>
> Signed-off-by: Jerome Brunet <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Thu, 2017-12-07 at 16:34 +0100, Andrew Lunn wrote:
> On Thu, Dec 07, 2017 at 03:27:12PM +0100, Jerome Brunet wrote:
> > The purpose of this change is to fix the incorrect detection of the link
> > partner (LP) advertised capabilities which sometimes happens with this PHY
> > (roughly 1 time in a dozen)
>
> Hi Jerome
>
> Since this is a real fix, please send it alone. Base it on net, not
> net-next. David will then get it applied to stable, so it will be
> backported.
I hesitated on this. For this fix to be "not too ugly" I need at least the
helper functions (patch 1 to 3) ... which are not really fixes.
Would it be Ok if send patches 1 to 5 to net ?
and 6 to 8 separately on net-next ?
>
> Thanks
> Andrew
On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote:
> Add read and write helpers to manipulate banked registers on this PHY
> This helps clarify the settings applied to these registers in the init
> function and upcoming changes.
>
> Signed-off-by: Jerome Brunet <[email protected]>
> ---
> drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++----------------
> 1 file changed, 67 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
> index d82aa8cea401..05054770aefb 100644
> --- a/drivers/net/phy/meson-gxl.c
> +++ b/drivers/net/phy/meson-gxl.c
> @@ -45,11 +45,13 @@
> #define FR_PLL_DIV0 0x1c
> #define FR_PLL_DIV1 0x1d
>
> -static int meson_gxl_config_init(struct phy_device *phydev)
> +static int meson_gxl_open_banks(struct phy_device *phydev)
Hi Jerome
Does the word bank come from the datasheet? Most of the phy drives use
page instead.
Also, we have discovered a race condition which affects drivers using
pages, which can lead to corruption of registers. At some point, i
expect we will be adding helpers to access paged registers, which do
the right thing with respect to locks.
So it would be nice if you used the work page, not bank.
Thanks
Andrew
On Thu, 2017-12-07 at 16:46 +0100, Andrew Lunn wrote:
> On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote:
> > Add read and write helpers to manipulate banked registers on this PHY
> > This helps clarify the settings applied to these registers in the init
> > function and upcoming changes.
> >
> > Signed-off-by: Jerome Brunet <[email protected]>
> > ---
> > drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++-------------
> > ---
> > 1 file changed, 67 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
> > index d82aa8cea401..05054770aefb 100644
> > --- a/drivers/net/phy/meson-gxl.c
> > +++ b/drivers/net/phy/meson-gxl.c
> > @@ -45,11 +45,13 @@
> > #define FR_PLL_DIV0 0x1c
> > #define FR_PLL_DIV1 0x1d
> >
> > -static int meson_gxl_config_init(struct phy_device *phydev)
> > +static int meson_gxl_open_banks(struct phy_device *phydev)
>
> Hi Jerome
>
> Does the word bank come from the datasheet? Most of the phy drives use
> page instead.
>
> Also, we have discovered a race condition which affects drivers using
> pages, which can lead to corruption of registers. At some point, i
> expect we will be adding helpers to access paged registers, which do
> the right thing with respect to locks.
>
> So it would be nice if you used the work page, not bank.
Banks actually comes from the datasheet, Yes.
I don't mind renaming it but I would be making things up. As you wish ?
Does the usual pages comes with this weird toggle thing to open the access ?
Would we able to use these generic helpers with our this kind of quirks ?
>
> Thanks
>
> Andrew
On Thu, Dec 07, 2017 at 03:27:14PM +0100, Jerome Brunet wrote:
> Enable interrupt support in meson-gxl PHY driver
Hi Jerome
Is it possible to implement did_interrupt()? That allows for shared
interrupts. It does however work fine without it, so long as the
interrupt is not shared.
Thanks
Andrew
On Thu, Dec 07, 2017 at 03:27:15PM +0100, Jerome Brunet wrote:
> Following previous changes, join the other authors of this driver and
> take the blame with them
:-)
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
> Banks actually comes from the datasheet, Yes.
> I don't mind renaming it but I would be making things up. As you wish ?
Keep it as is for the moment.
> Does the usual pages comes with this weird toggle thing to open the access ?
> Would we able to use these generic helpers with our this kind of quirks ?
I don't think the API has been defined yet. But what has been
discussed is adding functions to struct phy_driver. The driver can
then implement whatever is needed to select a given page. There will
then be helpers which take the lock, select the page, do the
read/write, select page 0, and unlock.
Supporting this funny toggle should not be a problem.
Andrew
On Thu, 2017-12-07 at 16:54 +0100, Andrew Lunn wrote:
> On Thu, Dec 07, 2017 at 03:27:14PM +0100, Jerome Brunet wrote:
> > Enable interrupt support in meson-gxl PHY driver
>
> Hi Jerome
>
> Is it possible to implement did_interrupt()? That allows for shared
> interrupts. It does however work fine without it, so long as the
> interrupt is not shared.
Hi Andrew,
It is always possible ;). The irq status registers gets reset on read.
In such case, ack_interrupt() and did_interrupt() would be more or less the same
function, except for the returned value, I guess ?
The phy being internal, I think it is unlikely to ever share its interrupt
though. If you prefer I implement this callback, I can certainly re-spin with it
?
Thanks for the lightning fast review by the way !
>
> Thanks
> Andrew
> The phy being internal, I think it is unlikely to ever share its interrupt
> though.
O.K, don't bother.
> Thanks for the lightning fast review by the way !
You are welcome.
Andrew
> Would it be Ok if send patches 1 to 5 to net ?
> and 6 to 8 separately on net-next ?
No. The rules for stable is that a patch must really fix something and
be minimal.
Documentation/process/stable-kernel-rules.rst
What might be best is to develop a minimal, but ugly patch for stable.
Get it applied. Around a week later, net will be merged into
net-next. You can then have a 'revert' patch, followed by this series
making it nice and clean.
Andrew
On Thu, 2017-12-07 at 17:12 +0100, Andrew Lunn wrote:
> > Would it be Ok if send patches 1 to 5 to net ?
> > and 6 to 8 separately on net-next ?
>
> No. The rules for stable is that a patch must really fix something and
> be minimal.
>
> Documentation/process/stable-kernel-rules.rst
>
> What might be best is to develop a minimal, but ugly patch for stable.
> Get it applied. Around a week later, net will be merged into
> net-next. You can then have a 'revert' patch, followed by this series
> making it nice and clean.
Looks like a plan. Will do.
Thanks Andrew.
>
> Andrew
On Thu, 2017-12-07 at 16:49 +0100, Andrew Lunn wrote:
> On Thu, Dec 07, 2017 at 03:27:13PM +0100, Jerome Brunet wrote:
> > The PHY performs just as well when left in its default configuration and
> > it makes senses because this poke gets reset just after init.
>
> The only thing which might speak against this, is some bootloader
> which sets something other than the default, and here we put it back
> to the value it should have. But if you say a reset will put it back
> to the default value anyway, this seems save.
I was worried about this too bu the bank also gets reset on power down and soft
reset, so we won't get bootloader value at this point
>
> Reviewed-by: Andrew Lunn <[email protected]>
>
> Andrew
On Thu, Dec 07, 2017 at 03:27:13PM +0100, Jerome Brunet wrote:
> The PHY performs just as well when left in its default configuration and
> it makes senses because this poke gets reset just after init.
The only thing which might speak against this, is some bootloader
which sets something other than the default, and here we put it back
to the value it should have. But if you say a reset will put it back
to the default value anyway, this seems save.
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On 07/12/2017 16:46, Andrew Lunn wrote:
> On Thu, Dec 07, 2017 at 03:27:10PM +0100, Jerome Brunet wrote:
>> Add read and write helpers to manipulate banked registers on this PHY
>> This helps clarify the settings applied to these registers in the init
>> function and upcoming changes.
>>
>> Signed-off-by: Jerome Brunet <[email protected]>
>> ---
>> drivers/net/phy/meson-gxl.c | 103 ++++++++++++++++++++++++++++----------------
>> 1 file changed, 67 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/net/phy/meson-gxl.c b/drivers/net/phy/meson-gxl.c
>> index d82aa8cea401..05054770aefb 100644
>> --- a/drivers/net/phy/meson-gxl.c
>> +++ b/drivers/net/phy/meson-gxl.c
>> @@ -45,11 +45,13 @@
>> #define FR_PLL_DIV0 0x1c
>> #define FR_PLL_DIV1 0x1d
>>
>> -static int meson_gxl_config_init(struct phy_device *phydev)
>> +static int meson_gxl_open_banks(struct phy_device *phydev)
>
> Hi Jerome
>
> Does the word bank come from the datasheet? Most of the phy drives use
> page instead.
Yes, it's explicitly described as banks in the datasheet.
Neil
>
> Also, we have discovered a race condition which affects drivers using
> pages, which can lead to corruption of registers. At some point, i
> expect we will be adding helpers to access paged registers, which do
> the right thing with respect to locks.
>
> So it would be nice if you used the work page, not bank.
>
> Thanks
>
> Andrew
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
On Thu, Dec 07, 2017 at 03:27:11PM +0100, Jerome Brunet wrote:
> Use the generic init function to populate some of the phydev
> structure fields
>
> Signed-off-by: Jerome Brunet <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Andrew
On Thu, Dec 07, 2017 at 05:02:35PM +0100, Andrew Lunn wrote:
> > Banks actually comes from the datasheet, Yes.
> > I don't mind renaming it but I would be making things up. As you wish ?
>
> Keep it as is for the moment.
>
> > Does the usual pages comes with this weird toggle thing to open the access ?
> > Would we able to use these generic helpers with our this kind of quirks ?
>
> I don't think the API has been defined yet. But what has been
> discussed is adding functions to struct phy_driver. The driver can
> then implement whatever is needed to select a given page. There will
> then be helpers which take the lock, select the page, do the
> read/write, select page 0, and unlock.
I'm not sure adding generic helpers really works, because the indirection
through phy_driver just makes the code more complex. For Marvell, I
ended up with:
http://git.armlinux.org.uk/cgit/linux-arm.git/commit/?h=phy&id=9ca46084228bb1b8851e7d24276d85c4ec6e13ae
and the preceding three commits.
The "generic" versions I came up with were basically lifting
marvell_read_paged(), marvell_write_paged() and marvell_modify_paged()
to phylib, along with the lower leve marvell_save_page(),
marvell_select_page() and marvell_restore_page(). The result is a
fair amount of out-of-line code and an assumption that it's a single
register. As soon as a PHY has other requirements, these generic
implementations don't work.
Also, unfortunately, we can't get help from sparse for statically
checking the locking - the __acquire()/__release() annotations don't
work for mutexes.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up