From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754720AbdDLP2G (ORCPT ); Wed, 12 Apr 2017 11:28:06 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:35093 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752868AbdDLP2A (ORCPT ); Wed, 12 Apr 2017 11:28:00 -0400 Subject: Re: [PATCH v5] drm/pl111: Initial drm/kms driver for pl111 To: Linus Walleij , Eric Anholt , linux-clk References: <20170411011801.15788-1-eric@anholt.net> <87lgr6ego2.fsf@eliezer.anholt.net> Cc: Mike Turquette , Stephen Boyd , "linux-kernel@vger.kernel.org" , "open list:DRM PANEL DRIVERS" , Russell King From: Neil Armstrong Organization: Baylibre Message-ID: <3cba2a40-64fa-b73d-cc21-ebc27f4051bf@baylibre.com> Date: Wed, 12 Apr 2017 17:27:53 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/12/2017 09:40 AM, Linus Walleij wrote: > On Wed, Apr 12, 2017 at 12:13 AM, Eric Anholt wrote: > >> Oh, one last thing I think we need to figure out: I'm using TIM2_CLKSEL, >> which seems to be necessary on this platform. My understanding is that >> this means that the pixel clock is divided from clcdclk instead of >> apb_pclk. Do you agree? > > Yes the pixed clock is always derived from clcdclk. > > In most older ARM reference designs this is a VCO so that > is why there is a clk_set_rate() on this in the fbdev code. > (On some platforms that even has no effect I guess.) > >> The fbdev driver is using >> clk_get(&fb->dev->dev, NULL) and not TIM2_CLKSEL, which I'm surprised by >> because I would have thought that would give us the first clock from the >> DT node (also clcdclk). > > So that thing is a 1-bit line that can select one of two clocks > to be muxed into the PL111/CLCD. > > I guess that up until now all platforms just left that line dangling in > the silicon. Congratulations, you came here first ;) > > Though when I look at the Nomadik it seems that it might be muxing > the clock between 48 and 72 MHz, and I've been using 48MHz > all along ooopsie. > > The current assumption in the bindings is that we have only > one clock and TIM2_CLKSEL is N/A. > > If we want proper clcdclk handling with CLKSEL you should > probably add some code to implement a real mux clock for > this using and drivers/clk/clk-mux.c > with select COMMON_CLK > so that the driver still only sees clcdclk but that in turn is a > mux that can select one of two sources and will react to > the clk_set_rate() call by selecting the clock which is > closest in frequency to what you want. > > This needs a small patch to alter the bindings too I guess. > A small clock node inside the CLCD, just like PCI bridges have > irqchips inside them etc: > > clcd@10120000 { > compatible = "arm,pl110", "arm,primecell"; > reg = <0x10120000 0x1000>; > (...) > clocks = <&clcdclk>, <&foo>; > clock-names = "clcdclk", "apb_pclk"; > > clcdclk: clock-controller@0 { > compatible = "arm,pl11x-clock-mux"; > clocks = <&source_a>, <&source_b>; > }; > }; > > This can be set up easily in the OF probe path since that > is what we're doing: just look for this subnode, if it is there > create the clock controller. > > I do not think the clk maintainers would mind a small mux > clock controller inside the CLCD driver to handle this mux > if we need it. Hi Linus, Indeed it's the way of handling this use case, but no need to add a clk node here, you can copy what we did in pwm-meson, meson-gx-mmc.c, or dwmac-meson8b.c (we went further by also adding clk dividers). In the probe code, simply add a clock mux provider with the two parents clock names (these can and should be platform specific) and only call a clk_set_parent() with the clock specified in the node clocks cell. > > It would *maybe* also be possible to add a second "clcdclk2" > to the block and make an educated decision on which clock > to use in the driver but that is not as elegant as using the > clock framework mux clock I think. > > Yours, > Linus Walleij Neil