Re: [PATCH v2 1/5] dt-bindings: display: simple-framebuffer: Add interconnects property

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 30-Jun-25 10:24 AM, Krzysztof Kozlowski wrote:
> On 29/06/2025 14:07, Hans de Goede wrote:
>> Hi Krzysztof,
>>
>> On 28-Jun-25 1:49 PM, Krzysztof Kozlowski wrote:
>>> On 27/06/2025 11:48, Luca Weiss wrote:
>>>> Hi Krzysztof,
>>>>
>>>> On Fri Jun 27, 2025 at 10:08 AM CEST, Krzysztof Kozlowski wrote:
>>>>> On Mon, Jun 23, 2025 at 08:44:45AM +0200, Luca Weiss wrote:
>>>>>> Document the interconnects property which is a list of interconnect
>>>>>> paths that is used by the framebuffer and therefore needs to be kept
>>>>>> alive when the framebuffer is being used.
>>>>>>
>>>>>> Acked-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/display/simple-framebuffer.yaml | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> index 296500f9da05e296dbbeec50ba5186b6b30aaffc..f0fa0ef23d91043dfb2b220c654b80e2e80850cd 100644
>>>>>> --- a/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/display/simple-framebuffer.yaml
>>>>>> @@ -79,6 +79,9 @@ properties:
>>>>>>    power-domains:
>>>>>>      description: List of power domains used by the framebuffer.
>>>>>>  
>>>>>> +  interconnects:
>>>>>> +    description: List of interconnect paths used by the framebuffer.
>>>>>> +
>>>>>
>>>>> maxItems: 1, or this is not a simple FB anymore. Anything which needs
>>>>> some sort of resources in unknown way is not simple anymore. You need
>>>>> device specific bindings.
>>>>
>>>> The bindings support an arbitrary number of clocks, regulators,
>>>> power-domains. Why should I artificially limit the interconnects to only
>>>> one?
>>>
>>> And IMO they should not. Bindings are not supposed to be generic.
>>
>> The simplefb binding is a binding to allow keeping the firmware, e.g.
>> uboot setup framebuffer alive to e.g. show a boot splash until
>> the native display-engine drive loads. Needing display-engine
>> specific bindings totally contradicts the whole goal of 
> 
> No, it does not. DT is well designed for that through expressing
> compatibility. I did not say you cannot have generic fallback for simple
> use case.
> 
> But this (and previous patchset) grows this into generic binding ONLY
> and that is not correct.

I think that it is important here to notice that this is not
a generic fallback binding, this is not and will never be
intended to replace have a proper binding for
the display-engine.

This is just a way to give the kernel access to the firmware
setup framebuffer to e.g. show a bootsplash but also fatal
kernel errors until the real display-engine driver loads.

Note sometimes the whole framebuffer memory is not touched
at all and the sole reason for having a driver attach to
the simplefb node early on is just to keep the resources
needed to keep the panel lit up around (on) until the real
display-engine driver comes along to claim those resources.

This avoids the display going black if the display-engine
driver only binds after the kernel starts turning off
unused resources, this typically happens when the display-engine
driver is a module.

>> It is generic by nature and I really do not see how clocks and
>> regulators are any different then interconnects here.
> 
> Yeah, they are also wrong. I already commented on this.
> 
>>
>> Note that we had a huge discussion about adding clock
>> and regulators to simplefb many years ago with pretty
>> much the same arguments against doing so. In the end it was
>> decided to add regulator and clocks support to the simplefb
>> bindings and non of the feared problems with e.g. ordening
>> of turning things on happened.
>>
>> A big part of this is that the claiming of clks / regulators /
>> interconnects by the simplefb driver is there to keep things on,
>> so it happens before the kernel starts tuning off unused resources
>> IOW everything is already up and running and this really is about
>> avoiding turning things off.
> 
> No one asks to drop them from the driver. I only want specific front
> compatible which will list and constrain the properties. It is not
> contradictory to your statements, U-boot support, driver support. I
> really do not see ANY argument why this cannot follow standard DT rules.

So what you are saying is that you want something like:

framebuffer0: framebuffer@1d385000 {
	compatible = "qcom.simple-framebuffer-sm8650-mdss", "simple-framebuffer";
}

and that the binding for qcom.simple-framebuffer-sm8650-mdss
can then list interconnects ?

Regards,

Hans