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@xxxxxxxxxx>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> 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)
> 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
