| Message ID | 1467364768-21741-1-git-send-email-ykk@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote: > The full name of PSR is Panel Self Refresh, panel device could refresh > itself with the hardware framebuffer in panel, this would make lots of > sense to save the power consumption. > > This patch have exported two symbols for platform driver to implement > the PSR function in hardware side: > - analogix_dp_active_psr() > - analogix_dp_inactive_psr() > > Signed-off-by: Yakir Yang <ykk@rock-chips.com> > --- > Changes in v3: > - split analogix_dp_enable_psr(), make it more clearly > analogix_dp_detect_sink_psr() > analogix_dp_enable_sink_psr() > - remove some nosie register setting comments > > Changes in v2: > - introduce in v2, splite the common Analogix DP changes out > > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++++++++++++++++++++++ > drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ > drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 ++++++++++++++++++ > drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ > include/drm/bridge/analogix_dp.h | 3 + > 5 files changed, 153 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index 32715da..b557097 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp) > return 0; > } > > +int analogix_dp_active_psr(struct device *dev) > +{ > + struct analogix_dp_device *dp = dev_get_drvdata(dev); > + > + if (!dp->psr_support) > + return -EINVAL; > + > + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | > + EDP_VSC_PSR_CRC_VALUES_VALID); > + return 0; > +} > +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); > + > +int analogix_dp_inactive_psr(struct device *dev) > +{ > + struct analogix_dp_device *dp = dev_get_drvdata(dev); > + > + if (!dp->psr_support) > + return -EINVAL; > + > + analogix_dp_send_psr_spd(dp, 0); > + return 0; > +} > +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); > + > +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) > +{ > + unsigned char psr_version; > + > + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, &psr_version); > + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); > + This info message is likely to be spammy since it's printed everytime the panel toggle on. Perhaps downgrade to debug level. > + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; > +} > + > +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) Return type is int, but the function never fails and you don't check the return value when calling it. Seems like this should be void. > +{ > + unsigned char psr_en; > + > + /* Disable psr function */ > + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); > + psr_en &= ~DP_PSR_ENABLE; > + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); > + > + /* Main-Link transmitter remains active during PSR active states */ > + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); > + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here. > + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); > + > + /* Enable psr function */ > + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); > + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | > + DP_PSR_CRC_VERIFICATION; Again, no need to read if you're just overwriting. > + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); > + > + analogix_dp_enable_psr_crc(dp); > + > + return 0; > +} > + > static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data) > { > int i; > @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) > > /* Enable video */ > analogix_dp_start_video(dp); > + > + dp->psr_support = analogix_dp_detect_sink_psr(dp); > + if (dp->psr_support) > + analogix_dp_enable_sink_psr(dp); > } > > int analogix_dp_get_modes(struct drm_connector *connector) > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > index b456380..6ca5dde 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h > @@ -177,6 +177,7 @@ struct analogix_dp_device { > int hpd_gpio; > bool force_hpd; > unsigned char edid[EDID_BLOCK_LENGTH * 2]; > + bool psr_support; > > struct analogix_dp_plat_data *plat_data; > }; > @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct analogix_dp_device *dp); > void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp); > void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); > void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); > +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); > +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); > + > #endif /* _ANALOGIX_DP_CORE_H */ > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > index 48030f0..e8372c7 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c > @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct analogix_dp_device *dp) > reg |= SCRAMBLING_DISABLE; > writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); > } > + > +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) > +{ > + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, > + dp->reg_base + ANALOGIX_DP_CRC_CON); > + > + usleep_range(10, 20); Is this sleep arbitrary, or documented somewhere? Could you add a comment explaining how this was arrived at? > + > + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); > +} > + > +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) > +{ > + unsigned int val; > + > + /* don't send info frame */ > + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > + val &= ~IF_EN; > + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > + > + /* configure single frame update mode */ > + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); > + > + /* configure VSC HB0 ~ HB3 */ > + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); > + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); > + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); > + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); > + > + /* configure VSC HB0 ~ HB3 */ > + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); > + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); > + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); > + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); > + > + /* configure DB0 / DB1 values */ > + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); Lots of hardcoded values here. I think this could be cleaned up. > + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); > + > + /* set reuse spd inforframe */ > + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); > + val |= REUSE_SPD_EN; > + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); > + > + /* mark info frame update */ > + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > + val = (val | IF_UP) & ~IF_EN; > + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > + > + /* send info frame */ > + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > + val |= IF_EN; > + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); > +} > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h > index cdcc6c5..a2698e4 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h > @@ -22,6 +22,8 @@ > #define ANALOGIX_DP_VIDEO_CTL_8 0x3C > #define ANALOGIX_DP_VIDEO_CTL_10 0x44 > > +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 > + > #define ANALOGIX_DP_PLL_REG_1 0xfc > #define ANALOGIX_DP_PLL_REG_2 0x9e4 > #define ANALOGIX_DP_PLL_REG_3 0x9e8 > @@ -30,6 +32,21 @@ > > #define ANALOGIX_DP_PD 0x12c > > +#define ANALOGIX_DP_IF_TYPE 0x244 > +#define ANALOGIX_DP_IF_PKT_DB1 0x254 > +#define ANALOGIX_DP_IF_PKT_DB2 0x258 > +#define ANALOGIX_DP_SPD_HB0 0x2F8 > +#define ANALOGIX_DP_SPD_HB1 0x2FC > +#define ANALOGIX_DP_SPD_HB2 0x300 > +#define ANALOGIX_DP_SPD_HB3 0x304 > +#define ANALOGIX_DP_SPD_PB0 0x308 > +#define ANALOGIX_DP_SPD_PB1 0x30C > +#define ANALOGIX_DP_SPD_PB2 0x310 > +#define ANALOGIX_DP_SPD_PB3 0x314 > +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 > +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C > +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 > + > #define ANALOGIX_DP_LANE_MAP 0x35C > > #define ANALOGIX_DP_ANALOG_CTL_1 0x370 > @@ -103,6 +120,8 @@ > > #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 > > +#define ANALOGIX_DP_CRC_CON 0x890 > + > /* ANALOGIX_DP_TX_SW_RESET */ > #define RESET_DP_TX (0x1 << 0) > > @@ -151,6 +170,7 @@ > #define VID_CHK_UPDATE_TYPE_SHIFT (4) > #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) > #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) > +#define REUSE_SPD_EN (0x1 << 3) > > /* ANALOGIX_DP_VIDEO_CTL_8 */ > #define VID_HRES_TH(x) (((x) & 0xf) << 4) > @@ -376,4 +396,12 @@ > #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) > #define VIDEO_MODE_MASTER_MODE (0x0 << 0) > > +/* ANALOGIX_DP_PKT_SEND_CTL */ > +#define IF_UP (0x1 << 4) > +#define IF_EN (0x1 << 0) > + > +/* ANALOGIX_DP_CRC_CON */ > +#define PSR_VID_CRC_FLUSH (0x1 << 2) > +#define PSR_VID_CRC_ENABLE (0x1 << 0) > + > #endif /* _ANALOGIX_DP_REG_H */ > diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h > index 261b86d..183a336 100644 > --- a/include/drm/bridge/analogix_dp.h > +++ b/include/drm/bridge/analogix_dp.h > @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { > struct drm_connector *); > }; > > +int analogix_dp_active_psr(struct device *dev); > +int analogix_dp_inactive_psr(struct device *dev); Why active/inactive instead of enable/disable, which is used everywhere else? > + > int analogix_dp_resume(struct device *dev); > int analogix_dp_suspend(struct device *dev); > > -- > 1.9.1 > >
Sean, Thanks for your review. On 07/02/2016 03:46 AM, Sean Paul wrote: > On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote: >> The full name of PSR is Panel Self Refresh, panel device could refresh >> itself with the hardware framebuffer in panel, this would make lots of >> sense to save the power consumption. >> >> This patch have exported two symbols for platform driver to implement >> the PSR function in hardware side: >> - analogix_dp_active_psr() >> - analogix_dp_inactive_psr() >> >> Signed-off-by: Yakir Yang <ykk@rock-chips.com> >> --- >> Changes in v3: >> - split analogix_dp_enable_psr(), make it more clearly >> analogix_dp_detect_sink_psr() >> analogix_dp_enable_sink_psr() >> - remove some nosie register setting comments >> >> Changes in v2: >> - introduce in v2, splite the common Analogix DP changes out >> >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++++++++++++++++++++++ >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ >> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 ++++++++++++++++++ >> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ >> include/drm/bridge/analogix_dp.h | 3 + >> 5 files changed, 153 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> index 32715da..b557097 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp) >> return 0; >> } >> >> +int analogix_dp_active_psr(struct device *dev) >> +{ >> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >> + >> + if (!dp->psr_support) >> + return -EINVAL; >> + >> + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | >> + EDP_VSC_PSR_CRC_VALUES_VALID); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); >> + >> +int analogix_dp_inactive_psr(struct device *dev) >> +{ >> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >> + >> + if (!dp->psr_support) >> + return -EINVAL; >> + >> + analogix_dp_send_psr_spd(dp, 0); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); >> + >> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) >> +{ >> + unsigned char psr_version; >> + >> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, &psr_version); >> + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); >> + > This info message is likely to be spammy since it's printed everytime > the panel toggle on. Perhaps downgrade to debug level. Okay, done. >> + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; >> +} >> + >> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) > Return type is int, but the function never fails and you don't check > the return value when calling it. Seems like this should be void. Done. >> +{ >> + unsigned char psr_en; >> + >> + /* Disable psr function */ >> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >> + psr_en &= ~DP_PSR_ENABLE; >> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >> + >> + /* Main-Link transmitter remains active during PSR active states */ >> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >> + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; > Why read psr_en if you're just going to overwrite it? Perhaps you meant |= here. > Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it directly is enough. >> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >> + >> + /* Enable psr function */ >> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >> + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | >> + DP_PSR_CRC_VERIFICATION; > Again, no need to read if you're just overwriting. Yes, ditto >> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >> + >> + analogix_dp_enable_psr_crc(dp); >> + >> + return 0; >> +} >> + >> static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data) >> { >> int i; >> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) >> >> /* Enable video */ >> analogix_dp_start_video(dp); >> + >> + dp->psr_support = analogix_dp_detect_sink_psr(dp); >> + if (dp->psr_support) >> + analogix_dp_enable_sink_psr(dp); >> } >> >> int analogix_dp_get_modes(struct drm_connector *connector) >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> index b456380..6ca5dde 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >> @@ -177,6 +177,7 @@ struct analogix_dp_device { >> int hpd_gpio; >> bool force_hpd; >> unsigned char edid[EDID_BLOCK_LENGTH * 2]; >> + bool psr_support; >> >> struct analogix_dp_plat_data *plat_data; >> }; >> @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct analogix_dp_device *dp); >> void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp); >> void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); >> void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); >> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); >> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); >> + >> #endif /* _ANALOGIX_DP_CORE_H */ >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> index 48030f0..e8372c7 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >> @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct analogix_dp_device *dp) >> reg |= SCRAMBLING_DISABLE; >> writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); >> } >> + >> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) >> +{ >> + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, >> + dp->reg_base + ANALOGIX_DP_CRC_CON); >> + >> + usleep_range(10, 20); > Is this sleep arbitrary, or documented somewhere? Could you add a > comment explaining how this was arrived at? > Yes, this sleep is arbitrary, there is no document about the >> + >> + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); >> +} >> + >> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) >> +{ >> + unsigned int val; >> + >> + /* don't send info frame */ >> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> + val &= ~IF_EN; >> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> + >> + /* configure single frame update mode */ >> + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); >> + >> + /* configure VSC HB0 ~ HB3 */ >> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); >> + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); >> + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); >> + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); >> + >> + /* configure VSC HB0 ~ HB3 */ >> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); >> + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); >> + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); >> + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); >> + >> + /* configure DB0 / DB1 values */ >> + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); > Lots of hardcoded values here. I think this could be cleaned up. For "HB0~HB3", "PB0~PB3" and "DB1", I don't understand very well. Those seems to be a kind of head number, I got those magic values from our IC side. So I think those should be okay to keep the hardcode values here :-D But for the "0x3" in "ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL", I would fix it with suitable macro. >> + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); >> + >> + /* set reuse spd inforframe */ >> + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >> + val |= REUSE_SPD_EN; >> + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >> + >> + /* mark info frame update */ >> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> + val = (val | IF_UP) & ~IF_EN; >> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> + >> + /* send info frame */ >> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> + val |= IF_EN; >> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >> +} >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >> index cdcc6c5..a2698e4 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >> @@ -22,6 +22,8 @@ >> #define ANALOGIX_DP_VIDEO_CTL_8 0x3C >> #define ANALOGIX_DP_VIDEO_CTL_10 0x44 >> >> +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 >> + >> #define ANALOGIX_DP_PLL_REG_1 0xfc >> #define ANALOGIX_DP_PLL_REG_2 0x9e4 >> #define ANALOGIX_DP_PLL_REG_3 0x9e8 >> @@ -30,6 +32,21 @@ >> >> #define ANALOGIX_DP_PD 0x12c >> >> +#define ANALOGIX_DP_IF_TYPE 0x244 >> +#define ANALOGIX_DP_IF_PKT_DB1 0x254 >> +#define ANALOGIX_DP_IF_PKT_DB2 0x258 >> +#define ANALOGIX_DP_SPD_HB0 0x2F8 >> +#define ANALOGIX_DP_SPD_HB1 0x2FC >> +#define ANALOGIX_DP_SPD_HB2 0x300 >> +#define ANALOGIX_DP_SPD_HB3 0x304 >> +#define ANALOGIX_DP_SPD_PB0 0x308 >> +#define ANALOGIX_DP_SPD_PB1 0x30C >> +#define ANALOGIX_DP_SPD_PB2 0x310 >> +#define ANALOGIX_DP_SPD_PB3 0x314 >> +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 >> +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C >> +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 >> + >> #define ANALOGIX_DP_LANE_MAP 0x35C >> >> #define ANALOGIX_DP_ANALOG_CTL_1 0x370 >> @@ -103,6 +120,8 @@ >> >> #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 >> >> +#define ANALOGIX_DP_CRC_CON 0x890 >> + >> /* ANALOGIX_DP_TX_SW_RESET */ >> #define RESET_DP_TX (0x1 << 0) >> >> @@ -151,6 +170,7 @@ >> #define VID_CHK_UPDATE_TYPE_SHIFT (4) >> #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) >> #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) >> +#define REUSE_SPD_EN (0x1 << 3) >> >> /* ANALOGIX_DP_VIDEO_CTL_8 */ >> #define VID_HRES_TH(x) (((x) & 0xf) << 4) >> @@ -376,4 +396,12 @@ >> #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) >> #define VIDEO_MODE_MASTER_MODE (0x0 << 0) >> >> +/* ANALOGIX_DP_PKT_SEND_CTL */ >> +#define IF_UP (0x1 << 4) >> +#define IF_EN (0x1 << 0) >> + >> +/* ANALOGIX_DP_CRC_CON */ >> +#define PSR_VID_CRC_FLUSH (0x1 << 2) >> +#define PSR_VID_CRC_ENABLE (0x1 << 0) >> + >> #endif /* _ANALOGIX_DP_REG_H */ >> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h >> index 261b86d..183a336 100644 >> --- a/include/drm/bridge/analogix_dp.h >> +++ b/include/drm/bridge/analogix_dp.h >> @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { >> struct drm_connector *); >> }; >> >> +int analogix_dp_active_psr(struct device *dev); >> +int analogix_dp_inactive_psr(struct device *dev); > Why active/inactive instead of enable/disable, which is used everywhere else? Done Thanks, - Yakir >> + >> int analogix_dp_resume(struct device *dev); >> int analogix_dp_suspend(struct device *dev); >> >> -- >> 1.9.1 >> >> > >
On 07/08/2016 10:26 AM, Yakir Yang wrote: > Sean, > > Thanks for your review. > > On 07/02/2016 03:46 AM, Sean Paul wrote: >> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote: >>> The full name of PSR is Panel Self Refresh, panel device could refresh >>> itself with the hardware framebuffer in panel, this would make lots of >>> sense to save the power consumption. >>> >>> This patch have exported two symbols for platform driver to implement >>> the PSR function in hardware side: >>> - analogix_dp_active_psr() >>> - analogix_dp_inactive_psr() >>> >>> Signed-off-by: Yakir Yang <ykk@rock-chips.com> >>> --- >>> Changes in v3: >>> - split analogix_dp_enable_psr(), make it more clearly >>> analogix_dp_detect_sink_psr() >>> analogix_dp_enable_sink_psr() >>> - remove some nosie register setting comments >>> >>> Changes in v2: >>> - introduce in v2, splite the common Analogix DP changes out >>> >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 >>> ++++++++++++++++++++++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 >>> ++++++++++++++++++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ >>> include/drm/bridge/analogix_dp.h | 3 + >>> 5 files changed, 153 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> index 32715da..b557097 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct >>> analogix_dp_device *dp) >>> return 0; >>> } >>> >>> +int analogix_dp_active_psr(struct device *dev) >>> +{ >>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>> + >>> + if (!dp->psr_support) >>> + return -EINVAL; >>> + >>> + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | >>> + EDP_VSC_PSR_CRC_VALUES_VALID); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); >>> + >>> +int analogix_dp_inactive_psr(struct device *dev) >>> +{ >>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>> + >>> + if (!dp->psr_support) >>> + return -EINVAL; >>> + >>> + analogix_dp_send_psr_spd(dp, 0); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); >>> + >>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) >>> +{ >>> + unsigned char psr_version; >>> + >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, >>> &psr_version); >>> + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); >>> + >> This info message is likely to be spammy since it's printed everytime >> the panel toggle on. Perhaps downgrade to debug level. > > Okay, done. > >>> + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; >>> +} >>> + >>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) >> Return type is int, but the function never fails and you don't check >> the return value when calling it. Seems like this should be void. > > Done. > >>> +{ >>> + unsigned char psr_en; >>> + >>> + /* Disable psr function */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en &= ~DP_PSR_ENABLE; >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + /* Main-Link transmitter remains active during PSR active >>> states */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; >> Why read psr_en if you're just going to overwrite it? Perhaps you >> meant |= here. >> > > Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just > configure it directly is enough. > >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + /* Enable psr function */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | >>> + DP_PSR_CRC_VERIFICATION; >> Again, no need to read if you're just overwriting. > > Yes, ditto > >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + analogix_dp_enable_psr_crc(dp); >>> + >>> + return 0; >>> +} >>> + >>> static unsigned char analogix_dp_calc_edid_check_sum(unsigned char >>> *edid_data) >>> { >>> int i; >>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct >>> analogix_dp_device *dp) >>> >>> /* Enable video */ >>> analogix_dp_start_video(dp); >>> + >>> + dp->psr_support = analogix_dp_detect_sink_psr(dp); >>> + if (dp->psr_support) >>> + analogix_dp_enable_sink_psr(dp); >>> } >>> >>> int analogix_dp_get_modes(struct drm_connector *connector) >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> index b456380..6ca5dde 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> @@ -177,6 +177,7 @@ struct analogix_dp_device { >>> int hpd_gpio; >>> bool force_hpd; >>> unsigned char edid[EDID_BLOCK_LENGTH * 2]; >>> + bool psr_support; >>> >>> struct analogix_dp_plat_data *plat_data; >>> }; >>> @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct >>> analogix_dp_device *dp); >>> void analogix_dp_config_video_slave_mode(struct analogix_dp_device >>> *dp); >>> void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); >>> void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); >>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); >>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); >>> + >>> #endif /* _ANALOGIX_DP_CORE_H */ >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> index 48030f0..e8372c7 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct >>> analogix_dp_device *dp) >>> reg |= SCRAMBLING_DISABLE; >>> writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); >>> } >>> + >>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) >>> +{ >>> + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, >>> + dp->reg_base + ANALOGIX_DP_CRC_CON); >>> + >>> + usleep_range(10, 20); >> Is this sleep arbitrary, or documented somewhere? Could you add a >> comment explaining how this was arrived at? >> > > Yes, this sleep is arbitrary, there is no document about the Oh, implement reply. I double check the datesheet, oh, i make a mistaken here. The CRC_FLUSH wasn't a trigger, but a enable gate. >PSR Video CRC flush enable. The PSR video CRC value is initialized at every v-sync leading edge. >>> >>> + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); So I need to delete this register setting. Thanks, - Yakir >>> +} >>> + >>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) >>> +{ >>> + unsigned int val; >>> + >>> + /* don't send info frame */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val &= ~IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + >>> + /* configure single frame update mode */ >>> + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); >>> + >>> + /* configure VSC HB0 ~ HB3 */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); >>> + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); >>> + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); >>> + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); >>> + >>> + /* configure VSC HB0 ~ HB3 */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); >>> + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); >>> + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); >>> + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); >>> + >>> + /* configure DB0 / DB1 values */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); >> Lots of hardcoded values here. I think this could be cleaned up. > > For "HB0~HB3", "PB0~PB3" and "DB1", I don't understand very well. > Those seems to be a kind of head number, I got those magic values from > our IC side. So I think those should be okay to keep the hardcode > values here :-D > > But for the "0x3" in "ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL", I would fix > it with suitable macro. > >>> + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); >>> + >>> + /* set reuse spd inforframe */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>> + val |= REUSE_SPD_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>> + >>> + /* mark info frame update */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val = (val | IF_UP) & ~IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + >>> + /* send info frame */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val |= IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> +} >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> index cdcc6c5..a2698e4 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> @@ -22,6 +22,8 @@ >>> #define ANALOGIX_DP_VIDEO_CTL_8 0x3C >>> #define ANALOGIX_DP_VIDEO_CTL_10 0x44 >>> >>> +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 >>> + >>> #define ANALOGIX_DP_PLL_REG_1 0xfc >>> #define ANALOGIX_DP_PLL_REG_2 0x9e4 >>> #define ANALOGIX_DP_PLL_REG_3 0x9e8 >>> @@ -30,6 +32,21 @@ >>> >>> #define ANALOGIX_DP_PD 0x12c >>> >>> +#define ANALOGIX_DP_IF_TYPE 0x244 >>> +#define ANALOGIX_DP_IF_PKT_DB1 0x254 >>> +#define ANALOGIX_DP_IF_PKT_DB2 0x258 >>> +#define ANALOGIX_DP_SPD_HB0 0x2F8 >>> +#define ANALOGIX_DP_SPD_HB1 0x2FC >>> +#define ANALOGIX_DP_SPD_HB2 0x300 >>> +#define ANALOGIX_DP_SPD_HB3 0x304 >>> +#define ANALOGIX_DP_SPD_PB0 0x308 >>> +#define ANALOGIX_DP_SPD_PB1 0x30C >>> +#define ANALOGIX_DP_SPD_PB2 0x310 >>> +#define ANALOGIX_DP_SPD_PB3 0x314 >>> +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 >>> +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C >>> +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 >>> + >>> #define ANALOGIX_DP_LANE_MAP 0x35C >>> >>> #define ANALOGIX_DP_ANALOG_CTL_1 0x370 >>> @@ -103,6 +120,8 @@ >>> >>> #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 >>> >>> +#define ANALOGIX_DP_CRC_CON 0x890 >>> + >>> /* ANALOGIX_DP_TX_SW_RESET */ >>> #define RESET_DP_TX (0x1 << 0) >>> >>> @@ -151,6 +170,7 @@ >>> #define VID_CHK_UPDATE_TYPE_SHIFT (4) >>> #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) >>> #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) >>> +#define REUSE_SPD_EN (0x1 << 3) >>> >>> /* ANALOGIX_DP_VIDEO_CTL_8 */ >>> #define VID_HRES_TH(x) (((x) & 0xf) << 4) >>> @@ -376,4 +396,12 @@ >>> #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) >>> #define VIDEO_MODE_MASTER_MODE (0x0 << 0) >>> >>> +/* ANALOGIX_DP_PKT_SEND_CTL */ >>> +#define IF_UP (0x1 << 4) >>> +#define IF_EN (0x1 << 0) >>> + >>> +/* ANALOGIX_DP_CRC_CON */ >>> +#define PSR_VID_CRC_FLUSH (0x1 << 2) >>> +#define PSR_VID_CRC_ENABLE (0x1 << 0) >>> + >>> #endif /* _ANALOGIX_DP_REG_H */ >>> diff --git a/include/drm/bridge/analogix_dp.h >>> b/include/drm/bridge/analogix_dp.h >>> index 261b86d..183a336 100644 >>> --- a/include/drm/bridge/analogix_dp.h >>> +++ b/include/drm/bridge/analogix_dp.h >>> @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { >>> struct drm_connector *); >>> }; >>> >>> +int analogix_dp_active_psr(struct device *dev); >>> +int analogix_dp_inactive_psr(struct device *dev); >> Why active/inactive instead of enable/disable, which is used >> everywhere else? > > Done > > Thanks, > - Yakir > >>> + >>> int analogix_dp_resume(struct device *dev); >>> int analogix_dp_suspend(struct device *dev); >>> >>> -- >>> 1.9.1 >>> >>> >> >> >
On Thu, Jul 7, 2016 at 7:26 PM, Yakir Yang <ykk@rock-chips.com> wrote: > Sean, > > Thanks for your review. > > > On 07/02/2016 03:46 AM, Sean Paul wrote: >> >> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote: >>> >>> The full name of PSR is Panel Self Refresh, panel device could refresh >>> itself with the hardware framebuffer in panel, this would make lots of >>> sense to save the power consumption. >>> >>> This patch have exported two symbols for platform driver to implement >>> the PSR function in hardware side: >>> - analogix_dp_active_psr() >>> - analogix_dp_inactive_psr() >>> >>> Signed-off-by: Yakir Yang <ykk@rock-chips.com> >>> --- >>> Changes in v3: >>> - split analogix_dp_enable_psr(), make it more clearly >>> analogix_dp_detect_sink_psr() >>> analogix_dp_enable_sink_psr() >>> - remove some nosie register setting comments >>> >>> Changes in v2: >>> - introduce in v2, splite the common Analogix DP changes out >>> >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 >>> ++++++++++++++++++++++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 >>> ++++++++++++++++++ >>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ >>> include/drm/bridge/analogix_dp.h | 3 + >>> 5 files changed, 153 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> index 32715da..b557097 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct >>> analogix_dp_device *dp) >>> return 0; >>> } >>> >>> +int analogix_dp_active_psr(struct device *dev) >>> +{ >>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>> + >>> + if (!dp->psr_support) >>> + return -EINVAL; >>> + >>> + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | >>> + EDP_VSC_PSR_CRC_VALUES_VALID); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); >>> + >>> +int analogix_dp_inactive_psr(struct device *dev) >>> +{ >>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>> + >>> + if (!dp->psr_support) >>> + return -EINVAL; >>> + >>> + analogix_dp_send_psr_spd(dp, 0); >>> + return 0; >>> +} >>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); >>> + >>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) >>> +{ >>> + unsigned char psr_version; >>> + >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, >>> &psr_version); >>> + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); >>> + >> >> This info message is likely to be spammy since it's printed everytime >> the panel toggle on. Perhaps downgrade to debug level. > > > Okay, done. > >>> + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; >>> +} >>> + >>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) >> >> Return type is int, but the function never fails and you don't check >> the return value when calling it. Seems like this should be void. > > > Done. > >>> +{ >>> + unsigned char psr_en; >>> + >>> + /* Disable psr function */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en &= ~DP_PSR_ENABLE; >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + /* Main-Link transmitter remains active during PSR active states >>> */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; >> >> Why read psr_en if you're just going to overwrite it? Perhaps you meant |= >> here. >> > > Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it > directly is enough. > >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + /* Enable psr function */ >>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>> + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | >>> + DP_PSR_CRC_VERIFICATION; >> >> Again, no need to read if you're just overwriting. > > > Yes, ditto > > >>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>> + >>> + analogix_dp_enable_psr_crc(dp); >>> + >>> + return 0; >>> +} >>> + >>> static unsigned char analogix_dp_calc_edid_check_sum(unsigned char >>> *edid_data) >>> { >>> int i; >>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct >>> analogix_dp_device *dp) >>> >>> /* Enable video */ >>> analogix_dp_start_video(dp); >>> + >>> + dp->psr_support = analogix_dp_detect_sink_psr(dp); >>> + if (dp->psr_support) >>> + analogix_dp_enable_sink_psr(dp); >>> } >>> >>> int analogix_dp_get_modes(struct drm_connector *connector) >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> index b456380..6ca5dde 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>> @@ -177,6 +177,7 @@ struct analogix_dp_device { >>> int hpd_gpio; >>> bool force_hpd; >>> unsigned char edid[EDID_BLOCK_LENGTH * 2]; >>> + bool psr_support; >>> >>> struct analogix_dp_plat_data *plat_data; >>> }; >>> @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct >>> analogix_dp_device *dp); >>> void analogix_dp_config_video_slave_mode(struct analogix_dp_device >>> *dp); >>> void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); >>> void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); >>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); >>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); >>> + >>> #endif /* _ANALOGIX_DP_CORE_H */ >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> index 48030f0..e8372c7 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>> @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct >>> analogix_dp_device *dp) >>> reg |= SCRAMBLING_DISABLE; >>> writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); >>> } >>> + >>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) >>> +{ >>> + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, >>> + dp->reg_base + ANALOGIX_DP_CRC_CON); >>> + >>> + usleep_range(10, 20); >> >> Is this sleep arbitrary, or documented somewhere? Could you add a >> comment explaining how this was arrived at? >> > > Yes, this sleep is arbitrary, there is no document about the > Ok, so I assume we actually need the delay for things to work? If not, delete the delay. If we need it, please add a comment saying the wait is required and the value was chosen via trial and error. > >>> + >>> + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); >>> +} >>> + >>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) >>> +{ >>> + unsigned int val; >>> + >>> + /* don't send info frame */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val &= ~IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + >>> + /* configure single frame update mode */ >>> + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); >>> + >>> + /* configure VSC HB0 ~ HB3 */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); >>> + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); >>> + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); >>> + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); >>> + >>> + /* configure VSC HB0 ~ HB3 */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); >>> + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); >>> + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); >>> + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); >>> + >>> + /* configure DB0 / DB1 values */ >>> + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); >> >> Lots of hardcoded values here. I think this could be cleaned up. > > > For "HB0~HB3", "PB0~PB3" and "DB1", I don't understand very well. Those > seems to be a kind of head number, I got those magic values from our IC > side. So I think those should be okay to keep the hardcode values here :-D > Hmm. Well, I'd probably pull them out into some HBX_MAGIC_VALUE define, but I suppose it's fine to keep them as-is. Please at least add a comment above explaining that they're magic values provided by your IC team. > But for the "0x3" in "ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL", I would fix it > with suitable macro. > > >>> + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); >>> + >>> + /* set reuse spd inforframe */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>> + val |= REUSE_SPD_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>> + >>> + /* mark info frame update */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val = (val | IF_UP) & ~IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + >>> + /* send info frame */ >>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> + val |= IF_EN; >>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>> +} >>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> index cdcc6c5..a2698e4 100644 >>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>> @@ -22,6 +22,8 @@ >>> #define ANALOGIX_DP_VIDEO_CTL_8 0x3C >>> #define ANALOGIX_DP_VIDEO_CTL_10 0x44 >>> >>> +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 >>> + >>> #define ANALOGIX_DP_PLL_REG_1 0xfc >>> #define ANALOGIX_DP_PLL_REG_2 0x9e4 >>> #define ANALOGIX_DP_PLL_REG_3 0x9e8 >>> @@ -30,6 +32,21 @@ >>> >>> #define ANALOGIX_DP_PD 0x12c >>> >>> +#define ANALOGIX_DP_IF_TYPE 0x244 >>> +#define ANALOGIX_DP_IF_PKT_DB1 0x254 >>> +#define ANALOGIX_DP_IF_PKT_DB2 0x258 >>> +#define ANALOGIX_DP_SPD_HB0 0x2F8 >>> +#define ANALOGIX_DP_SPD_HB1 0x2FC >>> +#define ANALOGIX_DP_SPD_HB2 0x300 >>> +#define ANALOGIX_DP_SPD_HB3 0x304 >>> +#define ANALOGIX_DP_SPD_PB0 0x308 >>> +#define ANALOGIX_DP_SPD_PB1 0x30C >>> +#define ANALOGIX_DP_SPD_PB2 0x310 >>> +#define ANALOGIX_DP_SPD_PB3 0x314 >>> +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 >>> +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C >>> +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 >>> + >>> #define ANALOGIX_DP_LANE_MAP 0x35C >>> >>> #define ANALOGIX_DP_ANALOG_CTL_1 0x370 >>> @@ -103,6 +120,8 @@ >>> >>> #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 >>> >>> +#define ANALOGIX_DP_CRC_CON 0x890 >>> + >>> /* ANALOGIX_DP_TX_SW_RESET */ >>> #define RESET_DP_TX (0x1 << 0) >>> >>> @@ -151,6 +170,7 @@ >>> #define VID_CHK_UPDATE_TYPE_SHIFT (4) >>> #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) >>> #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) >>> +#define REUSE_SPD_EN (0x1 << 3) >>> >>> /* ANALOGIX_DP_VIDEO_CTL_8 */ >>> #define VID_HRES_TH(x) (((x) & 0xf) << 4) >>> @@ -376,4 +396,12 @@ >>> #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) >>> #define VIDEO_MODE_MASTER_MODE (0x0 << 0) >>> >>> +/* ANALOGIX_DP_PKT_SEND_CTL */ >>> +#define IF_UP (0x1 << 4) >>> +#define IF_EN (0x1 << 0) >>> + >>> +/* ANALOGIX_DP_CRC_CON */ >>> +#define PSR_VID_CRC_FLUSH (0x1 << 2) >>> +#define PSR_VID_CRC_ENABLE (0x1 << 0) >>> + >>> #endif /* _ANALOGIX_DP_REG_H */ >>> diff --git a/include/drm/bridge/analogix_dp.h >>> b/include/drm/bridge/analogix_dp.h >>> index 261b86d..183a336 100644 >>> --- a/include/drm/bridge/analogix_dp.h >>> +++ b/include/drm/bridge/analogix_dp.h >>> @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { >>> struct drm_connector *); >>> }; >>> >>> +int analogix_dp_active_psr(struct device *dev); >>> +int analogix_dp_inactive_psr(struct device *dev); >> >> Why active/inactive instead of enable/disable, which is used everywhere >> else? > > > Done > > Thanks, > - Yakir > > >>> + >>> int analogix_dp_resume(struct device *dev); >>> int analogix_dp_suspend(struct device *dev); >>> >>> -- >>> 1.9.1 >>> >>> >> >> > >
Sean, On 07/12/2016 11:29 PM, Sean Paul wrote: > On Thu, Jul 7, 2016 at 7:26 PM, Yakir Yang <ykk@rock-chips.com> wrote: >> Sean, >> >> Thanks for your review. >> >> >> On 07/02/2016 03:46 AM, Sean Paul wrote: >>> On Fri, Jul 1, 2016 at 5:19 AM, Yakir Yang <ykk@rock-chips.com> wrote: >>>> The full name of PSR is Panel Self Refresh, panel device could refresh >>>> itself with the hardware framebuffer in panel, this would make lots of >>>> sense to save the power consumption. >>>> >>>> This patch have exported two symbols for platform driver to implement >>>> the PSR function in hardware side: >>>> - analogix_dp_active_psr() >>>> - analogix_dp_inactive_psr() >>>> >>>> Signed-off-by: Yakir Yang <ykk@rock-chips.com> >>>> --- >>>> Changes in v3: >>>> - split analogix_dp_enable_psr(), make it more clearly >>>> analogix_dp_detect_sink_psr() >>>> analogix_dp_enable_sink_psr() >>>> - remove some nosie register setting comments >>>> >>>> Changes in v2: >>>> - introduce in v2, splite the common Analogix DP changes out >>>> >>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 >>>> ++++++++++++++++++++++ >>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ >>>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 >>>> ++++++++++++++++++ >>>> drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ >>>> include/drm/bridge/analogix_dp.h | 3 + >>>> 5 files changed, 153 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>>> index 32715da..b557097 100644 >>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >>>> @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct >>>> analogix_dp_device *dp) >>>> return 0; >>>> } >>>> >>>> +int analogix_dp_active_psr(struct device *dev) >>>> +{ >>>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>>> + >>>> + if (!dp->psr_support) >>>> + return -EINVAL; >>>> + >>>> + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | >>>> + EDP_VSC_PSR_CRC_VALUES_VALID); >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); >>>> + >>>> +int analogix_dp_inactive_psr(struct device *dev) >>>> +{ >>>> + struct analogix_dp_device *dp = dev_get_drvdata(dev); >>>> + >>>> + if (!dp->psr_support) >>>> + return -EINVAL; >>>> + >>>> + analogix_dp_send_psr_spd(dp, 0); >>>> + return 0; >>>> +} >>>> +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); >>>> + >>>> +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) >>>> +{ >>>> + unsigned char psr_version; >>>> + >>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, >>>> &psr_version); >>>> + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); >>>> + >>> This info message is likely to be spammy since it's printed everytime >>> the panel toggle on. Perhaps downgrade to debug level. >> >> Okay, done. >> >>>> + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; >>>> +} >>>> + >>>> +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) >>> Return type is int, but the function never fails and you don't check >>> the return value when calling it. Seems like this should be void. >> >> Done. >> >>>> +{ >>>> + unsigned char psr_en; >>>> + >>>> + /* Disable psr function */ >>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>>> + psr_en &= ~DP_PSR_ENABLE; >>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>>> + >>>> + /* Main-Link transmitter remains active during PSR active states >>>> */ >>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>>> + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; >>> Why read psr_en if you're just going to overwrite it? Perhaps you meant |= >>> here. >>> >> Yes, it's my mistaken, no need to read the DP_PSR_EN_CFG, just configure it >> directly is enough. >> >>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>>> + >>>> + /* Enable psr function */ >>>> + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); >>>> + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | >>>> + DP_PSR_CRC_VERIFICATION; >>> Again, no need to read if you're just overwriting. >> >> Yes, ditto >> >> >>>> + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); >>>> + >>>> + analogix_dp_enable_psr_crc(dp); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static unsigned char analogix_dp_calc_edid_check_sum(unsigned char >>>> *edid_data) >>>> { >>>> int i; >>>> @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct >>>> analogix_dp_device *dp) >>>> >>>> /* Enable video */ >>>> analogix_dp_start_video(dp); >>>> + >>>> + dp->psr_support = analogix_dp_detect_sink_psr(dp); >>>> + if (dp->psr_support) >>>> + analogix_dp_enable_sink_psr(dp); >>>> } >>>> >>>> int analogix_dp_get_modes(struct drm_connector *connector) >>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>>> index b456380..6ca5dde 100644 >>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h >>>> @@ -177,6 +177,7 @@ struct analogix_dp_device { >>>> int hpd_gpio; >>>> bool force_hpd; >>>> unsigned char edid[EDID_BLOCK_LENGTH * 2]; >>>> + bool psr_support; >>>> >>>> struct analogix_dp_plat_data *plat_data; >>>> }; >>>> @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct >>>> analogix_dp_device *dp); >>>> void analogix_dp_config_video_slave_mode(struct analogix_dp_device >>>> *dp); >>>> void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); >>>> void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); >>>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); >>>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); >>>> + >>>> #endif /* _ANALOGIX_DP_CORE_H */ >>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>>> index 48030f0..e8372c7 100644 >>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c >>>> @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct >>>> analogix_dp_device *dp) >>>> reg |= SCRAMBLING_DISABLE; >>>> writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); >>>> } >>>> + >>>> +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) >>>> +{ >>>> + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, >>>> + dp->reg_base + ANALOGIX_DP_CRC_CON); >>>> + >>>> + usleep_range(10, 20); >>> Is this sleep arbitrary, or documented somewhere? Could you add a >>> comment explaining how this was arrived at? >>> >> Yes, this sleep is arbitrary, there is no document about the >> > Ok, so I assume we actually need the delay for things to work? If not, > delete the delay. If we need it, please add a comment saying the wait > is required and the value was chosen via trial and error. I should delete this delay, cause i found that PSR_VID_CRC_FLUSH not a trigger bit, but a enable gate. Here is the TRM explain: PSR_VID_CRC_FLUSH: PSR Video CRC flush enable. The PSR video CRC value is initialized at every v-sync leading edge. So I just need to set this flag after enable PSR CRC function, no need to remove it :-D >>>> + >>>> + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); >>>> +} >>>> + >>>> +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) >>>> +{ >>>> + unsigned int val; >>>> + >>>> + /* don't send info frame */ >>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> + val &= ~IF_EN; >>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> + >>>> + /* configure single frame update mode */ >>>> + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); >>>> + >>>> + /* configure VSC HB0 ~ HB3 */ >>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); >>>> + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); >>>> + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); >>>> + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); >>>> + >>>> + /* configure VSC HB0 ~ HB3 */ >>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); >>>> + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); >>>> + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); >>>> + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); >>>> + >>>> + /* configure DB0 / DB1 values */ >>>> + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); >>> Lots of hardcoded values here. I think this could be cleaned up. >> >> For "HB0~HB3", "PB0~PB3" and "DB1", I don't understand very well. Those >> seems to be a kind of head number, I got those magic values from our IC >> side. So I think those should be okay to keep the hardcode values here :-D >> > Hmm. Well, I'd probably pull them out into some HBX_MAGIC_VALUE > define, but I suppose it's fine to keep them as-is. Please at least > add a comment above explaining that they're magic values provided by > your IC team. Done ;) Thanks, - Yakir >> But for the "0x3" in "ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL", I would fix it >> with suitable macro. >> >> >>>> + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); >>>> + >>>> + /* set reuse spd inforframe */ >>>> + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>>> + val |= REUSE_SPD_EN; >>>> + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); >>>> + >>>> + /* mark info frame update */ >>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> + val = (val | IF_UP) & ~IF_EN; >>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> + >>>> + /* send info frame */ >>>> + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> + val |= IF_EN; >>>> + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); >>>> +} >>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>>> b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>>> index cdcc6c5..a2698e4 100644 >>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h >>>> @@ -22,6 +22,8 @@ >>>> #define ANALOGIX_DP_VIDEO_CTL_8 0x3C >>>> #define ANALOGIX_DP_VIDEO_CTL_10 0x44 >>>> >>>> +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 >>>> + >>>> #define ANALOGIX_DP_PLL_REG_1 0xfc >>>> #define ANALOGIX_DP_PLL_REG_2 0x9e4 >>>> #define ANALOGIX_DP_PLL_REG_3 0x9e8 >>>> @@ -30,6 +32,21 @@ >>>> >>>> #define ANALOGIX_DP_PD 0x12c >>>> >>>> +#define ANALOGIX_DP_IF_TYPE 0x244 >>>> +#define ANALOGIX_DP_IF_PKT_DB1 0x254 >>>> +#define ANALOGIX_DP_IF_PKT_DB2 0x258 >>>> +#define ANALOGIX_DP_SPD_HB0 0x2F8 >>>> +#define ANALOGIX_DP_SPD_HB1 0x2FC >>>> +#define ANALOGIX_DP_SPD_HB2 0x300 >>>> +#define ANALOGIX_DP_SPD_HB3 0x304 >>>> +#define ANALOGIX_DP_SPD_PB0 0x308 >>>> +#define ANALOGIX_DP_SPD_PB1 0x30C >>>> +#define ANALOGIX_DP_SPD_PB2 0x310 >>>> +#define ANALOGIX_DP_SPD_PB3 0x314 >>>> +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 >>>> +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C >>>> +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 >>>> + >>>> #define ANALOGIX_DP_LANE_MAP 0x35C >>>> >>>> #define ANALOGIX_DP_ANALOG_CTL_1 0x370 >>>> @@ -103,6 +120,8 @@ >>>> >>>> #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 >>>> >>>> +#define ANALOGIX_DP_CRC_CON 0x890 >>>> + >>>> /* ANALOGIX_DP_TX_SW_RESET */ >>>> #define RESET_DP_TX (0x1 << 0) >>>> >>>> @@ -151,6 +170,7 @@ >>>> #define VID_CHK_UPDATE_TYPE_SHIFT (4) >>>> #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) >>>> #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) >>>> +#define REUSE_SPD_EN (0x1 << 3) >>>> >>>> /* ANALOGIX_DP_VIDEO_CTL_8 */ >>>> #define VID_HRES_TH(x) (((x) & 0xf) << 4) >>>> @@ -376,4 +396,12 @@ >>>> #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) >>>> #define VIDEO_MODE_MASTER_MODE (0x0 << 0) >>>> >>>> +/* ANALOGIX_DP_PKT_SEND_CTL */ >>>> +#define IF_UP (0x1 << 4) >>>> +#define IF_EN (0x1 << 0) >>>> + >>>> +/* ANALOGIX_DP_CRC_CON */ >>>> +#define PSR_VID_CRC_FLUSH (0x1 << 2) >>>> +#define PSR_VID_CRC_ENABLE (0x1 << 0) >>>> + >>>> #endif /* _ANALOGIX_DP_REG_H */ >>>> diff --git a/include/drm/bridge/analogix_dp.h >>>> b/include/drm/bridge/analogix_dp.h >>>> index 261b86d..183a336 100644 >>>> --- a/include/drm/bridge/analogix_dp.h >>>> +++ b/include/drm/bridge/analogix_dp.h >>>> @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { >>>> struct drm_connector *); >>>> }; >>>> >>>> +int analogix_dp_active_psr(struct device *dev); >>>> +int analogix_dp_inactive_psr(struct device *dev); >>> Why active/inactive instead of enable/disable, which is used everywhere >>> else? >> >> Done >> >> Thanks, >> - Yakir >> >> >>>> + >>>> int analogix_dp_resume(struct device *dev); >>>> int analogix_dp_suspend(struct device *dev); >>>> >>>> -- >>>> 1.9.1 >>>> >>>> >>> >> > >
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index 32715da..b557097 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -97,6 +97,66 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp) return 0; } +int analogix_dp_active_psr(struct device *dev) +{ + struct analogix_dp_device *dp = dev_get_drvdata(dev); + + if (!dp->psr_support) + return -EINVAL; + + analogix_dp_send_psr_spd(dp, EDP_VSC_PSR_STATE_ACTIVE | + EDP_VSC_PSR_CRC_VALUES_VALID); + return 0; +} +EXPORT_SYMBOL_GPL(analogix_dp_active_psr); + +int analogix_dp_inactive_psr(struct device *dev) +{ + struct analogix_dp_device *dp = dev_get_drvdata(dev); + + if (!dp->psr_support) + return -EINVAL; + + analogix_dp_send_psr_spd(dp, 0); + return 0; +} +EXPORT_SYMBOL_GPL(analogix_dp_inactive_psr); + +static bool analogix_dp_detect_sink_psr(struct analogix_dp_device *dp) +{ + unsigned char psr_version; + + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_SUPPORT, &psr_version); + dev_info(dp->dev, "Panel PSR version : %x\n", psr_version); + + return (psr_version & DP_PSR_IS_SUPPORTED) ? true : false; +} + +static int analogix_dp_enable_sink_psr(struct analogix_dp_device *dp) +{ + unsigned char psr_en; + + /* Disable psr function */ + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); + psr_en &= ~DP_PSR_ENABLE; + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); + + /* Main-Link transmitter remains active during PSR active states */ + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); + psr_en = DP_PSR_MAIN_LINK_ACTIVE | DP_PSR_CRC_VERIFICATION; + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); + + /* Enable psr function */ + analogix_dp_read_byte_from_dpcd(dp, DP_PSR_EN_CFG, &psr_en); + psr_en = DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE | + DP_PSR_CRC_VERIFICATION; + analogix_dp_write_byte_to_dpcd(dp, DP_PSR_EN_CFG, psr_en); + + analogix_dp_enable_psr_crc(dp); + + return 0; +} + static unsigned char analogix_dp_calc_edid_check_sum(unsigned char *edid_data) { int i; @@ -921,6 +981,10 @@ static void analogix_dp_commit(struct analogix_dp_device *dp) /* Enable video */ analogix_dp_start_video(dp); + + dp->psr_support = analogix_dp_detect_sink_psr(dp); + if (dp->psr_support) + analogix_dp_enable_sink_psr(dp); } int analogix_dp_get_modes(struct drm_connector *connector) diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h index b456380..6ca5dde 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h @@ -177,6 +177,7 @@ struct analogix_dp_device { int hpd_gpio; bool force_hpd; unsigned char edid[EDID_BLOCK_LENGTH * 2]; + bool psr_support; struct analogix_dp_plat_data *plat_data; }; @@ -278,4 +279,7 @@ int analogix_dp_is_video_stream_on(struct analogix_dp_device *dp); void analogix_dp_config_video_slave_mode(struct analogix_dp_device *dp); void analogix_dp_enable_scrambling(struct analogix_dp_device *dp); void analogix_dp_disable_scrambling(struct analogix_dp_device *dp); +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp); +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1); + #endif /* _ANALOGIX_DP_CORE_H */ diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c index 48030f0..e8372c7 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c @@ -1322,3 +1322,57 @@ void analogix_dp_disable_scrambling(struct analogix_dp_device *dp) reg |= SCRAMBLING_DISABLE; writel(reg, dp->reg_base + ANALOGIX_DP_TRAINING_PTN_SET); } + +void analogix_dp_enable_psr_crc(struct analogix_dp_device *dp) +{ + writel(PSR_VID_CRC_FLUSH | PSR_VID_CRC_ENABLE, + dp->reg_base + ANALOGIX_DP_CRC_CON); + + usleep_range(10, 20); + + writel(PSR_VID_CRC_ENABLE, dp->reg_base + ANALOGIX_DP_CRC_CON); +} + +void analogix_dp_send_psr_spd(struct analogix_dp_device *dp, int db1) +{ + unsigned int val; + + /* don't send info frame */ + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); + val &= ~IF_EN; + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); + + /* configure single frame update mode */ + writel(0x3, dp->reg_base + ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL); + + /* configure VSC HB0 ~ HB3 */ + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_HB0); + writel(0x07, dp->reg_base + ANALOGIX_DP_SPD_HB1); + writel(0x02, dp->reg_base + ANALOGIX_DP_SPD_HB2); + writel(0x08, dp->reg_base + ANALOGIX_DP_SPD_HB3); + + /* configure VSC HB0 ~ HB3 */ + writel(0x00, dp->reg_base + ANALOGIX_DP_SPD_PB0); + writel(0x16, dp->reg_base + ANALOGIX_DP_SPD_PB1); + writel(0xCE, dp->reg_base + ANALOGIX_DP_SPD_PB2); + writel(0x5D, dp->reg_base + ANALOGIX_DP_SPD_PB3); + + /* configure DB0 / DB1 values */ + writel(0x00, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB0); + writel(db1, dp->reg_base + ANALOGIX_DP_VSC_SHADOW_DB1); + + /* set reuse spd inforframe */ + val = readl(dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); + val |= REUSE_SPD_EN; + writel(val, dp->reg_base + ANALOGIX_DP_VIDEO_CTL_3); + + /* mark info frame update */ + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); + val = (val | IF_UP) & ~IF_EN; + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); + + /* send info frame */ + val = readl(dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); + val |= IF_EN; + writel(val, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL); +} diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h index cdcc6c5..a2698e4 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h @@ -22,6 +22,8 @@ #define ANALOGIX_DP_VIDEO_CTL_8 0x3C #define ANALOGIX_DP_VIDEO_CTL_10 0x44 +#define ANALOGIX_DP_SPDIF_AUDIO_CTL_0 0xD8 + #define ANALOGIX_DP_PLL_REG_1 0xfc #define ANALOGIX_DP_PLL_REG_2 0x9e4 #define ANALOGIX_DP_PLL_REG_3 0x9e8 @@ -30,6 +32,21 @@ #define ANALOGIX_DP_PD 0x12c +#define ANALOGIX_DP_IF_TYPE 0x244 +#define ANALOGIX_DP_IF_PKT_DB1 0x254 +#define ANALOGIX_DP_IF_PKT_DB2 0x258 +#define ANALOGIX_DP_SPD_HB0 0x2F8 +#define ANALOGIX_DP_SPD_HB1 0x2FC +#define ANALOGIX_DP_SPD_HB2 0x300 +#define ANALOGIX_DP_SPD_HB3 0x304 +#define ANALOGIX_DP_SPD_PB0 0x308 +#define ANALOGIX_DP_SPD_PB1 0x30C +#define ANALOGIX_DP_SPD_PB2 0x310 +#define ANALOGIX_DP_SPD_PB3 0x314 +#define ANALOGIX_DP_PSR_FRAME_UPDATE_CTRL 0x318 +#define ANALOGIX_DP_VSC_SHADOW_DB0 0x31C +#define ANALOGIX_DP_VSC_SHADOW_DB1 0x320 + #define ANALOGIX_DP_LANE_MAP 0x35C #define ANALOGIX_DP_ANALOG_CTL_1 0x370 @@ -103,6 +120,8 @@ #define ANALOGIX_DP_SOC_GENERAL_CTL 0x800 +#define ANALOGIX_DP_CRC_CON 0x890 + /* ANALOGIX_DP_TX_SW_RESET */ #define RESET_DP_TX (0x1 << 0) @@ -151,6 +170,7 @@ #define VID_CHK_UPDATE_TYPE_SHIFT (4) #define VID_CHK_UPDATE_TYPE_1 (0x1 << 4) #define VID_CHK_UPDATE_TYPE_0 (0x0 << 4) +#define REUSE_SPD_EN (0x1 << 3) /* ANALOGIX_DP_VIDEO_CTL_8 */ #define VID_HRES_TH(x) (((x) & 0xf) << 4) @@ -376,4 +396,12 @@ #define VIDEO_MODE_SLAVE_MODE (0x1 << 0) #define VIDEO_MODE_MASTER_MODE (0x0 << 0) +/* ANALOGIX_DP_PKT_SEND_CTL */ +#define IF_UP (0x1 << 4) +#define IF_EN (0x1 << 0) + +/* ANALOGIX_DP_CRC_CON */ +#define PSR_VID_CRC_FLUSH (0x1 << 2) +#define PSR_VID_CRC_ENABLE (0x1 << 0) + #endif /* _ANALOGIX_DP_REG_H */ diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h index 261b86d..183a336 100644 --- a/include/drm/bridge/analogix_dp.h +++ b/include/drm/bridge/analogix_dp.h @@ -38,6 +38,9 @@ struct analogix_dp_plat_data { struct drm_connector *); }; +int analogix_dp_active_psr(struct device *dev); +int analogix_dp_inactive_psr(struct device *dev); + int analogix_dp_resume(struct device *dev); int analogix_dp_suspend(struct device *dev);
The full name of PSR is Panel Self Refresh, panel device could refresh itself with the hardware framebuffer in panel, this would make lots of sense to save the power consumption. This patch have exported two symbols for platform driver to implement the PSR function in hardware side: - analogix_dp_active_psr() - analogix_dp_inactive_psr() Signed-off-by: Yakir Yang <ykk@rock-chips.com> --- Changes in v3: - split analogix_dp_enable_psr(), make it more clearly analogix_dp_detect_sink_psr() analogix_dp_enable_sink_psr() - remove some nosie register setting comments Changes in v2: - introduce in v2, splite the common Analogix DP changes out drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 64 ++++++++++++++++++++++ drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++ drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c | 54 ++++++++++++++++++ drivers/gpu/drm/bridge/analogix/analogix_dp_reg.h | 28 ++++++++++ include/drm/bridge/analogix_dp.h | 3 + 5 files changed, 153 insertions(+)