| Message ID | 20230731100349.184553-3-fe@dev.tdt.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: mxl: add mxl,control-gate dts property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dt-meta-schema | fail | build log |
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.
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
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
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
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
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
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 >
>> 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
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"; }; ...
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(+)