Re: [PATCH] drm/exynos: add hdmi power on/off sequence

[Tomasz Figa <t.figa@samsung.com>]

Hi Shirish,

On 11.03.2014 12:26, Shirish S wrote:
> This patch implements the power on/off sequence
> (and its dependant functions) of HDMI exynos5250
> as provided by the hardware team.
>
> Signed-off-by: Shirish S <s.shirish@samsung.com>
> ---
>   drivers/gpu/drm/exynos/exynos_hdmi.c |  137 +++++++++++++++++++++++++++++-----
>   drivers/gpu/drm/exynos/regs-hdmi.h   |   15 ++++
>   2 files changed, 133 insertions(+), 19 deletions(-)
>   mode change 100644 => 100755 drivers/gpu/drm/exynos/exynos_hdmi.c
>   mode change 100644 => 100755 drivers/gpu/drm/exynos/regs-hdmi.h

Please do not change file modes.

>
> diff --git a/drivers/gpu/drm/exynos/exynos_hdmi.c b/drivers/gpu/drm/exynos/exynos_hdmi.c
> old mode 100644
> new mode 100755
> index 12fdf55..ee000f7
> --- a/drivers/gpu/drm/exynos/exynos_hdmi.c
> +++ b/drivers/gpu/drm/exynos/exynos_hdmi.c
> @@ -180,6 +180,7 @@ struct hdmi_context {
>
>   	void __iomem			*regs;
>   	int				irq;
> +	void __iomem			*phy_pow_ctrl_reg;
>
>   	struct i2c_client		*ddc_port;
>   	struct i2c_client		*hdmiphy_port;
> @@ -387,6 +388,33 @@ static inline void hdmi_reg_writemask(struct hdmi_context *hdata,
>   	writel(value, hdata->regs + reg_id);
>   }
>
> +
> +static inline void hdmi_phy_pow_ctrl_reg_writemask(struct hdmi_context *hdata,
> +				 u32 value, u32 mask)
> +{
> +	u32 old = readl(hdata->phy_pow_ctrl_reg);
> +	value = (value & mask) | (old & ~mask);
> +	writel(value, hdata->phy_pow_ctrl_reg);
> +}

Do you really need a separate function for just two accesses?

> +
> +static int hdmiphy_reg_writeb(struct hdmi_context *hdata,
> +			u32 reg_offset, u8 value)
> +{
> +	if (hdata->hdmiphy_port) {

I'd say this function should be called at all if hdmiphy_port is NULL. 
Anyway...

> +		u8 buffer[2];
> +		int ret;
> +
> +		buffer[0] = reg_offset;
> +		buffer[1] = value;
> +
> +		ret = i2c_master_send(hdata->hdmiphy_port, buffer, 2);
> +		if (ret == 2)
> +			return 0;
> +		return ret;
> +	} else

CodingStyle: If one if branch needs braces, then the other one should 
have them as well.

> +		return 0;

If this is still needed, the code could be simplified by rewriting this as:

	if (!hdata->hdmiphy_port)
		return 0;

	/* ... */

> +}
> +
>   static void hdmi_v13_regs_dump(struct hdmi_context *hdata, char *prefix)
>   {
>   #define DUMPREG(reg_id) \
> @@ -1386,19 +1414,14 @@ static void hdmi_mode_apply(struct hdmi_context *hdata)
>
>   static void hdmiphy_conf_reset(struct hdmi_context *hdata)
>   {
> -	u8 buffer[2];
>   	u32 reg;
>
>   	clk_disable_unprepare(hdata->res.sclk_hdmi);
>   	clk_set_parent(hdata->res.mout_hdmi, hdata->res.sclk_pixel);
>   	clk_prepare_enable(hdata->res.sclk_hdmi);
>
> -	/* operation mode */
> -	buffer[0] = 0x1f;
> -	buffer[1] = 0x00;
> -
> -	if (hdata->hdmiphy_port)
> -		i2c_master_send(hdata->hdmiphy_port, buffer, 2);
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_ENABLE_MODE_SET);

Hmm, you should be able to use i2c_smbus_write_byte_data().

>
>   	if (hdata->type == HDMI_TYPE13)
>   		reg = HDMI_V13_PHY_RSTOUT;
> @@ -1414,16 +1437,42 @@ static void hdmiphy_conf_reset(struct hdmi_context *hdata)
>
>   static void hdmiphy_poweron(struct hdmi_context *hdata)
>   {
> -	if (hdata->type == HDMI_TYPE14)
> -		hdmi_reg_writemask(hdata, HDMI_PHY_CON_0, 0,
> -			HDMI_PHY_POWER_OFF_EN);
> +	if (hdata->type == HDMI_TYPE13)

Shouldn't the check be HDMI_TYPE != HDMI_TYPE14 to also cover other 
types than 13 and 14?

> +		return;
> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	/* For PHY Mode Setting */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_ENABLE_MODE_SET);
> +	/* Phy Power On */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_POWER,
> +						HDMI_PHY_POWER_ON);
> +	/* For PHY Mode Setting */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_DISABLE_MODE_SET);

i2c_smbus_write_byte_data() should work for above 3 calls as well.

> +	/* PHY SW Reset */
> +	hdmiphy_conf_reset(hdata);
>   }
>
>   static void hdmiphy_poweroff(struct hdmi_context *hdata)
>   {
> -	if (hdata->type == HDMI_TYPE14)
> -		hdmi_reg_writemask(hdata, HDMI_PHY_CON_0, ~0,
> -			HDMI_PHY_POWER_OFF_EN);
> +	if (hdata->type == HDMI_TYPE13)
> +		return;

Same comment about type check as above.

> +
> +	DRM_DEBUG_KMS("\n");
> +
> +	/* PHY SW Reset */
> +	hdmiphy_conf_reset(hdata);
> +	/* For PHY Mode Setting */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_ENABLE_MODE_SET);
> +	/* PHY Power Off */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_POWER,
> +						HDMI_PHY_POWER_OFF);
> +	/* For PHY Mode Setting */
> +	hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_DISABLE_MODE_SET);

For above 3 i2c_smbus_write_byte_data() could be used too.

>   }
>
>   static void hdmiphy_conf_apply(struct hdmi_context *hdata)
> @@ -1476,6 +1525,14 @@ static void hdmiphy_conf_apply(struct hdmi_context *hdata)
>   		DRM_ERROR("failed to read hdmiphy config\n");
>   		return;
>   	}
> +	usleep_range(10000, 12000);

Why?

> +
> +	ret = hdmiphy_reg_writeb(hdata, HDMIPHY_MODE_SET_DONE,
> +						HDMI_PHY_DISABLE_MODE_SET);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to enable hdmiphy\n");
> +		return;
> +	}

i2c_smbus_write_byte_data()

>
>   	for (i = 0; i < ret; i++)
>   		DRM_DEBUG_KMS("hdmiphy[0x%02x] write[0x%02x] - "
> @@ -1767,10 +1824,10 @@ static void hdmi_poweron(struct exynos_drm_display *display)
>   	if (regulator_bulk_enable(res->regul_count, res->regul_bulk))
>   		DRM_DEBUG_KMS("failed to enable regulator bulk\n");
>
> -	clk_prepare_enable(res->hdmiphy);
> -	clk_prepare_enable(res->hdmi);
> -	clk_prepare_enable(res->sclk_hdmi);

Are you sure about this? Where the clocks are handled after these lines 
are removed?

> +	hdmi_phy_pow_ctrl_reg_writemask(hdata, PMU_HDMI_PHY_ENABLE,
> +		PMU_HDMI_PHY_CONTROL_MASK);
>
> +	/* HDMI PHY Enable and Power-On */
>   	hdmiphy_poweron(hdata);
>   	hdmi_commit(display);
>   }
> @@ -1790,11 +1847,16 @@ static void hdmi_poweroff(struct exynos_drm_display *display)
>   	 * its reset state seems to meet the condition.
>   	 */
>   	hdmiphy_conf_reset(hdata);
> +
> +	/* HDMI System Disable */
> +	hdmi_reg_writemask(hdata, HDMI_CON_0, 0, HDMI_EN);
> +
>   	hdmiphy_poweroff(hdata);
>
> -	clk_disable_unprepare(res->sclk_hdmi);
> -	clk_disable_unprepare(res->hdmi);
> -	clk_disable_unprepare(res->hdmiphy);

Ditto.

> +	/* HDMI PHY Disable */
> +	hdmi_phy_pow_ctrl_reg_writemask(hdata, PMU_HDMI_PHY_DISABLE,
> +		PMU_HDMI_PHY_CONTROL_MASK);
> +
>   	regulator_bulk_disable(res->regul_count, res->regul_bulk);
>
>   	pm_runtime_put_sync(hdata->dev);
> @@ -1947,6 +2009,36 @@ err_data:
>   	return NULL;
>   }
>
> +static int drm_hdmi_dt_parse_phy_pow_control(struct hdmi_context *hdata)
> +{
> +	struct device_node *phy_pow_ctrl_node;
> +	u32 buf[2];
> +	int ret = 0;
> +
> +	phy_pow_ctrl_node = of_find_node_by_name(NULL, "phy-power-control");
> +	if (!phy_pow_ctrl_node)
> +		return 0;

Where is this node documented?

> +
> +	/* reg property holds two informations: addr of pmu register, size */
> +	if (of_property_read_u32_array(phy_pow_ctrl_node, "reg",
> +			(u32 *)&buf, 2)) {
> +		DRM_ERROR("faild to get phy power control reg\n");

typo: s/faild/failed/

> +		ret = -EINVAL;
> +		goto fail;
> +	}
> +

This is not the correct way of parsing reg property. Please see 
of_iomap() or of_address_to_resource()+devm_ioremap_resource().

> +	hdata->phy_pow_ctrl_reg = devm_ioremap(hdata->dev, buf[0],  buf[1]);
> +	if (!hdata->phy_pow_ctrl_reg) {
> +		DRM_ERROR("failed to ioremap phy pmu reg\n");
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +fail:
> +	of_node_put(phy_pow_ctrl_node);
> +	return ret;
> +}

Anyway, the whole PMU mapping above seems to be wrong. Since PMU is 
already defined as a syscon, a reference to the PMU node should passed 
through a DT property and the syscon interface should be used to obtain 
a regmap that gives you access to PMU registers.

See the following commit for an example of use:

4f1f653a68d6 watchdog: s3c2410_wdt: use syscon regmap [...]

> +
>   static struct of_device_id hdmi_match_types[] = {
>   	{
>   		.compatible = "samsung,exynos5-hdmi",
> @@ -2010,6 +2102,13 @@ static int hdmi_bind(struct device *dev, struct device *master, void *data)
>   		return ret;
>   	}
>
> +	/* map hdmiphy power control reg */
> +	ret = drm_hdmi_dt_parse_phy_pow_control(hdata);
> +	if (ret) {
> +		DRM_ERROR("failed to map phy power control registers\n");
> +		return ret;
> +	}
> +
>   	/* DDC i2c driver */
>   	ddc_node = of_parse_phandle(dev->of_node, "ddc", 0);
>   	if (!ddc_node) {
> diff --git a/drivers/gpu/drm/exynos/regs-hdmi.h b/drivers/gpu/drm/exynos/regs-hdmi.h
> old mode 100644
> new mode 100755
> index ef1b3eb..e686fe9
> --- a/drivers/gpu/drm/exynos/regs-hdmi.h
> +++ b/drivers/gpu/drm/exynos/regs-hdmi.h
> @@ -578,4 +578,19 @@
>   #define HDMI_TG_VACT_ST4_H		HDMI_TG_BASE(0x0074)
>   #define HDMI_TG_3D			HDMI_TG_BASE(0x00F0)
>
> +/* HDMI PHY Registers Offsets*/
> +
> +#define HDMIPHY_POWER			(0x74 >> 2)
> +#define HDMIPHY_MODE_SET_DONE		(0x7C >> 2)

CodingStyle: Lowercase is preferred for hexadecimal numbers.

> +
> +/* HDMI PHY Values */
> +#define HDMI_PHY_POWER_ON		0x80
> +#define HDMI_PHY_POWER_OFF		0xFF

Ditto.

Best regards,
Tomasz