Re: [PATCH] drm/sun4i: hdmi ddc clk: Fix size of m divider

[Chen-Yu Tsai]

On Wed, Apr 22, 2020 at 5:23 PM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> Hi,
>
> On Wed, Apr 15, 2020 at 07:52:28PM +0200, Jernej Åkrabec wrote:
> > Dne sreda, 15. april 2020 ob 12:42:14 CEST je Maxime Ripard napisal(a):
> > > On Mon, Apr 13, 2020 at 06:09:08PM +0200, Jernej Åkrabec wrote:
> > > > Dne ponedeljek, 13. april 2020 ob 16:12:39 CEST je Chen-Yu Tsai
> > napisal(a):
> > > > > On Mon, Apr 13, 2020 at 6:11 PM Chen-Yu Tsai <wens@xxxxxxxx> wrote:
> > > > > > On Mon, Apr 13, 2020 at 5:55 PM Jernej Skrabec
> > > > > > <jernej.skrabec@xxxxxxxx>
> > > >
> > > > wrote:
> > > > > > > m divider in DDC clock register is 4 bits wide. Fix that.
> > > > > > >
> > > > > > > Fixes: 9c5681011a0c ("drm/sun4i: Add HDMI support")
> > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@xxxxxxxx>
> > > > > >
> > > > > > Reviewed-by: Chen-Yu Tsai <wens@xxxxxxxx>
> > > > >
> > > > > Cc stable?
> > > >
> > > > I don't think it's necessary:
> > > > 1. It doesn't change much (anything?) for me when reading EDID. I don't
> > > > think it's super important to have precise DDC clock in order to properly
> > > > read EDID. 2. No matter if it has "Cc stable" tag or not, it will be
> > > > eventually picked for stable due to fixes tag.
> > > >
> > > > This was only small observation when I was researching EDID readout issue
> > > > on A20 board, but sadly, I wasn't able to figure out why reading it
> > > > sometimes fails. I noticed similar issue on SoCs with DE2 (most
> > > > prominently on OrangePi PC2 - H5), but there was easy workaround - I just
> > > > disabled video driver in U- Boot. However, if A20 display driver gets
> > > > disabled in U-Boot, it totally breaks video output on my TV when Linux
> > > > boots (no output). I guess there is more fundamental problem with clocks
> > > > than just field size. I think we should add more constraints in clock
> > > > driver, like preset some clock parents and not allow to change parents
> > > > when setting rate, but carefully, so simplefb doesn't break. Such
> > > > constraints should also solve problems with dual head setups.
> > > I disagree here. Doing all sorts of special case just doesn't scale,
> > > and we'll never have the special cases sorted out on all the boards
> > > (and it's a nightmare to maintain).
> > >
> > > Especially since it's basically putting a blanket over the actual
> > > issue and looking the other way. If there's something wrong with how
> > > we deal with (re)parenting, we should fix that. It impacts more than
> > > just DRM, and all the SoCs.
> >
> > I agree with you that automatic solution would be best, but I just don't see
> > it how it would be done.
>
> > Dual head display pipeline is pretty complex for clock driver to get it right
> > on it's own. There are different possible setups and some of them are hot
> > pluggable, like HDMI.
>
> Do you have an actual scenario that is broken right now?
>
> > And there are also SoC specific quirks, like A64, where for some reason, MIPI
> > DPHY and HDMI PHY share same clock parent - PLL_VIDEO0. Technically, MIPI DPHY
> > can be clocked from PLL_PERIPH0 (fixed to 600 MHz), but that's not really
> > helpful. I'm not even sure if there is any good solution to this - certainly
> > HDMI and MIPI can't claim exclusivity and somehow best common rate must be
> > found for PLL_VIDEO0, if that's even possible.
>
> IIRC the DSI DPHY needs a clock running at 297MHz, which is pretty much what the
> HDMI PHY should need too (or 148.5, but that's pretty easy to generate from
> 297). So which problem do we have there?
>
> > I was sure that HDMI PHY on A64 can be clocked from PLL_VIDEO1, which would
> > solve main issue, but to date, I didn't find any way to do that.
> >
> > That's pretty off topic, so I hope original patch can be merged as-is.
>
> It does, sorry
>
> Acked-by: Maxime Ripard <maxime@xxxxxxxxxx>

Looks like this hasn't landed yet.

ChenYu