Message-ID: <20141112154549.GF26488@ulmo>
Date:	Wed, 12 Nov 2014 16:45:50 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Tomeu Vizoso <tomeu.vizoso@...labora.com>
Cc:	linux-tegra@...r.kernel.org,
	Javier Martinez Canillas <javier.martinez@...labora.co.uk>,
	mikko.perttunen@...si.fi, acourbot@...dia.com,
	Mikko Perttunen <mperttunen@...dia.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>, linux-kernel@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v4 10/13] memory: tegra: Add EMC (external memory
 controller) driver

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?

> +	 */
> +	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?

> +	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?

> +
> +	dev_err(&tegra->pdev->dev, "timing update failed\n");
> +}

Should this return an error that can be propagated?

> +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?

> +
> +		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.

> +		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?

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.

Thierry

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists