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 at 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
