Message-ID: <CAA8EJppUkLYfHUcNcJA5or4ZVJsbTe74a6GGV1CR6zqCWmVjRA@mail.gmail.com>
Date: Wed, 5 Feb 2025 13:23:06 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: Rob Clark <robdclark@...il.com>, Abhinav Kumar <quic_abhinavk@...cinc.com>, 
	Sean Paul <sean@...rly.run>, Marijn Suijten <marijn.suijten@...ainline.org>, 
	David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>, linux-arm-msm@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] drm/msm/dsi/phy: Protect PHY_CMN_CLK_CFG1 against
 clock driver

Hi,

On Wed, 5 Feb 2025 at 11:34, Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 05/02/2025 03:51, Dmitry Baryshkov wrote:
> > On Tue, Feb 04, 2025 at 04:46:04PM +0100, Krzysztof Kozlowski wrote:
> >> On 04/02/2025 15:26, Dmitry Baryshkov wrote:
> >>> On Tue, Feb 04, 2025 at 10:21:25AM +0100, Krzysztof Kozlowski wrote:
> >>>> On 03/02/2025 18:41, Dmitry Baryshkov wrote:
> >>>>> On Mon, Feb 03, 2025 at 06:29:19PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> PHY_CMN_CLK_CFG1 register is updated by the PHY driver and by a mux
> >>>>>> clock from Common Clock Framework:
> >>>>>> devm_clk_hw_register_mux_parent_hws().  There could be a path leading to
> >>>>>> concurrent and conflicting updates between PHY driver and clock
> >>>>>> framework, e.g. changing the mux and enabling PLL clocks.
> >>>>>>
> >>>>>> Add dedicated spinlock to be sure all PHY_CMN_CLK_CFG1 updates are
> >>>>>> synchronized.
> >>>>>>
> >>>>>> Fixes: 1ef7c99d145c ("drm/msm/dsi: add support for 7nm DSI PHY/PLL")
> >>>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Changes in v2:
> >>>>>> 1. Store BIT(4) and BIT(5) in local var in dsi_pll_enable_global_clk()
> >>>>>> ---
> >>>>>>  drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c | 35 +++++++++++++++++++------------
> >>>>>>  1 file changed, 22 insertions(+), 13 deletions(-)
> >>>>>>
> >>>>>> 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 c164f845653816291ad96c863257f75462ef58e7..e26f53f7cde8f0f6419a633f5d39784dc2e5bb98 100644
> >>>>>> --- a/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>>>>> +++ b/drivers/gpu/drm/msm/dsi/phy/dsi_phy_7nm.c
> >>>>>> @@ -83,6 +83,9 @@ struct dsi_pll_7nm {
> >>>>>>          /* protects REG_DSI_7nm_PHY_CMN_CLK_CFG0 register */
> >>>>>>          spinlock_t postdiv_lock;
> >>>>>>
> >>>>>> +        /* protects REG_DSI_7nm_PHY_CMN_CLK_CFG1 register */
> >>>>>> +        spinlock_t pclk_mux_lock;
> >>>>>> +
> >>>>>>          struct pll_7nm_cached_state cached_state;
> >>>>>>
> >>>>>>          struct dsi_pll_7nm *slave;
> >>>>>> @@ -381,22 +384,32 @@ static void dsi_pll_cmn_clk_cfg0_write(struct dsi_pll_7nm *pll, u32 val)
> >>>>>>          spin_unlock_irqrestore(&pll->postdiv_lock, flags);
> >>>>>>  }
> >>>>>>
> >>>>>> -static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >>>>>> +static void dsi_pll_cmn_clk_cfg1_update(struct dsi_pll_7nm *pll, u32 mask,
> >>>>>> +                                        u32 val)
> >>>>>>  {
> >>>>>> +        unsigned long flags;
> >>>>>>          u32 data;
> >>>>>>
> >>>>>> +        spin_lock_irqsave(&pll->pclk_mux_lock, flags);
> >>>>>>          data = readl(pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>>>> -        writel(data & ~BIT(5), pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>>>> +        data &= ~mask;
> >>>>>> +        data |= val & mask;
> >>>>>> +
> >>>>>> +        writel(data, pll->phy->base + REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>>>>> +        spin_unlock_irqrestore(&pll->pclk_mux_lock, flags);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void dsi_pll_disable_global_clk(struct dsi_pll_7nm *pll)
> >>>>>> +{
> >>>>>> +        dsi_pll_cmn_clk_cfg1_update(pll, BIT(5), 0);
> >>>>>>  }
> >>>>>>
> >>>>>>  static void dsi_pll_enable_global_clk(struct dsi_pll_7nm *pll)
> >>>>>>  {
> >>>>>> -        u32 data;
> >>>>>> +        u32 cfg_1 = BIT(5) | BIT(4);
> >>>>>
> >>>>> Please define these two bits too.
> >>>>
> >>>> Why? They were not defined before. This only moving existing code.
> >>>
> >>> Previously it was just a bit magic. Currently you are adding them as
> >>
> >> No, previous code:
> >>
> >> writel(data | BIT(5) | BIT(4), pll->phy->base +
> >> REG_DSI_7nm_PHY_CMN_CLK_CFG1);
> >>
> >> This is a mask and update in the same time, because:
> >>      (data & (BIT(5) | BIT(4)) | BIT(5) | BIT(4)
> >> is just redudant.
> >>
> >> I did not do any logical change, I did not add any mask or field.
> >> Everything was already there.
> >
> > Yes... and no. Previously it was just writel(foo | BIT(5) | BIT(4)). Now
>
> You did not address my comment. Previous code was:
>
> (foo & (BIT(5) | BIT(4)) | BIT(5) | BIT(4)
>
> Just for shorter syntax it was written different way:
>
> foo | BIT(5) | BIT(4)

Previously it was a simple writel() with some bit magic. Now you call
dsi_pll_cmn_clk_cfg1_update() passing the register bit field through
the 'mask' argument. I'm asking to get those masks defined. Is it
possible?

Yes, the code is equivalent and results in the same values being
written to the same registers.
At the same time you have added a logical entity, a masked write. I
want to be able to understand if bits 4 and 5 are a part of the same
register field or they belong to two different fields and can be
written separately. I really don't understand why are we spending so
much time arguing about a simple #define. Okay, in case of drm/msm it
is not a #define, it is <reg><bitfield/></reg>. The net result is the
same.

>
> > your code adds BIT(5) as a 'mask' parameter. Is it a correct mask for
>
> No, my code does not add it. It was already there, look:
>
> foo | BIT(5) | BIT(4)
>       ^^^^^^^ here
>
>
> > that field? That's why I'm asking you to define those - you have changed
>
> No, I did not change bitwrites. The code is 100% equivalent, both
> logically and assembly.
>
> You mistake maybe with some other part doing "writel(data & ~BIT(5)" in
> dsi_pll_disable_global_clk() but that's just poor diff.
>
> > bitwrites to the masked bit writes. Masks should be defined.
> >
> >>
> >>
> >>> masks. I want to know if BIT(4) and BIT(5) are parts of the same
> >>> bitfield (2 bits wide) or if they define two different bits.
> >>
> >> While in general you are right, it does not matter for this fix. If this
> >> are separate bitfields - fix is correct. If this is one bitfield - fix
> >> is still correct. You could claim that if this was one bitfield, using
> >> 2xBIT() is not logical, but this was there already, so again my fix is
> >> only fixing and keeping entire logic or inconsistencies intact.
> >
>
>
> Best regards,
> Krzysztof



-- 
With best wishes
Dmitry

Powered by blists - more mailing lists