Skip to contentSkip to conversation

clk: mxl: add mxl,control-gate dts property

PATCH In review Florian Eckert 4 participants 11 messages Jul 31, 2023 – Aug 9, 2023

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover clk: mxl: add mxl,control-gate dts property

Gate clocks can be controlled either from this cgu clk driver or directly from power management driver/daemon. It is dependent on the power policy/profile requirements of the end product. To take control of gate clks from this driver.

Until now, the source code had to be changed for this purpose by adding the flag 'GATE_CLK_HW' to the LGM_GATE macro in the source file 'drivers/clk/x86/clk-lgm.c'.

This can be better handled via the device tree, so that the source no longer needs to be changed. For this purpose, a new option 'mxl,control-gate' is added, which specifies that the gate is controlled by this driver.

Florian Eckert (2):
clk: mxl: add mxl,control-gate dts property
dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

 .../bindings/clock/intel,cgu-lgm.yaml         | 11 +++++++
 drivers/clk/x86/clk-cgu.c                     | 30 +++++++++++--------
 2 files changed, 28 insertions(+), 13 deletions(-)
Signature · 1 line
2.30.2
#1 lore raw

PATCH 1/2 clk: mxl: add mxl,control-gate dts property

Gate clocks can be controlled either from this cgu clk driver or directly from power management driver/daemon. It is dependent on the power policy/profile requirements of the end product. To take control of gate clks from this driver.

Until now, the source code had to be changed for this purpose by adding the flag 'GATE_CLK_HW' to the LGM_GATE macro in the source file 'drivers/clk/x86/clk-lgm.c'.

This can be better handled via the device tree, so that the source no longer needs to be changed. For this purpose, a new option 'mxl,control-gate' is added, which specifies that the gate is controlled by this driver.

---
 drivers/clk/x86/clk-cgu.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)
drivers/clk/x86/clk-cgu.c +17 -13
index 89b53f280aee..cb4e92ea54bf 100644
@@ -339,6 +339,8 @@ int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
 {
 	struct clk_hw *hw;
 	unsigned int idx;
+	const char *name;
+	unsigned int count, i;
 
 	for (idx = 0; idx < nr_clk; idx++, list++) {
 		switch (list->type) {
@@ -355,19 +357,21 @@ int lgm_clk_register_branches(struct lgm_clk_provider *ctx,
 			hw = lgm_clk_register_fixed_factor(ctx, list);
 			break;
 		case CLK_TYPE_GATE:
-			if (list->gate_flags & GATE_CLK_HW) {
-				hw = lgm_clk_register_gate(ctx, list);
-			} else {
-				/*
-				 * GATE_CLKs can be controlled either from
-				 * CGU clk driver i.e. this driver or directly
-				 * from power management driver/daemon. It is
-				 * dependent on the power policy/profile requirements
-				 * of the end product. To override control of gate
-				 * clks from this driver, provide NULL for this index
-				 * of gate clk provider.
-				 */
-				hw = NULL;
+			/* Check if cgu should control the gate clock */
+			hw = NULL;
+			count = of_property_count_strings(ctx->np,
+							  "mxl,control-gate");
+			if (count <= 0)
+				break;
+			for (i = 0; i < count; i++) {
+				of_property_read_string_index(ctx->np,
+							      "mxl,control-gate",
+							      i, &name);
+				if (!strncmp(list->name, name, strlen(list->name))) {
+					dev_err(ctx->dev, "enable gate control for %s\n",
+						list->name);
+					hw = lgm_clk_register_gate(ctx, list);
+				}
 			}
 			break;
 
Signature · 1 line
2.30.2
#2 lore raw

PATCH 2/2 dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

In reply to Florian Eckert

Add the new option 'mxl,control-gate'. Gate clocks can be controlled either from this cgu clk driver or directly from power management driver/daemon. It is dependent on the power policy/profile requirements of the end product. To take control of gate clks from this driver, add the name of the gate to this <mxl,control-gate> devicetree property. Please refer to 'drivers/clk/x86/clk-lgm.c' source file for the gate names.

---
 .../devicetree/bindings/clock/intel,cgu-lgm.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)
Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml +11 -0
index 76609a390429..755d13a65477 100644
@@ -28,6 +28,16 @@ properties:
   '#clock-cells':
     const: 1
 
+    mxl,control-gate:
+      description:
+        Gate clocks can be controlled either from this cgu clk driver or
+        directly from power management driver/daemon. It is dependent on the
+        power policy/profile requirements of the end product. To take
+        control of gate clks from this driver, add the name of the gate
+        to this <mxl,control-gate> devicetree property. Please refer to
+        drivers/clk/x86/clk-lgm.c source file for the gate names.
+      $ref: /schemas/types.yaml#/definitions/string-array
+
 required:
   - compatible
   - reg
@@ -41,6 +51,7 @@ examples:
         compatible = "intel,cgu-lgm";
         reg = <0xe0200000 0x33c>;
         #clock-cells = <1>;
+        mxl,control-gate = "g_gptc0", "g_gptc1";
     };
 
 ...
Signature · 1 line
2.30.2
maintainer
#3 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Quoted 7 lines · Florian Eckert

Add the new option 'mxl,control-gate'. Gate clocks can be controlled either from this cgu clk driver or directly from power management driver/daemon. It is dependent on the power policy/profile requirements of the end product. To take control of gate clks from this driver, add the name of the gate to this <mxl,control-gate> devicetree property. Please refer to 'drivers/clk/x86/clk-lgm.c' source file for the gate names.

Quoted 5 lines
---
 .../devicetree/bindings/clock/intel,cgu-lgm.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

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/dt-review-ci/linux/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml: properties:#clock-cells: 'anyOf' conditional failed, one must be fixed:
'mxl,control-gate' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'typeSize', 'unevaluatedProperties', 'uniqueItems']
'type' was expected
from schema $id: http://devicetree.org/meta-schemas/keywords.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml: properties:#clock-cells: 'mxl,control-gate' is not one of ['description', 'deprecated', 'const', 'enum', 'minimum', 'maximum', 'multipleOf', 'default', '$ref', 'oneOf']
from schema $id: http://devicetree.org/meta-schemas/core.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/intel,cgu-lgm.example.dtb: clock-controller@e0200000: 'mxl,control-gate' does 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),.*', '^100ask,.*', '^70mai,.*', '^8dev,.*', '^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,.*', '^acbel,.*', '^acer,.*', '^acme,.*', '^actions,.*', '^active-semi,.*', '^ad,.*', '^adafruit,.*', '^adapteva,.*', '^adaptrum,.*', '^adh,.*', '^adi,.*', '^advantech,.*', '^aeroflexgaisler,.*', '^aesop,.*', '^airoha,.*', '^al,.*', '^alcatel,.*', '^aldec,.*', '^alfa-network,.*', '^allegro,.*', '
^allo,.*', '^allwinner,.*', '^alphascale,.*', '^alps,.*', '^alt,.*', '^altr,.*', '^amarula,.*', '^amazon,.*', '^amcc,.*', '^amd,.*', '^amediatech,.*', '^amlogic,.*', '^ampere,.*', '^ampire,.*', '^ams,.*', '^amstaos,.*', '^analogix,.*', '^anbernic,.*', '^andestech,.*', '^anvo,.*', '^apm,.*', '^apple,.*', '^aptina,.*', '^arasan,.*', '^archermind,.*', '^arctic,.*', '^arcx,.*', '^aries,.*', '^arm,.*', '^armadeus,.*', '^arrow,.*', '^artesyn,.*', '^asahi-kasei,.*', '^asc,.*', '^asix,.*', '^aspeed,.*', '^asrock,.*', '^asus,.*', '^atheros,.*', '^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,.*', '^bsh,.*', '^bticino,.*', '^buffalo,.*', '^bur,.*', '^bytedance,.*', '^calamp,.*', '^calaosystems,.*', '^calxeda,.*', '^ canaan,.*', '^caninos,.*', '^capella,.*', '^cascoda,.*', '^catalyst,.*', '^cavium,.*', '^cdns,.*', '^cdtech,.*', '^cellwise,.*', '^ceva,.*', '^chargebyte,.*', '^checkpoint,.*', '^chefree,.*', '^chipidea,.*', '^chipone,.*', '^chipspark,.*', '^chongzhou,.*', '^chrontel,.*', '^chrp,.*', '^chunghwa,.*', '^chuwi,.*', '^ciaa,.*', '^cirrus,.*', '^cisco,.*', '^clockwork,.*', '^cloos,.*', '^cloudengines,.*', '^cnm,.*', '^cnxt,.*', '^colorfly,.*', '^compulab,.*', '^congatec,.*', '^coreriver,.*', '^corpro,.*', '^cortina,.*', '^cosmic,.*', '^crane,.*', '^creative,.*', '^crystalfontz,.*', '^csky,.*', '^csq,.*', '^ctera,.*', '^ctu,.*', '^cubietech,.*', '^cui,.*', '^cypress,.*', '^cyx,.*', '^cznic,.*', '^dallas,.*', '^dataimage,.*', '^davicom,.*', '^dell,.*', '^delta,.*', '^densitron,.*', '^denx,.*', '^devantech,.*', '^dfi,.*', '^dh,.*', '^difrnce,.*', '^digi,.*', '^digilent,.*', '^diodes,.*', '^dioo,.*', '^dlc,.*', '^dlg,.*', '^dlink,.*', '^dmo,.*', '^domintech,.*', '^dongwoon,.*', '^dptechnics,. *', '^dragino,.*', '^ds,.*', '^dserve,.*', '^dynaimage,.*', '^ea,.*', '^ebang,.*', '^ebbg,.*', '^ebs-systart,.*', '^ebv,.*', '^eckelmann,.*', '^edgeble,.*', '^edimax,.*', '^edt,.*', '^ees,.*', '^eeti,.*', '^einfochips,.*', '^eink,.*', '^elan,.*', '^element14,.*', '^elgin,.*', '^elida,.*', '^elimo,.*', '^elpida,.*', '^embedfire,.*', '^embest,.*', '^emlid,.*', '^emmicro,.*', '^empire-electronix,.*', '^emtrion,.*', '^enclustra,.*', '^endless,.*', '^ene,.*', '^energymicro,.*', '^engicam,.*', '^engleder,.*', '^epcos,.*', '^epfl,.*', '^epson,.*', '^esp,.*', '^est,.*', '^ettus,.*', '^eukrea,.*', '^everest,.*', '^everspin,.*', '^evervision,.*', '^exar,.*', '^excito,.*', '^exegin,.*', '^ezchip,.*', '^facebook,.*', '^fairphone,.*', '^faraday,.*', '^fastrax,.*', '^fcs,.*', '^feixin,.*', '^feiyang,.*', '^fii,.*', '^firefly,.*', '^focaltech,.*', '^forlinx,.*', '^frida,.*', '^friendlyarm,.*', '^fsl,.*', '^fujitsu,.*', '^fxtec,.*', '^gardena,.*', '^gateworks,.*', '^gcw,.*', '^ge,.*', '^geekbuying, .*', '^gef,.*', '^gemei,.*', '^genesys,.*', '^geniatech,.*', '^giantec,.*', '^giantplus,.*', '^globalscale,.*', '^globaltop,.*', '^gmt,.*', '^goldelico,.*', '^goodix,.*', '^google,.*', '^gplus,.*', '^grinn,.*', '^grmn,.*', '^gumstix,.*', '^gw,.*', '^hannstar,.*', '^haochuangyi,.*', '^haoyu,.*', '^hardkernel,.*', '^hechuang,.*', '^hideep,.*', '^himax,.*', '^hirschmann,.*', '^hisi,.*', '^hisilicon,.*', '^hit,.*', '^hitex,.*', '^holt,.*', '^holtek,.*', '^honestar,.*', '^honeywell,.*', '^hoperun,.*', '^hp,.*', '^hpe,.*', '^hsg,.*', '^huawei,.*', '^hugsun,.*', '^hwacom,.*', '^hxt,.*', '^hycon,.*', '^hydis,.*', '^hynitron,.*', '^hynix,.*', '^hyundai,.*', '^i2se,.*', '^ibm,.*', '^icplus,.*', '^idt,.*', '^ifi,.*', '^ilitek,.*', '^imagis,.*', '^img,.*', '^imi,.*', '^incircuit,.*', '^indiedroid,.*', '^inet-tek,.*', '^infineon,.*', '^inforce,.*', '^ingenic,.*', '^ingrasys,.*', '^injoinic,.*', '^innocomm,.*', '^innolux,.*', '^inside-secure,.*', '^insignal,.*', '^inspur,.*', '^intel,.*', '^inter control,.*', '^invensense,.*', '^inventec,.*', '^inversepath,.*', '^iom,.*', '^irondevice,.*', '^isee,.*', '^isil,.*', '^issi,.*', '^ite,.*', '^itead,.*', '^itian,.*', '^ivo,.*', '^iwave,.*', '^jadard,.*', '^jdi,.*', '^jedec,.*', '^jesurun,.*', '^jethome,.*', '^jianda,.*', '^joz,.*', '^kam,.*', '^karo,.*', '^keithkoep,.*', '^keymile,.*', '^khadas,.*', '^kiebackpeter,.*', '^kinetic,.*', '^kingdisplay,.*', '^kingnovel,.*', '^kionix,.*', '^kobo,.*', '^kobol,.*', '^koe,.*', '^kontron,.*', '^kosagi,.*', '^kvg,.*', '^kyo,.*', '^lacie,.*', '^laird,.*', '^lamobo,.*', '^lantiq,.*', '^lattice,.*', '^lctech,.*', '^leadtek,.*', '^leez,.*', '^lego,.*', '^lemaker,.*', '^lenovo,.*', '^lg,.*', '^lgphilips,.*', '^libretech,.*', '^licheepi,.*', '^linaro,.*', '^lineartechnology,.*', '^linksprite,.*', '^linksys,.*', '^linutronix,.*', '^linux,.*', '^linx,.*', '^liteon,.*', '^litex,.*', '^lltc,.*', '^logicpd,.*', '^logictechno,.*', '^longcheer,.*', '^lontium,.*', '^loongmasses,.*', '^loongson,.*', '^lsi, .*', '^lunzn,.*', '^lwn,.*', '^lxa,.*', '^m5stack,.*', '^macnica,.*', '^mantix,.*', '^mapleboard,.*', '^marvell,.*', '^maxbotix,.*', '^maxim,.*', '^maxlinear,.*', '^mbvl,.*', '^mcube,.*', '^meas,.*', '^mecer,.*', '^mediatek,.*', '^megachips,.*', '^mele,.*', '^melexis,.*', '^melfas,.*', '^mellanox,.*', '^memsensing,.*', '^memsic,.*', '^menlo,.*', '^mentor,.*', '^meraki,.*', '^merrii,.*', '^methode,.*', '^micrel,.*', '^microchip,.*', '^microcrystal,.*', '^micron,.*', '^microsoft,.*', '^microsys,.*', '^mikroe,.*', '^mikrotik,.*', '^miniand,.*', '^minix,.*', '^miramems,.*', '^mitsubishi,.*', '^mixel,.*', '^miyoo,.*', '^mntre,.*', '^modtronix,.*', '^moortec,.*', '^mosaixtech,.*', '^motorcomm,.*', '^motorola,.*', '^moxa,.*', '^mpl,.*', '^mps,.*', '^mqmaker,.*', '^mrvl,.*', '^mscc,.*', '^msi,.*', '^mstar,.*', '^mti,.*', '^multi-inno,.*', '^mundoreader,.*', '^murata,.*', '^mxic,.*', '^mxicy,.*', '^myir,.*', '^national,.*', '^nec,.*', '^neonode,.*', '^netgear,.*', '^netlogic,.*', '^netron-dy ,.*', '^netronix,.*', '^netxeon,.*', '^neweast,.*', '^newhaven,.*', '^newvision,.*', '^nexbox,.*', '^nextthing,.*', '^ni,.*', '^nintendo,.*', '^nlt,.*', '^nokia,.*', '^nordic,.*', '^novatek,.*', '^novtech,.*', '^nutsboard,.*', '^nuvoton,.*', '^nvd,.*', '^nvidia,.*', '^nxp,.*', '^oceanic,.*', '^ocs,.*', '^oct,.*', '^okaya,.*', '^oki,.*', '^olimex,.*', '^olpc,.*', '^oneplus,.*', '^onie,.*', '^onion,.*', '^onnn,.*', '^ontat,.*', '^opalkelly,.*', '^openailab,.*', '^opencores,.*', '^openembed,.*', '^openpandora,.*', '^openrisc,.*', '^option,.*', '^oranth,.*', '^orisetech,.*', '^ortustech,.*', '^osddisplays,.*', '^osmc,.*', '^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,.*', '^polyhex, .*', '^portwell,.*', '^poslab,.*', '^pov,.*', '^powertip,.*', '^powervr,.*', '^primux,.*', '^probox2,.*', '^prt,.*', '^pulsedlight,.*', '^purism,.*', '^qca,.*', '^qcom,.*', '^qemu,.*', '^qi,.*', '^qiaodian,.*', '^qihua,.*', '^qishenglong,.*', '^qnap,.*', '^quanta,.*', '^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,.*', '^senseair,.*', '^sensirion,.*', '^sensortek,.*', '^sercomm,.*', '^sff,.*', '^sgd,.*', '^sgmicro,.*', '^sgx,.*', '^sharp,.*', '^shift,.*', '^shimafuji,.*', '^shineworld,.*', '^shiratech,.*', '^si-en,.*', '^si -linux,.*', '^siemens,.*', '^sifive,.*', '^sigma,.*', '^sii,.*', '^sil,.*', '^silabs,.*', '^silan,.*', '^silead,.*', '^silergy,.*', '^silex-insight,.*', '^siliconfile,.*', '^siliconmitus,.*', '^silvaco,.*', '^simtek,.*', '^sinlinx,.*', '^sinovoip,.*', '^sinowealth,.*', '^sipeed,.*', '^sirf,.*', '^sis,.*', '^sitronix,.*', '^skov,.*', '^skyworks,.*', '^smartlabs,.*', '^smsc,.*', '^snps,.*', '^sochip,.*', '^socionext,.*', '^solidrun,.*', '^solomon,.*', '^sony,.*', '^sourceparts,.*', '^spansion,.*', '^sparkfun,.*', '^spinalhdl,.*', '^sprd,.*', '^square,.*', '^ssi,.*', '^sst,.*', '^sstar,.*', '^st,.*', '^st-ericsson,.*', '^starfive,.*', '^starry,.*', '^startek,.*', '^starterkit,.*', '^ste,.*', '^stericsson,.*', '^storlink,.*', '^storm,.*', '^storopack,.*', '^summit,.*', '^sunchip,.*', '^sundance,.*', '^sunplus,.*', '^supermicro,.*', '^swir,.*', '^syna,.*', '^synology,.*', '^synopsys,.*', '^tbs,.*', '^tbs-biometrics,.*', '^tcg,.*', '^tcl,.*', '^tcs,.*', '^tdo,.*', '^team-source-display,.* ', '^technexion,.*', '^technologic,.*', '^techstar,.*', '^teejet,.*', '^teltonika,.*', '^tempo,.*', '^terasic,.*', '^tesla,.*', '^tfc,.*', '^thead,.*', '^thine,.*', '^thingyjp,.*', '^thundercomm,.*', '^thwc,.*', '^ti,.*', '^tianma,.*', '^tlm,.*', '^tmt,.*', '^topeet,.*', '^topic,.*', '^toppoly,.*', '^topwise,.*', '^toradex,.*', '^toshiba,.*', '^toumaz,.*', '^tpk,.*', '^tplink,.*', '^tpo,.*', '^tq,.*', '^traverse,.*', '^tronfy,.*', '^tronsmart,.*', '^truly,.*', '^tsd,.*', '^tyan,.*', '^u-blox,.*', '^u-boot,.*', '^ubnt,.*', '^ucrobotics,.*', '^udoo,.*', '^ufispace,.*', '^ugoos,.*', '^uniwest,.*', '^upisemi,.*', '^urt,.*', '^usi,.*', '^utoo,.*', '^v3,.*', '^vaisala,.*', '^vamrs,.*', '^variscite,.*', '^vdl,.*', '^vertexcom,.*', '^via,.*', '^vialab,.*', '^vicor,.*', '^videostrong,.*', '^virtio,.*', '^virtual,.*', '^vishay,.*', '^visionox,.*', '^vitesse,.*', '^vivante,.*', '^vivax,.*', '^vocore,.*', '^voipac,.*', '^vot,.*', '^vxt,.*', '^wanchanglong,.*', '^wand,.*', '^waveshare,.*', '^wd, .*', '^we,.*', '^welltech,.*', '^wetek,.*', '^wexler,.*', '^whwave,.*', '^wi2wi,.*', '^widora,.*', '^wiligear,.*', '^willsemi,.*', '^winbond,.*', '^wingtech,.*', '^winlink,.*', '^winstar,.*', '^wirelesstag,.*', '^wits,.*', '^wlf,.*', '^wm,.*', '^wobo,.*', '^x-powers,.*', '^xen,.*', '^xes,.*', '^xiaomi,.*', '^xillybus,.*', '^xingbangda,.*', '^xinpeng,.*', '^xiphera,.*', '^xlnx,.*', '^xnano,.*', '^xunlong,.*', '^xylon,.*', '^yadro,.*', '^yamaha,.*', '^yes-optoelectronics,.*', '^yic,.*', '^yiming,.*', '^ylm,.*', '^yna,.*', '^yones-toptech,.*', '^ys,.*', '^ysoft,.*', '^zarlink,.*', '^zealz,.*', '^zeitec,.*', '^zidoo,.*', '^zii,.*', '^zinitix,.*', '^zkmagic,.*', '^zte,.*', '^zyxel,.*', 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/vendor-prefixes.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/clock/intel,cgu-lgm.example.dtb: clock-controller@e0200000: 'mxl,control-gate' does not match any of the regexes: 'pinctrl-[0-9]+'
from schema $id: http://devicetree.org/schemas/clock/intel,cgu-lgm.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/[email protected]

The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.

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 after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.

#4 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

In reply to Florian Eckert
Quoted 6 lines · Florian Eckert

Add the new option 'mxl,control-gate'. Gate clocks can be controlled either from this cgu clk driver or directly from power management driver/daemon. It is dependent on the power policy/profile requirements of the end product. To take control of gate clks from this driver, add the name of the gate to this <mxl,control-gate> devicetree property. Please refer to 'drivers/clk/x86/clk-lgm.c' source file for the gate names.

Choosing which driver controls clocks is OS policy, not DT property. Any reference to drivers in property description is already a warning sign.

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

Best regards,
Krzysztof

#5 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Thanks for your reply,

Quoted 4 lines

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

You have correctly identified that this is not a hardware configuration, but a driver configuration. Currently, the driver is configured so that the gates cannot be switched via the clk subsystem callbacks. When registering the data structures from the driver, I have to pass a flag GATE_CLK_HW so that the gate is managed by the driver.

I didn't want to always change the source of the driver when it has to take care of the GATE, so I wanted to map this via the dts.

I have a board support package from Maxlinear for the Lightning Mountain Soc with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Due to missing documents it is not possible to send these drivers upstream. Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

Best regards

Florian

#6 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Quoted 17 lines · Florian Eckert

Thanks for your reply,

Quoted 4 lines

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

You have correctly identified that this is not a hardware configuration, but a driver configuration. Currently, the driver is configured so that the gates cannot be switched via the clk subsystem callbacks. When registering the data structures from the driver, I have to pass a flag GATE_CLK_HW so that the gate is managed by the driver.

I didn't want to always change the source of the driver when it has to take
care of the GATE, so I wanted to map this via the dts.

Quoted 7 lines

I have a board support package from Maxlinear for the Lightning Mountain Soc
with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Due to missing documents it is not possible to send these drivers upstream.

So when you upstream them, the binding becomes wrong or not needed? Sorry, bindings are entirely independent of OS, so using this as an argument is clear no-go.

Quoted 4 lines

Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

None of this explains any hardware related part of this binding. You created now policy for one specific OS. Devicetree, which is OS independent, is not for such purposes.

Best regards,
Krzysztof

#7 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Hello Krzysztof,

Quoted 20 lines
Quoted 20 lines
Quoted 4 lines

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

You have correctly identified that this is not a hardware configuration, but a driver configuration. Currently, the driver is configured so that the gates cannot be switched via the clk subsystem callbacks. When registering the data structures from the driver, I have to pass a flag GATE_CLK_HW so that the gate is managed by the driver.

I didn't want to always change the source of the driver when it has to take
care of the GATE, so I wanted to map this via the dts.

I have a board support package from Maxlinear for the Lightning Mountain Soc
with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Quoted 6 lines
Quoted 2 lines

Due to missing documents it is not possible to send these drivers upstream.

So when you upstream them, the binding becomes wrong or not needed? Sorry, bindings are entirely independent of OS, so using this as an argument is clear no-go.

Yes, that would probably be the case, as the maxlinear drivers are at an early stage and are not yet upstreamable in my opinion. If I had the documents, I would take a closer look. But they are developing behind closed doors. Nothing can be contributed. Not until the drivers are hopefully upstream at some point as the cgu-lgm.

Quoted 8 lines
Quoted 4 lines

Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

None of this explains any hardware related part of this binding. You created now policy for one specific OS. Devicetree, which is OS independent, is not for such purposes.

Yes this would be the case. Maybe I need to patch the cgu-lgm.c [1] and send it upstream to restore the old behavior. Because the following commit has changed the behaviour [2]. Unfortunately, it is also included in 5.15 stable branch. Which in my opinion should not have happened!

Best regards,
Florian

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/x86/clk-lgm.c?h=v6.5-rc4
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/clk/x86/clk-cgu.c?h=v5.15.123&id=a0583edea4fdb7b5b87a077263dddab476e9f138

#8 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Quoted 47 lines · Florian Eckert

Hello Krzysztof,

Quoted 23 lines
Quoted 23 lines
Quoted 4 lines

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

You have correctly identified that this is not a hardware configuration,
but a driver configuration. Currently, the driver is configured so that
the gates cannot be switched via the clk subsystem callbacks. When registering the data structures from the driver, I have to pass a flag GATE_CLK_HW so that the gate is managed by the driver.

I didn't want to always change the source of the driver when it has to take
care of the GATE, so I wanted to map this via the dts.

I have a board support package from Maxlinear for the Lightning Mountain
Soc
with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Quoted 6 lines
Quoted 2 lines

Due to missing documents it is not possible to send these drivers upstream.

So when you upstream them, the binding becomes wrong or not needed? Sorry, bindings are entirely independent of OS, so using this as an argument is clear no-go.

Yes, that would probably be the case, as the maxlinear drivers are at an early stage and are not yet upstreamable in my opinion. If I had the documents, I would take a closer look. But they are developing behind closed doors. Nothing can be contributed. Not until the drivers are hopefully upstream at some point as the cgu-lgm.

Quoted 8 lines
Quoted 4 lines

Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

None of this explains any hardware related part of this binding. You created now policy for one specific OS. Devicetree, which is OS independent, is not for such purposes.

Quoted 5 lines

Yes this would be the case. Maybe I need to patch the cgu-lgm.c [1] and send it upstream to restore the old behavior. Because the following commit has changed the behaviour [2]. Unfortunately, it is also included in 5.15 stable branch. Which in my opinion should not have happened!

Then unfortunately this is not a correct change.

Best regards,
Krzysztof

#9 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

In reply to Florian Eckert
Quoted 24 lines · Florian Eckert

This email was sent from outside of MaxLinear.

Thanks for your reply,

Quoted 4 lines

You described the desired Linux feature or behavior, not the actual hardware. The bindings are about the latter, so instead you need to rephrase the property and its description to match actual hardware capabilities/features/configuration etc.

You have correctly identified that this is not a hardware configuration, but a driver configuration. Currently, the driver is configured so that the gates cannot be switched via the clk subsystem callbacks. When registering the data structures from the driver, I have to pass a flag GATE_CLK_HW so that the gate is managed by the driver.

I didn't want to always change the source of the driver when it has to take
care of the GATE, so I wanted to map this via the dts.

I have a board support package from Maxlinear for the Lightning Mountain Soc
with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Quoted 6 lines

Due to missing documents it is not possible to send these drivers upstream.
Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

Could you please give more details on your target?

In what kind of condition, you want to change the flag?

In LGM SoC,  some gate clocks can be covered by EPU (power management module).

that is the reason clock driver introduced the HW/SW flag definition.

However gptc and watchdog are not covered by EPU.  it can only be controlled via clock

driver.   So I'm not quite sure the target to change the flag for these two clocks.

IMHO,  it's HW fixed in LGM SoC.

Quoted 5 lines

Best regards

Florian

#10 lore raw

Re: [PATCH 2/2] dt-bindings: clock: intel,cgu-lgm: add mxl,control-gate option

Quoted 9 lines
Quoted 9 lines

I didn't want to always change the source of the driver when it has to take
care of the GATE, so I wanted to map this via the dts.

I have a board support package from Maxlinear for the Lightning Mountain Soc
with other drivers that are not upstream now. Some of them use the clock framework some of them does not.

Quoted 8 lines
Quoted 6 lines

Due to missing documents it is not possible to send these drivers upstream.
Strictly speaking, this is about the gptc and the watchdog.

Since it is a buildin_platform driver, it can also not work via module parameters.

Could you please give more details on your target?

We are currently putting our own board with the new Maxlinear URX85x into operation. From Maxlinear we have received a board support package BSP named UGW 9.1.45.

Since we not only have Maxlinear devices, but also other SoC from other manufacturers, we cannot use the BSP. We therefore need to use OpenWrt vanilla (master, openwrt-23.05) to support all our devices with the same software stack.

We have therefore picked all the relevant software components from UGW and ported them to the next openwrt-23.05 stable release.

Due to the last rebasing of the kernel by openwrt to 5.15.123 [1], this patch [2] has been included. After that change, the gptc and watchdog drivers can't find the cgu device tree handlers and stopped working!

Quoted 1 line

In what kind of condition, you want to change the flag?

Since we don't have access to the latest internal MxL code base, we don't know what has been changed since then. Therefore we ended up with reverting your last change [2] to get the gptc and watchdog driver working again.

Quoted 2 lines

In LGM SoC,  some gate clocks can be covered by EPU (power management module).

We have already seen that in your BSP. But this driver is not integrated upstream and is unfortunately only maintained in the BSP :-(. So no one knows about that.

Quoted 1 line

that is the reason clock driver introduced the HW/SW flag definition.

Since we don't have a hardware description, but only your drivers, it is difficult for us to find out how everything fits together. In addition, not all drivers are upstream, which makes it even more confusing for us.

Quoted 4 lines

However gptc and watchdog are not covered by EPU.  it can only be controlled via clock
driver.   So I'm not quite sure the target to change the flag for these two clocks.

So only Maxlinear knows the internals, then this is up to you to make the right choice.

The major problem is that not all relevant drivers are upstreamed. Mixing upstreamed and proprietary drivers definitely leads to regressions.

By the way, up to now the SoC is not selectable. There are patches needed from your BSP for the kernel to make the SoC URX85x selectable with 'make menuconfig'

Can you *please* integrate them upstream, so that the patches do not always have to be extracted and rebased from the BSP!

We can discuss this in a separate thread.
Thanks for your feedback

Best regards

Florian

[1] https://git.openwrt.org/?p=openwrt/openwrt.git;a=commit;h=7efec0acca80b231ab8e69729a4bdaf11ef84541
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/clk/x86/clk-cgu.c?h=linux-5.15.y&id=a0583edea4fdb7b5b87a077263dddab476e9f138