[PATCH 0/2] clk: mxl: add mxl,control-gate dts property

[PATCH 0/2] clk: mxl: add mxl,control-gate dts property

[Florian Eckert]

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(-)

-- 
2.30.2

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

[Florian Eckert]

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.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/clk/x86/clk-cgu.c | 30 +++++++++++++++++-------------
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/drivers/clk/x86/clk-cgu.c b/drivers/clk/x86/clk-cgu.c
index 89b53f280aee..cb4e92ea54bf 100644
--- a/drivers/clk/x86/clk-cgu.c
+++ b/drivers/clk/x86/clk-cgu.c
@@ -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;
 
-- 
2.30.2

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

[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.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../devicetree/bindings/clock/intel,cgu-lgm.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml b/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
index 76609a390429..755d13a65477 100644
--- a/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
+++ b/Documentation/devicetree/bindings/clock/intel,cgu-lgm.yaml
@@ -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";
     };
 
 ...
-- 
2.30.2

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

[Rob Herring]

On Mon, 31 Jul 2023 12:03:49 +0200, Florian Eckert wrote:
> 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.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../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/20230731100349.184553-3-fe@dev.tdt.de

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.

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

[Krzysztof Kozlowski]

On 31/07/2023 12:03, Florian Eckert wrote:
> 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

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

[Florian Eckert]

Thanks for your reply,

> 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

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

[Krzysztof Kozlowski]

On 31/07/2023 14:59, Florian Eckert wrote:
> Thanks for your reply,
> 
>> 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.

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.

> 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

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

[Florian Eckert]

Hello Krzysztof,

>>> 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.
> 
> 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.

>> 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

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

[Krzysztof Kozlowski]

On 01/08/2023 10:09, Florian Eckert wrote:
> Hello Krzysztof,
> 
>>>> 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.
>>
>> 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.
> 
>>> 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!

Then unfortunately this is not a correct change.

Best regards,
Krzysztof

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

[Yi xin Zhu]

On 31/7/2023 8:59 pm, Florian Eckert wrote:
> This email was sent from outside of MaxLinear.
>
>
> Thanks for your reply,
>
>> 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.

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.

>
> Best regards
>
> Florian
>

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

[Florian Eckert]

>> 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.
> 
> 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!

> 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.

> 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.

> 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.

> 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