* [PATCH net-next 0/4] net: phy: add dt property for realtek phy @ 2021-06-01 9:04 Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang ` (3 more replies) 0 siblings, 4 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-01 9:04 UTC (permalink / raw) To: davem, kuba, robh+dt, andrew, hkallweit1, linux, f.fainelli Cc: linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy 2021-06-01 9:04 [PATCH net-next 0/4] net: phy: add dt property for realtek phy Joakim Zhang @ 2021-06-01 9:04 ` Joakim Zhang 2021-06-01 13:32 ` Rob Herring 2021-06-02 2:39 ` Andrew Lunn 2021-06-01 9:04 ` [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock Joakim Zhang ` (2 subsequent siblings) 3 siblings, 2 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-01 9:04 UTC (permalink / raw) To: davem, kuba, robh+dt, andrew, hkallweit1, linux, f.fainelli Cc: linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang @ 2021-06-01 13:32 ` Rob Herring 2021-06-02 2:39 ` Andrew Lunn 1 sibling, 0 replies; 14+ messages in thread From: Rob Herring @ 2021-06-01 13:32 UTC (permalink / raw) To: Joakim Zhang Cc: devicetree, kuba, netdev, robh+dt, linux-imx, andrew, linux, f.fainelli, linux-kernel, davem, hkallweit1 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang 2021-06-01 13:32 ` Rob Herring @ 2021-06-02 2:39 ` Andrew Lunn 2021-06-02 3:14 ` Joakim Zhang 1 sibling, 1 reply; 14+ messages in thread From: Andrew Lunn @ 2021-06-02 2:39 UTC (permalink / raw) To: Joakim Zhang Cc: davem, kuba, robh+dt, hkallweit1, linux, f.fainelli, linux-imx, netdev, devicetree, linux-kernel > +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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy 2021-06-02 2:39 ` Andrew Lunn @ 2021-06-02 3:14 ` Joakim Zhang 0 siblings, 0 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-02 3:14 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, robh+dt, hkallweit1, linux, f.fainelli, dl-linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock 2021-06-01 9:04 [PATCH net-next 0/4] net: phy: add dt property for realtek phy Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang @ 2021-06-01 9:04 ` Joakim Zhang 2021-06-01 11:48 ` Russell King (Oracle) 2021-06-01 22:07 ` Andrew Lunn 2021-06-01 9:04 ` [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue Joakim Zhang 3 siblings, 2 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-01 9:04 UTC (permalink / raw) To: davem, kuba, robh+dt, andrew, hkallweit1, linux, f.fainelli Cc: linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock 2021-06-01 9:04 ` [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock Joakim Zhang @ 2021-06-01 11:48 ` Russell King (Oracle) 2021-06-02 2:18 ` Joakim Zhang 2021-06-01 22:07 ` Andrew Lunn 1 sibling, 1 reply; 14+ messages in thread From: Russell King (Oracle) @ 2021-06-01 11:48 UTC (permalink / raw) To: Joakim Zhang Cc: davem, kuba, robh+dt, andrew, hkallweit1, f.fainelli, linux-imx, netdev, devicetree, linux-kernel 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! ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock 2021-06-01 11:48 ` Russell King (Oracle) @ 2021-06-02 2:18 ` Joakim Zhang 0 siblings, 0 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-02 2:18 UTC (permalink / raw) To: Russell King Cc: davem, kuba, robh+dt, andrew, hkallweit1, f.fainelli, dl-linux-imx, netdev, devicetree, linux-kernel 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&data=04%7C01%7Cqiangqin > g.zhang%40nxp.com%7Ca2666831ffdc43a0a27908d924f33ee9%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581449420768288%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiLCJXVCI6Mn0%3D%7C1000&sdata=4SFmi5Fst3r7jPzy%2F%2B%2Bej88 > GN7N91e1DxD7GyrA2dig%3D&reserved=0 > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock 2021-06-01 9:04 ` [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock Joakim Zhang 2021-06-01 11:48 ` Russell King (Oracle) @ 2021-06-01 22:07 ` Andrew Lunn 2021-06-02 2:23 ` Joakim Zhang 1 sibling, 1 reply; 14+ messages in thread From: Andrew Lunn @ 2021-06-01 22:07 UTC (permalink / raw) To: Joakim Zhang Cc: davem, kuba, robh+dt, hkallweit1, linux, f.fainelli, linux-imx, netdev, devicetree, linux-kernel > +struct rtl821x_priv { > + u32 quirks; I'm not sure quirks is the correct word here. I would probably use features, or flags. Andrew ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock 2021-06-01 22:07 ` Andrew Lunn @ 2021-06-02 2:23 ` Joakim Zhang 0 siblings, 0 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-02 2:23 UTC (permalink / raw) To: Andrew Lunn Cc: davem, kuba, robh+dt, hkallweit1, linux, f.fainelli, dl-linux-imx, netdev, devicetree, linux-kernel, Russell King 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode 2021-06-01 9:04 [PATCH net-next 0/4] net: phy: add dt property for realtek phy Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock Joakim Zhang @ 2021-06-01 9:04 ` Joakim Zhang 2021-06-01 11:52 ` Russell King (Oracle) 2021-06-01 9:04 ` [PATCH net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue Joakim Zhang 3 siblings, 1 reply; 14+ messages in thread From: Joakim Zhang @ 2021-06-01 9:04 UTC (permalink / raw) To: davem, kuba, robh+dt, andrew, hkallweit1, linux, f.fainelli Cc: linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode 2021-06-01 9:04 ` [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode Joakim Zhang @ 2021-06-01 11:52 ` Russell King (Oracle) 2021-06-02 2:22 ` Joakim Zhang 0 siblings, 1 reply; 14+ messages in thread From: Russell King (Oracle) @ 2021-06-01 11:52 UTC (permalink / raw) To: Joakim Zhang Cc: davem, kuba, robh+dt, andrew, hkallweit1, f.fainelli, linux-imx, netdev, devicetree, linux-kernel 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! ^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode 2021-06-01 11:52 ` Russell King (Oracle) @ 2021-06-02 2:22 ` Joakim Zhang 0 siblings, 0 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-02 2:22 UTC (permalink / raw) To: Russell King Cc: davem, kuba, robh+dt, andrew, hkallweit1, f.fainelli, dl-linux-imx, netdev, devicetree, linux-kernel 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&data=04%7C01%7Cqiangqin > g.zhang%40nxp.com%7Ceacabd25941448301acb08d924f3b6de%7C686ea1d3b > c2b4c6fa92cd99c5c301635%7C0%7C0%7C637581451436345731%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW > wiLCJXVCI6Mn0%3D%7C1000&sdata=3jyYeGVBXXb5jCDtyTrt3DI9MwVE > OT5Et9tpCZG26gY%3D&reserved=0 > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue 2021-06-01 9:04 [PATCH net-next 0/4] net: phy: add dt property for realtek phy Joakim Zhang ` (2 preceding siblings ...) 2021-06-01 9:04 ` [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode Joakim Zhang @ 2021-06-01 9:04 ` Joakim Zhang 3 siblings, 0 replies; 14+ messages in thread From: Joakim Zhang @ 2021-06-01 9:04 UTC (permalink / raw) To: davem, kuba, robh+dt, andrew, hkallweit1, linux, f.fainelli Cc: linux-imx, netdev, devicetree, linux-kernel 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-06-02 3:14 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-01 9:04 [PATCH net-next 0/4] net: phy: add dt property for realtek phy Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 1/4] dt-bindings: net: add dt binding for realtek rtl82xx phy Joakim Zhang 2021-06-01 13:32 ` Rob Herring 2021-06-02 2:39 ` Andrew Lunn 2021-06-02 3:14 ` Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 2/4] net: phy: realtek: add dt property to disable CLKOUT clock Joakim Zhang 2021-06-01 11:48 ` Russell King (Oracle) 2021-06-02 2:18 ` Joakim Zhang 2021-06-01 22:07 ` Andrew Lunn 2021-06-02 2:23 ` Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 3/4] net: phy: realteck: add dt property to disable ALDPS mode Joakim Zhang 2021-06-01 11:52 ` Russell King (Oracle) 2021-06-02 2:22 ` Joakim Zhang 2021-06-01 9:04 ` [PATCH net-next 4/4] net: phy: realtek: add delay to fix RXC generation issue Joakim Zhang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.