Re: [PATCH v5 3/8] clk: tegra: Implement Tegra210 EMC clock

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

On Tue, Mar 10, 2020 at 08:44:38PM +0300, Dmitry Osipenko wrote:
> 10.03.2020 18:19, Thierry Reding пишет:
> > From: Joseph Lo <josephl@nvidia.com>
> > 
> > The EMC clock needs to carefully coordinate with the EMC controller
> > programming to make sure external memory can be properly clocked. Do so
> > by hooking up the EMC clock with an EMC provider that will specify which
> > rates are supported by the EMC and provide a callback to use for setting
> > the clock rate at the EMC.
> > 
> > Based on work by Peter De Schrijver <pdeschrijver@nvidia.com>.
> > 
> > Signed-off-by: Joseph Lo <josephl@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v5:
> > - major rework and cleanup
> 
> ...
> > +static u8 tegra210_clk_emc_get_parent(struct clk_hw *hw)
> > +{
> > +	struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw);
> > +	u32 value;
> > +	u8 src;
> > +
> > +	value = readl_relaxed(emc->regs + CLK_SOURCE_EMC);
> > +	src = (value >> CLK_SOURCE_EMC_2X_CLK_SRC_SHIFT) &
> > +			CLK_SOURCE_EMC_2X_CLK_SRC_MASK;
> 
> What about to use a generic FIELD_GET/PREP()?

Done.

> > +static int tegra210_clk_emc_set_rate(struct clk_hw *hw, unsigned long rate,
> > +				     unsigned long parent_rate)
> > +{
> > +	struct tegra210_clk_emc *emc = to_tegra210_clk_emc(hw);
> > +	struct tegra210_clk_emc_provider *provider = emc->provider;
> > +	struct tegra210_clk_emc_config *config;
> > +	struct device *dev = provider->dev;
> > +	struct clk_hw *old, *new, *parent;
> > +	u8 old_idx, new_idx, index;
> > +	struct clk *clk;
> > +	unsigned int i;
> > +	int err;
> > +
> > +	if (!provider || !provider->configs || provider->num_configs == 0)
> > +		return -EINVAL;
> 
> Why all these checks are needed? I don't think it ever could fail,
> couldn't it?

This could fail if no EMC provider is attached, which happens, for
example, when the EMC driver is not loaded.

> 
> > +static int emc_table_lookup(struct tegra_emc *emc, unsigned long rate)
> > +{
> > +	int i;
> 
> unsigned int
> 
> Same for all other occurrences in the code.

This was fixed automatically after I fixed the rebase issues.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]