Message ID | 1466534502-17233-2-git-send-email-eric@nelint.com |
---|---|
State | RFC |
Delegated to: | Stefano Babic |
Headers | show |
On 06/21/2016 08:41 PM, Eric Nelson wrote: > Allow the calibration data from mmdc_do_write_level_calibration > and mmdc_do_dqs_calibration to be returned to the caller for > display. > > Signed-off-by: Eric Nelson <eric@nelint.com> Why don't you create a separate function to read those params ? > --- > arch/arm/cpu/armv7/mx6/ddr.c | 29 +++++++++++++++++++++++------ > arch/arm/include/asm/arch-mx6/mx6-ddr.h | 4 ++-- > 2 files changed, 25 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c > index 1e7ae28..bde6fe3 100644 > --- a/arch/arm/cpu/armv7/mx6/ddr.c > +++ b/arch/arm/cpu/armv7/mx6/ddr.c > @@ -86,7 +86,7 @@ static void modify_dg_result(u32 *reg_st0, u32 *reg_st1, u32 *reg_ctrl) > writel(val_ctrl, reg_ctrl); > } > > -int mmdc_do_write_level_calibration(void) > +int mmdc_do_write_level_calibration(struct mx6_mmdc_calibration *calib) > { > struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR; > struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR; > @@ -195,10 +195,17 @@ int mmdc_do_write_level_calibration(void) > readl(&mmdc1->mpwldectrl1)); > > /* We must force a readback of these values, to get them to stick */ > - readl(&mmdc0->mpwldectrl0); > - readl(&mmdc0->mpwldectrl1); > - readl(&mmdc1->mpwldectrl0); > - readl(&mmdc1->mpwldectrl1); > + if (calib) { > + calib->p0_mpwldectrl0 = readl(&mmdc0->mpwldectrl0); > + calib->p0_mpwldectrl1 = readl(&mmdc0->mpwldectrl1); > + calib->p1_mpwldectrl0 = readl(&mmdc1->mpwldectrl0); > + calib->p1_mpwldectrl1 = readl(&mmdc1->mpwldectrl1); > + } else { > + readl(&mmdc0->mpwldectrl0); > + readl(&mmdc0->mpwldectrl1); > + readl(&mmdc1->mpwldectrl0); > + readl(&mmdc1->mpwldectrl1); > + } > > /* enable DDR logic power down timer: */ > setbits_le32(&mmdc0->mdpdc, 0x00005500); > @@ -212,7 +219,7 @@ int mmdc_do_write_level_calibration(void) > return errors; > } > > -int mmdc_do_dqs_calibration(void) > +int mmdc_do_dqs_calibration(struct mx6_mmdc_calibration *calib) > { > struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR; > struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR; > @@ -548,6 +555,16 @@ int mmdc_do_dqs_calibration(void) > > debug("Final do_dqs_calibration error mask: 0x%x\n", errors); > > + if (calib) { > + calib->p0_mpdgctrl0 = readl(&mmdc0->mpdgctrl0); > + calib->p0_mpdgctrl1 = readl(&mmdc0->mpdgctrl1); > + calib->p1_mpdgctrl0 = readl(&mmdc1->mpdgctrl0); > + calib->p1_mpdgctrl1 = readl(&mmdc1->mpdgctrl1); > + calib->p0_mprddlctl = readl(&mmdc0->mprddlctl); > + calib->p1_mprddlctl = readl(&mmdc1->mprddlctl); > + calib->p0_mpwrdlctl = readl(&mmdc0->mpwrdlctl); > + calib->p1_mpwrdlctl = readl(&mmdc1->mpwrdlctl); > + } > return errors; > } > #endif > diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h b/arch/arm/include/asm/arch-mx6/mx6-ddr.h > index 12c30d2..948862c 100644 > --- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h > +++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h > @@ -457,8 +457,8 @@ void mx6sl_dram_iocfg(unsigned width, > const struct mx6sl_iomux_grp_regs *); > > #if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) || defined(CONFIG_MX6D) > -int mmdc_do_write_level_calibration(void); > -int mmdc_do_dqs_calibration(void); > +int mmdc_do_write_level_calibration(struct mx6_mmdc_calibration *calib); > +int mmdc_do_dqs_calibration(struct mx6_mmdc_calibration *calib); > #endif > > /* configure mx6 mmdc registers */ >
Hi Marek, On 06/22/2016 04:18 PM, Marek Vasut wrote: > On 06/21/2016 08:41 PM, Eric Nelson wrote: >> Allow the calibration data from mmdc_do_write_level_calibration >> and mmdc_do_dqs_calibration to be returned to the caller for >> display. >> >> Signed-off-by: Eric Nelson <eric@nelint.com> > > Why don't you create a separate function to read those params ? > Probably because in my implementation, the calibration routine didn't actually write the registers. It was left to the caller to hand them to mx6_dram_cfg. You're right though. A routine to gather calibration data would be simpler and prevent the "if (calibration)" junk in the two routines. Regards, Eric
On 06/23/2016 01:52 AM, Eric Nelson wrote: > Hi Marek, > > On 06/22/2016 04:18 PM, Marek Vasut wrote: >> On 06/21/2016 08:41 PM, Eric Nelson wrote: >>> Allow the calibration data from mmdc_do_write_level_calibration >>> and mmdc_do_dqs_calibration to be returned to the caller for >>> display. >>> >>> Signed-off-by: Eric Nelson <eric@nelint.com> >> >> Why don't you create a separate function to read those params ? >> > > Probably because in my implementation, the calibration routine > didn't actually write the registers. It was left to the caller > to hand them to mx6_dram_cfg. > > You're right though. A routine to gather calibration data would > be simpler and prevent the "if (calibration)" junk in the two > routines. Try avoiding polluting the code with the if (calib) junk more :)
On 06/22/2016 04:57 PM, Marek Vasut wrote: > On 06/23/2016 01:52 AM, Eric Nelson wrote: >> Hi Marek, >> >> On 06/22/2016 04:18 PM, Marek Vasut wrote: >>> On 06/21/2016 08:41 PM, Eric Nelson wrote: >>>> Allow the calibration data from mmdc_do_write_level_calibration >>>> and mmdc_do_dqs_calibration to be returned to the caller for >>>> display. >>>> >>>> Signed-off-by: Eric Nelson <eric@nelint.com> >>> >>> Why don't you create a separate function to read those params ? >>> >> >> Probably because in my implementation, the calibration routine >> didn't actually write the registers. It was left to the caller >> to hand them to mx6_dram_cfg. >> >> You're right though. A routine to gather calibration data would >> be simpler and prevent the "if (calibration)" junk in the two >> routines. > > Try avoiding polluting the code with the if (calib) junk more :) > Will fix in V2 with the addition of a routine to gather the calibration data. This will also remove the need for patch 2/12, but the remainder could still use some review.
diff --git a/arch/arm/cpu/armv7/mx6/ddr.c b/arch/arm/cpu/armv7/mx6/ddr.c index 1e7ae28..bde6fe3 100644 --- a/arch/arm/cpu/armv7/mx6/ddr.c +++ b/arch/arm/cpu/armv7/mx6/ddr.c @@ -86,7 +86,7 @@ static void modify_dg_result(u32 *reg_st0, u32 *reg_st1, u32 *reg_ctrl) writel(val_ctrl, reg_ctrl); } -int mmdc_do_write_level_calibration(void) +int mmdc_do_write_level_calibration(struct mx6_mmdc_calibration *calib) { struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR; struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR; @@ -195,10 +195,17 @@ int mmdc_do_write_level_calibration(void) readl(&mmdc1->mpwldectrl1)); /* We must force a readback of these values, to get them to stick */ - readl(&mmdc0->mpwldectrl0); - readl(&mmdc0->mpwldectrl1); - readl(&mmdc1->mpwldectrl0); - readl(&mmdc1->mpwldectrl1); + if (calib) { + calib->p0_mpwldectrl0 = readl(&mmdc0->mpwldectrl0); + calib->p0_mpwldectrl1 = readl(&mmdc0->mpwldectrl1); + calib->p1_mpwldectrl0 = readl(&mmdc1->mpwldectrl0); + calib->p1_mpwldectrl1 = readl(&mmdc1->mpwldectrl1); + } else { + readl(&mmdc0->mpwldectrl0); + readl(&mmdc0->mpwldectrl1); + readl(&mmdc1->mpwldectrl0); + readl(&mmdc1->mpwldectrl1); + } /* enable DDR logic power down timer: */ setbits_le32(&mmdc0->mdpdc, 0x00005500); @@ -212,7 +219,7 @@ int mmdc_do_write_level_calibration(void) return errors; } -int mmdc_do_dqs_calibration(void) +int mmdc_do_dqs_calibration(struct mx6_mmdc_calibration *calib) { struct mmdc_p_regs *mmdc0 = (struct mmdc_p_regs *)MMDC_P0_BASE_ADDR; struct mmdc_p_regs *mmdc1 = (struct mmdc_p_regs *)MMDC_P1_BASE_ADDR; @@ -548,6 +555,16 @@ int mmdc_do_dqs_calibration(void) debug("Final do_dqs_calibration error mask: 0x%x\n", errors); + if (calib) { + calib->p0_mpdgctrl0 = readl(&mmdc0->mpdgctrl0); + calib->p0_mpdgctrl1 = readl(&mmdc0->mpdgctrl1); + calib->p1_mpdgctrl0 = readl(&mmdc1->mpdgctrl0); + calib->p1_mpdgctrl1 = readl(&mmdc1->mpdgctrl1); + calib->p0_mprddlctl = readl(&mmdc0->mprddlctl); + calib->p1_mprddlctl = readl(&mmdc1->mprddlctl); + calib->p0_mpwrdlctl = readl(&mmdc0->mpwrdlctl); + calib->p1_mpwrdlctl = readl(&mmdc1->mpwrdlctl); + } return errors; } #endif diff --git a/arch/arm/include/asm/arch-mx6/mx6-ddr.h b/arch/arm/include/asm/arch-mx6/mx6-ddr.h index 12c30d2..948862c 100644 --- a/arch/arm/include/asm/arch-mx6/mx6-ddr.h +++ b/arch/arm/include/asm/arch-mx6/mx6-ddr.h @@ -457,8 +457,8 @@ void mx6sl_dram_iocfg(unsigned width, const struct mx6sl_iomux_grp_regs *); #if defined(CONFIG_MX6QDL) || defined(CONFIG_MX6Q) || defined(CONFIG_MX6D) -int mmdc_do_write_level_calibration(void); -int mmdc_do_dqs_calibration(void); +int mmdc_do_write_level_calibration(struct mx6_mmdc_calibration *calib); +int mmdc_do_dqs_calibration(struct mx6_mmdc_calibration *calib); #endif /* configure mx6 mmdc registers */
Allow the calibration data from mmdc_do_write_level_calibration and mmdc_do_dqs_calibration to be returned to the caller for display. Signed-off-by: Eric Nelson <eric@nelint.com> --- arch/arm/cpu/armv7/mx6/ddr.c | 29 +++++++++++++++++++++++------ arch/arm/include/asm/arch-mx6/mx6-ddr.h | 4 ++-- 2 files changed, 25 insertions(+), 8 deletions(-)