linux-kernel - RE: [PATCH v15 05/10] clk: Add Sunplus SP7021 clock 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: <7febed72ae2747abb953a6f44a51102c@cqplus1.com>
Date:   Tue, 17 May 2022 09:30:24 +0000
From:   qinjian[覃健] <qinjian@...lus1.com>
To:     Stephen Boyd <sboyd@...nel.org>
CC:     "krzysztof.kozlowski@...aro.org" <krzysztof.kozlowski@...aro.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mturquette@...libre.com" <mturquette@...libre.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "maz@...nel.org" <maz@...nel.org>,
        "p.zabel@...gutronix.de" <p.zabel@...gutronix.de>,
        "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "arnd@...db.de" <arnd@...db.de>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-clk@...r.kernel.org" <linux-clk@...r.kernel.org>
Subject: RE: [PATCH v15 05/10] clk: Add Sunplus SP7021 clock driver

> > +               nf = fvco * m_table[m];
> > +               n = nf / F_27M;
> > +               if ((n * F_27M) == nf)
> > +                       break;
> > +       }
> > +       m = m_table[m];
> > +
> > +       if (!m) {
> 
> This can be resolved with a 'return' from a subfunction or a goto.

Sorry, Could you explain more clear?
Did you means like:
	If (!m) {
		ret = -EINVAL;
		goto err_not_found;
	}
	...
	return freq;

err_not_found:
	...
	return ret;
}

> 
> > +               pr_err("%s: %s freq:%lu not found a valid setting\n",
> > +                      __func__, clk_hw_get_name(&clk->hw), freq);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* save parameters */
> > +       clk->p[SEL_FRA] = 0;
> > +       clk->p[DIVR]    = r;
> > +       clk->p[DIVN]    = n;
> > +       clk->p[DIVM]    = m;
> > +
> > +       return freq;
> > +}




> > +
> > +                               df_quotient  = df / m;
> > +                               df_remainder = ((df % m) * 1000) / m;
> > +
> > +                               if (freq > df_quotient) {
> > +                                       df_quotient  = freq - df_quotient - 1;
> > +                                       df_remainder = 1000 - df_remainder;
> 
> Where does 1000 come from?

1000 is come from "borrow 1" in last operation.

> 
> > +                               } else {
> > +                                       df_quotient = df_quotient - freq;
> > +                               }
> > +




> > +static int sp_pll_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                          unsigned long prate)
> > +{
> > +       struct sp_pll *clk = to_sp_pll(hw);
> > +       unsigned long flags;
> > +       u32 reg;
> > +
> > +       spin_lock_irqsave(clk->lock, flags);
> 
> Please push the lock down. Maybe that means having two conditionals?
> Either way, it is too large because it calls into plla_set_rate() and
> plltv_set_rate() with the lock held.

Sorry, I don't understand "push the lock down. Maybe that means having two conditionals?" what means.
The plla_set_rate() & plltv_set_rate() is only include simple operations & HW reg writes.
I think it should be called with lock held.

static void plla_set_rate(struct sp_pll *clk)
{
	const u32 *pp = pa[clk->p[0]].regs;
	int i;

	for (i = 0; i < ARRAY_SIZE(pa->regs); i++)
		writel(0xffff0000 | pp[i], clk->reg + (i * 4));
}

static void plltv_set_rate(struct sp_pll *clk)
{
	u32 reg;

	reg  = HWM_FIELD_PREP(MASK_SEL_FRA, clk->p[SEL_FRA]);
	reg |= HWM_FIELD_PREP(MASK_SDM_MOD, clk->p[SDM_MOD]);
	reg |= HWM_FIELD_PREP(MASK_PH_SEL, clk->p[PH_SEL]);
	reg |= HWM_FIELD_PREP(MASK_NFRA, clk->p[NFRA]);
	writel(reg, clk->reg);

	reg  = HWM_FIELD_PREP(MASK_DIVR, clk->p[DIVR]);
	writel(reg, clk->reg + 4);

	reg  = HWM_FIELD_PREP(MASK_DIVN, clk->p[DIVN] - 1);
	reg |= HWM_FIELD_PREP(MASK_DIVM, clk->p[DIVM] - 1);
	writel(reg, clk->reg + 8);
}