Messages in this thread | | | Date | Tue, 29 Jul 2014 11:47:35 +0300 | From | Mikko Perttunen <> | Subject | Re: [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
| |