On Fri, Sep 13, 2024 at 11:41 AM Sean Anderson <sean.anderson@xxxxxxxx> wrote: > > 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 doesn't sound promising to me. > 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. By some definition of easy I guess... I want the warning gone, so I'm going to apply this patch. When/if all the cases have been fixed, I'll happily revert it. Rob
