| Message ID | 20250217-drm-msm-phy-pll-cfg-reg-v4-4-106b0d1df51e@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drm/msm/dsi/phy: Improvements around concurrent PHY_CMN_CLK_CFG[01] | expand |
On Mon, Feb 17, 2025 at 12:53:17PM +0100, Krzysztof Kozlowski wrote: > Add bitfields for PHY_CMN_CLK_CFG0 and PHY_CMN_CLK_CFG1 registers to > avoid hard-coding bit masks and shifts and make the code a bit more > readable. While touching the lines in dsi_7nm_pll_save_state() > resulting cached->pix_clk_div assignment would be too big, so just > combine pix_clk_div and bit_clk_div into one cached state to make > everything simpler. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > Changes in v4: > 1. Add mising bitfield.h include > 2. One more FIELD_GET and DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL (Dmitry) > > Changes in v3: > 1. Use FIELD_GET > 2. Keep separate bit_clk_div and pix_clk_div > 3. Rebase (some things moved to previous patches) > > Changes in v2: > 1. New patch > --- > drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 ++++++++----- > drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 1 + > 2 files changed, 9 insertions(+), 5 deletions(-) > > @@ -739,7 +741,8 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide > u32 data; > > data = readl(pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > - writel(data | 3, pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > + writel(data | DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL(3), > + pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); Nit: should this also be using dsi_pll_cmn_clk_cfg1_update() ? > > phy_pll_out_dsi_parent = pll_post_out_div; > } else {
On 17/02/2025 14:13, Dmitry Baryshkov wrote: > On Mon, Feb 17, 2025 at 12:53:17PM +0100, Krzysztof Kozlowski wrote: >> Add bitfields for PHY_CMN_CLK_CFG0 and PHY_CMN_CLK_CFG1 registers to >> avoid hard-coding bit masks and shifts and make the code a bit more >> readable. While touching the lines in dsi_7nm_pll_save_state() >> resulting cached->pix_clk_div assignment would be too big, so just >> combine pix_clk_div and bit_clk_div into one cached state to make >> everything simpler. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> Changes in v4: >> 1. Add mising bitfield.h include >> 2. One more FIELD_GET and DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL (Dmitry) >> >> Changes in v3: >> 1. Use FIELD_GET >> 2. Keep separate bit_clk_div and pix_clk_div >> 3. Rebase (some things moved to previous patches) >> >> Changes in v2: >> 1. New patch >> --- >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 ++++++++----- >> drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 1 + >> 2 files changed, 9 insertions(+), 5 deletions(-) >> >> @@ -739,7 +741,8 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide >> u32 data; >> >> data = readl(pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); >> - writel(data | 3, pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); >> + writel(data | DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL(3), >> + pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > > Nit: should this also be using dsi_pll_cmn_clk_cfg1_update() ? This is before clocks are registered so there is really no chance for simultaneous access. It is rather then question of code readability or obviousness. Best regards, Krzysztof
On Mon, Feb 17, 2025 at 02:37:31PM +0100, Krzysztof Kozlowski wrote: > On 17/02/2025 14:13, Dmitry Baryshkov wrote: > > On Mon, Feb 17, 2025 at 12:53:17PM +0100, Krzysztof Kozlowski wrote: > >> Add bitfields for PHY_CMN_CLK_CFG0 and PHY_CMN_CLK_CFG1 registers to > >> avoid hard-coding bit masks and shifts and make the code a bit more > >> readable. While touching the lines in dsi_7nm_pll_save_state() > >> resulting cached->pix_clk_div assignment would be too big, so just > >> combine pix_clk_div and bit_clk_div into one cached state to make > >> everything simpler. > >> > >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > >> > >> --- > >> > >> Changes in v4: > >> 1. Add mising bitfield.h include > >> 2. One more FIELD_GET and DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL (Dmitry) > >> > >> Changes in v3: > >> 1. Use FIELD_GET > >> 2. Keep separate bit_clk_div and pix_clk_div > >> 3. Rebase (some things moved to previous patches) > >> > >> Changes in v2: > >> 1. New patch > >> --- > >> drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 ++++++++----- > >> drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 1 + > >> 2 files changed, 9 insertions(+), 5 deletions(-) > >> > >> @@ -739,7 +741,8 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide > >> u32 data; > >> > >> data = readl(pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > >> - writel(data | 3, pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > >> + writel(data | DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL(3), > >> + pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); > > > > Nit: should this also be using dsi_pll_cmn_clk_cfg1_update() ? > > > This is before clocks are registered so there is really no chance for > simultaneous access. > > It is rather then question of code readability or obviousness. That's why I added it as a nit. I don't think that it's required, but I think it might improve the patch.
diff --git a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c index 798168180c1ab6c96ec2384f854302720cb27932..cf63b4c5c3c0c39f0031dbe948b1694765f01af8 100644 --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c @@ -3,6 +3,7 @@ * Copyright (c) 2018, The Linux Foundation */ +#include <linux/bitfield.h> #include <linux/clk.h> #include <linux/clk-provider.h> #include <linux/iopoll.h> @@ -572,11 +573,11 @@ static void dsi_7nm_pll_save_state(struct msm_dsi_phy *phy) cached->pll_out_div &= 0x3; cmn_clk_cfg0 = readl(phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG0); - cached->bit_clk_div = cmn_clk_cfg0 & 0xf; - cached->pix_clk_div = (cmn_clk_cfg0 & 0xf0) >> 4; + cached->bit_clk_div = FIELD_GET(DSI_7nm_PHY_CMN_CLK_CFG0_DIV_CTRL_3_0__MASK, cmn_clk_cfg0); + cached->pix_clk_div = FIELD_GET(DSI_7nm_PHY_CMN_CLK_CFG0_DIV_CTRL_7_4__MASK, cmn_clk_cfg0); cmn_clk_cfg1 = readl(phy_base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); - cached->pll_mux = cmn_clk_cfg1 & 0x3; + cached->pll_mux = FIELD_GET(DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL__MASK, cmn_clk_cfg1); DBG("DSI PLL%d outdiv %x bit_clk_div %x pix_clk_div %x pll_mux %x", pll_7nm->phy->id, cached->pll_out_div, cached->bit_clk_div, @@ -598,7 +599,8 @@ static int dsi_7nm_pll_restore_state(struct msm_dsi_phy *phy) dsi_pll_cmn_clk_cfg0_write(pll_7nm, DSI_7nm_PHY_CMN_CLK_CFG0_DIV_CTRL_3_0(cached->bit_clk_div) | DSI_7nm_PHY_CMN_CLK_CFG0_DIV_CTRL_7_4(cached->pix_clk_div)); - dsi_pll_cmn_clk_cfg1_update(pll_7nm, 0x3, cached->pll_mux); + dsi_pll_cmn_clk_cfg1_update(pll_7nm, DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL__MASK, + cached->pll_mux); ret = dsi_pll_7nm_vco_set_rate(phy->vco_hw, pll_7nm->vco_current_rate, @@ -739,7 +741,8 @@ static int pll_7nm_register(struct dsi_pll_7nm *pll_7nm, struct clk_hw **provide u32 data; data = readl(pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); - writel(data | 3, pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); + writel(data | DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL(3), + pll_7nm->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1); phy_pll_out_dsi_parent = pll_post_out_div; } else { diff --git a/drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml b/drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml index 35f7f40e405b7dd9687725eae754522a7136725e..d2c8c46bb04159da6e539bfe80a4b5dc9ffdf367 100644 --- a/drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml +++ b/drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml @@ -17,6 +17,7 @@ xsi:schemaLocation="https://gitlab.freedesktop.org/freedreno/ rules-fd.xsd"> <bitfield name="CLK_EN" pos="5" type="boolean"/> <bitfield name="CLK_EN_SEL" pos="4" type="boolean"/> <bitfield name="BITCLK_SEL" low="2" high="3" type="uint"/> + <bitfield name="DSICLK_SEL" low="0" high="1" type="uint"/> </reg32> <reg32 offset="0x00018" name="GLBL_CTRL"/> <reg32 offset="0x0001c" name="RBUF_CTRL"/>
Add bitfields for PHY_CMN_CLK_CFG0 and PHY_CMN_CLK_CFG1 registers to avoid hard-coding bit masks and shifts and make the code a bit more readable. While touching the lines in dsi_7nm_pll_save_state() resulting cached->pix_clk_div assignment would be too big, so just combine pix_clk_div and bit_clk_div into one cached state to make everything simpler. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- Changes in v4: 1. Add mising bitfield.h include 2. One more FIELD_GET and DSI_7nm_PHY_CMN_CLK_CFG1_DSICLK_SEL (Dmitry) Changes in v3: 1. Use FIELD_GET 2. Keep separate bit_clk_div and pix_clk_div 3. Rebase (some things moved to previous patches) Changes in v2: 1. New patch --- drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 13 ++++++++----- drivers/gpu/drm/msm/registers/display/dsi_phy_7nm.xml | 1 + 2 files changed, 9 insertions(+), 5 deletions(-)