| Message ID | 20220228124345.99474-2-singh.kuldeep87k@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | DTC fixes for Arm pl022 bindings | expand |
On 2022-02-28 12:43, Kuldeep Singh wrote: > Add missing minItems property to clocks in ARM pl022 bindings. > > This also helps in resolving below dtc warnings: > arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clocks: [[4]] is too short > From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml > arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clock-names: ['apb_pclk'] is too short > From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml Who says that minItems is missing? Looking at the PL022 TRM[1] it seems clear that SSPCLK is pretty fundamental to useful operation. If that DT ever worked, it must be that the same clock is wired to both inputs, and the fact that that's how the neighbouring PL011 is described is strongly suggestive. If the point of schema is to find errors in DTs, doesn't it make more sense to fix the DTs than to weaken the schema just to shut it up? Of course in this particular case there's also the question of whether the most humane way to "fix" the Seattle DTs is to simply delete them, but that's orthogonal. Robin. [1] https://developer.arm.com/documentation/ddi0194/h/?lang=en > Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> > --- > Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml > index 6d633728fc2b..7d36e15db5b3 100644 > --- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml > +++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml > @@ -34,6 +34,7 @@ properties: > maxItems: 1 > > clocks: > + minItems: 1 > maxItems: 2 > > clock-names:
On Mon, Feb 28, 2022 at 02:26:12PM +0000, Robin Murphy wrote: > Who says that minItems is missing? Looking at the PL022 TRM[1] it seems > clear that SSPCLK is pretty fundamental to useful operation. If that DT ever > worked, it must be that the same clock is wired to both inputs, and the fact > that that's how the neighbouring PL011 is described is strongly suggestive. Well, it could also be that the clock is wired to some other clock which is always on (which I guess is why the driver allows this in the first place, there's a lot of sloppy code around stuff like that in the tree). > If the point of schema is to find errors in DTs, doesn't it make more sense > to fix the DTs than to weaken the schema just to shut it up? I do agree on this point though, given that the clock is actually required for useful operation what's happening here is that the schema is detecting an error in the use of the binding.
On 2022-02-28 15:11, Mark Brown wrote: > On Mon, Feb 28, 2022 at 02:26:12PM +0000, Robin Murphy wrote: > >> Who says that minItems is missing? Looking at the PL022 TRM[1] it seems >> clear that SSPCLK is pretty fundamental to useful operation. If that DT ever >> worked, it must be that the same clock is wired to both inputs, and the fact >> that that's how the neighbouring PL011 is described is strongly suggestive. > > Well, it could also be that the clock is wired to some other clock which > is always on (which I guess is why the driver allows this in the first > place, there's a lot of sloppy code around stuff like that in the tree). I wouldn't say the driver "allows" it, so much as it just blindly grabs the first clock assuming it's SSPCLK per the binding, and thus it will happen to work out if the underlying physical clock is the same as, or equivalent to, the APB PCLK. Otherwise, it's already into some degree of not working properly, by virtue of reading the wrong clock rate. Robin.
On Mon, Feb 28, 2022 at 03:27:08PM +0000, Robin Murphy wrote: > On 2022-02-28 15:11, Mark Brown wrote: > > Well, it could also be that the clock is wired to some other clock which > > is always on (which I guess is why the driver allows this in the first > > place, there's a lot of sloppy code around stuff like that in the tree). > I wouldn't say the driver "allows" it, so much as it just blindly grabs the > first clock assuming it's SSPCLK per the binding, and thus it will happen to > work out if the underlying physical clock is the same as, or equivalent to, > the APB PCLK. Otherwise, it's already into some degree of not working > properly, by virtue of reading the wrong clock rate. Ah, the APB clock requirement is inherited from the AMBA implementation isn't it? We really ought to be extending an AMBA binding here...
On 2022-02-28 15:46, Mark Brown wrote: > On Mon, Feb 28, 2022 at 03:27:08PM +0000, Robin Murphy wrote: >> On 2022-02-28 15:11, Mark Brown wrote: > >>> Well, it could also be that the clock is wired to some other clock which >>> is always on (which I guess is why the driver allows this in the first >>> place, there's a lot of sloppy code around stuff like that in the tree). > >> I wouldn't say the driver "allows" it, so much as it just blindly grabs the >> first clock assuming it's SSPCLK per the binding, and thus it will happen to >> work out if the underlying physical clock is the same as, or equivalent to, >> the APB PCLK. Otherwise, it's already into some degree of not working >> properly, by virtue of reading the wrong clock rate. > > Ah, the APB clock requirement is inherited from the AMBA implementation > isn't it? We really ought to be extending an AMBA binding here... Yup, both the "apb_pclk" clock specifier and the "arm,primecell" compatible technically belong to the common AMBA binding, but I'm not sure whether schema has the ability to compose at such fine-grained a level :/ Robin.
diff --git a/Documentation/devicetree/bindings/spi/spi-pl022.yaml b/Documentation/devicetree/bindings/spi/spi-pl022.yaml index 6d633728fc2b..7d36e15db5b3 100644 --- a/Documentation/devicetree/bindings/spi/spi-pl022.yaml +++ b/Documentation/devicetree/bindings/spi/spi-pl022.yaml @@ -34,6 +34,7 @@ properties: maxItems: 1 clocks: + minItems: 1 maxItems: 2 clock-names:
Add missing minItems property to clocks in ARM pl022 bindings. This also helps in resolving below dtc warnings: arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clocks: [[4]] is too short From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml arch/arm64/boot/dts/amd/amd-overdrive.dt.yaml: spi@e1020000: clock-names: ['apb_pclk'] is too short From schema: Documentation/devicetree/bindings/spi/spi-pl022.yaml Signed-off-by: Kuldeep Singh <singh.kuldeep87k@gmail.com> --- Documentation/devicetree/bindings/spi/spi-pl022.yaml | 1 + 1 file changed, 1 insertion(+)