| Message ID | 20241109094623.37518-2-linux@fw-web.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fix some binding check errors for marvell | expand |
On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> > > after converting the ahci-platform binding to yaml the following files > reporting "'anyOf' conditional failed" on > > sata@540000: sata-port@0 > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > index 1e0ab35cc686..2b5e45d2c5a6 100644 > --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts > +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > @@ -214,6 +214,7 @@ &cp0_sata0 { > > sata-port@1 { > phys = <&cp0_comphy3 1>; > + status = "okay"; > }; > }; > > diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > index 7af949092b91..6bdc4f1e6939 100644 > --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > @@ -433,11 +433,13 @@ &cp0_sata0 { > /* 7 + 12 SATA connector (J24) */ > sata-port@0 { > phys = <&cp0_comphy2 0>; > + status = "okay"; > }; > > /* M.2-2250 B-key (J39) */ > sata-port@1 { > phys = <&cp0_comphy3 1>; > + status = "okay"; > }; > }; > diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > index 7e595ac80043..161beec0b6b0 100644 > --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { > > sata-port@0 { > reg = <0>; > + status = "disabled"; > }; I don't know the yaml too well, but it is not obvious how adding a few status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". Maybe you can expand the explanation a bit? Andrew
Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: >> From: Frank Wunderlich <frank-w@public-files.de> >> >> after converting the ahci-platform binding to yaml the following files >> reporting "'anyOf' conditional failed" on >> >> sata@540000: sata-port@0 >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts >> index 1e0ab35cc686..2b5e45d2c5a6 100644 >> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts >> @@ -214,6 +214,7 @@ &cp0_sata0 { >> >> sata-port@1 { >> phys = <&cp0_comphy3 1>; >> + status = "okay"; >> }; >> }; > >> >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts >> index 7af949092b91..6bdc4f1e6939 100644 >> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts >> @@ -433,11 +433,13 @@ &cp0_sata0 { >> /* 7 + 12 SATA connector (J24) */ >> sata-port@0 { >> phys = <&cp0_comphy2 0>; >> + status = "okay"; >> }; >> >> /* M.2-2250 B-key (J39) */ >> sata-port@1 { >> phys = <&cp0_comphy3 1>; >> + status = "okay"; >> }; >> }; >> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi >> index 7e595ac80043..161beec0b6b0 100644 >> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi >> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi >> @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { >> >> sata-port@0 { >> reg = <0>; >> + status = "disabled"; >> }; > >I don't know the yaml too well, but it is not obvious how adding a few >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > >Maybe you can expand the explanation a bit? > > Andrew Hi angelo, I guess the dtbs_check only checks required properties from yaml if the node is enabled. As you know, phys that can supply different types (sata,usb,pcie,*gmii,...),but only one mode can be used per phy. So only one controller can be used with it,the other(s) can not. I do not know marvell,but there are similar in mediatek (xsphy) and rockchip (combphy). From my PoV it makes sense to check only enabled nodes for required properties,but i do not know internals of dtbs_check. This patch is 2 years old and i only rebased it and run dtbs check with the others to have a clean result...i can test again without this one to check if anyOf is shown again. regards Frank
> Gesendet: Sonntag, 10. November 2024 um 10:25 > Von: "Frank Wunderlich" <frank-w@public-files.de> > Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: > >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > >> From: Frank Wunderlich <frank-w@public-files.de> > >> > >> after converting the ahci-platform binding to yaml the following files > >> reporting "'anyOf' conditional failed" on > >> > >> sata@540000: sata-port@0 ... > > > >I don't know the yaml too well, but it is not obvious how adding a few > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > >Maybe you can expand the explanation a bit? > > > > Andrew > > Hi angelo, > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. > > As you know, phys that can supply different types (sata,usb,pcie,*gmii,...),but only one mode can be used per phy. So only one controller can be used with it,the other(s) can not. I do not know marvell,but there are similar in mediatek (xsphy) and rockchip (combphy). > > From my PoV it makes sense to check only enabled nodes for required properties,but i do not know internals of dtbs_check. This patch is 2 years old and i only rebased it and run dtbs check with the others to have a clean result...i can test again without this one to check if anyOf is shown again. > > regards Frank Hi issue is still there and patch is still needed...without it i get these messages: $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/ata/ahci-platform.yaml UPD include/config/kernel.release DTC [C] arch/arm64/boot/dts/marvell/armada-7040-db.dtb arch/arm64/boot/dts/marvell/armada-7040-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/armada-7040-mochabin.dtb DTC [C] arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dtb arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/armada-8040-db.dtb DTC [C] arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtb arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dtb arch/arm64/boot/dts/marvell/armada-8040-mcbin-singleshot.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dtb DTC [C] arch/arm64/boot/dts/marvell/cn9130-db.dtb arch/arm64/boot/dts/marvell/cn9130-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9130-db-B.dtb arch/arm64/boot/dts/marvell/cn9130-db-B.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9131-db.dtb arch/arm64/boot/dts/marvell/cn9131-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# /media/data_ext/git/kernel/BPI-R2-4.14/arch/arm64/boot/dts/marvell/cn9131-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9131-db-B.dtb arch/arm64/boot/dts/marvell/cn9131-db-B.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/cn9131-db-B.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9132-db.dtb arch/arm64/boot/dts/marvell/cn9132-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/cn9132-db.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/cn9132-db.dtb: sata@540000: sata-port@1: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9132-db-B.dtb arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/cn9132-db-B.dtb: sata@540000: sata-port@1: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9130-crb-A.dtb DTC [C] arch/arm64/boot/dts/marvell/cn9130-crb-B.dtb arch/arm64/boot/dts/marvell/cn9130-crb-B.dtb: sata@540000: sata-port@1: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/ac5x-rd-carrier-cn9131.dtb arch/arm64/boot/dts/marvell/ac5x-rd-carrier-cn9131.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# arch/arm64/boot/dts/marvell/ac5x-rd-carrier-cn9131.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9130-cf-base.dtb arch/arm64/boot/dts/marvell/cn9130-cf-base.dtb: sata@540000: sata-port@1: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9130-cf-pro.dtb arch/arm64/boot/dts/marvell/cn9130-cf-pro.dtb: sata@540000: sata-port@1: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9131-cf-solidwan.dtb arch/arm64/boot/dts/marvell/cn9131-cf-solidwan.dtb: sata@540000: sata-port@0: 'anyOf' conditional failed, one must be fixed: 'phys' is a required property 'target-supply' is a required property from schema $id: http://devicetree.org/schemas/ata/ahci-platform.yaml# DTC [C] arch/arm64/boot/dts/marvell/cn9132-clearfog.dtb that imho confirms my guess that only enabled nodes are checked and without the disabled this node is always enabled and by disabling the SoC-node and enabling at board-level let the others (here printed) disabled and so not need the required properties. i can try to add short description about it, something like this: The dtbs-check only checks enabled nodes and there required nodes must be present. Nodes are enabled by default (current state for sata@540000 node), but some boards seem to use the phy somewhere else or just not want to use the sata contoller and so miss the required properties 'phys' and 'target-supply'. So disable the sata@540000 node at SoC level and enable it where it is filled with required properties. maybe adding this phrase to commit is enough? regards Frank</frank-w@public-files.de></andrew@lunn.ch></frank-w@public-files.de>
On Sun, Nov 10, 2024 at 3:25 AM Frank Wunderlich <frank-w@public-files.de> wrote: > > Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: > >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > >> From: Frank Wunderlich <frank-w@public-files.de> > >> > >> after converting the ahci-platform binding to yaml the following files > >> reporting "'anyOf' conditional failed" on > >> > >> sata@540000: sata-port@0 > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > >> index 1e0ab35cc686..2b5e45d2c5a6 100644 > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > >> @@ -214,6 +214,7 @@ &cp0_sata0 { > >> > >> sata-port@1 { > >> phys = <&cp0_comphy3 1>; > >> + status = "okay"; > >> }; > >> }; > > > >> > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > >> index 7af949092b91..6bdc4f1e6939 100644 > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > >> @@ -433,11 +433,13 @@ &cp0_sata0 { > >> /* 7 + 12 SATA connector (J24) */ > >> sata-port@0 { > >> phys = <&cp0_comphy2 0>; > >> + status = "okay"; > >> }; > >> > >> /* M.2-2250 B-key (J39) */ > >> sata-port@1 { > >> phys = <&cp0_comphy3 1>; > >> + status = "okay"; > >> }; > >> }; > >> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > >> index 7e595ac80043..161beec0b6b0 100644 > >> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > >> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > >> @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { > >> > >> sata-port@0 { > >> reg = <0>; > >> + status = "disabled"; > >> }; > > > >I don't know the yaml too well, but it is not obvious how adding a few > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > >Maybe you can expand the explanation a bit? > > > > Andrew > > Hi angelo, > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. Yes, that is exactly how it works. Rob
On Mon, Nov 11, 2024 at 10:25:12AM -0600, Rob Herring wrote: > On Sun, Nov 10, 2024 at 3:25 AM Frank Wunderlich > <frank-w@public-files.de> wrote: > > > > Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: > > >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > > >> From: Frank Wunderlich <frank-w@public-files.de> > > >> > > >> after converting the ahci-platform binding to yaml the following files > > >> reporting "'anyOf' conditional failed" on > > >> > > >> sata@540000: sata-port@0 > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > >> index 1e0ab35cc686..2b5e45d2c5a6 100644 > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > >> @@ -214,6 +214,7 @@ &cp0_sata0 { > > >> > > >> sata-port@1 { > > >> phys = <&cp0_comphy3 1>; > > >> + status = "okay"; > > >> }; > > >> }; > > > > > >> > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > >> index 7af949092b91..6bdc4f1e6939 100644 > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > >> @@ -433,11 +433,13 @@ &cp0_sata0 { > > >> /* 7 + 12 SATA connector (J24) */ > > >> sata-port@0 { > > >> phys = <&cp0_comphy2 0>; > > >> + status = "okay"; > > >> }; > > >> > > >> /* M.2-2250 B-key (J39) */ > > >> sata-port@1 { > > >> phys = <&cp0_comphy3 1>; > > >> + status = "okay"; > > >> }; > > >> }; > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > >> index 7e595ac80043..161beec0b6b0 100644 > > >> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > >> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > >> @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { > > >> > > >> sata-port@0 { > > >> reg = <0>; > > >> + status = "disabled"; > > >> }; > > > > > >I don't know the yaml too well, but it is not obvious how adding a few > > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > > > >Maybe you can expand the explanation a bit? > > > > > > Andrew > > > > Hi angelo, > > > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. > > Yes, that is exactly how it works. So from this, can i imply that phys is a required property? Looking at the above patch, it appears that for armada-*.dts, sata-port@0 always uses phys = <&cp0_comphy2 0> and sata-port@1 uses phys = <&cp0_comphy3 1>. Is this an actual SoC property? Could it be moved up into the .dtsi file? Or is it really a board property? Andrew
> Gesendet: Montag, 11. November 2024 um 18:15
> Von: "Andrew Lunn" <andrew@lunn.ch>
> An: "Rob Herring" <robh@kernel.org>
> CC: frank-w@public-files.de, "Frank Wunderlich" <linux@fw-web.de>, "Damien Le Moal" <dlemoal@kernel.org>, "Niklas Cassel" <cassel@kernel.org>, "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Conor Dooley" <conor+dt@kernel.org>, "Gregory Clement" <gregory.clement@bootlin.com>, "Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>, "Russell King" <linux@armlinux.org.uk>, "Hans de Goede" <hdegoede@redhat.com>, "Jens Axboe" <axboe@kernel.dk>, linux-ide@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
> Betreff: Re: [PATCH v1 1/3] arm64: dts: marvell: Fix anyOf conditional failed
>
> On Mon, Nov 11, 2024 at 10:25:12AM -0600, Rob Herring wrote:
> > On Sun, Nov 10, 2024 at 3:25 AM Frank Wunderlich
> > <frank-w@public-files.de> wrote:
> > >
> > > Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>:
> > > >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote:
> > > >> From: Frank Wunderlich <frank-w@public-files.de>
> > > >>
> > > >> after converting the ahci-platform binding to yaml the following files
> > > >> reporting "'anyOf' conditional failed" on
> > > >>
> > > >> sata@540000: sata-port@0
> > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > >> index 1e0ab35cc686..2b5e45d2c5a6 100644
> > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts
> > > >> @@ -214,6 +214,7 @@ &cp0_sata0 {
> > > >>
> > > >> sata-port@1 {
> > > >> phys = <&cp0_comphy3 1>;
> > > >> + status = "okay";
> > > >> };
> > > >> };
> > > >
> > > >>
> > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> > > >> index 7af949092b91..6bdc4f1e6939 100644
> > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts
> > > >> @@ -433,11 +433,13 @@ &cp0_sata0 {
> > > >> /* 7 + 12 SATA connector (J24) */
> > > >> sata-port@0 {
> > > >> phys = <&cp0_comphy2 0>;
> > > >> + status = "okay";
> > > >> };
> > > >>
> > > >> /* M.2-2250 B-key (J39) */
> > > >> sata-port@1 {
> > > >> phys = <&cp0_comphy3 1>;
> > > >> + status = "okay";
> > > >> };
> > > >> };
> > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> > > >> index 7e595ac80043..161beec0b6b0 100644
> > > >> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> > > >> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi
> > > >> @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 {
> > > >>
> > > >> sata-port@0 {
> > > >> reg = <0>;
> > > >> + status = "disabled";
> > > >> };
> > > >
> > > >I don't know the yaml too well, but it is not obvious how adding a few
> > > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed".
> > > >
> > > >Maybe you can expand the explanation a bit?
> > > >
> > > > Andrew
> > >
> > > Hi angelo,
> > >
> > > I guess the dtbs_check only checks required properties from yaml if the node is enabled.
> >
> > Yes, that is exactly how it works.
>
> So from this, can i imply that phys is a required property?
>
> Looking at the above patch, it appears that for armada-*.dts,
> sata-port@0 always uses phys = <&cp0_comphy2 0> and sata-port@1 uses
> phys = <&cp0_comphy3 1>. Is this an actual SoC property? Could it be
> moved up into the .dtsi file? Or is it really a board property?
as i said the phy may operate in different modes (not know marvell here, but on other vendors phys are defined at board level), maybe the boards where the phy is missing the phy is used in another mode. Without knowing the SoC and boards disable it at SoC-level and enable only the nodes containing a phys property is all i can do here to fix the issue. Imho it is always a good idea to enable only the conrollers a board will use.
> Andrew
> </frank-w@public-files.de></andrew@lunn.ch></frank-w@public-files.de></axboe@kernel.dk></hdegoede@redhat.com></linux@armlinux.org.uk></sebastian.hesselbarth@gmail.com></gregory.clement@bootlin.com></conor+dt@kernel.org></krzk+dt@kernel.org></cassel@kernel.org></dlemoal@kernel.org></linux@fw-web.de></robh@kernel.org></andrew@lunn.ch>
On Mon, Nov 11, 2024 at 06:15:16PM +0100, Andrew Lunn wrote: > On Mon, Nov 11, 2024 at 10:25:12AM -0600, Rob Herring wrote: > > On Sun, Nov 10, 2024 at 3:25 AM Frank Wunderlich > > <frank-w@public-files.de> wrote: > > > > > > Am 9. November 2024 18:29:44 MEZ schrieb Andrew Lunn <andrew@lunn.ch>: > > > >On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > > > >> From: Frank Wunderlich <frank-w@public-files.de> > > > >> > > > >> after converting the ahci-platform binding to yaml the following files > > > >> reporting "'anyOf' conditional failed" on > > > >> > > > >> sata@540000: sata-port@0 > > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > > >> index 1e0ab35cc686..2b5e45d2c5a6 100644 > > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts > > > >> @@ -214,6 +214,7 @@ &cp0_sata0 { > > > >> > > > >> sata-port@1 { > > > >> phys = <&cp0_comphy3 1>; > > > >> + status = "okay"; > > > >> }; > > > >> }; > > > > > > > >> > > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > > >> index 7af949092b91..6bdc4f1e6939 100644 > > > >> --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > > >> +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts > > > >> @@ -433,11 +433,13 @@ &cp0_sata0 { > > > >> /* 7 + 12 SATA connector (J24) */ > > > >> sata-port@0 { > > > >> phys = <&cp0_comphy2 0>; > > > >> + status = "okay"; > > > >> }; > > > >> > > > >> /* M.2-2250 B-key (J39) */ > > > >> sata-port@1 { > > > >> phys = <&cp0_comphy3 1>; > > > >> + status = "okay"; > > > >> }; > > > >> }; > > > >> diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > > >> index 7e595ac80043..161beec0b6b0 100644 > > > >> --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > > >> +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi > > > >> @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { > > > >> > > > >> sata-port@0 { > > > >> reg = <0>; > > > >> + status = "disabled"; > > > >> }; > > > > > > > >I don't know the yaml too well, but it is not obvious how adding a few > > > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > > > > > >Maybe you can expand the explanation a bit? > > > > > > > > Andrew > > > > > > Hi angelo, > > > > > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. > > > > Yes, that is exactly how it works. > > So from this, can i imply that phys is a required property? > > Looking at the above patch, it appears that for armada-*.dts, > sata-port@0 always uses phys = <&cp0_comphy2 0> and sata-port@1 uses > phys = <&cp0_comphy3 1>. Is this an actual SoC property? Could it be > moved up into the .dtsi file? Or is it really a board property? Depends if the phy connection/assignment is really fixed or all boards so far just happen to use the same one. If it is fixed and it's just a matter of only one user can be active at a time, then yes, moving to the SoC dtsi makes sense. The connection in the h/w is there, enabled or not. Also, then the board is only dealing with "status" like many of the blocks. Rob
On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > From: Frank Wunderlich <frank-w@public-files.de> Thanks for reviving these. > after converting the ahci-platform binding to yaml the following files > reporting "'anyOf' conditional failed" on Here and the subject, "fixing anyOf" isn't very specific and is just an implementation detail of the schema. "Add missing required 'phys' property" would be more exact. Rob
> Gesendet: Montag, 11. November 2024 um 21:36 > Von: "Rob Herring" <robh@kernel.org> > An: "Frank Wunderlich" <linux@fw-web.de> > CC: "Damien Le Moal" <dlemoal@kernel.org>, "Niklas Cassel" <cassel@kernel.org>, "Krzysztof Kozlowski" <krzk+dt@kernel.org>, "Conor Dooley" <conor+dt@kernel.org>, "Andrew Lunn" <andrew@lunn.ch>, "Gregory Clement" <gregory.clement@bootlin.com>, "Sebastian Hesselbarth" <sebastian.hesselbarth@gmail.com>, "Russell King" <linux@armlinux.org.uk>, "Frank Wunderlich" <frank-w@public-files.de>, "Hans de Goede" <hdegoede@redhat.com>, "Jens Axboe" <axboe@kernel.dk>, linux-ide@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org > Betreff: Re: [PATCH v1 1/3] arm64: dts: marvell: Fix anyOf conditional failed > > On Sat, Nov 09, 2024 at 10:46:19AM +0100, Frank Wunderlich wrote: > > From: Frank Wunderlich <frank-w@public-files.de> > > Thanks for reviving these. > > > after converting the ahci-platform binding to yaml the following files > > reporting "'anyOf' conditional failed" on > > Here and the subject, "fixing anyOf" isn't very specific and is just an > implementation detail of the schema. "Add missing required 'phys' > property" would be more exact. imho it does not match what patch does...i do not add required phys...i just disable the nodes and enable them only where phys is set. > Rob > </frank-w@public-files.de></axboe@kernel.dk></hdegoede@redhat.com></frank-w@public-files.de></linux@armlinux.org.uk></sebastian.hesselbarth@gmail.com></gregory.clement@bootlin.com></andrew@lunn.ch></conor+dt@kernel.org></krzk+dt@kernel.org></cassel@kernel.org></dlemoal@kernel.org></linux@fw-web.de></robh@kernel.org>
On Mon, Nov 11, 2024 at 10:25:12AM -0600, Rob Herring wrote: > > > > > >I don't know the yaml too well, but it is not obvious how adding a few > > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > > > >Maybe you can expand the explanation a bit? > > > > > > Andrew > > > > Hi angelo, > > > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. > > Yes, that is exactly how it works. > > Rob Hello Rob, If we look at e.g. this binding: Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml We can see that it does not define iommu-map in the binding, likewise the binding does have: unevaluatedProperties: false If I apply my patch that adds iommu-map for e.g. the pcie2x1l0 node: (the patch does not add anything to the binding above): https://lore.kernel.org/linux-rockchip/20241107123732.1160063-2-cassel@kernel.org/ If look at the pcie2x1l0 node, it is marked as status = "disabled" in arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi but is marked as status = "enabled" in arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts If I run CHECK_DTBS for this dts/dtb: $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make CHECK_DTBS=y rockchip/rk3588-rock-5b.dtb DTC [C] arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtb $ No warnings. What am I missing? Considering the warning in this series where the binding also had unevaluatedProperties: false I would have expected the same error for the pcie2x1l0 node. (And if I look at most PCI controler bindings, they actually do define iommu-map, so it seems a requirement for it to be defined if used.) Kind regards, Niklas
On Tue, Nov 12, 2024 at 01:36:51PM +0100, Niklas Cassel wrote: > On Mon, Nov 11, 2024 at 10:25:12AM -0600, Rob Herring wrote: > > > > > > > >I don't know the yaml too well, but it is not obvious how adding a few > > > >status = "disabled"; status = "okay"; fixes a "'anyOf' conditional failed". > > > > > > > >Maybe you can expand the explanation a bit? > > > > > > > > Andrew > > > > > > Hi angelo, > > > > > > I guess the dtbs_check only checks required properties from yaml if the node is enabled. > > > > Yes, that is exactly how it works. > > > > Rob > > Hello Rob, > > If we look at e.g. this binding: > Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml > > We can see that it does not define iommu-map in the binding, > likewise the binding does have: > unevaluatedProperties: false > > > If I apply my patch that adds iommu-map for e.g. the pcie2x1l0 node: > (the patch does not add anything to the binding above): > https://lore.kernel.org/linux-rockchip/20241107123732.1160063-2-cassel@kernel.org/ > > > If look at the pcie2x1l0 node, it is marked as status = "disabled" > in arch/arm64/boot/dts/rockchip/rk3588-extra.dtsi > > but is marked as status = "enabled" > in arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts > > If I run CHECK_DTBS for this dts/dtb: > $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- make CHECK_DTBS=y rockchip/rk3588-rock-5b.dtb > DTC [C] arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtb > $ > > No warnings. > > What am I missing? > > Considering the warning in this series where the binding also > had unevaluatedProperties: false > I would have expected the same error for the pcie2x1l0 node. > > (And if I look at most PCI controler bindings, they actually do define > iommu-map, so it seems a requirement for it to be defined if used.) > Or perhaps the question should have been, if iommu-map is an exception, why isn't iommus also an exception? Kind regards, Niklas
diff --git a/arch/arm64/boot/dts/marvell/armada-7040-db.dts b/arch/arm64/boot/dts/marvell/armada-7040-db.dts index 1e0ab35cc686..2b5e45d2c5a6 100644 --- a/arch/arm64/boot/dts/marvell/armada-7040-db.dts +++ b/arch/arm64/boot/dts/marvell/armada-7040-db.dts @@ -214,6 +214,7 @@ &cp0_sata0 { sata-port@1 { phys = <&cp0_comphy3 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts index 7af949092b91..6bdc4f1e6939 100644 --- a/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts +++ b/arch/arm64/boot/dts/marvell/armada-7040-mochabin.dts @@ -433,11 +433,13 @@ &cp0_sata0 { /* 7 + 12 SATA connector (J24) */ sata-port@0 { phys = <&cp0_comphy2 0>; + status = "okay"; }; /* M.2-2250 B-key (J39) */ sata-port@1 { phys = <&cp0_comphy3 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts index 7005a32a6e1e..225a54ab688d 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts +++ b/arch/arm64/boot/dts/marvell/armada-8040-clearfog-gt-8k.dts @@ -475,6 +475,7 @@ &cp1_sata0 { sata-port@1 { phys = <&cp1_comphy0 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-8040-db.dts b/arch/arm64/boot/dts/marvell/armada-8040-db.dts index 2ec19d364e62..fe5d6cb9d692 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-db.dts +++ b/arch/arm64/boot/dts/marvell/armada-8040-db.dts @@ -145,9 +145,12 @@ &cp0_sata0 { sata-port@0 { phys = <&cp0_comphy1 0>; + status = "okay"; }; + sata-port@1 { phys = <&cp0_comphy3 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi index e88ff5b179c8..5043cf2eb33e 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-8040-mcbin.dtsi @@ -245,6 +245,7 @@ &cp0_sata0 { /* CPM Lane 5 - U29 */ sata-port@1 { phys = <&cp0_comphy5 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dts b/arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dts index 3e5e0651ce68..9c25a88581e4 100644 --- a/arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dts +++ b/arch/arm64/boot/dts/marvell/armada-8040-puzzle-m801.dts @@ -408,10 +408,12 @@ &cp0_sata0 { sata-port@0 { phys = <&cp0_comphy2 0>; + status = "okay"; }; sata-port@1 { phys = <&cp0_comphy5 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi index 7e595ac80043..161beec0b6b0 100644 --- a/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi +++ b/arch/arm64/boot/dts/marvell/armada-cp11x.dtsi @@ -347,10 +347,12 @@ CP11X_LABEL(sata0): sata@540000 { sata-port@0 { reg = <0>; + status = "disabled"; }; sata-port@1 { reg = <1>; + status = "disabled"; }; }; diff --git a/arch/arm64/boot/dts/marvell/cn9130-crb-B.dts b/arch/arm64/boot/dts/marvell/cn9130-crb-B.dts index 0904cb0309ae..34194745f79e 100644 --- a/arch/arm64/boot/dts/marvell/cn9130-crb-B.dts +++ b/arch/arm64/boot/dts/marvell/cn9130-crb-B.dts @@ -28,6 +28,7 @@ sata-port@0 { status = "okay"; /* Generic PHY, providing serdes lanes */ phys = <&cp0_comphy2 0>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/cn9131-db.dtsi b/arch/arm64/boot/dts/marvell/cn9131-db.dtsi index ad7360c83048..626042fce7e2 100644 --- a/arch/arm64/boot/dts/marvell/cn9131-db.dtsi +++ b/arch/arm64/boot/dts/marvell/cn9131-db.dtsi @@ -127,6 +127,7 @@ &cp1_sata0 { sata-port@1 { /* Generic PHY, providing serdes lanes */ phys = <&cp1_comphy5 1>; + status = "okay"; }; }; diff --git a/arch/arm64/boot/dts/marvell/cn9132-db.dtsi b/arch/arm64/boot/dts/marvell/cn9132-db.dtsi index e753cfdac697..f91fc69905b8 100644 --- a/arch/arm64/boot/dts/marvell/cn9132-db.dtsi +++ b/arch/arm64/boot/dts/marvell/cn9132-db.dtsi @@ -175,6 +175,7 @@ &cp2_sata0 { sata-port@0 { /* Generic PHY, providing serdes lanes */ phys = <&cp2_comphy2 0>; + status = "okay"; }; };