Re: [PATCH] phy: freescale: fsl-samsung-hdmi: fix build error in fsl_samsung_hdmi_phy_configure_pll_lock_det

在 2025/1/3 09:34, Pei Xiao 写道:

hi Adam,

在 2025/1/2 23:04, Adam Ford 写道:

quoted

On Thu, Jan 2, 2025 at 6:15 AM Dominique Martinet
[off-list ref] wrote:

quoted

Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600:

quoted

quoted

index 5eac70a1e858..3e4d1a5160ea 100644

--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c

@@ -341,7 +341,7 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
                        break;
        }

-       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
+       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div == 4 ? div - 1 : div), phy->regs + PHY_REG(12));

The for-loop above this line states:   for (div = 0; div < 4; div++)
How could this ever reach 4?  If it did reach 4, the calculation for
int_pllclk would need to be recalculated since int_pllclk = pclk / (1
<< div);

But... for (div = 0; div < 4; div++) does reach 4, if the break
condition didn't match, which is something the compiler cannot ensure
here.

The old code would just fall out of any of the switch cases and fallback
to div = 1 if pixclk > 297000000, which is likely incorrect, so in that
sense just padding this through `& 3` and pretending it will never
happen is probably acceptable, but this ought to have a better comment
than what Pei just sent.

Maybe we use the MAX function to set div = max(div,3);

do you mean:
writeb(FIELD_PREP(REG12_CK_DIV_MASK, min(div, 3)), phy->regs + PHY_REG(12)); 

Does anyone have any suggestions? 

This compilation issue will result in errors on some versions of gcc, 

so it still needs to be resolved after all.

quoted

quoted

(this was correct with the old lookup tables, I'm not sure if we can't
compute any higher frequencies now?)

My preference would be to actually check and handle this somehow since I
don't think this part of the code is that performance critical that we
can't afford an extra instruction, e.g. something like that:
if (WARN_ONCE(div == 4, "pixclk %u out of range", pclk))
        (appropriate fallback or return?)

but I haven't spent the time to actually check so will leave that up to
you.

Thank you both,
--
Dominique

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy