[PATCH] drm/modes: Fix outdated drm_mode_vrefresh return value documentation
The vrefresh field in drm_display_mode struct was removed so the function no longer checks if it is set before calculating it. Fixes: 0425662fdf05 ("drm: Nuke mode->vrefresh") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/drm_modes.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c index 3c8034a8c27b..2d51ab2734a0 100644 --- a/drivers/gpu/drm/drm_modes.c +++ b/drivers/gpu/drm/drm_modes.c @@ -803,8 +803,7 @@ EXPORT_SYMBOL(drm_mode_set_name); * @mode: mode * * Returns: - * @modes's vrefresh rate in Hz, rounded to the nearest integer. Calculates the - * value first if it is not yet set. + * @modes's vrefresh rate in Hz, rounded to the nearest integer. */ int drm_mode_vrefresh(const struct drm_display_mode *mode) { -- 2.38.1
Re: [PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
Hi Marek, On Mon, 23 May 2022 at 23:15, Marek Vasut wrote: > > On 5/23/22 15:01, Jonathan Liu wrote: > > The code from [1] sets SYS_CTRL_1 to different values depending on the > > desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the > > positive edge of the clock with the pixel data while other values delay > > the clock by a fraction of the clock period. A clock phase of 1/2 aligns > > the negative edge of the clock with the pixel data. > > > > The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to > > aligning the positive edge of the clock with the pixel data. This won't > > work correctly for panels that require aligning the negative edge of the > > clock with the pixel data. > > > > Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is > > present in bus_flags, otherwise adjust the clock phase to 1/2 as > > appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE. > > > > [1] https://github.com/tdjastrzebski/ICN6211-Configurator > > > > Signed-off-by: Jonathan Liu > > --- > > V2: Use GENMASK and FIELD_PREP macros > > --- > > drivers/gpu/drm/bridge/chipone-icn6211.c | 18 -- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c > > b/drivers/gpu/drm/bridge/chipone-icn6211.c > > index 47dea657a752..f1538fb5f8a9 100644 > > --- a/drivers/gpu/drm/bridge/chipone-icn6211.c > > +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c > > @@ -9,6 +9,8 @@ > > #include > > #include > > > > +#include > > +#include > > #include > > #include > > #include > > @@ -26,6 +28,11 @@ > > #define PD_CTRL(n) (0x0a + ((n) & 0x3)) /* 0..3 */ > > #define RST_CTRL(n) (0x0e + ((n) & 0x1)) /* 0..1 */ > > #define SYS_CTRL(n) (0x10 + ((n) & 0x7)) /* 0..4 */ > > +#define SYS_CTRL_1_CLK_PHASE_MSK GENMASK(5, 4) > > This should be GENMASK(7, 6) , no ? Clock phase 0 = 0b_1000_1000 = 0x88 Clock phase 1/4 = 0b_1001_1000 = 0x98 Clock phase 1/2 = 0b_1010_1000 = 0xA8 Clock phase 3/4 = 0b_1011_1000 = 0xB8 The clock phase bits are 5:4 not 7:6. The upper 2 bits and lower 4 bits are unknown. Regards, Jonathan
[PATCH v2] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
The code from [1] sets SYS_CTRL_1 to different values depending on the desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the positive edge of the clock with the pixel data while other values delay the clock by a fraction of the clock period. A clock phase of 1/2 aligns the negative edge of the clock with the pixel data. The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to aligning the positive edge of the clock with the pixel data. This won't work correctly for panels that require aligning the negative edge of the clock with the pixel data. Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is present in bus_flags, otherwise adjust the clock phase to 1/2 as appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE. [1] https://github.com/tdjastrzebski/ICN6211-Configurator Signed-off-by: Jonathan Liu --- V2: Use GENMASK and FIELD_PREP macros --- drivers/gpu/drm/bridge/chipone-icn6211.c | 18 -- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 47dea657a752..f1538fb5f8a9 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -9,6 +9,8 @@ #include #include +#include +#include #include #include #include @@ -26,6 +28,11 @@ #define PD_CTRL(n) (0x0a + ((n) & 0x3)) /* 0..3 */ #define RST_CTRL(n)(0x0e + ((n) & 0x1)) /* 0..1 */ #define SYS_CTRL(n)(0x10 + ((n) & 0x7)) /* 0..4 */ +#define SYS_CTRL_1_CLK_PHASE_MSK GENMASK(5, 4) +#define CLK_PHASE_00 +#define CLK_PHASE_1_4 1 +#define CLK_PHASE_1_2 2 +#define CLK_PHASE_3_4 3 #define RGB_DRV(n) (0x18 + ((n) & 0x3)) /* 0..3 */ #define RGB_DLY(n) (0x1c + ((n) & 0x1)) /* 0..1 */ #define RGB_TEST_CTRL 0x1e @@ -336,7 +343,7 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, const struct drm_bridge_state *bridge_state; u16 hfp, hbp, hsync; u32 bus_flags; - u8 pol, id[4]; + u8 pol, sys_ctrl_1, id[4]; chipone_readb(icn, VENDOR_ID, id); chipone_readb(icn, DEVICE_ID_H, id + 1); @@ -414,7 +421,14 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, chipone_configure_pll(icn, mode); chipone_writeb(icn, SYS_CTRL(0), 0x40); - chipone_writeb(icn, SYS_CTRL(1), 0x88); + sys_ctrl_1 = 0x88; + + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) + sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_0); + else + sys_ctrl_1 |= FIELD_PREP(SYS_CTRL_1_CLK_PHASE_MSK, CLK_PHASE_1_2); + + chipone_writeb(icn, SYS_CTRL(1), sys_ctrl_1); /* icn6211 specific sequence */ chipone_writeb(icn, MIPI_FORCE_0, 0x20); -- 2.36.1
[PATCH] drm: bridge: icn6211: Adjust clock phase using SYS_CTRL_1
The code from [1] sets SYS_CTRL_1 to different values depending on the desired clock phase (0, 1/4, 1/2 or 3/4). A clock phase of 0 aligns the positive edge of the clock with the pixel data while other values delay the clock by a fraction of the clock period. A clock phase of 1/2 aligns the negative edge of the clock with the pixel data. The driver currently hard codes SYS_CTRL_1 to 0x88 which corresponds to aligning the positive edge of the clock with the pixel data. This won't work correctly for panels that require aligning the negative edge of the clock with the pixel data. Adjust the clock phase to 0 if DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE is present in bus_flags, otherwise adjust the clock phase to 1/2 as appropriate for DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE. [1] https://github.com/tdjastrzebski/ICN6211-Configurator Signed-off-by: Jonathan Liu --- drivers/gpu/drm/bridge/chipone-icn6211.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/bridge/chipone-icn6211.c b/drivers/gpu/drm/bridge/chipone-icn6211.c index 47dea657a752..df0290059aa3 100644 --- a/drivers/gpu/drm/bridge/chipone-icn6211.c +++ b/drivers/gpu/drm/bridge/chipone-icn6211.c @@ -57,6 +57,10 @@ #define BIST_CHESS_XY_H0x30 #define BIST_FRAME_TIME_L 0x31 #define BIST_FRAME_TIME_H 0x32 +#define CLK_PHASE_00x88 +#define CLK_PHASE_1_4 0x98 +#define CLK_PHASE_1_2 0xa8 +#define CLK_PHASE_3_4 0xb8 #define FIFO_MAX_ADDR_LOW 0x33 #define SYNC_EVENT_DLY 0x34 #define HSW_MIN0x35 @@ -414,7 +418,11 @@ static void chipone_atomic_enable(struct drm_bridge *bridge, chipone_configure_pll(icn, mode); chipone_writeb(icn, SYS_CTRL(0), 0x40); - chipone_writeb(icn, SYS_CTRL(1), 0x88); + + if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_POSEDGE) + chipone_writeb(icn, SYS_CTRL(1), CLK_PHASE_0); + else + chipone_writeb(icn, SYS_CTRL(1), CLK_PHASE_1_2); /* icn6211 specific sequence */ chipone_writeb(icn, MIPI_FORCE_0, 0x20); -- 2.36.1
Re: [PATCH] drm/bridge: dw-mipi-dsi: Move drm_bridge_add into probe
Hi Jagan, On Thu, 24 Jun 2021 at 22:34, Jagan Teki wrote: > > Hi Jonathan, > > On Fri, Jun 18, 2021 at 6:40 PM Jonathan Liu wrote: > > > > Hi Jagan, > > > > On Wed, 3 Feb 2021 at 09:13, Jagan Teki wrote: > > > @@ -1167,6 +1151,20 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, > > > dw_mipi_dsi_debugfs_init(dsi); > > > pm_runtime_enable(dev); > > > > > > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, > > > + &panel, &bridge); > > > + if (ret) > > > + return ERR_PTR(ret); > > > > On RK3399 if the error is EPROBE_DEFER, __dw_mipi_dsi_probe can be > > called again and result in the following errors: > > [0.717589] debugfs: Directory 'ff96.mipi' with parent '/' > > already present! > > [0.717601] dw-mipi-dsi-rockchip ff96.mipi: failed to create debugfs > > root > > [0.717606] dw-mipi-dsi-rockchip ff96.mipi: Unbalanced > > pm_runtime_enable! > > Is this when you test bridge on rk3399 or panel? MIPI-DSI to LVDS bridge. Regards, Jonathan
Re: [PATCH] drm/bridge: dw-mipi-dsi: Move drm_bridge_add into probe
Hi Jagan, On Wed, 3 Feb 2021 at 09:13, Jagan Teki wrote: > @@ -1167,6 +1151,20 @@ __dw_mipi_dsi_probe(struct platform_device *pdev, > dw_mipi_dsi_debugfs_init(dsi); > pm_runtime_enable(dev); > > + ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, > + &panel, &bridge); > + if (ret) > + return ERR_PTR(ret); On RK3399 if the error is EPROBE_DEFER, __dw_mipi_dsi_probe can be called again and result in the following errors: [0.717589] debugfs: Directory 'ff96.mipi' with parent '/' already present! [0.717601] dw-mipi-dsi-rockchip ff96.mipi: failed to create debugfs root [0.717606] dw-mipi-dsi-rockchip ff96.mipi: Unbalanced pm_runtime_enable! Regards, Jonathan
Re: [PATCH] drm/bridge: ti-sn65dsi83: Fix null pointer dereference in remove callback
Hi Marek, On Fri, 18 Jun 2021 at 00:14, Laurent Pinchart wrote: > > Hi Jonathan, > > Thank you for the patch. > > On Thu, Jun 17, 2021 at 09:19:25PM +1000, Jonathan Liu wrote: > > If attach has not been called, unloading the driver can result in a null > > pointer dereference in mipi_dsi_detach as ctx->dsi has not been assigned > > yet. > > Shouldn't this be done in a brige .detach() operation instead ? > Could you please take a look? I don't have a working setup to test moving the code to detach. > > Fixes: ceb515ba29ba6b ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and > > SN65DSI84 driver") > > Signed-off-by: Jonathan Liu > > --- > > drivers/gpu/drm/bridge/ti-sn65dsi83.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > index 750f2172ef08..8e9f45c5c7c1 100644 > > --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c > > @@ -671,8 +671,11 @@ static int sn65dsi83_remove(struct i2c_client *client) > > { > > struct sn65dsi83 *ctx = i2c_get_clientdata(client); > > > > - mipi_dsi_detach(ctx->dsi); > > - mipi_dsi_device_unregister(ctx->dsi); > > + if (ctx->dsi) { > > + mipi_dsi_detach(ctx->dsi); > > + mipi_dsi_device_unregister(ctx->dsi); > > + } > > + > > drm_bridge_remove(&ctx->bridge); > > of_node_put(ctx->host_node); > > Thanks. Regards, Jonathan
[PATCH] drm/bridge: ti-sn65dsi83: Fix null pointer dereference in remove callback
If attach has not been called, unloading the driver can result in a null pointer dereference in mipi_dsi_detach as ctx->dsi has not been assigned yet. Fixes: ceb515ba29ba6b ("drm/bridge: ti-sn65dsi83: Add TI SN65DSI83 and SN65DSI84 driver") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/bridge/ti-sn65dsi83.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi83.c b/drivers/gpu/drm/bridge/ti-sn65dsi83.c index 750f2172ef08..8e9f45c5c7c1 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi83.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi83.c @@ -671,8 +671,11 @@ static int sn65dsi83_remove(struct i2c_client *client) { struct sn65dsi83 *ctx = i2c_get_clientdata(client); - mipi_dsi_detach(ctx->dsi); - mipi_dsi_device_unregister(ctx->dsi); + if (ctx->dsi) { + mipi_dsi_detach(ctx->dsi); + mipi_dsi_device_unregister(ctx->dsi); + } + drm_bridge_remove(&ctx->bridge); of_node_put(ctx->host_node); -- 2.32.0
Re: [RESEND PATCH] drm/rockchip: dsi: move all lane config except LCDC mux to bind()
On Mon, 19 Apr 2021 at 02:04, Thomas Hebb wrote: > > When we first enable the DSI encoder, we currently program some per-chip > configuration that we look up in rk3399_chip_data based on the device > tree compatible we match. This data configures various parameters of the > MIPI lanes, including on RK3399 whether DSI1 is slaved to DSI0 in a > dual-mode configuration. It also selects which LCDC (i.e. VOP) to scan > out from. > > This causes a problem in RK3399 dual-mode configurations, though: panel > prepare() callbacks run before the encoder gets enabled and expect to be > able to write commands to the DSI bus, but the bus isn't fully > functional until the lane and master/slave configuration have been > programmed. As a result, dual-mode panels (and possibly others too) fail > to turn on when the rockchipdrm driver is initially loaded. > > Because the LCDC mux is the only thing we don't know until enable time > (and is the only thing that can ever change), we can actually move most > of the initialization to bind() and get it out of the way early. That's > what this change does. (Rockchip's 4.4 BSP kernel does it in mode_set(), > which also avoids the issue, but bind() seems like the more correct > place to me.) > > Tested on a Google Scarlet board (Acer Chromebook Tab 10), which has a > Kingdisplay KD097D04 dual-mode panel. Prior to this change, the panel's > backlight would turn on but no image would appear when initially loading > rockchipdrm. If I kept rockchipdrm loaded and reloaded the panel driver, > it would come on. With this change, the panel successfully turns on > during initial rockchipdrm load as expected. > > Fixes: 2d4f7bdafd70 ("drm/rockchip: dsi: migrate to use dw-mipi-dsi bridge > driver") > Signed-off-by: Thomas Hebb Tested-by: Jonathan Liu Fixes MIPI-DSI panel initialization for me on RK3399 too. Regards, Jonathan
Re: [PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function
Hi Sam, On Sun, 8 Nov 2020 at 9:47 pm, Sam Ravnborg wrote: > Hi Russell, > > On Sun, Nov 08, 2020 at 09:57:25AM +, Russell King - ARM Linux admin > wrote: > > On Sun, Nov 08, 2020 at 10:53:22AM +0100, Sam Ravnborg wrote: > > > Russell, > > > > > > On Sat, Oct 31, 2020 at 07:17:47PM +1100, Jonathan Liu wrote: > > > > It has been observed that resetting force in the detect function can > > > > result in the PHY being powered down in response to hot-plug detect > > > > being asserted, even when the HDMI connector is forced on. > > > > > > > > Enabling debug messages and adding a call to dump_stack() in > > > > dw_hdmi_phy_power_off() shows the following in dmesg: > > > > [ 160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin > > > > [ 160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0 > iterations > > > > > > > > Call trace: > > > > dw_hdmi_phy_power_off > > > > dw_hdmi_phy_disable > > > > dw_hdmi_update_power > > > > dw_hdmi_detect > > > > dw_hdmi_connector_detect > > > > drm_helper_probe_detect_ctx > > > > drm_helper_hpd_irq_event > > > > dw_hdmi_irq > > > > irq_thread_fn > > > > irq_thread > > > > kthread > > > > ret_from_fork > > > > > > > > Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode > forcing") > > > > Signed-off-by: Jonathan Liu > > > > > > you are the original author of this code - any comments on this patch? > > > > No further comments beyond what has already been discussed, and the > > long and short of it is it's been so long that I don't remember why > > that code was there. Given that, I'm not even in a position to ack > > the change. Sorry. > Thanks for the quick reply. > > Given that this fixes a problem for Jonathan I will apply this to -fixes > if there is no other feedback the next couple of days. > If it introduces regression we can take it from there. > > Jonathan - please ping me if I forget. > > Sam > Ping. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function
Hi Sam, On Sun, 8 Nov 2020 at 21:47, Sam Ravnborg wrote: > > Hi Russell, > > On Sun, Nov 08, 2020 at 09:57:25AM +, Russell King - ARM Linux admin > wrote: > > On Sun, Nov 08, 2020 at 10:53:22AM +0100, Sam Ravnborg wrote: > > > Russell, > > > > > > On Sat, Oct 31, 2020 at 07:17:47PM +1100, Jonathan Liu wrote: > > > > It has been observed that resetting force in the detect function can > > > > result in the PHY being powered down in response to hot-plug detect > > > > being asserted, even when the HDMI connector is forced on. > > > > > > > > Enabling debug messages and adding a call to dump_stack() in > > > > dw_hdmi_phy_power_off() shows the following in dmesg: > > > > [ 160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin > > > > [ 160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0 > > > > iterations > > > > > > > > Call trace: > > > > dw_hdmi_phy_power_off > > > > dw_hdmi_phy_disable > > > > dw_hdmi_update_power > > > > dw_hdmi_detect > > > > dw_hdmi_connector_detect > > > > drm_helper_probe_detect_ctx > > > > drm_helper_hpd_irq_event > > > > dw_hdmi_irq > > > > irq_thread_fn > > > > irq_thread > > > > kthread > > > > ret_from_fork > > > > > > > > Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode forcing") > > > > Signed-off-by: Jonathan Liu > > > > > > you are the original author of this code - any comments on this patch? > > > > No further comments beyond what has already been discussed, and the > > long and short of it is it's been so long that I don't remember why > > that code was there. Given that, I'm not even in a position to ack > > the change. Sorry. > Thanks for the quick reply. > > Given that this fixes a problem for Jonathan I will apply this to -fixes > if there is no other feedback the next couple of days. > If it introduces regression we can take it from there. > > Jonathan - please ping me if I forget. > > Sam Ping. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: bridge: dw-hdmi: Avoid resetting force in the detect function
It has been observed that resetting force in the detect function can result in the PHY being powered down in response to hot-plug detect being asserted, even when the HDMI connector is forced on. Enabling debug messages and adding a call to dump_stack() in dw_hdmi_phy_power_off() shows the following in dmesg: [ 160.637413] dwhdmi-rockchip ff94.hdmi: EVENT=plugin [ 160.637433] dwhdmi-rockchip ff94.hdmi: PHY powered down in 0 iterations Call trace: dw_hdmi_phy_power_off dw_hdmi_phy_disable dw_hdmi_update_power dw_hdmi_detect dw_hdmi_connector_detect drm_helper_probe_detect_ctx drm_helper_hpd_irq_event dw_hdmi_irq irq_thread_fn irq_thread kthread ret_from_fork Fixes: 381f05a7a842 ("drm: bridge/dw_hdmi: add connector mode forcing") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c index 748df1cacd2b..0c79a9ba48bb 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c @@ -2327,12 +2327,6 @@ static enum drm_connector_status dw_hdmi_detect(struct dw_hdmi *hdmi) { enum drm_connector_status result; - mutex_lock(&hdmi->mutex); - hdmi->force = DRM_FORCE_UNSPECIFIED; - dw_hdmi_update_power(hdmi); - dw_hdmi_update_phy_mask(hdmi); - mutex_unlock(&hdmi->mutex); - result = hdmi->phy.ops->read_hpd(hdmi, hdmi->phy.data); mutex_lock(&hdmi->mutex); -- 2.29.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/rockchip: dw_hdmi: fix incorrect clock in vpll clock error message
Error message incorrectly refers to grf clock instead of vpll clock. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c index 23de359a1dec..830bdd5e9b7c 100644 --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c @@ -202,7 +202,7 @@ static int rockchip_hdmi_parse_dt(struct rockchip_hdmi *hdmi) } else if (PTR_ERR(hdmi->vpll_clk) == -EPROBE_DEFER) { return -EPROBE_DEFER; } else if (IS_ERR(hdmi->vpll_clk)) { - DRM_DEV_ERROR(hdmi->dev, "failed to get grf clock\n"); + DRM_DEV_ERROR(hdmi->dev, "failed to get vpll clock\n"); return PTR_ERR(hdmi->vpll_clk); } -- 2.29.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity
Hi Giulio, On Wed, 12 Dec 2018 at 04:20, Giulio Benetti wrote: > > Hi Jonathan, > > Il 11/12/2018 11:49, Jonathan Liu ha scritto: > > Hi Giulio, > > > > On Thu, 6 Dec 2018 at 22:00, Giulio Benetti > > wrote: > >> > >> Hi Jonathan, > >> > >> Il 06/12/2018 08:29, Jonathan Liu ha scritto: > >>> Hi Giulio, > >>> > >>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti > >>> wrote: > >>>> > >>>> Differently from other Lcd signals, HSYNC and VSYNC signals > >>>> result inverted if their bits are cleared to 0. > >>>> > >>>> Invert their settings of IO_POL register. > >>>> > >>>> Signed-off-by: Giulio Benetti > >>>> --- > >>>>drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > >>>>1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>> index 3c15cf2..aaf911a 100644 > >>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct > >>>> sun4i_tcon *tcon, > >>>>SUN4I_TCON0_BASIC3_H_SYNC(hsync)); > >>>> > >>>> /* Setup the polarity of the various signals */ > >>>> - if (!(mode->flags & DRM_MODE_FLAG_PHSYNC)) > >>>> + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > >>>> val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; > >>>> > >>>> - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) > >>>> + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > >>>> val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE; > >>>> > >>>> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG, > >>> > >>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7" > >>> LCD touchscreen (Innolux AT070TN92) connected to Olimex > >>> A20-OLinuXino-MICRO that the image does not display correctly after > >>> this change. > >>> The image is shifted to the right. > >>> > >>> Reverting the change results in the image being displayed correctly on > >>> the screen. > >>> > >>> I have in the device tree a "panel" node with compatible = > >>> "innolux,at070tn92" which uses the timings in > >>> drivers/gpu/drm/panel/panel-simple.c. > >>> > >>> Any ideas? > > > >> > >> Checking Display Datasheet: > >> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf > >> > >> Page 13 section 3.3.2 you can see it needs active low VS and HS. > >> > >> You can refer to this Thread and check scope captures about VS and HS > >> versus TCON0_IOPOL register: > >> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html > >> > >> There should be something that wrongly sets one of these or both: > >> mode->flags |= DRM_MODE_FLAG_PHSYNC; > >> and/or > >> mode->flags |= DRM_MODE_FLAG_PVSYNC; > >> > >> Checked in panel-simple.c but it's not there. > > > > flags is 0 because it is not assigned in the struct definition for the > > panel. > > I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to > 0x0300 but to 0. > What is checked is exactly mode->flags, so the problem seems to be upstream. > > This is my doubt, it seems mode->flags is not initialized or overriden > at a certain point, this is why I want to debug it with Jtag tomorrow. > If you look at the change made by your patch: --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon *tcon, SUN4I_TCON0_BASIC3_H_SYNC(hsync)); /* Setup the polarity of the various signals */ - if (!(mode->flags & DRM_MODE_FLAG_PHSYNC)) + if (mode->flags & DRM_MODE_FLAG_PHSYNC) val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) + if (mode->flags & DRM_MODE_FLAG_PVSYNC) val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE; regmap_update_bits(tcon->
Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity
Hi Giulio, On Wed, 12 Dec 2018 at 04:39, Giulio Benetti wrote: > > Forgot to ask you, > > Il 11/12/2018 18:20, Giulio Benetti ha scritto: > > Hi Jonathan, > > > > Il 11/12/2018 11:49, Jonathan Liu ha scritto: > >> Hi Giulio, > >> > >> On Thu, 6 Dec 2018 at 22:00, Giulio Benetti > >> wrote: > >>> > >>> Hi Jonathan, > >>> > >>> Il 06/12/2018 08:29, Jonathan Liu ha scritto: > >>>> Hi Giulio, > >>>> > >>>> On Thu, 15 Feb 2018 at 17:54, Giulio Benetti > >>>> wrote: > >>>>> > >>>>> Differently from other Lcd signals, HSYNC and VSYNC signals > >>>>> result inverted if their bits are cleared to 0. > >>>>> > >>>>> Invert their settings of IO_POL register. > >>>>> > >>>>> Signed-off-by: Giulio Benetti > >>>>> --- > >>>>>drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > >>>>>1 file changed, 2 insertions(+), 2 deletions(-) > >>>>> > >>>>> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>>> b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>>> index 3c15cf2..aaf911a 100644 > >>>>> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>>> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >>>>> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct > >>>>> sun4i_tcon *tcon, > >>>>>SUN4I_TCON0_BASIC3_H_SYNC(hsync)); > >>>>> > >>>>> /* Setup the polarity of the various signals */ > >>>>> - if (!(mode->flags & DRM_MODE_FLAG_PHSYNC)) > >>>>> + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > >>>>> val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; > >>>>> > >>>>> - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) > >>>>> + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > >>>>> val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE; > >>>>> > >>>>> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG, > >>>> > >>>> I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7" > >>>> LCD touchscreen (Innolux AT070TN92) connected to Olimex > >>>> A20-OLinuXino-MICRO that the image does not display correctly after > >>>> this change. > >>>> The image is shifted to the right. > >>>> > >>>> Reverting the change results in the image being displayed correctly on > >>>> the screen. > >>>> > >>>> I have in the device tree a "panel" node with compatible = > >>>> "innolux,at070tn92" which uses the timings in > >>>> drivers/gpu/drm/panel/panel-simple.c. > >>>> > >>>> Any ideas? > >> > >>> > >>> Checking Display Datasheet: > >>> https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf > >>> > >>> > >>> Page 13 section 3.3.2 you can see it needs active low VS and HS. > >>> > >>> You can refer to this Thread and check scope captures about VS and HS > >>> versus TCON0_IOPOL register: > >>> https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html > >>> > >>> > >>> There should be something that wrongly sets one of these or both: > >>> mode->flags |= DRM_MODE_FLAG_PHSYNC; > >>> and/or > >>> mode->flags |= DRM_MODE_FLAG_PVSYNC; > >>> > >>> Checked in panel-simple.c but it's not there. > >> > >> flags is 0 because it is not assigned in the struct definition for the > >> panel. > > > > I don't think it is 0, because otherwise IO_POL_REG wouldn't be set to > > 0x0300 but to 0. > > What is checked is exactly mode->flags, so the problem seems to be > > upstream. > > > > This is my doubt, it seems mode->flags is not initialized or overriden > > at a certain point, this is why I want to debug it with Jtag tomorrow. > > > >> Before this change, TCON0_IO_POL_REG would be 0x0300 (both bits > >> set to 1) and image displays correctly > After this change, > >> TCON0_IO_POL_REG is 0x (both bits set to 0) > >> and image doesn't display correctly. > >> > >> Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J. > > > > 0x0300 as I've triple checked with scope means Positive H/Vsync, > > and 0x Negative H/VSync. > > > > Please check on the Thread I've pointed you above where there are all > > the links to the scope captures. > > > > Are you completely sure you're using the correct panel? > > This is because if with 0x0300 it works correctly, it means that > > you're using Positive VS and HS but on datasheet on Figure 3.2 it shows > > that they must be negative. > > > > Do you have any chance to measure those signals with a scope? > > > > Tomorrow, while debugging, I'll re-check H/Vsync signals again. > > > > Kind regards > > Can you precisely point me the sources of: > - u-boot > - kernel > - dts > > you're using? > > Thanks U-Boot - git tag v2017.03 Linux - git tag v4.19.6 DTS - see device tree changes in https://github.com/net147/linux/tree/sun7i-drm-wip but change the compatible to "innolux,at070tn92" Thanks. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity
Hi Giulio, On Thu, 6 Dec 2018 at 22:00, Giulio Benetti wrote: > > Hi Jonathan, > > Il 06/12/2018 08:29, Jonathan Liu ha scritto: > > Hi Giulio, > > > > On Thu, 15 Feb 2018 at 17:54, Giulio Benetti > > wrote: > >> > >> Differently from other Lcd signals, HSYNC and VSYNC signals > >> result inverted if their bits are cleared to 0. > >> > >> Invert their settings of IO_POL register. > >> > >> Signed-off-by: Giulio Benetti > >> --- > >> drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> index 3c15cf2..aaf911a 100644 > >> --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > >> @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct > >> sun4i_tcon *tcon, > >> SUN4I_TCON0_BASIC3_H_SYNC(hsync)); > >> > >> /* Setup the polarity of the various signals */ > >> - if (!(mode->flags & DRM_MODE_FLAG_PHSYNC)) > >> + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > >> val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; > >> > >> - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) > >> + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > >> val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE; > >> > >> regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG, > > > > I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7" > > LCD touchscreen (Innolux AT070TN92) connected to Olimex > > A20-OLinuXino-MICRO that the image does not display correctly after > > this change. > > The image is shifted to the right. > > > > Reverting the change results in the image being displayed correctly on > > the screen. > > > > I have in the device tree a "panel" node with compatible = > > "innolux,at070tn92" which uses the timings in > > drivers/gpu/drm/panel/panel-simple.c. > > > > Any ideas? > > Checking Display Datasheet: > https://www.olimex.com/Products/Retired/A13-LCD7-TS/resources/S700-AT070TN92.pdf > > Page 13 section 3.3.2 you can see it needs active low VS and HS. > > You can refer to this Thread and check scope captures about VS and HS > versus TCON0_IOPOL register: > https://lists.freedesktop.org/archives/dri-devel/2018-January/163874.html > > There should be something that wrongly sets one of these or both: > mode->flags |= DRM_MODE_FLAG_PHSYNC; > and/or > mode->flags |= DRM_MODE_FLAG_PVSYNC; > > Checked in panel-simple.c but it's not there. flags is 0 because it is not assigned in the struct definition for the panel. Before this change, TCON0_IO_POL_REG would be 0x0300 (both bits set to 1) and image displays correctly. After this change, TCON0_IO_POL_REG is 0x (both bits set to 0) and image doesn't display correctly. Checked using "devmem2 0x01c0c088" on A20-OLinuXino-MICRO Rev J. > > At the moment I don't have a board to check it with me, I'll try to do > it soon, but I'm full with other work at the moment. > > If you have the chance to debug you could try to find in which point > mode->flags gets changed with those 2 flags. > > When the problem came out I've noticed the same thing for u-boot too but > it's been decided to not patch it because in that case every single > sunxi defconfig had to be changed from: > CONFIG_VIDEO_LCD_MODE="...,sync:3,..." > to: > CONFIG_VIDEO_LCD_MODE="...,sync:0,..." > and it would have been a great mess, probably not worth it: > https://lists.denx.de/pipermail/u-boot/2018-February/321405.html > > So keep in mind that TCON driver works differently for HS and > VS(inverted) polarity in u-boot and linux. > > Hope to work this out soon. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/sun4i: fix HSYNC and VSYNC polarity
Hi Giulio, On Thu, 15 Feb 2018 at 17:54, Giulio Benetti wrote: > > Differently from other Lcd signals, HSYNC and VSYNC signals > result inverted if their bits are cleared to 0. > > Invert their settings of IO_POL register. > > Signed-off-by: Giulio Benetti > --- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > index 3c15cf2..aaf911a 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -389,10 +389,10 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon > *tcon, > SUN4I_TCON0_BASIC3_H_SYNC(hsync)); > > /* Setup the polarity of the various signals */ > - if (!(mode->flags & DRM_MODE_FLAG_PHSYNC)) > + if (mode->flags & DRM_MODE_FLAG_PHSYNC) > val |= SUN4I_TCON0_IO_POL_HSYNC_POSITIVE; > > - if (!(mode->flags & DRM_MODE_FLAG_PVSYNC)) > + if (mode->flags & DRM_MODE_FLAG_PVSYNC) > val |= SUN4I_TCON0_IO_POL_VSYNC_POSITIVE; > > regmap_update_bits(tcon->regs, SUN4I_TCON0_IO_POL_REG, I am running Linux 4.19.6 and noticed with Olimex LCD-OLinuXino-7TS 7" LCD touchscreen (Innolux AT070TN92) connected to Olimex A20-OLinuXino-MICRO that the image does not display correctly after this change. The image is shifted to the right. Reverting the change results in the image being displayed correctly on the screen. I have in the device tree a "panel" node with compatible = "innolux,at070tn92" which uses the timings in drivers/gpu/drm/panel/panel-simple.c. Any ideas? Thanks. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH libdrm] tests/util: Add support for sun4i-drm module
Add support for sun4i DRM driver merged for Linux 4.7. Signed-off-by: Jonathan Liu --- tests/util/kms.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/util/kms.c b/tests/util/kms.c index 8b3e7878..ffae2531 100644 --- a/tests/util/kms.c +++ b/tests/util/kms.c @@ -144,6 +144,7 @@ static const char * const modules[] = { "mediatek", "meson", "pl111", + "sun4i-drm", }; int util_open(const char *device, const char *module) -- 2.16.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate
This patchset fixes several issues in sun4i_tmds_determine_rate that I discovered while trying to get a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz to display at its native resolution. Changes for v3: - Improve commit message for unset best_parent Changes for v2: - Split into separate patches for each issue - Add details to commit message for reproducing issue Jonathan Liu (3): drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.15.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 2/3] drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate
best_div is set to i which corresponds to rate halving when it should be set to j which corresponds to the divider. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 4d235e5ea31c..88eeeaf34638 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; - best_div = i; + best_div = j; } } } -- 2.15.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 3/3] drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate
It was only checking the divider when determing the closest match if it could not match the requested rate exactly. For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this resulted in 1280x800 mode not being available and the following in dmesg when the kernel is booted with drm.debug=0x3e: [drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 1480 1680 800 810 816 831 0x48 0x5 [drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 88eeeaf34638..3ecffa52c814 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (!best_parent || abs(rate - rounded / i) < - abs(rate - best_parent / best_div)) { + if (!best_parent || + abs(rate - rounded / i / j) < + abs(rate - best_parent / best_half / + best_div)) { best_parent = rounded; + best_half = i; best_div = j; } } -- 2.15.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate
It is possible that if there is no exact rate match and "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values (e.g. if rounded is 2 * ideal) that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met and best_parent is never set. This results in req->rate and req->best_parent_rate being assigned 0. To avoid this, we set best_parent to the first calculated rate if it is unset. The sun4i_tmds_calc_divider function already has a similar check. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (abs(rate - rounded / i) < + if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; best_div = i; -- 2.15.1 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate
Hi Maxime, On 5 January 2018 at 21:03, Maxime Ripard wrote: > On Fri, Jan 05, 2018 at 09:44:39AM +1100, Jonathan Liu wrote: >> On 5 January 2018 at 06:56, Maxime Ripard >> wrote: >> > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: >> >> We should check if the best match has been set before comparing it. >> >> >> >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") >> >> Signed-off-by: Jonathan Liu >> >> --- >> >> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- >> >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> index dc332ea56f6c..4d235e5ea31c 100644 >> >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw >> >> *hw, >> >> goto out; >> >> } >> >> >> >> - if (abs(rate - rounded / i) < >> >> + if (!best_parent || abs(rate - rounded / i) >> >> < >> > >> > Why is that causing any issue? >> > >> > If best_parent is set to 0... >> > >> >> abs(rate - best_parent / best_div)) { >> > >> > ... the value returned here is going to be rate, which is going to be >> > higher than the first part of the comparison meaning ... >> > >> >> best_parent = rounded; >> > >> > ... that best_parent is going to be set there. >> >> Consider the following: >> rate = 8350 >> rounded = ideal * 2 >> >> It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" >> gives high enough values that the condition "abs(rate - rounded / i) < >> abs(rate - best_parent / best_div)" is never met. >> >> Then you can end up with: >> req->rate = 0 >> req->best_parent_rate = 0 >> req->best_parent_hw = ... >> >> Also, the sun4i_tmds_calc_divider function has a similar check. > > Ok. That explanation must be part of your commit log, or at least > which problem you're trying to address and in which situation it will > trigger. Ok. I have added amended the commit message with explanation for v3. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate
Hi Maxime, On 5 January 2018 at 06:56, Maxime Ripard wrote: > On Tue, Dec 26, 2017 at 10:12:25PM +1100, Jonathan Liu wrote: >> We should check if the best match has been set before comparing it. >> >> Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") >> Signed-off-by: Jonathan Liu >> --- >> drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> index dc332ea56f6c..4d235e5ea31c 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c >> @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, >> goto out; >> } >> >> - if (abs(rate - rounded / i) < >> + if (!best_parent || abs(rate - rounded / i) < > > Why is that causing any issue? > > If best_parent is set to 0... > >> abs(rate - best_parent / best_div)) { > > ... the value returned here is going to be rate, which is going to be > higher than the first part of the comparison meaning ... > >> best_parent = rounded; > > ... that best_parent is going to be set there. Consider the following: rate = 8350 rounded = ideal * 2 It is possible that if "rounded = clk_hw_round_rate(parent, ideal)" gives high enough values that the condition "abs(rate - rounded / i) < abs(rate - best_parent / best_div)" is never met. Then you can end up with: req->rate = 0 req->best_parent_rate = 0 req->best_parent_hw = ... Also, the sun4i_tmds_calc_divider function has a similar check. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 0/3] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate
This patchset fixes several issues in sun4i_tmds_determine_rate that I discovered while trying to get a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz to display at its native resolution. Changes for v2: - Split into separate patches for each issue - Add details to commit message for reproducing issue Jonathan Liu (3): drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) -- 2.15.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 2/3] drm/sun4i: hdmi: Fix incorrect assignment in sun4i_tmds_determine_rate
best_div is set to i which corresponds to rate halving when it should be set to j which corresponds to the divider. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 4d235e5ea31c..88eeeaf34638 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -105,7 +105,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; - best_div = i; + best_div = j; } } } -- 2.15.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 3/3] drm/sun4i: hdmi: Add missing rate halving check in sun4i_tmds_determine_rate
It was only checking the divider when determing the closest match if it could not match the requested rate exactly. For a projector connected to an Olimex A20-OLinuXino-LIME using HDMI with a native resolution of 1280x800 and pixel clock of 83.5 MHz, this resulted in 1280x800 mode not being available and the following in dmesg when the kernel is booted with drm.debug=0x3e: [drm:drm_mode_debug_printmodeline] Modeline 37:"1280x800" 60 83500 1280 1352 1480 1680 800 810 816 831 0x48 0x5 [drm:drm_mode_prune_invalid] Not using 1280x800 mode: NOCLOCK Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index 88eeeaf34638..3ecffa52c814 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,9 +102,12 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (!best_parent || abs(rate - rounded / i) < - abs(rate - best_parent / best_div)) { + if (!best_parent || + abs(rate - rounded / i / j) < + abs(rate - best_parent / best_half / + best_div)) { best_parent = rounded; + best_half = i; best_div = j; } } -- 2.15.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 1/3] drm/sun4i: hdmi: Check for unset best_parent in sun4i_tmds_determine_rate
We should check if the best match has been set before comparing it. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..4d235e5ea31c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,7 +102,7 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (abs(rate - rounded / i) < + if (!best_parent || abs(rate - rounded / i) < abs(rate - best_parent / best_div)) { best_parent = rounded; best_div = i; -- 2.15.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/sun4i: hdmi: Fix sun4i_tmds_determine_rate
There are several issues in sun4i_tmds_determine_rate: - doesn't check if the best match was already set before comparing it with the enumerated parameters which could result in integer divide by zero - doesn't consider rate halving when determining closest match if it can't match the requested rate exactly - sets best_div to i which corresponds to rate halving when it should be set to j which corresponds to the divider Fix these issues. Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support") Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c index dc332ea56f6c..3ecffa52c814 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_tmds_clk.c @@ -102,10 +102,13 @@ static int sun4i_tmds_determine_rate(struct clk_hw *hw, goto out; } - if (abs(rate - rounded / i) < - abs(rate - best_parent / best_div)) { + if (!best_parent || + abs(rate - rounded / i / j) < + abs(rate - best_parent / best_half / + best_div)) { best_parent = rounded; - best_div = i; + best_half = i; + best_div = j; } } } -- 2.15.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [14/23] drm/sun4i: Create minimal multipliers and dividers
Hi Maxime, On 17 October 2017 at 20:06, Maxime Ripard wrote: > The various outputs the TCON can provide have different constraints on the > dotclock divider. Let's make them configurable by the various mode_set > functions. > > Signed-off-by: Maxime Ripard > Reviewed-by: Chen-Yu Tsai > --- > drivers/gpu/drm/sun4i/sun4i_dotclock.c | 10 +++--- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 ++ > drivers/gpu/drm/sun4i/sun4i_tcon.h | 2 ++ > 3 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > index d401156490f3..023f39bda633 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c > +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c > @@ -17,8 +17,9 @@ > #include "sun4i_dotclock.h" > > struct sun4i_dclk { > - struct clk_hw hw; > - struct regmap *regmap; > + struct clk_hw hw; > + struct regmap *regmap; > + struct sun4i_tcon *tcon; > }; > > static inline struct sun4i_dclk *hw_to_dclk(struct clk_hw *hw) > @@ -73,11 +74,13 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw > *hw, > static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate, > unsigned long *parent_rate) > { > + struct sun4i_dclk *dclk = hw_to_dclk(hw); > + struct sun4i_tcon *tcon = dclk->tcon; > unsigned long best_parent = 0; > u8 best_div = 1; > int i; > > - for (i = 6; i <= 127; i++) { > + for (i = tcon->dclk_min_div; i <= tcon->dclk_max_div; i++) { > unsigned long ideal = rate * i; > unsigned long rounded; > > @@ -167,6 +170,7 @@ int sun4i_dclk_create(struct device *dev, struct > sun4i_tcon *tcon) > dclk = devm_kzalloc(dev, sizeof(*dclk), GFP_KERNEL); > if (!dclk) > return -ENOMEM; > + dclk->tcon = tcon; > > init.name = clk_name; > init.ops = &sun4i_dclk_ops; > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c > b/drivers/gpu/drm/sun4i/sun4i_tcon.c > index f69bcdf11cb8..3efa1ab045cd 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c > @@ -177,6 +177,8 @@ static void sun4i_tcon0_mode_set_rgb(struct sun4i_tcon > *tcon, > u8 clk_delay; > u32 val = 0; > > + tcon->dclk_min_div = 6; > + tcon->dclk_max_div = 127; > sun4i_tcon0_mode_set_common(tcon, mode); > > /* Adjust clock delay */ > diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h > b/drivers/gpu/drm/sun4i/sun4i_tcon.h > index f61bf6d83b4a..4141fbd97ddf 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h > +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h > @@ -169,6 +169,8 @@ struct sun4i_tcon { > > /* Pixel clock */ > struct clk *dclk; > + u8 dclk_max_div; > + u8 dclk_min_div; > > /* Reset control */ > struct reset_control*lcd_rst; I have 4.3" RGB LCD enabled on sun7i-a20-olinuxino-lime and hdmi disabled. After applying this patch the LCD no longer turns on. If I add some debug statements to sun4i_dclk_recalc_rate, it shows tcon->dclk_min_div and tcon->dclk_max_div are both 0. Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [15/23] drm/sun4i: Add LVDS support
Hi Maxime, On 17 October 2017 at 20:06, Maxime Ripard wrote: > The TCON supports the LVDS interface to output to a panel or a bridge. > Let's add support for it. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/sun4i/Makefile | 1 +- > drivers/gpu/drm/sun4i/sun4i_lvds.c | 183 - > drivers/gpu/drm/sun4i/sun4i_lvds.h | 18 +++- > drivers/gpu/drm/sun4i/sun4i_tcon.c | 193 +- > drivers/gpu/drm/sun4i/sun4i_tcon.h | 25 - > 5 files changed, 419 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/sun4i/sun4i_lvds.c > create mode 100644 drivers/gpu/drm/sun4i/sun4i_lvds.h > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile > index cfba2c07519c..6fee15d016ef 100644 > --- a/drivers/gpu/drm/sun4i/Makefile > +++ b/drivers/gpu/drm/sun4i/Makefile > @@ -10,6 +10,7 @@ sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o > > sun4i-tcon-y += sun4i_tcon.o > sun4i-tcon-y += sun4i_rgb.o > +sun4i-tcon-y += sun4i_lvds.o > sun4i-tcon-y += sun4i_dotclock.o > sun4i-tcon-y += sun4i_crtc.o > > diff --git a/drivers/gpu/drm/sun4i/sun4i_lvds.c > b/drivers/gpu/drm/sun4i/sun4i_lvds.c > new file mode 100644 > index ..635a3f505ecb > --- /dev/null > +++ b/drivers/gpu/drm/sun4i/sun4i_lvds.c > @@ -0,0 +1,183 @@ > +/* > + * Copyright (C) 2015 NextThing Co > + * Copyright (C) 2015-2017 Free Electrons > + * > + * Maxime Ripard > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation; either version 2 of > + * the License, or (at your option) any later version. > + */ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +#include "sun4i_crtc.h" > +#include "sun4i_tcon.h" > +#include "sun4i_lvds.h" > + > +struct sun4i_lvds { > + struct drm_connectorconnector; > + struct drm_encoder encoder; > + > + struct sun4i_tcon *tcon; > +}; > + > +static inline struct sun4i_lvds * > +drm_connector_to_sun4i_lvds(struct drm_connector *connector) > +{ > + return container_of(connector, struct sun4i_lvds, > + connector); > +} > + > +static inline struct sun4i_lvds * > +drm_encoder_to_sun4i_lvds(struct drm_encoder *encoder) > +{ > + return container_of(encoder, struct sun4i_lvds, > + encoder); > +} > + > +static int sun4i_lvds_get_modes(struct drm_connector *connector) > +{ > + struct sun4i_lvds *lvds = > + drm_connector_to_sun4i_lvds(connector); > + struct sun4i_tcon *tcon = lvds->tcon; > + > + return drm_panel_get_modes(tcon->panel); > +} > + > +static struct drm_connector_helper_funcs sun4i_lvds_con_helper_funcs = { > + .get_modes = sun4i_lvds_get_modes, > +}; > + > +static void > +sun4i_lvds_connector_destroy(struct drm_connector *connector) > +{ > + struct sun4i_lvds *lvds = drm_connector_to_sun4i_lvds(connector); > + struct sun4i_tcon *tcon = lvds->tcon; > + > + drm_panel_detach(tcon->panel); > + drm_connector_cleanup(connector); > +} > + > +static const struct drm_connector_funcs sun4i_lvds_con_funcs = { > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy= sun4i_lvds_connector_destroy, > + .reset = drm_atomic_helper_connector_reset, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > +static void sun4i_lvds_encoder_enable(struct drm_encoder *encoder) > +{ > + struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder); > + struct sun4i_tcon *tcon = lvds->tcon; > + > + DRM_DEBUG_DRIVER("Enabling LVDS output\n"); > + > + if (!IS_ERR(tcon->panel)) { > + drm_panel_prepare(tcon->panel); > + drm_panel_enable(tcon->panel); > + } > +} > + > +static void sun4i_lvds_encoder_disable(struct drm_encoder *encoder) > +{ > + struct sun4i_lvds *lvds = drm_encoder_to_sun4i_lvds(encoder); > + struct sun4i_tcon *tcon = lvds->tcon; > + > + DRM_DEBUG_DRIVER("Disabling LVDS output\n"); > + > + if (!IS_ERR(tcon->panel)) { > + drm_panel_disable(tcon->panel); > + drm_panel_unprepare(tcon->panel); > + } > +} > + > +static const struct drm_encoder_helper_funcs sun4i_lvds_enc_helper_funcs = { > + .disable= sun4i_lvds_encoder_disable, > + .enable = sun4i_lvds_encoder_enable, > +}; > + > +static const struct drm_encoder_funcs sun4i_lvds_enc_funcs = { > + .destroy= drm_encoder_cleanup, > +}; > + > +int sun4i_lvds_init(struct drm_device *drm, struct sun4i_tcon *tcon) > +{ > +
[PATCH] drm/sun4i: tcon: Add dithering support for RGB565/RGB666 LCD panels
Dithering is supported on TCON channel 0 which is used for LCD panels. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 37 + drivers/gpu/drm/sun4i/sun4i_tcon.h | 18 +- 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 68751b999877..cf313ca858b3 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -12,11 +12,13 @@ #include #include +#include #include #include #include #include #include +#include #include @@ -168,7 +170,9 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon, struct drm_display_mode *mode) { unsigned int bp, hsync, vsync; + u32 bus_format = 0; u8 clk_delay; + struct drm_connector *connector = tcon->panel->connector; u32 val = 0; /* Configure the dot clock */ @@ -230,6 +234,39 @@ void sun4i_tcon0_mode_set(struct sun4i_tcon *tcon, SUN4I_TCON0_IO_POL_HSYNC_POSITIVE | SUN4I_TCON0_IO_POL_VSYNC_POSITIVE, val); + if (connector->display_info.num_bus_formats) + bus_format = connector->display_info.bus_formats[0]; + + switch (bus_format) { + case MEDIA_BUS_FMT_RGB565_1X16: + case MEDIA_BUS_FMT_RGB666_1X18: + /* Enable dithering */ + /* FIXME: Undocumented bits */ + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED0_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED1_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED2_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED3_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED4_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_SEED5_REG, 0x); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB0_REG, 0x0101); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB1_REG, 0x1515); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB2_REG, 0x5757); + regmap_write(tcon->regs, SUN4I_TCON0_FRM_TAB3_REG, 0x7f7f); + val = SUN4I_TCON0_FRM_CTL_ENABLE; + + if (bus_format == MEDIA_BUS_FMT_RGB565_1X16) { + val |= SUN4I_TCON0_FRM_CTL_MODE_R; + val |= SUN4I_TCON0_FRM_CTL_MODE_B; + } + + regmap_write(tcon->regs, SUN4I_TCON0_FRM_CTL_REG, val); + break; + default: + /* Disable dithering */ + regmap_write(tcon->regs, SUN4I_TCON0_FRM_CTL_REG, 0); + break; + } + /* Map output pins to channel 0 */ regmap_update_bits(tcon->regs, SUN4I_TCON_GCTL_REG, SUN4I_TCON_GCTL_IOMAP_MASK, diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index d9e1357cc8ae..d64d45144c91 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -31,7 +31,23 @@ #define SUN4I_TCON_GINT0_VBLANK_INT(pipe) BIT(15 - (pipe)) #define SUN4I_TCON_GINT1_REG 0x8 -#define SUN4I_TCON_FRM_CTL_REG 0x10 + +#define SUN4I_TCON0_FRM_CTL_REG0x10 +#define SUN4I_TCON0_FRM_CTL_ENABLE BIT(31) +#define SUN4I_TCON0_FRM_CTL_MODE_R BIT(6) +#define SUN4I_TCON0_FRM_CTL_MODE_G BIT(5) +#define SUN4I_TCON0_FRM_CTL_MODE_B BIT(4) + +#define SUN4I_TCON0_FRM_SEED0_REG 0x14 +#define SUN4I_TCON0_FRM_SEED1_REG 0x18 +#define SUN4I_TCON0_FRM_SEED2_REG 0x1c +#define SUN4I_TCON0_FRM_SEED3_REG 0x20 +#define SUN4I_TCON0_FRM_SEED4_REG 0x24 +#define SUN4I_TCON0_FRM_SEED5_REG 0x28 +#define SUN4I_TCON0_FRM_TAB0_REG 0x2c +#define SUN4I_TCON0_FRM_TAB1_REG 0x30 +#define SUN4I_TCON0_FRM_TAB2_REG 0x34 +#define SUN4I_TCON0_FRM_TAB3_REG 0x38 #define SUN4I_TCON0_CTL_REG0x40 #define SUN4I_TCON0_CTL_TCON_ENABLEBIT(31) -- 2.14.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panel: simple: Add missing panel_simple_unprepare calls
During panel removal or system shutdown panel_simple_disable is called which disables the panel backlight but the panel is still powered due to missing calls to panel_simple_unprepare. Fixes: d02fd93e2cd8 ("drm/panel: simple - Disable panel on shutdown") Cc: sta...@vger.kernel.org # v3.16+ Signed-off-by: Jonathan Liu --- drivers/gpu/drm/panel/panel-simple.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 474fa759e06e..234af81fb3d0 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -369,6 +369,7 @@ static int panel_simple_remove(struct device *dev) drm_panel_remove(&panel->base); panel_simple_disable(&panel->base); + panel_simple_unprepare(&panel->base); if (panel->ddc) put_device(&panel->ddc->dev); @@ -384,6 +385,7 @@ static void panel_simple_shutdown(struct device *dev) struct panel_simple *panel = dev_get_drvdata(dev); panel_simple_disable(&panel->base); + panel_simple_unprepare(&panel->base); } static const struct drm_display_mode ampire_am_480272h3tmqw_t01h_mode = { -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/panel: simple: Fix width and height for Olimex LCD-OLinuXino-4.3TS
The physical size of the panel is 105.5 (W) x 67.2 (H) x 4.05 (D) mm but the active display area is 95.04 (W) x 53.856 (H) mm. The width and height should be set to the active display area. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/panel/panel-simple.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 474fa759e06e..39a622a547e7 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -1522,8 +1522,8 @@ static const struct panel_desc olimex_lcd_olinuxino_43ts = { .modes = &olimex_lcd_olinuxino_43ts_mode, .num_modes = 1, .size = { - .width = 105, - .height = 67, + .width = 95, + .height = 54, }, .bus_format = MEDIA_BUS_FMT_RGB888_1X24, }; -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/sun4i: Implement drm_driver lastclose to restore fbdev console
Hi Maxime, On 10 July 2017 at 16:44, Maxime Ripard wrote: > On Sun, Jul 09, 2017 at 11:11:07PM +0800, Chen-Yu Tsai wrote: >> On Sun, Jul 9, 2017 at 3:59 PM, Jonathan Liu wrote: >> > The drm_driver lastclose callback is called when the last userspace >> > DRM client has closed. Call drm_fbdev_cma_restore_mode to restore >> > the fbdev console otherwise the fbdev console will stop working. >> > >> > Signed-off-by: Jonathan Liu >> >> This should have >> >> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") >> >> Otherwise, >> >> Reviewed-by: Chen-Yu Tsai > > And it should be sent to stable too. > > Can you resend it? Will do. > > Thanks! > Maxime > > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v3] drm/sun4i: Implement drm_driver lastclose to restore fbdev console
The drm_driver lastclose callback is called when the last userspace DRM client has closed. Call drm_fbdev_cma_restore_mode to restore the fbdev console otherwise the fbdev console will stop working. Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support") Cc: sta...@vger.kernel.org Signed-off-by: Jonathan Liu Reviewed-by: Chen-Yu Tsai --- Changes for v3: - Add 'Fixes:' tag - Add CC to stable - Add 'Reviewed-by: Chen-Yu Tsai ' Changes for v2: - Rename sun4i_drm_lastclose to sun4i_drv_lastclose drivers/gpu/drm/sun4i/sun4i_drv.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index abc7d8fe06b4..a45a627283a1 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -25,12 +25,20 @@ #include "sun4i_framebuffer.h" #include "sun4i_tcon.h" +static void sun4i_drv_lastclose(struct drm_device *dev) +{ + struct sun4i_drv *drv = dev->dev_private; + + drm_fbdev_cma_restore_mode(drv->fbdev); +} + DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops); static struct drm_driver sun4i_drv_driver = { .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_ATOMIC, /* Generic Operations */ + .lastclose = sun4i_drv_lastclose, .fops = &sun4i_drv_fops, .name = "sun4i-drm", .desc = "Allwinner sun4i Display Engine", -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/sun4i: Implement drm_driver lastclose to restore fbdev console
The drm_driver lastclose callback is called when the last userspace DRM client has closed. Call drm_fbdev_cma_restore_mode to restore the fbdev console otherwise the fbdev console will stop working. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_drv.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index abc7d8fe06b4..c434540d409c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -25,12 +25,20 @@ #include "sun4i_framebuffer.h" #include "sun4i_tcon.h" +static void sun4i_drm_lastclose(struct drm_device *dev) +{ + struct sun4i_drv *drv = dev->dev_private; + + drm_fbdev_cma_restore_mode(drv->fbdev); +} + DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops); static struct drm_driver sun4i_drv_driver = { .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_ATOMIC, /* Generic Operations */ + .lastclose = sun4i_drm_lastclose, .fops = &sun4i_drv_fops, .name = "sun4i-drm", .desc = "Allwinner sun4i Display Engine", -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2] drm/sun4i: Implement drm_driver lastclose to restore fbdev console
The drm_driver lastclose callback is called when the last userspace DRM client has closed. Call drm_fbdev_cma_restore_mode to restore the fbdev console otherwise the fbdev console will stop working. Signed-off-by: Jonathan Liu --- Changes for v2: - Rename sun4i_drm_lastclose to sun4i_drv_lastclose drivers/gpu/drm/sun4i/sun4i_drv.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c index abc7d8fe06b4..a45a627283a1 100644 --- a/drivers/gpu/drm/sun4i/sun4i_drv.c +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c @@ -25,12 +25,20 @@ #include "sun4i_framebuffer.h" #include "sun4i_tcon.h" +static void sun4i_drv_lastclose(struct drm_device *dev) +{ + struct sun4i_drv *drv = dev->dev_private; + + drm_fbdev_cma_restore_mode(drv->fbdev); +} + DEFINE_DRM_GEM_CMA_FOPS(sun4i_drv_fops); static struct drm_driver sun4i_drv_driver = { .driver_features= DRIVER_GEM | DRIVER_MODESET | DRIVER_PRIME | DRIVER_ATOMIC, /* Generic Operations */ + .lastclose = sun4i_drv_lastclose, .fops = &sun4i_drv_fops, .name = "sun4i-drm", .desc = "Allwinner sun4i Display Engine", -- 2.13.2 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Chen-Yu and Maxime, On 30 June 2017 at 13:16, Chen-Yu Tsai wrote: > On Fri, Jun 30, 2017 at 6:22 AM, Jonathan Liu wrote: >> Hi Maxime, >> >> On 30 June 2017 at 01:56, Maxime Ripard >> wrote: >>> On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >>>> >> + u32 int_status; >>>> >> + u32 fifo_status; >>>> >> + /* Read needs empty flag unset, write needs full flag unset */ >>>> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >>>> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >>>> >> + int ret; >>>> >> + >>>> >> + /* Wait until error or FIFO ready */ >>>> >> + ret = readl_poll_timeout(hdmi->base + >>>> >> SUN4I_HDMI_DDC_INT_STATUS_REG, >>>> >> + int_status, >>>> >> + is_err_status(int_status) || >>>> >> + is_fifo_flag_unset(hdmi, &fifo_status, >>>> >> flag), >>>> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * >>>> >> byte_time, >>>> >> + 10); >>>> >> + >>>> >> + if (is_err_status(int_status)) >>>> >> + return -EIO; >>>> >> + if (ret) >>>> >> + return -ETIMEDOUT; >>>> > >>>> > Why not just have >>>> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, >>>> > reg, >>>> > !(reg & flag), 100, 10); >>>> > >>>> > if (ret < 0) >>>> > if (is_err_status()) >>>> > return -EIO; >>>> > return ret; >>>> > >>>> > >>>> >>>> If I check error status after readl_poll_timeout and there is an error >>>> (e.g. the I2C address does not have a corresponding device connected >>>> or nothing connected to HDMI port) it will keep checking the fifo >>>> status even though error bit is set in the int status and then timeout >>>> after 100 ms. If it checks the int status register at the same time, >>>> it will error after 100 nanoseconds. I don't want to introduce >>>> unnecessary delays considering part of the reason for adding this >>>> driver to make it more usable for non-standard use cases. >>> >>> Well, polling for 100ms doesn't seem great either. What was the >>> rationale behind that timeout? >>> >> >> When an error occurs one of the error bits will be set in the >> INT_STATUS register so this is detected very quickly if I check the >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in >> case the I2C slave does clock stretching in which case the transfer >> may take longer than the predicted time. >> >>> And we can also reverse the check and look at the INT_STATUS >>> register. The errors will be there, and we can program the threshold >>> we want in both directions and use the >>> DDC_FIFO_Request_Interrupt_Status bit. >>> >> >> I did try that when I was doing the v3 patch but I couldn't get it to >> work as mentioned previously in the v3 patch discussion. I programmed >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl >> register at the same time as setting FIFO_Address_Clear but the >> request interrupt status bit did not get updated to the appropriate >> state that is consistent with the FIFO level and the thresholds. I did >> try this several times for subsequent patch versions without success. > > The manual says "When FIFO level is above this value in read mode, DMA > request and FIFO request interrupt are asserted if relative enable is on." > > Perhaps try enabling the interrupts? But if that were the case, wouldn't > using interrupts instead of polling be better? > > ChenYu > I managed to get the thresholds working so switching to using interrupts instead of polling will be my next goal. >> >>> Maxime >>> >>> -- >>> Maxime Ripard, Free Electrons >>> Embedded Linux and Kernel engineering >>> http://free-electrons.com >> >> Regards, >> Jonathan Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 30 June 2017 at 19:58, Jonathan Liu wrote: > Hi Maxime, > > On 30 June 2017 at 19:34, Maxime Ripard > wrote: >> Hi, >> >> On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote: >>> Hi Maxime, >>> >>> On 30 June 2017 at 01:56, Maxime Ripard >>> wrote: >>> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >>> >> >> + u32 int_status; >>> >> >> + u32 fifo_status; >>> >> >> + /* Read needs empty flag unset, write needs full flag unset */ >>> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >>> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >>> >> >> + int ret; >>> >> >> + >>> >> >> + /* Wait until error or FIFO ready */ >>> >> >> + ret = readl_poll_timeout(hdmi->base + >>> >> >> SUN4I_HDMI_DDC_INT_STATUS_REG, >>> >> >> + int_status, >>> >> >> + is_err_status(int_status) || >>> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, >>> >> >> flag), >>> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * >>> >> >> byte_time, >>> >> >> + 10); >>> >> >> + >>> >> >> + if (is_err_status(int_status)) >>> >> >> + return -EIO; >>> >> >> + if (ret) >>> >> >> + return -ETIMEDOUT; >>> >> > >>> >> > Why not just have >>> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, >>> >> > reg, >>> >> > !(reg & flag), 100, 10); >>> >> > >>> >> > if (ret < 0) >>> >> > if (is_err_status()) >>> >> > return -EIO; >>> >> > return ret; >>> >> > >>> >> > >>> >> >>> >> If I check error status after readl_poll_timeout and there is an error >>> >> (e.g. the I2C address does not have a corresponding device connected >>> >> or nothing connected to HDMI port) it will keep checking the fifo >>> >> status even though error bit is set in the int status and then timeout >>> >> after 100 ms. If it checks the int status register at the same time, >>> >> it will error after 100 nanoseconds. I don't want to introduce >>> >> unnecessary delays considering part of the reason for adding this >>> >> driver to make it more usable for non-standard use cases. >>> > >>> > Well, polling for 100ms doesn't seem great either. What was the >>> > rationale behind that timeout? >>> > >>> >>> When an error occurs one of the error bits will be set in the >>> INT_STATUS register so this is detected very quickly if I check the >>> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in >>> case the I2C slave does clock stretching in which case the transfer >>> may take longer than the predicted time. >> >> 100ms isn't stretching anymore, it's worse than that. >> > > What would you suggest? > You need to consider the fact that there are I2C devices such as sensors that may do I2C clock stretching up to several tens of milliseconds. For example the HTU21D humidity sensor takes up to 50 ms max to perform measurement of temperature during which it is holding down the clock to do I2C clock stretching. >>> > And we can also reverse the check and look at the INT_STATUS >>> > register. The errors will be there, and we can program the threshold >>> > we want in both directions and use the >>> > DDC_FIFO_Request_Interrupt_Status bit. >>> > >>> >>> I did try that when I was doing the v3 patch but I couldn't get it to >>> work as mentioned previously in the v3 patch discussion. I programmed >>> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl >>> register at the same time as setting FIFO_Address_Clear but the >>> request interrupt status bit did not get updated to the appropriate >>> state that is consistent with the FIFO level and the thresholds. I did >>> try this several times for subsequent patch versions without success. >> >> What values did you set it to? >> > > FIFO_RX_TRIGGER_THRES: 0 > FIFO_TX_TRIGGER_THRES: 15 > >> Maxime >> >> -- >> Maxime Ripard, Free Electrons >> Embedded Linux and Kernel engineering >> http://free-electrons.com > > Regards, > Jonathan Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 30 June 2017 at 19:34, Maxime Ripard wrote: > Hi, > > On Fri, Jun 30, 2017 at 08:22:13AM +1000, Jonathan Liu wrote: >> Hi Maxime, >> >> On 30 June 2017 at 01:56, Maxime Ripard >> wrote: >> > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >> >> >> + u32 int_status; >> >> >> + u32 fifo_status; >> >> >> + /* Read needs empty flag unset, write needs full flag unset */ >> >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >> >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >> >> >> + int ret; >> >> >> + >> >> >> + /* Wait until error or FIFO ready */ >> >> >> + ret = readl_poll_timeout(hdmi->base + >> >> >> SUN4I_HDMI_DDC_INT_STATUS_REG, >> >> >> + int_status, >> >> >> + is_err_status(int_status) || >> >> >> + is_fifo_flag_unset(hdmi, &fifo_status, >> >> >> flag), >> >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * >> >> >> byte_time, >> >> >> + 10); >> >> >> + >> >> >> + if (is_err_status(int_status)) >> >> >> + return -EIO; >> >> >> + if (ret) >> >> >> + return -ETIMEDOUT; >> >> > >> >> > Why not just have >> >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, >> >> > reg, >> >> > !(reg & flag), 100, 10); >> >> > >> >> > if (ret < 0) >> >> > if (is_err_status()) >> >> > return -EIO; >> >> > return ret; >> >> > >> >> > >> >> >> >> If I check error status after readl_poll_timeout and there is an error >> >> (e.g. the I2C address does not have a corresponding device connected >> >> or nothing connected to HDMI port) it will keep checking the fifo >> >> status even though error bit is set in the int status and then timeout >> >> after 100 ms. If it checks the int status register at the same time, >> >> it will error after 100 nanoseconds. I don't want to introduce >> >> unnecessary delays considering part of the reason for adding this >> >> driver to make it more usable for non-standard use cases. >> > >> > Well, polling for 100ms doesn't seem great either. What was the >> > rationale behind that timeout? >> > >> >> When an error occurs one of the error bits will be set in the >> INT_STATUS register so this is detected very quickly if I check the >> INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in >> case the I2C slave does clock stretching in which case the transfer >> may take longer than the predicted time. > > 100ms isn't stretching anymore, it's worse than that. > What would you suggest? >> > And we can also reverse the check and look at the INT_STATUS >> > register. The errors will be there, and we can program the threshold >> > we want in both directions and use the >> > DDC_FIFO_Request_Interrupt_Status bit. >> > >> >> I did try that when I was doing the v3 patch but I couldn't get it to >> work as mentioned previously in the v3 patch discussion. I programmed >> the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl >> register at the same time as setting FIFO_Address_Clear but the >> request interrupt status bit did not get updated to the appropriate >> state that is consistent with the FIFO level and the thresholds. I did >> try this several times for subsequent patch versions without success. > > What values did you set it to? > FIFO_RX_TRIGGER_THRES: 0 FIFO_TX_TRIGGER_THRES: 15 > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v8] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v8: - Fix clock rate in comment being 100 MHz when it is actually 100 kHz - Fix clearing of bits in interrupt status register as they are cleared by writing 1 to the bit rather than 0 - Set FIFO RX/TX thresholds so interrupt status register can be used to check if FIFO is ready instead of having to additionally check the FIFO status register - Remove unused linux/ktime.h include in sun4i_hdmi_i2c.c - Reduce timeout for clearing FIFO from 100 ms to 2 ms - Use BIT macro instead of GENMASK for SUN4I_HDMI_DDC_BYTE_COUNT_MAX to make it clearer that the maximum is derived from the field bit width rather than its position in the register Changes for v7: - Fix mixed declarations and code compiler warning for level variable Changes for v6: - Use fixed byte time of 100 us instead of dynamically calculating from DDC clock that is set to a fixed 100 MHz rate anyway - Change is_fifo_flag_unset to not read the status register as well to be more consistent with is_err_status Changes for v5: - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter - Rework to use readl_poll_timeout for checking FIFO status Changes for v4: - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c - Clean up indentation in sun4i_hdmi.h - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the value to use the GENMASK macro to make it clear that it is derived from the width of the field in the register - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register - Change struct i2c_adapter to be const by using devm_kmemdup on creation - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an I2C message - Instead of waiting for 1-2 bytes to transfer, wait for the time it would take for remaining bytes to transfer (limited by FIFO size) - Add additional comments Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 24 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 + 4 files changed, 256 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..0957ff2076ac 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD
Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 30 June 2017 at 01:56, Maxime Ripard wrote: > On Wed, Jun 28, 2017 at 08:39:33PM +1000, Jonathan Liu wrote: >> >> + u32 int_status; >> >> + u32 fifo_status; >> >> + /* Read needs empty flag unset, write needs full flag unset */ >> >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >> >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >> >> + int ret; >> >> + >> >> + /* Wait until error or FIFO ready */ >> >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG, >> >> + int_status, >> >> + is_err_status(int_status) || >> >> + is_fifo_flag_unset(hdmi, &fifo_status, >> >> flag), >> >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * >> >> byte_time, >> >> + 10); >> >> + >> >> + if (is_err_status(int_status)) >> >> + return -EIO; >> >> + if (ret) >> >> + return -ETIMEDOUT; >> > >> > Why not just have >> > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg, >> > !(reg & flag), 100, 10); >> > >> > if (ret < 0) >> > if (is_err_status()) >> > return -EIO; >> > return ret; >> > >> > >> >> If I check error status after readl_poll_timeout and there is an error >> (e.g. the I2C address does not have a corresponding device connected >> or nothing connected to HDMI port) it will keep checking the fifo >> status even though error bit is set in the int status and then timeout >> after 100 ms. If it checks the int status register at the same time, >> it will error after 100 nanoseconds. I don't want to introduce >> unnecessary delays considering part of the reason for adding this >> driver to make it more usable for non-standard use cases. > > Well, polling for 100ms doesn't seem great either. What was the > rationale behind that timeout? > When an error occurs one of the error bits will be set in the INT_STATUS register so this is detected very quickly if I check the INT_STATUS and FIFO_STATUS at the same time. The 100 ms timeout is in case the I2C slave does clock stretching in which case the transfer may take longer than the predicted time. > And we can also reverse the check and look at the INT_STATUS > register. The errors will be there, and we can program the threshold > we want in both directions and use the > DDC_FIFO_Request_Interrupt_Status bit. > I did try that when I was doing the v3 patch but I couldn't get it to work as mentioned previously in the v3 patch discussion. I programmed the FIFO_RX_TRIGGER_THRES and FIFO_TX_TRIGGER_THRES in DDC_FIFO_Ctrl register at the same time as setting FIFO_Address_Clear but the request interrupt status bit did not get updated to the appropriate state that is consistent with the FIFO level and the thresholds. I did try this several times for subsequent patch versions without success. > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Chen-Yu, On 29 June 2017 at 12:47, Chen-Yu Tsai wrote: > Hi, > > On Wed, Jun 28, 2017 at 6:52 PM, Jonathan Liu wrote: >> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: >> "As in the general case the DDC bus is accessible by the kernel at the I2C >> level, drivers must make all reasonable efforts to expose it as an I2C >> adapter and use drm_get_edid() instead of abusing this function." >> >> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used >> for purposes other than reading the EDID such as modifying the EDID or >> using the HDMI DDC pins as an I2C bus through the I2C dev interface from >> userspace (e.g. i2c-tools). >> >> Implement this for A10s. >> >> Signed-off-by: Jonathan Liu >> --- >> Changes for v7: >> - Fix mixed declarations and code compiler warning for level variable >> >> Changes for v6: >> - Use fixed byte time of 100 us instead of dynamically calculating from DDC >>clock that is set to a fixed 100 MHz rate anyway >> - Change is_fifo_flag_unset to not read the status register as well to be >>more consistent with is_err_status >> >> Changes for v5: >> - Use devm_kzalloc instead of devm_kmemdup and remove const struct >> i2c_adapter >> - Rework to use readl_poll_timeout for checking FIFO status >> >> Changes for v4: >> - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c >> - Clean up indentation in sun4i_hdmi.h >> - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX >>and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the >>value to use the GENMASK macro to make it clear that it is derived from >>the width of the field in the register >> - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be >>SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW >> - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register >> - Change struct i2c_adapter to be const by using devm_kmemdup on creation >> - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring >> an >>I2C message >> - Instead of waiting for 1-2 bytes to transfer, wait for the time it would >>take for remaining bytes to transfer (limited by FIFO size) >> - Add additional comments >> >> Changes for v3: >> - Explain why drm_do_get_edid should be used and why it's better to expose >> it >>as an I2C adapter in commit message >> - Reorder bit defines in descending order for consistency >> - Keep old unused macros instead of removing them >> - The v2 algorithm split large transfers into 16 byte transfers but this may >>cause a large single write to be treated as multiple writes causing data >>corruption. The algorithm has been reworked to not split larger transfers >>and make more use of the FIFO to avoid this. >> - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to >>sun4i_hdmi_i2c.c >> - Reformatted code >> - Instead of masking bits that we don't want to check for errors, explicitly >>check the error bits >> - Clear error bits at start of transfer in case of error from previous >> transfer >> - Poll for completion of FIFO clear after setting FIFO clear bit >> >> Changes for v2: >> - Rebased against Maxime's sunxi-drm/for-next branch >> - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted >> if >>any of the calls after the I2C adapter is created fails >> - Remove unnecessary includes in sun4i_hdmi_i2c.c >> >> drivers/gpu/drm/sun4i/Makefile | 1 + >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++ >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 234 >> + >> 4 files changed, 269 insertions(+), 90 deletions(-) >> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> > > [...] > >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> new file mode 100644 >> index ..ce954ee25ae4 >> --- /dev/null >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> @@ -0,0 +1,234 @@ > > [...] > >> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool >> read) >> +{ >> + /* >> +* 1 byte takes 9 clock cycles (8 bits + 1 ACK) = 90 us for 100 MHz >> +* clock. As clock rate is fixed, just r
Re: [PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 28 June 2017 at 19:20, Maxime Ripard wrote: > On Wed, Jun 28, 2017 at 12:36:52AM +1000, Jonathan Liu wrote: >> +#define SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK ( \ >> + SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION | \ >> + SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW | \ >> + SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW | \ >> + SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR | \ >> + SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR | \ >> + SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR \ >> +) >> + >> +static bool is_err_status(u32 int_status) >> +{ >> + return !!(int_status & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK); >> +} >> + >> +static bool is_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 *fifo_status, >> +u32 flag) >> +{ >> + *fifo_status = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); >> + return !(*fifo_status & flag); >> +} >> + >> +static int fifo_transfer(struct sun4i_hdmi *hdmi, u8 *buf, int len, bool >> read) >> +{ >> + /* 1 byte takes 9 clock cycles (8 bits + 1 ACK) */ >> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC, >> +clk_get_rate(hdmi->ddc_clk)) * >> 9; > > There's no real need for it to be dynamic. The clock rate will not > change, and the order of magnitude is roughly 100us, so let's just use > that (and make a comment). > Ok. >> + u32 int_status; >> + u32 fifo_status; >> + /* Read needs empty flag unset, write needs full flag unset */ >> + u32 flag = read ? SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY : >> + SUN4I_HDMI_DDC_FIFO_STATUS_FULL; >> + int ret; >> + >> + /* Wait until error or FIFO ready */ >> + ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG, >> + int_status, >> + is_err_status(int_status) || >> + is_fifo_flag_unset(hdmi, &fifo_status, flag), >> + min(len, SUN4I_HDMI_DDC_FIFO_SIZE) * >> byte_time, >> + 10); >> + >> + if (is_err_status(int_status)) >> + return -EIO; >> + if (ret) >> + return -ETIMEDOUT; > > Why not just have > ret = readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG, reg, > !(reg & flag), 100, 10); > > if (ret < 0) > if (is_err_status()) > return -EIO; > return ret; > > If I check error status after readl_poll_timeout and there is an error (e.g. the I2C address does not have a corresponding device connected or nothing connected to HDMI port) it will keep checking the fifo status even though error bit is set in the int status and then timeout after 100 ms. If it checks the int status register at the same time, it will error after 100 nanoseconds. I don't want to introduce unnecessary delays considering part of the reason for adding this driver to make it more usable for non-standard use cases. >> + >> + /* Read FIFO level */ >> + int level = (int)(fifo_status & SUN4I_HDMI_DDC_FIFO_STATUS_LEVEL_MASK); > > and explicitly read the fifo status here. That will make you remove > that function that does two things while claiming that it does only > one, and it will be more obvious. > I will fix the is_fifo_flag_unset function so it only does one thing. > You can also just use reg at this point, instead of reading it once > again. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v5] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v5: - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter - Rework to use readl_poll_timeout for checking FIFO status Changes for v4: - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c - Clean up indentation in sun4i_hdmi.h - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the value to use the GENMASK macro to make it clear that it is derived from the width of the field in the register - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register - Change struct i2c_adapter to be const by using devm_kmemdup on creation - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an I2C message - Instead of waiting for 1-2 bytes to transfer, wait for the time it would take for remaining bytes to transfer (limited by FIFO size) - Add additional comments Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 226 + 4 files changed, 261 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..eaff92fe236c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,33 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5) +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3) +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2) +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1) +#define SUN4I_HDMI_DDC_INT_ST
[PATCH v6] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v6: - Use fixed byte time of 100 us instead of dynamically calculating from DDC clock that is set to a fixed 100 MHz rate anyway - Change is_fifo_flag_unset to not read the status register as well to be more consistent with is_err_status Changes for v5: - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter - Rework to use readl_poll_timeout for checking FIFO status Changes for v4: - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c - Clean up indentation in sun4i_hdmi.h - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the value to use the GENMASK macro to make it clear that it is derived from the width of the field in the register - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register - Change struct i2c_adapter to be const by using devm_kmemdup on creation - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an I2C message - Instead of waiting for 1-2 bytes to transfer, wait for the time it would take for remaining bytes to transfer (limited by FIFO size) - Add additional comments Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++ drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 233 + 4 files changed, 268 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..eaff92fe236c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,33 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5) +#define SUN4I_HDMI_DDC_INT_
[PATCH v7] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v7: - Fix mixed declarations and code compiler warning for level variable Changes for v6: - Use fixed byte time of 100 us instead of dynamically calculating from DDC clock that is set to a fixed 100 MHz rate anyway - Change is_fifo_flag_unset to not read the status register as well to be more consistent with is_err_status Changes for v5: - Use devm_kzalloc instead of devm_kmemdup and remove const struct i2c_adapter - Rework to use readl_poll_timeout for checking FIFO status Changes for v4: - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c - Clean up indentation in sun4i_hdmi.h - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the value to use the GENMASK macro to make it clear that it is derived from the width of the field in the register - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register - Change struct i2c_adapter to be const by using devm_kmemdup on creation - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an I2C message - Instead of waiting for 1-2 bytes to transfer, wait for the time it would take for remaining bytes to transfer (limited by FIFO size) - Add additional comments Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++ drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 234 + 4 files changed, 269 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..eaff92fe236c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,33 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_
Re: [PATCH v4] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 27 June 2017 at 05:05, Maxime Ripard wrote: > On Sat, Jun 24, 2017 at 04:10:54PM +1000, Jonathan Liu wrote: >> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: >> "As in the general case the DDC bus is accessible by the kernel at the I2C >> level, drivers must make all reasonable efforts to expose it as an I2C >> adapter and use drm_get_edid() instead of abusing this function." >> >> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used >> for purposes other than reading the EDID such as modifying the EDID or >> using the HDMI DDC pins as an I2C bus through the I2C dev interface from >> userspace (e.g. i2c-tools). >> >> Implement this for A10s. >> >> Signed-off-by: Jonathan Liu >> --- >> Changes for v4: >> - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c >> - Clean up indentation in sun4i_hdmi.h >> - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX >>and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the >>value to use the GENMASK macro to make it clear that it is derived from >>the width of the field in the register >> - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be >>SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW >> - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register >> - Change struct i2c_adapter to be const by using devm_kmemdup on creation >> - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring >> an >>I2C message >> - Instead of waiting for 1-2 bytes to transfer, wait for the time it would >>take for remaining bytes to transfer (limited by FIFO size) >> - Add additional comments >> >> Changes for v3: >> - Explain why drm_do_get_edid should be used and why it's better to expose >> it >>as an I2C adapter in commit message >> - Reorder bit defines in descending order for consistency >> - Keep old unused macros instead of removing them >> - The v2 algorithm split large transfers into 16 byte transfers but this may >>cause a large single write to be treated as multiple writes causing data >>corruption. The algorithm has been reworked to not split larger transfers >>and make more use of the FIFO to avoid this. >> - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to >>sun4i_hdmi_i2c.c >> - Reformatted code >> - Instead of masking bits that we don't want to check for errors, explicitly >>check the error bits >> - Clear error bits at start of transfer in case of error from previous >> transfer >> - Poll for completion of FIFO clear after setting FIFO clear bit >> >> Changes for v2: >> - Rebased against Maxime's sunxi-drm/for-next branch >> - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted >> if >>any of the calls after the I2C adapter is created fails >> - Remove unnecessary includes in sun4i_hdmi_i2c.c >> >> drivers/gpu/drm/sun4i/Makefile | 1 + >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++ >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 242 >> + >> 4 files changed, 277 insertions(+), 90 deletions(-) >> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> >> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile >> index e29fd3a2ba9c..43c753cafc88 100644 >> --- a/drivers/gpu/drm/sun4i/Makefile >> +++ b/drivers/gpu/drm/sun4i/Makefile >> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o >> sun4i-drm-y += sun4i_framebuffer.o >> >> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o >> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o >> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o >> sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> index 2f2f2ff1ea63..eaff92fe236c 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> @@ -96,6 +96,7 @@ >> #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) >> #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8) >> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8) >> #define SUN4I_HDMI_DDC_CTRL_RESET
[PATCH v4] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v4: - Carry over copyright from initial I2C code into sun4i_hdmi_i2c.c - Clean up indentation in sun4i_hdmi.h - Rename SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE to SUN4I_HDMI_DDC_BYTE_COUNT_MAX and group it under the SUN4I_HDMI_DDC_BYTE_COUNT_REG define, changing the value to use the GENMASK macro to make it clear that it is derived from the width of the field in the register - Fix SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW typo which should be SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW - Remove redundant rewriting of SUN4I_HDMI_DDC_INT_STATUS_REG register - Change struct i2c_adapter to be const by using devm_kmemdup on creation - Return -ETIMEDOUT instead of -EIO if there is timeout while transferring an I2C message - Instead of waiting for 1-2 bytes to transfer, wait for the time it would take for remaining bytes to transfer (limited by FIFO size) - Add additional comments Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 23 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++ drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 242 + 4 files changed, 277 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..eaff92fe236c 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,33 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_OVERFLOW BIT(5) +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3) +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2) +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1) +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0) + #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) +#d
Re: [PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 22 June 2017 at 07:26, Maxime Ripard wrote: > On Wed, Jun 21, 2017 at 07:42:47PM +1000, Jonathan Liu wrote: >> >> +static int wait_fifo_flag_unset(struct sun4i_hdmi *hdmi, u32 flag) >> >> +{ >> >> + /* 1 byte takes 9 clock cycles (8 bits + 1 ack) */ >> >> + unsigned long byte_time = DIV_ROUND_UP(USEC_PER_SEC, >> >> +clk_get_rate(hdmi->ddc_clk)) >> >> * 9; >> >> + ktime_t wait_timeout = ktime_add_us(ktime_get(), 10); >> >> + u32 reg; >> >> + >> >> + for (;;) { >> >> + /* Check for errors */ >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + if (reg & SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK) { >> >> + writel(reg, hdmi->base + >> >> SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + return -EIO; >> >> + } >> >> + >> >> + /* Check if requested FIFO flag is unset */ >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_STATUS_REG); >> >> + if (!(reg & flag)) >> >> + return 0; >> >> + >> >> + /* Timeout */ >> >> + if (ktime_compare(ktime_get(), wait_timeout) > 0) >> >> + return -EIO; >> >> + >> >> + /* Wait for 1-2 bytes to transfer */ >> >> + usleep_range(byte_time, 2 * byte_time); >> >> + } >> >> + >> >> + return -EIO; >> >> +} >> >> + >> >> +static int wait_fifo_read_ready(struct sun4i_hdmi *hdmi) >> >> +{ >> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY); >> >> +} >> >> + >> >> +static int wait_fifo_write_ready(struct sun4i_hdmi *hdmi) >> >> +{ >> >> + return wait_fifo_flag_unset(hdmi, SUN4I_HDMI_DDC_FIFO_STATUS_FULL); >> >> +} >> > >> > Both of these can use readl_poll_timeout, and I'm not sure you need to >> > be that aggressive in your timings. >> > >> I have set my timings to minimize communication delays - e.g. when >> userspace is reading from I2C one byte at a time (like i2cdump from >> i2c-tools). > > I wouldn't try to optimise that too much. There's already a lot of > overhead, it's way inefficient, and it's not really a significant use > case anyway. > Ok. >> readl_poll_timeout can't be used to check 2 registers at >> once and I couldn't get DDC_FIFO_Request_Interrupt_Status_Bit in >> DDC_Int_Status register to behave properly. > > Can't you just use readl_poll_timeout, and then if it timed out check > for errors? > I think it is more flexible this way as I can adjust the usleep_range min. >> I also wanted to minimize the chance of FIFO underrun/overrun. >> >> >> + >> >> +static int xfer_msg(struct sun4i_hdmi *hdmi, struct i2c_msg *msg) >> >> +{ >> >> + u32 reg; >> >> + int i; >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + reg &= ~SUN4I_HDMI_DDC_INT_STATUS_ERROR_MASK; >> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_INT_STATUS_REG); >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); >> >> + reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; >> >> + reg |= (msg->flags & I2C_M_RD) ? >> >> +SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ : >> >> +SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE; >> >> + writel(reg, hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); >> >> + >> >> + writel(SUN4I_HDMI_DDC_ADDR_SLAVE(msg->addr), >> >> +hdmi->base + SUN4I_HDMI_DDC_ADDR_REG); >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); >> >> + writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR, >> >> +hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); >> >> + >> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG, >> >> +reg, >> >> +!(reg & SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR), >> >> +100, 10)) >> &
Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 21 June 2017 at 18:41, Maxime Ripard wrote: > On Tue, Jun 13, 2017 at 09:53:31PM +1000, Jonathan Liu wrote: >> >> --- /dev/null >> >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> >> @@ -0,0 +1,163 @@ >> >> +/* >> >> + * Copyright (C) 2017 Jonathan Liu >> >> + * >> >> + * Jonathan Liu >> > >> > Is it? >> >> I could add you to the copyright since you did the old one. But the >> implementation is different. >> I intend to rework this I2C driver to use the FIFO more to avoid >> splitting larger transfers > 16 bytes and do the transfer in a single >> command. Would you like to be added to the copyright? > > You took some code that you didn't create, and added some more > stuff. The copyright on the initial code remains, and it has nothing > to do with whether the author wants it or not, (s)he should be > mentionned, along with you of course. > I will update the copyright header accordingly. >> >> +? SUN4I_HDMI_DDC_CMD_IMPLICIT_READ >> >> +: SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE, >> >> +hdmi->base + SUN4I_HDMI_DDC_CMD_REG); >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); >> >> + writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD, >> >> +hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); >> >> + >> >> + if (!(msg->flags & I2C_M_RD)) { >> >> + for (i = 0; i < count; i++) { >> >> + writeb(*msg->buf++, hdmi->base >> >> ++ SUN4I_HDMI_DDC_FIFO_DATA_REG); >> > >> > writesb? >> I intend to rework the FIFO handling so will need to continue using writeb. > > Then you'll change it when you'll rework it. Before then, you can use > writesb. > I have already reworked it in V3. >> > >> >> + --msg->len; >> >> + } >> >> + } >> >> + >> >> + if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, >> >> +reg, >> >> +!(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), >> >> +100, 10)) >> >> + return -EIO; >> >> + >> >> + reg = readl(hdmi->base + SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG); >> >> + reg &= ~SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST; >> I want to check all the other bits that are set if there are errors. > > Same thing here: you'll change it when that happens. I have already reworked it in V3. > >> > >> > You don't need to use that mask. >> > >> >> + if (reg != SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) >> >> + return -EIO; >> > >> > !(reg & SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE) would be enough. >> I want to check all the other bits that are set if there are errors. > > Same thing here. I have already reworked it in V3. > >> > >> >> + >> >> + if (msg->flags & I2C_M_RD) { >> >> + for (i = 0; i < count; i++) { >> >> + *msg->buf++ = readb(hdmi->base >> >> + + SUN4I_HDMI_DDC_FIFO_DATA_REG); >> > >> > readsb ? >> I am reworking the FIFO handling so I will need to continue to use readb. > > Same thing here. I have already reworked it in V3. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com Regards, Jonathan ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 21 June 2017 at 18:51, Maxime Ripard wrote: > On Thu, Jun 15, 2017 at 01:29:33AM +1000, Jonathan Liu wrote: >> The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: >> "As in the general case the DDC bus is accessible by the kernel at the I2C >> level, drivers must make all reasonable efforts to expose it as an I2C >> adapter and use drm_get_edid() instead of abusing this function." >> >> Exposing the DDC bus as an I2C adapter is more beneficial as it can be used >> for purposes other than reading the EDID such as modifying the EDID or >> using the HDMI DDC pins as an I2C bus through the I2C dev interface from >> userspace (e.g. i2c-tools). >> >> Implement this for A10s. >> >> Signed-off-by: Jonathan Liu >> --- >> Changes for v3: >> - Explain why drm_do_get_edid should be used and why it's better to expose >> it >>as an I2C adapter in commit message >> - Reorder bit defines in descending order for consistency >> - Keep old unused macros instead of removing them >> - The v2 algorithm split large transfers into 16 byte transfers but this may >>cause a large single write to be treated as multiple writes causing data >>corruption. The algorithm has been reworked to not split larger transfers >>and make more use of the FIFO to avoid this. >> - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to >>sun4i_hdmi_i2c.c >> - Reformatted code >> - Instead of masking bits that we don't want to check for errors, explicitly >>check the error bits >> - Clear error bits at start of transfer in case of error from previous >> transfer >> - Poll for completion of FIFO clear after setting FIFO clear bit >> >> Changes for v2: >> - Rebased against Maxime's sunxi-drm/for-next branch >> - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted >> if >>any of the calls after the I2C adapter is created fails >> - Remove unnecessary includes in sun4i_hdmi_i2c.c >> >> drivers/gpu/drm/sun4i/Makefile | 1 + >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 21 >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++- >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 >> + >> 4 files changed, 253 insertions(+), 90 deletions(-) >> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> >> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile >> index e29fd3a2ba9c..43c753cafc88 100644 >> --- a/drivers/gpu/drm/sun4i/Makefile >> +++ b/drivers/gpu/drm/sun4i/Makefile >> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o >> sun4i-drm-y += sun4i_framebuffer.o >> >> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o >> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o >> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o >> sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> index 2f2f2ff1ea63..018af9cbb593 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> @@ -96,6 +96,7 @@ >> #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) >> #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8) >> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8) >> #define SUN4I_HDMI_DDC_CTRL_RESETBIT(0) >> >> @@ -105,14 +106,30 @@ >> #define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8) >> #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff) >> >> +#define SUN4I_HDMI_DDC_INT_STATUS_REG0x50c >> +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) >> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOW BIT(6) >> +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOW BIT(5) >> +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4) >> +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERROR BIT(3) >> +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERROR BIT(2) >> +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERROR BIT(1) >> +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETE BIT(0) >> + >> #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 >> #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) >> >> +#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514 >> +#define S
[PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 21 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 + 4 files changed, 253 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..018af9cbb593 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,30 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOWBIT(5) +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3) +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2) +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1) +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0) + #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) +#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514 +#define SUN4I_HDMI_DDC_FIFO_STATUS_FULLBIT(6) +#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY BIT(5) + #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c #define SUN4I_HDMI_DDC_CMD_REG 0x520 #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 #define SUN4I_HDMI_DDC_CLK_REG 0x528 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3) @@ -123,6 +140,7 @@ #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLEBIT(8) #define SUN4I_HDMI_DDC_FIFO_SIZE 16 +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE 1023 enum sun4i_hdmi_pkt_type { SUN4I_HDMI_PKT_AVI = 2, @@ -146,6 +164,8 @@ struct sun4i_hdmi { struct clk *ddc_clk; struct clk *tmds_clk; + struct i2c_adapter *i2c; + struct sun4i_drv*drv; boo
[PATCH v3] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The documentation for drm_do_get_edid in drivers/gpu/drm/drm_edid.c states: "As in the general case the DDC bus is accessible by the kernel at the I2C level, drivers must make all reasonable efforts to expose it as an I2C adapter and use drm_get_edid() instead of abusing this function." Exposing the DDC bus as an I2C adapter is more beneficial as it can be used for purposes other than reading the EDID such as modifying the EDID or using the HDMI DDC pins as an I2C bus through the I2C dev interface from userspace (e.g. i2c-tools). Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v3: - Explain why drm_do_get_edid should be used and why it's better to expose it as an I2C adapter in commit message - Reorder bit defines in descending order for consistency - Keep old unused macros instead of removing them - The v2 algorithm split large transfers into 16 byte transfers but this may cause a large single write to be treated as multiple writes causing data corruption. The algorithm has been reworked to not split larger transfers and make more use of the FIFO to avoid this. - Moved the creation of the DDC clock from sun4i_hdmi_enc.c to sun4i_hdmi_i2c.c - Reformatted code - Instead of masking bits that we don't want to check for errors, explicitly check the error bits - Clear error bits at start of transfer in case of error from previous transfer - Poll for completion of FIFO clear after setting FIFO clear bit Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 21 drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 101 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 220 + 4 files changed, 253 insertions(+), 90 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..018af9cbb593 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -96,6 +96,7 @@ #define SUN4I_HDMI_DDC_CTRL_ENABLE BIT(31) #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) @@ -105,14 +106,30 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INT_STATUS_REG 0x50c +#define SUN4I_HDMI_DDC_INT_STATUS_ILLEGAL_FIFO_OPERATION BIT(7) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_RX_FIFO_UNDERFLOWBIT(6) +#define SUN4I_HDMI_DDC_INT_STATUS_DDC_TX_FIFO_UNDERFLOWBIT(5) +#define SUN4I_HDMI_DDC_INT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INT_STATUS_ARBITRATION_ERRORBIT(3) +#define SUN4I_HDMI_DDC_INT_STATUS_ACK_ERRORBIT(2) +#define SUN4I_HDMI_DDC_INT_STATUS_BUS_ERRORBIT(1) +#define SUN4I_HDMI_DDC_INT_STATUS_TRANSFER_COMPLETEBIT(0) + #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) +#define SUN4I_HDMI_DDC_FIFO_STATUS_REG 0x514 +#define SUN4I_HDMI_DDC_FIFO_STATUS_FULLBIT(6) +#define SUN4I_HDMI_DDC_FIFO_STATUS_EMPTY BIT(5) + #define SUN4I_HDMI_DDC_FIFO_DATA_REG 0x518 #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c #define SUN4I_HDMI_DDC_CMD_REG 0x520 #define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 #define SUN4I_HDMI_DDC_CLK_REG 0x528 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3) @@ -123,6 +140,7 @@ #define SUN4I_HDMI_DDC_LINE_CTRL_SCL_ENABLEBIT(8) #define SUN4I_HDMI_DDC_FIFO_SIZE 16 +#define SUN4I_HDMI_DDC_MAX_TRANSFER_SIZE 1023 enum sun4i_hdmi_pkt_type { SUN4I_HDMI_PKT_AVI = 2, @@ -146,6 +164,8 @@ struct sun4i_hdmi { struct clk *ddc_clk; struct clk *tmds_clk; + struct i2c_adapter *i2c; + struct sun4i_drv*drv; boo
Re: [PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Maxime, On 13 June 2017 at 21:15, Maxime Ripard wrote: > On Mon, Jun 12, 2017 at 03:52:35PM +1000, Jonathan Liu wrote: >> The drm_get_edid function should be used instead of drm_do_get_edid by >> exposing the DDC bus as an I2C adapter. Implement this for A10s. >> >> Signed-off-by: Jonathan Liu > > Your commit log should explain *why* that function should be used > instead, and why it's better to expose it as an i2c adadpter. Ok. > >> --- >> Changes for v2: >> - Rebased against Maxime's sunxi-drm/for-next branch >> - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted >> if >>any of the calls after the I2C adapter is created fails >> - Remove unnecessary includes in sun4i_hdmi_i2c.c >> >> drivers/gpu/drm/sun4i/Makefile | 1 + >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 11 ++- >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 103 +++-- >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 163 >> + >> 4 files changed, 189 insertions(+), 89 deletions(-) >> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> >> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile >> index e29fd3a2ba9c..43c753cafc88 100644 >> --- a/drivers/gpu/drm/sun4i/Makefile >> +++ b/drivers/gpu/drm/sun4i/Makefile >> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o >> sun4i-drm-y += sun4i_framebuffer.o >> >> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o >> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o >> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o >> sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o > > You probably also need to depend on the I2C framework in order to > work, right? In Kconfig, DRM_SUN4I depends on DRM which depends on I2C already. Is there anything else I need to do here? > >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> index 2f2f2ff1ea63..4c01dbe89cd9 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> @@ -97,6 +97,7 @@ >> #define SUN4I_HDMI_DDC_CTRL_START_CMDBIT(30) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASKBIT(8) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ(0 << 8) >> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) > > All the other bit defines are ordered by descending orders, please > keep it consistent. Ok. > >> #define SUN4I_HDMI_DDC_CTRL_RESETBIT(0) >> >> #define SUN4I_HDMI_DDC_ADDR_REG 0x504 >> @@ -105,6 +106,10 @@ >> #define SUN4I_HDMI_DDC_ADDR_OFFSET(off) (((off) & 0xff) << 8) >> #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr) ((addr) & 0xff) >> >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG 0x50c > > It is called INT_STATUS in the datasheet > >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4) >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETEBIT(0) >> + >> #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 >> #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) >> >> @@ -112,7 +117,8 @@ >> #define SUN4I_HDMI_DDC_BYTE_COUNT_REG0x51c >> >> #define SUN4I_HDMI_DDC_CMD_REG 0x520 >> -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ6 >> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE3 >> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 > > Same remark here, and why do you remove the old one? Ok, I will not remove any old macros. > >> >> #define SUN4I_HDMI_DDC_CLK_REG 0x528 >> #define SUN4I_HDMI_DDC_CLK_M(m) (((m) & 0x7) << 3) >> @@ -146,6 +152,8 @@ struct sun4i_hdmi { >> struct clk *ddc_clk; >> struct clk *tmds_clk; >> >> + struct i2c_adapter *i2c; >> + >> struct sun4i_drv*drv; >> >> boolhdmi_monitor; >> @@ -153,5 +161,6 @@ struct sun4i_hdmi { >> >> int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); >> int sun4i_tmds_create(struct sun4i_hdmi *hdmi); >> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi); >> >> #endif /* _SUN4I_HDMI_H_ */ >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> index d3398f6250ef..2a8c0b14eabc 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> @@ -29,8 +29,6 @@ >> #include "sun4i_hdmi.h"
[PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The drm_get_edid function should be used instead of drm_do_get_edid by exposing the DDC bus as an I2C adapter. Implement this for A10s. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 11 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 96 +++ drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 166 + 4 files changed, 190 insertions(+), 84 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2589bc92be59..8e7a70f67f04 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -100,6 +100,7 @@ #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) #define SUN4I_HDMI_DDC_ADDR_REG0x504 @@ -108,6 +109,10 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE BIT(0) + #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) @@ -115,7 +120,8 @@ #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c #define SUN4I_HDMI_DDC_CMD_REG 0x520 -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 #define SUN4I_HDMI_DDC_CLK_REG 0x528 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3) @@ -181,6 +187,8 @@ struct sun4i_hdmi { struct clk *ddc_clk; struct clk *tmds_clk; + struct i2c_adapter *i2c; + struct sun4i_drv*drv; boolhdmi_monitor; @@ -192,5 +200,6 @@ int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); int sun6i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); int sun4i_tmds_create(struct sun4i_hdmi *hdmi); int sun6i_tmds_create(struct sun4i_hdmi *hdmi); +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi); #endif /* _SUN4I_HDMI_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index e9abf93eb41c..6c9b11c4cf68 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -186,93 +186,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = { .destroy= drm_encoder_cleanup, }; -static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi, -unsigned int blk, unsigned int offset, -u8 *buf, unsigned int count) -{ - unsigned long reg; - int i; - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; - writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ, - hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - - writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) | - SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) | - SUN4I_HDMI_DDC_ADDR_OFFSET(offset) | - SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR), - hdmi->base + SUN4I_HDMI_DDC_ADDR_REG); - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); - writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR, - hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); - - writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); - writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ, - hdmi->base + SUN4I_HDMI_DDC_CMD_REG); - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - writel(reg | SUN4I_HDMI_DDC_CTRL_START_CMD, - hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - - if (readl_poll_timeout(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG, reg, - !(reg & SUN4I_HDMI_DDC_CTRL_START_CMD), - 100, 10)) - return -EIO; - - for (i = 0; i < count; i++) - buf[i] = readb(hdmi->base + SU
[PATCH v2] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
The drm_get_edid function should be used instead of drm_do_get_edid by exposing the DDC bus as an I2C adapter. Implement this for A10s. Signed-off-by: Jonathan Liu --- Changes for v2: - Rebased against Maxime's sunxi-drm/for-next branch - Fix up error paths in sun4i_hdmi_bind so that the I2C adapter is deleted if any of the calls after the I2C adapter is created fails - Remove unnecessary includes in sun4i_hdmi_i2c.c drivers/gpu/drm/sun4i/Makefile | 1 + drivers/gpu/drm/sun4i/sun4i_hdmi.h | 11 ++- drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 103 +++-- drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 163 + 4 files changed, 189 insertions(+), 89 deletions(-) create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile index e29fd3a2ba9c..43c753cafc88 100644 --- a/drivers/gpu/drm/sun4i/Makefile +++ b/drivers/gpu/drm/sun4i/Makefile @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o sun4i-drm-y += sun4i_framebuffer.o sun4i-drm-hdmi-y += sun4i_hdmi_enc.o +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h b/drivers/gpu/drm/sun4i/sun4i_hdmi.h index 2f2f2ff1ea63..4c01dbe89cd9 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h @@ -97,6 +97,7 @@ #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) #define SUN4I_HDMI_DDC_ADDR_REG0x504 @@ -105,6 +106,10 @@ #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4) +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE BIT(0) + #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) @@ -112,7 +117,8 @@ #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c #define SUN4I_HDMI_DDC_CMD_REG 0x520 -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 #define SUN4I_HDMI_DDC_CLK_REG 0x528 #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3) @@ -146,6 +152,8 @@ struct sun4i_hdmi { struct clk *ddc_clk; struct clk *tmds_clk; + struct i2c_adapter *i2c; + struct sun4i_drv*drv; boolhdmi_monitor; @@ -153,5 +161,6 @@ struct sun4i_hdmi { int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); int sun4i_tmds_create(struct sun4i_hdmi *hdmi); +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi); #endif /* _SUN4I_HDMI_H_ */ diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c index d3398f6250ef..2a8c0b14eabc 100644 --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c @@ -29,8 +29,6 @@ #include "sun4i_hdmi.h" #include "sun4i_tcon.h" -#define DDC_SEGMENT_ADDR 0x30 - static inline struct sun4i_hdmi * drm_encoder_to_sun4i_hdmi(struct drm_encoder *encoder) { @@ -184,93 +182,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs = { .destroy= drm_encoder_cleanup, }; -static int sun4i_hdmi_read_sub_block(struct sun4i_hdmi *hdmi, -unsigned int blk, unsigned int offset, -u8 *buf, unsigned int count) -{ - unsigned long reg; - int i; - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - reg &= ~SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK; - writel(reg | SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ, - hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - - writel(SUN4I_HDMI_DDC_ADDR_SEGMENT(offset >> 8) | - SUN4I_HDMI_DDC_ADDR_EDDC(DDC_SEGMENT_ADDR << 1) | - SUN4I_HDMI_DDC_ADDR_OFFSET(offset) | - SUN4I_HDMI_DDC_ADDR_SLAVE(DDC_ADDR), - hdmi->base + SUN4I_HDMI_DDC_ADDR_REG); - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); - writel(reg | SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR, - hdmi->base + SUN4I_HDMI_DDC_FIFO_CTRL_REG); - - writel(count, hdmi->base + SUN4I_HDMI_DDC_BYTE_COUNT_REG); - writel(SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ, - hdmi->base + SUN4I_HDMI_DDC_CMD_REG); - - reg = readl(hdmi->base + SUN4I_HDMI_DDC_CTRL_REG); - writel(re
Re: [PATCH] drm/sun4i: hdmi: Implement I2C adapter for A10s DDC bus
Hi Chen-Yu, On 12 June 2017 at 13:28, Chen-Yu Tsai wrote: > On Mon, Jun 12, 2017 at 10:12 AM, Jonathan Liu wrote: >> The drm_get_edid function should be used instead of drm_do_get_edid by >> exposing the DDC bus as an I2C adapter. Implement this for A10s. > > Nice! It is my first Linux kernel driver so there may be other things I have overlooked. I am not sure about the behaviour of I2C writes larger than 16 bytes but the EEPROMs I have tested only have a write buffer of 8 bytes so writes are limited to a maximum of 8 bytes at a time anyway. > >> >> Signed-off-by: Jonathan Liu >> --- >> drivers/gpu/drm/sun4i/Makefile | 1 + >> drivers/gpu/drm/sun4i/sun4i_hdmi.h | 11 ++- >> drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c | 96 +++ >> drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c | 166 >> + >> 4 files changed, 190 insertions(+), 84 deletions(-) >> create mode 100644 drivers/gpu/drm/sun4i/sun4i_hdmi_i2c.c >> >> diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile >> index e29fd3a2ba9c..43c753cafc88 100644 >> --- a/drivers/gpu/drm/sun4i/Makefile >> +++ b/drivers/gpu/drm/sun4i/Makefile >> @@ -2,6 +2,7 @@ sun4i-drm-y += sun4i_drv.o >> sun4i-drm-y += sun4i_framebuffer.o >> >> sun4i-drm-hdmi-y += sun4i_hdmi_enc.o >> +sun4i-drm-hdmi-y += sun4i_hdmi_i2c.o >> sun4i-drm-hdmi-y += sun4i_hdmi_ddc_clk.o >> sun4i-drm-hdmi-y += sun4i_hdmi_tmds_clk.o >> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> index 2589bc92be59..8e7a70f67f04 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi.h >> @@ -100,6 +100,7 @@ >> #define SUN4I_HDMI_DDC_CTRL_START_CMD BIT(30) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_MASK BIT(8) >> #define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_READ (0 << 8) >> +#define SUN4I_HDMI_DDC_CTRL_FIFO_DIR_WRITE (1 << 8) >> #define SUN4I_HDMI_DDC_CTRL_RESET BIT(0) >> >> #define SUN4I_HDMI_DDC_ADDR_REG0x504 >> @@ -108,6 +109,10 @@ >> #define SUN4I_HDMI_DDC_ADDR_OFFSET(off)(((off) & 0xff) << 8) >> #define SUN4I_HDMI_DDC_ADDR_SLAVE(addr)((addr) & 0xff) >> >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_REG0x50c >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_FIFO_REQUEST BIT(4) >> +#define SUN4I_HDMI_DDC_INTERRUPT_STATUS_TRANSFER_COMPLETE BIT(0) >> + >> #define SUN4I_HDMI_DDC_FIFO_CTRL_REG 0x510 >> #define SUN4I_HDMI_DDC_FIFO_CTRL_CLEAR BIT(31) >> >> @@ -115,7 +120,8 @@ >> #define SUN4I_HDMI_DDC_BYTE_COUNT_REG 0x51c >> >> #define SUN4I_HDMI_DDC_CMD_REG 0x520 >> -#define SUN4I_HDMI_DDC_CMD_EXPLICIT_EDDC_READ 6 >> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_WRITE 3 >> +#define SUN4I_HDMI_DDC_CMD_IMPLICIT_READ 5 >> >> #define SUN4I_HDMI_DDC_CLK_REG 0x528 >> #define SUN4I_HDMI_DDC_CLK_M(m)(((m) & 0x7) << 3) >> @@ -181,6 +187,8 @@ struct sun4i_hdmi { >> struct clk *ddc_clk; >> struct clk *tmds_clk; >> >> + struct i2c_adapter *i2c; >> + >> struct sun4i_drv*drv; >> >> boolhdmi_monitor; >> @@ -192,5 +200,6 @@ int sun4i_ddc_create(struct sun4i_hdmi *hdmi, struct clk >> *clk); >> int sun6i_ddc_create(struct sun4i_hdmi *hdmi, struct clk *clk); >> int sun4i_tmds_create(struct sun4i_hdmi *hdmi); >> int sun6i_tmds_create(struct sun4i_hdmi *hdmi); >> +int sun4i_hdmi_i2c_create(struct sun4i_hdmi *hdmi); > > However you seem to have based it on the wrong branch/patches. > You based them on my A31 HDMI patches, instead of what Maxime has > in his sunxi-drm/for-next branch. > > I could add this to my A31 patches, though it probably won't make > 4.13. Alternatively you could rebase them on top of Maxime's branch. Yes, I forgot about that. I will rebase against sunxi-drm/for-next for V2. > >> >> #endif /* _SUN4I_HDMI_H_ */ >> diff --git a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> index e9abf93eb41c..6c9b11c4cf68 100644 >> --- a/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> +++ b/drivers/gpu/drm/sun4i/sun4i_hdmi_enc.c >> @@ -186,93 +186,13 @@ static const struct drm_encoder_funcs sun4i_hdmi_funcs >> = { >> .destroy= drm_encoder_cleanup, >> }; >> >> -static int sun4i_hdmi_read_
[PATCH][v2] drm/sun4i: rgb: Enable panel after controller
The panel should be enabled after the controller so that we do not have visual glitches on the panel while the controller is setup. Similarly, the panel should be disabled before the controller. Signed-off-by: Jonathan Liu --- Changes in v2: - Changed the commit message to be clearer drivers/gpu/drm/sun4i/sun4i_rgb.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index c3ff10f..4e4bea6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -152,15 +152,16 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Enabling RGB output\n"); - if (!IS_ERR(tcon->panel)) { + if (!IS_ERR(tcon->panel)) drm_panel_prepare(tcon->panel); - drm_panel_enable(tcon->panel); - } /* encoder->bridge can be NULL; drm_bridge_enable checks for it */ drm_bridge_enable(encoder->bridge); sun4i_tcon_channel_enable(tcon, 0); + + if (!IS_ERR(tcon->panel)) + drm_panel_enable(tcon->panel); } static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) @@ -171,15 +172,16 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Disabling RGB output\n"); + if (!IS_ERR(tcon->panel)) + drm_panel_disable(tcon->panel); + sun4i_tcon_channel_disable(tcon, 0); /* encoder->bridge can be NULL; drm_bridge_disable checks for it */ drm_bridge_disable(encoder->bridge); - if (!IS_ERR(tcon->panel)) { - drm_panel_disable(tcon->panel); + if (!IS_ERR(tcon->panel)) drm_panel_unprepare(tcon->panel); - } } static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder, -- 2.10.0
[PATCH] drm/sun4i: rgb: Enable panel after controller
Hi Maxime, On 23 September 2016 at 23:16, Maxime Ripard wrote: > On Thu, Sep 22, 2016 at 08:03:31AM +1000, Jonathan Liu wrote: >> Hi Maxime, >> >> On Thursday, 22 September 2016, Maxime Ripard > free-electrons. >> com> wrote: >> >> > On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote: >> > > The panel should be enabled after the controller so that the panel >> > > prepare/enable delays are properly taken into account. Similarly, the >> > > panel should be disabled before the controller so that the panel >> > > unprepare/disable delays are properly taken into account. >> > > >> > > This is useful for avoiding visual glitches. >> > >> > This is not really taking any delays into account, especially since >> > drm_panel_enable and prepare are suppose to block until their >> > operation is complete. >> >> >> drm_panel_prepare turns on power to the LCD using enable-gpios property of >> the panel and then blocks for prepare delay. The prepare delay for panel >> can be set to how long it takes between the time the panel is powered to >> when it is ready to receive images. If backlight property is specified the >> backlight will be off while the panel is powered on. >> >> drm_panel_enable blocks for enable delay and then turns on the backlight. >> The enable delay can be set to how long it takes for panel to start making >> the image visible after receiving the first valid frame. For example if the >> panel starts off as white and the TFT takes some time to initialize to >> black before it shows the image being received. >> >> Refer to drivers/gpu/drm/panel-panel.simple.c for details. > > From drm_panel.h: > > """ > * drm_panel_enable - enable a panel > * @panel: DRM panel > * > * Calling this function will cause the panel display drivers to be turned on > * and the backlight to be enabled. Content will be visible on screen after > * this call completes. > """ > > """ > * drm_panel_prepare - power on a panel > * @panel: DRM panel > * > * Calling this function will enable power and deassert any reset signals to > * the panel. After this has completed it is possible to communicate with any > * integrated circuitry via a command bus. > """ > > Those comments clearly says that the caller should not have to deal > with the delays, even more so by just moving calls around and hoping > that the code running in between is adding enough delay for the panel > to behave properly. > > Maxime > > -- > Maxime Ripard, Free Electrons > Embedded Linux and Kernel engineering > http://free-electrons.com https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/include/drm/drm_panel.h#n34 In comment for struct drm_panel_funcs: /** * struct drm_panel_funcs - perform operations on a given panel * @disable: disable panel (turn off back light, etc.) * @unprepare: turn off panel * @prepare: turn on panel and perform set up * @enable: enable panel (turn on back light, etc.) * @get_modes: add modes to the connector that the panel is attached to and * return the number of modes added * @get_timings: copy display timings into the provided array and return * the number of display timings available * * The .prepare() function is typically called before the display controller * starts to transmit video data. Panel drivers can use this to turn the panel * on and wait for it to become ready. If additional configuration is required * (via a control bus such as I2C, SPI or DSI for example) this is a good time * to do that. * * After the display controller has started transmitting video data, it's safe * to call the .enable() function. This will typically enable the backlight to * make the image on screen visible. Some panels require a certain amount of * time or frames before the image is displayed. This function is responsible * for taking this into account before enabling the backlight to avoid visual * glitches. * * Before stopping video transmission from the display controller it can be * necessary to turn off the panel to avoid visual glitches. This is done in * the .disable() function. Analogously to .enable() this typically involves * turning off the backlight and waiting for some time to make sure no image * is visible on the panel. It is then safe for the display controller to * cease transmission of video data. * * To save power when no video data is transmitted, a driver can power down * the panel. This is the job of the .unprepare() function. */ https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-simple.c?id=refs/tags/v4.8-rc7#n39
[PATCH] drm/sun4i: rgb: Enable panel after controller
Hi Maxime, On 22 September 2016 at 07:03, Maxime Ripard wrote: > On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote: >> The panel should be enabled after the controller so that the panel >> prepare/enable delays are properly taken into account. Similarly, the >> panel should be disabled before the controller so that the panel >> unprepare/disable delays are properly taken into account. >> >> This is useful for avoiding visual glitches. > > This is not really taking any delays into account, especially since > drm_panel_enable and prepare are suppose to block until their > operation is complete. drm_panel_prepare turns on power to the LCD using enable-gpios property of the panel and then blocks for prepare delay. The prepare delay for panel can be set to how long it takes between the time the panel is powered to when it is ready to receive images. If backlight property is specified the backlight will be off while the panel is powered on. drm_panel_enable blocks for enable delay and then turns on the backlight. The enable delay can be set to how long it takes for panel to start making the image visible after receiving the first valid frame. For example if the panel starts off as white and the TFT takes some time to initialize to black before it shows the image being received. Refer to drivers/gpu/drm/panel-panel.simple.c for details. Regards, Jonathan
[PATCH] drm/sun4i: rgb: Enable panel after controller
Hi Maxime, On Thursday, 22 September 2016, Maxime Ripard wrote: > On Wed, Sep 21, 2016 at 11:03:04PM +1000, Jonathan Liu wrote: > > The panel should be enabled after the controller so that the panel > > prepare/enable delays are properly taken into account. Similarly, the > > panel should be disabled before the controller so that the panel > > unprepare/disable delays are properly taken into account. > > > > This is useful for avoiding visual glitches. > > This is not really taking any delays into account, especially since > drm_panel_enable and prepare are suppose to block until their > operation is complete. drm_panel_prepare turns on power to the LCD using enable-gpios property of the panel and then blocks for prepare delay. The prepare delay for panel can be set to how long it takes between the time the panel is powered to when it is ready to receive images. If backlight property is specified the backlight will be off while the panel is powered on. drm_panel_enable blocks for enable delay and then turns on the backlight. The enable delay can be set to how long it takes for panel to start making the image visible after receiving the first valid frame. For example if the panel starts off as white and the TFT takes some time to initialize to black before it shows the image being received. Refer to drivers/gpu/drm/panel-panel.simple.c for details. Regards, Jonathan -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20160922/28e3d06f/attachment.html>
[PATCH] drm/sun4i: rgb: Enable panel after controller
The panel should be enabled after the controller so that the panel prepare/enable delays are properly taken into account. Similarly, the panel should be disabled before the controller so that the panel unprepare/disable delays are properly taken into account. This is useful for avoiding visual glitches. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index c3ff10f..4e4bea6 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -152,15 +152,16 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Enabling RGB output\n"); - if (!IS_ERR(tcon->panel)) { + if (!IS_ERR(tcon->panel)) drm_panel_prepare(tcon->panel); - drm_panel_enable(tcon->panel); - } /* encoder->bridge can be NULL; drm_bridge_enable checks for it */ drm_bridge_enable(encoder->bridge); sun4i_tcon_channel_enable(tcon, 0); + + if (!IS_ERR(tcon->panel)) + drm_panel_enable(tcon->panel); } static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) @@ -171,15 +172,16 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Disabling RGB output\n"); + if (!IS_ERR(tcon->panel)) + drm_panel_disable(tcon->panel); + sun4i_tcon_channel_disable(tcon, 0); /* encoder->bridge can be NULL; drm_bridge_disable checks for it */ drm_bridge_disable(encoder->bridge); - if (!IS_ERR(tcon->panel)) { - drm_panel_disable(tcon->panel); + if (!IS_ERR(tcon->panel)) drm_panel_unprepare(tcon->panel); - } } static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder, -- 2.10.0
[PATCH] drm/panel: simple: Fix bus_format for the Olimex LCD-OLinuXino-4.3TS
The format is RGB888 not RGB666. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/panel/panel-simple.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c index 85143d1..d4aa68e 100644 --- a/drivers/gpu/drm/panel/panel-simple.c +++ b/drivers/gpu/drm/panel/panel-simple.c @@ -1186,7 +1186,7 @@ static const struct panel_desc olimex_lcd_olinuxino_43ts = { .width = 105, .height = 67, }, - .bus_format = MEDIA_BUS_FMT_RGB666_1X18, + .bus_format = MEDIA_BUS_FMT_RGB888_1X24, }; /* -- 2.9.3
[PATCH] drm/sun4i: rgb: add missing calls to drm_panel_{prepare, unprepare}
If the enable-gpios property of a simple panel in device tree is set, the GPIO is not toggled on/off because of missing calls to drm_panel_prepare and drm_panel_unprepare. Signed-off-by: Jonathan Liu --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index f5bbac6..d6943e9 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -151,6 +151,7 @@ static void sun4i_rgb_encoder_enable(struct drm_encoder *encoder) DRM_DEBUG_DRIVER("Enabling RGB output\n"); + drm_panel_prepare(tcon->panel); drm_panel_enable(tcon->panel); sun4i_tcon_channel_enable(tcon, 0); } @@ -165,6 +166,7 @@ static void sun4i_rgb_encoder_disable(struct drm_encoder *encoder) sun4i_tcon_channel_disable(tcon, 0); drm_panel_disable(tcon->panel); + drm_panel_unprepare(tcon->panel); } static void sun4i_rgb_encoder_mode_set(struct drm_encoder *encoder, -- 2.9.3
[linux-sunxi] [PATCH 11/19] drm: sun4i: Add composite output
On 31 October 2015 at 01:20, Maxime Ripard wrote: > Some Allwinner SoCs have an IP called the TV encoder that is used to output > composite and VGA signals. In such a case, we need to use the second TCON > channel. > > Add support for that TV encoder. > > Signed-off-by: Maxime Ripard > --- > drivers/gpu/drm/sun4i/Makefile| 1 + > drivers/gpu/drm/sun4i/sun4i_drv.c | 10 +- > drivers/gpu/drm/sun4i/sun4i_tv.c | 532 > ++ > drivers/gpu/drm/sun4i/sun4i_tv.h | 18 ++ > 4 files changed, 560 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.c > create mode 100644 drivers/gpu/drm/sun4i/sun4i_tv.h > > diff --git a/drivers/gpu/drm/sun4i/Makefile b/drivers/gpu/drm/sun4i/Makefile > index 4d230b658a05..fc0dc8be82d9 100644 > --- a/drivers/gpu/drm/sun4i/Makefile > +++ b/drivers/gpu/drm/sun4i/Makefile > @@ -6,5 +6,6 @@ sun4i-drm-y += sun4i_layer.o > sun4i-drm-y += sun4i_tcon.o > > sun4i-drm-y += sun4i_rgb.o > +sun4i-drm-y += sun4i_tv.o > > obj-$(CONFIG_DRM_SUN4I)+= sun4i-drm.o > diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c > b/drivers/gpu/drm/sun4i/sun4i_drv.c > index f2c9c8a2eb75..3cf7a9e89afa 100644 > --- a/drivers/gpu/drm/sun4i/sun4i_drv.c > +++ b/drivers/gpu/drm/sun4i/sun4i_drv.c > @@ -22,6 +22,7 @@ > #include "sun4i_layer.h" > #include "sun4i_rgb.h" > #include "sun4i_tcon.h" > +#include "sun4i_tv.h" > > static void sun4i_drv_preclose(struct drm_device *drm, >struct drm_file *file_priv) > @@ -134,12 +135,18 @@ static int sun4i_drv_load(struct drm_device *drm, > unsigned long flags) > goto err_free_crtc; > } > > + ret = sun4i_tv_init(drm); > + if (ret) { > + dev_err(drm->dev, "Couldn't create our RGB output\n"); RGB should be composite. Seems like a copy-paste error. > + goto err_free_rgb; > + } > + > /* Create our framebuffer */ > drv->fbdev = sun4i_framebuffer_init(drm); > if (IS_ERR(drv->fbdev)) { > dev_err(drm->dev, "Couldn't create our framebuffer\n"); > ret = PTR_ERR(drv->fbdev); > - goto err_free_rgb; > + goto err_free_tv; > } > > /* Enable connectors polling */ Regards, Jonathan