| Message ID | d4b59d5e94c4c4a488cd24c6c169e418ad53980d.1735524526.git.xiaopei01@kylinos.cn |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: freescale: fsl-samsung-hdmi: fix build error in fsl_samsung_hdmi_phy_configure_pll_lock_det | expand |
On Sun, Dec 29, 2024 at 8:11 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: > > FIELD_PREP() checks that a constant fits into the available bitfield, > but the index div equals to 4,is out of range, which gcc > correctly complains about: > > In function ‘fsl_samsung_hdmi_phy_configure_pll_lock_det’, > inlined from ‘fsl_samsung_hdmi_phy_configure’ at > drivers/phy/freescale/phy-fsl-samsung-hdmi.c :470:2: > ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_538’ > declared with attribute error: FIELD_PREP: value too large for the field > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > | ^ > ././include/linux/compiler_types.h:523:4: note: in definition of > macro ‘__compiletime_assert’ 523 | prefix ## suffix(); > | ^~~~~~ > ././include/linux/compiler_types.h:542:2: note: in expansion of macro > ‘_compiletime_assert’ > 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > __COUNTER__) > > Rework this so the field value which is restricted to the range of 0 to 3, > so build error will fix. > > Fixes: d567679f2b6a ("phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") > Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn> > --- > drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > 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); adam > /* > * Calculation for the frequency lock detector target code (fld_tg_code) > -- > 2.25.1 >
在 2024/12/31 10:11, Adam Ford 写道: > On Sun, Dec 29, 2024 at 8:11 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: >> FIELD_PREP() checks that a constant fits into the available bitfield, >> but the index div equals to 4,is out of range, which gcc >> correctly complains about: >> >> In function ‘fsl_samsung_hdmi_phy_configure_pll_lock_det’, >> inlined from ‘fsl_samsung_hdmi_phy_configure’ at >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c :470:2: >> ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_538’ >> declared with attribute error: FIELD_PREP: value too large for the field >> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, >> __COUNTER__) >> | ^ >> ././include/linux/compiler_types.h:523:4: note: in definition of >> macro ‘__compiletime_assert’ 523 | prefix ## suffix(); >> | ^~~~~~ >> ././include/linux/compiler_types.h:542:2: note: in expansion of macro >> ‘_compiletime_assert’ >> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, >> __COUNTER__) >> >> Rework this so the field value which is restricted to the range of 0 to 3, >> so build error will fix. >> >> Fixes: d567679f2b6a ("phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn> >> --- >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c >> 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); yes, you are right,may be never reach 4,so div - 1 wille fix FIELD_PREP build error. #define REG12_CK_DIV_MASK GENMASK(5, 4) only two bit ,max value is 3. thanks! Pei. > > adam >> /* >> * Calculation for the frequency lock detector target code (fld_tg_code) >> -- >> 2.25.1 >>
On Mon, Dec 30, 2024 at 8:19 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: > > > 在 2024/12/31 10:11, Adam Ford 写道: > > On Sun, Dec 29, 2024 at 8:11 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: > >> FIELD_PREP() checks that a constant fits into the available bitfield, > >> but the index div equals to 4,is out of range, which gcc > >> correctly complains about: > >> > >> In function ‘fsl_samsung_hdmi_phy_configure_pll_lock_det’, > >> inlined from ‘fsl_samsung_hdmi_phy_configure’ at > >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c :470:2: > >> ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_538’ > >> declared with attribute error: FIELD_PREP: value too large for the field > >> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > >> __COUNTER__) > >> | ^ > >> ././include/linux/compiler_types.h:523:4: note: in definition of > >> macro ‘__compiletime_assert’ 523 | prefix ## suffix(); > >> | ^~~~~~ > >> ././include/linux/compiler_types.h:542:2: note: in expansion of macro > >> ‘_compiletime_assert’ > >> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, > >> __COUNTER__) > >> > >> Rework this so the field value which is restricted to the range of 0 to 3, > >> so build error will fix. > >> > >> Fixes: d567679f2b6a ("phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") > >> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn> > >> --- > >> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c > >> 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); > > yes, you are right,may be never reach 4,so div - 1 wille fix FIELD_PREP > > build error. > > #define REG12_CK_DIV_MASK GENMASK(5, 4) > > only two bit ,max value is 3. Would doing a logical AND of (div & 0x03) make the compiler error go away? I feel like the evaluation of checking if div==4 is unnecessary since the for-loop won't let it get to 4 and it might add an additional instruction. adam > > thanks! > > Pei. > > > > > adam > >> /* > >> * Calculation for the frequency lock detector target code (fld_tg_code) > >> -- > >> 2.25.1 > >>
在 2025/1/1 01:02, Adam Ford 写道: > On Mon, Dec 30, 2024 at 8:19 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: >> >> >> 在 2024/12/31 10:11, Adam Ford 写道: >>> On Sun, Dec 29, 2024 at 8:11 PM Pei Xiao <xiaopei01@kylinos.cn> wrote: >>>> FIELD_PREP() checks that a constant fits into the available bitfield, >>>> but the index div equals to 4,is out of range, which gcc >>>> correctly complains about: >>>> >>>> In function ‘fsl_samsung_hdmi_phy_configure_pll_lock_det’, >>>> inlined from ‘fsl_samsung_hdmi_phy_configure’ at >>>> drivers/phy/freescale/phy-fsl-samsung-hdmi.c :470:2: >>>> ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_538’ >>>> declared with attribute error: FIELD_PREP: value too large for the field >>>> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, >>>> __COUNTER__) >>>> | ^ >>>> ././include/linux/compiler_types.h:523:4: note: in definition of >>>> macro ‘__compiletime_assert’ 523 | prefix ## suffix(); >>>> | ^~~~~~ >>>> ././include/linux/compiler_types.h:542:2: note: in expansion of macro >>>> ‘_compiletime_assert’ >>>> 542 | _compiletime_assert(condition, msg, __compiletime_assert_, >>>> __COUNTER__) >>>> >>>> Rework this so the field value which is restricted to the range of 0 to 3, >>>> so build error will fix. >>>> >>>> Fixes: d567679f2b6a ("phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") >>>> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn> >>>> --- >>>> drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c >>>> 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); >> >> yes, you are right,may be never reach 4,so div - 1 wille fix FIELD_PREP >> >> build error. >> >> #define REG12_CK_DIV_MASK GENMASK(5, 4) >> >> only two bit ,max value is 3. > > Would doing a logical AND of (div & 0x03) make the compiler error go > away? I feel like the evaluation of checking if div==4 is unnecessary > since the for-loop won't let it get to 4 and it might add an > additional instruction. Yes, thank you for your great modification. After adding & 0x03, the compilation failure disappeared. I apologize for my messy code. Thank you. I will send out the modified patch later. > adam >> >> thanks! >> >> Pei. >> >>> >>> adam >>>> /* >>>> * Calculation for the frequency lock detector target code (fld_tg_code) >>>> -- >>>> 2.25.1 >>>>
Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600: > > 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. (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,
On Thu, Jan 2, 2025 at 6:15 AM Dominique Martinet <dominique.martinet@atmark-techno.com> wrote: > > Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600: > > > 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); > (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
hi Adam, 在 2025/1/2 23:04, Adam Ford 写道: > On Thu, Jan 2, 2025 at 6:15 AM Dominique Martinet > <dominique.martinet@atmark-techno.com> wrote: >> >> Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600: >>>> 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)); >> (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
在 2025/1/3 09:34, Pei Xiao 写道: > hi Adam, > > 在 2025/1/2 23:04, Adam Ford 写道: >> On Thu, Jan 2, 2025 at 6:15 AM Dominique Martinet >> <dominique.martinet@atmark-techno.com> wrote: >>> Adam Ford wrote on Mon, Dec 30, 2024 at 08:11:16PM -0600: >>>>> 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. >>> (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
diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c 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)); /* * Calculation for the frequency lock detector target code (fld_tg_code)
FIELD_PREP() checks that a constant fits into the available bitfield, but the index div equals to 4,is out of range, which gcc correctly complains about: In function ‘fsl_samsung_hdmi_phy_configure_pll_lock_det’, inlined from ‘fsl_samsung_hdmi_phy_configure’ at drivers/phy/freescale/phy-fsl-samsung-hdmi.c :470:2: ././include/linux/compiler_types.h:542:38: error: call to ‘__compiletime_assert_538’ declared with attribute error: FIELD_PREP: value too large for the field 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) | ^ ././include/linux/compiler_types.h:523:4: note: in definition of macro ‘__compiletime_assert’ 523 | prefix ## suffix(); | ^~~~~~ ././include/linux/compiler_types.h:542:2: note: in expansion of macro ‘_compiletime_assert’ 542 | _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__) Rework this so the field value which is restricted to the range of 0 to 3, so build error will fix. Fixes: d567679f2b6a ("phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn> --- drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)