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