| Message ID | 20180227222701.9716-1-jernej.skrabec@siol.net |
|---|---|
| Headers | show |
| Series | Implement H3/H5 HDMI driver | expand |
Hi, On Tue, Feb 27, 2018 at 11:26:46PM +0100, Jernej Skrabec wrote: > Some NM PLLs doesn't work well when their output clock rate is set below > certain rate. > > Add support for that constrain. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > --- > drivers/clk/sunxi-ng/ccu_nm.c | 11 +++++++---- > drivers/clk/sunxi-ng/ccu_nm.h | 27 +++++++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu_nm.c b/drivers/clk/sunxi-ng/ccu_nm.c > index a16de092bf94..7fc743c78c1b 100644 > --- a/drivers/clk/sunxi-ng/ccu_nm.c > +++ b/drivers/clk/sunxi-ng/ccu_nm.c > @@ -20,7 +20,7 @@ struct _ccu_nm { > }; > > static void ccu_nm_find_best(unsigned long parent, unsigned long rate, > - struct _ccu_nm *nm) > + unsigned long min_rate, struct _ccu_nm *nm) > { > unsigned long best_rate = 0; > unsigned long best_n = 0, best_m = 0; > @@ -30,7 +30,7 @@ static void ccu_nm_find_best(unsigned long parent, unsigned long rate, > for (_m = nm->min_m; _m <= nm->max_m; _m++) { > unsigned long tmp_rate = parent * _n / _m; > > - if (tmp_rate > rate) > + if (tmp_rate > rate || tmp_rate < min_rate) > continue; > > if ((rate - tmp_rate) < (rate - best_rate)) { > @@ -117,6 +117,9 @@ static long ccu_nm_round_rate(struct clk_hw *hw, unsigned long rate, > if (nm->common.features & CCU_FEATURE_FIXED_POSTDIV) > rate *= nm->fixed_post_div; > > + if (rate < nm->min_rate) > + rate = nm->min_rate; > + I guess you can just return there. There's no point in trying to find the factors at this point, since the minimum should be a value that can be reached. > if (ccu_frac_helper_has_rate(&nm->common, &nm->frac, rate)) { > if (nm->common.features & CCU_FEATURE_FIXED_POSTDIV) > rate /= nm->fixed_post_div; > @@ -134,7 +137,7 @@ static long ccu_nm_round_rate(struct clk_hw *hw, unsigned long rate, > _nm.min_m = 1; > _nm.max_m = nm->m.max ?: 1 << nm->m.width; > > - ccu_nm_find_best(*parent_rate, rate, &_nm); > + ccu_nm_find_best(*parent_rate, rate, nm->min_rate, &_nm); Therefore, you don't need to change the prototype there either. Maxime
On Tue, Feb 27, 2018 at 11:26:51PM +0100, Jernej Skrabec wrote: > TCON checks for LVDS properties even if it doesn't support it. Add a > check to skip that part of the code if TCON doesn't support channel 0. > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> I have already sent a similar patch here: https://lists.freedesktop.org/archives/dri-devel/2018-February/166665.html Maxime
Hi Maxime, Dne torek, 27. februar 2018 ob 23:26:45 CET je Jernej Skrabec napisal(a): > This series implements H3/H5 HDMI driver. It was tested on OrangePi 2 (H3), > OrangePi Plus2e (H3) and OrangePi PC2 (H5) with many resolutions and it > works well. Bug, which prevented correct operation for some resolutions, > is also fixed. > > Code is based on linux-next, next-20180226 tag. Today I tried on this series on next-20180228, but resolution switching doesn't really work. The reason for this is use of clk_set_rate_exclusive() in sun4i_tcon1_mode_set(). If I revert it to ordinary clk_set_rate() it works ok. I investigated a bit and it seems that clocks stays at first set even if tcon (the owner of exclusive lock) request the new one. Example: [ 69.325732] TCON clk wanted: 148500000 [ 69.325740] TCON real clk: 69272728 [ 69.325770] HDMI clk wanted: 148500000 [ 69.325773] HDMI real clk: 138545455 [ 69.325815] HDMI PHY clk wanted: 148500000 [ 69.325819] HDMI PHY real clk: 138545454 Is there anything I can do to help debug this? Best regards, Jernej -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Maxime, Dne sreda, 28. februar 2018 ob 08:36:08 CET je Maxime Ripard napisal(a): > On Tue, Feb 27, 2018 at 11:26:51PM +0100, Jernej Skrabec wrote: > > TCON checks for LVDS properties even if it doesn't support it. Add a > > check to skip that part of the code if TCON doesn't support channel 0. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > I have already sent a similar patch here: > https://lists.freedesktop.org/archives/dri-devel/2018-February/166665.html Right. However, check last chunk in my patch. There is no need to call sun4i_rgb_init() if TCON doesn't support channel 0. It doesn't do anything, except producing warning. Will you add that this change to your patch and then I can remove this patch from next revision? BTW, your patch won't apply cleanly, since you didn't base it on latest code (every TCON variant has at least one entry now). Best regards, Jernej -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Dne sreda, 28. februar 2018 ob 08:34:40 CET je Maxime Ripard napisal(a): > Hi, > > On Tue, Feb 27, 2018 at 11:26:46PM +0100, Jernej Skrabec wrote: > > Some NM PLLs doesn't work well when their output clock rate is set below > > certain rate. > > > > Add support for that constrain. > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > --- > > > > drivers/clk/sunxi-ng/ccu_nm.c | 11 +++++++---- > > drivers/clk/sunxi-ng/ccu_nm.h | 27 +++++++++++++++++++++++++++ > > 2 files changed, 34 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/sunxi-ng/ccu_nm.c b/drivers/clk/sunxi-ng/ccu_nm.c > > index a16de092bf94..7fc743c78c1b 100644 > > --- a/drivers/clk/sunxi-ng/ccu_nm.c > > +++ b/drivers/clk/sunxi-ng/ccu_nm.c > > @@ -20,7 +20,7 @@ struct _ccu_nm { > > > > }; > > > > static void ccu_nm_find_best(unsigned long parent, unsigned long rate, > > > > - struct _ccu_nm *nm) > > + unsigned long min_rate, struct _ccu_nm *nm) > > > > { > > > > unsigned long best_rate = 0; > > unsigned long best_n = 0, best_m = 0; > > > > @@ -30,7 +30,7 @@ static void ccu_nm_find_best(unsigned long parent, > > unsigned long rate,> > > for (_m = nm->min_m; _m <= nm->max_m; _m++) { > > > > unsigned long tmp_rate = parent * _n / _m; > > > > - if (tmp_rate > rate) > > + if (tmp_rate > rate || tmp_rate < min_rate) > > > > continue; > > > > if ((rate - tmp_rate) < (rate - best_rate)) { > > > > @@ -117,6 +117,9 @@ static long ccu_nm_round_rate(struct clk_hw *hw, > > unsigned long rate,> > > if (nm->common.features & CCU_FEATURE_FIXED_POSTDIV) > > > > rate *= nm->fixed_post_div; > > > > + if (rate < nm->min_rate) > > + rate = nm->min_rate; > > + > > I guess you can just return there. There's no point in trying to find > the factors at this point, since the minimum should be a value that > can be reached. > Right, I already tested it and it works. Best regards, Jernej > > if (ccu_frac_helper_has_rate(&nm->common, &nm->frac, rate)) { > > > > if (nm->common.features & CCU_FEATURE_FIXED_POSTDIV) > > > > rate /= nm->fixed_post_div; > > > > @@ -134,7 +137,7 @@ static long ccu_nm_round_rate(struct clk_hw *hw, > > unsigned long rate,> > > _nm.min_m = 1; > > _nm.max_m = nm->m.max ?: 1 << nm->m.width; > > > > - ccu_nm_find_best(*parent_rate, rate, &_nm); > > + ccu_nm_find_best(*parent_rate, rate, nm->min_rate, &_nm); > > Therefore, you don't need to change the prototype there either. > > Maxime > > -- > Maxime Ripard, Bootlin (formerly Free Electrons) > Embedded Linux and Kernel engineering > https://bootlin.com -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Feb 28, 2018 at 10:23:35PM +0100, Jernej Škrabec wrote: > Dne torek, 27. februar 2018 ob 23:26:45 CET je Jernej Skrabec napisal(a): > > This series implements H3/H5 HDMI driver. It was tested on OrangePi 2 (H3), > > OrangePi Plus2e (H3) and OrangePi PC2 (H5) with many resolutions and it > > works well. Bug, which prevented correct operation for some resolutions, > > is also fixed. > > > > Code is based on linux-next, next-20180226 tag. > > Today I tried on this series on next-20180228, but resolution switching > doesn't really work. The reason for this is use of clk_set_rate_exclusive() in > sun4i_tcon1_mode_set(). If I revert it to ordinary clk_set_rate() it works ok. > I investigated a bit and it seems that clocks stays at first set even if tcon > (the owner of exclusive lock) request the new one. Example: > > [ 69.325732] TCON clk wanted: 148500000 > [ 69.325740] TCON real clk: 69272728 > [ 69.325770] HDMI clk wanted: 148500000 > [ 69.325773] HDMI real clk: 138545455 > [ 69.325815] HDMI PHY clk wanted: 148500000 > [ 69.325819] HDMI PHY real clk: 138545454 > > Is there anything I can do to help debug this? I don't recall if disable hook is called during a mode set, but if it is, you can add a call to clk_rate_exclusive_put. Maxime
Hi, On Wed, Feb 28, 2018 at 10:43:30PM +0100, Jernej Škrabec wrote: > Dne sreda, 28. februar 2018 ob 08:36:08 CET je Maxime Ripard napisal(a): > > On Tue, Feb 27, 2018 at 11:26:51PM +0100, Jernej Skrabec wrote: > > > TCON checks for LVDS properties even if it doesn't support it. Add a > > > check to skip that part of the code if TCON doesn't support channel 0. > > > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net> > > > > I have already sent a similar patch here: > > https://lists.freedesktop.org/archives/dri-devel/2018-February/166665.html > > Right. However, check last chunk in my patch. There is no need to call > sun4i_rgb_init() if TCON doesn't support channel 0. It doesn't do anything, > except producing warning. Will you add that this change to your patch and then > I can remove this patch from next revision? They are orthogonal to me though. Mine fixes the spurious LVDS error messages that you were mentionning in your commit log. Your point here is that we shouldn't need to even register the LVDS and RGB output when there's no channel 0. This is obviously true, but it should be in a separate patch. > BTW, your patch won't apply cleanly, since you didn't base it on latest code > (every TCON variant has at least one entry now). I'll fix it when applying. Maxime