Received: by 2002:ac0:98c7:0:0:0:0:0 with SMTP id g7-v6csp2370920imd; Sun, 28 Oct 2018 08:37:23 -0700 (PDT) X-Google-Smtp-Source: AJdET5fKssSb0l/Jzt4MVuEidh+kRkATiywnebWbMmhyrCcWWZXUaZYFrn3etKfimt6AfGxTWAqB X-Received: by 2002:a63:a902:: with SMTP id u2-v6mr10618461pge.207.1540741043178; Sun, 28 Oct 2018 08:37:23 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1540741043; cv=none; d=google.com; s=arc-20160816; b=nIeODznHAFIFKwrpiq36yd7DryPGeAN//Fcjmc8A0ScZ822XVXQm15Bz1p3G7WAYRN 9cqCYkVOxgmSzMfQgMM9XCl9ucMvPQEI57lxkcV/7COsJ1njfEBXLpT9DaeTcJ3iWojO 63zdt5I7xdRNMRZFmL75XjmQuEt5a57/1X+s9Wo6Lkii6Y78MFFtGnBwgdzmeXDdEMTu OqcFbHanoSJDOLmkIxF/UJIPr/vd0Rk1EBkmSVGcLMuRsxJEJ580NZvrIzmZabHNI+UY W46hegDb81FkRTF/SrUjqs3WKis5BxcgyF5jO0r+bEazLDCM3AWbh4hmdQ/bBXjpJCaX 6INA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=8H5+pr6zZtTqgpWeVWWL0pZvKzgckGKijrvseCixJCQ=; b=nBr3I6CutIUs3ZM4BLJp860KW+ihEokt5RBP2PnMZlRyqe1RYUG/h3XNaUBX+0hN3n qQPva3yYaZ7mtzu8P4ZqOeq8v+RGZguK0z04/mkmX0chHokKgHF36lVfOY4U1ozCqtSx 9QLN+ND/I3XsPQeTHCXP85Kc6XoYpvGENIwbteaYcRwTDznMS69M6un57tD+hjcpaK1n NesnRDqdz3KHnrSRt2UTz6SceGU7WQMvN+GhPFt2SeomDsaTkmK43FlhckVGhDrW8Mto erjhE+RYjo5zAU5dDHi4EOjFv1G1WZvrwZEqVxgaf3POMZEc1dginZXilGXcUk+7tfOm 35Lw== 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 20-v6si18293559pgk.190.2018.10.28.08.36.42; Sun, 28 Oct 2018 08:37:23 -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 S1727690AbeJ1X6F (ORCPT + 99 others); Sun, 28 Oct 2018 19:58:05 -0400 Received: from mail-sh2.amlogic.com ([58.32.228.45]:33160 "EHLO mail-sh2.amlogic.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726492AbeJ1X6F (ORCPT ); Sun, 28 Oct 2018 19:58:05 -0400 Received: from [192.168.0.111] (223.167.21.242) by mail-sh2.amlogic.com (10.18.11.6) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Sun, 28 Oct 2018 23:12:59 +0800 Subject: Re: [PATCH v5 3/3] clk: meson: add sub MMC clock controller driver To: Jerome Brunet , Neil Armstrong CC: Yixun Lan , Kevin Hilman , Carlo Caione , Michael Turquette , Stephen Boyd , Rob Herring , Miquel Raynal , Boris Brezillon , Martin Blumenstingl , Liang Yang , Jian Hu , Qiufang Dai , Hanjie Lin , Victor Wan , , , , References: <1539839245-13793-1-git-send-email-jianxin.pan@amlogic.com> <1539839245-13793-4-git-send-email-jianxin.pan@amlogic.com> From: Jianxin Pan Message-ID: Date: Sun, 28 Oct 2018 23:12:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [223.167.21.242] X-ClientProxiedBy: mail-sh2.amlogic.com (10.18.11.6) To mail-sh2.amlogic.com (10.18.11.6) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Jerome, On 2018/10/25 20:54, Jerome Brunet wrote: > On Thu, 2018-10-25 at 19:48 +0800, Jianxin Pan wrote: >> Hi Jerome, >> >> On 2018/10/24 17:01, Jerome Brunet wrote: >>> On Thu, 2018-10-18 at 13:07 +0800, Jianxin Pan wrote: >>>> From: Yixun Lan >>>> >>>> The patch will add a MMC clock controller driver which used by MMC or NAND, >>>> It provide a mux and divider clock, and three phase clocks - core, tx, tx. >>>> [...] >>>> >>>> -/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */ >>>> +static void clk_regmap_div_init(struct clk_hw *hw) >>>> +{ >>>> + struct clk_regmap *clk = to_clk_regmap(hw); >>>> + struct clk_regmap_div_data *div = clk_get_regmap_div_data(clk); >>>> + unsigned int val; >>>> + int ret; >>>> + >>>> + ret = regmap_read(clk->map, div->offset, &val); >>>> + if (ret) >>>> + return; >>>> >>>> + val &= (clk_div_mask(div->width) << div->shift); >>>> + if (!val) >>>> + regmap_update_bits(clk->map, div->offset, >>>> + clk_div_mask(div->width) << div->shift, >>>> + clk_div_mask(div->width)); >>> >>> This is wrong for several reasons: >>> * You should hard code the initial value in the driver. >>> * If shift is not 0, I doubt this will give the expected result. >> >> The value 0x00 of divider means nand clock off then read/write nand register is forbidden. > > That is not entirely true, you can access the clock register or you'd be in a > chicken and egg situation. > >> Should we set the initial value in nand driver, or in sub emmc clk driver? > > In the nand driver, which is the consumer of the clock. see my previous comments > about it. > > If your device (nand in your case) needs a (sane) clock before doing anything > else, just call clk_set_rate() and enable it with clk_prepare_enable(). > > Look at our MMC driver, this is the first thing done after registering the > clocks. > > The controller does no care at all about the clock rate or state. It is up to > the consumer to express their needs. > > On a more general note: > I agree that having a 0 value for CLK_DIVIDER_ONE_BASED divider makes no sense. > If you think this point needs to be addressed, please: > > * make separated generic patch > * against driver/clk/clk-divider.c > * addressing specifically CLK_DIVIDER_ONE_BASED dividers (not all, as your > change does) > * with some comments explaining the intent. In our emmc driver, the CLK_DIV_MASK is hard coded before clock registering in meson_mmc_clk_init(): clk_reg |= CLK_DIV_MASK; writel(clk_reg, host->regs + SD_EMMC_CLOCK); It's hard coded In previous version of nand driver. I will drop the new ops. In addition , Martin suggested in previous discussions that "sclk-div" could be used [0][1]. This divider is just like a "sclk-div" with sclk->hi->width ==0. > >> >>> >>>> +} >>> >>> Please drop this. OK. Thank you. >>> >>>> + >>>> +/* Would prefer clk_regmap_div_ro_ops but clashes with qcom */ >>>> const struct clk_ops clk_regmap_divider_ops = { >>>> .recalc_rate = clk_regmap_div_recalc_rate, >>>> .round_rate = clk_regmap_div_round_rate, >>>> @@ -122,6 +139,14 @@ static int clk_regmap_div_set_rate(struct clk_hw *hw, unsigned long rate, >>>> }; >>>> EXPORT_SYMBOL_GPL(clk_regmap_divider_ops); >>>> [...] >>>> + >>>> +static struct clk_regmap_mux_data mmc_clkc_mux_data = { >>>> + .offset = SD_EMMC_CLOCK, >>>> + .mask = 0x3, >>>> + .shift = 6, >>>> + .flags = CLK_DIVIDER_ROUND_CLOSEST, > > Missed that earlier > > This flag makes no sense for a mux. > Anyway this particular mux should never round up has doing so would be unsafe. > The clock provided to the nand or the eMMC should be the requested or lower. > OK, I will drop it. Thanks for your time. >>>> +}; >>>> + >>>> +static struct clk_regmap_div_data mmc_clkc_div_data = { >>>> + .offset = SD_EMMC_CLOCK, >>>> + .shift = 0, >>>> + .width = 6, >>>> + .flags = CLK_DIVIDER_ROUND_CLOSEST | CLK_DIVIDER_ONE_BASED, > > Same here, drop CLK_DIVIDER_ROUND_CLOSEST OK, I will drop it. Thank you! > >>>> +}; >>>> + >>>> +static struct meson_clk_phase_data mmc_clkc_core_phase = { >>>> + .ph = { >>>> + .reg_off = SD_EMMC_CLOCK, >>>> + .shift = 8, [...] > > . > [0] https://patchwork.kernel.org/patch/10607157/#22238243 [1]https://lore.kernel.org/lkml/CAFBinCCuqUmVNdwUm7WbkHy1eWvOA5oQ5FcOuytbYNbgGcXnRQ@mail.gmail.com/T/#u