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