| Message ID | 20210829203027.276143-2-marijn.suijten@somainline.org |
|---|---|
| State | Accepted |
| Commit | f1db21c315f4b4f8c3fbea56aac500673132d317 |
| Headers | show |
| Series | [1/3] arm: dts: qcom: apq8064: Use 27MHz PXO clock as DSI PLL reference | expand |
On Sun, 29 Aug 2021 at 23:30, Marijn Suijten <marijn.suijten@somainline.org> wrote: > > The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > clock and should hence use PXO, not CXO which runs at 19.2MHz. > > Note that none of the DSI PHY/PLL drivers currently use this "ref" > clock; they all rely on (sometimes inexistant) global clock names and > usually function normally without a parent clock. This discrepancy will > be corrected in a future patch, for which this change needs to be in > place first. > > Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Checked the downstream driver, it always uses 27 MHz clock in calculations. > --- > arch/arm/boot/dts/qcom-apq8064.dtsi | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi > index 2687c4e890ba..77659b783759 100644 > --- a/arch/arm/boot/dts/qcom-apq8064.dtsi > +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi > @@ -198,7 +198,7 @@ cxo_board: cxo_board { > clock-frequency = <19200000>; > }; > > - pxo_board { > + pxo_board: pxo_board { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <27000000>; > @@ -1306,7 +1306,7 @@ dsi0_phy: dsi-phy@4700200 { > reg-names = "dsi_pll", "dsi_phy", "dsi_phy_regulator"; > clock-names = "iface_clk", "ref"; > clocks = <&mmcc DSI_M_AHB_CLK>, > - <&cxo_board>; > + <&pxo_board>; > }; > > > -- > 2.33.0 >
On Mon, 30 Aug 2021 at 11:28, Marijn Suijten <marijn.suijten@somainline.org> wrote: > > Hi Dmitry, > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > <marijn.suijten@somainline.org> wrote: > >> > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > >> > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > >> clock; they all rely on (sometimes inexistant) global clock names and > >> usually function normally without a parent clock. This discrepancy will > >> be corrected in a future patch, for which this change needs to be in > >> place first. > >> > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > Given our concerns for msm8974 not updating DT in parallel with the > kernel (hence the need for a global-name fallback because "ref" is > missing from the DT), should we worry about the same for apq8064? That > is, is there a chance that the kernel but not the firmware is upgraded > leading to the wrong parent clock being used? The msm8960 variant of > the 28nm PLL driver uses parent_rate in a few places and might read > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. Checked the code. It uses .parent_names = "pxo", so changing ref clock should not matter. We'd need to fix ref clocks and after that we can switch parent names to fw_name. > > - Marijn -- With best wishes Dmitry
On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote: > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten > <marijn.suijten@somainline.org> wrote: > > > > Hi Dmitry, > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > > <marijn.suijten@somainline.org> wrote: > > >> > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > > >> > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > > >> clock; they all rely on (sometimes inexistant) global clock names and > > >> usually function normally without a parent clock. This discrepancy will > > >> be corrected in a future patch, for which this change needs to be in > > >> place first. > > >> > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > > > > Given our concerns for msm8974 not updating DT in parallel with the > > kernel (hence the need for a global-name fallback because "ref" is > > missing from the DT), should we worry about the same for apq8064? That > > is, is there a chance that the kernel but not the firmware is upgraded > > leading to the wrong parent clock being used? The msm8960 variant of > > the 28nm PLL driver uses parent_rate in a few places and might read > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. > > Checked the code. It uses .parent_names = "pxo", so changing ref > clock should not matter. We'd need to fix ref clocks and after that we > can switch parent names to fw_name. Correct, hence why this patch is ordered before the switch to .fw_name. These patches can't go in the same series if apq8064 doesn't update its firmware in parallel with the kernel just like msm8974. Do you know if this is the case? If so, how much time do you think should be between the DT fix (this patch) and migrating the drivers? - Marijn
On Mon, 30 Aug 2021 at 17:14, Marijn Suijten <marijn.suijten@somainline.org> wrote: > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote: > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten > > <marijn.suijten@somainline.org> wrote: > > > > > > Hi Dmitry, > > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > > > <marijn.suijten@somainline.org> wrote: > > > >> > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > > > >> > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > > > >> clock; they all rely on (sometimes inexistant) global clock names and > > > >> usually function normally without a parent clock. This discrepancy will > > > >> be corrected in a future patch, for which this change needs to be in > > > >> place first. > > > >> > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > > > > > > > Given our concerns for msm8974 not updating DT in parallel with the > > > kernel (hence the need for a global-name fallback because "ref" is > > > missing from the DT), should we worry about the same for apq8064? That > > > is, is there a chance that the kernel but not the firmware is upgraded > > > leading to the wrong parent clock being used? The msm8960 variant of > > > the 28nm PLL driver uses parent_rate in a few places and might read > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. > > > > Checked the code. It uses .parent_names = "pxo", so changing ref > > clock should not matter. We'd need to fix ref clocks and after that we > > can switch parent names to fw_name. > > Correct, hence why this patch is ordered before the switch to .fw_name. > These patches can't go in the same series if apq8064 doesn't update its > firmware in parallel with the kernel just like msm8974. Do you know if > this is the case? If so, how much time do you think should be between > the DT fix (this patch) and migrating the drivers? You can have parent_data with .fw_name and .name in it. .name will be used as a fallback if .fw_name doesn't match.
On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote: > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten > <marijn.suijten@somainline.org> wrote: > > > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote: > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten > > > <marijn.suijten@somainline.org> wrote: > > > > > > > > Hi Dmitry, > > > > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > > > > <marijn.suijten@somainline.org> wrote: > > > > >> > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > > > > >> > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > > > > >> clock; they all rely on (sometimes inexistant) global clock names and > > > > >> usually function normally without a parent clock. This discrepancy will > > > > >> be corrected in a future patch, for which this change needs to be in > > > > >> place first. > > > > >> > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > > > > > > > > > > Given our concerns for msm8974 not updating DT in parallel with the > > > > kernel (hence the need for a global-name fallback because "ref" is > > > > missing from the DT), should we worry about the same for apq8064? That > > > > is, is there a chance that the kernel but not the firmware is upgraded > > > > leading to the wrong parent clock being used? The msm8960 variant of > > > > the 28nm PLL driver uses parent_rate in a few places and might read > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. > > > > > > Checked the code. It uses .parent_names = "pxo", so changing ref > > > clock should not matter. We'd need to fix ref clocks and after that we > > > can switch parent names to fw_name. > > > > Correct, hence why this patch is ordered before the switch to .fw_name. > > These patches can't go in the same series if apq8064 doesn't update its > > firmware in parallel with the kernel just like msm8974. Do you know if > > this is the case? If so, how much time do you think should be between > > the DT fix (this patch) and migrating the drivers? > > You can have parent_data with .fw_name and .name in it. .name will be > used as a fallback if .fw_name doesn't match. The problem is that it will always find the "ref" clock which references &cxo_board until the DT is updated with this patch to use &pxo_board instead. Question is, will the kernel and DT usually/always be updated in parallel? - Marijn
On Mon 30 Aug 09:25 CDT 2021, Marijn Suijten wrote: > On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote: > > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten > > <marijn.suijten@somainline.org> wrote: > > > > > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote: > > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten > > > > <marijn.suijten@somainline.org> wrote: > > > > > > > > > > Hi Dmitry, > > > > > > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > > > > > <marijn.suijten@somainline.org> wrote: > > > > > >> > > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > > > > > >> > > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > > > > > >> clock; they all rely on (sometimes inexistant) global clock names and > > > > > >> usually function normally without a parent clock. This discrepancy will > > > > > >> be corrected in a future patch, for which this change needs to be in > > > > > >> place first. > > > > > >> > > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > > > > > > > > > > > > > Given our concerns for msm8974 not updating DT in parallel with the > > > > > kernel (hence the need for a global-name fallback because "ref" is > > > > > missing from the DT), should we worry about the same for apq8064? That > > > > > is, is there a chance that the kernel but not the firmware is upgraded > > > > > leading to the wrong parent clock being used? The msm8960 variant of > > > > > the 28nm PLL driver uses parent_rate in a few places and might read > > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. > > > > > > > > Checked the code. It uses .parent_names = "pxo", so changing ref > > > > clock should not matter. We'd need to fix ref clocks and after that we > > > > can switch parent names to fw_name. > > > > > > Correct, hence why this patch is ordered before the switch to .fw_name. > > > These patches can't go in the same series if apq8064 doesn't update its > > > firmware in parallel with the kernel just like msm8974. Do you know if > > > this is the case? If so, how much time do you think should be between > > > the DT fix (this patch) and migrating the drivers? > > > > You can have parent_data with .fw_name and .name in it. .name will be > > used as a fallback if .fw_name doesn't match. > > The problem is that it will always find the "ref" clock which references > &cxo_board until the DT is updated with this patch to use &pxo_board > instead. Question is, will the kernel and DT usually/always be updated > in parallel? > Afaik these devices all boots off a boot.img, which means that it's unlikely that a new kernel is installed on a device with an older DT. None of them would boot mainline off the DT that shipped with the original product. As such, if I pick this patch up as a fix for 5.15 you can respin the other two patches and they can land in 5.16 and I would be surprised if anyone will run into any issues with it. I.e. I've applied this patch. Regards, Bjorn
On Mon 30 Aug 10:37 CDT 2021, Marijn Suijten wrote: > On Mon, Aug 30, 2021 at 10:17:37AM -0500, Bjorn Andersson wrote: > > On Mon 30 Aug 09:25 CDT 2021, Marijn Suijten wrote: > > > > > On Mon, Aug 30, 2021 at 05:18:37PM +0300, Dmitry Baryshkov wrote: > > > > On Mon, 30 Aug 2021 at 17:14, Marijn Suijten > > > > <marijn.suijten@somainline.org> wrote: > > > > > > > > > > On Mon, Aug 30, 2021 at 04:24:58PM +0300, Dmitry Baryshkov wrote: > > > > > > On Mon, 30 Aug 2021 at 11:28, Marijn Suijten > > > > > > <marijn.suijten@somainline.org> wrote: > > > > > > > > > > > > > > Hi Dmitry, > > > > > > > > > > > > > > On 8/30/21 3:18 AM, Dmitry Baryshkov wrote: > > > > > > > > On Sun, 29 Aug 2021 at 23:30, Marijn Suijten > > > > > > > > <marijn.suijten@somainline.org> wrote: > > > > > > > >> > > > > > > > >> The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference > > > > > > > >> clock and should hence use PXO, not CXO which runs at 19.2MHz. > > > > > > > >> > > > > > > > >> Note that none of the DSI PHY/PLL drivers currently use this "ref" > > > > > > > >> clock; they all rely on (sometimes inexistant) global clock names and > > > > > > > >> usually function normally without a parent clock. This discrepancy will > > > > > > > >> be corrected in a future patch, for which this change needs to be in > > > > > > > >> place first. > > > > > > > >> > > > > > > > >> Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> > > > > > > > > > > > > > > > > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > > > > > > > > > > > > > > Checked the downstream driver, it always uses 27 MHz clock in calculations. > > > > > > > > > > > > > > > > > > > > > Given our concerns for msm8974 not updating DT in parallel with the > > > > > > > kernel (hence the need for a global-name fallback because "ref" is > > > > > > > missing from the DT), should we worry about the same for apq8064? That > > > > > > > is, is there a chance that the kernel but not the firmware is upgraded > > > > > > > leading to the wrong parent clock being used? The msm8960 variant of > > > > > > > the 28nm PLL driver uses parent_rate in a few places and might read > > > > > > > cxo's 19.2MHz erroneously instead of using pxo's 27MHz. > > > > > > > > > > > > Checked the code. It uses .parent_names = "pxo", so changing ref > > > > > > clock should not matter. We'd need to fix ref clocks and after that we > > > > > > can switch parent names to fw_name. > > > > > > > > > > Correct, hence why this patch is ordered before the switch to .fw_name. > > > > > These patches can't go in the same series if apq8064 doesn't update its > > > > > firmware in parallel with the kernel just like msm8974. Do you know if > > > > > this is the case? If so, how much time do you think should be between > > > > > the DT fix (this patch) and migrating the drivers? > > > > > > > > You can have parent_data with .fw_name and .name in it. .name will be > > > > used as a fallback if .fw_name doesn't match. > > > > > > The problem is that it will always find the "ref" clock which references > > > &cxo_board until the DT is updated with this patch to use &pxo_board > > > instead. Question is, will the kernel and DT usually/always be updated > > > in parallel? > > > > > > > Afaik these devices all boots off a boot.img, which means that it's > > unlikely that a new kernel is installed on a device with an older DT. > > None of them would boot mainline off the DT that shipped with the > > original product. > > That was my understanding as well, DT overlays are a "new thing" afaik > and most devices (at least all Sony's that I'm working with) use an > appended DTB that's always in-sync with the kernel image. > I think that with the introduction of DT overlays the system becomes more flexible and as such more susceptible for bugs caused by unexpected DT versions. I think in practice the real issues comes when the DTB is delivered separately (i.e. not by boot.img) or inbetween two kernel releases where the Qualcomm tree might not be in sync with the driver tree. > > As such, if I pick this patch up as a fix for 5.15 you can respin the > > other two patches and they can land in 5.16 and I would be surprised if > > anyone will run into any issues with it. > > > > I.e. I've applied this patch. > > Sounds good, I'll leave this patch out from v2. Should it have a Fixes: > tag to get backported too? > Sounds good, I added to this: Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY") > Since most review seems to be in I'll respin v2 shortly with the > addition of the "ref" clock to msm8974, that should probably get the > same treatment (added to 5.15 fixes) then we can land this patchset in > 5.16 (maybe without .name= fallback if Dmitry is okay with that). > Sounds good. Regards, Bjorn
On 2021-08-30 10:58:09, Bjorn Andersson wrote: [...] > > > > > > Afaik these devices all boots off a boot.img, which means that it's > > > unlikely that a new kernel is installed on a device with an older DT. > > > None of them would boot mainline off the DT that shipped with the > > > original product. > > > > That was my understanding as well, DT overlays are a "new thing" afaik > > and most devices (at least all Sony's that I'm working with) use an > > appended DTB that's always in-sync with the kernel image. > > > > I think that with the introduction of DT overlays the system becomes > more flexible and as such more susceptible for bugs caused by unexpected > DT versions. Offtopic: We have some problems with this on newer Sony devices where the BL indeed tries to overlay this DTBO on the DT, which is usually a downstream DT not fitting on top of a mainline kernel+appended-DTB. The solution is to simply wipe DTBO, and afaik it should be fine to compile the overlay bits directly inside the appended-DTB anyway. Leads to unsuspecting problems at times, but it is manageable. > I think in practice the real issues comes when the DTB is delivered > separately (i.e. not by boot.img) or inbetween two kernel releases where > the Qualcomm tree might not be in sync with the driver tree. Dmitry sees this as a problem for msm8974 but I'm not familiar enough with the board. I take it this doesn't use appended DTBs then? > > > As such, if I pick this patch up as a fix for 5.15 you can respin the > > > other two patches and they can land in 5.16 and I would be surprised if > > > anyone will run into any issues with it. > > > > > > I.e. I've applied this patch. > > > > Sounds good, I'll leave this patch out from v2. Should it have a Fixes: > > tag to get backported too? > > > > Sounds good, I added to this: > > Fixes: 6969d1d9c615 ("ARM: dts: qcom-apq8064: Set 'cxo_board' as ref clock of the DSI PHY") Did the same on the v2 respin since it seems like those patches were incomplete without the driver change. > > Since most review seems to be in I'll respin v2 shortly with the > > addition of the "ref" clock to msm8974, that should probably get the > > same treatment (added to 5.15 fixes) then we can land this patchset in > > 5.16 (maybe without .name= fallback if Dmitry is okay with that). > > > > Sounds good. Sent the msm8974 patch separately and re-spun a v2. I haven't added the .name="xo" fallback yet while awaiting Dmitry to see if that counts as enough time for firmware to be delivered between kernel 5.15 and 5.16. - Marijn
diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi index 2687c4e890ba..77659b783759 100644 --- a/arch/arm/boot/dts/qcom-apq8064.dtsi +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi @@ -198,7 +198,7 @@ cxo_board: cxo_board { clock-frequency = <19200000>; }; - pxo_board { + pxo_board: pxo_board { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <27000000>; @@ -1306,7 +1306,7 @@ dsi0_phy: dsi-phy@4700200 { reg-names = "dsi_pll", "dsi_phy", "dsi_phy_regulator"; clock-names = "iface_clk", "ref"; clocks = <&mmcc DSI_M_AHB_CLK>, - <&cxo_board>; + <&pxo_board>; };
The 28NM DSI PLL driver for msm8960 calculates with a 27MHz reference clock and should hence use PXO, not CXO which runs at 19.2MHz. Note that none of the DSI PHY/PLL drivers currently use this "ref" clock; they all rely on (sometimes inexistant) global clock names and usually function normally without a parent clock. This discrepancy will be corrected in a future patch, for which this change needs to be in place first. Cc: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org> --- arch/arm/boot/dts/qcom-apq8064.dtsi | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)