lkml.org 
[lkml]   [2014]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 8/8] clk: tegra: Add EMC clock driver
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


\
 
 \ /
  Last update: 2014-07-29 11:21    [W:0.132 / U:0.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site