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

[Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

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