[linux-sunxi] Re: [PATCH 4/4] simplefb: add clock handling code

[jonsmirl at gmail.com]

On Mon, Aug 25, 2014 at 11:12 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Mon, Aug 25, 2014 at 04:27:04PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 08/25/2014 04:23 PM, jonsmirl at gmail.com wrote:
>> > On Mon, Aug 25, 2014 at 10:16 AM, Thierry Reding
>> > <thierry.reding at gmail.com> wrote:
>> >> On Mon, Aug 25, 2014 at 03:47:43PM +0200, Hans de Goede wrote:
>> >>> On 08/25/2014 03:39 PM, Thierry Reding wrote:
>> >>>> On Mon, Aug 25, 2014 at 02:44:10PM +0200, Maxime Ripard wrote:
>> >>>>> On Mon, Aug 25, 2014 at 02:12:30PM +0200, Thierry Reding wrote:
>> >>>>>> On Wed, Aug 13, 2014 at 07:01:06PM +0200, Maxime Ripard wrote:
>> >>>>>>> On Wed, Aug 13, 2014 at 10:38:09AM -0600, Stephen Warren wrote:
>> >>>>>> [...]
>> >>>>>>>> If not, perhaps the clock driver should force the clock to be
>> >>>>>>>> enabled (perhaps only if the DRM/KMS driver isn't enabled?).
>> >>>>>>>
>> >>>>>>> I'm sorry, but I'm not going to take any code that will do that in our
>> >>>>>>> clock driver.
>> >>>>>>>
>> >>>>>>> I'm not going to have a huge list of ifdef depending on configuration
>> >>>>>>> options to know which clock to enable, especially when clk_get should
>> >>>>>>> have the consumer device as an argument.
>> >>>>>>
>> >>>>>> Are you saying is that you want to solve a platform-specific problem by
>> >>>>>> pushing code into simple, generic drivers so that your platform code can
>> >>>>>> stay "clean"?
>> >>>>>
>> >>>>> Are you saying that this driver would become "dirty" with such a patch?
>> >>>>
>> >>>> Yes. Others have said the same and even provided alternative solutions
>> >>>> on how to solve what's seemingly a platform-specific problem in a
>> >>>> platform-specific way.
>> >>>
>> >>> This is not platform specific, any platform with a complete clock driver
>> >>> will suffer from the same problem (the clock driver disabling unclaimed
>> >>> ahb gates, and thus killing the video output) if it wants to use simplefb
>> >>> for early console support.
>> >>
>> >> It is platform specific in that your platform may require certain clocks
>> >> to remain on. The next platform may require power domains to remain on
>> >> during boot and yet another one may rely on regulators to stay on during
>> >> boot. By your argument simplefb will need to be taught to handle pretty
>> >> much every type of resource that the kernel has.
>> >
>> > Why can't simplefb be a driver library that is called from a device
>> > specific device driver that only claims the clocks (or regulators)?
>> > Then build all of these device specific drivers into the generic ARM
>> > kernel. They will be quite small since all they do is claim the clocks
>> > (or regulator).  Maybe we can even figure out some protocol for
>> > removing the unused ones from memory later.
>> >
>> > Later during the boot process the device specific driver can load its
>> > KMS code which has also been implemented as a driver library. Maybe
>> > use E_PROBE_DEFER to do this. Match on the device ID, claim the
>> > clocks, defer until the full KMS library can be loaded.
>>
>> There is no need for all this complexity, all that is needed is for the
>> simplefb driver to be thought to claim + enable any clocks listed in
>> its dt node.
>
> Out of curiosity, how does this work in practice? How does the
> bootloader create this entry? Does it scan the DT to see which clocks
> the real hardware device references and then simply copies them to the
> simplefb node?

That's why this is such a problem. There is a general rule in Linux -
one device, one driver.  All of the kernel device driver code is
written around this assumption. We're trying to make two drivers for
one device and the kernel is not designed to support that. There
shouldn't be an independent 'simplefb' node. That is creating two
device descriptions for a single device.

Adding 'chosen' information to the devices nodes might work.

video at 1345 {
    compatible = "sunxi-a20-video";
    reg = <...>;
    clocks = <...>;
    chosen {
      compatible = "simplefb";     // dynamically added by uboot
      buffer = <0x4564>;    // dynamically added by uboot
   };
};

This will initialize the simplefb driver without attaching it to any
specific hardware.  It can then look in its parent node for clocks and
regulators and claim them.


>
> Thierry



-- 
Jon Smirl
jonsmirl at gmail.com