From: Chris Morgan <macromorgan@hotmail.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Chris Morgan <macroalpha82@gmail.com>, abel.vesa@nxp.com, festevam@gmail.com, heiko@sntech.de, kernel@pengutronix.de, lee.jones@linaro.org, lenb@kernel.org, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, mturquette@baylibre.com, rafael.j.wysocki@intel.com, rjw@rjwysocki.net, s.hauer@pengutronix.de, sboyd@kernel.org, shawnguo@kernel.org, zhangqing@rock-chips.com Subject: Re: [PATCH v4 1/4] clk: fractional-divider: Export approximation algorithm to the CCF users Date: Tue, 7 Sep 2021 21:17:47 -0500 [thread overview] Message-ID: <SN6PR06MB534203466EA3D4E22858479FA5D49@SN6PR06MB5342.namprd06.prod.outlook.com> (raw) In-Reply-To: <YTeqEilet1p4vTAU@smile.fi.intel.com> On Tue, Sep 07, 2021 at 09:06:10PM +0300, Andy Shevchenko wrote: > On Tue, Sep 07, 2021 at 08:54:04PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote: > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > Unfortunately, I can confirm this breaks the DSI panel on the Rockchip > > > PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When > > > I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation > > > algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider: > > > Hide clk_fractional_divider_ops from wide audience" the panel begins > > > working again as expected on the master branch. > > > > > > It looks like an assumption is made in the vop_crtc_mode_fixup() > > > function in the rockchip_drm_vop.c that gets broken with this change. > > > Specifically, the function says in the comments "When DRM gives us a > > > mode, we should add 999 Hz to it.". I believe this is no longer true > > > after this clk change, and when I remove the + 999 from the function > > > the DSI panel works again. Note that I do not know the implications > > > of removing this 999 aside from that it fixes the DSI panel on my > > > PX30 after this change, so I don't know if it's a positive change > > > or not. > > > > > > Thank you. > > > > Thank you for the report! > > > > I'll check this. Perhaps Heiko can help with testing as well on his side. > > On the first glance the mentioned patch may not be the culprit because it does > not change the functional behaviour (if I'm not mistaken). What really changes > it is the additional flag that removes the left-shift of the rate in the > calculations. I noticed the behavior on the 5.14 kernel was to set the numerator at an ungodly 7649082492112076800 and the denominator at 1 (no, that's not a typo). I think it tried to write 65535 to the register though, but it would go through this a few times and eventually settle on 1:1 as the fractional ratio (which I assume is all good, because that would work). Contrast this to the 5.15 behavior where it would try to set the ratio to 17001:17000, which would cause the DSI screen to fail to initalize. After tracing through the code I figured out that the VOP was trying to add 999 to the clock and set it at 17000999. 17000000/17000999 gives us 0, and subtracting 1 from that gives us a -1. The fls_long function would then return 64, and if we subtract 16 (the value of fd->mwidth for my board) it would tell us to shift the 17000999 48 bits to the left, which matches the ungodly large number. With the changes in 5.15 if I remove the + 999 from the VOP driver the clock then gets set at 17000000, since the parent is at 17000000 that gives us a 1:1 where everything works and everything is fine. Long story short I think this is a bug that's existed all along, and this change simply exposed it in a manner where it stopped working despite the bug being present. Unfortunately I neither know enough about the hardware to be confident in this fix beyond my specific board, nor do I have enough hardware to test it on anything except a Rockchip rk3326 with a DSI panel. > > To me sounds like you found a proper solution to the issue and that +999 is > a hack against the (blindly?) copied part of the algorithm used in fractional > divider. Have you read the top comment in clk-fractional-divider.c? It should > explain how it works after my series. No, but I probably should read the docs more. I just stumbled on this series doing a bisect when the DSI panel stopped working. > > In any case I'm not going to come to any conclusions right now and also want > to hear from people who have better understanding of this hardware. Yeah, I want to see what Heiko says after some more research, or anyone who has more familiarity with clocks/DRM than I do or who has more hardware to test on than I do. I intended to send a message informing you that "hey, this breaks upstream", but I think it turns out it's more a matter of "hey, this makes a broken upstream break instead of limp along". Thank you. > > -- > With Best Regards, > Andy Shevchenko > > _______________________________________________ Linux-rockchip mailing list Linux-rockchip@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-rockchip
WARNING: multiple messages have this Message-ID (diff)
From: Chris Morgan <macromorgan@hotmail.com> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Cc: Chris Morgan <macroalpha82@gmail.com>, abel.vesa@nxp.com, festevam@gmail.com, heiko@sntech.de, kernel@pengutronix.de, lee.jones@linaro.org, lenb@kernel.org, linux-acpi@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, linux-imx@nxp.com, linux-kernel@vger.kernel.org, linux-rockchip@lists.infradead.org, mturquette@baylibre.com, rafael.j.wysocki@intel.com, rjw@rjwysocki.net, s.hauer@pengutronix.de, sboyd@kernel.org, shawnguo@kernel.org, zhangqing@rock-chips.com Subject: Re: [PATCH v4 1/4] clk: fractional-divider: Export approximation algorithm to the CCF users Date: Tue, 7 Sep 2021 21:17:47 -0500 [thread overview] Message-ID: <SN6PR06MB534203466EA3D4E22858479FA5D49@SN6PR06MB5342.namprd06.prod.outlook.com> (raw) In-Reply-To: <YTeqEilet1p4vTAU@smile.fi.intel.com> On Tue, Sep 07, 2021 at 09:06:10PM +0300, Andy Shevchenko wrote: > On Tue, Sep 07, 2021 at 08:54:04PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 07, 2021 at 10:44:00AM -0500, Chris Morgan wrote: > > > From: Chris Morgan <macromorgan@hotmail.com> > > > > > > Unfortunately, I can confirm this breaks the DSI panel on the Rockchip > > > PX30 (and possibly other SoCs). Tested on my Odroid Go Advance. When > > > I revert 4e7cf74fa3b2 "clk: fractional-divider: Export approximation > > > algorithm to the CCF users" and 928f9e268611 "clk: fractional-divider: > > > Hide clk_fractional_divider_ops from wide audience" the panel begins > > > working again as expected on the master branch. > > > > > > It looks like an assumption is made in the vop_crtc_mode_fixup() > > > function in the rockchip_drm_vop.c that gets broken with this change. > > > Specifically, the function says in the comments "When DRM gives us a > > > mode, we should add 999 Hz to it.". I believe this is no longer true > > > after this clk change, and when I remove the + 999 from the function > > > the DSI panel works again. Note that I do not know the implications > > > of removing this 999 aside from that it fixes the DSI panel on my > > > PX30 after this change, so I don't know if it's a positive change > > > or not. > > > > > > Thank you. > > > > Thank you for the report! > > > > I'll check this. Perhaps Heiko can help with testing as well on his side. > > On the first glance the mentioned patch may not be the culprit because it does > not change the functional behaviour (if I'm not mistaken). What really changes > it is the additional flag that removes the left-shift of the rate in the > calculations. I noticed the behavior on the 5.14 kernel was to set the numerator at an ungodly 7649082492112076800 and the denominator at 1 (no, that's not a typo). I think it tried to write 65535 to the register though, but it would go through this a few times and eventually settle on 1:1 as the fractional ratio (which I assume is all good, because that would work). Contrast this to the 5.15 behavior where it would try to set the ratio to 17001:17000, which would cause the DSI screen to fail to initalize. After tracing through the code I figured out that the VOP was trying to add 999 to the clock and set it at 17000999. 17000000/17000999 gives us 0, and subtracting 1 from that gives us a -1. The fls_long function would then return 64, and if we subtract 16 (the value of fd->mwidth for my board) it would tell us to shift the 17000999 48 bits to the left, which matches the ungodly large number. With the changes in 5.15 if I remove the + 999 from the VOP driver the clock then gets set at 17000000, since the parent is at 17000000 that gives us a 1:1 where everything works and everything is fine. Long story short I think this is a bug that's existed all along, and this change simply exposed it in a manner where it stopped working despite the bug being present. Unfortunately I neither know enough about the hardware to be confident in this fix beyond my specific board, nor do I have enough hardware to test it on anything except a Rockchip rk3326 with a DSI panel. > > To me sounds like you found a proper solution to the issue and that +999 is > a hack against the (blindly?) copied part of the algorithm used in fractional > divider. Have you read the top comment in clk-fractional-divider.c? It should > explain how it works after my series. No, but I probably should read the docs more. I just stumbled on this series doing a bisect when the DSI panel stopped working. > > In any case I'm not going to come to any conclusions right now and also want > to hear from people who have better understanding of this hardware. Yeah, I want to see what Heiko says after some more research, or anyone who has more familiarity with clocks/DRM than I do or who has more hardware to test on than I do. I intended to send a message informing you that "hey, this breaks upstream", but I think it turns out it's more a matter of "hey, this makes a broken upstream break instead of limp along". Thank you. > > -- > With Best Regards, > Andy Shevchenko > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-09-08 2:18 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-08-12 17:00 [PATCH v4 1/4] clk: fractional-divider: Export approximation algorithm to the CCF users Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 17:00 ` [PATCH v4 2/4] clk: fractional-divider: Hide clk_fractional_divider_ops from wide audience Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 17:00 ` [PATCH v4 3/4] clk: fractional-divider: Introduce POWER_OF_TWO_PS flag Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 17:00 ` [PATCH v4 4/4] clk: fractional-divider: Document the arithmetics used behind the code Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 17:00 ` Andy Shevchenko 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 19:56 ` Stephen Boyd 2021-08-12 19:56 ` [PATCH v4 1/4] clk: fractional-divider: Export approximation algorithm to the CCF users Stephen Boyd 2021-08-12 19:56 ` Stephen Boyd 2021-08-13 9:43 ` Andy Shevchenko 2021-08-13 9:43 ` Andy Shevchenko 2021-08-13 9:43 ` Andy Shevchenko 2021-08-17 12:45 ` Andy Shevchenko 2021-08-17 12:45 ` Andy Shevchenko 2021-08-17 12:45 ` Andy Shevchenko 2021-09-07 15:44 ` Chris Morgan 2021-09-07 15:44 ` Chris Morgan 2021-09-07 15:44 ` Chris Morgan 2021-09-07 17:54 ` Andy Shevchenko 2021-09-07 17:54 ` Andy Shevchenko 2021-09-07 17:54 ` Andy Shevchenko 2021-09-07 18:06 ` Andy Shevchenko 2021-09-07 18:06 ` Andy Shevchenko 2021-09-07 18:06 ` Andy Shevchenko 2021-09-08 2:17 ` Chris Morgan [this message] 2021-09-08 2:17 ` Chris Morgan 2021-09-08 10:52 ` Andy Shevchenko 2021-09-08 10:52 ` Andy Shevchenko 2021-09-08 10:52 ` Andy Shevchenko
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=SN6PR06MB534203466EA3D4E22858479FA5D49@SN6PR06MB5342.namprd06.prod.outlook.com \ --to=macromorgan@hotmail.com \ --cc=abel.vesa@nxp.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=festevam@gmail.com \ --cc=heiko@sntech.de \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=lenb@kernel.org \ --cc=linux-acpi@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-rockchip@lists.infradead.org \ --cc=macroalpha82@gmail.com \ --cc=mturquette@baylibre.com \ --cc=rafael.j.wysocki@intel.com \ --cc=rjw@rjwysocki.net \ --cc=s.hauer@pengutronix.de \ --cc=sboyd@kernel.org \ --cc=shawnguo@kernel.org \ --cc=zhangqing@rock-chips.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.