[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code
Julian Calaby
julian.calaby at gmail.com
Mon Sep 29 17:10:55 PDT 2014
Hi Thierry,
On Tue, Sep 30, 2014 at 1:19 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Tue, Sep 30, 2014 at 12:46:11AM +1000, Julian Calaby wrote:
>> Hi Thierry,
>>
>> On Mon, Sep 29, 2014 at 11:21 PM, Thierry Reding
>> <thierry.reding at gmail.com> wrote:
>> > On Mon, Sep 29, 2014 at 09:00:01PM +1000, Julian Calaby wrote:
>> >> Hi Thierry,
>> >
>> > If you address people directly please make sure they are in the To:
>> > line. Or at least Cc.
>>
>> Sorry about that, the mailing list I received this through (Google
>> Groups based) generally strips to: and CC: lines, so my mail client
>> (Gmail) doesn't do it automatically. I'm still getting used to it.
>
> Yeah, I wish mailing lists would stop doing that.
>
>> >> On Mon, Sep 29, 2014 at 8:18 PM, Thierry Reding
>> >> <thierry.reding at gmail.com> wrote:
>> >> > On Mon, Sep 29, 2014 at 11:23:01AM +0200, Maxime Ripard wrote:
>> >> >> On Mon, Sep 29, 2014 at 10:06:39AM +0200, Thierry Reding wrote:
>> >> > [...]
>> >> >> > simplefb doesn't deal at all with hardware details. It simply uses what
>> >> >> > firmware has set up, which is the only reason why it will work for many
>> >> >> > people. What is passed in via its device tree node is the minimum amount
>> >> >> > of information needed to draw something into the framebuffer. Also note
>> >> >> > that the simplefb device tree node is not statically added to a DTS file
>> >> >> > but needs to be dynamically generated by firmware at runtime.
>> >> >>
>> >> >> Which makes the whole even simpler, since the firmware already knows
>> >> >> all about which clocks it had to enable.
>> >> >
>> >> > It makes things very complicated in the firmware because it now needs to
>> >> > be able to generate DTB content that we would otherwise be able to do
>> >> > much easier with a text editor.
>> >>
>> >> As far as the kernel is concerned, this is a solved problem.
>> >
>> > It's not completely solved. There's still the issue of no generic way to
>> > specify regulators like you can do for clocks, resets or power domains.
>> > But the kernel isn't the real issue here. The issue is the firmware that
>> > now has to go out of its way not only to initialize display hardware but
>> > also create device tree content just to make Linux not turn everything
>> > off.
>>
>> My point is that the firmware is going to be doing complicated stuff
>> already, adding and using some helpers to configure a device tree node
>> is relatively simple in comparison to dealing with the actual
>> hardware. It wouldn't surprise me if u-boot, for example, ended up
>> with a set of functions to handle this exact case as more graphics
>> hardware gets brought up.
>
> Not all firmware is based on U-Boot. Essentially whatever binding
> changes we make will need to be implemented in all firmware. And the
> complexity isn't so much about writing the actual DT data, but more
> about figuring out which data to write. Every firmware image needs to
> know exactly which clocks and other resources to transcribe for a given
> board. It'll essentially need to contain some sort of "driver" for each
> device that parses a DTB, correlates the data to what it knows of the
> device internals and write a subset of that data back into the DTB in a
> slightly different format. That's just whacky.
>
> DT was meant to simplify things.
Ok, fair enough, while it's a tempting solution - it makes the kernel
end of things nice and simple, it's probably not completely scalable.
U-boot definitely does have "drivers" for the hardware it supports,
but we can't expect every bootloader to do that.
>> >> Firmware is going to be doing some dark magic to set up the hardware
>> >> to be a dumb frame buffer and some other stuff to add the simplefb
>> >> device node - so by this point, adding the clocks (or whatever)
>> >> required by the hardware should be fairly uncomplicated - the firmware
>> >> already knows the hardware intimately. As for the actual device tree
>> >> manipulations, U-boot (or whatever) will probably just grow some
>> >> helper functions to make this simple.
>> >
>> > Have you looked at the code needed to do this? It's not at all trivial.
>> > And the point is really that all this information is there already, so
>> > we're completely duplicating this into a dynamically created device tree
>> > node and for what reason? Only to have one driver request all these
>> > resources and have them forcefully released a few seconds later.
>> >
>> >> Alternatively, it could simply add the relevant data to an existing
>> >> device node and munge it's compatible property so simplefb picks it
>> >> up.
>> >
>> > Yes, I think that'd be a much better solution. Of course it's going to
>> > be very difficult to make that work with a generic driver because now
>> > that generic driver needs to parse the DT binding for any number of
>> > "compatible" devices.
>>
>> Not necessarily.
>>
>> The patch that started this discussion can work with any number of
>> clocks specified in a "clocks" property. Therefore all that needs to
>> happen is that the final hardware binding specifies it's clocks that
>> way.
>
> Are you suggesting that we should be modeling the hardware specific
> binding to match what the simplefb binding does?
I'll admit that this sounded pretty dumb when I thought over it in the
morning. Let's call it a strawman and let's call me a hilariously
unskilled debater.
I don't have a kernel tree in front of me, so I don't know that all
the helpers used below exist or are named like I've shown, but code
like this could grab all the resources specified in a tree of nodes:
void simplefb_grab_all_resources(node *root)
{
node *next = root;
node *cur;
while( next ) {
cur = next;
get_all_resources(cur);
if( next = of_get_first_child(cur) )
continue;
if( next = of_get_next_sibling(cur) )
continue;
while( !next && cur != root ) {
cur = of_get_parent_node(cur);
next = of_get_next_sibling(cur);
}
if( cur == root )
break;
}
}
If we added in a few tests in get_all_resources() to keep it from
doing things which are obviously stupid (e.g. grabbing resources for
nodes which have drivers we already have registered) that should work
for any binding and could easily be extended to any resource type.
We're already using linked lists to hold references to those
resources, so there's no problems I can see here, other than adding
some code.
>> I'm sure that as hardware diversifies, the other subsystems will grow
>> in similar directions and eventually be dealt with using similarly
>> generic code.
>
> For regulators this already works very differently. As opposed to the
> clocks/clock-names type of binding it uses one where the consumer name
> of the regulator comes from the prefix of a -supply property. That is
> you'd get something like this:
>
> foo-supply = <®1>;
> bar-supply = <®2>;
>
> And since you don't have enough information in the kernel simplefb
> driver to attach any meaning to these, the best you can do would be
> iterating over a range and have:
>
> 0-supply = <®0>;
> 1-supply = <®1>;
> ...
> n-supply = <®2>;
>
> This is made more difficult by the fact that these regulators may be
> required by components not immediately related to the display engine.
> They could be for an attached panel, a video bridge or the +5V pin on
> the HDMI connector.
Surely an iterator could be constructed to go through each node and
process every one named something like "*-supply"?
As for there being regulators for each separate part of the device,
could we read what their current state is then maintain that state?
I'll admit that this is getting rather hairy at this point.
Finally, the clock subsystem has evolved away from "*-clock" bindings
towards the current "clocks" and "clock-names" bindings. I'm sure
other subsystems will follow.
Thanks,
--
Julian Calaby
Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
More information about the linux-arm-kernel
mailing list