Re: [PATCH v4 10/13] memory: tegra: Add EMC (external memory controller) driver
From: Mikko Perttunen
Date: Wed Nov 12 2014 - 11:25:12 EST
On 11/12/2014 05:45 PM, Thierry Reding wrote:
On Wed, Nov 12, 2014 at 08:56:33AM +0100, Tomeu Vizoso wrote:
[...]
diff --git a/drivers/memory/tegra/tegra124-emc.c b/drivers/memory/tegra/tegra124-emc.c
[...]
+static int t124_emc_burst_regs[] = {
The t124 prefix seems rather redundant in a Tegra124-specific file. Also
these should really be unsigned.
+struct emc_timing {
+ unsigned long rate;
+
+ /*
+ * 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.
I don't understand what this means. Can you elaborate?
We could store the three specifically named u32's separately, but using
a union we can easily keep the indexing for the burst registers
identical to the indexing used in the downstream and ChromeOS kernels.
In reality, these burst register lists are generated by NVIDIA's
downstream tool, so allowing use of the same lists minimizes possible
(probably very difficult to spot) mistakes when porting mach
files/device trees from those kernels to the upstream kernel.
+ */
+ union {
+ u32 emc_burst_data[ARRAY_SIZE(t124_emc_burst_regs)];
+ struct {
+ u32 __pad0[121];
+ u32 emc_xm2dqspadctrl2;
+ u32 __pad1[15];
+ u32 emc_zcal_interval;
+ u32 __pad2[1];
+ u32 emc_mrs_wait_cnt;
+ };
+ };
[...]
+struct tegra_emc {
+ struct platform_device *pdev;
I don't see this ever used other than for accessing pdev->dev, in which
case it would be much simpler and save a lot of characters if this was
simply:
struct device *dev;
+ struct tegra_mc *mc;
+
+ void __iomem *emc_regs;
There is no second set of registers, so the emc_ prefix is useless.
+
+ enum emc_dram_type dram_type;
+ u8 dram_num;
Should be an unsized type and match what's returned by the MC accessor.
+
+ struct emc_timing last_timing;
struct emc_timing is rather big, can this be a pointer to an entry in
the timings array instead?
No. The timing set by the pre-kernel boot process might not correspond
to any timing in the kernel's timing lists. Of course, we don't need
most of the values at that point (and don't populate them either), so it
would be possible to special case the timing change function to use some
separate structure for the first timing change. Doing what is done now
is simpler, though. It is also how it's done downstream (not that that
is a good indicator of good design..)
+ struct emc_timing *timings;
+ unsigned int num_timings;
+};
+
+/* Timing change sequence functions */
+
+static void emc_ccfifo_writel(struct tegra_emc *tegra, u32 val,
+ unsigned long offs)
+{
+ writel(val, tegra->emc_regs + EMC_CCFIFO_DATA);
+ writel(offs, tegra->emc_regs + EMC_CCFIFO_ADDR);
+}
+
+static void emc_seq_update_timing(struct tegra_emc *tegra)
+{
+ int i;
unsigned int, please.
+
+ writel(1, tegra->emc_regs + EMC_TIMING_CONTROL);
+
+ for (i = 0; i < EMC_STATUS_UPDATE_TIMEOUT; ++i) {
+ if (!(readl(tegra->emc_regs + EMC_STATUS) &
+ EMC_STATUS_TIMING_UPDATE_STALLED))
+ return;
+ }
You're busy-looping 1000 times here. What are the real criteria here?
How long is this allowed to take? Should this not be a timed loop where
the number of iterations doesn't matter?
Logic directly taken from downstream/hw testing code. A timeout would
sound more logical, but dunno what to use.
+
+ dev_err(&tegra->pdev->dev, "timing update failed\n");
+}
Should this return an error that can be propagated?
I'm not sure if it's safe to stop during the timing change sequence.. at
least the downstream code doesn't try either. To know for sure, you'd
need some internal guy who really knows the sequence.
+static void emc_seq_disable_auto_cal(struct tegra_emc *tegra)
+{
+ int i;
+
+ writel(0, tegra->emc_regs + EMC_AUTO_CAL_INTERVAL);
+
+ for (i = 0; i < EMC_STATUS_UPDATE_TIMEOUT; ++i) {
+ if (!(readl(tegra->emc_regs + EMC_AUTO_CAL_STATUS) &
+ EMC_AUTO_CAL_STATUS_ACTIVE))
+ return;
+ }
+
+ dev_err(&tegra->pdev->dev, "auto cal disable failed\n");
+}
Same comments as for emc_seq_update_timing().
+
+static void emc_seq_wait_clkchange(struct tegra_emc *tegra)
+{
+ int i;
+
+ for (i = 0; i < EMC_STATUS_UPDATE_TIMEOUT; ++i) {
+ if (readl(tegra->emc_regs + EMC_INTSTATUS) &
+ EMC_INTSTATUS_CLKCHANGE_COMPLETE)
+ return;
+ }
+
+ dev_err(&tegra->pdev->dev, "clkchange failed\n");
+}
And again.
+
+static void emc_prepare_timing_change(struct tegra_emc *tegra,
+ const struct emc_timing *timing)
+{
+ u32 val, val2;
+ bool update = false;
+ int pre_wait = 0;
+ int i;
Both of these can be unsigned.
+ enum emc_dll_change dll_change;
+
+ if ((tegra->last_timing.emc_mode_1 & 0x1) == (timing->emc_mode_1 & 1))
+ dll_change = DLL_CHANGE_NONE;
+ else if (timing->emc_mode_1 & 1)
+ dll_change = DLL_CHANGE_ON;
+ else
+ dll_change = DLL_CHANGE_OFF;
+
+ /* Clear CLKCHANGE_COMPLETE interrupts */
+
+ writel(EMC_INTSTATUS_CLKCHANGE_COMPLETE,
+ tegra->emc_regs + EMC_INTSTATUS);
+
+ /* Disable dynamic self-refresh */
+
+ val = readl(tegra->emc_regs + EMC_CFG);
+ if (val & EMC_CFG_PWR_MASK) {
+ val &= ~EMC_CFG_POWER_FEATURES_MASK;
+ writel(val, tegra->emc_regs + EMC_CFG);
+
+ pre_wait = 5;
+ }
+
+ /* Disable SEL_DPD_CTRL for clock change */
+
+ val = readl(tegra->emc_regs + EMC_SEL_DPD_CTRL);
+ if (val & (tegra->dram_type == DRAM_TYPE_DDR3 ?
+ EMC_SEL_DPD_CTRL_DDR3_MASK : EMC_SEL_DPD_CTRL_MASK)) {
This is really hard to read. Perhaps something like:
if (tegra->dram_type == DRAM_TYPE_DDR3)
mask = EMC_SEL_DPD_CTRL_DDR3_MASK;
else
mask = EMC_SEL_DPD_CTRL_MASK;
?
+ val &= ~(EMC_SEL_DPD_CTRL_DATA_SEL_DPD |
+ EMC_SEL_DPD_CTRL_ODT_SEL_DPD |
+ EMC_SEL_DPD_CTRL_CA_SEL_DPD |
+ EMC_SEL_DPD_CTRL_CLK_SEL_DPD);
+ if (tegra->dram_type == DRAM_TYPE_DDR3)
+ val &= ~EMC_SEL_DPD_CTRL_RESET_SEL_DPD;
These seem to be all the fields already present in the respective masks,
so why not just:
val &= ~mask;
?
+ writel(val, tegra->emc_regs + EMC_SEL_DPD_CTRL);
+ }
+
+ /* Prepare DQ/DQS for clock change */
+
+ val = readl(tegra->emc_regs + EMC_BGBIAS_CTL0);
+ val2 = tegra->last_timing.emc_bgbias_ctl0;
+ if (!(timing->emc_bgbias_ctl0 &
+ EMC_BGBIAS_CTL0_BIAS0_DSC_E_PWRD_IBIAS_RX) &&
+ (val & EMC_BGBIAS_CTL0_BIAS0_DSC_E_PWRD_IBIAS_RX)) {
+ val2 &= ~EMC_BGBIAS_CTL0_BIAS0_DSC_E_PWRD_IBIAS_RX;
+ update = true;
+ }
+
+ if ((val & EMC_BGBIAS_CTL0_BIAS0_DSC_E_PWRD) ||
+ (val & EMC_BGBIAS_CTL0_BIAS0_DSC_E_PWRD_IBIAS_VTTGEN)) {
+ update = true;
+ }
+
+ if (update) {
+ writel(val2, tegra->emc_regs + EMC_BGBIAS_CTL0);
+ if (pre_wait < 5)
+ pre_wait = 5;
+ }
+
+ update = false;
+ val = readl(tegra->emc_regs + EMC_XM2DQSPADCTRL2);
+ if (timing->emc_xm2dqspadctrl2 & EMC_XM2DQSPADCTRL2_VREF_ENABLE &&
+ !(val & EMC_XM2DQSPADCTRL2_VREF_ENABLE)) {
+ val |= EMC_XM2DQSPADCTRL2_VREF_ENABLE;
+ update = true;
+ }
+
+ if (timing->emc_xm2dqspadctrl2 & EMC_XM2DQSPADCTRL2_RX_FT_REC_ENABLE &&
+ !(val & EMC_XM2DQSPADCTRL2_RX_FT_REC_ENABLE)) {
+ val |= EMC_XM2DQSPADCTRL2_RX_FT_REC_ENABLE;
+ update = true;
+ }
+
+ if (update) {
+ writel(val, tegra->emc_regs + EMC_XM2DQSPADCTRL2);
+ if (pre_wait < 30)
+ pre_wait = 30;
+ }
+
+ /* Wait to settle */
+
+ if (pre_wait) {
+ emc_seq_update_timing(tegra);
+ udelay(pre_wait);
+ }
+
+ /* Program CTT_TERM control */
+
+ if (tegra->last_timing.emc_ctt_term_ctrl != timing->emc_ctt_term_ctrl) {
There are a whole lot of very long lines caused by tegra->last_timing.
How about introducing a temporary variable:
struct emc_timing *last = &tegra->last_timing;
or simply:
struct emc_timing *last = tegra->last_timing;
after a change I suggested earlier.
+ emc_seq_disable_auto_cal(tegra);
+ writel(timing->emc_ctt_term_ctrl,
+ tegra->emc_regs + EMC_CTT_TERM_CTRL);
+ emc_seq_update_timing(tegra);
+ }
+
+ /* Program burst shadow registers */
+
+ for (i = 0; i < ARRAY_SIZE(timing->emc_burst_data); ++i)
+ __raw_writel(timing->emc_burst_data[i],
+ tegra->emc_regs + t124_emc_burst_regs[i]);
+
+ tegra_mc_write_emem_configuration(tegra->mc, timing->rate);
+
+ val = timing->emc_cfg & ~EMC_CFG_POWER_FEATURES_MASK;
+ emc_ccfifo_writel(tegra, val, EMC_CFG);
+
+ /* Program AUTO_CAL_CONFIG */
+
+ if (timing->emc_auto_cal_config2 !=
+ tegra->last_timing.emc_auto_cal_config2)
+ emc_ccfifo_writel(tegra, timing->emc_auto_cal_config2,
+ EMC_AUTO_CAL_CONFIG2);
+ if (timing->emc_auto_cal_config3 !=
+ tegra->last_timing.emc_auto_cal_config3)
+ emc_ccfifo_writel(tegra, timing->emc_auto_cal_config3,
+ EMC_AUTO_CAL_CONFIG3);
+ if (timing->emc_auto_cal_config !=
+ tegra->last_timing.emc_auto_cal_config) {
+ val = timing->emc_auto_cal_config;
+ val &= EMC_AUTO_CAL_CONFIG_AUTO_CAL_START;
+ emc_ccfifo_writel(tegra, val, EMC_AUTO_CAL_CONFIG);
+ }
+
+ /* DDR3: predict MRS long wait count */
+
+ if (tegra->dram_type == DRAM_TYPE_DDR3 && dll_change == DLL_CHANGE_ON) {
+ u32 cnt = 32;
+
+ if (timing->emc_zcal_interval != 0 &&
+ tegra->last_timing.emc_zcal_interval == 0)
+ cnt -= tegra->dram_num * 256;
Hmm... I don't understand this: cnt == 32 at the beginning, now you're
subtracting something that's >= 256 if dram_num > 0. Is that really
intended?
Looking at the downstream kernel (some version of it, at least), I see
that cnt = 512. Not sure why I don't have that.
+
+ val = timing->emc_mrs_wait_cnt
+ & EMC_MRS_WAIT_CNT_SHORT_WAIT_MASK
+ >> EMC_MRS_WAIT_CNT_SHORT_WAIT_SHIFT;
I think >> has higher precedence than &, so you probably want
parentheses here.
Yes.
+ if (cnt < val)
+ cnt = val;
+
+ val = timing->emc_mrs_wait_cnt
+ & ~EMC_MRS_WAIT_CNT_LONG_WAIT_MASK;
+ val |= (cnt << EMC_MRS_WAIT_CNT_LONG_WAIT_SHIFT)
+ & EMC_MRS_WAIT_CNT_LONG_WAIT_MASK;
+
+ writel(val, tegra->emc_regs + EMC_MRS_WAIT_CNT);
+ }
+
+ val = timing->emc_cfg_2;
+ val &= ~EMC_CFG_2_DIS_STP_OB_CLK_DURING_NON_WR;
+ emc_ccfifo_writel(tegra, val, EMC_CFG_2);
+
+ /* DDR3: Turn off DLL and enter self-refresh */
+
+ if (tegra->dram_type == DRAM_TYPE_DDR3 && dll_change == DLL_CHANGE_OFF)
+ emc_ccfifo_writel(tegra, timing->emc_mode_1, EMC_EMRS);
+
+ /* Disable refresh controller */
+
+ emc_ccfifo_writel(tegra, EMC_REFCTRL_DEV_SEL(tegra->dram_num),
+ EMC_REFCTRL);
+ if (tegra->dram_type == DRAM_TYPE_DDR3)
+ emc_ccfifo_writel(tegra,
+ EMC_DRAM_DEV_SEL(tegra->dram_num)
+ | EMC_SELF_REF_CMD_ENABLED,
+ EMC_SELF_REF);
+
+ /* Flow control marker */
+
+ emc_ccfifo_writel(tegra, 1, EMC_STALL_THEN_EXE_AFTER_CLKCHANGE);
+
+ /* DDR3: Exit self-refresh */
+
+ if (tegra->dram_type == DRAM_TYPE_DDR3)
+ emc_ccfifo_writel(tegra,
+ EMC_DRAM_DEV_SEL(tegra->dram_num),
+ EMC_SELF_REF);
+ emc_ccfifo_writel(tegra,
+ EMC_REFCTRL_DEV_SEL(tegra->dram_num)
+ | EMC_REFCTRL_ENABLE,
+ EMC_REFCTRL);
+
+ /* Set DRAM mode registers */
+
+ if (tegra->dram_type == DRAM_TYPE_DDR3) {
+ if (timing->emc_mode_1 != tegra->last_timing.emc_mode_1)
+ emc_ccfifo_writel(tegra, timing->emc_mode_1, EMC_EMRS);
+ if (timing->emc_mode_2 != tegra->last_timing.emc_mode_2)
+ emc_ccfifo_writel(tegra, timing->emc_mode_2, EMC_EMRS2);
These patterns repeat a lot, perhaps some helpers would be useful.
Something like:
#define emc_ccfifo_writel_if_changed(emc, old, new, field, offset) \
if ((new)->field != (old)->field) \
emc_ccfifo_writel(emc, (new)->field, offset);
...
emc_ccfifo_writel_if_changed(tegra, last, timing,
emc_mode_1, EMC_EMRS);
That's not as much of an improvement as I had hoped... so feel free to
ignore.
+static int load_timings_from_dt(struct tegra_emc *tegra,
+ struct device_node *node)
+{
+ struct device_node *child;
+ int child_count = of_get_child_count(node);
+ int i = 0, err;
unsigned int for i.
+
+ tegra->timings = devm_kzalloc(&tegra->pdev->dev,
+ sizeof(struct emc_timing) * child_count,
+ GFP_KERNEL);
devm_kcalloc()/
+ if (!tegra->timings)
+ return -ENOMEM;
+
+ tegra->num_timings = child_count;
+
+ for_each_child_of_node(node, child) {
+ struct emc_timing *timing = tegra->timings + (i++);
timing = &tegra->timings[i++];?
+
+ err = load_one_timing_from_dt(tegra, timing, child);
+ if (err)
+ return err;
+ }
+
+ sort(tegra->timings, tegra->num_timings, sizeof(struct emc_timing),
+ cmp_timings, NULL);
sizeof(*timing)?
+
+ return 0;
+}
+
+static const struct of_device_id tegra_emc_of_match[] = {
+ { .compatible = "nvidia,tegra124-emc" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, tegra_emc_of_match);
+
+static struct tegra_emc *emc_instance;
Can we find a way around this global variable? I suppose that somebody
is going to call the tegra_emc_{prepare,complete}_timing_change() below,
so perhaps the caller needs to get a reference to the EMC and pass that
in?
+static struct emc_timing *tegra_emc_find_timing(unsigned long rate)
+{
+ int i;
unsigned
+
+ if (!emc_instance)
+ return NULL;
+
+ for (i = 0; i < emc_instance->num_timings; ++i) {
+ if (emc_instance->timings[i].rate == rate)
+ break;
+ }
+
+ if (i == emc_instance->num_timings) {
+ dev_err(&emc_instance->pdev->dev, "no timing for rate %lu\n", rate);
+ return NULL;
+ }
+
+ return emc_instance->timings + i;
This could be rewritten in a similar way than I suggested for the MC
changes earlier.
+}
+
+int tegra_emc_prepare_timing_change(unsigned long rate)
+{
+ struct emc_timing *timing = tegra_emc_find_timing(rate);
+
+ if (!timing)
+ return -ENOENT;
+
+ emc_prepare_timing_change(emc_instance, timing);
+
+ return 0;
+}
It seems like this really ought to return an error if
emc_prepare_timing_change() fails.
+
+void tegra_emc_complete_timing_change(unsigned long rate)
+{
+ struct emc_timing *timing = tegra_emc_find_timing(rate);
+
+ if (!timing)
+ return;
+
+ emc_complete_timing_change(emc_instance, timing);
+}
+
+static int tegra_emc_probe(struct platform_device *pdev)
+{
+ struct tegra_emc *tegra;
+ struct device_node *node;
+ struct platform_device *mc_pdev;
Perhaps simply "mc"?
+ struct resource *res;
+ u32 ram_code, node_ram_code;
You only need node_ram_code in the loop, so perhaps move it there and
make the name something less cumbersome like "value"?
+ int err;
+
+ tegra = devm_kzalloc(&pdev->dev, sizeof(*tegra), GFP_KERNEL);
+ if (!tegra)
+ return -ENOMEM;
+
+ tegra->pdev = pdev;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ tegra->emc_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(tegra->emc_regs)) {
+ dev_err(&pdev->dev, "failed to map EMC regs\n");
No need for this error message, devm_ioremap_resource() prints one of
you.
+ return PTR_ERR(tegra->emc_regs);
+ }
+
+ node = of_parse_phandle(pdev->dev.of_node,
+ "nvidia,memory-controller", 0);
+ if (!node) {
+ dev_err(&pdev->dev, "could not get memory controller\n");
+ return -ENOENT;
+ }
+
+ mc_pdev = of_find_device_by_node(node);
+ if (!mc_pdev)
+ return -ENOENT;
+
+ tegra->mc = platform_get_drvdata(mc_pdev);
+ if (!tegra->mc)
+ return -ENOENT;
Perhaps this ought to be -EPROBE_DEFER to handle that case?
+
+ of_node_put(node);
I think that should go right after the call to of_find_device_by_node(),
otherwise it'll leak in case of errors earlier.
+
+ ram_code = tegra_read_ram_code();
+
+ tegra->num_timings = 0;
+
+ for_each_child_of_node(pdev->dev.of_node, node) {
+ err = of_property_read_u32(node, "nvidia,ram-code", &node_ram_code);
+ if (err || (node_ram_code != ram_code))
+ continue;
+
+ err = load_timings_from_dt(tegra, node);
+ if (err)
+ return err;
+ break;
+ }
+
+ if (tegra->num_timings == 0)
+ dev_warn(&pdev->dev, "no memory timings for ram code %u registered\n", ram_code);
Isn't this an error? What's the use of proceeding if this happens?
True. This is leftover from when I split the clock driver out of this
one. The clock driver can be used readonly without timings.
This is slightly more complicated than I think it should be. Perhaps
split this into a helper function that finds a node matching the RAM
code, then:
node = tegra_emc_find_node_by_ram_code(pdev->dev.of_node, ram_code);
if (!node)
return -ENOENT;
err = load_timings_from_dt(tegra, node);
if (err)
return err;
Also I think you need to of_node_put() the node when you're done with
it.
+
+ err = emc_init(tegra);
+ if (err) {
+ dev_err(&pdev->dev, "initialization failed: %d\n", err);
+ return err;
+ }
+
+ platform_set_drvdata(pdev, tegra);
+
+ emc_instance = tegra;
+
+ return 0;
+};
+
+static struct platform_driver tegra_emc_driver = {
+ .probe = tegra_emc_probe,
+ .driver = {
+ .name = "tegra-emc",
+ .of_match_table = tegra_emc_of_match,
Perhaps you also want to add a ".suppress_bind_attrs = true;" here to
avoid people from causing oopses by unbinding via sysfs.
True.
Thierry
Thanks for reviewing, sorry I couldn't answer all.
I'll leave the rest to Tomeu ;)
Cheers,
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/