* [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 4:07 ` Kai-Heng Feng
0 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 4:07 UTC (permalink / raw)
To: peppe.cavallaro, alexandre.torgue, joabreu
Cc: Kai-Heng Feng, David S. Miller, Jakub Kicinski, Maxime Coquelin,
Andrew Lunn, Heiner Kallweit, Russell King, Marek Behún,
Ivan Bornyakov, Pali Rohár, netdev, linux-stm32,
linux-arm-kernel, linux-kernel
BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
instead of setting another value, keep it untouched and restore the saved
value on system resume.
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 16 +++++
drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++
drivers/net/phy/marvell.c | 58 ++++++++++++-------
include/linux/marvell_phy.h | 1 +
5 files changed, 61 insertions(+), 20 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 8e8778cfbbadd..f8a2879e0264a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -857,6 +857,16 @@ static const struct dmi_system_id quark_pci_dmi[] = {
{}
};
+static const struct dmi_system_id use_preset_led[] = {
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
+ },
+ },
+ {}
+};
+
static int quark_default_data(struct pci_dev *pdev,
struct plat_stmmacenet_data *plat)
{
@@ -989,6 +999,7 @@ static int intel_eth_pci_probe(struct pci_dev *pdev,
struct intel_priv_data *intel_priv;
struct plat_stmmacenet_data *plat;
struct stmmac_resources res;
+ struct stmmac_priv *priv;
int ret;
intel_priv = devm_kzalloc(&pdev->dev, sizeof(*intel_priv), GFP_KERNEL);
@@ -1075,6 +1086,11 @@ static int intel_eth_pci_probe(struct pci_dev *pdev,
goto err_dvr_probe;
}
+ if (dmi_check_system(use_preset_led)) {
+ priv = netdev_priv(dev_get_drvdata(&pdev->dev));
+ priv->use_preset_led = true;
+ }
+
return 0;
err_dvr_probe:
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 40b5ed94cb54a..525701acbbdbb 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -319,6 +319,8 @@ struct stmmac_priv {
/* XDP BPF Program */
unsigned long *af_xdp_zc_qps;
struct bpf_prog *xdp_prog;
+
+ bool use_preset_led;
};
enum stmmac_state {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 63ff2dad8c85f..155412656b8bf 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -49,6 +49,7 @@
#include "dwmac1000.h"
#include "dwxgmac2.h"
#include "hwif.h"
+#include <linux/marvell_phy.h>
/* As long as the interface is active, we keep the timestamping counter enabled
* with fine resolution and binary rollover. This avoid non-monotonic behavior
@@ -1236,6 +1237,9 @@ static int stmmac_init_phy(struct net_device *dev)
return -ENODEV;
}
+ if (priv->use_preset_led)
+ phydev->dev_flags |= MARVELL_PHY_USE_PRESET_LED;
+
ret = phylink_connect_phy(priv->phylink, phydev);
}
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 739859c0dfb18..45be432188781 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -304,6 +304,7 @@ struct marvell_priv {
u32 last;
u32 step;
s8 pair;
+ u16 led_def_config;
};
static int marvell_read_page(struct phy_device *phydev)
@@ -748,32 +749,49 @@ static int m88e1510_config_aneg(struct phy_device *phydev)
static void marvell_config_led(struct phy_device *phydev)
{
- u16 def_config;
+ struct marvell_priv *priv = phydev->priv;
int err;
- switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
- /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
- case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
- case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
- def_config = MII_88E1121_PHY_LED_DEF;
- break;
- /* Default PHY LED config:
- * LED[0] .. 1000Mbps Link
- * LED[1] .. 100Mbps Link
- * LED[2] .. Blink, Activity
- */
- case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
- if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
- def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
- else
- def_config = MII_88E1510_PHY_LED_DEF;
- break;
- default:
+ if (priv->led_def_config == -1)
return;
+
+ if (priv->led_def_config)
+ goto write;
+
+ if (phydev->dev_flags & MARVELL_PHY_USE_PRESET_LED) {
+ priv->led_def_config = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
+ MII_PHY_LED_CTRL);
+ if (priv->led_def_config < 0) {
+ priv->led_def_config = -1;
+ return;
+ }
+ } else {
+ switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
+ /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
+ case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
+ case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
+ priv->led_def_config = MII_88E1121_PHY_LED_DEF;
+ break;
+ /* Default PHY LED config:
+ * LED[0] .. 1000Mbps Link
+ * LED[1] .. 100Mbps Link
+ * LED[2] .. Blink, Activity
+ */
+ case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
+ if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
+ priv->led_def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
+ else
+ priv->led_def_config = MII_88E1510_PHY_LED_DEF;
+ break;
+ default:
+ priv->led_def_config = -1;
+ return;
+ }
}
+write:
err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
- def_config);
+ priv->led_def_config);
if (err < 0)
phydev_warn(phydev, "Fail to config marvell phy LED.\n");
}
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index ea5995d9ad6c1..492f07620b6c0 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -43,5 +43,6 @@
#define MARVELL_PHY_M1145_FLAGS_RESISTANCE BIT(0)
#define MARVELL_PHY_M1118_DNS323_LEDS BIT(1)
#define MARVELL_PHY_LED0_LINK_LED1_ACTIVE BIT(2)
+#define MARVELL_PHY_USE_PRESET_LED BIT(3)
#endif /* _MARVELL_PHY_H */
--
2.33.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 4:07 ` Kai-Heng Feng
@ 2022-01-14 4:35 ` Jakub Kicinski
-1 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2022-01-14 4:35 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, 14 Jan 2022 12:07:54 +0800 Kai-Heng Feng wrote:
> BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> instead of setting another value, keep it untouched and restore the saved
> value on system resume.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
I defer to PHY experts for review. Coincidentally the first Marvell
flag appears dead, nobody sets it:
$ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
$
unless it's read from DT under different name or something.
Once you get some reviews please wait for net-next to open:
http://vger.kernel.org/~davem/net-next.html
and repost. It should happen the week of Jan 24th. When you repost
please drop the first patch, I believe Russell does not like the BIT()
macro, his opinion overrides checkpatch.
Thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 4:35 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2022-01-14 4:35 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, 14 Jan 2022 12:07:54 +0800 Kai-Heng Feng wrote:
> BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> instead of setting another value, keep it untouched and restore the saved
> value on system resume.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
I defer to PHY experts for review. Coincidentally the first Marvell
flag appears dead, nobody sets it:
$ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
$
unless it's read from DT under different name or something.
Once you get some reviews please wait for net-next to open:
http://vger.kernel.org/~davem/net-next.html
and repost. It should happen the week of Jan 24th. When you repost
please drop the first patch, I believe Russell does not like the BIT()
macro, his opinion overrides checkpatch.
Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 4:35 ` Jakub Kicinski
@ 2022-01-14 6:47 ` Kai-Heng Feng
-1 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 6:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 12:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 14 Jan 2022 12:07:54 +0800 Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> I defer to PHY experts for review. Coincidentally the first Marvell
> flag appears dead, nobody sets it:
>
> $ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
> drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
> include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
> $
>
> unless it's read from DT under different name or something.
It was introduced by 95d21ff4c645 without any user. Should we keep it?
>
>
> Once you get some reviews please wait for net-next to open:
>
> http://vger.kernel.org/~davem/net-next.html
>
> and repost. It should happen the week of Jan 24th. When you repost
> please drop the first patch, I believe Russell does not like the BIT()
> macro, his opinion overrides checkpatch.
Of course. I'll wait for the review and resubmit the 2nd patch.
Kai-Heng
>
> Thanks!
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 6:47 ` Kai-Heng Feng
0 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 6:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 12:35 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Fri, 14 Jan 2022 12:07:54 +0800 Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
>
> I defer to PHY experts for review. Coincidentally the first Marvell
> flag appears dead, nobody sets it:
>
> $ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
> drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
> include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
> $
>
> unless it's read from DT under different name or something.
It was introduced by 95d21ff4c645 without any user. Should we keep it?
>
>
> Once you get some reviews please wait for net-next to open:
>
> http://vger.kernel.org/~davem/net-next.html
>
> and repost. It should happen the week of Jan 24th. When you repost
> please drop the first patch, I believe Russell does not like the BIT()
> macro, his opinion overrides checkpatch.
Of course. I'll wait for the review and resubmit the 2nd patch.
Kai-Heng
>
> Thanks!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 6:47 ` Kai-Heng Feng
@ 2022-01-14 16:23 ` Jakub Kicinski
-1 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2022-01-14 16:23 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, 14 Jan 2022 14:47:47 +0800 Kai-Heng Feng wrote:
> > Coincidentally the first Marvell flag appears dead, nobody sets it:
> >
> > $ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
> > drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
> > include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
> > $
> >
> > unless it's read from DT under different name or something.
>
> It was introduced by 95d21ff4c645 without any user. Should we keep it?
Not unless someone explains that it's actually used somehow.
Please post a patch once net-next opens.
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 16:23 ` Jakub Kicinski
0 siblings, 0 replies; 21+ messages in thread
From: Jakub Kicinski @ 2022-01-14 16:23 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Maxime Coquelin, Andrew Lunn, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, 14 Jan 2022 14:47:47 +0800 Kai-Heng Feng wrote:
> > Coincidentally the first Marvell flag appears dead, nobody sets it:
> >
> > $ git grep MARVELL_PHY_M1145_FLAGS_RESISTANCE
> > drivers/net/phy/marvell.c: if (phydev->dev_flags & MARVELL_PHY_M1145_FLAGS_RESISTANCE) {
> > include/linux/marvell_phy.h:#define MARVELL_PHY_M1145_FLAGS_RESISTANCE 0x00000001
> > $
> >
> > unless it's read from DT under different name or something.
>
> It was introduced by 95d21ff4c645 without any user. Should we keep it?
Not unless someone explains that it's actually used somehow.
Please post a patch once net-next opens.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 4:07 ` Kai-Heng Feng
@ 2022-01-14 6:53 ` kernel test robot
-1 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2022-01-14 6:53 UTC (permalink / raw)
To: Kai-Heng Feng, peppe.cavallaro, alexandre.torgue, joabreu
Cc: llvm, kbuild-all, Kai-Heng Feng, Jakub Kicinski, Maxime Coquelin,
Andrew Lunn, Heiner Kallweit, Russell King, Marek Behún
Hi Kai-Heng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.16 next-20220113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/net-phy-marvell-Use-BIT-macro-for-flags/20220114-120936
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git fb3b0673b7d5b477ed104949450cd511337ba3c6
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220114/202201141429.5HZ7UZB6-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82c8aca93488730ce8f66101e0f3538f14b551dd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/83e7c70ef5632f6b94caf8221a20db0881ee6ce2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kai-Heng-Feng/net-phy-marvell-Use-BIT-macro-for-flags/20220114-120936
git checkout 83e7c70ef5632f6b94caf8221a20db0881ee6ce2
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/phy/marvell.c:755:27: warning: result of comparison of constant -1 with expression of type 'u16' (aka 'unsigned short') is always false [-Wtautological-constant-out-of-range-compare]
if (priv->led_def_config == -1)
~~~~~~~~~~~~~~~~~~~~ ^ ~~
1 warning generated.
vim +755 drivers/net/phy/marvell.c
749
750 static void marvell_config_led(struct phy_device *phydev)
751 {
752 struct marvell_priv *priv = phydev->priv;
753 int err;
754
> 755 if (priv->led_def_config == -1)
756 return;
757
758 if (priv->led_def_config)
759 goto write;
760
761 if (phydev->dev_flags & MARVELL_PHY_USE_PRESET_LED) {
762 priv->led_def_config = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
763 MII_PHY_LED_CTRL);
764 if (priv->led_def_config < 0) {
765 priv->led_def_config = -1;
766 return;
767 }
768 } else {
769 switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
770 /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
771 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
772 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
773 priv->led_def_config = MII_88E1121_PHY_LED_DEF;
774 break;
775 /* Default PHY LED config:
776 * LED[0] .. 1000Mbps Link
777 * LED[1] .. 100Mbps Link
778 * LED[2] .. Blink, Activity
779 */
780 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
781 if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
782 priv->led_def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
783 else
784 priv->led_def_config = MII_88E1510_PHY_LED_DEF;
785 break;
786 default:
787 priv->led_def_config = -1;
788 return;
789 }
790 }
791
792 write:
793 err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
794 priv->led_def_config);
795 if (err < 0)
796 phydev_warn(phydev, "Fail to config marvell phy LED.\n");
797 }
798
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 6:53 ` kernel test robot
0 siblings, 0 replies; 21+ messages in thread
From: kernel test robot @ 2022-01-14 6:53 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 3938 bytes --]
Hi Kai-Heng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v5.16 next-20220113]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Kai-Heng-Feng/net-phy-marvell-Use-BIT-macro-for-flags/20220114-120936
base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git fb3b0673b7d5b477ed104949450cd511337ba3c6
config: x86_64-randconfig-a016 (https://download.01.org/0day-ci/archive/20220114/202201141429.5HZ7UZB6-lkp(a)intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 82c8aca93488730ce8f66101e0f3538f14b551dd)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/83e7c70ef5632f6b94caf8221a20db0881ee6ce2
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Kai-Heng-Feng/net-phy-marvell-Use-BIT-macro-for-flags/20220114-120936
git checkout 83e7c70ef5632f6b94caf8221a20db0881ee6ce2
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/net/phy/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
>> drivers/net/phy/marvell.c:755:27: warning: result of comparison of constant -1 with expression of type 'u16' (aka 'unsigned short') is always false [-Wtautological-constant-out-of-range-compare]
if (priv->led_def_config == -1)
~~~~~~~~~~~~~~~~~~~~ ^ ~~
1 warning generated.
vim +755 drivers/net/phy/marvell.c
749
750 static void marvell_config_led(struct phy_device *phydev)
751 {
752 struct marvell_priv *priv = phydev->priv;
753 int err;
754
> 755 if (priv->led_def_config == -1)
756 return;
757
758 if (priv->led_def_config)
759 goto write;
760
761 if (phydev->dev_flags & MARVELL_PHY_USE_PRESET_LED) {
762 priv->led_def_config = phy_read_paged(phydev, MII_MARVELL_LED_PAGE,
763 MII_PHY_LED_CTRL);
764 if (priv->led_def_config < 0) {
765 priv->led_def_config = -1;
766 return;
767 }
768 } else {
769 switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
770 /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
771 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
772 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
773 priv->led_def_config = MII_88E1121_PHY_LED_DEF;
774 break;
775 /* Default PHY LED config:
776 * LED[0] .. 1000Mbps Link
777 * LED[1] .. 100Mbps Link
778 * LED[2] .. Blink, Activity
779 */
780 case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
781 if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
782 priv->led_def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
783 else
784 priv->led_def_config = MII_88E1510_PHY_LED_DEF;
785 break;
786 default:
787 priv->led_def_config = -1;
788 return;
789 }
790 }
791
792 write:
793 err = phy_write_paged(phydev, MII_MARVELL_LED_PAGE, MII_PHY_LED_CTRL,
794 priv->led_def_config);
795 if (err < 0)
796 phydev_warn(phydev, "Fail to config marvell phy LED.\n");
797 }
798
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 4:07 ` Kai-Heng Feng
@ 2022-01-14 13:09 ` Andrew Lunn
-1 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 13:09 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 12:07:54PM +0800, Kai-Heng Feng wrote:
> BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> instead of setting another value, keep it untouched and restore the saved
> value on system resume.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 16 +++++
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++
> drivers/net/phy/marvell.c | 58 ++++++++++++-------
> include/linux/marvell_phy.h | 1 +
> 5 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 8e8778cfbbadd..f8a2879e0264a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -857,6 +857,16 @@ static const struct dmi_system_id quark_pci_dmi[] = {
> {}
> };
>
> +static const struct dmi_system_id use_preset_led[] = {
> + {
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> + },
> + },
> + {}
> +};
This is a PHY property. Why is the MAC involved?
Please also think about how to make this generic, so any ACPI based
system can use it, with any PHY.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 13:09 ` Andrew Lunn
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 13:09 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 12:07:54PM +0800, Kai-Heng Feng wrote:
> BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> instead of setting another value, keep it untouched and restore the saved
> value on system resume.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 16 +++++
> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +
> .../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++
> drivers/net/phy/marvell.c | 58 ++++++++++++-------
> include/linux/marvell_phy.h | 1 +
> 5 files changed, 61 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 8e8778cfbbadd..f8a2879e0264a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -857,6 +857,16 @@ static const struct dmi_system_id quark_pci_dmi[] = {
> {}
> };
>
> +static const struct dmi_system_id use_preset_led[] = {
> + {
> + .matches = {
> + DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> + DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> + },
> + },
> + {}
> +};
This is a PHY property. Why is the MAC involved?
Please also think about how to make this generic, so any ACPI based
system can use it, with any PHY.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 13:09 ` Andrew Lunn
@ 2022-01-14 15:22 ` Kai-Heng Feng
-1 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 15:22 UTC (permalink / raw)
To: Andrew Lunn
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 9:09 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Jan 14, 2022 at 12:07:54PM +0800, Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 16 +++++
> > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +
> > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++
> > drivers/net/phy/marvell.c | 58 ++++++++++++-------
> > include/linux/marvell_phy.h | 1 +
> > 5 files changed, 61 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index 8e8778cfbbadd..f8a2879e0264a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -857,6 +857,16 @@ static const struct dmi_system_id quark_pci_dmi[] = {
> > {}
> > };
> >
> > +static const struct dmi_system_id use_preset_led[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> > + },
> > + },
> > + {}
> > +};
>
> This is a PHY property. Why is the MAC involved?
This is inspired by commit a93f7fe13454 ("net: phy: marvell: add new
default led configure for m88e151x") which passes the flag from MAC to
PHY.
>
> Please also think about how to make this generic, so any ACPI based
> system can use it, with any PHY.
ACPI property won't work because it ties to ACPI platform device or
PCI device, so the property still needs to be passed from MAC to PHY.
So the only approach I can think of is to use DMI match directly in PHY driver.
Kai-Heng
>
> Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 15:22 ` Kai-Heng Feng
0 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 15:22 UTC (permalink / raw)
To: Andrew Lunn
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 9:09 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Fri, Jan 14, 2022 at 12:07:54PM +0800, Kai-Heng Feng wrote:
> > BIOS on Dell Edge Gateway 3200 already makes its own phy LED setting, so
> > instead of setting another value, keep it untouched and restore the saved
> > value on system resume.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > ---
> > .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 16 +++++
> > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 2 +
> > .../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 ++
> > drivers/net/phy/marvell.c | 58 ++++++++++++-------
> > include/linux/marvell_phy.h | 1 +
> > 5 files changed, 61 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > index 8e8778cfbbadd..f8a2879e0264a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> > @@ -857,6 +857,16 @@ static const struct dmi_system_id quark_pci_dmi[] = {
> > {}
> > };
> >
> > +static const struct dmi_system_id use_preset_led[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_SYS_VENDOR, "Dell EMC"),
> > + DMI_MATCH(DMI_PRODUCT_NAME, "Edge Gateway 3200"),
> > + },
> > + },
> > + {}
> > +};
>
> This is a PHY property. Why is the MAC involved?
This is inspired by commit a93f7fe13454 ("net: phy: marvell: add new
default led configure for m88e151x") which passes the flag from MAC to
PHY.
>
> Please also think about how to make this generic, so any ACPI based
> system can use it, with any PHY.
ACPI property won't work because it ties to ACPI platform device or
PCI device, so the property still needs to be passed from MAC to PHY.
So the only approach I can think of is to use DMI match directly in PHY driver.
Kai-Heng
>
> Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 15:22 ` Kai-Heng Feng
@ 2022-01-14 17:25 ` Andrew Lunn
-1 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 17:25 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
> > This is a PHY property. Why is the MAC involved?
>
> This is inspired by commit a93f7fe13454 ("net: phy: marvell: add new
> default led configure for m88e151x") which passes the flag from MAC to
> PHY.
But in this case, the MAC does not care what the LEDs are. The
platform wants them left alone, not the MAC.
> > Please also think about how to make this generic, so any ACPI based
> > system can use it, with any PHY.
...
> So the only approach I can think of is to use DMI match directly in PHY driver.
In the phylib core. And the core then asks the specific PHY driver to
not touch the LED configuration. It is then generic.
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 17:25 ` Andrew Lunn
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 17:25 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
> > This is a PHY property. Why is the MAC involved?
>
> This is inspired by commit a93f7fe13454 ("net: phy: marvell: add new
> default led configure for m88e151x") which passes the flag from MAC to
> PHY.
But in this case, the MAC does not care what the LEDs are. The
platform wants them left alone, not the MAC.
> > Please also think about how to make this generic, so any ACPI based
> > system can use it, with any PHY.
...
> So the only approach I can think of is to use DMI match directly in PHY driver.
In the phylib core. And the core then asks the specific PHY driver to
not touch the LED configuration. It is then generic.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 4:07 ` Kai-Heng Feng
@ 2022-01-14 13:20 ` Andrew Lunn
-1 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 13:20 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
> static void marvell_config_led(struct phy_device *phydev)
> {
> - u16 def_config;
> + struct marvell_priv *priv = phydev->priv;
> int err;
>
> - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
> - def_config = MII_88E1121_PHY_LED_DEF;
> - break;
> - /* Default PHY LED config:
> - * LED[0] .. 1000Mbps Link
> - * LED[1] .. 100Mbps Link
> - * LED[2] .. Blink, Activity
> - */
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
> - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
> - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
> - else
> - def_config = MII_88E1510_PHY_LED_DEF;
> - break;
> - default:
> + if (priv->led_def_config == -1)
> return;
> +
> + if (priv->led_def_config)
> + goto write;
Really?
Please restructure this code. Take it apart into helpers. You need:
A function to set the actual LED configuration.
A function to decide what, if any, configuration to set
A function to store the current configuration on suspend.
A function to restore the current configuration on resume.
Lots of little functions will make it much easier to understand, and
avoid 1980s BASIC style.
I'm also surprised you need to deal with suspend/resume. Why does the
BIOS not set the LED mode on resume, same as it does on power up?
Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 13:20 ` Andrew Lunn
0 siblings, 0 replies; 21+ messages in thread
From: Andrew Lunn @ 2022-01-14 13:20 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
> static void marvell_config_led(struct phy_device *phydev)
> {
> - u16 def_config;
> + struct marvell_priv *priv = phydev->priv;
> int err;
>
> - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
> - def_config = MII_88E1121_PHY_LED_DEF;
> - break;
> - /* Default PHY LED config:
> - * LED[0] .. 1000Mbps Link
> - * LED[1] .. 100Mbps Link
> - * LED[2] .. Blink, Activity
> - */
> - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
> - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
> - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
> - else
> - def_config = MII_88E1510_PHY_LED_DEF;
> - break;
> - default:
> + if (priv->led_def_config == -1)
> return;
> +
> + if (priv->led_def_config)
> + goto write;
Really?
Please restructure this code. Take it apart into helpers. You need:
A function to set the actual LED configuration.
A function to decide what, if any, configuration to set
A function to store the current configuration on suspend.
A function to restore the current configuration on resume.
Lots of little functions will make it much easier to understand, and
avoid 1980s BASIC style.
I'm also surprised you need to deal with suspend/resume. Why does the
BIOS not set the LED mode on resume, same as it does on power up?
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
2022-01-14 13:20 ` Andrew Lunn
@ 2022-01-14 15:25 ` Kai-Heng Feng
-1 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 15:25 UTC (permalink / raw)
To: Andrew Lunn
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 9:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > static void marvell_config_led(struct phy_device *phydev)
> > {
> > - u16 def_config;
> > + struct marvell_priv *priv = phydev->priv;
> > int err;
> >
> > - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> > - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
> > - def_config = MII_88E1121_PHY_LED_DEF;
> > - break;
> > - /* Default PHY LED config:
> > - * LED[0] .. 1000Mbps Link
> > - * LED[1] .. 100Mbps Link
> > - * LED[2] .. Blink, Activity
> > - */
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
> > - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
> > - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
> > - else
> > - def_config = MII_88E1510_PHY_LED_DEF;
> > - break;
> > - default:
> > + if (priv->led_def_config == -1)
> > return;
> > +
> > + if (priv->led_def_config)
> > + goto write;
>
> Really?
>
> Please restructure this code. Take it apart into helpers. You need:
>
> A function to set the actual LED configuration.
> A function to decide what, if any, configuration to set
> A function to store the current configuration on suspend.
> A function to restore the current configuration on resume.
>
> Lots of little functions will make it much easier to understand, and
> avoid 1980s BASIC style.
Sure, will turn these into helper functions.
>
> I'm also surprised you need to deal with suspend/resume. Why does the
> BIOS not set the LED mode on resume, same as it does on power up?
I was told this is a BIOS limitation. I'll ask vendor _why_ the LED
can't be restored by BIOS.
Kai-Heng
>
> Andrew
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH 2/2] stmmac: intel: Honor phy LED set by system firmware on a Dell hardware
@ 2022-01-14 15:25 ` Kai-Heng Feng
0 siblings, 0 replies; 21+ messages in thread
From: Kai-Heng Feng @ 2022-01-14 15:25 UTC (permalink / raw)
To: Andrew Lunn
Cc: peppe.cavallaro, alexandre.torgue, joabreu, David S. Miller,
Jakub Kicinski, Maxime Coquelin, Heiner Kallweit, Russell King,
Marek Behún, Ivan Bornyakov, Pali Rohár, netdev,
linux-stm32, linux-arm-kernel, linux-kernel
On Fri, Jan 14, 2022 at 9:20 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > static void marvell_config_led(struct phy_device *phydev)
> > {
> > - u16 def_config;
> > + struct marvell_priv *priv = phydev->priv;
> > int err;
> >
> > - switch (MARVELL_PHY_FAMILY_ID(phydev->phy_id)) {
> > - /* Default PHY LED config: LED[0] .. Link, LED[1] .. Activity */
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1121R):
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1318S):
> > - def_config = MII_88E1121_PHY_LED_DEF;
> > - break;
> > - /* Default PHY LED config:
> > - * LED[0] .. 1000Mbps Link
> > - * LED[1] .. 100Mbps Link
> > - * LED[2] .. Blink, Activity
> > - */
> > - case MARVELL_PHY_FAMILY_ID(MARVELL_PHY_ID_88E1510):
> > - if (phydev->dev_flags & MARVELL_PHY_LED0_LINK_LED1_ACTIVE)
> > - def_config = MII_88E1510_PHY_LED0_LINK_LED1_ACTIVE;
> > - else
> > - def_config = MII_88E1510_PHY_LED_DEF;
> > - break;
> > - default:
> > + if (priv->led_def_config == -1)
> > return;
> > +
> > + if (priv->led_def_config)
> > + goto write;
>
> Really?
>
> Please restructure this code. Take it apart into helpers. You need:
>
> A function to set the actual LED configuration.
> A function to decide what, if any, configuration to set
> A function to store the current configuration on suspend.
> A function to restore the current configuration on resume.
>
> Lots of little functions will make it much easier to understand, and
> avoid 1980s BASIC style.
Sure, will turn these into helper functions.
>
> I'm also surprised you need to deal with suspend/resume. Why does the
> BIOS not set the LED mode on resume, same as it does on power up?
I was told this is a BIOS limitation. I'll ask vendor _why_ the LED
can't be restored by BIOS.
Kai-Heng
>
> Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 21+ messages in thread