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;
References: <20200216173446.1823-1-linux.amoon@gmail.com> <20200216173446.1823-4-linux.amoon@gmail.com> <1jmu9hzlo2.fsf@starbuckisacylon.baylibre.com> <CANAwSgSaQgU=H3h0S9deT11HA8z9R=Fhy5Kawii9tSBxKf2Wgw@mail.gmail.com> <CAFBinCCSosE1XfwbKZOR9G+DVYg8zFcKShmTNWUhh1e8W0VoAQ@mail.gmail.com> <CANAwSgRZy1K0GZq30cEoH2KiJfjX-5LvkMy79ZeM_aSEyrkD+g@mail.gmail.com>
User-agent: mu4e 1.3.3; emacs 26.3
From:   Jerome Brunet <jbrunet@baylibre.com>
To:     Anand Moon <linux.amoon@gmail.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Michael Turquette <mturquette@baylibre.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Kevin Hilman <khilman@baylibre.com>,
        devicetree <devicetree@vger.kernel.org>,
        linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
        linux-amlogic@lists.infradead.org,
        Linux Kernel <linux-kernel@vger.kernel.org>,
        "open list\:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>
Subject: Re: [PATCHv1 3/3] clk: meson: g12a: set cpu clock divider flags too CLK_IS_CRITICAL
In-reply-to: <CANAwSgRZy1K0GZq30cEoH2KiJfjX-5LvkMy79ZeM_aSEyrkD+g@mail.gmail.com>
Date:   Mon, 24 Feb 2020 10:31:01 +0100
Message-ID: <1jo8togwmi.fsf@starbuckisacylon.baylibre.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On Sun 23 Feb 2020 at 14:34, Anand Moon <linux.amoon@gmail.com> wrote:

> Hi Martin / Jerome / Neil,
>
> On Fri, 21 Feb 2020 at 02:45, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> wrote:
>>
>> Hi Anand,
>>
>> On Mon, Feb 17, 2020 at 2:30 PM Anand Moon <linux.amoon@gmail.com> wrote:
>> [...]
>> > > > @@ -681,7 +682,7 @@ static struct clk_regmap g12b_cpub_clk = {
>> > > >                       &g12a_sys_pll.hw
>> > > >               },
>> > > >               .num_parents = 2,
>> > > > -             .flags = CLK_SET_RATE_PARENT,
>> > > > +             .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>> > >
>> > > Why not. Neil what do you think of this ?
>> > > If nothing is claiming this clock and enabling it then I suppose it
>> > > could make sense.
>> > >
>> > I would like core developers to handle this.
>> > Sorry for the noise.
>> can you please resend this patch with only the change to g12b_cpub_clk?
>> I have no G12B board myself so it would be great if you could take care of this!
>>
>>
>> Martin
>
> Thanks, yes I will try again, but I have a question.
>
> On eMMC module  *cpub_clk* is not getting enabled, see below is
> clk_summay of eMMC.

I'm sorry but I don't understand the link between the cpu clock of the
second cluster and MMC.

> [...]
>           fclk_div2_div               1        1        0   999999985
>         0     0  50000
>              fclk_div2                2        2        0   999999985
>         0     0  50000
>                 ff3f0000.ethernet#m250_sel       1        1        0
> 999999985          0     0  50000
>                    ff3f0000.ethernet#m250_div       1        1
> 0   249999997          0     0  50000
>                       ff3f0000.ethernet#fixed_div2       1        1
>     0   124999998          0     0  50000
>                          ff3f0000.ethernet#rgmii_tx_en       1
> 1        0   124999998          0     0  50000
>                 ffe07000.mmc#mux       1        1        0   999999985
>          0     0  50000
>                    ffe07000.mmc#div       1        1        0
> 199999997          0     0  50000
>                 cpub_clk_dyn1_sel       0        0        0
> 999999985          0     0  50000
>                    cpub_clk_dyn1       0        0        0   999999985
>          0     0  50000
>                       cpub_clk_dyn       0        0        0
> 999999985          0     0  50000
>                          cpub_clk       0        0        0
> 999999985          0     0  50000
>                             cpub_clk_div8       0        0        0
> 124999998          0     0  50000
>                             cpub_clk_div7       0        0        0
> 142857140          0     0  50000
>                             cpub_clk_div6       0        0        0
> 166666664          0     0  50000
>                                cpub_clk_trace_sel       0        0
>    0   166666664          0     0  50000
>                                   cpub_clk_trace       0        0
>   0   166666664          0     0  50000
>                             cpub_clk_div5       0        0        0
> 199999997          0     0  50000
>                                cpub_clk_apb_sel       0        0
>  0   199999997          0     0  50000
>                                   cpub_clk_apb       0        0
> 0   199999997          0     0  50000
>                             cpub_clk_div4       0        0        0
> 249999996          0     0  50000
>                             cpub_clk_div3       0        0        0
> 333333328          0     0  50000
>                                cpub_clk_atb_sel       0        0
>  0   333333328          0     0  50000
>                                   cpub_clk_atb       0        0
> 0   333333328          0     0  50000
>                             cpub_clk_div2       0        0        0
> 499999992          0     0  50000
>                                cpub_clk_axi_sel       0        0
>  0   499999992          0     0  50000
>                                   cpub_clk_axi       0        0
> 0   499999992          0     0  50000
>                             cpub_clk_div16_en       0        0
> 0   999999985          0     0  50000
>                                cpub_clk_div16       0        0
> 0    62499999          0     0  50000

I can't read that.

>
> After enable *cpub_clk* flags with
> .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> this clk is enabled on microSD card see clk_summary below.

Again, I don't get the relationship between cpub and sdcard (or eMMC)

> [...]
>          fclk_div2_div               1        1        0   999999985
>        0     0  50000
>              fclk_div2                3        3        0   999999985
>         0     0  50000
>                 ff3f0000.ethernet#m250_sel       1        1        0
> 999999985          0     0  50000
>                    ff3f0000.ethernet#m250_div       1        1
> 0   249999997          0     0  50000
>                       ff3f0000.ethernet#fixed_div2       1        1
>     0   124999998          0     0  50000
>                          ff3f0000.ethernet#rgmii_tx_en       1
> 1        0   124999998          0     0  50000
>                 ffe05000.sd#mux       1        1        0   999999985
>         0     0  50000
>                    ffe05000.sd#div       1        1        0
> 50000000          0     0  50000
>                 cpub_clk_dyn1_sel       1        1        0
> 999999985          0     0  50000
>                    cpub_clk_dyn1       1        1        0   999999985
>          0     0  50000
>                       cpub_clk_dyn       1        1        0
> 999999985          0     0  50000
>                          cpub_clk       1        1        0
> 999999985          0     0  50000
>                             cpub_clk_div8       0        0        0
> 124999998          0     0  50000
>                             cpub_clk_div7       0        0        0
> 142857140          0     0  50000
>                             cpub_clk_div6       0        0        0
> 166666664          0     0  50000
>                                cpub_clk_trace_sel       0        0
>    0   166666664          0     0  50000
>                                   cpub_clk_trace       0        0
>   0   166666664          0     0  50000
>                             cpub_clk_div5       0        0        0
> 199999997          0     0  50000
>                                cpub_clk_apb_sel       0        0
>  0   199999997          0     0  50000
>                                   cpub_clk_apb       0        0
> 0   199999997          0     0  50000
>                             cpub_clk_div4       0        0        0
> 249999996          0     0  50000
>                             cpub_clk_div3       0        0        0
> 333333328          0     0  50000
>                                cpub_clk_atb_sel       0        0
>  0   333333328          0     0  50000
>                                   cpub_clk_atb       0        0
> 0   333333328          0     0  50000
>                             cpub_clk_div2       0        0        0
> 499999992          0     0  50000
>                                cpub_clk_axi_sel       0        0
>  0   499999992          0     0  50000
>                                   cpub_clk_axi       0        0
> 0   499999992          0     0  50000
>                             cpub_clk_div16_en       0        0
> 0   999999985          0     0  50000
>                                cpub_clk_div16       0        0
> 0    62499999          0     0  50000
>                    cpub_clk_dyn1_div       0        0        0
> 999999985          0     0  50000
>
> Is this correct approach to set the flags to enable *cpub_clk*.
> .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
>
> What I meant is their *Dyn_enable[26]* field for enable/disable for
> HHI_SYS_CPU_CLK_CNTL0 and HHI_SYS_CPUB_CLK_CNTL clk controller.
> in the S922X datasheets which could help resolve this issue.
> Any thought on this.

I sorry but I'm just lost. I don't understand anything above so I can't
comment.

>
> -Anand