Re: [PATCH net-next v2 2/2] drivers: net: Remove device_node checks with of_mdiobus_register()

[Jose Abreu]

On 16-05-2018 00:56, Florian Fainelli wrote:
> A number of drivers have the following pattern:
>
> if (np)
>   of_mdiobus_register()
> else
>   mdiobus_register()
>
> which the implementation of of_mdiobus_register() now takes care of.
> Remove that pattern in drivers that strictly adhere to it.
>
> Signed-off-by: Florian Fainelli <f.faine...@gmail.com>
> ---
>  drivers/net/dsa/bcm_sf2.c |  8 ++--
>  drivers/net/dsa/mv88e6xxx/chip.c  |  5 +
>  drivers/net/ethernet/cadence/macb_main.c  | 12 +++-
>  drivers/net/ethernet/freescale/fec_main.c |  8 ++--
>  drivers/net/ethernet/marvell/mvmdio.c |  5 +
>  drivers/net/ethernet/renesas/sh_eth.c | 11 +++
>  drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |  5 +

For stmmac:

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Thanks and Best Regards,
Jose Miguel Abreu

>  drivers/net/ethernet/ti/davinci_mdio.c|  8 +++-
>  drivers/net/phy/mdio-gpio.c   |  6 +-
>  drivers/net/phy/mdio-mscc-miim.c  |  6 +-
>  drivers/net/usb/lan78xx.c |  7 ++-
>  11 files changed, 20 insertions(+), 61 deletions(-)
>

Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration

[Jose Abreu]

Hi Laurent,


On 02-03-2017 15:38, Laurent Pinchart wrote:
> Hi Jose,
>
> On Thursday 02 Mar 2017 14:50:02 Jose Abreu wrote:
>> On 02-03-2017 13:41, Laurent Pinchart wrote:
>>>> Hmm, this is kind of confusing. Why do you need a phy_ops and
>>>> then a separate configure_phy function? Can't we just use phy_ops
>>>> always? If its external phy then it would need to implement all
>>>> phy_ops functions.
>>> The phy_ops are indeed meant to support vendor PHYs. The .configure_phy()
>>> operation is meant to support Synopsys PHYs that don't comply with the
>>> HDMI TX MHL and 3D PHYs I2C register layout and thus need a different
>>> configure function, but can share the other operations with the HDMI TX
>>> MHL and 3D PHYs. Ideally I'd like to move that code to the dw-hdmi core,
>>> but at the moment I don't have enough information to decide whether the
>>> register layout corresponds to the standard Synopsys HDMI TX 2.0 PHY or
>>> if it has been modified by the vendor. Once we'll have a second SoC using
>>> the same PHY we should have more information to consolidate the code. Of
>>> course, if you can share the HDMI TX 2.0 PHY datasheet, I won't mind
>>> reworking the code now ;-)
>> Well, if I want to keep my job I can't share the datasheet, and I
>> do want to keep my job :)
> That's so selfish :-D
>
>> As far as I know this shouldn't change much. I already used (it
>> was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY.
> I really wonder what exactly has been integrated in the Renesas R-Car Gen3 
> SoC. The PHY is certainly reported as an HDMI TX 2.0 PHY, but the registers 
> seem different compared to the 2.0 PHY you've tested.
>
>> But I am not following your logic here, sorry: You are mentioning a
>> custom phy, right?
> Custom is probably a bad name. From what I've been told it's a standard 
> Synopsys PHY, but I can't rule out vendor-specific customizations.
>
>> If its custom then it should implement its own phy_ops. And I don't think
>> that phy logic should go into core, this should all be extracted because I
>> really believe it will make the code easier to read. Imagine this
>> organization:
>>
>> Folders/Files:
>> drm/bridge/dw-hdmi/dw-hdmi-core.c
>> drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
>> drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
>> drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
>> Structures:
>> dw_hdmi_pdata
>> dw_hdmi_phy_pdata
>> dw_hdmi_phy_ops
> That looks good to me.
>
>> As the phy is interacted using controller we would need to pass
>> some callbacks to the phy, but ultimately the phy would be a
>> platform driver which could have its own compatible string that
>> would be associated with controller (not sure exactly about this
>> because I only used this in non-dt).
> We already have glue code, having to provide separate glue and PHY drivers 
> seems a bit overkill to me. If we could get rid of glue code by adding a node 
> for the PHY in DT that would be nice, but if we have to keep the glue then we 
> can as well use platform data there.
>
>> This is just an idea though. I followed this logic in the RX side
>> and its very easy to add a new phy now, its a matter of creating
>> a new file, implement ops and associate it with controller. Of
>> course some phys will be very similar, for that we can use minor
>> tweaks via id detection.
>>
>> What do you think?
> It sounds neat overall, but I wonder whether it should really block this 
> series, or be added on top of it :-) I think we're already moving in the 
> right 
> direction here.
>

This series is definitely a good starting point and a good
improvement. We can think later about abstracting even more the
phy from the controller. I think this will be a major step and
will reflect better how the HW is modeled.

You can add my reviewed-by in all the patches in this series in
your next submission (or in the merge).

Also, if you do a next submission what do you think about moving
all the dw-hdmi files to a new folder called for example
'synopsys' or 'dw-hdmi'? Because we already have 5 files. I think
'synopsys' instead of 'dw-hdmi' would be better because in the
future we may add support for other protocols (for example
display port).

Side note: I think you missed in the cc Archit Taneja

Best regards,
Jose Miguel Abreu

Re: [PATCH v4 8/9] drm: bridge: dw-hdmi: Remove device type from platform data

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> From: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
>
> The device type isn't used anymore now that workarounds and PHY-specific
> operations are performed based on version information read at runtime.
> Remove it.
>
> Signed-off-by: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c| 2 --
>  drivers/gpu/drm/imx/dw_hdmi-imx.c   | 2 --
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 1 -
>  include/drm/bridge/dw_hdmi.h| 7 ---
>  4 files changed, 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index b835d81bb471..132c00685796 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -127,7 +127,6 @@ struct dw_hdmi {
>   struct drm_connector connector;
>   struct drm_bridge bridge;
>  
> - enum dw_hdmi_devtype dev_type;
>   unsigned int version;
>  
>   struct platform_device *audio;
> @@ -2014,7 +2013,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>  
>   hdmi->plat_data = plat_data;
>   hdmi->dev = dev;
> - hdmi->dev_type = plat_data->dev_type;
>   hdmi->sample_rate = 48000;
>   hdmi->disabled = true;
>   hdmi->rxsense = true;
> diff --git a/drivers/gpu/drm/imx/dw_hdmi-imx.c 
> b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> index f645275e6e63..f039641070ac 100644
> --- a/drivers/gpu/drm/imx/dw_hdmi-imx.c
> +++ b/drivers/gpu/drm/imx/dw_hdmi-imx.c
> @@ -175,7 +175,6 @@ static struct dw_hdmi_plat_data imx6q_hdmi_drv_data = {
>   .mpll_cfg   = imx_mpll_cfg,
>   .cur_ctr= imx_cur_ctr,
>   .phy_config = imx_phy_config,
> - .dev_type   = IMX6Q_HDMI,
>   .mode_valid = imx6q_hdmi_mode_valid,
>  };
>  
> @@ -183,7 +182,6 @@ static struct dw_hdmi_plat_data imx6dl_hdmi_drv_data = {
>   .mpll_cfg = imx_mpll_cfg,
>   .cur_ctr  = imx_cur_ctr,
>   .phy_config = imx_phy_config,
> - .dev_type = IMX6DL_HDMI,
>   .mode_valid = imx6dl_hdmi_mode_valid,
>  };
>  
> diff --git a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c 
> b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> index a6d4a0236e8f..d53827413996 100644
> --- a/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c
> @@ -237,7 +237,6 @@ static const struct dw_hdmi_plat_data 
> rockchip_hdmi_drv_data = {
>   .mpll_cfg   = rockchip_mpll_cfg,
>   .cur_ctr= rockchip_cur_ctr,
>   .phy_config = rockchip_phy_config,
> - .dev_type   = RK3288_HDMI,
>  };
>  
>  static const struct of_device_id dw_hdmi_rockchip_dt_ids[] = {
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index dd330259a3dc..545f04fae3b6 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -21,12 +21,6 @@ enum {
>   DW_HDMI_RES_MAX,
>  };
>  
> -enum dw_hdmi_devtype {
> - IMX6Q_HDMI,
> - IMX6DL_HDMI,
> - RK3288_HDMI,
> -};
> -
>  enum dw_hdmi_phy_type {
>   DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
>   DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> @@ -65,7 +59,6 @@ struct dw_hdmi_phy_ops {
>  };
>  
>  struct dw_hdmi_plat_data {
> - enum dw_hdmi_devtype dev_type;
>   enum drm_mode_status (*mode_valid)(struct drm_connector *connector,
>  struct drm_display_mode *mode);
>  

Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration

[Jose Abreu]

Hi Laurent,


On 02-03-2017 13:41, Laurent Pinchart wrote:
>
>> Hmm, this is kind of confusing. Why do you need a phy_ops and
>> then a separate configure_phy function? Can't we just use phy_ops
>> always? If its external phy then it would need to implement all
>> phy_ops functions.
> The phy_ops are indeed meant to support vendor PHYs. The .configure_phy() 
> operation is meant to support Synopsys PHYs that don't comply with the HDMI 
> TX 
> MHL and 3D PHYs I2C register layout and thus need a different configure 
> function, but can share the other operations with the HDMI TX MHL and 3D 
> PHYs. 
> Ideally I'd like to move that code to the dw-hdmi core, but at the moment I 
> don't have enough information to decide whether the register layout 
> corresponds to the standard Synopsys HDMI TX 2.0 PHY or if it has been 
> modified by the vendor. Once we'll have a second SoC using the same PHY we 
> should have more information to consolidate the code. Of course, if you can 
> share the HDMI TX 2.0 PHY datasheet, I won't mind reworking the code now ;-)
>

Well, if I want to keep my job I can't share the datasheet, and I
do want to keep my job :)

As far as I know this shouldn't change much. I already used (it
was like a year ago) the dw-hdmi driver in a HDMI TX 2.0 PHY. But
I am not following your logic here, sorry: You are mentioning a
custom phy, right? If its custom then it should implement its own
phy_ops. And I don't think that phy logic should go into core,
this should all be extracted because I really believe it will
make the code easier to read. Imagine this organization:

Folders/Files:
drm/bridge/dw-hdmi/dw-hdmi-core.c
drm/bridge/dw-hdmi/dw-hdmi-phy-synopsys.c
drm/bridge/dw-hdmi/dw-hdmi-phy-*renesas*.c
drm/bridge/dw-hdmi/dw-hdmi-phy-something.c
Structures:
dw_hdmi_pdata
dw_hdmi_phy_pdata
dw_hdmi_phy_ops

As the phy is interacted using controller we would need to pass
some callbacks to the phy, but ultimately the phy would be a
platform driver which could have its own compatible string that
would be associated with controller (not sure exactly about this
because I only used this in non-dt).

This is just an idea though. I followed this logic in the RX side
and its very easy to add a new phy now, its a matter of creating
a new file, implement ops and associate it with controller. Of
course some phys will be very similar, for that we can use minor
tweaks via id detection.

What do you think?

Best regards,
Jose Miguel Abreu

Re: [PATCH v4 7/9] drm: bridge: dw-hdmi: Add support for custom PHY configuration

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> From: Kieran Bingham 
>
> The DWC HDMI TX controller interfaces with a companion PHY. While
> Synopsys provides multiple standard PHYs, SoC vendors can also integrate
> a custom PHY.
>
> Modularize PHY configuration to support vendor PHYs through platform
> data. The existing PHY configuration code was originally written to
> support the DWC HDMI 3D TX PHY, and seems to be compatible with the DWC
> MLP PHY. The HDMI 2.0 PHY will require a separate configuration
> function.
>
> Signed-off-by: Kieran Bingham 
> Signed-off-by: Laurent Pinchart 
> ---
> Changes since v1:
>
> - Check pdata->phy_configure in hdmi_phy_configure() to avoid
>   dereferencing NULL pointer.
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 109 
> ++-
>  include/drm/bridge/dw_hdmi.h |   7 +++
>  2 files changed, 81 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index cb2703862be2..b835d81bb471 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -118,6 +118,9 @@ struct dw_hdmi_phy_data {
>   const char *name;
>   unsigned int gen;
>   bool has_svsret;
> + int (*configure)(struct dw_hdmi *hdmi,
> +  const struct dw_hdmi_plat_data *pdata,
> +  unsigned long mpixelclock);
>  };
>  
>  struct dw_hdmi {
> @@ -860,8 +863,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, 
> int msec)
>   return true;
>  }
>  
> -static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> -  unsigned char addr)
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +unsigned char addr)
>  {
>   hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
>   hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR);
> @@ -873,6 +876,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, 
> unsigned short data,
>   HDMI_PHY_I2CM_OPERATION_ADDR);
>   hdmi_phy_wait_i2c_done(hdmi, 1000);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>  
>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>  {
> @@ -993,37 +997,67 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
>   return 0;
>  }
>  
> -static int hdmi_phy_configure(struct dw_hdmi *hdmi)
> +/*
> + * PHY configuration function for the DWC HDMI 3D TX PHY. Based on the 
> available
> + * information the DWC MHL PHY has the same register layout and is thus also
> + * supported by this function.
> + */
> +static int hdmi_phy_configure_dwc_hdmi_3d_tx(struct dw_hdmi *hdmi,
> + const struct dw_hdmi_plat_data *pdata,
> + unsigned long mpixelclock)
>  {
> - const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
> - const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>   const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>   const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
>   const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
>  
>   /* PLL/MPLL Cfg - always match on final entry */
>   for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - mpll_config->mpixelclock)
> + if (mpixelclock <= mpll_config->mpixelclock)
>   break;
>  
>   for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - curr_ctrl->mpixelclock)
> + if (mpixelclock <= curr_ctrl->mpixelclock)
>   break;
>  
>   for (; phy_config->mpixelclock != ~0UL; phy_config++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - phy_config->mpixelclock)
> + if (mpixelclock <= phy_config->mpixelclock)
>   break;
>  
>   if (mpll_config->mpixelclock == ~0UL ||
>   curr_ctrl->mpixelclock == ~0UL ||
> - phy_config->mpixelclock == ~0UL) {
> - dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
> - hdmi->hdmi_data.video_mode.mpixelclock);
> + phy_config->mpixelclock == ~0UL)
>   return -EINVAL;
> - }
> +
> + dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[0].cpce,
> +   HDMI_3D_TX_PHY_CPCE_CTRL);
> + dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[0].gmp,
> +   HDMI_3D_TX_PHY_GMPCTRL);
> + dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[0],
> +   HDMI_3D_TX_PHY_CURRCTRL);
> +
> + dw_hdmi_phy_i2c_write(hdmi, 0, HDMI_3D_TX_PHY_PLLPHBYCTRL);
> + dw_hdmi_phy_i2c_write(hdmi, 

Re: [PATCH v4 5/9] drm: bridge: dw-hdmi: Fix the PHY power up sequence

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> When powering the PHY up we need to wait for the PLL to lock. This is
> done by polling the TX_PHY_LOCK bit in the HDMI_PHY_STAT0 register
> (interrupt-based wait could be implemented as well but is likely
> overkill). The bit is asserted when the PLL locks, but the current code
> incorrectly waits for the bit to be deasserted. Fix it, and while at it,
> replace the udelay() with a sleep as the code never runs in
> non-sleepable context.
>
> To be consistent with the power down implementation move the poll loop
> to the power off function.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 65 
> +++-
>  1 file changed, 37 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 85348ba6bb1c..0aa3ad404f77 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -949,9 +949,44 @@ static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
>   dw_hdmi_phy_gen2_pddq(hdmi, 1);
>  }
>  
> +static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
> +{
> + const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
> + unsigned int i;
> + u8 val;
> +
> + if (phy->gen == 1) {
> + dw_hdmi_phy_enable_powerdown(hdmi, false);
> +
> + /* Toggle TMDS enable. */
> + dw_hdmi_phy_enable_tmds(hdmi, 0);
> + dw_hdmi_phy_enable_tmds(hdmi, 1);
> + return 0;
> + }
> +
> + dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> + dw_hdmi_phy_gen2_pddq(hdmi, 0);
> +
> + /* Wait for PHY PLL lock */
> + for (i = 0; i < 5; ++i) {
> + val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
> + if (val)
> + break;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + if (!val) {
> + dev_err(hdmi->dev, "PHY PLL failed to lock\n");
> + return -ETIMEDOUT;
> + }
> +
> + dev_dbg(hdmi->dev, "PHY PLL locked %u iterations\n", i);
> + return 0;
> +}
> +
>  static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  {
> - u8 val, msec;
>   const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>   const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>   const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
> @@ -1019,33 +1054,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>   hdmi_phy_i2c_write(hdmi, HDMI_3D_TX_PHY_CKCALCTRL_OVERRIDE,
>  HDMI_3D_TX_PHY_CKCALCTRL);
>  
> - dw_hdmi_phy_enable_powerdown(hdmi, false);
> -
> - /* toggle TMDS enable */
> - dw_hdmi_phy_enable_tmds(hdmi, 0);
> - dw_hdmi_phy_enable_tmds(hdmi, 1);
> -
> - /* gen2 tx power on */
> - dw_hdmi_phy_gen2_txpwron(hdmi, 1);
> - dw_hdmi_phy_gen2_pddq(hdmi, 0);
> -
> - /* Wait for PHY PLL lock */
> - msec = 5;
> - do {
> - val = hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_TX_PHY_LOCK;
> - if (!val)
> - break;
> -
> - if (msec == 0) {
> - dev_err(hdmi->dev, "PHY PLL not locked\n");
> - return -ETIMEDOUT;
> - }
> -
> - udelay(1000);
> - msec--;
> - } while (1);
> -
> - return 0;
> + return dw_hdmi_phy_power_on(hdmi);
>  }
>  
>  static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)

Re: [PATCH v4 6/9] drm: bridge: dw-hdmi: Create PHY operations

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> The HDMI TX controller support different PHYs whose programming
> interface can vary significantly, especially with vendor PHYs that are
> not provided by Synopsys. To support them, create a PHY operation
> structure that can be provided by the platform glue layer. The existing
> PHY handling code (limited to Synopsys PHY support) is refactored into a
> set of default PHY operations that are used automatically when the
> platform glue doesn't provide its own operations.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 91 
> 
>  include/drm/bridge/dw_hdmi.h | 18 +++-
>  2 files changed, 80 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 0aa3ad404f77..cb2703862be2 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -141,8 +141,12 @@ struct dw_hdmi {
>   u8 edid[HDMI_EDID_LEN];
>   bool cable_plugin;
>  
> - const struct dw_hdmi_phy_data *phy;
> - bool phy_enabled;
> + struct {
> + const struct dw_hdmi_phy_ops *ops;
> + const char *name;
> + void *data;
> + bool enabled;
> + } phy;
>  
>   struct drm_display_mode previous_mode;
>  
> @@ -831,6 +835,10 @@ static void hdmi_video_packetize(struct dw_hdmi *hdmi)
> HDMI_VP_CONF);
>  }
>  
> +/* 
> -
> + * Synopsys PHY Handling
> + */
> +
>  static inline void hdmi_phy_test_clear(struct dw_hdmi *hdmi,
>  unsigned char bit)
>  {
> @@ -987,6 +995,7 @@ static int dw_hdmi_phy_power_on(struct dw_hdmi *hdmi)
>  
>  static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  {
> + const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
>   const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>   const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>   const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
> @@ -1019,7 +1028,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>   dw_hdmi_phy_power_off(hdmi);
>  
>   /* Leave low power consumption mode by asserting SVSRET. */
> - if (hdmi->phy->has_svsret)
> + if (phy->has_svsret)
>   dw_hdmi_phy_enable_svsret(hdmi, 1);
>  
>   /* PHY reset. The reset signal is active high on Gen2 PHYs. */
> @@ -1057,7 +1066,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>   return dw_hdmi_phy_power_on(hdmi);
>  }
>  
> -static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
> +static int dw_hdmi_phy_init(struct dw_hdmi *hdmi, void *data,
> + struct drm_display_mode *mode)
>  {
>   int i, ret;
>  
> @@ -1071,10 +1081,31 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
>   return ret;
>   }
>  
> - hdmi->phy_enabled = true;
>   return 0;
>  }
>  
> +static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi, void *data)
> +{
> + dw_hdmi_phy_power_off(hdmi);
> +}
> +
> +static enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
> +   void *data)
> +{
> + return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
> + connector_status_connected : connector_status_disconnected;
> +}
> +
> +static const struct dw_hdmi_phy_ops dw_hdmi_synopsys_phy_ops = {
> + .init = dw_hdmi_phy_init,
> + .disable = dw_hdmi_phy_disable,
> + .read_hpd = dw_hdmi_phy_read_hpd,
> +};
> +
> +/* 
> -
> + * HDMI TX Setup
> + */
> +
>  static void hdmi_tx_hdcp_config(struct dw_hdmi *hdmi)
>  {
>   u8 de;
> @@ -1289,16 +1320,6 @@ static void hdmi_av_composer(struct dw_hdmi *hdmi,
>   hdmi_writeb(hdmi, vsync_len, HDMI_FC_VSYNCINWIDTH);
>  }
>  
> -static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
> -{
> - if (!hdmi->phy_enabled)
> - return;
> -
> - dw_hdmi_phy_power_off(hdmi);
> -
> - hdmi->phy_enabled = false;
> -}
> -
>  /* HDMI Initialization Step B.4 */
>  static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
>  {
> @@ -1431,9 +1452,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct 
> drm_display_mode *mode)
>  

Re: [PATCH v4 4/9] drm: bridge: dw-hdmi: Fix the PHY power down sequence

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> The PHY requires us to wait for the PHY to switch to low power mode
> after deasserting TXPWRON and before asserting PDDQ in the power down
> sequence, otherwise power down will fail.
>
> The PHY power down can be monitored though the TX_READY bit, available
> through I2C in the PHY registers, or the TX_PHY_LOCK bit, available
> through the HDMI TX registers. As the two are equivalent, let's pick the
> easier solution of polling the TX_PHY_LOCK bit.
>
> The power down code is currently duplicated in multiple places. To avoid
> spreading multiple calls to a TX_PHY_LOCK poll function, we have to
> refactor the power down code and group it all in a single function.
>
> Tests showed that one poll iteration was enough for TX_PHY_LOCK to
> become low, without requiring any additional delay. Retrying the read
> five times with a 1ms to 2ms delay between each attempt should thus be
> more than enough.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 52 
> +---
>  1 file changed, 43 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index d863b3393aee..85348ba6bb1c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -116,6 +116,7 @@ struct dw_hdmi_i2c {
>  struct dw_hdmi_phy_data {
>   enum dw_hdmi_phy_type type;
>   const char *name;
> + unsigned int gen;
>   bool has_svsret;
>  };
>  
> @@ -914,6 +915,40 @@ static void dw_hdmi_phy_sel_interface_control(struct 
> dw_hdmi *hdmi, u8 enable)
>HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
>  
> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
> +{
> + const struct dw_hdmi_phy_data *phy = hdmi->phy.data;
> + unsigned int i;
> + u16 val;
> +
> + if (phy->gen == 1) {
> + dw_hdmi_phy_enable_tmds(hdmi, 0);
> + dw_hdmi_phy_enable_powerdown(hdmi, true);
> + return;
> + }
> +
> + dw_hdmi_phy_gen2_txpwron(hdmi, 0);
> +
> + /*
> +  * Wait for TX_PHY_LOCK to be deasserted to indicate that the PHY went
> +  * to low power mode.
> +  */
> + for (i = 0; i < 5; ++i) {
> + val = hdmi_readb(hdmi, HDMI_PHY_STAT0);
> + if (!(val & HDMI_PHY_TX_PHY_LOCK))
> + break;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + if (val & HDMI_PHY_TX_PHY_LOCK)
> + dev_warn(hdmi->dev, "PHY failed to power down\n");
> + else
> + dev_dbg(hdmi->dev, "PHY powered down in %u iterations\n", i);
> +
> + dw_hdmi_phy_gen2_pddq(hdmi, 1);
> +}
> +
>  static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  {
>   u8 val, msec;
> @@ -946,11 +981,7 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>   return -EINVAL;
>   }
>  
> - /* gen2 tx power off */
> - dw_hdmi_phy_gen2_txpwron(hdmi, 0);
> -
> - /* gen2 pddq */
> - dw_hdmi_phy_gen2_pddq(hdmi, 1);
> + dw_hdmi_phy_power_off(hdmi);
>  
>   /* Leave low power consumption mode by asserting SVSRET. */
>   if (hdmi->phy->has_svsret)
> @@ -1025,8 +1056,6 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
>   for (i = 0; i < 2; i++) {
>   dw_hdmi_phy_sel_data_en_pol(hdmi, 1);
>   dw_hdmi_phy_sel_interface_control(hdmi, 0);
> - dw_hdmi_phy_enable_tmds(hdmi, 0);
> - dw_hdmi_phy_enable_powerdown(hdmi, true);
>  
>   ret = hdmi_phy_configure(hdmi);
>   if (ret)
> @@ -1256,8 +1285,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
>   if (!hdmi->phy_enabled)
>   return;
>  
> - dw_hdmi_phy_enable_tmds(hdmi, 0);
> - dw_hdmi_phy_enable_powerdown(hdmi, true);
> + dw_hdmi_phy_power_off(hdmi);
>  
>   hdmi->phy_enabled = false;
>  }
> @@ -1827,23 +1855,29 @@ static const struct dw_hdmi_phy_data dw_hdmi_phys[] = 
> {
>   {
>   .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
>   .name = "DWC HDMI TX PHY",
> + .gen = 1,
>   }, {
>   .type = DW_HDMI_PHY_DWC_MHL_PHY_HEAC,
>   .name = "DWC MHL PHY + HEAC PHY",
> + .gen = 2,
>   .has_svsret = true,
>   }, {
>   .type = DW_HDMI_PHY_DWC_MHL_PHY,
>

Re: [PATCH v4 1/9] drm: bridge: dw-hdmi: Remove unused functions

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> Most of the hdmi_phy_test_*() functions are unused. Remove them.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 26 --
>  1 file changed, 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 9a9ec27d9e28..ce7496399ccf 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -837,32 +837,6 @@ static inline void hdmi_phy_test_clear(struct dw_hdmi 
> *hdmi,
> HDMI_PHY_TST0_TSTCLR_MASK, HDMI_PHY_TST0);
>  }
>  
> -static inline void hdmi_phy_test_enable(struct dw_hdmi *hdmi,
> - unsigned char bit)
> -{
> - hdmi_modb(hdmi, bit << HDMI_PHY_TST0_TSTEN_OFFSET,
> -   HDMI_PHY_TST0_TSTEN_MASK, HDMI_PHY_TST0);
> -}
> -
> -static inline void hdmi_phy_test_clock(struct dw_hdmi *hdmi,
> -unsigned char bit)
> -{
> - hdmi_modb(hdmi, bit << HDMI_PHY_TST0_TSTCLK_OFFSET,
> -   HDMI_PHY_TST0_TSTCLK_MASK, HDMI_PHY_TST0);
> -}
> -
> -static inline void hdmi_phy_test_din(struct dw_hdmi *hdmi,
> -  unsigned char bit)
> -{
> - hdmi_writeb(hdmi, bit, HDMI_PHY_TST1);
> -}
> -
> -static inline void hdmi_phy_test_dout(struct dw_hdmi *hdmi,
> -   unsigned char bit)
> -{
> - hdmi_writeb(hdmi, bit, HDMI_PHY_TST2);
> -}
> -
>  static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, int msec)
>  {
>   u32 val;

Re: [PATCH v4 2/9] drm: bridge: dw-hdmi: Move CSC configuration out of PHY code

[Jose Abreu]

Hi Laurent,


On 01-03-2017 22:39, Laurent Pinchart wrote:
> The color space converter isn't part of the PHY, move its configuration
> out of PHY code.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 25 ++---
>  1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index ce7496399ccf..906583beb08b 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -914,7 +914,7 @@ static void dw_hdmi_phy_sel_interface_control(struct 
> dw_hdmi *hdmi, u8 enable)
>HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
>  
> -static int hdmi_phy_configure(struct dw_hdmi *hdmi, int cscon)
> +static int hdmi_phy_configure(struct dw_hdmi *hdmi)
>  {
>   u8 val, msec;
>   const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
> @@ -946,14 +946,6 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, int 
> cscon)
>   return -EINVAL;
>   }
>  
> - /* Enable csc path */
> - if (cscon)
> - val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH;
> - else
> - val = HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS;
> -
> - hdmi_writeb(hdmi, val, HDMI_MC_FLOWCTRL);
> -
>   /* gen2 tx power off */
>   dw_hdmi_phy_gen2_txpwron(hdmi, 0);
>  
> @@ -1028,10 +1020,6 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, 
> int cscon)
>  static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
>  {
>   int i, ret;
> - bool cscon;
> -
> - /*check csc whether needed activated in HDMI mode */
> - cscon = hdmi->sink_is_hdmi && is_color_space_conversion(hdmi);
>  
>   /* HDMI Phy spec says to do the phy initialization sequence twice */
>   for (i = 0; i < 2; i++) {
> @@ -1040,8 +1028,7 @@ static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
>   dw_hdmi_phy_enable_tmds(hdmi, 0);
>   dw_hdmi_phy_enable_powerdown(hdmi, true);
>  
> - /* Enable CSC */
> - ret = hdmi_phy_configure(hdmi, cscon);
> + ret = hdmi_phy_configure(hdmi);
>   if (ret)
>   return ret;
>   }
> @@ -1303,6 +1290,14 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi 
> *hdmi)
>   clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
>   hdmi_writeb(hdmi, clkdis, HDMI_MC_CLKDIS);
>   }
> +
> + /* Enable color space conversion if needed (for HDMI sinks only). */
> + if (hdmi->sink_is_hdmi && is_color_space_conversion(hdmi))
> + hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
> + HDMI_MC_FLOWCTRL);
> + else
> + hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_BYPASS,
> + HDMI_MC_FLOWCTRL);
>  }
>  
>  static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)

Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling

[Jose Abreu]

Hi Laurent,


Sorry for the delayed answer but I am quite busy at the moment.


On 06-01-2017 01:48, Laurent Pinchart wrote:

[snip]

 The TX_READY signal is documented in the i.MX6 datasheet as being a PHY
 output signal, but there seems to be no HDMI TX register from which its
 state can be read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register
 through I2C ? How long is the PHY expected to take to set TX_READY to 0
 ?
>>> TX_READY can be read from register 0x1A of phy, BIT(2) (through
>>> I2C).
>> That's what I thought, I'll poll that then. Do you have any idea how long
>> it's supposed to take, to set an appropriate timeout ?

For 3d tx phy and for 25 MHz input reference clock the power-up
time is ~1ms, there is no data in the docs to power-down time but
it should be similar. Reference clock value is board dependent
and the minimum value for HDMI shall be 13.5MHz.

> On i.MX6 (3D TX PHY) register 0x1a reads as 0x0007 before powering down the 
> PHY (by deasserting TXPWRON) and as 0x immediately after. On RK3288 (MHL 
> PHY) the register reads as 0x0207 and as 0x immediately after deasserting 
> TXPWRON. It seems that one I2C read is a sufficient delay for TX_READY to go 
> low.
>>> Not sure if same offset for all phys though.
>> Most probably not, it would be too easy :-) I'll investigate (which will
>> likely include lots of guesswork). If you can find any information about
>> that (and especially about the MHL and HDMI 2.0 PHYs) that would be very
>> appreciated, as I don't have access to any documentation that mentions a
>> TX_READY bit for those.
> I haven't tested the HDMI 2.0 PHY yet, but I'd be surprised if the TX_READY 
> bit was in the same register at the same position.
>

The info I got the register offset is from an HDMI 2.0 phy :)
Notice that there are a lot of phy versions and some of them
(used in dw-hdmi) maybe customized, I don't think I have access
to that custom phys documentation. Please test the HDMI 2.0 phy
and check if the register is the same, it should be. In the
meantime it would really be helpful if some of the developers
that used dw-hdmi supplied this info about the registers as they
should know exactly what phy was used.

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling

[Jose Abreu]

Hi Laurent,


On 05-01-2017 12:29, Laurent Pinchart wrote:
> Hi Jose,
>
> On Tuesday 20 Dec 2016 12:17:23 Jose Abreu wrote:
>> On 20-12-2016 11:45, Russell King - ARM Linux wrote:
>>> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote:
>>>> Instead of spreading version-dependent PHY power handling code around,
>>>> group it in two functions to power the PHY on and off and use them
>>>> through the driver.
>>>>
>>>> Powering off the PHY at the beginning of the setup phase is currently
>>>> split in two locations for first and second generation PHYs, group all
>>>> the operations in the dw_hdmi_phy_init() function.
>>> This changes the behaviour of the driver.
>>>
>>>> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
>>>> +{
>>>> +  if (hdmi->phy->gen == 1) {
>>>> +  dw_hdmi_phy_enable_tmds(hdmi, 0);
>>>> +  dw_hdmi_phy_enable_powerdown(hdmi, true);
>>>> +  } else {
>>>> +  dw_hdmi_phy_gen2_txpwron(hdmi, 0);
>>>> +  dw_hdmi_phy_gen2_pddq(hdmi, 1);
>>>> +  }
>>>> +}
>>>> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi
>>>> *hdmi)
>>>>
>>>>if (!hdmi->phy_enabled)
>>>>
>>>>return;
>>>>
>>>> -  dw_hdmi_phy_enable_tmds(hdmi, 0);
>>>> -  dw_hdmi_phy_enable_powerdown(hdmi, true);
>>>> -
>>>> +  dw_hdmi_phy_power_off(hdmi);
>>> This makes dw_hdmi_phy_disable() power down a gen2 phy.
>>>
>>> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a
>>> gen2 phy.  I've been carrying this change for a while, which I've had
>>> to revert (and finally expunge), as it causes problems on iMX6:
>>>
>>> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi
>>> *hdmi)> 
>>> if (!hdmi->phy_enabled)
>>> 
>>> return;
>>>
>>> +   /* Actually set the phy into low power mode */
>>> +   dw_hdmi_phy_gen2_txpwron(hdmi, 0);
>>> +
>>> +   /* FIXME: We should wait for TX_READY to be deasserted */
>>> +
>>> +   dw_hdmi_phy_gen2_pddq(hdmi, 1);
>>> +
>>> +   /* This appears to have no effect on iMX6 */
>>>
>>> dw_hdmi_phy_enable_tmds(hdmi, 0);
>>> dw_hdmi_phy_enable_powerdown(hdmi, true);
>>>
>>> So, I think your change here will cause problems for iMX6.
>>>
>>> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD
>>> bouncing when the PHY is powered down.  I can't promise when I'll be
>>> able to check for that again.
>> Indeed TX_READY must be low before asserting pddq.
> The TX_READY signal is documented in the i.MX6 datasheet as being a PHY 
> output 
> signal, but there seems to be no HDMI TX register from which its state can be 
> read. Do I need to poll the HDMI_PHY_PTRPT_ENBL register through I2C ? How 
> long is the PHY expected to take to set TX_READY to 0 ?

TX_READY can be read from register 0x1A of phy, BIT(2) (through
I2C). Not sure if same offset for all phys though.

Best regards,
Jose Miguel Abreu

>
>> HPD and RXSENSE should work in power down mode by enabling enhpdrxsense
>> bit in phy_conf0 BUT to enter power down you must disable TX_PWRON,
>> enhpdrxsense and enable PDDQ and PHY_RESET. Only after doing this (and
>> consequently entering power down mode) you can enable again enhpdrxsense.
> Thanks, I'll give it a try.
>

Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime

[Jose Abreu]

Hi Laurent,


On 05-01-2017 11:44, Laurent Pinchart wrote:

[snip]

>>> I've tried moving SVSRET right before the reset and everything still seems
>>> to work fine, so I'll submit a patch.
>>>
>>> Is the rest of the sequence correct ? When should SVSRET be deasserted
>>> (the driver currently keeps it asserted at all times) ?
>> It should not be deasserted.
> At all ? Even when powering the PHY down ?

The source code I have here does not deassert the signal, even
when powering down BUT I am checking the docs and this signal can
optionally go low after the phy goes to power down mode (i.e.
after all power down sequence is correctly done, this includes
waiting for TX_READY to go low as Russell King previously said),
though its not mandatory. I think I miss explained how this mode
works: When deasserted the phy goes to low power mode, thats why
you need to assert it in power up. We can try to add this in
power down sequence but its very important to check the status of
TX_READY before changing SVSRET.

Best regards,
Jose Miguel Abreu

[snip]

Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime

[Jose Abreu]

Hi Laurent,


On 05-01-2017 00:15, Laurent Pinchart wrote:
> Hi Jose,
>
> On Tuesday 20 Dec 2016 15:01:52 Jose Abreu wrote:
>> On 20-12-2016 13:11, Laurent Pinchart wrote:
>>> On Tuesday 20 Dec 2016 11:39:21 Jose Abreu wrote:
>>>> On 20-12-2016 01:33, Laurent Pinchart wrote:
>>>>> Detect the PHY type and use it to handle the PHY type-specific SVSRET
>>>>> signal.
>>>>>
>>>>> Signed-off-by: Laurent Pinchart
>>>>> <laurent.pinchart+rene...@ideasonboard.com>
>>>>> ---
>>>>>
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 68 +++--
>>>>>  include/drm/bridge/dw_hdmi.h | 10 ++
>>>>>  2 files changed, 75 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>>>>> b/drivers/gpu/drm/bridge/dw-hdmi.c index f4faa14213e5..ef4f2f96ed2c
>>>>> 100644
>>>>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>>>>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>> [snip]
>>
>>> I don't have access to the documentation so I can't comment on that :-)
>>> What does the SVSRET signal control (and what does the name stand for) ?
>> SVSRET stands for SVSRET :) (no idea what it means) Its a low
>> power mode of consumption.
>>
>>> By de-asserting PHY reset, do you mean
>>>
>>> /* PHY reset */
>>> hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_DEASSERT, HDMI_MC_PHYRSTZ);
>>> hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_ASSERT, HDMI_MC_PHYRSTZ);
>>>
>>> ? HDMI_MC_PHYRSTZ_DEASSERT is defined as 0x01 and HDMI_MC_PHYRSTZ_ASSERT
>>> as 0x00, which I believe leads to correct operation on Gen2 PHYs, but is
>>> incorrect on Gen1 PHYs that have an active low reset signal. Could you
>>> confirm that ? The DEASSERT and ASSERT macros should be renamed as
>>> they're obviously incorrect.
>> Correct. Older phys require PHYRSTZ to be deasserted (i.e. low)
>> for a PHY-dependent time. Newer phys require PHYRSTZ to be
>> asserted (i.e. high) for, again, a PHY-dependent time.
> Thanks for the confirmation, I'll rename the macros.
>
>> This is the kind of things that made me suggest you to extract
>> all the phy configuration from dw-hdmi. I think that having a
>> bunch of if's because of all the phy's that we need to support
>> does not make much sense. The downside is, of course, having code
>> duplicated.
> I agree with you in principle, but I'd rather do that when I'll have devices 
> to test the code on. The three devices (soon to be) supported in mainline by 
> the dw-hdmi drivers, i.MX6, RK3288 and R-Car Gen3, all use Gen2 PHYs, so I 
> can't test the Gen1 code paths meaningfully at the moment.
>
>>> I can move the SVSRET assertion before PHY reset and test it on RK3288 and
>>> R-Car H3.
>> Probably wont make much difference unless you have a way to
>> measure how much power the phy is consuming. But I think it is
>> the right thing to do according to docs.
> The init sequence is currently
>
> - Power the PHY off (TXPWRON = 0, PDDQ = 1)
> - Reset the PHY (through HDMI_MC_PHYRSTZ and HDMI_MC_HEACPHY_RST)
> - Configure the PHY through I2C
> - Power the PHY on (TXPWRON = 1, PDDQ = 0)
> - Set SVSRET
> - Wait for PHY PLL lock

What I have here for the most recent phy we tested is this:
- Board specific configuration (should not be needed by you)
- Set MC_PHY_RST high
- Set TXPWRON = 0
- Set PDDQ = 1
- Set SVSRET = 0
- Board specific impendance calibration reset (should not be
needed by you)
- Set SVSRET = 1
- Set MC_PHY_RST low
- Configure phy through I2C
- Set PDDQ = 0
- Set TXPWRON = 1,
- Wait for phy pll lock

>
> I've tried moving SVSRET right before the reset and everything still seems to 
> work fine, so I'll submit a patch.
>
> Is the rest of the sequence correct ? When should SVSRET be deasserted (the 
> driver currently keeps it asserted at all times) ?

It should not be deasserted.

>
> Speaking of reset, I believe support for HEAC PHYs is broken, given that the 
> driver asserts the reset signal (through the HDMI_MC_HEACPHY_RST register) 
> but 
> never deasserts it.

Hmm, probably, but not sure. I never tested this in older phys.

>
>> [snip]
>>
>>> The SoC datasheet mentions the SVSRET bit in the HDMI TX registers but
>>> doesn't document it any further. If I don't set SVSRET the HDMI output
>>> stays dead, so I assume I need to set it :-)
>> Hmm, ok. I still haven't figured out which phy you are using so
>> can't comment much further.
> I'll then leave has_svsret set to true for DW_HDMI_PHY_DWC_HDMI20_TX_PHY as 
> tests show it's needed.
>

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime

[Jose Abreu]

Hi Laurent,


On 20-12-2016 13:11, Laurent Pinchart wrote:
> Hi Jose,
>
> On Tuesday 20 Dec 2016 11:39:21 Jose Abreu wrote:
>> On 20-12-2016 01:33, Laurent Pinchart wrote:
>>> Detect the PHY type and use it to handle the PHY type-specific SVSRET
>>> signal.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> <laurent.pinchart+rene...@ideasonboard.com>
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 68 +--
>>>  include/drm/bridge/dw_hdmi.h | 10 ++
>>>  2 files changed, 75 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/dw-hdmi.c index f4faa14213e5..ef4f2f96ed2c
>>> 100644
>>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c

[snip]

> I don't have access to the documentation so I can't comment on that :-) What  
> does the SVSRET signal control (and what does the name stand for) ?

SVSRET stands for SVSRET :) (no idea what it means) Its a low
power mode of consumption.

>
> By de-asserting PHY reset, do you mean
>
> /* PHY reset */
> hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_DEASSERT, HDMI_MC_PHYRSTZ);
> hdmi_writeb(hdmi, HDMI_MC_PHYRSTZ_ASSERT, HDMI_MC_PHYRSTZ);
>
> ? HDMI_MC_PHYRSTZ_DEASSERT is defined as 0x01 and HDMI_MC_PHYRSTZ_ASSERT as 
> 0x00, which I believe leads to correct operation on Gen2 PHYs, but is 
> incorrect on Gen1 PHYs that have an active low reset signal. Could you 
> confirm 
> that ? The DEASSERT and ASSERT macros should be renamed as they're obviously 
> incorrect.

Correct. Older phys require PHYRSTZ to be deasserted (i.e. low)
for a PHY-dependent time. Newer phys require PHYRSTZ to be
asserted (i.e. high) for, again, a PHY-dependent time.

This is the kind of things that made me suggest you to extract
all the phy configuration from dw-hdmi. I think that having a
bunch of if's because of all the phy's that we need to support
does not make much sense. The downside is, of course, having code
duplicated.

>
> I can move the SVSRET assertion before PHY reset and test it on RK3288 and R-
> Car H3.

Probably wont make much difference unless you have a way to
measure how much power the phy is consuming. But I think it is
the right thing to do according to docs.

[snip]

> The SoC datasheet mentions the SVSRET bit in the HDMI TX registers but 
> doesn't 
> document it any further. If I don't set SVSRET the HDMI output stays dead, so 
> I assume I need to set it :-)
>

Hmm, ok. I still haven't figured out which phy you are using so
can't comment much further.

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 17/29] drm: bridge: dw-hdmi: Refactor PHY power handling

[Jose Abreu]

Hi Russell,


On 20-12-2016 11:45, Russell King - ARM Linux wrote:
> On Tue, Dec 20, 2016 at 03:33:48AM +0200, Laurent Pinchart wrote:
>> Instead of spreading version-dependent PHY power handling code around,
>> group it in two functions to power the PHY on and off and use them
>> through the driver.
>>
>> Powering off the PHY at the beginning of the setup phase is currently
>> split in two locations for first and second generation PHYs, group all
>> the operations in the dw_hdmi_phy_init() function.
> This changes the behaviour of the driver.
>
>> +static void dw_hdmi_phy_power_off(struct dw_hdmi *hdmi)
>> +{
>> +if (hdmi->phy->gen == 1) {
>> +dw_hdmi_phy_enable_tmds(hdmi, 0);
>> +dw_hdmi_phy_enable_powerdown(hdmi, true);
>> +} else {
>> +dw_hdmi_phy_gen2_txpwron(hdmi, 0);
>> +dw_hdmi_phy_gen2_pddq(hdmi, 1);
>> +}
>> +}
>> @@ -1290,9 +1302,7 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
>>  if (!hdmi->phy_enabled)
>>  return;
>>  
>> -dw_hdmi_phy_enable_tmds(hdmi, 0);
>> -dw_hdmi_phy_enable_powerdown(hdmi, true);
>> -
>> +dw_hdmi_phy_power_off(hdmi);
> This makes dw_hdmi_phy_disable() power down a gen2 phy.
>
> The iMX6 has a DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY phy, which you list as a
> gen2 phy.  I've been carrying this change for a while, which I've had
> to revert (and finally expunge), as it causes problems on iMX6:
>
> @@ -1112,6 +1112,14 @@ static void dw_hdmi_phy_disable(struct dw_hdmi *hdmi)
> if (!hdmi->phy_enabled)
> return;
>
> +   /* Actually set the phy into low power mode */
> +   dw_hdmi_phy_gen2_txpwron(hdmi, 0);
> +
> +   /* FIXME: We should wait for TX_READY to be deasserted */
> +
> +   dw_hdmi_phy_gen2_pddq(hdmi, 1);
> +
> +   /* This appears to have no effect on iMX6 */
> dw_hdmi_phy_enable_tmds(hdmi, 0);
> dw_hdmi_phy_enable_powerdown(hdmi, true);
>
> So, I think your change here will cause problems for iMX6.
>
> From what I remember, it seems that iMX6 has issues with RXSENSE/HPD
> bouncing when the PHY is powered down.  I can't promise when I'll be
> able to check for that again.
>

Indeed TX_READY must be low before asserting pddq. HPD and
RXSENSE should work in power down mode by enabling enhpdrxsense
bit in phy_conf0 BUT to enter power down you must disable
TX_PWRON, enhpdrxsense and enable PDDQ and PHY _RESET. Only after
doing this (and consequently entering power down mode) you can
enable again enhpdrxsense.

Best regards,
Jose Miguel Abreu

Re: [PATCH v2 16/29] drm: bridge: dw-hdmi: Detect PHY type at runtime

[Jose Abreu]

Hi Laurent,


On 20-12-2016 01:33, Laurent Pinchart wrote:
> Detect the PHY type and use it to handle the PHY type-specific SVSRET
> signal.
>
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 68 
> ++--
>  include/drm/bridge/dw_hdmi.h | 10 ++
>  2 files changed, 75 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index f4faa14213e5..ef4f2f96ed2c 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -113,6 +113,12 @@ struct dw_hdmi_i2c {
>   boolis_regaddr;
>  };
>  
> +struct dw_hdmi_phy_data {
> + enum dw_hdmi_phy_type type;
> + const char *name;
> + bool has_svsret;
> +};
> +
>  struct dw_hdmi {
>   struct drm_connector connector;
>   struct drm_bridge bridge;
> @@ -134,7 +140,9 @@ struct dw_hdmi {
>   u8 edid[HDMI_EDID_LEN];
>   bool cable_plugin;
>  
> + const struct dw_hdmi_phy_data *phy;
>   bool phy_enabled;
> +
>   struct drm_display_mode previous_mode;
>  
>   struct i2c_adapter *ddc;
> @@ -1015,7 +1023,8 @@ static int hdmi_phy_configure(struct dw_hdmi *hdmi, int 
> cscon)
>   dw_hdmi_phy_gen2_txpwron(hdmi, 1);
>   dw_hdmi_phy_gen2_pddq(hdmi, 0);
>  
> - if (hdmi->dev_type == RK3288_HDMI)
> + /* The DWC MHL and HDMI 2.0 PHYs need the SVSRET signal to be set. */
> + if (hdmi->phy->has_svsret)
>   dw_hdmi_phy_enable_svsret(hdmi, 1);

I didn't review the original code but according to the docs the
svsret signal should be set before de-asserting phy reset.

>  
>   /*Wait for PHY PLL lock */
> @@ -1840,6 +1849,54 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
>   return IRQ_HANDLED;
>  }
>  
> +static const struct dw_hdmi_phy_data dw_hdmi_phys[] = {
> + {
> + .type = DW_HDMI_PHY_DWC_HDMI_TX_PHY,
> + .name = "DWC HDMI TX PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_MHL_PHY_HEAC,
> + .name = "DWC MHL PHY + HEAC PHY",
> + .has_svsret = true,
> + }, {
> + .type = DW_HDMI_PHY_DWC_MHL_PHY,
> + .name = "DWC MHL PHY",
> + .has_svsret = true,
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC,
> + .name = "DWC HDMI 3D TX PHY + HEAC PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY,
> + .name = "DWC HDMI 3D TX PHY",
> + }, {
> + .type = DW_HDMI_PHY_DWC_HDMI20_TX_PHY,
> + .name = "DWC HDMI 2.0 TX PHY",
> + .has_svsret = true,

Hmm, what I have here is that only MHL phys have svsret. Is this
case for your phy?

Best regards,
Jose Miguel Abreu
> + }
> +};
> +
> +static int dw_hdmi_detect_phy(struct dw_hdmi *hdmi)
> +{
> + unsigned int i;
> + u8 phy_type;
> +
> + phy_type = hdmi_readb(hdmi, HDMI_CONFIG2_ID);
> +
> + for (i = 0; i < ARRAY_SIZE(dw_hdmi_phys); ++i) {
> + if (dw_hdmi_phys[i].type == phy_type) {
> + hdmi->phy = _hdmi_phys[i];
> + return 0;
> + }
> + }
> +
> + if (phy_type == DW_HDMI_PHY_VENDOR_PHY)
> + dev_err(hdmi->dev, "Unsupported vendor HDMI PHY\n");
> + else
> + dev_err(hdmi->dev, "Unsupported HDMI PHY type (%02x)\n",
> + phy_type);
> +
> + return -ENODEV;
> +}
> +
>  static struct dw_hdmi *
>  __dw_hdmi_probe(struct platform_device *pdev,
>   const struct dw_hdmi_plat_data *plat_data)
> @@ -1950,9 +2007,14 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   goto err_iahb;
>   }
>  
> - dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP\n",
> + ret = dw_hdmi_detect_phy(hdmi);
> + if (ret < 0)
> + goto err_iahb;
> +
> + dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP (%s)\n",
>hdmi->version >> 12, hdmi->version & 0xfff,
> -  prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without");
> +  prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without",
> +  hdmi->phy->name);
>  
>   initialize_hdmi_ih_mutes(hdmi);
>  
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 3bb22a849830..b080a171a23f 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -27,6 +27,16 @@ enum dw_hdmi_devtype {
>   RK3288_HDMI,
>  };
>  
> +enum dw_hdmi_phy_type {
> + DW_HDMI_PHY_DWC_HDMI_TX_PHY = 0x00,
> + DW_HDMI_PHY_DWC_MHL_PHY_HEAC = 0xb2,
> + DW_HDMI_PHY_DWC_MHL_PHY = 0xc2,
> + DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY_HEAC = 0xe2,
> + DW_HDMI_PHY_DWC_HDMI_3D_TX_PHY = 0xf2,
> + DW_HDMI_PHY_DWC_HDMI20_TX_PHY = 0xf3,
> + DW_HDMI_PHY_VENDOR_PHY = 0xfe,
> +};
> +
>  struct dw_hdmi_mpll_config {
>   unsigned long mpixelclock;

Re: [PATCH v2 15/29] drm: bridge: dw-hdmi: Handle overflow workaround based on device version

[Jose Abreu]

Hi Laurent,


On 20-12-2016 01:33, Laurent Pinchart wrote:
> Use the device version queried at runtime instead of the device type
> provided through platform data to handle the overflow workaround. This
> will make support of other SoCs integrating the same HDMI TX controller
> version easier.
>
> Among the supported platforms only i.MX6DL and i.MX6Q have been
> identified as needing the workaround. Disabling it on Rockchip RK3288
> (which integrates a v2.00a controller) didn't produce any error or
> artifact.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 46 
> 
>  1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 730a7558d4d4..f4faa14213e5 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -117,8 +117,10 @@ struct dw_hdmi {
>   struct drm_connector connector;
>   struct drm_bridge bridge;
>  
> - struct platform_device *audio;
>   enum dw_hdmi_devtype dev_type;
> + unsigned int version;
> +
> + struct platform_device *audio;
>   struct device *dev;
>   struct clk *isfr_clk;
>   struct clk *iahb_clk;
> @@ -1323,19 +1325,38 @@ static void hdmi_enable_audio_clk(struct dw_hdmi 
> *hdmi)
>  /* Workaround to clear the overflow condition */
>  static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
>  {
> - int count;
> + unsigned int count;
> + unsigned int i;
>   u8 val;
>  
> - /* TMDS software reset */
> - hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> + /*
> +  * Under some circumstances the Frame Composer arithmetic unit can miss
> +  * an FC register write due to being busy processing the previous one.
> +  * The issue can be worked around by issuing a TMDS software reset and
> +  * then write one of the FC registers several times.
> +  *
> +  * The number of iterations matters and depends on the HDMI TX revision
> +  * (and possibly on the platform). So far only i.MX6Q (v1.30a) and
> +  * i.MX6DL (v1.31a) have been identified as needing the workaround, with
> +  * 4 and 1 iterations respectively.
> +  */
>  
> - val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> - if (hdmi->dev_type == IMX6DL_HDMI) {
> - hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
> + switch (hdmi->version) {
> + case 0x130a:
> + count = 4;
> + break;
> + case 0x131a:
> + count = 1;
> + break;
> + default:
>   return;
>   }
>  
> - for (count = 0; count < 4; count++)
> + /* TMDS software reset */
> + hdmi_writeb(hdmi, (u8)~HDMI_MC_SWRSTZ_TMDSSWRST_REQ, HDMI_MC_SWRSTZ);
> +
> + val = hdmi_readb(hdmi, HDMI_FC_INVIDCONF);
> + for (i = 0; i < count; i++)
>   hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
>  }
>  
> @@ -1832,7 +1853,6 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   int irq;
>   int ret;
>   u32 val = 1;
> - u16 version;
>   u8 prod_id0;
>   u8 prod_id1;
>   u8 config0;
> @@ -1917,21 +1937,21 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   }
>  
>   /* Product and revision IDs */
> - version = (hdmi_readb(hdmi, HDMI_DESIGN_ID) << 8)
> - | (hdmi_readb(hdmi, HDMI_REVISION_ID) << 0);
> + hdmi->version = (hdmi_readb(hdmi, HDMI_DESIGN_ID) << 8)
> +   | (hdmi_readb(hdmi, HDMI_REVISION_ID) << 0);
>   prod_id0 = hdmi_readb(hdmi, HDMI_PRODUCT_ID0);
>   prod_id1 = hdmi_readb(hdmi, HDMI_PRODUCT_ID1);
>  
>   if (prod_id0 != HDMI_PRODUCT_ID0_HDMI_TX ||
>   (prod_id1 & ~HDMI_PRODUCT_ID1_HDCP) != HDMI_PRODUCT_ID1_HDMI_TX) {
>   dev_err(dev, "Unsupported HDMI controller (%04x:%02x:%02x)\n",
> - version, prod_id0, prod_id1);
> + hdmi->version, prod_id0, prod_id1);
>   ret = -ENODEV;
>   goto err_iahb;
>   }
>  
>   dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP\n",
> -  version >> 12, version & 0xfff,
> +  hdmi->version >> 12, hdmi->version & 0xfff,
>prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without");
>  
>   initialize_hdmi_ih_mutes(hdmi);

Re: [PATCH v2 14/29] drm: bridge: dw-hdmi: Detect AHB audio DMA using correct register

[Jose Abreu]

Hi Laurent,


On 20-12-2016 01:33, Laurent Pinchart wrote:
> Bit 0 in CONFIG1_ID tells whether the IP core uses an AHB slave
> interface for control. The correct way to identify AHB audio DMA support
> is through bit 1 in CONFIG3_ID.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 6 +++---
>  drivers/gpu/drm/bridge/dw-hdmi.h | 4 
>  2 files changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 1809247745b8..730a7558d4d4 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1836,7 +1836,7 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   u8 prod_id0;
>   u8 prod_id1;
>   u8 config0;
> - u8 config1;
> + u8 config3;
>  
>   hdmi = devm_kzalloc(dev, sizeof(*hdmi), GFP_KERNEL);
>   if (!hdmi)
> @@ -1988,9 +1988,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   pdevinfo.id = PLATFORM_DEVID_AUTO;
>  
>   config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID);
> - config1 = hdmi_readb(hdmi, HDMI_CONFIG1_ID);
> + config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
>  
> - if (config1 & HDMI_CONFIG1_AHB) {
> + if (config3 & HDMI_CONFIG3_AHBAUDDMA) {
>   struct dw_hdmi_audio_data audio;
>  
>   audio.phys = iores->start;
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h 
> b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 91d7fabbd6e5..a4fd64a203c9 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -559,6 +559,10 @@ enum {
>  /* CONFIG1_ID field values */
>   HDMI_CONFIG1_AHB = 0x01,
>  
> +/* CONFIG3_ID field values */
> + HDMI_CONFIG3_AHBAUDDMA = 0x02,
> + HDMI_CONFIG3_GPAUD = 0x01,
> +
>  /* IH_FC_INT2 field values */
>   HDMI_IH_FC_INT2_OVERFLOW_MASK = 0x03,
>   HDMI_IH_FC_INT2_LOW_PRIORITY_OVERFLOW = 0x02,

Re: [PATCH v2 13/29] drm: bridge: dw-hdmi: Reject invalid product IDs

[Jose Abreu]

Hi Laurent,


On 20-12-2016 01:33, Laurent Pinchart wrote:
> The DWC HDMI TX can be recognized by the two product identification
> registers. If the registers don't read as expect the IP will be very
> different than what the driver has been designed for, or will be
> misconfigured in a way that makes it non-operational (invalid memory
> address, incorrect clocks, ...). We should reject this situation with an
> error.
>
> While this isn't critical for proper operation with supported IPs at the
> moment, the driver will soon gain automatic device-specific handling
> based on runtime device identification. This change makes it easier to
> implement that without having to default to a random guess in case the
> device can't be identified.
>
> While at it print a readable version number in the device identification
> message instead of raw register values.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart+rene...@ideasonboard.com>

Reviewed-by: Jose Abreu <joab...@synopsys.com>

> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 25 +++--
>  drivers/gpu/drm/bridge/dw-hdmi.h |  8 
>  2 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 06c252f560ad..1809247745b8 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -1832,6 +1832,9 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   int irq;
>   int ret;
>   u32 val = 1;
> + u16 version;
> + u8 prod_id0;
> + u8 prod_id1;
>   u8 config0;
>   u8 config1;
>  
> @@ -1914,12 +1917,22 @@ __dw_hdmi_probe(struct platform_device *pdev,
>   }
>  
>   /* Product and revision IDs */
> - dev_info(dev,
> -  "Detected HDMI controller 0x%x:0x%x:0x%x:0x%x\n",
> -  hdmi_readb(hdmi, HDMI_DESIGN_ID),
> -  hdmi_readb(hdmi, HDMI_REVISION_ID),
> -  hdmi_readb(hdmi, HDMI_PRODUCT_ID0),
> -  hdmi_readb(hdmi, HDMI_PRODUCT_ID1));
> + version = (hdmi_readb(hdmi, HDMI_DESIGN_ID) << 8)
> + | (hdmi_readb(hdmi, HDMI_REVISION_ID) << 0);
> + prod_id0 = hdmi_readb(hdmi, HDMI_PRODUCT_ID0);
> + prod_id1 = hdmi_readb(hdmi, HDMI_PRODUCT_ID1);
> +
> + if (prod_id0 != HDMI_PRODUCT_ID0_HDMI_TX ||
> + (prod_id1 & ~HDMI_PRODUCT_ID1_HDCP) != HDMI_PRODUCT_ID1_HDMI_TX) {
> + dev_err(dev, "Unsupported HDMI controller (%04x:%02x:%02x)\n",
> + version, prod_id0, prod_id1);
> + ret = -ENODEV;
> + goto err_iahb;
> + }
> +
> + dev_info(dev, "Detected HDMI TX controller v%x.%03x %s HDCP\n",
> +  version >> 12, version & 0xfff,
> +  prod_id1 & HDMI_PRODUCT_ID1_HDCP ? "with" : "without");
>  
>   initialize_hdmi_ih_mutes(hdmi);
>  
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.h 
> b/drivers/gpu/drm/bridge/dw-hdmi.h
> index 08235aef2fa3..91d7fabbd6e5 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.h
> @@ -545,6 +545,14 @@
>  #define HDMI_I2CM_FS_SCL_LCNT_0_ADDR0x7E12
>  
>  enum {
> +/* PRODUCT_ID0 field values */
> + HDMI_PRODUCT_ID0_HDMI_TX = 0xa0,
> +
> +/* PRODUCT_ID1 field values */
> + HDMI_PRODUCT_ID1_HDCP = 0xc0,
> + HDMI_PRODUCT_ID1_HDMI_RX = 0x02,
> + HDMI_PRODUCT_ID1_HDMI_TX = 0x01,
> +
>  /* CONFIG0_ID field values */
>   HDMI_CONFIG0_I2S = 0x10,
>  

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 05-12-2016 11:32, Laurent Pinchart wrote:
> Hi Jose,
>
> On Monday 05 Dec 2016 10:50:19 Jose Abreu wrote:
>> On 02-12-2016 15:43, Laurent Pinchart wrote:
>>> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>>>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>>>> From: Kieran Bingham <kieran.bingham+rene...@ideasonboard.com>
>>>>>
>>>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>>>> changes. Replace this with a quirk field, so that the platform can
>>>>> specify the required quirks for the driver, rather than the driver
>>>>> becoming conditional on multiple platforms.
>>>>>
>>>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>>>> SVSRET bit in later documentation.
>>>> I'd really prefer that we did not go down the broken route of adding
>>>> a set of "quirk" flags - look at what a mess SDHCI has become through
>>>> allowing that kind of practice.
>>>>
>>>> I'd much rather we find a saner structure to this - and we know that
>>>> the hardware has ID registers in it which can be used (so far) to
>>>> identify the buggy hardware.
>>> I'd much prefer something that would allow runtime identification of the
>>> device and the corresponding actions to be taken. However, the amount of
>>> documentation we have on the DWC HDMI TX IP core (and the associated PHY)
>>> is pretty limited, given that Synopsys doesn't make the documentation
>>> available publicly. Changes made to the IP core by integrators could
>>> complicate this further. I'm trying to gather as much information as
>>> possible to make clean the code up, for instance by trying to identify
>>> the PHYs used on the various platforms we support. Progress is slow on
>>> that front, there isn't enough leaked information available online :-) I
>>> haven't given up though, but I'll need more time.
>>>
>>> I don't like quirks much either. They are however already used today, even
>>> if we trigger them through dev_type instead of quirk flags. This patch
>>> came from a previous version found in a BSP that simply sprinkled several
>>> if (hdmi-> dev_type == RCAR_HDMI) through the code. For instance,
>>>
>>> -   if (hdmi->dev_type == RK3288_HDMI)
>>> +   if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
>>> dw_hdmi_phy_enable_spare(hdmi, 1);
>>>
>>> which I think is worse than flags as it would quickly degenerate to
>>> spaghetti code.
>>>
>>> For this specific case, we've managed to identify that on Renesas
>>> platforms the bit set by this function is called SVSRET. Its usage isn't
>>> clear yet, but I suspect it to control one of the PHY input control
>>> signals, like the other bits in the same register. I'm trying to get more
>>> information to clean the implementation further, hopefully with a way to
>>> determine whether the signal is used based on PHY identification.
>> SVSRET is a low power mode consumption and is a PHY input signal
>> as you suggested.
> Thank you for the confirmation. Would you happen to know what SVSRET stands 
> for ?

Have no info about that. Sorry.

>
>> Most of the configurable input signals of the PHY are available by the
>> controller regbank. I don't think it is possible to detect this at runtime,
>> I think you have at least to hardcode which version of the PHY you are
>> using.
>>
>> I would suggest that maybe all the PHY logic should be extracted and then
>> use callbacks to glue controller and phy. Then, depending on the PHY you
>> could use empty stubs if, for example, a given PHY did not support SVSRET.
>> Still, I don't know if this is the best option. What I do know is that there
>> are a large number of PHY's with different flavors that can use the same
>> controller. The controller has different versions also, and each version can
>> have quirks but I think it would be easier to manage this driver if we had a
>> clear distinction between PHY and controller.
> Agreed, I'd like to go in that direction. What makes it quite difficult is 
> the 
> lack of documentation about the PHYs :-) I've found six different PHY types 
> that can be identified by the CONFIG2_ID register:
>
> Bits| Field   | Description
> --
> 7-0 | phytype  

Re: [PATCH 13/22] drm: bridge: dw-hdmi: Replace device type with platform quirks

[Jose Abreu]

Hi Laurent,


On 02-12-2016 15:43, Laurent Pinchart wrote:
> Hi Russell,
>
> On Friday 02 Dec 2016 14:24:01 Russell King - ARM Linux wrote:
>> On Fri, Dec 02, 2016 at 01:43:28AM +0200, Laurent Pinchart wrote:
>>> From: Kieran Bingham 
>>>
>>> The dw-hdmi driver declares a dev_type to distinguish platform specific
>>> changes. Replace this with a quirk field, so that the platform can
>>> specify the required quirks for the driver, rather than the driver
>>> becoming conditional on multiple platforms.
>>>
>>> As part of this, we rename the dw-hdmi 'spare' which is defined as the
>>> SVSRET bit in later documentation.
>> I'd really prefer that we did not go down the broken route of adding
>> a set of "quirk" flags - look at what a mess SDHCI has become through
>> allowing that kind of practice.
>>
>> I'd much rather we find a saner structure to this - and we know that
>> the hardware has ID registers in it which can be used (so far) to
>> identify the buggy hardware.
> I'd much prefer something that would allow runtime identification of the 
> device and the corresponding actions to be taken. However, the amount of 
> documentation we have on the DWC HDMI TX IP core (and the associated PHY) is 
> pretty limited, given that Synopsys doesn't make the documentation available 
> publicly. Changes made to the IP core by integrators could complicate this 
> further. I'm trying to gather as much information as possible to make clean 
> the code up, for instance by trying to identify the PHYs used on the various 
> platforms we support. Progress is slow on that front, there isn't enough 
> leaked information available online :-) I haven't given up though, but I'll 
> need more time.
>
> I don't like quirks much either. They are however already used today, even if 
> we trigger them through dev_type instead of quirk flags. This patch came from 
> a previous version found in a BSP that simply sprinkled several if (hdmi-
>> dev_type == RCAR_HDMI) through the code. For instance,
> - if (hdmi->dev_type == RK3288_HDMI)
> + if (hdmi->dev_type == RK3288_HDMI || hdmi->dev_type == RCAR_HDMI)
>   dw_hdmi_phy_enable_spare(hdmi, 1);
>
> which I think is worse than flags as it would quickly degenerate to spaghetti 
> code.
>
> For this specific case, we've managed to identify that on Renesas platforms 
> the bit set by this function is called SVSRET. Its usage isn't clear yet, but 
> I suspect it to control one of the PHY input control signals, like the other 
> bits in the same register. I'm trying to get more information to clean the 
> implementation further, hopefully with a way to determine whether the signal 
> is used based on PHY identification.

SVSRET is a low power mode consumption and is a PHY input signal
as you suggested. Most of the configurable input signals of the
PHY are available by the controller regbank. I don't think it is
possible to detect this at runtime, I think you have at least to
hardcode which version of the PHY you are using.

I would suggest that maybe all the PHY logic should be extracted
and then use callbacks to glue controller and phy. Then,
depending on the PHY you could use empty stubs if, for example, a
given PHY did not support SVSRET. Still, I don't know if this is
the best option. What I do know is that there are a large number
of PHY's with different flavors that can use the same controller.
The controller has different versions also, and each version can
have quirks but I think it would be easier to manage this driver
if we had a clear distinction between PHY and controller.


>
> This is all work in progress, and if anyone has access to any documentation 
> and can provide additional information I'll be grateful.
>
>>> Signed-off-by: Kieran Bingham 
>>> Signed-off-by: Laurent Pinchart
>>> 
>>> ---
>>>
>>>  drivers/gpu/drm/bridge/dw-hdmi.c| 14 ++
>>>  drivers/gpu/drm/bridge/dw-hdmi.h|  4 ++--
>>>  drivers/gpu/drm/imx/dw_hdmi-imx.c   |  3 +--
>>>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  2 +-
>>>  include/drm/bridge/dw_hdmi.h| 12 +---
>>>  5 files changed, 15 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> b/drivers/gpu/drm/bridge/dw-hdmi.c index 06a44a2cdf3b..26607185722f
>>> 100644
>>> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
>>> @@ -118,7 +118,6 @@ struct dw_hdmi {
>>> struct drm_bridge bridge;
>>> struct platform_device *audio;
>>>
>>> -   enum dw_hdmi_devtype dev_type;
>>> struct device *dev;
>>> struct clk *isfr_clk;
>>> struct clk *iahb_clk;
>>> @@ -896,11 +895,11 @@ static void dw_hdmi_phy_enable_tmds(struct dw_hdmi
>>> *hdmi, u8 enable)
>>>  HDMI_PHY_CONF0_ENTMDS_MASK);
>>>  }
>>>
>>> -static void dw_hdmi_phy_enable_spare(struct dw_hdmi *hdmi, 

Re: [PATCH 12/22] drm: bridge: dw-hdmi: Abstract the platform PHY configuration

[Jose Abreu]

Hi Laurent,


On 01-12-2016 23:43, Laurent Pinchart wrote:
> From: Kieran Bingham 
>
> Platforms implement the dw-hdmi with a separate PHY entity. It is
> configured over it's own I2C bus. To allow for different PHY's to be
> configured from the dw-hdmi driver, abstract the actual programming of
> the PHY to its own functions, as configured by the platform.
>
> Signed-off-by: Kieran Bingham 
> Signed-off-by: Laurent Pinchart 
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c| 79 
> ++---
>  drivers/gpu/drm/imx/dw_hdmi-imx.c   |  2 +
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c |  1 +
>  include/drm/bridge/dw_hdmi.h| 12 +
>  4 files changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c 
> b/drivers/gpu/drm/bridge/dw-hdmi.c
> index 074ceb1e4897..06a44a2cdf3b 100644
> --- a/drivers/gpu/drm/bridge/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c
> @@ -867,8 +867,8 @@ static bool hdmi_phy_wait_i2c_done(struct dw_hdmi *hdmi, 
> int msec)
>   return true;
>  }
>  
> -static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> -  unsigned char addr)
> +void dw_hdmi_phy_i2c_write(struct dw_hdmi *hdmi, unsigned short data,
> +unsigned char addr)
>  {
>   hdmi_writeb(hdmi, 0xFF, HDMI_IH_I2CMPHY_STAT0);
>   hdmi_writeb(hdmi, addr, HDMI_PHY_I2CM_ADDRESS_ADDR);
> @@ -880,6 +880,7 @@ static void hdmi_phy_i2c_write(struct dw_hdmi *hdmi, 
> unsigned short data,
>   HDMI_PHY_I2CM_OPERATION_ADDR);
>   hdmi_phy_wait_i2c_done(hdmi, 1000);
>  }
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_i2c_write);
>  
>  static void dw_hdmi_phy_enable_powerdown(struct dw_hdmi *hdmi, bool enable)
>  {
> @@ -930,38 +931,61 @@ static void dw_hdmi_phy_sel_interface_control(struct 
> dw_hdmi *hdmi, u8 enable)
>HDMI_PHY_CONF0_SELDIPIF_MASK);
>  }
>  
> -static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> -   enum dw_hdmi_resolution res_idx, int cscon)
> +int dw_hdmi_phy_configure_synopsys(struct dw_hdmi *hdmi,
> +const struct dw_hdmi_plat_data *pdata,
> +unsigned long mpixelclock,
> +enum dw_hdmi_resolution resolution)
> +
>  {
> - u8 val, msec;
> - const struct dw_hdmi_plat_data *pdata = hdmi->plat_data;
>   const struct dw_hdmi_mpll_config *mpll_config = pdata->mpll_cfg;
>   const struct dw_hdmi_curr_ctrl *curr_ctrl = pdata->cur_ctr;
>   const struct dw_hdmi_phy_config *phy_config = pdata->phy_config;
>  
>   /* PLL/MPLL Cfg - always match on final entry */
>   for (; mpll_config->mpixelclock != ~0UL; mpll_config++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - mpll_config->mpixelclock)
> + if (mpixelclock <= mpll_config->mpixelclock)
>   break;
>  
>   for (; curr_ctrl->mpixelclock != ~0UL; curr_ctrl++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - curr_ctrl->mpixelclock)
> + if (mpixelclock <= curr_ctrl->mpixelclock)
>   break;
>  
>   for (; phy_config->mpixelclock != ~0UL; phy_config++)
> - if (hdmi->hdmi_data.video_mode.mpixelclock <=
> - phy_config->mpixelclock)
> + if (mpixelclock <= phy_config->mpixelclock)
>   break;
>  
>   if (mpll_config->mpixelclock == ~0UL ||
>   curr_ctrl->mpixelclock == ~0UL ||
> - phy_config->mpixelclock == ~0UL) {
> - dev_err(hdmi->dev, "Pixel clock %d - unsupported by HDMI\n",
> - hdmi->hdmi_data.video_mode.mpixelclock);
> + phy_config->mpixelclock == ~0UL)
>   return -EINVAL;
> - }
> +
> + dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].cpce, 0x06);
> + dw_hdmi_phy_i2c_write(hdmi, mpll_config->res[resolution].gmp, 0x15);
> +
> + /* CURRCTRL */
> + dw_hdmi_phy_i2c_write(hdmi, curr_ctrl->curr[resolution], 0x10);
> +
> + dw_hdmi_phy_i2c_write(hdmi, 0x, 0x13); /* PLLPHBYCTRL */
> + dw_hdmi_phy_i2c_write(hdmi, 0x0006, 0x17);
> +
> + dw_hdmi_phy_i2c_write(hdmi, phy_config->term, 0x19); /* TXTERM */
> + dw_hdmi_phy_i2c_write(hdmi, phy_config->sym_ctr, 0x09); /* CKSYMTXCTRL 
> */
> + dw_hdmi_phy_i2c_write(hdmi, phy_config->vlev_ctr, 0x0E); /* VLEVCTRL */
> +
> + /* REMOVE CLK TERM */
> + dw_hdmi_phy_i2c_write(hdmi, 0x8000, 0x05); /* CKCALCTRL */
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_phy_configure_synopsys);
> +
> +static int hdmi_phy_configure(struct dw_hdmi *hdmi,
> +   enum dw_hdmi_resolution resolution, int cscon)
> +{
> + const struct dw_hdmi_plat_data *pdata 

Re: [PATCH 00/22] R-Car Gen3 HDMI output support

[Jose Abreu]

  |  50 +++
>  drivers/gpu/drm/bridge/dw-hdmi.c   | 364 
> -
>  drivers/gpu/drm/bridge/dw-hdmi.h   |   4 +-
>  drivers/gpu/drm/imx/dw_hdmi-imx.c  |  19 +-
>  drivers/gpu/drm/rcar-du/Kconfig|   8 +
>  drivers/gpu/drm/rcar-du/Makefile   |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c |  81 -
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |   4 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c  |  13 +-
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h  |   1 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c  |   7 +
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h |  23 ++
>  drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c | 105 ++
>  drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c|  17 +-
>  include/drm/bridge/dw_hdmi.h   |  35 +-
>  21 files changed, 765 insertions(+), 295 deletions(-)
>  create mode 100644 
> Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.txt
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_dw_hdmi.c
>

Patches 01 to 13 are: Reviewed-by: Jose Abreu <joab...@synopsys.com>

I have one minor comment for patch 12.

Best regards,
Jose Miguel Abreu

Re: [PATCH 1/3] drm: bridge: add DesignWare HDMI I2S audio support

[Jose Abreu]

Hi,

On 01-08-2016 05:57, Kuninori Morimoto wrote:
> Hi Jose
> Cc: Mark, Thierry, Daniel
>
>>> From: Kuninori Morimoto <kuninori.morimoto...@renesas.com>
>>>
>>> Current dw-hdmi is supporting sound via AHB bus, but it has
>>> I2S audio feature too. This patch adds I2S audio support to dw-hdmi.
>>> This HDMI I2S is supported by using ALSA SoC common HDMI encoder
>>> driver.
>>>
>>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto...@renesas.com>
>>> ---
>>>
>> I just tested this patch and everything seems ok. Should I give
>> my tested-by?
> Thank you for your test. I'm happy if it could have it.
>
> Mark, Thierry, Daniel
> I wonder who can be maintainer for this patch ??
>
> Best regards
> ---
> Kuninori Morimoto

Tested-by: Jose Abreu <joab...@synopsys.com>

Best regards,
Jose Miguel Abreu

Re: [PATCH 1/3] drm: bridge: add DesignWare HDMI I2S audio support

[Jose Abreu]

Hi,


On 24-06-2016 03:40, Kuninori Morimoto wrote:
> From: Kuninori Morimoto 
>
> Current dw-hdmi is supporting sound via AHB bus, but it has
> I2S audio feature too. This patch adds I2S audio support to dw-hdmi.
> This HDMI I2S is supported by using ALSA SoC common HDMI encoder
> driver.
>
> Signed-off-by: Kuninori Morimoto 
> ---
>

I just tested this patch and everything seems ok. Should I give
my tested-by?

Best regards,
Jose Miguel Abreu