Re: [PATCH v4 10/13] memory: tegra: Add EMC (external memory controller) driver

[Mikko Perttunen <mikko.perttunen@kapsi.fi>]

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