Add the 'realtek,eee-led-mode-disable' property to disable EEE
LED mode on Realtek PHYs that support it.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- document 'realtek,eee-led-mode-disable' instead of
'realtek,enable-ssc' in the initial version
---
.../devicetree/bindings/net/realtek.txt | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
create mode 100644 Documentation/devicetree/bindings/net/realtek.txt
diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
new file mode 100644
index 000000000000..63f7002fa704
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek.txt
@@ -0,0 +1,19 @@
+Realtek PHY properties.
+
+This document describes properties of Realtek PHYs.
+
+Optional properties:
+- realtek,eee-led-mode-disable: Disable EEE LED mode on this port.
+
+Example:
+
+mdio0 {
+ compatible = "snps,dwmac-mdio";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ ethphy: ethernet-phy@1 {
+ reg = <1>;
+ realtek,eee-led-mode-disable;
+ };
+};
--
2.22.0.410.gd8fdbe21b5-goog
The LED behavior of some Realtek PHYs is configurable. Add the
property 'realtek,led-modes' to specify the configuration of the
LEDs.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- patch added to the series
---
.../devicetree/bindings/net/realtek.txt | 9 +++++++++
include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
2 files changed, 26 insertions(+)
create mode 100644 include/dt-bindings/net/realtek.h
diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
index 71d386c78269..40b0d6f9ee21 100644
--- a/Documentation/devicetree/bindings/net/realtek.txt
+++ b/Documentation/devicetree/bindings/net/realtek.txt
@@ -9,6 +9,12 @@ Optional properties:
SSC is only available on some Realtek PHYs (e.g. RTL8211E).
+- realtek,led-modes: LED mode configuration.
+
+ A 0..3 element vector, with each element configuring the operating
+ mode of an LED. Omitted LEDs are turned off. Allowed values are
+ defined in "include/dt-bindings/net/realtek.h".
+
Example:
mdio0 {
@@ -20,5 +26,8 @@ mdio0 {
reg = <1>;
realtek,eee-led-mode-disable;
realtek,enable-ssc;
+ realtek,led-modes = <RTL8211E_LINK_ACTIVITY
+ RTL8211E_LINK_100
+ RTL8211E_LINK_1000>;
};
};
diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
new file mode 100644
index 000000000000..8d64f58d58f8
--- /dev/null
+++ b/include/dt-bindings/net/realtek.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _DT_BINDINGS_REALTEK_H
+#define _DT_BINDINGS_REALTEK_H
+
+/* LED modes for RTL8211E PHY */
+
+#define RTL8211E_LINK_10 1
+#define RTL8211E_LINK_100 2
+#define RTL8211E_LINK_1000 4
+#define RTL8211E_LINK_10_100 3
+#define RTL8211E_LINK_10_1000 5
+#define RTL8211E_LINK_100_1000 6
+#define RTL8211E_LINK_10_100_1000 7
+
+#define RTL8211E_LINK_ACTIVITY (1 << 16)
+
+#endif
--
2.22.0.410.gd8fdbe21b5-goog
Configure the RTL8211E LEDs behavior when the device tree property
'realtek,led-modes' is specified.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- patch added to the series
---
drivers/net/phy/realtek.c | 63 +++++++++++++++++++++++++++++++++++++--
1 file changed, 61 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 45fee4612031..559aec547738 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,6 +9,7 @@
* Copyright (c) 2004 Freescale Semiconductor, Inc.
*/
#include <linux/bitops.h>
+#include <linux/bits.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/of.h>
@@ -35,6 +36,15 @@
#define RTL8211E_EEE_LED_MODE1 0x05
#define RTL8211E_EEE_LED_MODE2 0x06
+/* RTL8211E extension page 44 */
+#define RTL8211E_LACR 0x1a
+#define RLT8211E_LACR_LEDACTCTRL_SHIFT 4
+#define RLT8211E_LACR_LEDACTCTRL_MASK GENMASK(6, 4)
+#define RTL8211E_LCR 0x1c
+#define RTL8211E_LCR_LEDCTRL_MASK (GENMASK(2, 0) | \
+ GENMASK(6, 4) | \
+ GENMASK(10, 8))
+
/* RTL8211E extension page 160 */
#define RTL8211E_SCR 0x1a
#define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
@@ -124,6 +134,56 @@ static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
return phy_restore_page(phydev, oldpage, ret);
}
+static int rtl8211e_config_leds(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+ int count, i, oldpage, ret;
+ u16 lacr_bits = 0, lcr_bits = 0;
+
+ if (!dev->of_node)
+ return 0;
+
+ if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
+ rtl8211e_disable_eee_led_mode(phydev);
+
+ count = of_property_count_elems_of_size(dev->of_node,
+ "realtek,led-modes",
+ sizeof(u32));
+ if (count < 0 || count > 3)
+ return -EINVAL;
+
+ for (i = 0; i < count; i++) {
+ u32 val;
+
+ of_property_read_u32_index(dev->of_node,
+ "realtek,led-modes", i, &val);
+ lacr_bits |= (u16)(val >> 16) <<
+ (RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
+ lcr_bits |= (u16)(val & 0xf) << (i * 4);
+ }
+
+ oldpage = rtl8211e_select_ext_page(phydev, 44);
+ if (oldpage < 0) {
+ dev_err(dev, "failed to select extended page: %d\n", oldpage);
+ goto err;
+ }
+
+ ret = __phy_modify(phydev, RTL8211E_LACR,
+ RLT8211E_LACR_LEDACTCTRL_MASK, lacr_bits);
+ if (ret) {
+ dev_err(dev, "failed to write LACR reg: %d\n", ret);
+ goto err;
+ }
+
+ ret = __phy_modify(phydev, RTL8211E_LCR,
+ RTL8211E_LCR_LEDCTRL_MASK, lcr_bits);
+ if (ret)
+ dev_err(dev, "failed to write LCR reg: %d\n", ret);
+
+err:
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8211e_config_init(struct phy_device *phydev)
{
struct device *dev = &phydev->mdio.dev;
@@ -137,8 +197,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
dev_err(dev, "failed to enable SSC on RXC: %d\n", ret);
}
- if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
- rtl8211e_disable_eee_led_mode(phydev);
+ rtl8211e_config_leds(phydev);
return 0;
}
--
2.22.0.410.gd8fdbe21b5-goog
Add the 'realtek,enable-ssc' property to enable Spread Spectrum
Clocking (SSC) on Realtek PHYs that support it.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- patch added to the series (kind of, it already existed, but now
the binding is created by another patch)
---
Documentation/devicetree/bindings/net/realtek.txt | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
index 63f7002fa704..71d386c78269 100644
--- a/Documentation/devicetree/bindings/net/realtek.txt
+++ b/Documentation/devicetree/bindings/net/realtek.txt
@@ -5,6 +5,10 @@ This document describes properties of Realtek PHYs.
Optional properties:
- realtek,eee-led-mode-disable: Disable EEE LED mode on this port.
+- realtek,enable-ssc : Enable Spread Spectrum Clocking (SSC) on this port.
+
+ SSC is only available on some Realtek PHYs (e.g. RTL8211E).
+
Example:
mdio0 {
@@ -15,5 +19,6 @@ mdio0 {
ethphy: ethernet-phy@1 {
reg = <1>;
realtek,eee-led-mode-disable;
+ realtek,enable-ssc;
};
};
--
2.22.0.410.gd8fdbe21b5-goog
EEE LED mode is enabled by default on the RTL8211E. Disable it when
the device tree property 'realtek,eee-led-mode-disable' exists.
The magic values to disable EEE LED mode were taken from the RTL8211E
datasheet, unfortunately they are not further documented.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- patch added to the series
---
drivers/net/phy/realtek.c | 37 ++++++++++++++++++++++++++++++++++++-
1 file changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945eb829..eb815cbe1e72 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,8 +9,9 @@
* Copyright (c) 2004 Freescale Semiconductor, Inc.
*/
#include <linux/bitops.h>
-#include <linux/phy.h>
#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy.h>
#define RTL821x_PHYSR 0x11
#define RTL821x_PHYSR_DUPLEX BIT(13)
@@ -26,6 +27,10 @@
#define RTL821x_EXT_PAGE_SELECT 0x1e
#define RTL821x_PAGE_SELECT 0x1f
+/* RTL8211E page 5 */
+#define RTL8211E_EEE_LED_MODE1 0x05
+#define RTL8211E_EEE_LED_MODE2 0x06
+
#define RTL8211F_INSR 0x1d
#define RTL8211F_TX_DELAY BIT(8)
@@ -53,6 +58,35 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
}
+static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
+{
+ int ret = 0;
+ int oldpage;
+
+ oldpage = phy_select_page(phydev, 5);
+ if (oldpage < 0)
+ goto out;
+
+ /* write magic values to disable EEE LED mode */
+ ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
+ if (ret)
+ goto out;
+
+ ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
+
+out:
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
+static int rtl8211e_config_init(struct phy_device *phydev)
+{
+ struct device *dev = &phydev->mdio.dev;
+
+ if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
+ rtl8211e_disable_eee_led_mode(phydev);
+
+ return 0;
+}
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
@@ -310,6 +344,7 @@ static struct phy_driver realtek_drvs[] = {
.name = "RTL8211E Gigabit Ethernet",
.config_init = &rtl8211e_config_init,
.ack_interrupt = &rtl821x_ack_interrupt,
+ .config_init = &rtl8211e_config_init,
.config_intr = &rtl8211e_config_intr,
.suspend = genphy_suspend,
.resume = genphy_resume,
--
2.22.0.410.gd8fdbe21b5-goog
By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
Enable it if the device tree property 'realtek,enable-ssc' exists.
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- enable SSC in config_init() instead of probe()
- fixed error check after enabling SSC
---
drivers/net/phy/realtek.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 9cd6241e2a6d..45fee4612031 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -9,6 +9,7 @@
* Copyright (c) 2004 Freescale Semiconductor, Inc.
*/
#include <linux/bitops.h>
+#include <linux/device.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/phy.h>
@@ -34,6 +35,10 @@
#define RTL8211E_EEE_LED_MODE1 0x05
#define RTL8211E_EEE_LED_MODE2 0x06
+/* RTL8211E extension page 160 */
+#define RTL8211E_SCR 0x1a
+#define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
+
#define RTL8211F_INSR 0x1d
#define RTL8211F_TX_DELAY BIT(8)
@@ -76,8 +81,8 @@ static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
return 0;
}
-static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
- int page, u32 regnum, u16 mask, u16 set)
+static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
+ u32 regnum, u16 mask, u16 set)
{
int ret = 0;
int oldpage;
@@ -122,6 +127,15 @@ static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
static int rtl8211e_config_init(struct phy_device *phydev)
{
struct device *dev = &phydev->mdio.dev;
+ int ret;
+
+ if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
+ ret = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
+ RTL8211E_SCR_DISABLE_RXC_SSC,
+ 0);
+ if (ret < 0)
+ dev_err(dev, "failed to enable SSC on RXC: %d\n", ret);
+ }
if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
rtl8211e_disable_eee_led_mode(phydev);
--
2.22.0.410.gd8fdbe21b5-goog
The RTL8211E has extension pages, which can be accessed after
selecting a page through a custom method. Add a function to
modify bits in a register of an extension page and a helper for
selecting an ext page.
rtl8211e_modify_ext_paged() is inspired by its counterpart
phy_modify_paged().
Signed-off-by: Matthias Kaehlcke <[email protected]>
---
Changes in v2:
- assign .read/write_page handlers for RTL8211E
- use phy_select_page() and phy_restore_page(), get rid of
rtl8211e_restore_page()
- s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
- updated commit message
---
drivers/net/phy/realtek.c | 42 +++++++++++++++++++++++++++++++++++++++
1 file changed, 42 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index eb815cbe1e72..9cd6241e2a6d 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -27,6 +27,9 @@
#define RTL821x_EXT_PAGE_SELECT 0x1e
#define RTL821x_PAGE_SELECT 0x1f
+#define RTL8211E_EXT_PAGE 7
+#define RTL8211E_EPAGSR 0x1e
+
/* RTL8211E page 5 */
#define RTL8211E_EEE_LED_MODE1 0x05
#define RTL8211E_EEE_LED_MODE2 0x06
@@ -58,6 +61,44 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
}
+static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
+{
+ int ret, oldpage;
+
+ oldpage = phy_select_page(phydev, RTL8211E_EXT_PAGE);
+ if (oldpage < 0)
+ return oldpage;
+
+ ret = __phy_write(phydev, RTL8211E_EPAGSR, page);
+ if (ret)
+ return phy_restore_page(phydev, page, ret);
+
+ return 0;
+}
+
+static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
+ int page, u32 regnum, u16 mask, u16 set)
+{
+ int ret = 0;
+ int oldpage;
+ int new;
+
+ oldpage = rtl8211e_select_ext_page(phydev, page);
+ if (oldpage < 0)
+ goto out;
+
+ ret = __phy_read(phydev, regnum);
+ if (ret < 0)
+ goto out;
+
+ new = (ret & ~mask) | set;
+ if (new != ret)
+ ret = __phy_write(phydev, regnum, new);
+
+out:
+ return phy_restore_page(phydev, oldpage, ret);
+}
+
static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
{
int ret = 0;
@@ -87,6 +128,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
return 0;
}
+
static int rtl8201_ack_interrupt(struct phy_device *phydev)
{
int err;
--
2.22.0.410.gd8fdbe21b5-goog
On Wed, Jul 03, 2019 at 12:37:23PM -0700, Matthias Kaehlcke wrote:
Hi Matthias
Maybe add a #define for 0, so we know what it does.
> +#define RTL8211E_LINK_10 1
> +#define RTL8211E_LINK_100 2
> +#define RTL8211E_LINK_1000 4
> + for (i = 0; i < count; i++) {
> + u32 val;
> +
> + of_property_read_u32_index(dev->of_node,
> + "realtek,led-modes", i, &val);
Please validate the value, 0 - 7.
Andrew
On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> The LED behavior of some Realtek PHYs is configurable. Add the
> property 'realtek,led-modes' to specify the configuration of the
> LEDs.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - patch added to the series
> ---
> .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> 2 files changed, 26 insertions(+)
> create mode 100644 include/dt-bindings/net/realtek.h
>
> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> index 71d386c78269..40b0d6f9ee21 100644
> --- a/Documentation/devicetree/bindings/net/realtek.txt
> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> @@ -9,6 +9,12 @@ Optional properties:
>
> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>
> +- realtek,led-modes: LED mode configuration.
> +
> + A 0..3 element vector, with each element configuring the operating
> + mode of an LED. Omitted LEDs are turned off. Allowed values are
> + defined in "include/dt-bindings/net/realtek.h".
> +
> Example:
>
> mdio0 {
> @@ -20,5 +26,8 @@ mdio0 {
> reg = <1>;
> realtek,eee-led-mode-disable;
> realtek,enable-ssc;
> + realtek,led-modes = <RTL8211E_LINK_ACTIVITY
> + RTL8211E_LINK_100
> + RTL8211E_LINK_1000>;
> };
> };
> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
> new file mode 100644
> index 000000000000..8d64f58d58f8
> --- /dev/null
> +++ b/include/dt-bindings/net/realtek.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_REALTEK_H
> +#define _DT_BINDINGS_REALTEK_H
> +
> +/* LED modes for RTL8211E PHY */
> +
> +#define RTL8211E_LINK_10 1
> +#define RTL8211E_LINK_100 2
> +#define RTL8211E_LINK_1000 4
> +#define RTL8211E_LINK_10_100 3
> +#define RTL8211E_LINK_10_1000 5
> +#define RTL8211E_LINK_100_1000 6
> +#define RTL8211E_LINK_10_100_1000 7
> +
> +#define RTL8211E_LINK_ACTIVITY (1 << 16)
I don't see where this is used.
> +
> +#endif
>
On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> EEE LED mode is enabled by default on the RTL8211E. Disable it when
> the device tree property 'realtek,eee-led-mode-disable' exists.
>
> The magic values to disable EEE LED mode were taken from the RTL8211E
> datasheet, unfortunately they are not further documented.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - patch added to the series
> ---
> drivers/net/phy/realtek.c | 37 ++++++++++++++++++++++++++++++++++++-
> 1 file changed, 36 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a669945eb829..eb815cbe1e72 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,8 +9,9 @@
> * Copyright (c) 2004 Freescale Semiconductor, Inc.
> */
> #include <linux/bitops.h>
> -#include <linux/phy.h>
> #include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/phy.h>
>
> #define RTL821x_PHYSR 0x11
> #define RTL821x_PHYSR_DUPLEX BIT(13)
> @@ -26,6 +27,10 @@
> #define RTL821x_EXT_PAGE_SELECT 0x1e
> #define RTL821x_PAGE_SELECT 0x1f
>
> +/* RTL8211E page 5 */
> +#define RTL8211E_EEE_LED_MODE1 0x05
> +#define RTL8211E_EEE_LED_MODE2 0x06
> +
> #define RTL8211F_INSR 0x1d
>
> #define RTL8211F_TX_DELAY BIT(8)
> @@ -53,6 +58,35 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> }
>
> +static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> +{
You define return type int but AFAICS the return value is never used,
also in subsequent patches.
> + int ret = 0;
> + int oldpage;
> +
> + oldpage = phy_select_page(phydev, 5);
> + if (oldpage < 0)
> + goto out;
> +
> + /* write magic values to disable EEE LED mode */
> + ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
> + if (ret)
> + goto out;
> +
> + ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
> +
> +out:
> + return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> +static int rtl8211e_config_init(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> +
> + if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> + rtl8211e_disable_eee_led_mode(phydev);
> +
> + return 0;
> +}
I suppose checkpatch complains about the missing empty line.
You add it in a later patch, in case of a v3 you could fix that.
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
> @@ -310,6 +344,7 @@ static struct phy_driver realtek_drvs[] = {
> .name = "RTL8211E Gigabit Ethernet",
> .config_init = &rtl8211e_config_init,
> .ack_interrupt = &rtl821x_ack_interrupt,
> + .config_init = &rtl8211e_config_init,
> .config_intr = &rtl8211e_config_intr,
> .suspend = genphy_suspend,
> .resume = genphy_resume,
>
On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a helper for
> selecting an ext page.
>
> rtl8211e_modify_ext_paged() is inspired by its counterpart
> phy_modify_paged().
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - assign .read/write_page handlers for RTL8211E
Maybe this was planned, but it's not part of the patch.
> - use phy_select_page() and phy_restore_page(), get rid of
> rtl8211e_restore_page()
> - s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
> - updated commit message
> ---
> drivers/net/phy/realtek.c | 42 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 42 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index eb815cbe1e72..9cd6241e2a6d 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -27,6 +27,9 @@
> #define RTL821x_EXT_PAGE_SELECT 0x1e
> #define RTL821x_PAGE_SELECT 0x1f
>
> +#define RTL8211E_EXT_PAGE 7
> +#define RTL8211E_EPAGSR 0x1e
> +
> /* RTL8211E page 5 */
> #define RTL8211E_EEE_LED_MODE1 0x05
> #define RTL8211E_EEE_LED_MODE2 0x06
> @@ -58,6 +61,44 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> }
>
> +static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
> +{
> + int ret, oldpage;
> +
> + oldpage = phy_select_page(phydev, RTL8211E_EXT_PAGE);
> + if (oldpage < 0)
> + return oldpage;
> +
> + ret = __phy_write(phydev, RTL8211E_EPAGSR, page);
> + if (ret)
> + return phy_restore_page(phydev, page, ret);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> + int page, u32 regnum, u16 mask, u16 set)
This __maybe_unused isn't too nice as you use the function in a subsequent patch.
> +{
> + int ret = 0;
> + int oldpage;
> + int new;
> +
> + oldpage = rtl8211e_select_ext_page(phydev, page);
> + if (oldpage < 0)
> + goto out;
> +
> + ret = __phy_read(phydev, regnum);
> + if (ret < 0)
> + goto out;
> +
> + new = (ret & ~mask) | set;
> + if (new != ret)
> + ret = __phy_write(phydev, regnum, new);
> +
> +out:
> + return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> {
> int ret = 0;
> @@ -87,6 +128,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
>
> return 0;
> }
> +
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
>
Please repost this patch set with a proper "[PATCH 0/7] ..." header posting
describing at a high level what this patch series is doing, how it is doing
it, and why it is doing it that way.
Such header postings are absolutely essential for the proper understanding
and review of your changes.
Thank you.
On 03.07.2019 22:13, Heiner Kallweit wrote:
> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
>> The LED behavior of some Realtek PHYs is configurable. Add the
>> property 'realtek,led-modes' to specify the configuration of the
>> LEDs.
>>
>> Signed-off-by: Matthias Kaehlcke <[email protected]>
>> ---
>> Changes in v2:
>> - patch added to the series
>> ---
>> .../devicetree/bindings/net/realtek.txt | 9 +++++++++
>> include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
>> 2 files changed, 26 insertions(+)
>> create mode 100644 include/dt-bindings/net/realtek.h
>>
>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
>> index 71d386c78269..40b0d6f9ee21 100644
>> --- a/Documentation/devicetree/bindings/net/realtek.txt
>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
>> @@ -9,6 +9,12 @@ Optional properties:
>>
>> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>>
>> +- realtek,led-modes: LED mode configuration.
>> +
>> + A 0..3 element vector, with each element configuring the operating
>> + mode of an LED. Omitted LEDs are turned off. Allowed values are
>> + defined in "include/dt-bindings/net/realtek.h".
>> +
>> Example:
>>
>> mdio0 {
>> @@ -20,5 +26,8 @@ mdio0 {
>> reg = <1>;
>> realtek,eee-led-mode-disable;
>> realtek,enable-ssc;
>> + realtek,led-modes = <RTL8211E_LINK_ACTIVITY
>> + RTL8211E_LINK_100
>> + RTL8211E_LINK_1000>;
>> };
>> };
>> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
>> new file mode 100644
>> index 000000000000..8d64f58d58f8
>> --- /dev/null
>> +++ b/include/dt-bindings/net/realtek.h
>> @@ -0,0 +1,17 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _DT_BINDINGS_REALTEK_H
>> +#define _DT_BINDINGS_REALTEK_H
>> +
>> +/* LED modes for RTL8211E PHY */
>> +
>> +#define RTL8211E_LINK_10 1
>> +#define RTL8211E_LINK_100 2
>> +#define RTL8211E_LINK_1000 4
>> +#define RTL8211E_LINK_10_100 3
>> +#define RTL8211E_LINK_10_1000 5
>> +#define RTL8211E_LINK_100_1000 6
>> +#define RTL8211E_LINK_10_100_1000 7
>> +
>> +#define RTL8211E_LINK_ACTIVITY (1 << 16)
>
> I don't see where this is used.
>
Clear now, disregard my comment.
>> +
>> +#endif
>>
>
On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> Configure the RTL8211E LEDs behavior when the device tree property
> 'realtek,led-modes' is specified.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - patch added to the series
> ---
> drivers/net/phy/realtek.c | 63 +++++++++++++++++++++++++++++++++++++--
> 1 file changed, 61 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 45fee4612031..559aec547738 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,6 +9,7 @@
> * Copyright (c) 2004 Freescale Semiconductor, Inc.
> */
> #include <linux/bitops.h>
> +#include <linux/bits.h>
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/of.h>
> @@ -35,6 +36,15 @@
> #define RTL8211E_EEE_LED_MODE1 0x05
> #define RTL8211E_EEE_LED_MODE2 0x06
>
> +/* RTL8211E extension page 44 */
> +#define RTL8211E_LACR 0x1a
> +#define RLT8211E_LACR_LEDACTCTRL_SHIFT 4
> +#define RLT8211E_LACR_LEDACTCTRL_MASK GENMASK(6, 4)
> +#define RTL8211E_LCR 0x1c
> +#define RTL8211E_LCR_LEDCTRL_MASK (GENMASK(2, 0) | \
> + GENMASK(6, 4) | \
> + GENMASK(10, 8))
> +
> /* RTL8211E extension page 160 */
> #define RTL8211E_SCR 0x1a
> #define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
> @@ -124,6 +134,56 @@ static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> return phy_restore_page(phydev, oldpage, ret);
> }
>
> +static int rtl8211e_config_leds(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + int count, i, oldpage, ret;
> + u16 lacr_bits = 0, lcr_bits = 0;
> +
> + if (!dev->of_node)
> + return 0;
> +
> + if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> + rtl8211e_disable_eee_led_mode(phydev);
> +
> + count = of_property_count_elems_of_size(dev->of_node,
> + "realtek,led-modes",
> + sizeof(u32));
> + if (count < 0 || count > 3)
> + return -EINVAL;
> +
> + for (i = 0; i < count; i++) {
> + u32 val;
> +
> + of_property_read_u32_index(dev->of_node,
> + "realtek,led-modes", i, &val);
> + lacr_bits |= (u16)(val >> 16) <<
> + (RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
This may be done in an easier to read way:
if (val & RTL8211E_LINK_ACTIVITY)
lacr_bits |= BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
> + lcr_bits |= (u16)(val & 0xf) << (i * 4);
> + }
> +
> + oldpage = rtl8211e_select_ext_page(phydev, 44);
> + if (oldpage < 0) {
> + dev_err(dev, "failed to select extended page: %d\n", oldpage);
In a PHY driver it may be more appropriate to use phydev_err() et al,
even though effectively it's the same.
> + goto err;
> + }
> +
> + ret = __phy_modify(phydev, RTL8211E_LACR,
> + RLT8211E_LACR_LEDACTCTRL_MASK, lacr_bits);
> + if (ret) {
> + dev_err(dev, "failed to write LACR reg: %d\n", ret);
> + goto err;
> + }
> +
> + ret = __phy_modify(phydev, RTL8211E_LCR,
> + RTL8211E_LCR_LEDCTRL_MASK, lcr_bits);
> + if (ret)
> + dev_err(dev, "failed to write LCR reg: %d\n", ret);
> +
> +err:
> + return phy_restore_page(phydev, oldpage, ret);
> +}
> +
> static int rtl8211e_config_init(struct phy_device *phydev)
> {
> struct device *dev = &phydev->mdio.dev;
> @@ -137,8 +197,7 @@ static int rtl8211e_config_init(struct phy_device *phydev)
> dev_err(dev, "failed to enable SSC on RXC: %d\n", ret);
> }
>
> - if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> - rtl8211e_disable_eee_led_mode(phydev);
> + rtl8211e_config_leds(phydev);
>
> return 0;
> }
>
Hi,
On Wed, Jul 03, 2019 at 10:09:39PM +0200, Heiner Kallweit wrote:
> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> > EEE LED mode is enabled by default on the RTL8211E. Disable it when
> > the device tree property 'realtek,eee-led-mode-disable' exists.
> >
> > The magic values to disable EEE LED mode were taken from the RTL8211E
> > datasheet, unfortunately they are not further documented.
> >
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> > Changes in v2:
> > - patch added to the series
> > ---
> > drivers/net/phy/realtek.c | 37 ++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 36 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index a669945eb829..eb815cbe1e72 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -9,8 +9,9 @@
> > * Copyright (c) 2004 Freescale Semiconductor, Inc.
> > */
> > #include <linux/bitops.h>
> > -#include <linux/phy.h>
> > #include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/phy.h>
> >
> > #define RTL821x_PHYSR 0x11
> > #define RTL821x_PHYSR_DUPLEX BIT(13)
> > @@ -26,6 +27,10 @@
> > #define RTL821x_EXT_PAGE_SELECT 0x1e
> > #define RTL821x_PAGE_SELECT 0x1f
> >
> > +/* RTL8211E page 5 */
> > +#define RTL8211E_EEE_LED_MODE1 0x05
> > +#define RTL8211E_EEE_LED_MODE2 0x06
> > +
> > #define RTL8211F_INSR 0x1d
> >
> > #define RTL8211F_TX_DELAY BIT(8)
> > @@ -53,6 +58,35 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> > return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> > }
> >
> > +static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> > +{
>
> You define return type int but AFAICS the return value is never used,
> also in subsequent patches.
ok, I'll change it to void
> > + int ret = 0;
> > + int oldpage;
> > +
> > + oldpage = phy_select_page(phydev, 5);
> > + if (oldpage < 0)
> > + goto out;
> > +
> > + /* write magic values to disable EEE LED mode */
> > + ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE1, 0x8b82);
> > + if (ret)
> > + goto out;
> > +
> > + ret = __phy_write(phydev, RTL8211E_EEE_LED_MODE2, 0x052b);
> > +
> > +out:
> > + return phy_restore_page(phydev, oldpage, ret);
> > +}
> > +
> > +static int rtl8211e_config_init(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > +
> > + if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> > + rtl8211e_disable_eee_led_mode(phydev);
> > +
> > + return 0;
> > +}
>
> I suppose checkpatch complains about the missing empty line.
> You add it in a later patch, in case of a v3 you could fix that.
Actually checkpatch does not complain, I'll fix it in v3.
Thanks
Matthias
On Wed, Jul 03, 2019 at 10:12:12PM +0200, Heiner Kallweit wrote:
> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> > The RTL8211E has extension pages, which can be accessed after
> > selecting a page through a custom method. Add a function to
> > modify bits in a register of an extension page and a helper for
> > selecting an ext page.
> >
> > rtl8211e_modify_ext_paged() is inspired by its counterpart
> > phy_modify_paged().
> >
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> > Changes in v2:
> > - assign .read/write_page handlers for RTL8211E
>
> Maybe this was planned, but it's not part of the patch.
Oops, it was definitely there when I tested ... I guess this got
somehow lost when changing the patch order and resolving minor
conflicts, seems like I only build tested after that :/
> > - use phy_select_page() and phy_restore_page(), get rid of
> > rtl8211e_restore_page()
> > - s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
> > - updated commit message
> > ---
> > drivers/net/phy/realtek.c | 42 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 42 insertions(+)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index eb815cbe1e72..9cd6241e2a6d 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -27,6 +27,9 @@
> > #define RTL821x_EXT_PAGE_SELECT 0x1e
> > #define RTL821x_PAGE_SELECT 0x1f
> >
> > +#define RTL8211E_EXT_PAGE 7
> > +#define RTL8211E_EPAGSR 0x1e
> > +
> > /* RTL8211E page 5 */
> > #define RTL8211E_EEE_LED_MODE1 0x05
> > #define RTL8211E_EEE_LED_MODE2 0x06
> > @@ -58,6 +61,44 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> > return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> > }
> >
> > +static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
> > +{
> > + int ret, oldpage;
> > +
> > + oldpage = phy_select_page(phydev, RTL8211E_EXT_PAGE);
> > + if (oldpage < 0)
> > + return oldpage;
> > +
> > + ret = __phy_write(phydev, RTL8211E_EPAGSR, page);
> > + if (ret)
> > + return phy_restore_page(phydev, page, ret);
> > +
> > + return 0;
> > +}
> > +
> > +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> > + int page, u32 regnum, u16 mask, u16 set)
>
> This __maybe_unused isn't too nice as you use the function in a subsequent patch.
It's needed to avoid a compiler warning (unless we don't care about
that for an interim version), the attribute is removed again in the
next patch.
On Wed, Jul 03, 2019 at 10:10:32PM +0200, Andrew Lunn wrote:
> > + for (i = 0; i < count; i++) {
> > + u32 val;
> > +
> > + of_property_read_u32_index(dev->of_node,
> > + "realtek,led-modes", i, &val);
>
> Please validate the value, 0 - 7.
ok, will be 0-7 and 0x10000 - 0x10007 (w/ RTL8211E_LINK_ACTIVITY) though.
This is the somewhat quirky part about the property, each value
translates to two registers. This seemed to be the cleanest solution
from the bindings perspective, but I'm open to other suggestions.
On Wed, Jul 03, 2019 at 10:28:17PM +0200, Heiner Kallweit wrote:
> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> > Configure the RTL8211E LEDs behavior when the device tree property
> > 'realtek,led-modes' is specified.
> >
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> > Changes in v2:
> > - patch added to the series
> > ---
> > drivers/net/phy/realtek.c | 63 +++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 45fee4612031..559aec547738 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -9,6 +9,7 @@
> > * Copyright (c) 2004 Freescale Semiconductor, Inc.
> > */
> > #include <linux/bitops.h>
> > +#include <linux/bits.h>
> > #include <linux/device.h>
> > #include <linux/module.h>
> > #include <linux/of.h>
> > @@ -35,6 +36,15 @@
> > #define RTL8211E_EEE_LED_MODE1 0x05
> > #define RTL8211E_EEE_LED_MODE2 0x06
> >
> > +/* RTL8211E extension page 44 */
> > +#define RTL8211E_LACR 0x1a
> > +#define RLT8211E_LACR_LEDACTCTRL_SHIFT 4
> > +#define RLT8211E_LACR_LEDACTCTRL_MASK GENMASK(6, 4)
> > +#define RTL8211E_LCR 0x1c
> > +#define RTL8211E_LCR_LEDCTRL_MASK (GENMASK(2, 0) | \
> > + GENMASK(6, 4) | \
> > + GENMASK(10, 8))
> > +
> > /* RTL8211E extension page 160 */
> > #define RTL8211E_SCR 0x1a
> > #define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
> > @@ -124,6 +134,56 @@ static int rtl8211e_disable_eee_led_mode(struct phy_device *phydev)
> > return phy_restore_page(phydev, oldpage, ret);
> > }
> >
> > +static int rtl8211e_config_leds(struct phy_device *phydev)
> > +{
> > + struct device *dev = &phydev->mdio.dev;
> > + int count, i, oldpage, ret;
> > + u16 lacr_bits = 0, lcr_bits = 0;
> > +
> > + if (!dev->of_node)
> > + return 0;
> > +
> > + if (of_property_read_bool(dev->of_node, "realtek,eee-led-mode-disable"))
> > + rtl8211e_disable_eee_led_mode(phydev);
> > +
> > + count = of_property_count_elems_of_size(dev->of_node,
> > + "realtek,led-modes",
> > + sizeof(u32));
> > + if (count < 0 || count > 3)
> > + return -EINVAL;
> > +
> > + for (i = 0; i < count; i++) {
> > + u32 val;
> > +
> > + of_property_read_u32_index(dev->of_node,
> > + "realtek,led-modes", i, &val);
> > + lacr_bits |= (u16)(val >> 16) <<
> > + (RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
>
> This may be done in an easier to read way:
>
> if (val & RTL8211E_LINK_ACTIVITY)
> lacr_bits |= BIT(RLT8211E_LACR_LEDACTCTRL_SHIFT + i);
indeed, that's more readable, thanks for the suggestion!
> > + lcr_bits |= (u16)(val & 0xf) << (i * 4);
> > + }
> > +
> > + oldpage = rtl8211e_select_ext_page(phydev, 44);
> > + if (oldpage < 0) {
> > + dev_err(dev, "failed to select extended page: %d\n", oldpage);
>
> In a PHY driver it may be more appropriate to use phydev_err() et al,
> even though effectively it's the same.
sounds good, I'll change it to the phydev_err() et al.
Thanks for the reviews!
Matthias
On 03.07.2019 22:36, Matthias Kaehlcke wrote:
> On Wed, Jul 03, 2019 at 10:12:12PM +0200, Heiner Kallweit wrote:
>> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
>>> The RTL8211E has extension pages, which can be accessed after
>>> selecting a page through a custom method. Add a function to
>>> modify bits in a register of an extension page and a helper for
>>> selecting an ext page.
>>>
>>> rtl8211e_modify_ext_paged() is inspired by its counterpart
>>> phy_modify_paged().
>>>
>>> Signed-off-by: Matthias Kaehlcke <[email protected]>
>>> ---
>>> Changes in v2:
>>> - assign .read/write_page handlers for RTL8211E
>>
>> Maybe this was planned, but it's not part of the patch.
>
> Oops, it was definitely there when I tested ... I guess this got
> somehow lost when changing the patch order and resolving minor
> conflicts, seems like I only build tested after that :/
>
RTL8211E also supports normal pages (reg 0x1f = page).
See e.g. rtl8168e_2_hw_phy_config in the r8169 driver, this network
chip has an integrated RTL8211E PHY. There settings on page 3 and 5
are done.
Therefore I would prefer to use .read/write_page for normal paging
in all Realtek PHY drivers. Means the code here would remain as it
is and just the changelog would need to be fixed.
>>> - use phy_select_page() and phy_restore_page(), get rid of
>>> rtl8211e_restore_page()
>>> - s/rtl821e_select_ext_page/rtl8211e_select_ext_page/
>>> - updated commit message
>>> ---
>>> drivers/net/phy/realtek.c | 42 +++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 42 insertions(+)
>>>
>>> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
>>> index eb815cbe1e72..9cd6241e2a6d 100644
>>> --- a/drivers/net/phy/realtek.c
>>> +++ b/drivers/net/phy/realtek.c
>>> @@ -27,6 +27,9 @@
>>> #define RTL821x_EXT_PAGE_SELECT 0x1e
>>> #define RTL821x_PAGE_SELECT 0x1f
>>>
>>> +#define RTL8211E_EXT_PAGE 7
>>> +#define RTL8211E_EPAGSR 0x1e
>>> +
>>> /* RTL8211E page 5 */
>>> #define RTL8211E_EEE_LED_MODE1 0x05
>>> #define RTL8211E_EEE_LED_MODE2 0x06
>>> @@ -58,6 +61,44 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>>> return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>>> }
>>>
>>> +static int rtl8211e_select_ext_page(struct phy_device *phydev, int page)
>>> +{
>>> + int ret, oldpage;
>>> +
>>> + oldpage = phy_select_page(phydev, RTL8211E_EXT_PAGE);
>>> + if (oldpage < 0)
>>> + return oldpage;
>>> +
>>> + ret = __phy_write(phydev, RTL8211E_EPAGSR, page);
>>> + if (ret)
>>> + return phy_restore_page(phydev, page, ret);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
>>> + int page, u32 regnum, u16 mask, u16 set)
>>
>> This __maybe_unused isn't too nice as you use the function in a subsequent patch.
>
> It's needed to avoid a compiler warning (unless we don't care about
> that for an interim version), the attribute is removed again in the
> next patch.
>
OK
On Wed, Jul 3, 2019 at 1:37 PM Matthias Kaehlcke <[email protected]> wrote:
>
> Add the 'realtek,eee-led-mode-disable' property to disable EEE
> LED mode on Realtek PHYs that support it.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - document 'realtek,eee-led-mode-disable' instead of
> 'realtek,enable-ssc' in the initial version
> ---
> .../devicetree/bindings/net/realtek.txt | 19 +++++++++++++++++++
> 1 file changed, 19 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/realtek.txt
>
> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> new file mode 100644
> index 000000000000..63f7002fa704
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> @@ -0,0 +1,19 @@
> +Realtek PHY properties.
> +
> +This document describes properties of Realtek PHYs.
> +
> +Optional properties:
> +- realtek,eee-led-mode-disable: Disable EEE LED mode on this port.
> +
> +Example:
> +
> +mdio0 {
> + compatible = "snps,dwmac-mdio";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy: ethernet-phy@1 {
> + reg = <1>;
> + realtek,eee-led-mode-disable;
I think if we're going to have custom properties for phys, we should
have a compatible string to at least validate whether the custom
properties are even valid for the node.
Rob
On Wed, Jul 03, 2019 at 11:01:09PM +0200, Heiner Kallweit wrote:
> On 03.07.2019 22:36, Matthias Kaehlcke wrote:
> > On Wed, Jul 03, 2019 at 10:12:12PM +0200, Heiner Kallweit wrote:
> >> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> >>> The RTL8211E has extension pages, which can be accessed after
> >>> selecting a page through a custom method. Add a function to
> >>> modify bits in a register of an extension page and a helper for
> >>> selecting an ext page.
> >>>
> >>> rtl8211e_modify_ext_paged() is inspired by its counterpart
> >>> phy_modify_paged().
> >>>
> >>> Signed-off-by: Matthias Kaehlcke <[email protected]>
> >>> ---
> >>> Changes in v2:
> >>> - assign .read/write_page handlers for RTL8211E
> >>
> >> Maybe this was planned, but it's not part of the patch.
> >
> > Oops, it was definitely there when I tested ... I guess this got
> > somehow lost when changing the patch order and resolving minor
> > conflicts, seems like I only build tested after that :/
> >
> RTL8211E also supports normal pages (reg 0x1f = page).
> See e.g. rtl8168e_2_hw_phy_config in the r8169 driver, this network
> chip has an integrated RTL8211E PHY. There settings on page 3 and 5
> are done.
> Therefore I would prefer to use .read/write_page for normal paging
> in all Realtek PHY drivers. Means the code here would remain as it
> is and just the changelog would need to be fixed.
Do I understand correctly that you suggest an additional patch that
assigns .read/write_page() for all entries of realtek_drvs?
On 03.07.2019 23:24, Matthias Kaehlcke wrote:
> On Wed, Jul 03, 2019 at 11:01:09PM +0200, Heiner Kallweit wrote:
>> On 03.07.2019 22:36, Matthias Kaehlcke wrote:
>>> On Wed, Jul 03, 2019 at 10:12:12PM +0200, Heiner Kallweit wrote:
>>>> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
>>>>> The RTL8211E has extension pages, which can be accessed after
>>>>> selecting a page through a custom method. Add a function to
>>>>> modify bits in a register of an extension page and a helper for
>>>>> selecting an ext page.
>>>>>
>>>>> rtl8211e_modify_ext_paged() is inspired by its counterpart
>>>>> phy_modify_paged().
>>>>>
>>>>> Signed-off-by: Matthias Kaehlcke <[email protected]>
>>>>> ---
>>>>> Changes in v2:
>>>>> - assign .read/write_page handlers for RTL8211E
>>>>
>>>> Maybe this was planned, but it's not part of the patch.
>>>
>>> Oops, it was definitely there when I tested ... I guess this got
>>> somehow lost when changing the patch order and resolving minor
>>> conflicts, seems like I only build tested after that :/
>>>
>> RTL8211E also supports normal pages (reg 0x1f = page).
>> See e.g. rtl8168e_2_hw_phy_config in the r8169 driver, this network
>> chip has an integrated RTL8211E PHY. There settings on page 3 and 5
>> are done.
>> Therefore I would prefer to use .read/write_page for normal paging
>> in all Realtek PHY drivers. Means the code here would remain as it
>> is and just the changelog would need to be fixed.
>
> Do I understand correctly that you suggest an additional patch that
> assigns .read/write_page() for all entries of realtek_drvs?
>
No, basically all the Realtek PHY drivers use the following already:
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
What I mean is that this should stay as it is, and not be overwritten
with the extended paging.
> I think if we're going to have custom properties for phys, we should
> have a compatible string to at least validate whether the custom
> properties are even valid for the node.
Hi Rob
What happens with other enumerable busses where a compatible string is
not used?
The Ethernet PHY subsystem will ignore the compatible string and load
the driver which fits the enumeration data. Using the compatible
string only to get the right YAML validator seems wrong. I would
prefer adding some other property with a clear name indicates its is
selecting the validator, and has nothing to do with loading the
correct driver. And it can then be used as well for USB and PCI
devices etc.
Andrew
On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> The LED behavior of some Realtek PHYs is configurable. Add the
> property 'realtek,led-modes' to specify the configuration of the
> LEDs.
>
> Signed-off-by: Matthias Kaehlcke <[email protected]>
> ---
> Changes in v2:
> - patch added to the series
> ---
> .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> 2 files changed, 26 insertions(+)
> create mode 100644 include/dt-bindings/net/realtek.h
>
> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> index 71d386c78269..40b0d6f9ee21 100644
> --- a/Documentation/devicetree/bindings/net/realtek.txt
> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> @@ -9,6 +9,12 @@ Optional properties:
>
> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>
> +- realtek,led-modes: LED mode configuration.
> +
> + A 0..3 element vector, with each element configuring the operating
> + mode of an LED. Omitted LEDs are turned off. Allowed values are
> + defined in "include/dt-bindings/net/realtek.h".
This should probably be made more general and we should define LED modes
that makes sense regardless of the PHY device, introduce a set of
generic functions for validating and then add new function pointer for
setting the LED configuration to the PHY driver. This would allow to be
more future proof where each PHY driver could expose standard LEDs class
devices to user-space, and it would also allow facilities like: ethtool
-p to plug into that.
Right now, each driver invents its own way of configuring LEDs, that
does not scale, and there is not really a good reason for that other
than reviewing drivers in isolation and therefore making it harder to
extract the commonality. Yes, I realize that since you are the latest
person submitting something in that area, you are being selected :)
> +
> Example:
>
> mdio0 {
> @@ -20,5 +26,8 @@ mdio0 {
> reg = <1>;
> realtek,eee-led-mode-disable;
> realtek,enable-ssc;
> + realtek,led-modes = <RTL8211E_LINK_ACTIVITY
> + RTL8211E_LINK_100
> + RTL8211E_LINK_1000>;
> };
> };
> diff --git a/include/dt-bindings/net/realtek.h b/include/dt-bindings/net/realtek.h
> new file mode 100644
> index 000000000000..8d64f58d58f8
> --- /dev/null
> +++ b/include/dt-bindings/net/realtek.h
> @@ -0,0 +1,17 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _DT_BINDINGS_REALTEK_H
> +#define _DT_BINDINGS_REALTEK_H
> +
> +/* LED modes for RTL8211E PHY */
> +
> +#define RTL8211E_LINK_10 1
> +#define RTL8211E_LINK_100 2
> +#define RTL8211E_LINK_1000 4
> +#define RTL8211E_LINK_10_100 3
> +#define RTL8211E_LINK_10_1000 5
> +#define RTL8211E_LINK_100_1000 6
> +#define RTL8211E_LINK_10_100_1000 7
> +
> +#define RTL8211E_LINK_ACTIVITY (1 << 16)
> +
> +#endif
>
--
Florian
On Wed, Jul 03, 2019 at 11:33:27PM +0200, Andrew Lunn wrote:
> > I think if we're going to have custom properties for phys, we should
> > have a compatible string to at least validate whether the custom
> > properties are even valid for the node.
>
> Hi Rob
>
> What happens with other enumerable busses where a compatible string is
> not used?
>
> The Ethernet PHY subsystem will ignore the compatible string and load
> the driver which fits the enumeration data. Using the compatible
> string only to get the right YAML validator seems wrong. I would
> prefer adding some other property with a clear name indicates its is
> selecting the validator, and has nothing to do with loading the
> correct driver. And it can then be used as well for USB and PCI
> devices etc.
I also have doubts whether a compatible string is the right answer
here. It's not needed/used by the subsystem, but would it be a
required property because it's needed for validation?
On Wed, Jul 03, 2019 at 11:27:41PM +0200, Heiner Kallweit wrote:
> On 03.07.2019 23:24, Matthias Kaehlcke wrote:
> > On Wed, Jul 03, 2019 at 11:01:09PM +0200, Heiner Kallweit wrote:
> >> On 03.07.2019 22:36, Matthias Kaehlcke wrote:
> >>> On Wed, Jul 03, 2019 at 10:12:12PM +0200, Heiner Kallweit wrote:
> >>>> On 03.07.2019 21:37, Matthias Kaehlcke wrote:
> >>>>> The RTL8211E has extension pages, which can be accessed after
> >>>>> selecting a page through a custom method. Add a function to
> >>>>> modify bits in a register of an extension page and a helper for
> >>>>> selecting an ext page.
> >>>>>
> >>>>> rtl8211e_modify_ext_paged() is inspired by its counterpart
> >>>>> phy_modify_paged().
> >>>>>
> >>>>> Signed-off-by: Matthias Kaehlcke <[email protected]>
> >>>>> ---
> >>>>> Changes in v2:
> >>>>> - assign .read/write_page handlers for RTL8211E
> >>>>
> >>>> Maybe this was planned, but it's not part of the patch.
> >>>
> >>> Oops, it was definitely there when I tested ... I guess this got
> >>> somehow lost when changing the patch order and resolving minor
> >>> conflicts, seems like I only build tested after that :/
> >>>
> >> RTL8211E also supports normal pages (reg 0x1f = page).
> >> See e.g. rtl8168e_2_hw_phy_config in the r8169 driver, this network
> >> chip has an integrated RTL8211E PHY. There settings on page 3 and 5
> >> are done.
> >> Therefore I would prefer to use .read/write_page for normal paging
> >> in all Realtek PHY drivers. Means the code here would remain as it
> >> is and just the changelog would need to be fixed.
> >
> > Do I understand correctly that you suggest an additional patch that
> > assigns .read/write_page() for all entries of realtek_drvs?
> >
>
> No, basically all the Realtek PHY drivers use the following already:
> .read_page = rtl821x_read_page,
> .write_page = rtl821x_write_page,
> What I mean is that this should stay as it is, and not be overwritten
> with the extended paging.
I now see the source of our/my misunderstanding. I'm working on a 4.19
kernel, which doesn't have your recent patch:
commit daf3ddbe11a2ff74c95bc814df8e5fe3201b4cb5
Author: Heiner Kallweit <[email protected]>
Date: Fri May 10 22:11:26 2019 +0200
net: phy: realtek: add missing page operations
That's what I intended to do for RTL8211E, no need to overwrite it
with the extended paging.
Thanks
Matthias
Hi Florian,
On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > The LED behavior of some Realtek PHYs is configurable. Add the
> > property 'realtek,led-modes' to specify the configuration of the
> > LEDs.
> >
> > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > ---
> > Changes in v2:
> > - patch added to the series
> > ---
> > .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> > include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> > 2 files changed, 26 insertions(+)
> > create mode 100644 include/dt-bindings/net/realtek.h
> >
> > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > index 71d386c78269..40b0d6f9ee21 100644
> > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > @@ -9,6 +9,12 @@ Optional properties:
> >
> > SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> >
> > +- realtek,led-modes: LED mode configuration.
> > +
> > + A 0..3 element vector, with each element configuring the operating
> > + mode of an LED. Omitted LEDs are turned off. Allowed values are
> > + defined in "include/dt-bindings/net/realtek.h".
>
> This should probably be made more general and we should define LED modes
> that makes sense regardless of the PHY device, introduce a set of
> generic functions for validating and then add new function pointer for
> setting the LED configuration to the PHY driver. This would allow to be
> more future proof where each PHY driver could expose standard LEDs class
> devices to user-space, and it would also allow facilities like: ethtool
> -p to plug into that.
>
> Right now, each driver invents its own way of configuring LEDs, that
> does not scale, and there is not really a good reason for that other
> than reviewing drivers in isolation and therefore making it harder to
> extract the commonality. Yes, I realize that since you are the latest
> person submitting something in that area, you are being selected :)
I see the merit of your proposal to come up with a generic mechanism
to configure Ethernet LEDs, however I can't justify spending much of
my work time on this. If it is deemed useful I'm happy to send another
version of the current patchset that addresses the reviewer's comments,
but if the implementation of a generic LED configuration interface is
a requirement I will have to abandon at least the LED configuration
part of this series.
On Wed, Jul 3, 2019 at 3:33 PM Andrew Lunn <[email protected]> wrote:
>
> > I think if we're going to have custom properties for phys, we should
> > have a compatible string to at least validate whether the custom
> > properties are even valid for the node.
>
> Hi Rob
>
> What happens with other enumerable busses where a compatible string is
> not used?
We usually have a compatible. USB and PCI both do. Sometimes it is a
defined format based on VID/PID.
> The Ethernet PHY subsystem will ignore the compatible string and load
> the driver which fits the enumeration data. Using the compatible
> string only to get the right YAML validator seems wrong. I would
> prefer adding some other property with a clear name indicates its is
> selecting the validator, and has nothing to do with loading the
> correct driver. And it can then be used as well for USB and PCI
> devices etc.
Just because Linux happens to not use compatible really has nothing to
do with whether or not the nodes should have a compatible. What does
FreeBSD want? U-boot?
I don't follow how adding a validate property would help. It would
need to be 'validate-node-as-a-realtek-phy'. The schema selection is
done for each schema on a node by node basis and has to be based on
some data in the node (or always applied). Using compatible or node
name are just the default way.
Rob
On Fri, Jul 05, 2019 at 10:17:16AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 3:33 PM Andrew Lunn <[email protected]> wrote:
> >
> > > I think if we're going to have custom properties for phys, we should
> > > have a compatible string to at least validate whether the custom
> > > properties are even valid for the node.
> >
> > Hi Rob
> >
> > What happens with other enumerable busses where a compatible string is
> > not used?
>
> We usually have a compatible. USB and PCI both do. Sometimes it is a
> defined format based on VID/PID.
Hi Rob
Is it defined what to do with this compatible? Just totally ignore it?
Validate it against the hardware and warning if it is wrong? Force
load the driver that implements the compatible, even thought bus
enumeration says it is the wrong driver?
> > The Ethernet PHY subsystem will ignore the compatible string and load
> > the driver which fits the enumeration data. Using the compatible
> > string only to get the right YAML validator seems wrong. I would
> > prefer adding some other property with a clear name indicates its is
> > selecting the validator, and has nothing to do with loading the
> > correct driver. And it can then be used as well for USB and PCI
> > devices etc.
>
> Just because Linux happens to not use compatible really has nothing to
> do with whether or not the nodes should have a compatible. What does
> FreeBSD want? U-boot?
>
> I don't follow how adding a validate property would help. It would
> need to be 'validate-node-as-a-realtek-phy'.
This makes it clear it is all about validating the DT, and nothing
about the actual running hardware. What i don't really want to see is
the poorly defined situation that DT contains a compatible string, but
we have no idea what it is actually used for. See the question above.
Andrew
On Wed, Jul 3, 2019 at 4:08 PM Matthias Kaehlcke <[email protected]> wrote:
>
> On Wed, Jul 03, 2019 at 11:33:27PM +0200, Andrew Lunn wrote:
> > > I think if we're going to have custom properties for phys, we should
> > > have a compatible string to at least validate whether the custom
> > > properties are even valid for the node.
> >
> > Hi Rob
> >
> > What happens with other enumerable busses where a compatible string is
> > not used?
> >
> > The Ethernet PHY subsystem will ignore the compatible string and load
> > the driver which fits the enumeration data. Using the compatible
> > string only to get the right YAML validator seems wrong. I would
> > prefer adding some other property with a clear name indicates its is
> > selecting the validator, and has nothing to do with loading the
> > correct driver. And it can then be used as well for USB and PCI
> > devices etc.
>
> I also have doubts whether a compatible string is the right answer
> here. It's not needed/used by the subsystem, but would it be a
> required property because it's needed for validation?
It could be required only for phy's with vendor specific properties.
On Fri, Jul 5, 2019 at 10:29 AM Andrew Lunn <[email protected]> wrote:
>
> On Fri, Jul 05, 2019 at 10:17:16AM -0600, Rob Herring wrote:
> > On Wed, Jul 3, 2019 at 3:33 PM Andrew Lunn <[email protected]> wrote:
> > >
> > > > I think if we're going to have custom properties for phys, we should
> > > > have a compatible string to at least validate whether the custom
> > > > properties are even valid for the node.
> > >
> > > Hi Rob
> > >
> > > What happens with other enumerable busses where a compatible string is
> > > not used?
> >
> > We usually have a compatible. USB and PCI both do. Sometimes it is a
> > defined format based on VID/PID.
>
> Hi Rob
>
> Is it defined what to do with this compatible? Just totally ignore it?
> Validate it against the hardware and warning if it is wrong? Force
> load the driver that implements the compatible, even thought bus
> enumeration says it is the wrong driver?
The short answer is either the problems get fixed or if DTs exist and
need to be supported which are wrong then the OS deals with the
problem to make things work as desired (see PowerMac code).
If the ethernet phy subsystem wants to ignore compatible, that is totally fine.
> > > The Ethernet PHY subsystem will ignore the compatible string and load
> > > the driver which fits the enumeration data. Using the compatible
> > > string only to get the right YAML validator seems wrong. I would
> > > prefer adding some other property with a clear name indicates its is
> > > selecting the validator, and has nothing to do with loading the
> > > correct driver. And it can then be used as well for USB and PCI
> > > devices etc.
> >
> > Just because Linux happens to not use compatible really has nothing to
> > do with whether or not the nodes should have a compatible. What does
> > FreeBSD want? U-boot?
> >
> > I don't follow how adding a validate property would help. It would
> > need to be 'validate-node-as-a-realtek-phy'.
>
> This makes it clear it is all about validating the DT, and nothing
> about the actual running hardware. What i don't really want to see is
> the poorly defined situation that DT contains a compatible string, but
> we have no idea what it is actually used for. See the question above.
What's poorly defined are the current bindings, type definitions of
properties, and what properties are valid or not in specific nodes. If
we only had to better define the rules around compatible use or
mismatches, we'd be a lot better off.
I'm not going to add properties solely for validation when we already
have a well defined, 15 year+ pattern for defining what a node
contains that practically every other subsystem and node uses. I guess
we just won't worry about validating ethernet phy nodes beyond some
basic checks.
Rob
On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <[email protected]> wrote:
>
> Hi Florian,
>
> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > property 'realtek,led-modes' to specify the configuration of the
> > > LEDs.
> > >
> > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > ---
> > > Changes in v2:
> > > - patch added to the series
> > > ---
> > > .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> > > include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> > > 2 files changed, 26 insertions(+)
> > > create mode 100644 include/dt-bindings/net/realtek.h
> > >
> > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > index 71d386c78269..40b0d6f9ee21 100644
> > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > @@ -9,6 +9,12 @@ Optional properties:
> > >
> > > SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > >
> > > +- realtek,led-modes: LED mode configuration.
> > > +
> > > + A 0..3 element vector, with each element configuring the operating
> > > + mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > + defined in "include/dt-bindings/net/realtek.h".
> >
> > This should probably be made more general and we should define LED modes
> > that makes sense regardless of the PHY device, introduce a set of
> > generic functions for validating and then add new function pointer for
> > setting the LED configuration to the PHY driver. This would allow to be
> > more future proof where each PHY driver could expose standard LEDs class
> > devices to user-space, and it would also allow facilities like: ethtool
> > -p to plug into that.
> >
> > Right now, each driver invents its own way of configuring LEDs, that
> > does not scale, and there is not really a good reason for that other
> > than reviewing drivers in isolation and therefore making it harder to
> > extract the commonality. Yes, I realize that since you are the latest
> > person submitting something in that area, you are being selected :)
I agree.
> I see the merit of your proposal to come up with a generic mechanism
> to configure Ethernet LEDs, however I can't justify spending much of
> my work time on this. If it is deemed useful I'm happy to send another
> version of the current patchset that addresses the reviewer's comments,
> but if the implementation of a generic LED configuration interface is
> a requirement I will have to abandon at least the LED configuration
> part of this series.
Can you at least define a common binding for this. Maybe that's just
removing 'realtek'. While the kernel side can evolve to a common
infrastructure, the DT bindings can't.
Rob
On 7/10/19 8:55 AM, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <[email protected]> wrote:
>>
>> Hi Florian,
>>
>> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
>>> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
>>>> The LED behavior of some Realtek PHYs is configurable. Add the
>>>> property 'realtek,led-modes' to specify the configuration of the
>>>> LEDs.
>>>>
>>>> Signed-off-by: Matthias Kaehlcke <[email protected]>
>>>> ---
>>>> Changes in v2:
>>>> - patch added to the series
>>>> ---
>>>> .../devicetree/bindings/net/realtek.txt | 9 +++++++++
>>>> include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
>>>> 2 files changed, 26 insertions(+)
>>>> create mode 100644 include/dt-bindings/net/realtek.h
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
>>>> index 71d386c78269..40b0d6f9ee21 100644
>>>> --- a/Documentation/devicetree/bindings/net/realtek.txt
>>>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
>>>> @@ -9,6 +9,12 @@ Optional properties:
>>>>
>>>> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
>>>>
>>>> +- realtek,led-modes: LED mode configuration.
>>>> +
>>>> + A 0..3 element vector, with each element configuring the operating
>>>> + mode of an LED. Omitted LEDs are turned off. Allowed values are
>>>> + defined in "include/dt-bindings/net/realtek.h".
>>>
>>> This should probably be made more general and we should define LED modes
>>> that makes sense regardless of the PHY device, introduce a set of
>>> generic functions for validating and then add new function pointer for
>>> setting the LED configuration to the PHY driver. This would allow to be
>>> more future proof where each PHY driver could expose standard LEDs class
>>> devices to user-space, and it would also allow facilities like: ethtool
>>> -p to plug into that.
>>>
>>> Right now, each driver invents its own way of configuring LEDs, that
>>> does not scale, and there is not really a good reason for that other
>>> than reviewing drivers in isolation and therefore making it harder to
>>> extract the commonality. Yes, I realize that since you are the latest
>>> person submitting something in that area, you are being selected :)
>
> I agree.
>
>> I see the merit of your proposal to come up with a generic mechanism
>> to configure Ethernet LEDs, however I can't justify spending much of
>> my work time on this. If it is deemed useful I'm happy to send another
>> version of the current patchset that addresses the reviewer's comments,
>> but if the implementation of a generic LED configuration interface is
>> a requirement I will have to abandon at least the LED configuration
>> part of this series.
>
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.
That would be a great start, and that is actually what I had in mind
(should have been more specific), I was not going to have you Matthias
do the grand slam and convert all this LED configuration into the LEDs
class etc. that would not be fair.
It seems to be that we can fairly easily agree on a common binding for
LED configuration, I would define something along those lines to be
flexible:
phy-led-configuration = <LED_NUM_MASK LED_CFG_MASK>;
where LED_NUM_MASK is one of:
0 -> link
1 -> activity
2 -> speed
that way you can define single/dual/triple LED configurations by
updating the bitmask.
LED_CFG_MASK is one of:
0 -> LED_CFG_10
1 -> LED_CFG_100
2 -> LED_CFG_1000
(let's assume 1Gbps or less for now)
or this can be combined in a single cell with a left shift.
Andrew, Heiner, do you see that approach working correctly and scaling
appropriately?
--
Florian
On Wed, Jul 10, 2019 at 09:55:12AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <[email protected]> wrote:
> >
> > Hi Florian,
> >
> > On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > > property 'realtek,led-modes' to specify the configuration of the
> > > > LEDs.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > > ---
> > > > Changes in v2:
> > > > - patch added to the series
> > > > ---
> > > > .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> > > > include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> > > > 2 files changed, 26 insertions(+)
> > > > create mode 100644 include/dt-bindings/net/realtek.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > > index 71d386c78269..40b0d6f9ee21 100644
> > > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > > @@ -9,6 +9,12 @@ Optional properties:
> > > >
> > > > SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > > >
> > > > +- realtek,led-modes: LED mode configuration.
> > > > +
> > > > + A 0..3 element vector, with each element configuring the operating
> > > > + mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > > + defined in "include/dt-bindings/net/realtek.h".
> > >
> > > This should probably be made more general and we should define LED modes
> > > that makes sense regardless of the PHY device, introduce a set of
> > > generic functions for validating and then add new function pointer for
> > > setting the LED configuration to the PHY driver. This would allow to be
> > > more future proof where each PHY driver could expose standard LEDs class
> > > devices to user-space, and it would also allow facilities like: ethtool
> > > -p to plug into that.
> > >
> > > Right now, each driver invents its own way of configuring LEDs, that
> > > does not scale, and there is not really a good reason for that other
> > > than reviewing drivers in isolation and therefore making it harder to
> > > extract the commonality. Yes, I realize that since you are the latest
> > > person submitting something in that area, you are being selected :)
>
> I agree.
>
> > I see the merit of your proposal to come up with a generic mechanism
> > to configure Ethernet LEDs, however I can't justify spending much of
> > my work time on this. If it is deemed useful I'm happy to send another
> > version of the current patchset that addresses the reviewer's comments,
> > but if the implementation of a generic LED configuration interface is
> > a requirement I will have to abandon at least the LED configuration
> > part of this series.
>
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.
Defining a common binding sounds good to me, I will follow up on
Florian's reply to this.
Hi Florian,
On Wed, Jul 10, 2019 at 09:28:39AM -0700, Florian Fainelli wrote:
> On 7/10/19 8:55 AM, Rob Herring wrote:
> > On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <[email protected]> wrote:
> >>
> >> Hi Florian,
> >>
> >> On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> >>> On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> >>>> The LED behavior of some Realtek PHYs is configurable. Add the
> >>>> property 'realtek,led-modes' to specify the configuration of the
> >>>> LEDs.
> >>>>
> >>>> Signed-off-by: Matthias Kaehlcke <[email protected]>
> >>>> ---
> >>>> Changes in v2:
> >>>> - patch added to the series
> >>>> ---
> >>>> .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> >>>> include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> >>>> 2 files changed, 26 insertions(+)
> >>>> create mode 100644 include/dt-bindings/net/realtek.h
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> index 71d386c78269..40b0d6f9ee21 100644
> >>>> --- a/Documentation/devicetree/bindings/net/realtek.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/realtek.txt
> >>>> @@ -9,6 +9,12 @@ Optional properties:
> >>>>
> >>>> SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> >>>>
> >>>> +- realtek,led-modes: LED mode configuration.
> >>>> +
> >>>> + A 0..3 element vector, with each element configuring the operating
> >>>> + mode of an LED. Omitted LEDs are turned off. Allowed values are
> >>>> + defined in "include/dt-bindings/net/realtek.h".
> >>>
> >>> This should probably be made more general and we should define LED modes
> >>> that makes sense regardless of the PHY device, introduce a set of
> >>> generic functions for validating and then add new function pointer for
> >>> setting the LED configuration to the PHY driver. This would allow to be
> >>> more future proof where each PHY driver could expose standard LEDs class
> >>> devices to user-space, and it would also allow facilities like: ethtool
> >>> -p to plug into that.
> >>>
> >>> Right now, each driver invents its own way of configuring LEDs, that
> >>> does not scale, and there is not really a good reason for that other
> >>> than reviewing drivers in isolation and therefore making it harder to
> >>> extract the commonality. Yes, I realize that since you are the latest
> >>> person submitting something in that area, you are being selected :)
> >
> > I agree.
> >
> >> I see the merit of your proposal to come up with a generic mechanism
> >> to configure Ethernet LEDs, however I can't justify spending much of
> >> my work time on this. If it is deemed useful I'm happy to send another
> >> version of the current patchset that addresses the reviewer's comments,
> >> but if the implementation of a generic LED configuration interface is
> >> a requirement I will have to abandon at least the LED configuration
> >> part of this series.
> >
> > Can you at least define a common binding for this. Maybe that's just
> > removing 'realtek'. While the kernel side can evolve to a common
> > infrastructure, the DT bindings can't.
>
> That would be a great start, and that is actually what I had in mind
> (should have been more specific), I was not going to have you Matthias
> do the grand slam and convert all this LED configuration into the LEDs
> class etc. that would not be fair.
>
> It seems to be that we can fairly easily agree on a common binding for
> LED configuration, I would define something along those lines to be
> flexible:
>
> phy-led-configuration = <LED_NUM_MASK LED_CFG_MASK>;
>
> where LED_NUM_MASK is one of:
>
> 0 -> link
> 1 -> activity
> 2 -> speed
I don't understand this proposal completely. Is LED_NUM_MASK actually
a mask/set (potentially containing multiple LEDs) or is it "one of"
the LEDs?
Are you suggesting to assign each LED a specific role (link, activity,
speed)?
Could you maybe post a specific example involving multiple LEDs?
Thanks
Matthias
> that way you can define single/dual/triple LED configurations by
> updating the bitmask.
>
> LED_CFG_MASK is one of:
>
> 0 -> LED_CFG_10
> 1 -> LED_CFG_100
> 2 -> LED_CFG_1000
>
> (let's assume 1Gbps or less for now)
>
> or this can be combined in a single cell with a left shift.
>
> Andrew, Heiner, do you see that approach working correctly and scaling
> appropriately?
On Wed, Jul 10, 2019 at 09:55:12AM -0600, Rob Herring wrote:
> On Wed, Jul 3, 2019 at 5:23 PM Matthias Kaehlcke <[email protected]> wrote:
> >
> > Hi Florian,
> >
> > On Wed, Jul 03, 2019 at 02:37:47PM -0700, Florian Fainelli wrote:
> > > On 7/3/19 12:37 PM, Matthias Kaehlcke wrote:
> > > > The LED behavior of some Realtek PHYs is configurable. Add the
> > > > property 'realtek,led-modes' to specify the configuration of the
> > > > LEDs.
> > > >
> > > > Signed-off-by: Matthias Kaehlcke <[email protected]>
> > > > ---
> > > > Changes in v2:
> > > > - patch added to the series
> > > > ---
> > > > .../devicetree/bindings/net/realtek.txt | 9 +++++++++
> > > > include/dt-bindings/net/realtek.h | 17 +++++++++++++++++
> > > > 2 files changed, 26 insertions(+)
> > > > create mode 100644 include/dt-bindings/net/realtek.h
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/net/realtek.txt b/Documentation/devicetree/bindings/net/realtek.txt
> > > > index 71d386c78269..40b0d6f9ee21 100644
> > > > --- a/Documentation/devicetree/bindings/net/realtek.txt
> > > > +++ b/Documentation/devicetree/bindings/net/realtek.txt
> > > > @@ -9,6 +9,12 @@ Optional properties:
> > > >
> > > > SSC is only available on some Realtek PHYs (e.g. RTL8211E).
> > > >
> > > > +- realtek,led-modes: LED mode configuration.
> > > > +
> > > > + A 0..3 element vector, with each element configuring the operating
> > > > + mode of an LED. Omitted LEDs are turned off. Allowed values are
> > > > + defined in "include/dt-bindings/net/realtek.h".
> > >
> > > This should probably be made more general and we should define LED modes
> > > that makes sense regardless of the PHY device, introduce a set of
> > > generic functions for validating and then add new function pointer for
> > > setting the LED configuration to the PHY driver. This would allow to be
> > > more future proof where each PHY driver could expose standard LEDs class
> > > devices to user-space, and it would also allow facilities like: ethtool
> > > -p to plug into that.
> > >
> > > Right now, each driver invents its own way of configuring LEDs, that
> > > does not scale, and there is not really a good reason for that other
> > > than reviewing drivers in isolation and therefore making it harder to
> > > extract the commonality. Yes, I realize that since you are the latest
> > > person submitting something in that area, you are being selected :)
>
> I agree.
>
> > I see the merit of your proposal to come up with a generic mechanism
> > to configure Ethernet LEDs, however I can't justify spending much of
> > my work time on this. If it is deemed useful I'm happy to send another
> > version of the current patchset that addresses the reviewer's comments,
> > but if the implementation of a generic LED configuration interface is
> > a requirement I will have to abandon at least the LED configuration
> > part of this series.
>
> Can you at least define a common binding for this. Maybe that's just
> removing 'realtek'. While the kernel side can evolve to a common
> infrastructure, the DT bindings can't.
I'm working on a generic binding.
I wonder what is the best process for reviewing/landing it, I'm
doubting between two options:
a) only post the binding doc and the generic PHY code that reads
the configuration from the DT. Post Realtek patches once
the binding/generic code has been acked.
pros: no churn from Realtek specific patches
cons: initially no (real) user of the new binding
b) post generic and Realtek changes together
pros: the binding has a user initially
cons: churn from Realtek specific patches
I can do either, depending on what maintainers/reviewers prefer. I'm
slightly inclined towards a)
> as of now it isn't even an API, the phy_device populates a new array
> in its struct with the values from the DT. PHY drivers access the
> array directly. Is it still preferable to post everything together?
>
> (maybe I'm too concerned about 'noise' from the driver patches while
> we are figuring out what exactly the binding should be).
We should try to have the DT parsing made generic in phylib, and add
new driver API calls to actually configure the LEDs.
Please also take a look at the Linux generic LED binding. It would be
nice to have something compatible with that. With time, the code could
morph into being part of the generic LED subsystem. So we are mostly
talking about triggers. But we offload the trigger to the hardware,
rather than have software trigger the blinking of the LEDs. So
something like:
ethernet-phy0 {
reg = <0>;
leds {
phy-led@0 {
reg = <0>
label = "left:green";
linux,default-trigger = "phy_link_1000_active";
}
phy-led@1 {
reg = <1>
label = "right:red";
linux,default-trigger = "phy_collision";
}
}
}
Andrew
On Mon, Jul 22, 2019 at 10:14:18AM -0700, Matthias Kaehlcke wrote:
> I'm working on a generic binding.
>
> I wonder what is the best process for reviewing/landing it, I'm
> doubting between two options:
>
> a) only post the binding doc and the generic PHY code that reads
> the configuration from the DT. Post Realtek patches once
> the binding/generic code has been acked.
>
> pros: no churn from Realtek specific patches
> cons: initially no (real) user of the new binding
>
> b) post generic and Realtek changes together
>
> pros: the binding has a user initially
> cons: churn from Realtek specific patches
>
> I can do either, depending on what maintainers/reviewers prefer. I'm
> slightly inclined towards a)
Hi Matthias
It is normal to include one user of any generic API which is added,
just to make is clear how an API should be used.
Andrew
Hi Andrew,
On Mon, Jul 22, 2019 at 09:01:33PM +0200, Andrew Lunn wrote:
> On Mon, Jul 22, 2019 at 10:14:18AM -0700, Matthias Kaehlcke wrote:
> > I'm working on a generic binding.
> >
> > I wonder what is the best process for reviewing/landing it, I'm
> > doubting between two options:
> >
> > a) only post the binding doc and the generic PHY code that reads
> > the configuration from the DT. Post Realtek patches once
> > the binding/generic code has been acked.
> >
> > pros: no churn from Realtek specific patches
> > cons: initially no (real) user of the new binding
> >
> > b) post generic and Realtek changes together
> >
> > pros: the binding has a user initially
> > cons: churn from Realtek specific patches
> >
> > I can do either, depending on what maintainers/reviewers prefer. I'm
> > slightly inclined towards a)
>
> Hi Matthias
>
> It is normal to include one user of any generic API which is added,
> just to make is clear how an API should be used.
as of now it isn't even an API, the phy_device populates a new array
in its struct with the values from the DT. PHY drivers access the
array directly. Is it still preferable to post everything together?
(maybe I'm too concerned about 'noise' from the driver patches while
we are figuring out what exactly the binding should be).
Thanks
Matthias