Re: [PATCH 8/8] clk: tegra: Add EMC clock driver

[Mikko Perttunen]

On 22/07/14 19:57, Stephen Warren wrote:
> On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> > The driver is currently only tested on Tegra124 Jetson TK1, but should
> > work with other Tegra124 boards, provided that correct EMC tables are
> > provided through the device tree. Older chip models have differing
> > timing change sequences, so they are not currently supported.
>
> > diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
>
> > +struct emc_timing {
> > +	unsigned long rate, parent_rate;
> > +	u8 parent_index;
> > +	struct clk *parent;
> > +
> > +	/* Store EMC burst data in a union to minimize mistakes. This allows
> > +	 * us to use the same burst data lists as used by the downstream and
> > +	 * ChromeOS kernels. */
>
> Nit: */ should be on its own line. This applies to many comments in the
> file.

Will fix.

> > +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> > + * Timing change sequence functions                *
> > + * * * * * * * * * * * * * * * * * * * * * * * * * */
>
> Nit: This kind of banner comment is unusual, but I guess it's fine.
>
> > +static void emc_seq_update_timing(struct tegra_emc *tegra)
> ...
> > +	dev_err(&tegra->pdev->dev, "timing update failed\n");
> > +	BUG();
> > +}
>
> Is there any way to avoid all these BUGs? Can we just continue on and
> retry the next time, or disallow any further clock rate changes or
> something?

I guess I can just remove the BUG()s and keep going. The clock might 
temporarily end up in a strange state but that should be fine since 
these shouldn't be happening anyway.

> > +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> > + * Debugfs entry                                   *
> > + * * * * * * * * * * * * * * * * * * * * * * * * * */
> > +
> > +static int emc_debug_rate_get(void *data, u64 *rate)
> > +{
> > +	struct tegra_emc *tegra = data;
> > +
> > +	*rate = clk_get_rate(tegra->hw.clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static int emc_debug_rate_set(void *data, u64 rate)
> > +{
> > +	struct tegra_emc *tegra = data;
> > +
> > +	return clk_set_rate(tegra->hw.clk, rate);
> > +}
> > +
> > +DEFINE_SIMPLE_ATTRIBUTE(emc_debug_rate_fops, emc_debug_rate_get,
> > +			emc_debug_rate_set, "%lld\n");
>
> I think the rate can already be obtained through
> ...debug/clock/clock_summary. I'm not sure about changing the rate, but
> shouldn't that be a feature of the common clock core, not individual
> drivers?

The core doesn't allow writing to the rate debugfs files, so this is the 
only way to trigger an EMC clock change for now. I agree that the core 
might be a better place. I don't know if there are any philosophical 
objections to that. I'd like to keep this in until a possible core 
feature addition. Mike, any comments?

> > +static int load_timings_from_dt(struct tegra_emc *tegra,
> > +				struct device_node *node)
> > +{
> ...
> > +	for_each_child_of_node(node, child) {
> ...
> > +		if (timing->rate <= prev_rate) {
> > +			dev_err(&tegra->pdev->dev,
> > +				"timing %s: rate not increasing\n",
> > +				child->name);
>
> I don't believe there's any guaranteed node enumeration order. If the
> driver needs the child nodes sorted, it should sort them itself.

True. I'll fix this.

> > +static const struct of_device_id tegra_car_of_match[] = {
> > +	{ .compatible = "nvidia,tegra124-car" },
> > +	{}
> > +};
> > +
> > +static const struct of_device_id tegra_mc_of_match[] = {
> > +	{ .compatible = "nvidia,tegra124-mc" },
> > +	{}
> > +};
>
> It would be better if this driver explicitly called into the driver for
> other modules, rather than directly touching their registers.

My local v2 already has the MC-related code split into Thierry's MC 
driver. The CAR register writing is still done from the EMC driver. I 
could add helpers for it to the CAR driver.

Thanks,
Mikko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/