[PATCH net-next 0/4] net: phy: add dt property for realtek phy

[PATCH net-next 0/4] net: phy: add dt property for realtek phy

[Joakim Zhang]

Add dt property for realtek phy.

Joakim Zhang (4):
  dt-bindings: net: add dt binding for realtek rtl82xx phy
  net: phy: realtek: add dt property to disable CLKOUT clock
  net: phy: realteck: add dt property to disable ALDPS mode
  net: phy: realtek: add delay to fix RXC generation issue

 .../bindings/net/realtek,rtl82xx.yaml         | 42 +++++++++++
 drivers/net/phy/realtek.c                     | 74 ++++++++++++++++++-
 2 files changed, 113 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml

-- 
2.17.1

[PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Joakim Zhang]

Add binding for realtek rtl82xx phy.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 .../bindings/net/realtek,rtl82xx.yaml         | 42 +++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml

diff --git a/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
new file mode 100644
index 000000000000..0075c06e39bb
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
@@ -0,0 +1,42 @@
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/realtek,rtl82xx.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Realtek RTL82xx PHY
+
+maintainers:
+  - Andrew Lunn <andrew@lunn.ch>
+  - Florian Fainelli <f.fainelli@gmail.com>
+  - Heiner Kallweit <hkallweit1@gmail.com>
+
+description:
+  Bindings for Realtek RTL82xx PHYs
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  rtl821x,clkout-disable:
+    description: Disable CLKOUT clock.
+    type: boolean
+
+  rtl821x,aldps-disable:
+    description: Disable ALDPS mode.
+    type: boolean
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    mdio {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        ethphy1: ethernet-phy@1 {
+                reg = <1>;
+                rtl821x,clkout-disable;
+                rtl821x,aldps-disable;
+        };
+    };
-- 
2.17.1

Re: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Rob Herring]

On Tue, 01 Jun 2021 17:04:05 +0800, Joakim Zhang wrote:
> Add binding for realtek rtl82xx phy.
> 
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  .../bindings/net/realtek,rtl82xx.yaml         | 42 +++++++++++++++++++
>  1 file changed, 42 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/realtek,rtl82xx.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/realtek,rtl82xx.example.dt.yaml: ethernet-phy@1: 'rtl821x,aldps-disable', 'rtl821x,clkout-disable' do not match any of the regexes: '^#.*', '^(at25|bm|devbus|dmacap|dsa|exynos|fsi[ab]|gpio-fan|gpio-key|gpio|gpmc|hdmi|i2c-gpio),.*', '^(keypad|m25p|max8952|max8997|max8998|mpmc),.*', '^(pinctrl-single|#pinctrl-single|PowerPC),.*', '^(pl022|pxa-mmc|rcar_sound|rotary-encoder|s5m8767|sdhci),.*', '^(simple-audio-card|st-plgpio|st-spics|ts),.*', '^70mai,.*', '^GEFanuc,.*', '^ORCL,.*', '^SUNW,.*', '^[a-zA-Z0-9#_][a-zA-Z0-9+\\-._@]{0,63}$', '^[a-zA-Z0-9+\\-._]*@[0-9a-zA-Z,]*$', '^abb,.*', '^abilis,.*', '^abracon,.*', '^abt,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^al,.*', '^alcatel,.*', '^allegro,.*', '^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*'
 , '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^andestech,.*', '^anvo,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arctic,.*', '^arcx,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^aspeed,.*', '^asus,.*', '^atlas,.*', '^atmel,.*', '^auo,.*', '^auvidea,.*', '^avago,.*', '^avia,.*', '^avic,.*', '^avnet,.*', '^awinic,.*', '^axentia,.*', '^axis,.*', '^azoteq,.*', '^azw,.*', '^baikal,.*', '^bananapi,.*', '^beacon,.*', '^beagle,.*', '^bhf,.*', '^bitmain,.*', '^blutek,.*', '^boe,.*', '^bosch,.*', '^boundary,.*', '^brcm,.*', '^broadmobi,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^calaosystems,.*', '^calxeda,.*', '^canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^checkpoint,.*', '^chefr
 ee,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csq,.*', '^cubietech,.*', '^cypress,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^dell,.*', '^delta,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,.*', '^dragino,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^edt,.*', '^eeti,.*', '^einfochips,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^embest,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^endless,.*'
 , '^ene,.*', '^energymicro,.*', '^engicam,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^ezchip,.*', '^facebook,.*', '^fairphone,.*', '^faraday,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^gardena,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying,.*', '^gef,.*', '^gemei,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^goodix,.*', '^google,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haoyu,.*', '^hardkernel,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperun,.*', '^hp,.*', '^hsg,.*', '^hugsun,.*', '^hwacom,.*', '^hycon,.*', '^hydis,.*', '^hyundai,.*', '^i2
 se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^ifi,.*', '^ilitek,.*', '^img,.*', '^imi,.*', '^incircuit,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^innolux,.*', '^inside-secure,.*', '^inspur,.*', '^intel,.*', '^intercontrol,.*', '^invensense,.*', '^inversepath,.*', '^iom,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^ivo,.*', '^iwave,.*', '^jdi,.*', '^jedec,.*', '^jesurun,.*', '^jianda,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^litex,.*', '^lltc,.*', '^logi
 cpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongson,.*', '^lsi,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^mikroe,.*', '^mikrotik,.*', '^miniand,.*', '^minix,.*', '^miramems,.*', '^mitsubishi,.*', '^modtronix,.*', '^mosaixtech,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^nec,.*', '^neonode,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.
 *', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^novtech,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^opencores,.*', '^openrisc,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^ouya,.*', '^overkiz,.*', '^ovti,.*', '^oxsemi,.*', '^ozzmaker,.*', '^panasonic,.*', '^parade,.*', '^parallax,.*', '^pda,.*', '^pericom,.*', '^pervasive,.*', '^phicomm,.*', '^phytec,.*', '^picochip,.*', '^pine64,.*', '^pineriver,.*', '^pixcir,.*', '^plantower,.*', '^plathome,.*', '^plda,.*', '^plx,.*', '^ply,.*', '^pni,.*', '^pocketbook,.*', '^polaroid,.*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^q
 ihua,.*', '^qnap,.*', '^radxa,.*', '^raidsonic,.*', '^ralink,.*', '^ramtron,.*', '^raspberrypi,.*', '^raydium,.*', '^rda,.*', '^realtek,.*', '^remarkable,.*', '^renesas,.*', '^rervision,.*', '^revotics,.*', '^rex,.*', '^richtek,.*', '^ricoh,.*', '^rikomagic,.*', '^riot,.*', '^riscv,.*', '^rockchip,.*', '^rocktech,.*', '^rohm,.*', '^ronbo,.*', '^roofull,.*', '^roseapplepi,.*', '^samsung,.*', '^samtec,.*', '^sancloud,.*', '^sandisk,.*', '^satoz,.*', '^sbs,.*', '^schindler,.*', '^seagate,.*', '^seeed,.*', '^seirobotics,.*', '^semtech,.*', '^sensirion,.*', '^sensortek,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shimafuji,.*', '^shiratech,.*', '^si-en,.*', '^si-linux,.*', '^siemens,.*', '^sifive,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skyworks,.*', '
 ^smartlabs,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^spansion,.*', '^sprd,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starry,.*', '^startek,.*', '^ste,.*', '^stericsson,.*', '^summit,.*', '^sunchip,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tdo,.*', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^tempo,.*', '^terasic,.*', '^tfc,.*', '^thine,.*', '^thingyjp,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^tyan,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ugoos,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*',
  '^via,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vocore,.*', '^voipac,.*', '^vot,.*', '^vxt,.*', '^wand,.*', '^waveshare,.*', '^wd,.*', '^we,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^winbond,.*', '^winstar,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^x-powers,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*'
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/vendor-prefixes.yaml

See https://patchwork.ozlabs.org/patch/1485921

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Andrew Lunn]

> +properties:
> +  rtl821x,clkout-disable:
> +    description: Disable CLKOUT clock.
> +    type: boolean
> +
> +  rtl821x,aldps-disable:
> +    description: Disable ALDPS mode.
> +    type: boolean

I think most of the problems are the ambiguity in the binding.

If rtl821x,clkout-disable is not present, should it enable the CLKOUT?
That needs clear define here.

Do we actually want a tristate here?

                rtl821x,clkout = <true>;

means ensure the clock is outputting.

                rtl821x,clkout = <false>;

means ensure the clock is not outputting.

And if the property is not in DT at all, leave the hardware alone, at
either its default value, or whatever came before has set it to?

    Andrew

RE: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy

[Joakim Zhang]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年6月2日 10:39
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> hkallweit1@gmail.com; linux@armlinux.org.uk; f.fainelli@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek
> rtl82xx phy
> 
> > +properties:
> > +  rtl821x,clkout-disable:
> > +    description: Disable CLKOUT clock.
> > +    type: boolean
> > +
> > +  rtl821x,aldps-disable:
> > +    description: Disable ALDPS mode.
> > +    type: boolean
> 
> I think most of the problems are the ambiguity in the binding.
> 
> If rtl821x,clkout-disable is not present, should it enable the CLKOUT?
> That needs clear define here.
No, don't need to, CLKOUT clock default is enabled after PHY hardware reset. Add this property for users
request to disable this clock output. I will improve the description.

> Do we actually want a tristate here?
> 
>                 rtl821x,clkout = <true>;
> 
> means ensure the clock is outputting.
> 
>                 rtl821x,clkout = <false>;
> 
> means ensure the clock is not outputting.
I think it's unnecessary. A boolean type here is enough.

> And if the property is not in DT at all, leave the hardware alone, at either its
> default value, or whatever came before has set it to?
Seems not.
1. If enable CLKOUT in boot loader or keep the hardware default value (CLKOUT enabled), DT would work with this patch.
2. If disable CLKOUT in boot loader, with this patch, driver would enable this clock if this property is not in DT.

So, I need first read PHYCR2 register, if DT has property then disable the clock, if not, keep the original value?

However, for ALDPS mode, the hardware default value is disabled. The driver enable ALDPS by default which caused issue at my side. So need a property to disable it. 

We had better add a property like " rtl821x,aldps-enable", but It seems break the existing behavior.

Best Regards,
Joakim Zhang
>     Andrew

[PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Joakim Zhang]

Add "rtl821x,clkout-disable" property for user to disable CLKOUT clock
to save PHY power.

Per RTL8211F guide, a PHY reset should be issued after setting these
bits in PHYCR2 register. After this patch, CLKOUT clock output to be
disabled.

Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/phy/realtek.c | 48 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 821e85a97367..4219c23ff2b0 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -8,6 +8,7 @@
  * Copyright (c) 2004 Freescale Semiconductor, Inc.
  */
 #include <linux/bitops.h>
+#include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/module.h>
 #include <linux/delay.h>
@@ -27,6 +28,7 @@
 #define RTL821x_PAGE_SELECT			0x1f
 
 #define RTL8211F_PHYCR1				0x18
+#define RTL8211F_PHYCR2				0x19
 #define RTL8211F_INSR				0x1d
 
 #define RTL8211F_TX_DELAY			BIT(8)
@@ -40,6 +42,8 @@
 #define RTL8211E_TX_DELAY			BIT(12)
 #define RTL8211E_RX_DELAY			BIT(11)
 
+#define RTL8211F_CLKOUT_EN			BIT(0)
+
 #define RTL8201F_ISR				0x1e
 #define RTL8201F_ISR_ANERR			BIT(15)
 #define RTL8201F_ISR_DUPLEX			BIT(13)
@@ -67,10 +71,17 @@
 
 #define RTL_GENERIC_PHYID			0x001cc800
 
+/* quirks for realtek phy */
+#define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
+
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
 MODULE_LICENSE("GPL");
 
+struct rtl821x_priv {
+	u32 quirks;
+};
+
 static int rtl821x_read_page(struct phy_device *phydev)
 {
 	return __phy_read(phydev, RTL821x_PAGE_SELECT);
@@ -81,6 +92,23 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
 	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
 }
 
+static int rtl821x_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct rtl821x_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
+		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
+
+	phydev->priv = priv;
+
+	return 0;
+}
+
 static int rtl8201_ack_interrupt(struct phy_device *phydev)
 {
 	int err;
@@ -291,6 +319,7 @@ static int rtl8211c_config_init(struct phy_device *phydev)
 
 static int rtl8211f_config_init(struct phy_device *phydev)
 {
+	struct rtl821x_priv *priv = phydev->priv;
 	struct device *dev = &phydev->mdio.dev;
 	u16 val_txdly, val_rxdly;
 	u16 val;
@@ -354,7 +383,23 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 			val_rxdly ? "enabled" : "disabled");
 	}
 
-	return 0;
+	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE) {
+		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
+				       RTL8211F_CLKOUT_EN, 0);
+		if (ret < 0) {
+			dev_err(&phydev->mdio.dev, "clkout disable failed\n");
+			return ret;
+		}
+	} else {
+		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
+				       RTL8211F_CLKOUT_EN, RTL8211F_CLKOUT_EN);
+		if (ret < 0) {
+			dev_err(&phydev->mdio.dev, "clkout enable failed\n");
+			return ret;
+		}
+	}
+
+	return genphy_soft_reset(phydev);
 }
 
 static int rtl8211e_config_init(struct phy_device *phydev)
@@ -847,6 +892,7 @@ static struct phy_driver realtek_drvs[] = {
 	}, {
 		PHY_ID_MATCH_EXACT(0x001cc916),
 		.name		= "RTL8211F Gigabit Ethernet",
+		.probe		= rtl821x_probe,
 		.config_init	= &rtl8211f_config_init,
 		.read_status	= rtlgen_read_status,
 		.config_intr	= &rtl8211f_config_intr,
-- 
2.17.1

Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Russell King (Oracle)]

On Tue, Jun 01, 2021 at 05:04:06PM +0800, Joakim Zhang wrote:
> Add "rtl821x,clkout-disable" property for user to disable CLKOUT clock
> to save PHY power.
> 
> Per RTL8211F guide, a PHY reset should be issued after setting these
> bits in PHYCR2 register. After this patch, CLKOUT clock output to be
> disabled.
> 
> Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
>  drivers/net/phy/realtek.c | 48 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index 821e85a97367..4219c23ff2b0 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -8,6 +8,7 @@
>   * Copyright (c) 2004 Freescale Semiconductor, Inc.
>   */
>  #include <linux/bitops.h>
> +#include <linux/of.h>
>  #include <linux/phy.h>
>  #include <linux/module.h>
>  #include <linux/delay.h>
> @@ -27,6 +28,7 @@
>  #define RTL821x_PAGE_SELECT			0x1f
>  
>  #define RTL8211F_PHYCR1				0x18
> +#define RTL8211F_PHYCR2				0x19
>  #define RTL8211F_INSR				0x1d
>  
>  #define RTL8211F_TX_DELAY			BIT(8)
> @@ -40,6 +42,8 @@
>  #define RTL8211E_TX_DELAY			BIT(12)
>  #define RTL8211E_RX_DELAY			BIT(11)
>  
> +#define RTL8211F_CLKOUT_EN			BIT(0)
> +
>  #define RTL8201F_ISR				0x1e
>  #define RTL8201F_ISR_ANERR			BIT(15)
>  #define RTL8201F_ISR_DUPLEX			BIT(13)
> @@ -67,10 +71,17 @@
>  
>  #define RTL_GENERIC_PHYID			0x001cc800
>  
> +/* quirks for realtek phy */
> +#define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
> +
>  MODULE_DESCRIPTION("Realtek PHY driver");
>  MODULE_AUTHOR("Johnson Leung");
>  MODULE_LICENSE("GPL");
>  
> +struct rtl821x_priv {
> +	u32 quirks;
> +};
> +
>  static int rtl821x_read_page(struct phy_device *phydev)
>  {
>  	return __phy_read(phydev, RTL821x_PAGE_SELECT);
> @@ -81,6 +92,23 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
>  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
>  }
>  
> +static int rtl821x_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct rtl821x_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
> +		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
> +
> +	phydev->priv = priv;
> +
> +	return 0;
> +}
> +
>  static int rtl8201_ack_interrupt(struct phy_device *phydev)
>  {
>  	int err;
> @@ -291,6 +319,7 @@ static int rtl8211c_config_init(struct phy_device *phydev)
>  
>  static int rtl8211f_config_init(struct phy_device *phydev)
>  {
> +	struct rtl821x_priv *priv = phydev->priv;
>  	struct device *dev = &phydev->mdio.dev;
>  	u16 val_txdly, val_rxdly;
>  	u16 val;
> @@ -354,7 +383,23 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  			val_rxdly ? "enabled" : "disabled");
>  	}
>  
> -	return 0;
> +	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE) {
> +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> +				       RTL8211F_CLKOUT_EN, 0);
> +		if (ret < 0) {
> +			dev_err(&phydev->mdio.dev, "clkout disable failed\n");

You already have "dev" above, so this can be simplified to:

			dev_err(dev, "clkout disable failed\n");

> +			return ret;
> +		}
> +	} else {
> +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> +				       RTL8211F_CLKOUT_EN, RTL8211F_CLKOUT_EN);
> +		if (ret < 0) {
> +			dev_err(&phydev->mdio.dev, "clkout enable failed\n");

Same here.

> +			return ret;
> +		}
> +	}

Do you really need to distinguish between the enable and disable
operation? Would the following be better?

	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE)
		clkout = 0;
	else
		clkout = RTL8211_CLKOUT_EN;

	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
			       RTL8211_CLKOUT_EN, clkout);
	if (ret < 0) {
		dev_err(dev, "clkout configuration failed: %pe\n",
			ERR_PTR(ret));
		return ret;
	}

Other questions:
- Does the clock output default to enabled or disabled during kernel
  boot without this patch? Does this state depend on the boot loader?
- Do we really need the use of negative logic here (which depends on
  the answer to the above question)?
- Could other bits in the RTL8211F_PHYCR2 register also require future
  configuration? Would it be better to store the desired PHYCR2
  register value in "priv" rather than using "quirks". Quirks can become
  very unweildy over time.

By way of example on the last point... probe() would become:

	priv->phycr2 = RTL8211_CLKOUT_EN;

	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
		priv->phycr2 &= ~RTL8211_CLKOUT_EN;

and config_init():

	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
			       RTL8211_CLKOUT_EN, priv->phycr2);
	if (ret < 0) {
		dev_err(dev, "clkout configuration failed: %pe\n",
			ERR_PTR(ret));
		return ret;
	}

Note that phycr2 would be a u16 value.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Joakim Zhang]

Hi Russell,

Thanks for your reviewing.

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: 2021年6月1日 19:49
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> andrew@lunn.ch; hkallweit1@gmail.com; f.fainelli@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable
> CLKOUT clock
> 
> On Tue, Jun 01, 2021 at 05:04:06PM +0800, Joakim Zhang wrote:
> > Add "rtl821x,clkout-disable" property for user to disable CLKOUT clock
> > to save PHY power.
> >
> > Per RTL8211F guide, a PHY reset should be issued after setting these
> > bits in PHYCR2 register. After this patch, CLKOUT clock output to be
> > disabled.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@nxp.com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> >  drivers/net/phy/realtek.c | 48
> > ++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index 821e85a97367..4219c23ff2b0 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -8,6 +8,7 @@
> >   * Copyright (c) 2004 Freescale Semiconductor, Inc.
> >   */
> >  #include <linux/bitops.h>
> > +#include <linux/of.h>
> >  #include <linux/phy.h>
> >  #include <linux/module.h>
> >  #include <linux/delay.h>
> > @@ -27,6 +28,7 @@
> >  #define RTL821x_PAGE_SELECT			0x1f
> >
> >  #define RTL8211F_PHYCR1				0x18
> > +#define RTL8211F_PHYCR2				0x19
> >  #define RTL8211F_INSR				0x1d
> >
> >  #define RTL8211F_TX_DELAY			BIT(8)
> > @@ -40,6 +42,8 @@
> >  #define RTL8211E_TX_DELAY			BIT(12)
> >  #define RTL8211E_RX_DELAY			BIT(11)
> >
> > +#define RTL8211F_CLKOUT_EN			BIT(0)
> > +
> >  #define RTL8201F_ISR				0x1e
> >  #define RTL8201F_ISR_ANERR			BIT(15)
> >  #define RTL8201F_ISR_DUPLEX			BIT(13)
> > @@ -67,10 +71,17 @@
> >
> >  #define RTL_GENERIC_PHYID			0x001cc800
> >
> > +/* quirks for realtek phy */
> > +#define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
> > +
> >  MODULE_DESCRIPTION("Realtek PHY driver");
> MODULE_AUTHOR("Johnson
> > Leung");  MODULE_LICENSE("GPL");
> >
> > +struct rtl821x_priv {
> > +	u32 quirks;
> > +};
> > +
> >  static int rtl821x_read_page(struct phy_device *phydev)  {
> >  	return __phy_read(phydev, RTL821x_PAGE_SELECT); @@ -81,6 +92,23
> @@
> > static int rtl821x_write_page(struct phy_device *phydev, int page)
> >  	return __phy_write(phydev, RTL821x_PAGE_SELECT, page);  }
> >
> > +static int rtl821x_probe(struct phy_device *phydev) {
> > +	struct device *dev = &phydev->mdio.dev;
> > +	struct rtl821x_priv *priv;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
> > +		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
> > +
> > +	phydev->priv = priv;
> > +
> > +	return 0;
> > +}
> > +
> >  static int rtl8201_ack_interrupt(struct phy_device *phydev)  {
> >  	int err;
> > @@ -291,6 +319,7 @@ static int rtl8211c_config_init(struct phy_device
> > *phydev)
> >
> >  static int rtl8211f_config_init(struct phy_device *phydev)  {
> > +	struct rtl821x_priv *priv = phydev->priv;
> >  	struct device *dev = &phydev->mdio.dev;
> >  	u16 val_txdly, val_rxdly;
> >  	u16 val;
> > @@ -354,7 +383,23 @@ static int rtl8211f_config_init(struct phy_device
> *phydev)
> >  			val_rxdly ? "enabled" : "disabled");
> >  	}
> >
> > -	return 0;
> > +	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE) {
> > +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> > +				       RTL8211F_CLKOUT_EN, 0);
> > +		if (ret < 0) {
> > +			dev_err(&phydev->mdio.dev, "clkout disable failed\n");
> 
> You already have "dev" above, so this can be simplified to:
> 
> 			dev_err(dev, "clkout disable failed\n");
Yes. Will fix it.

> > +			return ret;
> > +		}
> > +	} else {
> > +		ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> > +				       RTL8211F_CLKOUT_EN, RTL8211F_CLKOUT_EN);
> > +		if (ret < 0) {
> > +			dev_err(&phydev->mdio.dev, "clkout enable failed\n");
> 
> Same here.
Yes.

> > +			return ret;
> > +		}
> > +	}
> 
> Do you really need to distinguish between the enable and disable operation?
> Would the following be better?
> 
> 	if (priv->quirks & RTL821X_CLKOUT_DISABLE_FEATURE)
> 		clkout = 0;
> 	else
> 		clkout = RTL8211_CLKOUT_EN;
> 
> 	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> 			       RTL8211_CLKOUT_EN, clkout);
> 	if (ret < 0) {
> 		dev_err(dev, "clkout configuration failed: %pe\n",
> 			ERR_PTR(ret));
> 		return ret;
> 	}
Thanks, more clear, will improve it.

> Other questions:
> - Does the clock output default to enabled or disabled during kernel
>   boot without this patch? Does this state depend on the boot loader?
CLKOUT clock default is enabled after PHY reset. What the meaning of depending on the boot loader? I think also can enable it in boot loader.

> - Do we really need the use of negative logic here (which depends on
>   the answer to the above question)?
Yes, we need, Since the CLKOUT default is enabled, some board design may need this clock feed MAC, so we shouldn't
break existing design. 

> - Could other bits in the RTL8211F_PHYCR2 register also require future
>   configuration? Would it be better to store the desired PHYCR2
>   register value in "priv" rather than using "quirks". Quirks can become
>   very unweildy over time.
Make sense, it seems more universal.

> By way of example on the last point... probe() would become:
> 
> 	priv->phycr2 = RTL8211_CLKOUT_EN;
> 
> 	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
> 		priv->phycr2 &= ~RTL8211_CLKOUT_EN;
> 
> and config_init():
> 
> 	ret = phy_modify_paged(phydev, 0xa43, RTL8211F_PHYCR2,
> 			       RTL8211_CLKOUT_EN, priv->phycr2);
> 	if (ret < 0) {
> 		dev_err(dev, "clkout configuration failed: %pe\n",
> 			ERR_PTR(ret));
> 		return ret;
> 	}
> 
> Note that phycr2 would be a u16 value.
Thanks.

Best Regards,
Joakim Zhang
> Thanks.
> 
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=04%7C01%7Cqiangqin
> g.zhang%40nxp.com%7Ca2666831ffdc43a0a27908d924f33ee9%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581449420768288%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4SFmi5Fst3r7jPzy%2F%2B%2Bej88
> GN7N91e1DxD7GyrA2dig%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Andrew Lunn]

> +struct rtl821x_priv {
> +	u32 quirks;

I'm not sure quirks is the correct word here. I would probably use
features, or flags.

	  Andrew

RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock

[Joakim Zhang]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: 2021年6月2日 6:08
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> hkallweit1@gmail.com; linux@armlinux.org.uk; f.fainelli@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable
> CLKOUT clock
> 
> > +struct rtl821x_priv {
> > +	u32 quirks;
> 
> I'm not sure quirks is the correct word here. I would probably use features, or
> flags.

Ok, I will change to flags.

As Russell king suggests another solution, which would you prefer to?
I need a conclusion so that I can send out the V2, thanks.

Best Regards,
Joakim Zhang
> 	  Andrew

[PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode

[Joakim Zhang]

If enable Advance Link Down Power Saving (ALDPS) mode, it will change
crystal/clock behavior, which cause RXC clock stop for dozens to hundreds
of miliseconds. This is comfirmed by Realtek engineer.

For some MACs, it needs RXC clock to support RX logic, after this patch,
PHY can generate continuous RXC clock during auto-negotiation. This patch
adds dt property to disable ALDPS mode per users' requirement.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/phy/realtek.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 4219c23ff2b0..90e3a8cbfc2f 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -73,6 +73,7 @@
 
 /* quirks for realtek phy */
 #define RTL821X_CLKOUT_DISABLE_FEATURE		BIT(0)
+#define RTL821X_ALDPS_DISABLE_FEATURE		BIT(1)
 
 MODULE_DESCRIPTION("Realtek PHY driver");
 MODULE_AUTHOR("Johnson Leung");
@@ -104,6 +105,9 @@ static int rtl821x_probe(struct phy_device *phydev)
 	if (of_property_read_bool(dev->of_node, "rtl821x,clkout-disable"))
 		priv->quirks |= RTL821X_CLKOUT_DISABLE_FEATURE;
 
+	if (of_property_read_bool(dev->of_node, "rtl821x,aldps-disable"))
+		priv->quirks |= RTL821X_ALDPS_DISABLE_FEATURE;
+
 	phydev->priv = priv;
 
 	return 0;
@@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	u16 val;
 	int ret;
 
-	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
-	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
+	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
+		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
+		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
+	}
 
 	switch (phydev->interface) {
 	case PHY_INTERFACE_MODE_RGMII:
-- 
2.17.1

Re: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode

[Russell King (Oracle)]

On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote:
> @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device *phydev)
>  	u16 val;
>  	int ret;
>  
> -	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
> -	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> +	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
> +		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_XTAL_OFF;
> +		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> +	}

Similar questions as with the previous patch, but also... this doesn't
actually disable the feature if it was previously turned on. E.g. a
kexec() from a current kernel that has set these features into a
subsequent kernel that the DT requests the feature to be disabled. Or
a boot loader that has enabled this feature.

If DT specifies that this feature is disabled, shouldn't this code be
disabling it explicitly?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

RE: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode

[Joakim Zhang]

Hi Russell,

> -----Original Message-----
> From: Russell King <linux@armlinux.org.uk>
> Sent: 2021年6月1日 19:52
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: davem@davemloft.net; kuba@kernel.org; robh+dt@kernel.org;
> andrew@lunn.ch; hkallweit1@gmail.com; f.fainelli@gmail.com; dl-linux-imx
> <linux-imx@nxp.com>; netdev@vger.kernel.org; devicetree@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable
> ALDPS mode
> 
> On Tue, Jun 01, 2021 at 05:04:07PM +0800, Joakim Zhang wrote:
> > @@ -325,8 +329,10 @@ static int rtl8211f_config_init(struct phy_device
> *phydev)
> >  	u16 val;
> >  	int ret;
> >
> > -	val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |
> RTL8211F_ALDPS_XTAL_OFF;
> > -	phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val, val);
> > +	if (!(priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)) {
> > +		val = RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_PLL_OFF |
> RTL8211F_ALDPS_XTAL_OFF;
> > +		phy_modify_paged_changed(phydev, 0xa43, RTL8211F_PHYCR1, val,
> val);
> > +	}
> 
> Similar questions as with the previous patch, but also... this doesn't actually
> disable the feature if it was previously turned on. E.g. a
> kexec() from a current kernel that has set these features into a subsequent
> kernel that the DT requests the feature to be disabled. Or a boot loader that
> has enabled this feature.
Sorry, I don't know what your meanings. As I explained before, boot loader also can configure PHY registers,
but kernel should not depend on what boot loader did.

If you use kexec() to boot kernel with DT request to disable this clock, PHY probe also can parse this property to disable it.
I know little about kexec(), could you please explain more if I misunderstand?

> If DT specifies that this feature is disabled, shouldn't this code be disabling it
> explicitly?
Yes, exactly should.

Best Regards,
Joakim Zhang
> --
> RMK's Patch system:
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.ar
> mlinux.org.uk%2Fdeveloper%2Fpatches%2F&amp;data=04%7C01%7Cqiangqin
> g.zhang%40nxp.com%7Ceacabd25941448301acb08d924f3b6de%7C686ea1d3b
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581451436345731%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C1000&amp;sdata=3jyYeGVBXXb5jCDtyTrt3DI9MwVE
> OT5Et9tpCZG26gY%3D&amp;reserved=0
> FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue

[Joakim Zhang]

PHY will delay about 11.5ms to generate RXC clock when switching from
power down to normal operation. Read/write registers would also cause RXC
become unstable and stop for a while during this process. Realtek engineer
suggests 15ms or more delay can workaround this issue. All these
statistics are collected with ALDPS mode disabled, so use RTL821X_ALDPS_DISABLE
quirk check.

Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
---
 drivers/net/phy/realtek.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index 90e3a8cbfc2f..b45deda839f8 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -408,6 +408,22 @@ static int rtl8211f_config_init(struct phy_device *phydev)
 	return genphy_soft_reset(phydev);
 }
 
+static int rtl821x_resume(struct phy_device *phydev)
+{
+	struct rtl821x_priv *priv = phydev->priv;
+	int ret;
+
+	ret = genphy_resume(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* delay time is collected with ALDPS mode disabled. */
+	if (priv->quirks & RTL821X_ALDPS_DISABLE_FEATURE)
+		msleep(20);
+
+	return 0;
+}
+
 static int rtl8211e_config_init(struct phy_device *phydev)
 {
 	int ret = 0, oldpage;
@@ -904,7 +920,7 @@ static struct phy_driver realtek_drvs[] = {
 		.config_intr	= &rtl8211f_config_intr,
 		.handle_interrupt = rtl8211f_handle_interrupt,
 		.suspend	= genphy_suspend,
-		.resume		= genphy_resume,
+		.resume		= rtl821x_resume,
 		.read_page	= rtl821x_read_page,
 		.write_page	= rtl821x_write_page,
 	}, {
-- 
2.17.1