Received: by 2002:a25:8b12:0:0:0:0:0 with SMTP id i18csp598834ybl; Mon, 12 Aug 2019 23:06:19 -0700 (PDT) X-Google-Smtp-Source: APXvYqxKAdmTKCXAy3tmrZaR8sUgVjC30nCPzCp59nfvs61sPVxA4yzyd0RxE/qDcSxpCRwh56GQ X-Received: by 2002:a17:902:b949:: with SMTP id h9mr33480234pls.120.1565676379593; Mon, 12 Aug 2019 23:06:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1565676379; cv=none; d=google.com; s=arc-20160816; b=WQFvvsFue9Wfn//nCienmNFx6RbrYjT7cdlQI2p/cN2xQu0PPQmsKHeBVxwmmNz3HE mET+dTLg/+MsilAGcRfZcnYap/k7ETOz3/v/FuUl6GEPtHRb7c+2ZYERclWWeebqQ8WL HnY8FXfNiYnQNn5oWDdCSbg+66ChD567KLckeekizQSrtzSZbpjkg1heLjD8ESc6ZQP2 nWqtRPQ/lHQtaQHguCBsOL+A9Gf0WBLSO7RAmId5hck9mWXpCB3/l89Pnh/oTynY2sea M5pnzjwwl09FUgPwDkWfJQSp5nHeyK7Jzxn6T6GLIJxnOFU8xrSifG4YAYUynQSelDug 2BSw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=1P5wxvoX8QSHknU17CO16m6vbFQIpz/hTentJJCu9tE=; b=ogAp67DHt+WdgZjLB9xI4VnzyjgveVzr0hm2TA73DDX7kxs7x+sTlzwDfzewTxwSn6 5Mm4tUgOMYj2QoKZb+HCzFXQlAUs0A538DoQWzWiMGr0vEXOc/vTczG3jAHcUmxaRuwb A+YOeoELbwfe7lLa2J8+E8+lbWnGb3luZ8P0Un8N6gp9rFFlHwd/tUd6l8GhvSnHVUQQ AX6foQolpBP6IZVoXJl2iv/LdT332ZMjhbviZJyqehoBEd3LB8T07E5hLoZQ3EOihsi8 13+GH0uLeOlwxkJHAtv4GmAf/v80XKHLWJpLeFFXB6JLc2n6uvyGYxCnlJkz7ul73+5G e7Kw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 36si63068301pla.80.2019.08.12.23.06.03; Mon, 12 Aug 2019 23:06:19 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727249AbfHMGFJ (ORCPT + 99 others); Tue, 13 Aug 2019 02:05:09 -0400 Received: from relay9-d.mail.gandi.net ([217.70.183.199]:35933 "EHLO relay9-d.mail.gandi.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725842AbfHMGFI (ORCPT ); Tue, 13 Aug 2019 02:05:08 -0400 X-Originating-IP: 90.89.68.76 Received: from localhost (lfbn-1-10718-76.w90-89.abo.wanadoo.fr [90.89.68.76]) (Authenticated sender: maxime.ripard@bootlin.com) by relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 95673FF806; Tue, 13 Aug 2019 06:05:03 +0000 (UTC) Date: Tue, 13 Aug 2019 08:05:02 +0200 From: Maxime Ripard To: Michael Nazzareno Trimarchi Cc: Jagan Teki , David Airlie , Daniel Vetter , Chen-Yu Tsai , Michael Turquette , Rob Herring , Mark Rutland , linux-arm-kernel , linux-kernel , linux-clk , dri-devel , devicetree , linux-amarula , linux-sunxi Subject: Re: [PATCH v6 11/22] clk: sunxi-ng: a64: Add minimum rate for PLL_MIPI Message-ID: <20190813060502.teeevudz6cjn35tl@flea> References: <20190703114933.u3x4ej3v7ocewvif@flea> <20190711100100.cty3s6rs3w27low6@flea> <20190720065830.zn3txpyduakywcva@flea> <20190720093202.6fn6xmhvsgawscnu@flea> <20190724090513.vqnlmya3nqkl6pmu@flea> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20180716 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 29, 2019 at 08:59:04AM +0200, Michael Nazzareno Trimarchi wrote: > Hi > > On Wed, Jul 24, 2019 at 11:05 AM Maxime Ripard > wrote: > > > > On Mon, Jul 22, 2019 at 03:51:04PM +0530, Jagan Teki wrote: > > > Hi Maxime, > > > > > > On Sat, Jul 20, 2019 at 3:02 PM Maxime Ripard wrote: > > > > > > > > On Sat, Jul 20, 2019 at 12:46:27PM +0530, Jagan Teki wrote: > > > > > On Sat, Jul 20, 2019 at 12:28 PM Maxime Ripard > > > > > wrote: > > > > > > > > > > > > On Thu, Jul 11, 2019 at 07:43:16PM +0200, Michael Nazzareno Trimarchi wrote: > > > > > > > > > tcon-pixel clock is the rate that you want to achive on display side > > > > > > > > > and if you have 4 lanes 32bit or lanes and different bit number that > > > > > > > > > you need to have a clock that is able to put outside bits and speed > > > > > > > > > equal to pixel-clock * bits / lanes. so If you want a pixel-clock of > > > > > > > > > 40 mhz and you have 32bits and 4 lanes you need to have a clock of > > > > > > > > > 40 * 32 / 4 in no-burst mode. I think that this is done but most of > > > > > > > > > the display. > > > > > > > > > > > > > > > > So this is what the issue is then? > > > > > > > > > > > > > > > > This one does make sense, and you should just change the rate in the > > > > > > > > call to clk_set_rate in sun4i_tcon0_mode_set_cpu. > > > > > > > > > > > > > > > > I'm still wondering why that hasn't been brought up in either the > > > > > > > > discussion or the commit log before though. > > > > > > > > > > > > > > > Something like this? > > > > > > > > > > > > > > drivers/gpu/drm/sun4i/sun4i_tcon.c | 20 +++++++++++--------- > > > > > > > drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h | 2 -- > > > > > > > 2 files changed, 11 insertions(+), 11 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > > > > > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > > > > > index 64c43ee6bd92..42560d5c327c 100644 > > > > > > > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > > > > > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > > > > > > > @@ -263,10 +263,11 @@ static int sun4i_tcon_get_clk_delay(const struct > > > > > > > drm_display_mode *mode, > > > > > > > } > > > > > > > > > > > > > > static void sun4i_tcon0_mode_set_common(struct sun4i_tcon *tcon, > > > > > > > - const struct drm_display_mode *mode) > > > > > > > + const struct drm_display_mode *mode, > > > > > > > + u32 tcon_mul) > > > > > > > { > > > > > > > /* Configure the dot clock */ > > > > > > > - clk_set_rate(tcon->dclk, mode->crtc_clock * 1000); > > > > > > > + clk_set_rate(tcon->dclk, mode->crtc_clock * tcon_mul * 1000); > > > > > > > > > > > > > > /* Set the resolution */ > > > > > > > regmap_write(tcon->regs, SUN4I_TCON0_BASIC0_REG, > > > > > > > @@ -335,12 +336,13 @@ static void sun4i_tcon0_mode_set_cpu(struct > > > > > > > sun4i_tcon *tcon, > > > > > > > u8 bpp = mipi_dsi_pixel_format_to_bpp(device->format); > > > > > > > u8 lanes = device->lanes; > > > > > > > u32 block_space, start_delay; > > > > > > > - u32 tcon_div; > > > > > > > + u32 tcon_div, tcon_mul; > > > > > > > > > > > > > > - tcon->dclk_min_div = SUN6I_DSI_TCON_DIV; > > > > > > > - tcon->dclk_max_div = SUN6I_DSI_TCON_DIV; > > > > > > > + tcon->dclk_min_div = 4; > > > > > > > + tcon->dclk_max_div = 127; > > > > > > > > > > > > > > - sun4i_tcon0_mode_set_common(tcon, mode); > > > > > > > + tcon_mul = bpp / lanes; > > > > > > > + sun4i_tcon0_mode_set_common(tcon, mode, tcon_mul); > > > > > > > > > > > > > > /* Set dithering if needed */ > > > > > > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder)); > > > > > > > @@ -366,7 +368,7 @@ static void sun4i_tcon0_mode_set_cpu(struct > > > > > > > sun4i_tcon *tcon, > > > > > > > */ > > > > > > > regmap_read(tcon->regs, SUN4I_TCON0_DCLK_REG, &tcon_div); > > > > > > > tcon_div &= GENMASK(6, 0); > > > > > > > - block_space = mode->htotal * bpp / (tcon_div * lanes); > > > > > > > + block_space = mode->htotal * tcon_div * tcon_mul; > > > > > > > block_space -= mode->hdisplay + 40; > > > > > > > > > > > > > > regmap_write(tcon->regs, SUN4I_TCON0_CPU_TRI0_REG, > > > > > > > @@ -408,7 +410,7 @@ static void sun4i_tcon0_mode_set_lvds(struct > > > > > > > sun4i_tcon *tcon, > > > > > > > > > > > > > > tcon->dclk_min_div = 7; > > > > > > > tcon->dclk_max_div = 7; > > > > > > > - sun4i_tcon0_mode_set_common(tcon, mode); > > > > > > > + sun4i_tcon0_mode_set_common(tcon, mode, 1); > > > > > > > > > > > > > > /* Set dithering if needed */ > > > > > > > sun4i_tcon0_mode_set_dithering(tcon, sun4i_tcon_get_connector(encoder)); > > > > > > > @@ -487,7 +489,7 @@ static void sun4i_tcon0_mode_set_rgb(struct > > > > > > > sun4i_tcon *tcon, > > > > > > > > > > > > > > tcon->dclk_min_div = 6; > > > > > > > tcon->dclk_max_div = 127; > > > > > > > - sun4i_tcon0_mode_set_common(tcon, mode); > > > > > > > + sun4i_tcon0_mode_set_common(tcon, mode, 1); > > > > > > > > > > > > > > /* Set dithering if needed */ > > > > > > > sun4i_tcon0_mode_set_dithering(tcon, connector); > > > > > > > diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > > > > > > b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > > > > > > index 5c3ad5be0690..a07090579f84 100644 > > > > > > > --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > > > > > > +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.h > > > > > > > @@ -13,8 +13,6 @@ > > > > > > > #include > > > > > > > #include > > > > > > > > > > > > > > -#define SUN6I_DSI_TCON_DIV 4 > > > > > > > - > > > > > > > struct sun6i_dsi { > > > > > > > struct drm_connector connector; > > > > > > > struct drm_encoder encoder; > > > > > > > > > > > > I had more something like this in mind: > > > > > > http://code.bulix.org/nlp5a4-803511 > > > > > > > > > > Worth to look at it. was it working on your panel? meanwhile I will check it. > > > > > > > > I haven't tested it. > > > > > > > > > We have updated with below change [1], seems working on but is > > > > > actually checking the each divider as before start with 4... till 127. > > > > > > > > > > This new approach, is start looking the best divider from 4.. based on > > > > > the idea vs rounded it will ended up best divider like [2] > > > > > > > > But why? > > > > > > > > I mean, it's not like it's the first time I'm asking this... > > > > > > > > If the issue is what Micheal described, then the divider has nothing > > > > to do with it. We've had that discussion over and over again. > > > > > > This is what Michael is mentioned in above mail "tcon-pixel clock is > > > the rate that you want to achive on display side and if you have 4 > > > lanes 32bit or lanes and different bit number that you need to have > > > a clock that is able to put outside bits and speed equal to > > > pixel-clock * bits / lanes. so If you want a pixel-clock of 40 mhz > > > and you have 32bits and 4 lanes you need to have a clock of 40 * 32 > > > / 4 in no-burst mode. " > > > > Yeah, so we need to change the clock rate. > > > > > He is trying to manage the bpp/lanes into dclk_mul (in last mail) > > > and it can multiply with pixel clock which is rate argument in > > > sun4i_dclk_round_rate. > > > > > > The solution I have mentioned in dclk_min, max is bpp/lanes also > > > multiple rate in dotclock sun4i_dclk_round_rate. > > > > > > In both cases the overall pll_rate depends on dividers, the one that I > > > have on this patch is based on BSP and the Michael one is more generic > > > way so-that it can not to touch other functionalities and looping > > > dividers to find the best one. > > > > > > If dclk_min/max is bpp/lanes then dotclock directly using divider 6 > > > (assuming 24-bit and 4 lanes) and return the pll_rate and divider 6 > > > associated. > > > > > > if dclk_mul is bpp/lanes, on Michael new change, the dividers start > > > with 4 and end with 127 but the constant ideal rate which rate * > > > bpp/lanes but the loop from sun4i_dclk_round_rate computed the divider > > > as 6 only, ie what I'm mentioned on the above mail. > > > > We've been over this a couple of times already. > > > > The clock is generated like this: > > > > PLL -> TCON Module Clock -> TCON DCLK > > > > You want the TCON DCLK to be at the pixel clock rate * bpp / > > lanes. Fine, that makes sense. > > > > Except that the patch you've sent, instead of changing the rate > > itself, changes the ratio between the module clock and DCLK. > > > > And this is where the issue lies. First, from a logical viewpoint, it > > doesn't make sense. If you want to change the clock rate, then just do > > it. Don't hack around the multipliers trying to fall back to something > > that works for you. > > > > Then, the ratio itself needs to be set to 4. This is the part that > > we've discussed way too many times already, but in the Allwinner BSP, > > that ratio is hardcoded to 4, and we've had panels that need it at > > that value. > > > > So, what you want to do is to have: > > > > TCON DCLK = pixel clock * bpp / lanes > > TCON Module Clock = DCLK * 4 > > PLL = Module Clock * Module Clock Divider (which I believe is 1 in most cases) > > pll-mipi 1 1 1 178200000 > 0 0 50000 > tcon0 2 2 1 178200000 > 0 0 50000 > tcon-pixel-clock 1 1 1 29700000 > 0 0 50000 Is this before or after your patches? > This is an english problem from my side: > tcon-pixel-clock is DCLK > tcon0 must be tcon-pixel-clock * bpp / lanes, because the logic need to > put a bit every cycle. Again, I'm not saying this is wrong, but each time I've looked at it the BSP was using a 4 divider between the tcon module clock and the dotclock. So, please prove me wrong here. > One solution can be: > - set_rate_exclusive to tcon0 and calculate as display pixel clock * > bpp / lanes I'm not sure what set_rate_exclusive has to do with it. I mean, it's a good idea to use it, but it shouldn't really change anything to the discussion. > - calculate the tcon-pixel-clock using all divider I'm not sure what you mean by that. > Problem is that the function that calculate tcon-pixel-clock does not > have any constrain on the ideal value. It does have constraints, but you're right that it will not try to find an exact match and bail out if it can't do it, but will find the closest match. > What you are suggesting is not correct in my opinion or I'm not > following your suggesstion. Then prove me wrong. > What I know is that if we have a pixel-clock of dvi display of 50Mhz > and we have 4 lanes 32bit we need a clock in the logic of 400Mhz > (this is the ideal throughtput). > So tcon-pixel-clock is 50mhz and tcon0 is 400Mhz. There's one thing to keep in mind though. The TCON dotclock in the MIPI-DSI case isn't directly tied to the Bit Clock, it's simply an internal clock in the pipeline to feed the FIFO of the MIPI-DSI controller. The MIPI-DSI controller itself will generate that clock, and only that one will have those constraints. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com