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

2014-11-13 09:34:09

by Tomeu Vizoso

[permalink] [raw]

Subject: Re: [PATCH v4 05/13] of: Document timings subnode of nvidia,tegra-mc

On 12 November 2014 15:22, Thierry Reding <[email protected]> wrote:
> On Wed, Nov 12, 2014 at 08:56:28AM +0100, Tomeu Vizoso wrote:
> [...]
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt b/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra-mc.txt
> [...]
>> +Example board file:
>> +
>> +/ {
>> + memory-controller@0,70019000 {
>> + emc-timings-3 {
>> + nvidia,ram-code = <3>;
>
> I guess the alternative would be to keep the old names and simply use
> the reg property instead of nvidia,ram-code to be compliant with best
> practices in DT:
>
> emc-timings@3 {
> reg = <3>;
>
>> +
>> + timing-12750000 {
>> + clock-frequency = <12750000>;
>
> And:
>
> timing@12750000 {
> reg = <12750000>;
>
> But I haven't been following this series too closely so far, so maybe
> that had already been rejected.

It has been mentioned already, but I think nobody has shown any strong
opinions on it. I personally like how it is now as it matches how it's
already done for display-timings.

Regards,

Tomeu

> Thierry

2014-11-13 09:36:29

by Tomeu Vizoso

[permalink] [raw]

Subject: Re: [PATCH v4 04/13] of: document new emc-timings subnode in nvidia,tegra124-car

On 12 November 2014 15:18, Thierry Reding <[email protected]> wrote:
> On Wed, Nov 12, 2014 at 08:56:27AM +0100, Tomeu Vizoso wrote:
> [...]
>> Example SoC include file:
>>
>> / {
>> - tegra_car: clock {
>> + tegra_car: clock@60006000 {
>> compatible = "nvidia,tegra124-car";
>> reg = <0x60006000 0x1000>;
>> #clock-cells = <1>;
>> @@ -60,4 +83,23 @@ Example board file:
>> &tegra_car {
>> clocks = <&clk_32k> <&osc>;
>> };
>> +
>> + clock@60006000 {
>> + emc-timings@3 {
>
> Shouldn't this be following the same naming scheme as the memory
> controller's subnodes?
>
>> + nvidia,ram-code = <3>;
>> +
>> + timing@12750000 {
>
> And this?

Thanks for pointing these out. My bad.

Regards,

Tomeu

2014-11-13 10:52:09

by Nikolaus Schulz

[permalink] [raw]

Subject: Re: [PATCH v4 11/13] clk: tegra: Add EMC clock driver

On Wed, Nov 12, 2014 at 08:56:34AM +0100, Tomeu Vizoso wrote:
> From: Mikko Perttunen <[email protected]>
>
> 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.
>
> Signed-off-by: Mikko Perttunen <[email protected]>
> Signed-off-by: Tomeu Vizoso <[email protected]>
[...]
> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> new file mode 100644
> index 0000000..75beacd
> --- /dev/null
> +++ b/drivers/clk/tegra/clk-emc.c
[...]
> +/*
> + * Rounds up unless no higher rate exists, in which case down. This way is
> + * safer since things have EMC rate floors. Also don't touch parent_rate
> + * since we don't want the CCF to play with our parent clocks.
> + */
> +static long emc_round_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long *parent_rate)
> +{
> + struct tegra_emc *tegra = container_of(hw, struct tegra_emc, hw);
> + u8 ram_code = tegra_read_ram_code();
> + struct emc_timing *timing;
> + int i;
> +
> + /*
> + * Returning the original rate will lead to a more sensible error
> + * message when emc_set_rate fails.
> + */
> + if (tegra->num_timings == 0)
> + return rate;
> +
> + for (i = 0; i < tegra->num_timings; ++i) {
> + timing = tegra->timings + i;
> + if (timing->ram_code != ram_code)
> + continue;

Is timing->ram_code != ram_code really something that is allowed?

> +
> + if (timing->rate >= rate)
> + return timing->rate;
> + }
> +
> + return tegra->timings[tegra->num_timings - 1].rate;

If tegra->timings has timings for different ram_codes, the last timing
might not be for the requested ram_code.

> +}

2014-11-14 16:18:26

by Tomeu Vizoso

[permalink] [raw]

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

On 12 November 2014 16:45, Thierry Reding <[email protected]> wrote:
> On Wed, Nov 12, 2014 at 08:56:33AM +0100, Tomeu Vizoso wrote:

<snip>

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

I have removed the union and added properties to the DT for those
three registers.

>> +
>> + struct emc_timing last_timing;
>
> struct emc_timing is rather big, can this be a pointer to an entry in
> the timings array instead?

I'm leaving this as-is following Mikko's explanation.

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

Have done some digging and my understanding is that the vendor
recommends polling for at least 1 millisecond.

https://groups.google.com/a/chromium.org/forum/#!msg/chromium-os-reviews/ZtYLn04i53M/98VKBEh2Xd4J

The only thing I have in the TRM is this:

"Programming of this register does not trigger the shadow register update event
immediately. To prevent shadow register programming issued after
programming this
register from being latched accidentally, always poll for
TIMING_UPDATE_STALLED==0
after programming this register."

Both ChromeOS and L4T loop 1000 times with a delay of 1us. I will be
updating the patch to match this, and also add a comment.

>> +
>> + dev_err(&tegra->pdev->dev, "timing update failed\n");
>> +}
>
> Should this return an error that can be propagated?

I frankly don't know how this could be handled, as we are in the
middle of the sequence and AFAIK there's no way to back up the changes
done until this point. TTBOMK, the best we can do is to print an error
message that can help with debugging.

Maybe someone with knowledge of the internals can comment?

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

Yeah, seems like it should be s/32/512.

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

Ok, it's a bit problematic because the car driver gets probed very
early but I will see at getting a reference to the emc driver lazily.

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

Currently emc_prepare_timing_change() cannot fail.

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

Yes, now looks better.

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

Ok, done.

Thanks for the great review!

Regards,

Tomeu