On Mon, Oct 09, 2023 at 07:19:04AM +0900, Chanwoo Choi wrote:
> On 23. 7. 4. 18:32, Sascha Hauer wrote:
> > The currently supported RK3399 has a set of registers per channel, but
> > it has only a single DDRMON_CTRL register. With upcoming RK3588 this
> > will be different, the RK3588 has a DDRMON_CTRL register per channel.
> >
> > Instead of expecting a single DDRMON_CTRL register, loop over the
> > channels and write the channel specific DDRMON_CTRL register. Break
> > out early out of the loop when there is only a single DDRMON_CTRL
> > register like on the RK3399.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> > Reviewed-by: Sebastian Reichel <sebastian.reichel@xxxxxxxxxxxxx>
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> > drivers/devfreq/event/rockchip-dfi.c | 72 ++++++++++++++++++----------
> > 1 file changed, 48 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/devfreq/event/rockchip-dfi.c b/drivers/devfreq/event/rockchip-dfi.c
> > index 85ec93fd41858..2362d3953ba40 100644
> > --- a/drivers/devfreq/event/rockchip-dfi.c
> > +++ b/drivers/devfreq/event/rockchip-dfi.c
> > @@ -113,12 +113,13 @@ struct rockchip_dfi {
> > int burst_len;
> > int buswidth[DMC_MAX_CHANNELS];
> > int ddrmon_stride;
> > + bool ddrmon_ctrl_single;
> > };
> >
> > static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> > {
> > void __iomem *dfi_regs = dfi->regs;
> > - int ret = 0;
> > + int i, ret = 0;
> >
> > mutex_lock(&dfi->mutex);
> >
> > @@ -132,29 +133,41 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> > goto out;
> > }
> >
> > - /* clear DDRMON_CTRL setting */
> > - writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN | DDRMON_CTRL_SOFTWARE_EN |
> > - DDRMON_CTRL_HARDWARE_EN), dfi_regs + DDRMON_CTRL);
> > + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > + u32 ctrl = 0;
> >
> > - /* set ddr type to dfi */
> > - switch (dfi->ddr_type) {
> > - case ROCKCHIP_DDRTYPE_LPDDR2:
> > - case ROCKCHIP_DDRTYPE_LPDDR3:
> > - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR23, DDRMON_CTRL_DDR_TYPE_MASK),
> > - dfi_regs + DDRMON_CTRL);
> > - break;
> > - case ROCKCHIP_DDRTYPE_LPDDR4:
> > - case ROCKCHIP_DDRTYPE_LPDDR4X:
> > - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_LPDDR4, DDRMON_CTRL_DDR_TYPE_MASK),
> > - dfi_regs + DDRMON_CTRL);
> > - break;
> > - default:
> > - break;
> > - }
> > + if (!(dfi->channel_mask & BIT(i)))
> > + continue;
> >
> > - /* enable count, use software mode */
> > - writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> > - dfi_regs + DDRMON_CTRL);
> > + /* clear DDRMON_CTRL setting */
> > + writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_TIMER_CNT_EN |
> > + DDRMON_CTRL_SOFTWARE_EN | DDRMON_CTRL_HARDWARE_EN),
> > + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > + /* set ddr type to dfi */
> > + switch (dfi->ddr_type) {
> > + case ROCKCHIP_DDRTYPE_LPDDR2:
> > + case ROCKCHIP_DDRTYPE_LPDDR3:
> > + ctrl = DDRMON_CTRL_LPDDR23;
> > + break;
> > + case ROCKCHIP_DDRTYPE_LPDDR4:
> > + case ROCKCHIP_DDRTYPE_LPDDR4X:
> > + ctrl = DDRMON_CTRL_LPDDR4;
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + writel_relaxed(HIWORD_UPDATE(ctrl, DDRMON_CTRL_DDR_TYPE_MASK),
> > + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > + /* enable count, use software mode */
> > + writel_relaxed(HIWORD_UPDATE(DDRMON_CTRL_SOFTWARE_EN, DDRMON_CTRL_SOFTWARE_EN),
> > + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > + if (dfi->ddrmon_ctrl_single)
> > + break;
> > + }
> > out:
> > mutex_unlock(&dfi->mutex);
> >
> > @@ -164,6 +177,7 @@ static int rockchip_dfi_enable(struct rockchip_dfi *dfi)
> > static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> > {
> > void __iomem *dfi_regs = dfi->regs;
> > + int i;
> >
> > mutex_lock(&dfi->mutex);
> >
> > @@ -174,8 +188,17 @@ static void rockchip_dfi_disable(struct rockchip_dfi *dfi)
> > if (dfi->usecount > 0)
> > goto out;
> >
> > - writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> > - dfi_regs + DDRMON_CTRL);
> > + for (i = 0; i < DMC_MAX_CHANNELS; i++) {
> > + if (!(dfi->channel_mask & BIT(i)))
> > + continue;
> > +
> > + writel_relaxed(HIWORD_UPDATE(0, DDRMON_CTRL_SOFTWARE_EN),
> > + dfi_regs + i * dfi->ddrmon_stride + DDRMON_CTRL);
> > +
> > + if (dfi->ddrmon_ctrl_single)
> > + break;
> > + }
> > +
> > clk_disable_unprepare(dfi->clk);
> > out:
> > mutex_unlock(&dfi->mutex);
> > @@ -666,6 +689,7 @@ static int rk3399_dfi_init(struct rockchip_dfi *dfi)
> > dfi->buswidth[1] = FIELD_GET(RK3399_PMUGRF_OS_REG2_BW_CH1, val) == 0 ? 4 : 2;
> >
> > dfi->ddrmon_stride = 0x14;
> > + dfi->ddrmon_ctrl_single = true;
> >
> > return 0;
> > };
>
> Even if rk3568 has the only one channle and don't need to check whether 'dfi->ddrmon_ctrl_single'
> is true or not because of 'if (!(dfi->channel_mask & BIT(i)))',
> I recommand the add 'dfi->ddrmon_ctrl_single = true;' for rk3568 in order to
> provide the number of DDRMON_CTRL reigster of rk3568.
>
> If rk3568 doesn't have the 'ddrmon_ctrl_single', actually it is not easy
> to catch what why are there no initilization for rk3568.
Ok, will change.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
