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

[Maxime Ripard]

On Thu, Jun 04, 2020 at 01:19:32PM +0800, Chen-Yu Tsai wrote:
> 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.

I just applied it.

Maxime

Attachment: signature.asc
Description: PGP signature