On Mon 22 Apr 2024 at 09:16, Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote: > On Sun 21 Apr 2024 at 20:14, Krzysztof Kozlowski <krzk@xxxxxxxxxx> wrote: > >> On 20/04/2024 18:15, Jan Dakinevich wrote: >>> >>> >>> On 4/20/24 00:09, Rob Herring wrote: >>>> On Fri, Apr 19, 2024 at 03:58:10PM +0300, Jan Dakinevich wrote: >>>>> Add device tree bindings for A1 SoC audio clock and reset controllers. >>>>> >>>>> Signed-off-by: Jan Dakinevich <jan.dakinevich@xxxxxxxxxxxxxxxxx> >>>>> --- >>>>> >>>>> This controller has 6 mandatory and up to 20 optional clocks. To describe >>>>> this, I use 'additionalItems'. It produces correct processed-schema.json: >>>>> >>>>> "clock-names": { >>>>> "maxItems": 26, >>>>> "items": [ >>>>> { >>>>> "const": "pclk" >>>>> }, >>>>> { >>>>> "const": "dds_in" >>>>> }, >>>>> { >>>>> "const": "fclk_div2" >>>>> }, >>>>> { >>>>> "const": "fclk_div3" >>>>> }, >>>>> { >>>>> "const": "hifi_pll" >>>>> }, >>>>> { >>>>> "const": "xtal" >>>>> } >>>>> ], >>>>> "additionalItems": { >>>>> "oneOf": [ >>>>> { >>>>> "pattern": "^slv_sclk[0-9]$" >>>>> }, >>>>> { >>>>> "pattern": "^slv_lrclk[0-9]$" >>>>> } >>>>> ] >>>>> }, >>>>> "type": "array", >>>>> "minItems": 6 >>>>> }, >>>>> >>>>> and it behaves as expected. However, the checking is followed by >>>>> complaints like this: >>>>> >>>>> Documentation/devicetree/bindings/clock/amlogic,a1-audio-clkc.yaml: properties:clock-names:additionalItems: {'oneOf': [{'pattern': '^slv_sclk[0-9]$'}, {'pattern': '^slv_lrclk[0-9]$'}]} is not of type 'boolean' >>>>> >>>>> And indeed, 'additionalItems' has boolean type in meta-schema. So, how to >>>>> do it right? >>>> >>>> The meta-schemas are written both to prevent nonsense that json-schema >>>> allows by default (e.g additionalitems (wrong case)) and constraints to >>>> follow the patterns we expect. I'm happy to loosen the latter case if >>>> there's really a need. >>>> >>>> Generally, most bindings shouldn't be using 'additionalItems' at all as >>>> all entries should be defined, but there's a few exceptions. Here, the >>>> only reasoning I see is 26 entries is a lot to write out, but that >>>> wouldn't really justify it. >>> >>> Writing a lot of entries don't scary me too much, but the reason is that >>> the existence of optional clock sources depends on schematics. Also, we >> >> Aren't you documenting SoC component, not a board? So how exactly it >> depends on schematics? SoC is done or not done... >> >>> unable to declare dt-nodes for 'clocks' array in any generic way, >>> because their declaration would depends on that what is actually >>> connected to the SoC (dt-node could be "fixed-clock" with specific rate >>> or something else). >> >> So these are clock inputs to the SoC? >> > > Yes, possibly. > Like an external crystal or a set clocks provided by an external codec > where the codec is the clock master of the link. > > This is same case as the AXG that was discussed here: > https://lore.kernel.org/linux-devicetree/20230808194811.113087-1-alexander.stein@xxxxxxxxxxx/ > > IMO, like the AXG, only the pclk is a required clock. > All the others - master and slave clocks - are optional. > The controller is designed to operate with grounded inputs Looking again at the implementation of the controller, there is a clear indication in patch 3 that the controller interface is the same as the AXG and that the above statement is true. The AXG had 8 master clocks wired in. The A1 just has 5 - and 3 grounded master clocks. This is why you to had to provide a mux input table to skip the grounded inputs. You would not have to do so if the controller was properly declared with the 8 master clock input, as it actually is. It also shows that it is a bad idea to name input after what is coming in (like you do with "dds_in" or "fclk_div2") instead of what they actually are like in the AXG (mst0, mst1, etc ...) > >>> >>> By the way, I don't know any example (neither for A1 SoC nor for other >>> Amlogic's SoCs) where these optional clocks are used, but they are >>> allowed by hw. > > Those scenario exists and have been tested. There is just no dts using > that upstream because they are all mostly copy of the AML ref design. > >>> >>> This is my understanding of this controller. I hope, Jerome Brunet will >>> clarify how it actually works. >> > > I think the simpliest way to deal with this to just list all the clocks > with 'minItems = 1'. It is going be hard to read with a lot of '<0>,' in > the DTS when do need those slave clocks but at least the binding doc > will be simple. > >> Best regards, >> Krzysztof > > If you are going ahead with this, please name the file > amlogic,axg-audio-clkc.yaml because this is really the first controller > of the type and is meant to be documented in the same file. > > You are free to handle the conversion of the AXG at the same time if > you'd like. It would be much appreciated if you do. -- Jerome