[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, this might cause sound glitches with some
HDMI devices, as the CTS+N is forced to zero when the stream is disabled
while the audio clock is still running.

This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it moves
the call to this function into dw_hdmi_audio_enable() and
dw_hdmi_audio_disable().

Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..5b6090c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -537,6 +537,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
 
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+	hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 {
 	unsigned long flags;
@@ -544,6 +550,7 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = true;
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+	hdmi_enable_audio_clk(hdmi, true);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -554,6 +561,7 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
 
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = false;
+	hdmi_enable_audio_clk(hdmi, false);
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
@@ -1343,11 +1351,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 	}
 }
 
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
-	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
 /* Workaround to clear the overflow condition */
 static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
 {
@@ -1430,7 +1433,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 
 		/* HDMI Initialization Step E - Configure audio */
 		hdmi_clk_regenerator_update_pixel_clock(hdmi);
-		hdmi_enable_audio_clk(hdmi);
+		hdmi_enable_audio_clk(hdmi, true);
 	}
 
 	/* not for DVI mode */
-- 
2.9.3

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, this might cause sound glitches with some
HDMI devices, as the CTS+N is forced to zero when the stream is disabled
while the audio clock is still running.

This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it moves
the call to this function into dw_hdmi_audio_enable() and
dw_hdmi_audio_disable().

Signed-off-by: Romain Perier <romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..5b6090c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -537,6 +537,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
 
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+	hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 {
 	unsigned long flags;
@@ -544,6 +550,7 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = true;
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+	hdmi_enable_audio_clk(hdmi, true);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -554,6 +561,7 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
 
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = false;
+	hdmi_enable_audio_clk(hdmi, false);
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
@@ -1343,11 +1351,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 	}
 }
 
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
-	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
 /* Workaround to clear the overflow condition */
 static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
 {
@@ -1430,7 +1433,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 
 		/* HDMI Initialization Step E - Configure audio */
 		hdmi_clk_regenerator_update_pixel_clock(hdmi);
-		hdmi_enable_audio_clk(hdmi);
+		hdmi_enable_audio_clk(hdmi, true);
 	}
 
 	/* not for DVI mode */
-- 
2.9.3

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
step E. and is kept enabled for later use. This clock should be enabled
and disabled along with the actual audio stream and not always on (that
is bad for PM). Futhermore, this might cause sound glitches with some
HDMI devices, as the CTS+N is forced to zero when the stream is disabled
while the audio clock is still running.

This commit adds a parameter to hdmi_audio_enable_clk() that controls
when the audio sample clock must be enabled or disabled. Then, it moves
the call to this function into dw_hdmi_audio_enable() and
dw_hdmi_audio_disable().

Signed-off-by: Romain Perier <romain.perier@collabora.com>
---
 drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..5b6090c 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -537,6 +537,12 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
 
+static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi, bool enable)
+{
+	hdmi_modb(hdmi, enable ? 0 : HDMI_MC_CLKDIS_AUDCLK_DISABLE,
+		  HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
+}
+
 void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 {
 	unsigned long flags;
@@ -544,6 +550,7 @@ void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = true;
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
+	hdmi_enable_audio_clk(hdmi, true);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
@@ -554,6 +561,7 @@ void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
 
 	spin_lock_irqsave(&hdmi->audio_lock, flags);
 	hdmi->audio_enable = false;
+	hdmi_enable_audio_clk(hdmi, false);
 	hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
 	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
 }
@@ -1343,11 +1351,6 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
 	}
 }
 
-static void hdmi_enable_audio_clk(struct dw_hdmi *hdmi)
-{
-	hdmi_modb(hdmi, 0, HDMI_MC_CLKDIS_AUDCLK_DISABLE, HDMI_MC_CLKDIS);
-}
-
 /* Workaround to clear the overflow condition */
 static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
 {
@@ -1430,7 +1433,7 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 
 		/* HDMI Initialization Step E - Configure audio */
 		hdmi_clk_regenerator_update_pixel_clock(hdmi);
-		hdmi_enable_audio_clk(hdmi);
+		hdmi_enable_audio_clk(hdmi, true);
 	}
 
 	/* not for DVI mode */
-- 
2.9.3

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().

How does this interact with the workaround given in my commit introducing
these functions?  (Commit b90120a96608).

Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
loaded with data prior to setting N non-zero.  If disabling the audio
clock prevents the audio FIFO being loaded, your patch will break iMX6.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().

How does this interact with the workaround given in my commit introducing
these functions?  (Commit b90120a96608).

Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
loaded with data prior to setting N non-zero.  If disabling the audio
clock prevents the audio FIFO being loaded, your patch will break iMX6.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().

How does this interact with the workaround given in my commit introducing
these functions?  (Commit b90120a96608).

Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
loaded with data prior to setting N non-zero.  If disabling the audio
clock prevents the audio FIFO being loaded, your patch will break iMX6.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hello,

Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>> step E. and is kept enabled for later use. This clock should be enabled
>> and disabled along with the actual audio stream and not always on (that
>> is bad for PM). Futhermore, this might cause sound glitches with some
>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>> while the audio clock is still running.
>>
>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>> when the audio sample clock must be enabled or disabled. Then, it moves
>> the call to this function into dw_hdmi_audio_enable() and
>> dw_hdmi_audio_disable().
> How does this interact with the workaround given in my commit introducing
> these functions?  (Commit b90120a96608).
>
> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> loaded with data prior to setting N non-zero.  If disabling the audio
> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>
Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
I only tested on Rockchip devices. An approach might be to introduce an
option for handling this errata, because that's platform specific and
other platforms (like Rockchip) are in conflict with this.

Romain

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hello,

Le 10/03/2017 ? 10:46, Russell King - ARM Linux a ?crit :
> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>> step E. and is kept enabled for later use. This clock should be enabled
>> and disabled along with the actual audio stream and not always on (that
>> is bad for PM). Futhermore, this might cause sound glitches with some
>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>> while the audio clock is still running.
>>
>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>> when the audio sample clock must be enabled or disabled. Then, it moves
>> the call to this function into dw_hdmi_audio_enable() and
>> dw_hdmi_audio_disable().
> How does this interact with the workaround given in my commit introducing
> these functions?  (Commit b90120a96608).
>
> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> loaded with data prior to setting N non-zero.  If disabling the audio
> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>
Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
I only tested on Rockchip devices. An approach might be to introduce an
option for handling this errata, because that's platform specific and
other platforms (like Rockchip) are in conflict with this.

Romain

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
> > On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> >> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >> step E. and is kept enabled for later use. This clock should be enabled
> >> and disabled along with the actual audio stream and not always on (that
> >> is bad for PM). Futhermore, this might cause sound glitches with some
> >> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >> while the audio clock is still running.
> >>
> >> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >> when the audio sample clock must be enabled or disabled. Then, it moves
> >> the call to this function into dw_hdmi_audio_enable() and
> >> dw_hdmi_audio_disable().
> > How does this interact with the workaround given in my commit introducing
> > these functions?  (Commit b90120a96608).
> >
> > Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> > loaded with data prior to setting N non-zero.  If disabling the audio
> > clock prevents the audio FIFO being loaded, your patch will break iMX6.
> >
> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
> I only tested on Rockchip devices. An approach might be to introduce an
> option for handling this errata, because that's platform specific and
> other platforms (like Rockchip) are in conflict with this.

I'd say that they _aren't_ in conflict with it - or are you saying
that the I2S driver was never functionally tested as working?

I also would not think that it's platform specific - remember that
this is Designware IP, and it's likely that other platforms with
exactly the same IP would suffer from the same problem.  It's
probably revision specific, but we need Synopsis' input on that.

However, I do believe that your patch probably doesn't have much
merit in any case: on a mode set, you enable the audio clock, and
it will remain enabled until the audio device is first opened and
then closed.  If another mode set comes along, then exactly the
same situation happens again.  So, really it isn't achieving all
that much.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 ? 10:46, Russell King - ARM Linux a ?crit :
> > On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
> >> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >> step E. and is kept enabled for later use. This clock should be enabled
> >> and disabled along with the actual audio stream and not always on (that
> >> is bad for PM). Futhermore, this might cause sound glitches with some
> >> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >> while the audio clock is still running.
> >>
> >> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >> when the audio sample clock must be enabled or disabled. Then, it moves
> >> the call to this function into dw_hdmi_audio_enable() and
> >> dw_hdmi_audio_disable().
> > How does this interact with the workaround given in my commit introducing
> > these functions?  (Commit b90120a96608).
> >
> > Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
> > loaded with data prior to setting N non-zero.  If disabling the audio
> > clock prevents the audio FIFO being loaded, your patch will break iMX6.
> >
> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
> I only tested on Rockchip devices. An approach might be to introduce an
> option for handling this errata, because that's platform specific and
> other platforms (like Rockchip) are in conflict with this.

I'd say that they _aren't_ in conflict with it - or are you saying
that the I2S driver was never functionally tested as working?

I also would not think that it's platform specific - remember that
this is Designware IP, and it's likely that other platforms with
exactly the same IP would suffer from the same problem.  It's
probably revision specific, but we need Synopsis' input on that.

However, I do believe that your patch probably doesn't have much
merit in any case: on a mode set, you enable the audio clock, and
it will remain enabled until the audio device is first opened and
then closed.  If another mode set comes along, then exactly the
same situation happens again.  So, really it isn't achieving all
that much.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hello,


Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 à 10:46, Russell King - ARM Linux a écrit :
>>> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>> and disabled along with the actual audio stream and not always on (that
>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>> while the audio clock is still running.
>>>>
>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>> the call to this function into dw_hdmi_audio_enable() and
>>>> dw_hdmi_audio_disable().
>>> How does this interact with the workaround given in my commit introducing
>>> these functions?  (Commit b90120a96608).
>>>
>>> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
>>> loaded with data prior to setting N non-zero.  If disabling the audio
>>> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>>>
>> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
>> I only tested on Rockchip devices. An approach might be to introduce an
>> option for handling this errata, because that's platform specific and
>> other platforms (like Rockchip) are in conflict with this.

> or are you saying
> that the I2S driver was never functionally tested as working?

No ^^

>
> I also would not think that it's platform specific - remember that
> this is Designware IP, and it's likely that other platforms with
> exactly the same IP would suffer from the same problem.  It's
> probably revision specific, but we need Synopsis' input on that.
>
> However, I do believe that your patch probably doesn't have much
> merit in any case: on a mode set, you enable the audio clock, and
> it will remain enabled until the audio device is first opened and
> then closed.  If another mode set comes along, then exactly the
> same situation happens again.  So, really it isn't achieving all
> that much.
>
The fact is we still have sound glitches caused by this workaround on
Rockchip, we probably have a revision
of the IP without this issue. If CTS+N is not forced to zero we no
longer have the glitches :/ (with or without touching the clock)

Romain

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hello,


Le 10/03/2017 ? 11:27, Russell King - ARM Linux a ?crit :
> On Fri, Mar 10, 2017 at 11:21:53AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 ? 10:46, Russell King - ARM Linux a ?crit :
>>> On Fri, Mar 10, 2017 at 10:35:09AM +0100, Romain Perier wrote:
>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>> and disabled along with the actual audio stream and not always on (that
>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>> while the audio clock is still running.
>>>>
>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>> the call to this function into dw_hdmi_audio_enable() and
>>>> dw_hdmi_audio_disable().
>>> How does this interact with the workaround given in my commit introducing
>>> these functions?  (Commit b90120a96608).
>>>
>>> Setting N=0 is a work-around for iMX6, and we need the audio FIFO to be
>>> loaded with data prior to setting N non-zero.  If disabling the audio
>>> clock prevents the audio FIFO being loaded, your patch will break iMX6.
>>>
>> Mhhh, the fact is I have no IMX6 devices here (only Rockchip). So
>> I only tested on Rockchip devices. An approach might be to introduce an
>> option for handling this errata, because that's platform specific and
>> other platforms (like Rockchip) are in conflict with this.

> or are you saying
> that the I2S driver was never functionally tested as working?

No ^^

>
> I also would not think that it's platform specific - remember that
> this is Designware IP, and it's likely that other platforms with
> exactly the same IP would suffer from the same problem.  It's
> probably revision specific, but we need Synopsis' input on that.
>
> However, I do believe that your patch probably doesn't have much
> merit in any case: on a mode set, you enable the audio clock, and
> it will remain enabled until the audio device is first opened and
> then closed.  If another mode set comes along, then exactly the
> same situation happens again.  So, really it isn't achieving all
> that much.
>
The fact is we still have sound glitches caused by this workaround on
Rockchip, we probably have a revision
of the IP without this issue. If CTS+N is not forced to zero we no
longer have the glitches :/ (with or without touching the clock)

Romain

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
> > I also would not think that it's platform specific - remember that
> > this is Designware IP, and it's likely that other platforms with
> > exactly the same IP would suffer from the same problem.  It's
> > probably revision specific, but we need Synopsis' input on that.
> >
> > However, I do believe that your patch probably doesn't have much
> > merit in any case: on a mode set, you enable the audio clock, and
> > it will remain enabled until the audio device is first opened and
> > then closed.  If another mode set comes along, then exactly the
> > same situation happens again.  So, really it isn't achieving all
> > that much.
>
> The fact is we still have sound glitches caused by this workaround on
> Rockchip, we probably have a revision
> of the IP without this issue. If CTS+N is not forced to zero we no
> longer have the glitches :/ (with or without touching the clock)

Are you sure that removing the workaround just doesn't result in the
same bug as on iMX6 appearing?  The bug concerned is the ordering of
the channels, so unless you're (eg0 monitoring left/right separately
or directing audio to just one channel, you may not (with modern TVs)
realise that the channels are swapped.  I'll include the errata
description and impact below.

There are occasional issues on iMX6 as well despite this work-around,
but I don't clearly remember what devmem2 tweaks I've done in the past
to get it to resolve itself, nor could I describe them from memory
any better than "burbling audio".


  When the AHB Audio DMA is started, by setting to 1'b1 for the first
  time the register field AHB_DMA_START.data_buffer_ready, the AHB
  Audio DMA will request data from the AHB bus to fill its internal
  AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
  during the time window between the first sample stored on the AHB
  DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
  of configured audio channels in samples. If this happens, the AHB
  DMA FIFO will reply with samples that are currently on the AHB
  Audio FIFO and will repeat the last sample after the empty condition
  is reached.

  Projected Impact:
  This will miss-align the audio stream, causing a shift in the
  distribution of audio on the channels on the HDMI sink side, with no
  knowledge of the DWC_hdmi_tx enabled system.


If you know that this definitely does not apply to your version, then
we need to split the audio enable/disable functions between the AHB
and I2S variants.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
> Hello,
> 
> Le 10/03/2017 ? 11:27, Russell King - ARM Linux a ?crit :
> > I also would not think that it's platform specific - remember that
> > this is Designware IP, and it's likely that other platforms with
> > exactly the same IP would suffer from the same problem.  It's
> > probably revision specific, but we need Synopsis' input on that.
> >
> > However, I do believe that your patch probably doesn't have much
> > merit in any case: on a mode set, you enable the audio clock, and
> > it will remain enabled until the audio device is first opened and
> > then closed.  If another mode set comes along, then exactly the
> > same situation happens again.  So, really it isn't achieving all
> > that much.
>
> The fact is we still have sound glitches caused by this workaround on
> Rockchip, we probably have a revision
> of the IP without this issue. If CTS+N is not forced to zero we no
> longer have the glitches :/ (with or without touching the clock)

Are you sure that removing the workaround just doesn't result in the
same bug as on iMX6 appearing?  The bug concerned is the ordering of
the channels, so unless you're (eg0 monitoring left/right separately
or directing audio to just one channel, you may not (with modern TVs)
realise that the channels are swapped.  I'll include the errata
description and impact below.

There are occasional issues on iMX6 as well despite this work-around,
but I don't clearly remember what devmem2 tweaks I've done in the past
to get it to resolve itself, nor could I describe them from memory
any better than "burbling audio".


  When the AHB Audio DMA is started, by setting to 1'b1 for the first
  time the register field AHB_DMA_START.data_buffer_ready, the AHB
  Audio DMA will request data from the AHB bus to fill its internal
  AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
  during the time window between the first sample stored on the AHB
  DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
  of configured audio channels in samples. If this happens, the AHB
  DMA FIFO will reply with samples that are currently on the AHB
  Audio FIFO and will repeat the last sample after the empty condition
  is reached.

  Projected Impact:
  This will miss-align the audio stream, causing a shift in the
  distribution of audio on the channels on the HDMI sink side, with no
  knowledge of the DWC_hdmi_tx enabled system.


If you know that this definitely does not apply to your version, then
we need to split the audio enable/disable functions between the AHB
and I2S variants.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

[-- Attachment #1: Type: text/plain, Size: 3259 bytes --]

Hello,


Le 10/03/2017 à 12:15, Russell King - ARM Linux a écrit :
> On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 à 11:27, Russell King - ARM Linux a écrit :
>>> I also would not think that it's platform specific - remember that
>>> this is Designware IP, and it's likely that other platforms with
>>> exactly the same IP would suffer from the same problem.  It's
>>> probably revision specific, but we need Synopsis' input on that.
>>>
>>> However, I do believe that your patch probably doesn't have much
>>> merit in any case: on a mode set, you enable the audio clock, and
>>> it will remain enabled until the audio device is first opened and
>>> then closed.  If another mode set comes along, then exactly the
>>> same situation happens again.  So, really it isn't achieving all
>>> that much.
>> The fact is we still have sound glitches caused by this workaround on
>> Rockchip, we probably have a revision
>> of the IP without this issue. If CTS+N is not forced to zero we no
>> longer have the glitches :/ (with or without touching the clock)
> Are you sure that removing the workaround just doesn't result in the
> same bug as on iMX6 appearing?  The bug concerned is the ordering of
> the channels, so unless you're (eg0 monitoring left/right separately
> or directing audio to just one channel, you may not (with modern TVs)
> realise that the channels are swapped.  I'll include the errata
> description and impact below.
>
> There are occasional issues on iMX6 as well despite this work-around,
> but I don't clearly remember what devmem2 tweaks I've done in the past
> to get it to resolve itself, nor could I describe them from memory
> any better than "burbling audio".

Well, I have reverted locally your patch (I did it quickly by hand
because it did not work via git revert... anyway...),
And I no longer have the issue at all. It's working fine all the time
and it's truly deterministic (see attachment)

Also, I have found that the version of the IP is not exactly the same.
On Rockchip it's 0x20:0xa:0xa0:0xc1, on IMX.6 it's  0x13:0xa:0xa0:0xc1.
>
>
>   When the AHB Audio DMA is started, by setting to 1'b1 for the first
>   time the register field AHB_DMA_START.data_buffer_ready, the AHB
>   Audio DMA will request data from the AHB bus to fill its internal
>   AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
>   during the time window between the first sample stored on the AHB
>   DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
>   of configured audio channels in samples. If this happens, the AHB
>   DMA FIFO will reply with samples that are currently on the AHB
>   Audio FIFO and will repeat the last sample after the empty condition
>   is reached.
>
>   Projected Impact:
>   This will miss-align the audio stream, causing a shift in the
>   distribution of audio on the channels on the HDMI sink side, with no
>   knowledge of the DWC_hdmi_tx enabled system.

Thanks for this, it helps!
It looks specific to AHB audio

>
>
> If you know that this definitely does not apply to your version, then
> we need to split the audio enable/disable functions between the AHB
> and I2S variants.
Mhhh, yeah both seem to work differently.

Romain


[-- Attachment #2: revert.patch --]
[-- Type: text/x-patch, Size: 3551 bytes --]

diff --git a/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c b/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
index aaf287d..ce589402 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi-i2s-audio.c
@@ -67,8 +67,6 @@ static int dw_hdmi_i2s_hw_params(struct device *dev, void *data,
 	hdmi_write(audio, conf0, HDMI_AUD_CONF0);
 	hdmi_write(audio, conf1, HDMI_AUD_CONF1);
 
-	dw_hdmi_audio_enable(hdmi);
-
 	return 0;
 }
 
@@ -77,8 +75,6 @@ static void dw_hdmi_i2s_audio_shutdown(struct device *dev, void *data)
 	struct dw_hdmi_i2s_audio_data *audio = data;
 	struct dw_hdmi *hdmi = audio->hdmi;
 
-	dw_hdmi_audio_disable(hdmi);
-
 	hdmi_write(audio, HDMI_AUD_CONF0_SW_RESET, HDMI_AUD_CONF0);
 }
 
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index b621fc7..7529cb9 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -19,7 +19,6 @@
 #include <linux/hdmi.h>
 #include <linux/mutex.h>
 #include <linux/of_device.h>
-#include <linux/spinlock.h>
 
 #include <drm/drm_of.h>
 #include <drm/drmP.h>
@@ -148,12 +147,8 @@ struct dw_hdmi {
 	bool rxsense;			/* rxsense state */
 	u8 phy_mask;			/* desired phy int mask settings */
 
-	spinlock_t audio_lock;
 	struct mutex audio_mutex;
 	unsigned int sample_rate;
-	unsigned int audio_cts;
-	unsigned int audio_n;
-	bool audio_enable;
 
 	void (*write)(struct dw_hdmi *hdmi, u8 val, int offset);
 	u8 (*read)(struct dw_hdmi *hdmi, int offset);
@@ -505,11 +500,7 @@ static void hdmi_set_clk_regenerator(struct dw_hdmi *hdmi,
 		__func__, sample_rate, ftdms / 1000000, (ftdms / 1000) % 1000,
 		n, cts);
 
-	spin_lock_irq(&hdmi->audio_lock);
-	hdmi->audio_n = n;
-	hdmi->audio_cts = cts;
-	hdmi_set_cts_n(hdmi, cts, hdmi->audio_enable ? n : 0);
-	spin_unlock_irq(&hdmi->audio_lock);
+	hdmi_set_cts_n(hdmi, cts, n);
 }
 
 static void hdmi_init_clk_regenerator(struct dw_hdmi *hdmi)
@@ -537,28 +528,6 @@ void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate)
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_set_sample_rate);
 
-void dw_hdmi_audio_enable(struct dw_hdmi *hdmi)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&hdmi->audio_lock, flags);
-	hdmi->audio_enable = true;
-	hdmi_set_cts_n(hdmi, hdmi->audio_cts, hdmi->audio_n);
-	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
-}
-EXPORT_SYMBOL_GPL(dw_hdmi_audio_enable);
-
-void dw_hdmi_audio_disable(struct dw_hdmi *hdmi)
-{
-	unsigned long flags;
-
-	spin_lock_irqsave(&hdmi->audio_lock, flags);
-	hdmi->audio_enable = false;
-	hdmi_set_cts_n(hdmi, hdmi->audio_cts, 0);
-	spin_unlock_irqrestore(&hdmi->audio_lock, flags);
-}
-EXPORT_SYMBOL_GPL(dw_hdmi_audio_disable);
-
 /*
  * this submodule is responsible for the video data synchronization.
  * for example, for RGB 4:4:4 input, the data map is defined as
@@ -1894,7 +1863,6 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 
 	mutex_init(&hdmi->mutex);
 	mutex_init(&hdmi->audio_mutex);
-	spin_lock_init(&hdmi->audio_lock);
 
 	of_property_read_u32(np, "reg-io-width", &val);
 
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index bae79f3..f040ade 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -63,7 +63,4 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 		 const struct dw_hdmi_plat_data *plat_data);
 
 void dw_hdmi_set_sample_rate(struct dw_hdmi *hdmi, unsigned int rate);
-void dw_hdmi_audio_enable(struct dw_hdmi *hdmi);
-void dw_hdmi_audio_disable(struct dw_hdmi *hdmi);
-
 #endif /* __IMX_HDMI_H__ */

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hello,


Le 10/03/2017 ? 12:15, Russell King - ARM Linux a ?crit :
> On Fri, Mar 10, 2017 at 11:58:19AM +0100, Romain Perier wrote:
>> Hello,
>>
>> Le 10/03/2017 ? 11:27, Russell King - ARM Linux a ?crit :
>>> I also would not think that it's platform specific - remember that
>>> this is Designware IP, and it's likely that other platforms with
>>> exactly the same IP would suffer from the same problem.  It's
>>> probably revision specific, but we need Synopsis' input on that.
>>>
>>> However, I do believe that your patch probably doesn't have much
>>> merit in any case: on a mode set, you enable the audio clock, and
>>> it will remain enabled until the audio device is first opened and
>>> then closed.  If another mode set comes along, then exactly the
>>> same situation happens again.  So, really it isn't achieving all
>>> that much.
>> The fact is we still have sound glitches caused by this workaround on
>> Rockchip, we probably have a revision
>> of the IP without this issue. If CTS+N is not forced to zero we no
>> longer have the glitches :/ (with or without touching the clock)
> Are you sure that removing the workaround just doesn't result in the
> same bug as on iMX6 appearing?  The bug concerned is the ordering of
> the channels, so unless you're (eg0 monitoring left/right separately
> or directing audio to just one channel, you may not (with modern TVs)
> realise that the channels are swapped.  I'll include the errata
> description and impact below.
>
> There are occasional issues on iMX6 as well despite this work-around,
> but I don't clearly remember what devmem2 tweaks I've done in the past
> to get it to resolve itself, nor could I describe them from memory
> any better than "burbling audio".

Well, I have reverted locally your patch (I did it quickly by hand
because it did not work via git revert... anyway...),
And I no longer have the issue at all. It's working fine all the time
and it's truly deterministic (see attachment)

Also, I have found that the version of the IP is not exactly the same.
On Rockchip it's 0x20:0xa:0xa0:0xc1, on IMX.6 it's  0x13:0xa:0xa0:0xc1.
>
>
>   When the AHB Audio DMA is started, by setting to 1'b1 for the first
>   time the register field AHB_DMA_START.data_buffer_ready, the AHB
>   Audio DMA will request data from the AHB bus to fill its internal
>   AHB DMA FIFO. It is possible that a AHB DMA FIFO read action occurs
>   during the time window between the first sample stored on the AHB
>   DMA FIFO and when the AHB DMA FIFO has stored, at least, the number
>   of configured audio channels in samples. If this happens, the AHB
>   DMA FIFO will reply with samples that are currently on the AHB
>   Audio FIFO and will repeat the last sample after the empty condition
>   is reached.
>
>   Projected Impact:
>   This will miss-align the audio stream, causing a shift in the
>   distribution of audio on the channels on the HDMI sink side, with no
>   knowledge of the DWC_hdmi_tx enabled system.

Thanks for this, it helps!
It looks specific to AHB audio

>
>
> If you know that this definitely does not apply to your version, then
> we need to split the audio enable/disable functions between the AHB
> and I2S variants.
Mhhh, yeah both seem to work differently.

Romain

-------------- next part --------------
A non-text attachment was scrubbed...
Name: revert.patch
Type: text/x-patch
Size: 3551 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170310/f920ffa6/attachment.bin>

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/10/2017 10:35 AM, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

Hi Romain, Russell, Jose,

This is a little out of scope, but I was wondering why the CTS calculation was not left
in AUTO mode in the dw-hdmi driver ?

In the Amlogic Vendor tree, they don't set the HDMI_AUD_CTS3_CTS_MANUAL bit and leave
the CTS value to 0.

I am wondering if the AUTO feature is specific to newer IP version, or it was disabled
in the iMX.6 IP configuration ? or simply tied to the AHB audio specificity.

It would be simpler and smarter to leave the AUTO feature enabled for I2S and S/PDIF
input when the IP supports it.

Jose, is there a config bit for this feature, or a specific IP version where it landed ?

Anyway, we should really differentiate the AHB audio and I2S/SPDIF audio in the CTS/N calculation,
since the I2S/SPDIF depends on an different clock for audio input and the AUTO CTS calculation
feature will certainly offer a better clock synchronization.

Thanks,
Neil

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/10/2017 10:35 AM, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

Hi Romain, Russell, Jose,

This is a little out of scope, but I was wondering why the CTS calculation was not left
in AUTO mode in the dw-hdmi driver ?

In the Amlogic Vendor tree, they don't set the HDMI_AUD_CTS3_CTS_MANUAL bit and leave
the CTS value to 0.

I am wondering if the AUTO feature is specific to newer IP version, or it was disabled
in the iMX.6 IP configuration ? or simply tied to the AHB audio specificity.

It would be simpler and smarter to leave the AUTO feature enabled for I2S and S/PDIF
input when the IP supports it.

Jose, is there a config bit for this feature, or a specific IP version where it landed ?

Anyway, we should really differentiate the AHB audio and I2S/SPDIF audio in the CTS/N calculation,
since the I2S/SPDIF depends on an different clock for audio input and the AUTO CTS calculation
feature will certainly offer a better clock synchronization.

Thanks,
Neil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/10/2017 10:35 AM, Romain Perier wrote:
> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> step E. and is kept enabled for later use. This clock should be enabled
> and disabled along with the actual audio stream and not always on (that
> is bad for PM). Futhermore, this might cause sound glitches with some
> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> while the audio clock is still running.
> 
> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> when the audio sample clock must be enabled or disabled. Then, it moves
> the call to this function into dw_hdmi_audio_enable() and
> dw_hdmi_audio_disable().
> 
> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> ---
>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 

Hi Romain, Russell, Jose,

This is a little out of scope, but I was wondering why the CTS calculation was not left
in AUTO mode in the dw-hdmi driver ?

In the Amlogic Vendor tree, they don't set the HDMI_AUD_CTS3_CTS_MANUAL bit and leave
the CTS value to 0.

I am wondering if the AUTO feature is specific to newer IP version, or it was disabled
in the iMX.6 IP configuration ? or simply tied to the AHB audio specificity.

It would be simpler and smarter to leave the AUTO feature enabled for I2S and S/PDIF
input when the IP supports it.

Jose, is there a config bit for this feature, or a specific IP version where it landed ?

Anyway, we should really differentiate the AHB audio and I2S/SPDIF audio in the CTS/N calculation,
since the I2S/SPDIF depends on an different clock for audio input and the AUTO CTS calculation
feature will certainly offer a better clock synchronization.

Thanks,
Neil

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
> On 03/10/2017 10:35 AM, Romain Perier wrote:
> > Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> > step E. and is kept enabled for later use. This clock should be enabled
> > and disabled along with the actual audio stream and not always on (that
> > is bad for PM). Futhermore, this might cause sound glitches with some
> > HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> > while the audio clock is still running.
> > 
> > This commit adds a parameter to hdmi_audio_enable_clk() that controls
> > when the audio sample clock must be enabled or disabled. Then, it moves
> > the call to this function into dw_hdmi_audio_enable() and
> > dw_hdmi_audio_disable().
> > 
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
> > ---
> >  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> 
> Hi Romain, Russell, Jose,
> 
> This is a little out of scope, but I was wondering why the CTS calculation
> was not left in AUTO mode in the dw-hdmi driver ?

There is no indication in the iMX6 manuals that the iMX6 supports
automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
marked as "reserved".

We're reliant on the information in the iMX6 manuals as we don't have
access to Synopsis' databooks for these parts (I understand you have
to be a customer to have access to that.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
> On 03/10/2017 10:35 AM, Romain Perier wrote:
> > Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> > step E. and is kept enabled for later use. This clock should be enabled
> > and disabled along with the actual audio stream and not always on (that
> > is bad for PM). Futhermore, this might cause sound glitches with some
> > HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> > while the audio clock is still running.
> > 
> > This commit adds a parameter to hdmi_audio_enable_clk() that controls
> > when the audio sample clock must be enabled or disabled. Then, it moves
> > the call to this function into dw_hdmi_audio_enable() and
> > dw_hdmi_audio_disable().
> > 
> > Signed-off-by: Romain Perier <romain.perier@collabora.com>
> > ---
> >  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> 
> Hi Romain, Russell, Jose,
> 
> This is a little out of scope, but I was wondering why the CTS calculation
> was not left in AUTO mode in the dw-hdmi driver ?

There is no indication in the iMX6 manuals that the iMX6 supports
automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
marked as "reserved".

We're reliant on the information in the iMX6 manuals as we don't have
access to Synopsis' databooks for these parts (I understand you have
to be a customer to have access to that.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi,


On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>> step E. and is kept enabled for later use. This clock should be enabled
>>> and disabled along with the actual audio stream and not always on (that
>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>> while the audio clock is still running.
>>>
>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>> the call to this function into dw_hdmi_audio_enable() and
>>> dw_hdmi_audio_disable().
>>>
>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>> ---
>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>> Hi Romain, Russell, Jose,
>>
>> This is a little out of scope, but I was wondering why the CTS calculation
>> was not left in AUTO mode in the dw-hdmi driver ?
> There is no indication in the iMX6 manuals that the iMX6 supports
> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> marked as "reserved".
>
> We're reliant on the information in the iMX6 manuals as we don't have
> access to Synopsis' databooks for these parts (I understand you have
> to be a customer to have access to that.)
>

(Synopsis -> Synopsys :))

Trying to catch up with the conversation:

1) In AHB audio mode the bits are always reserved.
2) I think we should enable/disable clock instead of just forcing
N/CTS, though, I don't know what could be the implications for
iMX platform. I remember at the time I tested this using I2S
(I've never used AHB), and HDMI protocol analyzers were
complaining about the N/CTS being forced to zero.
3) I also remember that there was something wrong with the N
calculations (I had to remove the if for pixel clock ==
25175000). I never submited a patch to that because I was running
out of time and didn't investigate this further (it could be for
example a problem specific to my setup).

Hope it helps a little bit.

Best regards,
Jose Miguel Abreu

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi,


On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>> step E. and is kept enabled for later use. This clock should be enabled
>>> and disabled along with the actual audio stream and not always on (that
>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>> while the audio clock is still running.
>>>
>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>> the call to this function into dw_hdmi_audio_enable() and
>>> dw_hdmi_audio_disable().
>>>
>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>> ---
>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>> Hi Romain, Russell, Jose,
>>
>> This is a little out of scope, but I was wondering why the CTS calculation
>> was not left in AUTO mode in the dw-hdmi driver ?
> There is no indication in the iMX6 manuals that the iMX6 supports
> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> marked as "reserved".
>
> We're reliant on the information in the iMX6 manuals as we don't have
> access to Synopsis' databooks for these parts (I understand you have
> to be a customer to have access to that.)
>

(Synopsis -> Synopsys :))

Trying to catch up with the conversation:

1) In AHB audio mode the bits are always reserved.
2) I think we should enable/disable clock instead of just forcing
N/CTS, though, I don't know what could be the implications for
iMX platform. I remember at the time I tested this using I2S
(I've never used AHB), and HDMI protocol analyzers were
complaining about the N/CTS being forced to zero.
3) I also remember that there was something wrong with the N
calculations (I had to remove the if for pixel clock ==
25175000). I never submited a patch to that because I was running
out of time and didn't investigate this further (it could be for
example a problem specific to my setup).

Hope it helps a little bit.

Best regards,
Jose Miguel Abreu

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi,


On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>> step E. and is kept enabled for later use. This clock should be enabled
>>> and disabled along with the actual audio stream and not always on (that
>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>> while the audio clock is still running.
>>>
>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>> the call to this function into dw_hdmi_audio_enable() and
>>> dw_hdmi_audio_disable().
>>>
>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>> ---
>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>
>> Hi Romain, Russell, Jose,
>>
>> This is a little out of scope, but I was wondering why the CTS calculation
>> was not left in AUTO mode in the dw-hdmi driver ?
> There is no indication in the iMX6 manuals that the iMX6 supports
> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> marked as "reserved".
>
> We're reliant on the information in the iMX6 manuals as we don't have
> access to Synopsis' databooks for these parts (I understand you have
> to be a customer to have access to that.)
>

(Synopsis -> Synopsys :))

Trying to catch up with the conversation:

1) In AHB audio mode the bits are always reserved.
2) I think we should enable/disable clock instead of just forcing
N/CTS, though, I don't know what could be the implications for
iMX platform. I remember at the time I tested this using I2S
(I've never used AHB), and HDMI protocol analyzers were
complaining about the N/CTS being forced to zero.
3) I also remember that there was something wrong with the N
calculations (I had to remove the if for pixel clock ==
25175000). I never submited a patch to that because I was running
out of time and didn't investigate this further (it could be for
example a problem specific to my setup).

Hope it helps a little bit.

Best regards,
Jose Miguel Abreu

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
> Hi,
> 
> 
> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> > On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
> >> On 03/10/2017 10:35 AM, Romain Perier wrote:
> >>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >>> step E. and is kept enabled for later use. This clock should be enabled
> >>> and disabled along with the actual audio stream and not always on (that
> >>> is bad for PM). Futhermore, this might cause sound glitches with some
> >>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >>> while the audio clock is still running.
> >>>
> >>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >>> when the audio sample clock must be enabled or disabled. Then, it moves
> >>> the call to this function into dw_hdmi_audio_enable() and
> >>> dw_hdmi_audio_disable().
> >>>
> >>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> >>> ---
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
> >>>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>>
> >> Hi Romain, Russell, Jose,
> >>
> >> This is a little out of scope, but I was wondering why the CTS calculation
> >> was not left in AUTO mode in the dw-hdmi driver ?
> > There is no indication in the iMX6 manuals that the iMX6 supports
> > automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> > marked as "reserved".
> >
> > We're reliant on the information in the iMX6 manuals as we don't have
> > access to Synopsis' databooks for these parts (I understand you have
> > to be a customer to have access to that.)
> >
> 
> (Synopsis -> Synopsys :))
> 
> Trying to catch up with the conversation:
> 
> 1) In AHB audio mode the bits are always reserved.
> 2) I think we should enable/disable clock instead of just forcing
> N/CTS, though, I don't know what could be the implications for
> iMX platform. I remember at the time I tested this using I2S
> (I've never used AHB), and HDMI protocol analyzers were
> complaining about the N/CTS being forced to zero.

What you're recommending is to basically ignore the published workaround,
which, presumably, was arrived at by proper analysis of the issue both
by Freescale engineers and Synopsys engineers, and instead try some other
solution.

I'm not sure that's a good idea.  Only those with knowledge of the IP can
say what effect disabling the audio clock will have: will it still allow
the FIFO to be loaded, as required by the errata?  If not, it's not a
solution that AHB can use.  I would imagine that _if_ it was as simple as
disabling the audio clock, that simple approach would have been documented
in Freescale's errata documents, rather than the rather more complex
method of zeroing the CTS/N values.

I think there are two choices here:

1) get some definitive information about the IP and the errata for AHB,
   and determine whether the solution you propose would work.  (That's
   out of scope for me, I've no contacts with FSL/NXP and I'm not a
   Synopsys customer.)

2) the I2S audio support stops re-using the AHB audio support functions,
   switching to their own implementation that behaves correctly for them.
   (Remember, it was I2S's choice to re-use the AHB audio support functions
   I added which we're now discussing - they didn't have to do that, and
   regressing AHB audio just for I2S is not something we should ever
   consider doing.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Russell King - ARM Linux]

On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
> Hi,
> 
> 
> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
> > On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
> >> On 03/10/2017 10:35 AM, Romain Perier wrote:
> >>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
> >>> step E. and is kept enabled for later use. This clock should be enabled
> >>> and disabled along with the actual audio stream and not always on (that
> >>> is bad for PM). Futhermore, this might cause sound glitches with some
> >>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
> >>> while the audio clock is still running.
> >>>
> >>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
> >>> when the audio sample clock must be enabled or disabled. Then, it moves
> >>> the call to this function into dw_hdmi_audio_enable() and
> >>> dw_hdmi_audio_disable().
> >>>
> >>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
> >>> ---
> >>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
> >>>  1 file changed, 9 insertions(+), 6 deletions(-)
> >>>
> >> Hi Romain, Russell, Jose,
> >>
> >> This is a little out of scope, but I was wondering why the CTS calculation
> >> was not left in AUTO mode in the dw-hdmi driver ?
> > There is no indication in the iMX6 manuals that the iMX6 supports
> > automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
> > marked as "reserved".
> >
> > We're reliant on the information in the iMX6 manuals as we don't have
> > access to Synopsis' databooks for these parts (I understand you have
> > to be a customer to have access to that.)
> >
> 
> (Synopsis -> Synopsys :))
> 
> Trying to catch up with the conversation:
> 
> 1) In AHB audio mode the bits are always reserved.
> 2) I think we should enable/disable clock instead of just forcing
> N/CTS, though, I don't know what could be the implications for
> iMX platform. I remember at the time I tested this using I2S
> (I've never used AHB), and HDMI protocol analyzers were
> complaining about the N/CTS being forced to zero.

What you're recommending is to basically ignore the published workaround,
which, presumably, was arrived at by proper analysis of the issue both
by Freescale engineers and Synopsys engineers, and instead try some other
solution.

I'm not sure that's a good idea.  Only those with knowledge of the IP can
say what effect disabling the audio clock will have: will it still allow
the FIFO to be loaded, as required by the errata?  If not, it's not a
solution that AHB can use.  I would imagine that _if_ it was as simple as
disabling the audio clock, that simple approach would have been documented
in Freescale's errata documents, rather than the rather more complex
method of zeroing the CTS/N values.

I think there are two choices here:

1) get some definitive information about the IP and the errata for AHB,
   and determine whether the solution you propose would work.  (That's
   out of scope for me, I've no contacts with FSL/NXP and I'm not a
   Synopsys customer.)

2) the I2S audio support stops re-using the AHB audio support functions,
   switching to their own implementation that behaves correctly for them.
   (Remember, it was I2S's choice to re-use the AHB audio support functions
   I added which we're now discussing - they didn't have to do that, and
   regressing AHB audio just for I2S is not something we should ever
   consider doing.)

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi Russell,


On 13-03-2017 13:10, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>> Hi,
>>
>>
>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>> and disabled along with the actual audio stream and not always on (that
>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>> while the audio clock is still running.
>>>>>
>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>> dw_hdmi_audio_disable().
>>>>>
>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>
>>>> Hi Romain, Russell, Jose,
>>>>
>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>> marked as "reserved".
>>>
>>> We're reliant on the information in the iMX6 manuals as we don't have
>>> access to Synopsis' databooks for these parts (I understand you have
>>> to be a customer to have access to that.)
>>>
>> (Synopsis -> Synopsys :))
>>
>> Trying to catch up with the conversation:
>>
>> 1) In AHB audio mode the bits are always reserved.
>> 2) I think we should enable/disable clock instead of just forcing
>> N/CTS, though, I don't know what could be the implications for
>> iMX platform. I remember at the time I tested this using I2S
>> (I've never used AHB), and HDMI protocol analyzers were
>> complaining about the N/CTS being forced to zero.
> What you're recommending is to basically ignore the published workaround,
> which, presumably, was arrived at by proper analysis of the issue both
> by Freescale engineers and Synopsys engineers, and instead try some other
> solution.
>
> I'm not sure that's a good idea.  Only those with knowledge of the IP can
> say what effect disabling the audio clock will have: will it still allow
> the FIFO to be loaded, as required by the errata?  If not, it's not a
> solution that AHB can use.  I would imagine that _if_ it was as simple as
> disabling the audio clock, that simple approach would have been documented
> in Freescale's errata documents, rather than the rather more complex
> method of zeroing the CTS/N values.

I'm just following what the user guide says: the final step of
configuration is enabling the audio clock. There is no reference
neither in datasheet neither in user guide about this behavior
but, as I said, I've never used AHB so I can't prove what is the
best solution. Could it be platform specific?

>
> I think there are two choices here:
>
> 1) get some definitive information about the IP and the errata for AHB,
>    and determine whether the solution you propose would work.  (That's
>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>    Synopsys customer.)

This is the right thing to do but if you can't test in the
Freescale platform then we have not much else to do.

Best regards,
Jose Miguel Abreu

>
> 2) the I2S audio support stops re-using the AHB audio support functions,
>    switching to their own implementation that behaves correctly for them.
>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>    I added which we're now discussing - they didn't have to do that, and
>    regressing AHB audio just for I2S is not something we should ever
>    consider doing.)
>

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi Russell,


On 13-03-2017 13:10, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>> Hi,
>>
>>
>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>> and disabled along with the actual audio stream and not always on (that
>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>> while the audio clock is still running.
>>>>>
>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>> dw_hdmi_audio_disable().
>>>>>
>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>
>>>> Hi Romain, Russell, Jose,
>>>>
>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>> marked as "reserved".
>>>
>>> We're reliant on the information in the iMX6 manuals as we don't have
>>> access to Synopsis' databooks for these parts (I understand you have
>>> to be a customer to have access to that.)
>>>
>> (Synopsis -> Synopsys :))
>>
>> Trying to catch up with the conversation:
>>
>> 1) In AHB audio mode the bits are always reserved.
>> 2) I think we should enable/disable clock instead of just forcing
>> N/CTS, though, I don't know what could be the implications for
>> iMX platform. I remember at the time I tested this using I2S
>> (I've never used AHB), and HDMI protocol analyzers were
>> complaining about the N/CTS being forced to zero.
> What you're recommending is to basically ignore the published workaround,
> which, presumably, was arrived at by proper analysis of the issue both
> by Freescale engineers and Synopsys engineers, and instead try some other
> solution.
>
> I'm not sure that's a good idea.  Only those with knowledge of the IP can
> say what effect disabling the audio clock will have: will it still allow
> the FIFO to be loaded, as required by the errata?  If not, it's not a
> solution that AHB can use.  I would imagine that _if_ it was as simple as
> disabling the audio clock, that simple approach would have been documented
> in Freescale's errata documents, rather than the rather more complex
> method of zeroing the CTS/N values.

I'm just following what the user guide says: the final step of
configuration is enabling the audio clock. There is no reference
neither in datasheet neither in user guide about this behavior
but, as I said, I've never used AHB so I can't prove what is the
best solution. Could it be platform specific?

>
> I think there are two choices here:
>
> 1) get some definitive information about the IP and the errata for AHB,
>    and determine whether the solution you propose would work.  (That's
>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>    Synopsys customer.)

This is the right thing to do but if you can't test in the
Freescale platform then we have not much else to do.

Best regards,
Jose Miguel Abreu

>
> 2) the I2S audio support stops re-using the AHB audio support functions,
>    switching to their own implementation that behaves correctly for them.
>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>    I added which we're now discussing - they didn't have to do that, and
>    regressing AHB audio just for I2S is not something we should ever
>    consider doing.)
>

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Jose Abreu]

Hi Russell,


On 13-03-2017 13:10, Russell King - ARM Linux wrote:
> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>> Hi,
>>
>>
>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>> and disabled along with the actual audio stream and not always on (that
>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>> while the audio clock is still running.
>>>>>
>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>> dw_hdmi_audio_disable().
>>>>>
>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>> ---
>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>
>>>> Hi Romain, Russell, Jose,
>>>>
>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>> marked as "reserved".
>>>
>>> We're reliant on the information in the iMX6 manuals as we don't have
>>> access to Synopsis' databooks for these parts (I understand you have
>>> to be a customer to have access to that.)
>>>
>> (Synopsis -> Synopsys :))
>>
>> Trying to catch up with the conversation:
>>
>> 1) In AHB audio mode the bits are always reserved.
>> 2) I think we should enable/disable clock instead of just forcing
>> N/CTS, though, I don't know what could be the implications for
>> iMX platform. I remember at the time I tested this using I2S
>> (I've never used AHB), and HDMI protocol analyzers were
>> complaining about the N/CTS being forced to zero.
> What you're recommending is to basically ignore the published workaround,
> which, presumably, was arrived at by proper analysis of the issue both
> by Freescale engineers and Synopsys engineers, and instead try some other
> solution.
>
> I'm not sure that's a good idea.  Only those with knowledge of the IP can
> say what effect disabling the audio clock will have: will it still allow
> the FIFO to be loaded, as required by the errata?  If not, it's not a
> solution that AHB can use.  I would imagine that _if_ it was as simple as
> disabling the audio clock, that simple approach would have been documented
> in Freescale's errata documents, rather than the rather more complex
> method of zeroing the CTS/N values.

I'm just following what the user guide says: the final step of
configuration is enabling the audio clock. There is no reference
neither in datasheet neither in user guide about this behavior
but, as I said, I've never used AHB so I can't prove what is the
best solution. Could it be platform specific?

>
> I think there are two choices here:
>
> 1) get some definitive information about the IP and the errata for AHB,
>    and determine whether the solution you propose would work.  (That's
>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>    Synopsys customer.)

This is the right thing to do but if you can't test in the
Freescale platform then we have not much else to do.

Best regards,
Jose Miguel Abreu

>
> 2) the I2S audio support stops re-using the AHB audio support functions,
>    switching to their own implementation that behaves correctly for them.
>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>    I added which we're now discussing - they didn't have to do that, and
>    regressing AHB audio just for I2S is not something we should ever
>    consider doing.)
>

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hi,


Le 13/03/2017 à 19:49, Jose Abreu a écrit :
> Hi Russell,
>
>
> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>> Hi,
>>>
>>>
>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>>> and disabled along with the actual audio stream and not always on (that
>>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>>> while the audio clock is still running.
>>>>>>
>>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>>> dw_hdmi_audio_disable().
>>>>>>
>>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>
>>>>> Hi Romain, Russell, Jose,
>>>>>
>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>> marked as "reserved".
>>>>
>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>> access to Synopsis' databooks for these parts (I understand you have
>>>> to be a customer to have access to that.)
>>>>
>>> (Synopsis -> Synopsys :))
>>>
>>> Trying to catch up with the conversation:
>>>
>>> 1) In AHB audio mode the bits are always reserved.
>>> 2) I think we should enable/disable clock instead of just forcing
>>> N/CTS, though, I don't know what could be the implications for
>>> iMX platform. I remember at the time I tested this using I2S
>>> (I've never used AHB), and HDMI protocol analyzers were
>>> complaining about the N/CTS being forced to zero.
>> What you're recommending is to basically ignore the published workaround,
>> which, presumably, was arrived at by proper analysis of the issue both
>> by Freescale engineers and Synopsys engineers, and instead try some other
>> solution.
>>
>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>> say what effect disabling the audio clock will have: will it still allow
>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>> disabling the audio clock, that simple approach would have been documented
>> in Freescale's errata documents, rather than the rather more complex
>> method of zeroing the CTS/N values.
> I'm just following what the user guide says: the final step of
> configuration is enabling the audio clock. There is no reference
> neither in datasheet neither in user guide about this behavior
> but, as I said, I've never used AHB so I can't prove what is the
> best solution. Could it be platform specific?

And that's precisely why I am enabling it

>
>> I think there are two choices here:
>>
>> 1) get some definitive information about the IP and the errata for AHB,
>>    and determine whether the solution you propose would work.  (That's
>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>    Synopsys customer.)
> This is the right thing to do but if you can't test in the
> Freescale platform then we have not much else to do.
>
> Best regards,
> Jose Miguel Abreu
>
>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>    switching to their own implementation that behaves correctly for them.
>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>    I added which we're now discussing - they didn't have to do that, and
>>    regressing AHB audio just for I2S is not something we should ever
>>    consider doing.)
>>

The workaround looks AHB specific in any cases and does not seem to work
with I2S. The I2S variant should not used the same functions, at least
for enabling/disabling audio stream.

Regards,
Romain

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Romain Perier]

Hi,


Le 13/03/2017 ? 19:49, Jose Abreu a ?crit :
> Hi Russell,
>
>
> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>> Hi,
>>>
>>>
>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>>> and disabled along with the actual audio stream and not always on (that
>>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>>> while the audio clock is still running.
>>>>>>
>>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>>> dw_hdmi_audio_disable().
>>>>>>
>>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>
>>>>> Hi Romain, Russell, Jose,
>>>>>
>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>> marked as "reserved".
>>>>
>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>> access to Synopsis' databooks for these parts (I understand you have
>>>> to be a customer to have access to that.)
>>>>
>>> (Synopsis -> Synopsys :))
>>>
>>> Trying to catch up with the conversation:
>>>
>>> 1) In AHB audio mode the bits are always reserved.
>>> 2) I think we should enable/disable clock instead of just forcing
>>> N/CTS, though, I don't know what could be the implications for
>>> iMX platform. I remember at the time I tested this using I2S
>>> (I've never used AHB), and HDMI protocol analyzers were
>>> complaining about the N/CTS being forced to zero.
>> What you're recommending is to basically ignore the published workaround,
>> which, presumably, was arrived at by proper analysis of the issue both
>> by Freescale engineers and Synopsys engineers, and instead try some other
>> solution.
>>
>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>> say what effect disabling the audio clock will have: will it still allow
>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>> disabling the audio clock, that simple approach would have been documented
>> in Freescale's errata documents, rather than the rather more complex
>> method of zeroing the CTS/N values.
> I'm just following what the user guide says: the final step of
> configuration is enabling the audio clock. There is no reference
> neither in datasheet neither in user guide about this behavior
> but, as I said, I've never used AHB so I can't prove what is the
> best solution. Could it be platform specific?

And that's precisely why I am enabling it

>
>> I think there are two choices here:
>>
>> 1) get some definitive information about the IP and the errata for AHB,
>>    and determine whether the solution you propose would work.  (That's
>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>    Synopsys customer.)
> This is the right thing to do but if you can't test in the
> Freescale platform then we have not much else to do.
>
> Best regards,
> Jose Miguel Abreu
>
>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>    switching to their own implementation that behaves correctly for them.
>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>    I added which we're now discussing - they didn't have to do that, and
>>    regressing AHB audio just for I2S is not something we should ever
>>    consider doing.)
>>

The workaround looks AHB specific in any cases and does not seem to work
with I2S. The I2S variant should not used the same functions, at least
for enabling/disabling audio stream.

Regards,
Romain

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/14/2017 08:53 AM, Romain Perier wrote:
> Hi,
> 
> 
> Le 13/03/2017 à 19:49, Jose Abreu a écrit :
>> Hi Russell,
>>
>>
>> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>>>> and disabled along with the actual audio stream and not always on (that
>>>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>>>> while the audio clock is still running.
>>>>>>>
>>>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>>>> dw_hdmi_audio_disable().
>>>>>>>
>>>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>> Hi Romain, Russell, Jose,
>>>>>>
>>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>>> marked as "reserved".
>>>>>
>>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>>> access to Synopsis' databooks for these parts (I understand you have
>>>>> to be a customer to have access to that.)
>>>>>
>>>> (Synopsis -> Synopsys :))
>>>>
>>>> Trying to catch up with the conversation:
>>>>
>>>> 1) In AHB audio mode the bits are always reserved.
>>>> 2) I think we should enable/disable clock instead of just forcing
>>>> N/CTS, though, I don't know what could be the implications for
>>>> iMX platform. I remember at the time I tested this using I2S
>>>> (I've never used AHB), and HDMI protocol analyzers were
>>>> complaining about the N/CTS being forced to zero.
>>> What you're recommending is to basically ignore the published workaround,
>>> which, presumably, was arrived at by proper analysis of the issue both
>>> by Freescale engineers and Synopsys engineers, and instead try some other
>>> solution.
>>>
>>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>>> say what effect disabling the audio clock will have: will it still allow
>>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>>> disabling the audio clock, that simple approach would have been documented
>>> in Freescale's errata documents, rather than the rather more complex
>>> method of zeroing the CTS/N values.
>> I'm just following what the user guide says: the final step of
>> configuration is enabling the audio clock. There is no reference
>> neither in datasheet neither in user guide about this behavior
>> but, as I said, I've never used AHB so I can't prove what is the
>> best solution. Could it be platform specific?
> 
> And that's precisely why I am enabling it
> 
>>
>>> I think there are two choices here:
>>>
>>> 1) get some definitive information about the IP and the errata for AHB,
>>>    and determine whether the solution you propose would work.  (That's
>>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>>    Synopsys customer.)
>> This is the right thing to do but if you can't test in the
>> Freescale platform then we have not much else to do.
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>>    switching to their own implementation that behaves correctly for them.
>>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>>    I added which we're now discussing - they didn't have to do that, and
>>>    regressing AHB audio just for I2S is not something we should ever
>>>    consider doing.)
>>>
> 
> The workaround looks AHB specific in any cases and does not seem to work
> with I2S. The I2S variant should not used the same functions, at least
> for enabling/disabling audio stream.
> 
> Regards,
> Romain
> 
Hi All,

Jose, thanks for your clarification.

It seems reasonable to indeed add I2S/SPDIF specific functions, or at least add
parameters to change the behavior when the audio clock is generated by the
Synopsys IP or by an external audio block as in I2S and SPDIF.

Nevertheless, on the Amlogic platforms, we will need to add this AUTO cts calculation
feature and will certainly need this behavior fix.

Romain, feel free to add the correct I2S behavior, I'll be happy to test your patches.

Thanks,
Neil

Re: [PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/14/2017 08:53 AM, Romain Perier wrote:
> Hi,
> 
> 
> Le 13/03/2017 à 19:49, Jose Abreu a écrit :
>> Hi Russell,
>>
>>
>> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>>>> and disabled along with the actual audio stream and not always on (that
>>>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>>>> while the audio clock is still running.
>>>>>>>
>>>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>>>> dw_hdmi_audio_disable().
>>>>>>>
>>>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>> Hi Romain, Russell, Jose,
>>>>>>
>>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>>> marked as "reserved".
>>>>>
>>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>>> access to Synopsis' databooks for these parts (I understand you have
>>>>> to be a customer to have access to that.)
>>>>>
>>>> (Synopsis -> Synopsys :))
>>>>
>>>> Trying to catch up with the conversation:
>>>>
>>>> 1) In AHB audio mode the bits are always reserved.
>>>> 2) I think we should enable/disable clock instead of just forcing
>>>> N/CTS, though, I don't know what could be the implications for
>>>> iMX platform. I remember at the time I tested this using I2S
>>>> (I've never used AHB), and HDMI protocol analyzers were
>>>> complaining about the N/CTS being forced to zero.
>>> What you're recommending is to basically ignore the published workaround,
>>> which, presumably, was arrived at by proper analysis of the issue both
>>> by Freescale engineers and Synopsys engineers, and instead try some other
>>> solution.
>>>
>>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>>> say what effect disabling the audio clock will have: will it still allow
>>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>>> disabling the audio clock, that simple approach would have been documented
>>> in Freescale's errata documents, rather than the rather more complex
>>> method of zeroing the CTS/N values.
>> I'm just following what the user guide says: the final step of
>> configuration is enabling the audio clock. There is no reference
>> neither in datasheet neither in user guide about this behavior
>> but, as I said, I've never used AHB so I can't prove what is the
>> best solution. Could it be platform specific?
> 
> And that's precisely why I am enabling it
> 
>>
>>> I think there are two choices here:
>>>
>>> 1) get some definitive information about the IP and the errata for AHB,
>>>    and determine whether the solution you propose would work.  (That's
>>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>>    Synopsys customer.)
>> This is the right thing to do but if you can't test in the
>> Freescale platform then we have not much else to do.
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>>    switching to their own implementation that behaves correctly for them.
>>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>>    I added which we're now discussing - they didn't have to do that, and
>>>    regressing AHB audio just for I2S is not something we should ever
>>>    consider doing.)
>>>
> 
> The workaround looks AHB specific in any cases and does not seem to work
> with I2S. The I2S variant should not used the same functions, at least
> for enabling/disabling audio stream.
> 
> Regards,
> Romain
> 
Hi All,

Jose, thanks for your clarification.

It seems reasonable to indeed add I2S/SPDIF specific functions, or at least add
parameters to change the behavior when the audio clock is generated by the
Synopsys IP or by an external audio block as in I2S and SPDIF.

Nevertheless, on the Amlogic platforms, we will need to add this AUTO cts calculation
feature and will certainly need this behavior fix.

Romain, feel free to add the correct I2S behavior, I'll be happy to test your patches.

Thanks,
Neil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH] drm: dw_hdmi: Gate audio sampler clock from the enablement functions

[Neil Armstrong]

On 03/14/2017 08:53 AM, Romain Perier wrote:
> Hi,
> 
> 
> Le 13/03/2017 ? 19:49, Jose Abreu a ?crit :
>> Hi Russell,
>>
>>
>> On 13-03-2017 13:10, Russell King - ARM Linux wrote:
>>> On Mon, Mar 13, 2017 at 12:55:58PM +0000, Jose Abreu wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 13-03-2017 09:36, Russell King - ARM Linux wrote:
>>>>> On Mon, Mar 13, 2017 at 10:27:08AM +0100, Neil Armstrong wrote:
>>>>>> On 03/10/2017 10:35 AM, Romain Perier wrote:
>>>>>>> Currently, the audio sampler clock is enabled from dw_hdmi_setup() at
>>>>>>> step E. and is kept enabled for later use. This clock should be enabled
>>>>>>> and disabled along with the actual audio stream and not always on (that
>>>>>>> is bad for PM). Futhermore, this might cause sound glitches with some
>>>>>>> HDMI devices, as the CTS+N is forced to zero when the stream is disabled
>>>>>>> while the audio clock is still running.
>>>>>>>
>>>>>>> This commit adds a parameter to hdmi_audio_enable_clk() that controls
>>>>>>> when the audio sample clock must be enabled or disabled. Then, it moves
>>>>>>> the call to this function into dw_hdmi_audio_enable() and
>>>>>>> dw_hdmi_audio_disable().
>>>>>>>
>>>>>>> Signed-off-by: Romain Perier <romain.perier@collabora.com>
>>>>>>> ---
>>>>>>>  drivers/gpu/drm/bridge/dw-hdmi.c | 15 +++++++++------
>>>>>>>  1 file changed, 9 insertions(+), 6 deletions(-)
>>>>>>>
>>>>>> Hi Romain, Russell, Jose,
>>>>>>
>>>>>> This is a little out of scope, but I was wondering why the CTS calculation
>>>>>> was not left in AUTO mode in the dw-hdmi driver ?
>>>>> There is no indication in the iMX6 manuals that the iMX6 supports
>>>>> automatic CTS calculation.  Bits 7:4 of the AUD_CTS3 register are
>>>>> marked as "reserved".
>>>>>
>>>>> We're reliant on the information in the iMX6 manuals as we don't have
>>>>> access to Synopsis' databooks for these parts (I understand you have
>>>>> to be a customer to have access to that.)
>>>>>
>>>> (Synopsis -> Synopsys :))
>>>>
>>>> Trying to catch up with the conversation:
>>>>
>>>> 1) In AHB audio mode the bits are always reserved.
>>>> 2) I think we should enable/disable clock instead of just forcing
>>>> N/CTS, though, I don't know what could be the implications for
>>>> iMX platform. I remember at the time I tested this using I2S
>>>> (I've never used AHB), and HDMI protocol analyzers were
>>>> complaining about the N/CTS being forced to zero.
>>> What you're recommending is to basically ignore the published workaround,
>>> which, presumably, was arrived at by proper analysis of the issue both
>>> by Freescale engineers and Synopsys engineers, and instead try some other
>>> solution.
>>>
>>> I'm not sure that's a good idea.  Only those with knowledge of the IP can
>>> say what effect disabling the audio clock will have: will it still allow
>>> the FIFO to be loaded, as required by the errata?  If not, it's not a
>>> solution that AHB can use.  I would imagine that _if_ it was as simple as
>>> disabling the audio clock, that simple approach would have been documented
>>> in Freescale's errata documents, rather than the rather more complex
>>> method of zeroing the CTS/N values.
>> I'm just following what the user guide says: the final step of
>> configuration is enabling the audio clock. There is no reference
>> neither in datasheet neither in user guide about this behavior
>> but, as I said, I've never used AHB so I can't prove what is the
>> best solution. Could it be platform specific?
> 
> And that's precisely why I am enabling it
> 
>>
>>> I think there are two choices here:
>>>
>>> 1) get some definitive information about the IP and the errata for AHB,
>>>    and determine whether the solution you propose would work.  (That's
>>>    out of scope for me, I've no contacts with FSL/NXP and I'm not a
>>>    Synopsys customer.)
>> This is the right thing to do but if you can't test in the
>> Freescale platform then we have not much else to do.
>>
>> Best regards,
>> Jose Miguel Abreu
>>
>>> 2) the I2S audio support stops re-using the AHB audio support functions,
>>>    switching to their own implementation that behaves correctly for them.
>>>    (Remember, it was I2S's choice to re-use the AHB audio support functions
>>>    I added which we're now discussing - they didn't have to do that, and
>>>    regressing AHB audio just for I2S is not something we should ever
>>>    consider doing.)
>>>
> 
> The workaround looks AHB specific in any cases and does not seem to work
> with I2S. The I2S variant should not used the same functions, at least
> for enabling/disabling audio stream.
> 
> Regards,
> Romain
> 
Hi All,

Jose, thanks for your clarification.

It seems reasonable to indeed add I2S/SPDIF specific functions, or at least add
parameters to change the behavior when the audio clock is generated by the
Synopsys IP or by an external audio block as in I2S and SPDIF.

Nevertheless, on the Amlogic platforms, we will need to add this AUTO cts calculation
feature and will certainly need this behavior fix.

Romain, feel free to add the correct I2S behavior, I'll be happy to test your patches.

Thanks,
Neil