Re: [PATCH v10 05/11] drm: bridge/dw_hdmi:split some phy configuration to platform driver
Hi Andy, Nice work on this patch series. Its getting better and better :). On 14/11/14 03:27, Andy Yan wrote: hdmi phy clock symbol and transmission termination value can adjust platform specific to get the best SI ^Is this signal integrity? Are these two disjoint features in separate patches? also add mode_valid interface for some platform may not support all the display mode Sounds like another separate patch to me. :) Also, This series is becoming quite large. With major changes and fixes mixed together. Patch 3 splits imx-drm. Patch 4 moves dw-drm out of imx-drm folder. Patch 7 adds binding Patch 9 converts to drm bridge. Can these be placed together easily? And in the start. i.e. patch 1, 2, 3, 4, Then all fixes etc can come afterwards? It helps when checking histories later as to how a driver was made and how fixes happened. Especially when file moves happen.. Cheers, ZubairLK Signed-off-by: Andy Yan andy@rock-chips.com --- Changes in v10: - split generic dw_hdmi.c improvements from patch#11 (add rk3288 support) Changes in v9: None Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None drivers/gpu/drm/bridge/dw_hdmi.c | 29 +++-- drivers/staging/imx-drm/dw_hdmi-imx.c | 10 +- include/drm/bridge/dw_hdmi.h | 7 +++ 3 files changed, 43 insertions(+), 3 deletions(-) ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 11/11] drm: bridge/dw_hdmi: add rockchip rk3288 support
Hi Andy
On 14/11/14 03:31, Andy Yan wrote:
Rockchip RK3288 hdmi is compatible with dw_hdmi
Signed-off-by: Andy Yan andy@rock-chips.com
---
Changes in v10:
- add more display mode support mpll configuration for rk3288
Changes in v9:
- move some phy configuration to platform driver
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/dw_hdmi.c| 10 +
drivers/gpu/drm/bridge/dw_hdmi.h| 3 +-
drivers/gpu/drm/rockchip/Kconfig| 10 +
drivers/gpu/drm/rockchip/Makefile | 2 +-
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 355
include/drm/bridge/dw_hdmi.h| 1 +
6 files changed, 379 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
b/drivers/gpu/drm/bridge/dw_hdmi.c
index a2876fe..1dd1f0b 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -715,6 +715,13 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
*hdmi, u8 enable)
HDMI_PHY_CONF0_ENTMDS_MASK);
}
+static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
+{
+ hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
+ HDMI_PHY_CONF0_SPARECTRL_OFFSET,
+ HDMI_PHY_CONF0_SPARECTRL_MASK);
+}
+
What does enable spare do? Can other future SoCs use it?
This looks like DW specific. And can be a separate commit that adds a feature
to the dw hdmi driver.
Thanks
ZubairLK
static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
{
hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
@@ -846,6 +853,9 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
unsigned char prep,
dw_hdmi_phy_gen2_txpwron(hdmi, 1);
dw_hdmi_phy_gen2_pddq(hdmi, 0);
+ if (hdmi-dev_type == RK3288_HDMI)
+ dw_hdmi_phy_enable_spare(hdmi, 1);
+
/*Wait for PHY PLL lock */
msec = 5;
do {
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h
b/drivers/gpu/drm/bridge/dw_hdmi.h
index b8412a9..30a6b28 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -837,7 +837,8 @@ enum {
HDMI_PHY_CONF0_PDZ_OFFSET = 7,
HDMI_PHY_CONF0_ENTMDS_MASK = 0x40,
HDMI_PHY_CONF0_ENTMDS_OFFSET = 6,
- HDMI_PHY_CONF0_SPARECTRL = 0x20,
+ HDMI_PHY_CONF0_SPARECTRL_MASK = 0x20,
+ HDMI_PHY_CONF0_SPARECTRL_OFFSET = 5,
Cheers,
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 11/11] drm: bridge/dw_hdmi: add rockchip rk3288 support
On 14/11/14 10:37, Andy Yan wrote:
On 2014年11月14日 18:23, Zubair Lutfullah Kakakhel wrote:
Hi Andy
On 14/11/14 03:31, Andy Yan wrote:
Rockchip RK3288 hdmi is compatible with dw_hdmi
Signed-off-by: Andy Yan andy@rock-chips.com
---
Changes in v10:
- add more display mode support mpll configuration for rk3288
Changes in v9:
- move some phy configuration to platform driver
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/dw_hdmi.c| 10 +
drivers/gpu/drm/bridge/dw_hdmi.h| 3 +-
drivers/gpu/drm/rockchip/Kconfig| 10 +
drivers/gpu/drm/rockchip/Makefile | 2 +-
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 355
include/drm/bridge/dw_hdmi.h| 1 +
6 files changed, 379 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
b/drivers/gpu/drm/bridge/dw_hdmi.c
index a2876fe..1dd1f0b 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -715,6 +715,13 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
*hdmi, u8 enable)
HDMI_PHY_CONF0_ENTMDS_MASK);
}
+static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
+{
+hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
+ HDMI_PHY_CONF0_SPARECTRL_OFFSET,
+ HDMI_PHY_CONF0_SPARECTRL_MASK);
+}
+
What does enable spare do? Can other future SoCs use it?
This looks like DW specific. And can be a separate commit that adds a
feature to the dw hdmi driver.
Actually I am not very clearly about this bit, but RK3288 HDMI will not
work without this bit enable.
On imx6, the description about this bit is:Reserved. Spare pin control.
On rk3288, the description is: svsret/sparectrl
Both are very simple.
Sounds like the IP Core in the rk3288 is slightly upgraded.
Separate this section in a different commit as it is generic DW stuff.
And add what you wrote above in the commit message.
Thanks
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 05/11] drm: bridge/dw_hdmi:split some phy configuration to platform driver
On 14/11/14 10:53, Andy Yan wrote: Hi ZubairLK: Thanks for your review. On 2014年11月14日 18:19, Zubair Lutfullah Kakakhel wrote: Hi Andy, Nice work on this patch series. Its getting better and better :). On 14/11/14 03:27, Andy Yan wrote: hdmi phy clock symbol and transmission termination value can adjust platform specific to get the best SI ^Is this signal integrity? yes , SI is signal integrity, such as eye diagram measurement Are these two disjoint features in separate patches? also add mode_valid interface for some platform may not support all the display mode Sounds like another separate patch to me. :) they can seperate Also, This series is becoming quite large. With major changes and fixes mixed together. Patch 3 splits imx-drm. Patch 4 moves dw-drm out of imx-drm folder. Patch 7 adds binding Patch 9 converts to drm bridge. Can these be placed together easily? And in the start. i.e. patch 1, 2, 3, 4, Then all fixes etc can come afterwards? It helps when checking histories later as to how a driver was made and how fixes happened. Especially when file moves happen.. Do you mean we can rearrange the patch series? put patch 3, 4 ,7, 9 together one bye one than followed by the fixes patches 5 ,6, 8, 11 ? Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. Then the rest are bug fixes and feature additions. Cheers, ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 05/11] drm: bridge/dw_hdmi:split some phy configuration to platform driver
On 14/11/14 11:08, Andy Yan wrote: On 2014年11月14日 18:55, Zubair Lutfullah Kakakhel wrote: On 14/11/14 10:53, Andy Yan wrote: Hi ZubairLK: Thanks for your review. On 2014年11月14日 18:19, Zubair Lutfullah Kakakhel wrote: Hi Andy, Nice work on this patch series. Its getting better and better :). On 14/11/14 03:27, Andy Yan wrote: hdmi phy clock symbol and transmission termination value can adjust platform specific to get the best SI ^Is this signal integrity? yes , SI is signal integrity, such as eye diagram measurement Are these two disjoint features in separate patches? also add mode_valid interface for some platform may not support all the display mode Sounds like another separate patch to me. :) they can seperate Also, This series is becoming quite large. With major changes and fixes mixed together. Patch 3 splits imx-drm. Patch 4 moves dw-drm out of imx-drm folder. Patch 7 adds binding Patch 9 converts to drm bridge. Can these be placed together easily? And in the start. i.e. patch 1, 2, 3, 4, Then all fixes etc can come afterwards? It helps when checking histories later as to how a driver was made and how fixes happened. Especially when file moves happen.. Do you mean we can rearrange the patch series? put patch 3, 4 ,7, 9 together one bye one than followed by the fixes patches 5 ,6, 8, 11 ? Yes. Rearrange so that the split imx-drm/imx-hdmi and conversion to drm-bridge is at the start of the series. Then the rest are bug fixes and feature additions. Can I put patch#1(make checkpatch happy) and patch#2 (defer probe) as the first two patch. Daniel from Google chromium think it's better to put the two slightly changes in the front for easy review. Sure. I am not the maintainer. They have to make the final decision. ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v10 11/11] drm: bridge/dw_hdmi: add rockchip rk3288 support
On 14/11/14 11:13, Andy Yan wrote:
On 2014年11月14日 18:53, Zubair Lutfullah Kakakhel wrote:
On 14/11/14 10:37, Andy Yan wrote:
On 2014年11月14日 18:23, Zubair Lutfullah Kakakhel wrote:
Hi Andy
On 14/11/14 03:31, Andy Yan wrote:
Rockchip RK3288 hdmi is compatible with dw_hdmi
Signed-off-by: Andy Yan andy@rock-chips.com
---
Changes in v10:
- add more display mode support mpll configuration for rk3288
Changes in v9:
- move some phy configuration to platform driver
Changes in v8: None
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/dw_hdmi.c| 10 +
drivers/gpu/drm/bridge/dw_hdmi.h| 3 +-
drivers/gpu/drm/rockchip/Kconfig| 10 +
drivers/gpu/drm/rockchip/Makefile | 2 +-
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 355
include/drm/bridge/dw_hdmi.h| 1 +
6 files changed, 379 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
b/drivers/gpu/drm/bridge/dw_hdmi.c
index a2876fe..1dd1f0b 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -715,6 +715,13 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
*hdmi, u8 enable)
HDMI_PHY_CONF0_ENTMDS_MASK);
}
+static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
+{
+hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
+ HDMI_PHY_CONF0_SPARECTRL_OFFSET,
+ HDMI_PHY_CONF0_SPARECTRL_MASK);
+}
+
What does enable spare do? Can other future SoCs use it?
This looks like DW specific. And can be a separate commit that adds a
feature to the dw hdmi driver.
Actually I am not very clearly about this bit, but RK3288 HDMI will not
work without this bit enable.
On imx6, the description about this bit is:Reserved. Spare pin control.
On rk3288, the description is: svsret/sparectrl
Both are very simple.
Sounds like the IP Core in the rk3288 is slightly upgraded.
Separate this section in a different commit as it is generic DW stuff.
And add what you wrote above in the commit message.
Only Separate function dw_hdmi_phy_enale_spare ? not include
if (hdmi-dev_type == rk3288_hdmi)
dw_hdmi_enable_phy_spare(hdmi, 1);
?
Yes. Separate dw_hdmi_phy_enable_spare and its associated defines
HDMI_PHY_CONF0_SPARECTRL_OFFSET
HDMI_PHY_CONF0_SPARECTRL_MASK
in the header
as one commit.
The if (hdmi-dev_type == rk3288_hdmi) code is part of the add rk3288 support.
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v9 9/9] drm: bridge/dw_hdmi: add rockchip rk3288 support
On 13/11/14 12:57, Andy Yan wrote:
rk3288 hdmi is compatible with Designware hdmi
this patch is depend on patch by Mark Yao Add drm
driver for Rockchip Socs
see https://lkml.org/lkml/2014/10/8/201
Signed-off-by: Andy Yan andy@rock-chips.com
Signed-off-by: Yakir Yang y...@rock-chips.com
---
Changes in v9:
- move some phy configuration to platform driver
Changes in v8:
- add support for rockchip rk3288 hdmi
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/gpu/drm/bridge/dw_hdmi.c| 45 +++-
This patch looks like it does alot to the dw_hdmi.c as well and not
just adds support for rk3288..
drivers/gpu/drm/bridge/dw_hdmi.h| 3 +-
drivers/gpu/drm/rockchip/Kconfig| 10 +
drivers/gpu/drm/rockchip/Makefile | 2 +-
drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 328
drivers/staging/imx-drm/dw_hdmi-imx.c | 8 +
include/drm/bridge/dw_hdmi.h| 8 +
7 files changed, 399 insertions(+), 5 deletions(-)
create mode 100644 drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c
b/drivers/gpu/drm/bridge/dw_hdmi.c
index ed75147..1dd1f0b 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -668,11 +668,15 @@ static inline void hdmi_phy_test_dout(struct dw_hdmi
*hdmi,
static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
{
- while ((hdmi_readb(hdmi, HDMI_IH_I2CMPHY_STAT0) 0x3) == 0) {
+ u32 val;
+
+ while ((val = hdmi_readb(hdmi, HDMI_IH_I2CMPHY_STAT0) 0x3) == 0) {
if (msec-- == 0)
return false;
udelay(1000);
}
+ hdmi_writeb(hdmi, val, HDMI_IH_I2CMPHY_STAT0);
+
return true;
}
Is this a bug fix?
@@ -711,6 +715,13 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
*hdmi, u8 enable)
HDMI_PHY_CONF0_ENTMDS_MASK);
}
+static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, u8 enable)
+{
+ hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
+ HDMI_PHY_CONF0_SPARECTRL_OFFSET,
+ HDMI_PHY_CONF0_SPARECTRL_MASK);
+}
+
static void dw_hdmi_phy_gen2_pddq(struct dw_hdmi *hdmi, u8 enable)
{
hdmi_mask_writeb(hdmi, enable, HDMI_PHY_CONF0,
@@ -746,6 +757,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
unsigned char prep,
u8 val, msec;
const struct mpll_config *mpll_cfg = hdmi-plat_data-mpll_cfg;
const struct curr_ctrl *curr_ctr = hdmi-plat_data-cur_ctr;
+ const struct sym_term *sym_term = hdmi-plat_data-sym_term;
if (prep)
return -EINVAL;
@@ -815,10 +827,17 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
unsigned char prep,
hdmi_phy_i2c_write(hdmi, 0x, 0x13); /* PLLPHBYCTRL */
hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
+
+ for (i = 0; sym_term[i].mpixelclock != (~0UL); i++)
+ if (hdmi-hdmi_data.video_mode.mpixelclock =
+ sym_term[i].mpixelclock)
+ break;
+
/* RESISTANCE TERM 133Ohm Cfg */
- hdmi_phy_i2c_write(hdmi, 0x0005, 0x19); /* TXTERM */
+ hdmi_phy_i2c_write(hdmi, sym_term[i].term, 0x19); /* TXTERM */
/* PREEMP Cgf 0.00 */
- hdmi_phy_i2c_write(hdmi, 0x800d, 0x09); /* CKSYMTXCTRL */
+ hdmi_phy_i2c_write(hdmi, sym_term[i].sym_ctr, 0x09); /* CKSYMTXCTRL */
+
All these seem generic improvements and not rk3288 specific?
/* TX/CK LVL 10 */
hdmi_phy_i2c_write(hdmi, 0x01ad, 0x0E); /* VLEVCTRL */
/* REMOVE CLK TERM */
@@ -834,6 +853,9 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi,
unsigned char prep,
dw_hdmi_phy_gen2_txpwron(hdmi, 1);
dw_hdmi_phy_gen2_pddq(hdmi, 0);
+ if (hdmi-dev_type == RK3288_HDMI)
+ dw_hdmi_phy_enable_spare(hdmi, 1);
+
/*Wait for PHY PLL lock */
msec = 5;
do {
@@ -1398,6 +1420,20 @@ static int dw_hdmi_connector_get_modes(struct
drm_connector *connector)
return 0;
}
+static enum drm_mode_status
+dw_hdmi_connector_mode_valid(struct drm_connector *connector,
+ struct drm_display_mode *mode)
+{
+ struct dw_hdmi *hdmi = container_of(connector,
+ struct dw_hdmi, connector);
+ enum drm_mode_status mode_status = MODE_OK;
+
+ if (hdmi-plat_data-mode_valid)
+ mode_status = hdmi-plat_data-mode_valid(connector, mode);
+
+ return mode_status;
+}
+
This too is disjoint from rk3288 support.
static struct drm_encoder *dw_hdmi_connector_best_encoder(struct
drm_connector
*connector)
{
@@ -1422,6 +1458,7 @@ static struct drm_connector_funcs
Re: [PATCH v7 5/7] drm: bridge/dw-hdmi: add support for multi byte register width access
Hi Andy,
This patch adds the reg-io-width binding.
Hence the binding patch should come before it.
On 11/11/14 12:53, Andy Yan wrote:
On rockchip rk3288, only word(32-bit) accesses are
permitted for hdmi registers. Byte width accesses (writeb,
readb) generate an imprecise external abort.
Signed-off-by: Andy Yan andy@rock-chips.com
---
}
static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
@@ -1499,6 +1527,23 @@ static int dw_hdmi_bind(struct device *dev, struct
device *master, void *data)
struct device_node *ddc_node;
struct resource *iores;
int ret, irq;
+ u32 val = 1;
+
+ of_property_read_u32(np, reg-io-width, val);
+
+ switch (val) {
+ case 4:
+ hdmi-write = dw_hdmi_writel;
+ hdmi-read = dw_hdmi_readl;
+ break;
+ case 1:
+ hdmi-write = dw_hdmi_writeb;
+ hdmi-read = dw_hdmi_readb;
+ break;
+ default:
+ dev_err(dev, reg-io-width must be 1 or 4\n);
+ return -EINVAL;
+ }
The binding patch says this is an optional property.
But here if undefined it returns -EINVAL.
I would keep it optional and default it to byte access.
Regards,
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 6/7] drm: bridge/dw-hdmi: convert dw-hdmi to drm_bridge mode
Hi Andy, On 11/11/14 12:54, Andy Yan wrote: From: Yakir Yang y...@rock-chips.com keep the connector birdge in dw_hdmi.c, handle encoder in dw_hdmi-imx.c Is there a reason for this separation? Keeping encoder in platform files? If yes, then the commit message should explain that. Signed-off-by: Andy Yan andy@rock-chips.com Signed-off-by: Yakir Yang y...@rock-chips.com --- Changes in v7: None Changes in v6: - move some modification from patch#5 Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: None drivers/gpu/drm/bridge/dw_hdmi.c | 228 +++--- drivers/staging/imx-drm/dw_hdmi-imx.c | 145 ++--- include/drm/bridge/dw_hdmi.h | 13 +- 3 files changed, 199 insertions(+), 187 deletions(-) Regards ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 5/7] drm: bridge/dw-hdmi: add support for multi byte register width access
On 11/11/14 14:23, Lucas Stach wrote:
Am Dienstag, den 11.11.2014, 14:20 + schrieb Zubair Lutfullah
Kakakhel:
Hi Andy,
This patch adds the reg-io-width binding.
Hence the binding patch should come before it.
On 11/11/14 12:53, Andy Yan wrote:
On rockchip rk3288, only word(32-bit) accesses are
permitted for hdmi registers. Byte width accesses (writeb,
readb) generate an imprecise external abort.
Signed-off-by: Andy Yan andy@rock-chips.com
---
}
static void hdmi_modb(struct dw_hdmi *hdmi, u8 data, u8 mask, unsigned reg)
@@ -1499,6 +1527,23 @@ static int dw_hdmi_bind(struct device *dev, struct
device *master, void *data)
struct device_node *ddc_node;
struct resource *iores;
int ret, irq;
+ u32 val = 1;
+
+ of_property_read_u32(np, reg-io-width, val);
+
+ switch (val) {
+ case 4:
+ hdmi-write = dw_hdmi_writel;
+ hdmi-read = dw_hdmi_readl;
+ break;
+ case 1:
+ hdmi-write = dw_hdmi_writeb;
+ hdmi-read = dw_hdmi_readb;
+ break;
+ default:
+ dev_err(dev, reg-io-width must be 1 or 4\n);
+ return -EINVAL;
+ }
The binding patch says this is an optional property.
But here if undefined it returns -EINVAL.
I would keep it optional and default it to byte access.
That's exactly what the patch does. val is initialized to 1, which is
not changed if the property could not be found in the DT. The default
case will only be taken if the property is present in DT but has any
other value than 1 or 4, which is an error.
I missed the initialization. My bad.
Thanks
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 7/7] dt-bindings: add document for dw-hdmi
Hi Andy,
Some minor comments inline.
On 11/11/14 12:54, Andy Yan wrote:
Signed-off-by: Andy Yan andy@rock-chips.com
---
Changes in v7: None
Changes in v6: None
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
.../devicetree/bindings/drm/bridge/dw-hdmi.txt | 38
++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
new file mode 100644
index 000..aa7ed17
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
@@ -0,0 +1,38 @@
+DesignWare HDMI bridge bindings
+
+Required properities:
^properties
+- compatibel: platform specific such as fsl,imx6q-hdmi,fsl,imx6dl-hdmi
^compatible
+ rockchip,rk3288-dw-hdmi
This will need to come in a separate series which adds rk3288 hdmi support..
+- reg: physical base address of the controller and length
+- ddc-i2c-bus: the ddc i2c bus
+- interrupts: The interrupt number to the cpu
+
+Optional properties
+- reg-io-width: the width of the reg:1,4, default set to 1 if not present
+
+Example:
+ hdmi: hdmi@012 {
+ compatible = fsl,imx6q-hdmi;
+ reg = 0x0012 0x9000;
+ interrupts = 0 115 0x04;
+ gpr = gpr;
+ clocks = clks 123, clks 124;
+ clock-names = iahb, isfr;
+ ddc-i2c-bus = i2c2;
+
+ port@0 {
+ reg = 0;
+
+ hdmi_mux_0: endpoint {
+ remote-endpoint = ipu1_di0_hdmi;
+ };
+ };
+
+ port@1 {
+ reg = 1;
+
+ hdmi_mux_1: endpoint {
+ remote-endpoint = ipu1_di1_hdmi;
+ };
+ };
+ };
Thanks
ZubairLK
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v7 5/7] drm: bridge/dw-hdmi: add support for multi byte register width access
On 11/11/14 14:46, Andy Yan wrote: On 2014年11月11日 22:20, Zubair Lutfullah Kakakhel wrote: Hi Andy, This patch adds the reg-io-width binding. Hence the binding patch should come before it. do you mean that I should put dts binding patch before this patch? Yes. The patch adding the binding to the Documentation folder comes before the driver implementing the binding. ZubairLK On 11/11/14 12:53, Andy Yan wrote: On rockchip rk3288, only word(32-bit) accesses are permitted for hdmi registers. Byte width accesses (writeb, readb) generate an imprecise external abort. ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 7/7] dt-bindings: add document for dw-hdmi
Nice work.
This patch should move the binding from
Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
to the location instead of making a new file and leaving the old
one in place.
And use git format-patch -M to highlight any changes.
Regards
ZubairLK
On 08/11/14 05:32, Andy Yan wrote:
Signed-off-by: Andy Yan andy@rock-chips.com
---
.../devicetree/bindings/drm/bridge/dw-hdmi.txt | 38
++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
new file mode 100644
index 000..aa7ed17
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
@@ -0,0 +1,38 @@
+DesignWare HDMI bridge bindings
+
+Required properities:
+- compatibel: platform specific such as fsl,imx6q-hdmi,fsl,imx6dl-hdmi
+ rockchip,rk3288-dw-hdmi
+- reg: physical base address of the controller and length
+- ddc-i2c-bus: the ddc i2c bus
+- interrupts: The interrupt number to the cpu
+
+Optional properties
+- reg-io-width: the width of the reg:1,4, default set to 1 if not present
+
+Example:
+ hdmi: hdmi@012 {
+ compatible = fsl,imx6q-hdmi;
+ reg = 0x0012 0x9000;
+ interrupts = 0 115 0x04;
+ gpr = gpr;
+ clocks = clks 123, clks 124;
+ clock-names = iahb, isfr;
+ ddc-i2c-bus = i2c2;
+
+ port@0 {
+ reg = 0;
+
+ hdmi_mux_0: endpoint {
+ remote-endpoint = ipu1_di0_hdmi;
+ };
+ };
+
+ port@1 {
+ reg = 1;
+
+ hdmi_mux_1: endpoint {
+ remote-endpoint = ipu1_di1_hdmi;
+ };
+ };
+ };
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 3/7] dw-hdmi: make checkpatch happy
Hi Andy,
In 3.18 rc4, I cannot find checkpatch errors in imx-hdmi.c
Have these errors come during the previous 2 patches.
If yes, then these changes need to be squashed into the previous
patches. No patch should add a checkpatch error and then fix it in a later
patch.
Regards
ZubairLK
On 08/11/14 05:29, Andy Yan wrote:
CHECK: Alignment should match open parenthesis
+ if ((hdmi-vic == 10) || (hdmi-vic == 11) ||
+ (hdmi-vic == 12) || (hdmi-vic == 13) ||
CHECK: braces {} should be used on all arms of this statement
+ if (hdmi-hdmi_data.video_mode.mdvi)
[...]
+ else {
[...]
Signed-off-by: Andy Yan andy@rock-chips.com
---
drivers/gpu/drm/bridge/dw_hdmi.c | 97
1 file changed, 48 insertions(+), 49 deletions(-)
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 7/7] dt-bindings: add document for dw-hdmi
On 10/11/14 09:44, Andy Yan wrote:
Hi ZubairLK
On 2014年11月10日 17:17, Zubair Lutfullah Kakakhel wrote:
Nice work.
This patch should move the binding from
Documentation/devicetree/bindings/staging/imx-drm/hdmi.txt
to the location instead of making a new file and leaving the old
one in place.
And use git format-patch -M to highlight any changes.
Regards
ZubairLK
I add these bindings for dw-hdmi core, because
properities like reg, interrupts, ddc-i2c-bus, reg-io-width are used
by dw-hdmi core, but properities like compatible, gpr,clk, are platform
specific , they are handled by the platform code like dw_hdmi-imx.c
and described in platform specific dt binds like /imx-drm/hdmi.txt
I understand.
Are there any changes to imx-drm/hdmi.txt?
Some properties for imx-drm/hdmi.txt would become optional/inherited from the
parent hdmi drm binding?
The changes should be highlighted in the commit message. Or we'll
be confused which is which..
ZubairLK
On 08/11/14 05:32, Andy Yan wrote:
Signed-off-by: Andy Yan andy@rock-chips.com
---
.../devicetree/bindings/drm/bridge/dw-hdmi.txt | 38
++
1 file changed, 38 insertions(+)
create mode 100644
Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
diff --git a/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
new file mode 100644
index 000..aa7ed17
--- /dev/null
+++ b/Documentation/devicetree/bindings/drm/bridge/dw-hdmi.txt
@@ -0,0 +1,38 @@
+DesignWare HDMI bridge bindings
+
+Required properities:
+- compatibel: platform specific such as fsl,imx6q-hdmi,fsl,imx6dl-hdmi
spellcheck. compatible
+ rockchip,rk3288-dw-hdmi
+- reg: physical base address of the controller and length
+- ddc-i2c-bus: the ddc i2c bus
+- interrupts: The interrupt number to the cpu
+
+Optional properties
+- reg-io-width: the width of the reg:1,4, default set to 1 if not present
+
+Example:
+hdmi: hdmi@012 {
+compatible = fsl,imx6q-hdmi;
+reg = 0x0012 0x9000;
+interrupts = 0 115 0x04;
+gpr = gpr;
+clocks = clks 123, clks 124;
+clock-names = iahb, isfr;
+ddc-i2c-bus = i2c2;
+
+port@0 {
+reg = 0;
+
+hdmi_mux_0: endpoint {
+remote-endpoint = ipu1_di0_hdmi;
+};
+};
+
+port@1 {
+reg = 1;
+
+hdmi_mux_1: endpoint {
+remote-endpoint = ipu1_di1_hdmi;
+};
+};
+};
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH V5 1/7] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi
Hi Andy,
A few comments inline.
On 08/11/14 05:28, Andy Yan wrote:
imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS)
use the interface compatible Designware HDMI IP, but they
also have some lightly difference, such as phy pll configuration,
register width, 4K support, clk useage, and the crtc mux configuration
is also platform specific.
To reuse the imx hdmi driver, split the platform specific code out
to dw_hdmi-imx.c.
Signed-off-by: Andy Yan andy@rock-chips.com
---
drivers/staging/imx-drm/Makefile | 2 +-
drivers/staging/imx-drm/dw_hdmi-imx.c | 214
drivers/staging/imx-drm/imx-hdmi.c| 257
--
drivers/staging/imx-drm/imx-hdmi.h| 43 ++
4 files changed, 320 insertions(+), 196 deletions(-)
create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c
diff --git a/drivers/staging/imx-drm/Makefile
b/drivers/staging/imx-drm/Makefile
index 582c438..809027d 100644
--- a/drivers/staging/imx-drm/Makefile
+++ b/drivers/staging/imx-drm/Makefile
@@ -9,4 +9,4 @@ obj-$(CONFIG_DRM_IMX_LDB) += imx-ldb.o
imx-ipuv3-crtc-objs := ipuv3-crtc.o ipuv3-plane.o
obj-$(CONFIG_DRM_IMX_IPUV3) += imx-ipuv3-crtc.o
-obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o
+obj-$(CONFIG_DRM_IMX_HDMI) += imx-hdmi.o dw_hdmi-imx.o
diff --git a/drivers/staging/imx-drm/dw_hdmi-imx.c
b/drivers/staging/imx-drm/dw_hdmi-imx.c
new file mode 100644
index 000..5422679
--- /dev/null
+++ b/drivers/staging/imx-drm/dw_hdmi-imx.c
@@ -0,0 +1,214 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
Please add the old freescale copyrights and a comment on how this file is
derived from.
And a note on platform specific file for imx hdmi using dwc hdmi drm bridge.
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/mfd/syscon.h
+#include linux/mfd/syscon/imx6q-iomuxc-gpr.h
+#include video/imx-ipu-v3.h
+#include linux/regmap.h
+#include linux/clk.h
+
+#include imx-drm.h
+#include imx-hdmi.h
+
+struct imx_hdmi_priv {
+ struct device *dev;
+ struct clk *isfr_clk;
+ struct clk *iahb_clk;
+ struct regmap *regmap;
+};
+
+static const struct mpll_config imx_mpll_cfg[] = {
+ {
+ 4525, {
+ { 0x01e0, 0x },
+ { 0x21e1, 0x },
+ { 0x41e2, 0x }
+ },
+ }, {
+ 9250, {
+ { 0x0140, 0x0005 },
+ { 0x2141, 0x0005 },
+ { 0x4142, 0x0005 },
+ },
+ }, {
+ 14850, {
+ { 0x00a0, 0x000a },
+ { 0x20a1, 0x000a },
+ { 0x40a2, 0x000a },
+ },
+ }, {
+ ~0UL, {
+ { 0x00a0, 0x000a },
+ { 0x2001, 0x000f },
+ { 0x4002, 0x000f },
+ },
+ }
+};
+
+static const struct curr_ctrl imx_cur_ctr[] = {
+ /* pixelclk bpp8bpp10 bpp12 */
+ {
+ 5400, { 0x091c, 0x091c, 0x06dc },
+ }, {
+ 5840, { 0x091c, 0x06dc, 0x06dc },
+ }, {
+ 7200, { 0x06dc, 0x06dc, 0x091c },
+ }, {
+ 7425, { 0x06dc, 0x0b5c, 0x091c },
+ }, {
+ 11880, { 0x091c, 0x091c, 0x06dc },
+ }, {
+ 21600, { 0x06dc, 0x0b5c, 0x091c },
+ }
+};
These pll clocks. Are they completely different?
I assume they are dependent on display resolutions. The old
ones should remain the same. With new additions for 4K clock
rates e.g.
+
+static int imx_hdmi_parse_dt(struct imx_hdmi_priv *hdmi)
+{
+ struct device_node *np = hdmi-dev-of_node;
+
+ hdmi-regmap = syscon_regmap_lookup_by_phandle(np, gpr);
+ if (IS_ERR(hdmi-regmap)) {
+ dev_err(hdmi-dev, Unable to get gpr\n);
+ return PTR_ERR(hdmi-regmap);
+ }
+
+ hdmi-isfr_clk = devm_clk_get(hdmi-dev, isfr);
+ if (IS_ERR(hdmi-isfr_clk)) {
+ dev_err(hdmi-dev, Unable to get HDMI isfr clk\n);
+ return PTR_ERR(hdmi-isfr_clk);
+ }
+
+ hdmi-iahb_clk = devm_clk_get(hdmi-dev, iahb);
+ if (IS_ERR(hdmi-iahb_clk)) {
+ dev_err(hdmi-dev, Unable to get HDMI iahb clk\n);
+ return PTR_ERR(hdmi-iahb_clk);
+ }
+
These two clocks. This is how they are structured on the jz4780 as well.
From an earlier patch series
yes, this IP core need to be clocked. But different Soc has different
usage of the clk, on freescale imx platform one clk is used for isfr, one is
used for iahb,
but on rockchip rk3288, one clk is used for control logic , the another is used
for hdcp.
I am not sure how are the clocks on jz4780
They might be
Re: [PATCH V2 1/2] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi
This one patch does too much to be reviewed easily.
One patch is supposed to modify/add one thing at a time in the kernel.
Separating platform specific code from imx-drm/imx-hdmi is one thing.
Adding support for multi-byte register access is something different.
i.e. Something like.
1/3 split platform specific code out.
2/3 move/rename imx-hdmi outside the folder
3/3 add support for multi byte register width access.
If there are other things that are not directly relevant to the patch,
it goes in a different patch. Bug fixes are also separate.
This should result in readable patches that can be reviewed easily.
Also, the approach with 4 byte access is ok. But you could use reg-shifts as
well perhaps.
Then you won't have to change so much of the code.
static inline void hdmi_writeb(struct dwc_hdmi *hdmi, u8 val, int offset)
+{
+ writeb(val, hdmi-regs + (offset hdmi-reg_shift));
+}
+
+static inline u8 hdmi_readb(struct dwc_hdmi *hdmi, int offset)
+{
+ return readb(hdmi-regs + (offset hdmi-reg_shift));
+}
And then in probe
+hdmi-reg_shift = 0;
+
+ if (of_property_read_u32(np, reg-shift, hdmi-reg_shift))
+ dev_warn(hdmi-dev, No reg-shift\n);
This way the reg-shift property can be defined using DT
Cheers,
ZubairLK
On 05/11/14 12:59, Andy Yan wrote:
imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS)
use the interface compatible Designware HDMI IP, but they
also have some lightly difference, such as phy pll configuration,
register width(imx hdmi register is one byte, but rk3288 is 4
bytes width), 4K support(imx6 doesn't support 4k, but rk3288 does),
clk useage,and the crtc mux configuration is also platform specific.
To reuse the imx hdmi driver, split the platform specific code out
to dw_hdmi-imx.c.
Signed-off-by: Andy Yan andy@rock-chips.com
---
drivers/staging/imx-drm/Makefile | 2 +-
drivers/staging/imx-drm/dw_hdmi-imx.c | 214 ++
drivers/staging/imx-drm/imx-hdmi.c| 726
++
include/drm/bridge/dw_hdmi.h | 114 ++
4 files changed, 634 insertions(+), 422 deletions(-)
create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c
create mode 100644 include/drm/bridge/dw_hdmi.h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 2/2] move imx-hdmi to bridge/dw-hdmi
Hi, On 04/11/14 13:39, Andy Yan wrote: From: Andy yan andy@rock-chips.com the original imx hdmi driver is under staging/imx-drm, which depends on imx-drm, so move the imx hdmi drvier out to drm/bridge and rename imx-hdmi to dw-hdmi Change-Id: I5f417372f256aa26cd00a3cd0160741680afd39d --- drivers/gpu/drm/bridge/Kconfig|5 + drivers/gpu/drm/bridge/Makefile |1 + drivers/gpu/drm/bridge/dw_hdmi.c | 1651 + drivers/gpu/drm/bridge/dw_hdmi.h | 1032 + drivers/staging/imx-drm/Kconfig |1 + drivers/staging/imx-drm/Makefile |2 +- drivers/staging/imx-drm/dw_hdmi-imx.c | 66 +- drivers/staging/imx-drm/imx-hdmi.c| 1651 - drivers/staging/imx-drm/imx-hdmi.h| 1032 - include/drm/bridge/dw_hdmi.h | 26 +- 10 files changed, 2737 insertions(+), 2730 deletions(-) create mode 100644 drivers/gpu/drm/bridge/dw_hdmi.c create mode 100644 drivers/gpu/drm/bridge/dw_hdmi.h delete mode 100644 drivers/staging/imx-drm/imx-hdmi.c delete mode 100644 drivers/staging/imx-drm/imx-hdmi.h Could you try generating this patch with the -M option? git format-patch should then understand its a rename. Then it should generate a smaller patch that is possible to review easily. Thanks ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH 1/2] imx-drm: imx-hdmi: split imx soc specific code from imx-hdmi
Hi Andy, On 04/11/14 13:33, Andy Yan wrote: imx6 and rockchip rk3288 and JZ4780 (Ingenic Xburst/MIPS) use the interface compatible Designware HDMI IP, but they also have some lightly difference, such as phy pll configuration, register width(imx hdmi register is one byte, but rk3288 is 4 bytes width), 4K support(imx6 doesn't support 4k, but rk3288 does), clk useage,and the crtc mux configuration is also platform specific. To reuse the imx hdmi driver, split the platform specific code out to dw_hdmi-imx.c. Change-Id: I85e8d08754052b118423729a01c6d17bf485f383 --- drivers/staging/imx-drm/Makefile | 2 +- drivers/staging/imx-drm/dw_hdmi-imx.c | 214 ++ drivers/staging/imx-drm/imx-hdmi.c| 726 ++ include/drm/bridge/dw_hdmi.h | 114 ++ 4 files changed, 634 insertions(+), 422 deletions(-) create mode 100644 drivers/staging/imx-drm/dw_hdmi-imx.c create mode 100644 include/drm/bridge/dw_hdmi.h This one patch does too much to be reviewed easily. One patch is supposed to modify/add one thing at a time in the kernel. Separating platform specific code from imx-drm/imx-hdmi is one thing. Adding support for multi-byte register access is something different. i.e. Something like. 1/3 split platform specific code out. 2/3 move/rename imx-hdmi outside the folder 3/3 add support for multi byte register width access. If there are other things that are not directly relevant to the patch, it goes in a different patch. Bug fixes are also separate. This should result in readable patches that can be reviewed easily. The maintainers might be able to guide on how to further split the patches if necessary. Cheers, ZubairLK ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
