Re: [PATCH v4 1/2] dt-bindings: remoteproc: mediatek: Make l1tcm reg exclusive to mt819x

["Nícolas F. R. A. Prado" <nfraprado@collabora.com>]

On Sat, May 07, 2022 at 06:33:31PM +0200, Krzysztof Kozlowski wrote:
> On 06/05/2022 23:32, Nícolas F. R. A. Prado wrote:
> > Commit ca23ecfdbd44 ("remoteproc/mediatek: support L1TCM") added support
> > for the l1tcm memory region on the MT8192 SCP, adding a new da_to_va
> > callback that handles l1tcm while keeping the old one for
> > back-compatibility with MT8183. However, since the mt8192 compatible was
> > missing from the dt-binding, the accompanying dt-binding commit
> > 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> > mistakenly added this reg as if it were for mt8183. And later
> > it became common to all platforms as their compatibles were added.
> > 
> > Fix the dt-binding so that the l1tcm reg can, and must, be present only
> > on the supported platforms: mt8192 and mt8195.
> > 
> > Fixes: 503c64cc42f1 ("dt-bindings: remoteproc: mediatek: add L1TCM memory region")
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > ---
> > The if:then: branches became rather long since it seems that it's not
> > possible to override the properties in them, only add new ones. That is,
> > I couldn't leave the items definition for all three regs in the global
> > reg-names and just decrease minItems and maxItems to 2 for
> > mt8183/mt8186.
> > 
> > Also I had to add a description to the global reg-names, since it
> > couldn't be neither missing nor empty.
> 
> It is possible:
> https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/example-schema.yaml#L91
> 
> Keep constraints and list of names in properties. Then in allOf:if:then
> raise minItems or lower maxItems, depending on the variant.

Hi Krzysztof,

that example only shows setting minItems to override the default value, but the
issue here is that it's not possible to override minItems/maxItems (after
they're already set, even if implicitly) with a different value in the if.

That is:

	properties:
	  reg-names:
	    items:
	      - const: sram
	      - const: cfg
	      - const: l1tcm

	if:
	  properties:
	    compatible:
	      enum:
		- mediatek,mt8183-scp
		- mediatek,mt8186-scp
	then:
	  properties:
	    reg-names:
	      minItems: 2
	      maxItems: 2

Generates the error on dtbs_check:

/home/nfraprado/ext/git/linux/arch/arm64/boot/dts/mediatek/mt8183-kukui-kakadu.dtb: scp@10500000: reg-names: ['sram', 'cfg'] is too short

I believe the tooling is implicitly adding

	      minItems: 3
	      maxItems: 3

to the common reg-names, and since it's not possible to override them, the
override to 2 doesn't work so they are kept at 3, causing the error.

Moving the minItems/maxItems to the common reg-names as a test gives:

/home/nfraprado/ext/git/linux/Documentation/devicetree/bindings/remoteproc/mtk,scp.yaml: properties:reg-names: {'minItems': 2, 'maxItems': 2, 'items': [{'const': 'sram'}, {'const': 'cfg'}, {'const': 'l1tcm'}]} should not be valid under {'required': ['maxItems']}
	hint: "maxItems" is not needed with an "items" list

That error, plus looking in the items meta-schema, suggests me that maxItems
isn't supposed to be set lower then the length of items. So even if the
minItems/maxItems override is fixed, there's still this issue. It seems like
defining the reg-names list separetely in each if branch is indeed the right way
to go.

Thanks,
Nícolas