On 9/13/24 11:07, Luca Ceresoli wrote: > Hello Sean, Geert, > > On Tue, 10 Sep 2024 17:13:55 -0500 > Rob Herring <robh+dt@xxxxxxxxxx> wrote: > >> On Wed, Mar 22, 2023 at 3:39 AM Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote: >> > >> > Hello Stephen, >> > >> > On Mon, 20 Mar 2023 14:27:56 -0700 >> > Stephen Boyd <sboyd@xxxxxxxxxx> wrote: >> > >> > > Quoting Sean Anderson (2023-01-24 08:23:45) >> > > > On 1/24/23 03:28, Geert Uytterhoeven wrote: >> > > > > Hi Luca, >> > > > > >> > > > > On Tue, Jan 24, 2023 at 9:12 AM Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> wrote: >> > > > >> On Thu, 19 Jan 2023 14:27:43 -0500 >> > > > >> Sean Anderson <sean.anderson@xxxxxxxx> wrote: >> > > > >> > On 1/11/23 10:55, Geert Uytterhoeven wrote: >> > > > > >> > > > >> I'm wondering whether Geert has a practical example of a situation >> > > > >> where it is better to have these properties optional. >> > > > > >> > > > > My issue was that these properties were introduced long after the >> > > > > initial bindings, hence pre-existing DTS does not have them. >> > > > > Yes, we can add them, but then we have to read out the OTP-programmed >> > > > > settings first. If that's the way to go, I can look into that, though... >> > > > >> > > > FWIW I think there's no need to update existing bindings which don't >> > > > have this property. The required aspect is mainly a reminder for new >> > > > device trees. >> > > > >> > > >> > > Is there any resolution on this thread? I'm dropping this patch from my >> > > queue. >> > >> > IIRC Geert kind of accepted the idea that these properties should stay >> > required. Which is a bit annoying but it's the safest option, so unless >> > there are new complaints with solid use cases for making them optionalm, >> > I think it's OK to drop the patch. >> >> The warnings related to this are now at the top of the list (by number >> of occurrences): >> >> 50 clock-generator@6a: 'idt,shutdown' is a required property >> 50 clock-generator@6a: 'idt,output-enable-active' is a required property >> >> IMO, if these properties haven't been needed for years, then they >> obviously aren't really required. > > I think Rob's point adds to Geert's observation that there are other > "idt,*" properties in the output nodes that may also be important to > have correctly set, and are optional. > > So, Sean, I understand when you state it's safer to have these set. > However this is valid for lots of other optional properties in any > binding. Optional properties _can_ be set if that's important, just > it's not mandatory to set them in all cases. > > As a matter of fact, we have been having for a long time some in-tree > device trees which don't set these properties, which I believe implies > it's OK for those cases to not set them, and to let them be set for the > device trees where it is important. > > Finally, there is a maintenance/legacy issue: if we wanted to keep these > properties optional, who would chase all the boards defined in existing > device trees to discover the correct values? > > Bottom line, my Reviewed-by tag is still valid. > > What is your opinion given these last few discussion point Sean? I am willing to send patches adding these properties for the appropriate boards. There are only 6 in tree (all Renesas): $ git grep -l idt,5p49 '**.dts*' arch/arm64/boot/dts/renesas/beacon-renesom-baseboard.dtsi arch/arm64/boot/dts/renesas/beacon-renesom-som.dtsi arch/arm64/boot/dts/renesas/hihope-common.dtsi arch/arm64/boot/dts/renesas/salvator-x.dtsi arch/arm64/boot/dts/renesas/salvator-xs.dtsi arch/arm64/boot/dts/renesas/ulcb.dtsi I was able to find schematics for ULCB. Salvator-X seems to be gone from Renesas's website (in favor of the -XS). I have requested access to the -XS schematics. The HiHope board doesn't seem to have schematics anywhere I could find (which is pretty unusual for a reference design...). The Beacon schematics are behind a support portal (or so I assume). That said, this info should be pretty easy to find for anyone with physical access to a board. Just boot it up and probe the voltage on the SD/OE pin. I've added some people who may have the hardware to CC. --Sean
