linux-kernel - Re: [PATCH v5 3/3] clk: meson: add sub MMC clock controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFBinCCuqUmVNdwUm7WbkHy1eWvOA5oQ5FcOuytbYNbgGcXnRQ@mail.gmail.com>
Date:   Thu, 25 Oct 2018 22:58:30 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     jbrunet@...libre.com
Cc:     jianxin.pan@...ogic.com, Neil Armstrong <narmstrong@...libre.com>,
        yixun.lan@...ogic.com, khilman@...libre.com, carlo@...one.org,
        mturquette@...libre.com, sboyd@...nel.org, robh@...nel.org,
        miquel.raynal@...tlin.com, boris.brezillon@...tlin.com,
        liang.yang@...ogic.com, jian.hu@...ogic.com,
        qiufang.dai@...ogic.com, hanjie.lin@...ogic.com,
        victor.wan@...ogic.com, linux-clk@...r.kernel.org,
        linux-amlogic@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 3/3] clk: meson: add sub MMC clock controller driver

Hi Jerome,

On Thu, Oct 25, 2018 at 2:54 PM Jerome Brunet <jbrunet@...libre.com> wrote:
[snip]
> > > > +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.
an old version of this series had the code still in the NAND driver
(by writing to the registers directly instead of using the clk API).
this looks pretty much like a "sclk-div" to me (as I commented in v3
of this series: [0]):
- value 0 means disabled
- positive divider values
- (probably no duty control, but that's optional as far as I
understand sclk-div)
- uses max divider value when enabling the clock

if switching to sclk-div works then we can get rid of some duplicate code


Regards
Martin


[0] https://patchwork.kernel.org/patch/10607157/#22238243