[PATCH V2 1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range

[PATCH V2 1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range

[Adam Ford]

The Integer divder uses values of P,M, and S to determine the PLL
rate.  Currently, the range of M was set based on a series of
table entries where the range was limited.  Since the ref manual
shows it is 8-bit wide, expand the range to be up to 255.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V2:  Fix typo in comment

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 2c8038864357..412c03b7dcd6 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -406,16 +406,15 @@ static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u1
 				continue;
 
 			/*
-			 * TODO: Ref Manual doesn't state the range of _m
-			 * so this should be further refined if possible.
-			 * This range was set based on the original values
-			 * in the lookup table
+			 * The Ref manual doesn't explicitly state the range of M,
+			 * but it does show it as an 8-bit value, so reject
+			 * any value above 255.
 			 */
 			tmp = (u64)fout * (_p * _s);
 			do_div(tmp, 24 * MHZ);
-			_m = tmp;
-			if (_m < 0x30 || _m > 0x7b)
+			if (tmp > 255)
 				continue;
+			_m = tmp;
 
 			/*
 			 * Rev 2 of the Ref Manual states the
-- 
2.45.2

[PATCH V2 2/3] phy: freescale: fsl-samsung-hdmi: Stop searching when exact match is found

[Adam Ford]

There are a series of for-loops which check various values of P and S
for the integer divder PLL.  The for loops search all entries and use
the one closest to the nominal, but it continues to searches through
all for loops even after the nominal is achieved.  Ending when the
nominal value is found stops wasting time, since it will not find
a better value than a deviation of 0 Hz.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V2:  No Change
diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 412c03b7dcd6..121f67455cec 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -440,9 +440,13 @@ static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u1
 				min_delta = delta;
 				best_freq = tmp;
 			}
+
+			/* If we have an exact match, stop looking for a better value */
+			if (!delta)
+				goto done;
 		}
 	}
-
+done:
 	if (best_freq) {
 		*p = best_p;
 		*m = best_m;
-- 
2.45.2

[PATCH V2 3/3] phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation

[Adam Ford]

Currently, the calcuation for fld_tg_code is based on a lookup table,
but there are gaps in the lookup table, and frequencies in these
gaps may not properly use the correct divider.  Based on the description
of FLD_CK_DIV, the internal PLL frequency should be less than 50 MHz,
so directly calcuate the value of FLD_CK_DIV from pixclk.
This allow for proper calcuation of any pixel clock and eliminates a
few gaps in the LUT.

Since the value of the int_pllclk is in Hz, do the fixed-point
math in Hz to achieve a more accurate value and reduces the complexity
of the caluation to 24MHz * (256 / int_pllclk).

Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
---
V2:  No change

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 121f67455cec..5eac70a1e858 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -331,25 +331,17 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 {
 	u32 pclk = cfg->pixclk;
 	u32 fld_tg_code;
-	u32 pclk_khz;
-	u8 div = 1;
-
-	switch (cfg->pixclk) {
-	case  22250000 ...  47500000:
-		div = 1;
-		break;
-	case  50349650 ...  99000000:
-		div = 2;
-		break;
-	case 100699300 ... 198000000:
-		div = 4;
-		break;
-	case 205000000 ... 297000000:
-		div = 8;
-		break;
+	u32 int_pllclk;
+	u8 div;
+
+	/* Find int_pllclk speed */
+	for (div = 0; div < 4; div++) {
+		int_pllclk = pclk / (1 << div);
+		if (int_pllclk < (50 * MHZ))
+			break;
 	}
 
-	writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG(12));
+	writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
 
 	/*
 	 * Calculation for the frequency lock detector target code (fld_tg_code)
@@ -362,10 +354,8 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
 	 *        settings rounding up always too. TODO: Check if that is
 	 *        correct.
 	 */
-	pclk /= div;
-	pclk_khz = pclk / 1000;
-	fld_tg_code = 256 * 1000 * 1000 / pclk_khz * 24;
-	fld_tg_code = DIV_ROUND_UP(fld_tg_code, 1000);
+
+	fld_tg_code =  DIV_ROUND_UP(24 * MHZ * 256, int_pllclk);
 
 	/* FLD_TOL and FLD_RP_CODE taken from downstream driver */
 	writeb(FIELD_PREP(REG13_TG_CODE_LOW_MASK, fld_tg_code),
-- 
2.45.2

Re: [PATCH V2 3/3] phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation

[Geert Uytterhoeven]

Hi Adam,

On Sat, Oct 26, 2024 at 3:21 PM Adam Ford <aford173@gmail.com> wrote:
> Currently, the calcuation for fld_tg_code is based on a lookup table,

calculation (everywhere)

> but there are gaps in the lookup table, and frequencies in these
> gaps may not properly use the correct divider.  Based on the description
> of FLD_CK_DIV, the internal PLL frequency should be less than 50 MHz,
> so directly calcuate the value of FLD_CK_DIV from pixclk.
> This allow for proper calcuation of any pixel clock and eliminates a
> few gaps in the LUT.
>
> Since the value of the int_pllclk is in Hz, do the fixed-point
> math in Hz to achieve a more accurate value and reduces the complexity
> of the caluation to 24MHz * (256 / int_pllclk).
>
> Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Thanks for your patch, which is now commit d567679f2b6a8bce ("phy:
freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") in
next-20241209 and later.

> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -331,25 +331,17 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
>  {
>         u32 pclk = cfg->pixclk;
>         u32 fld_tg_code;
> -       u32 pclk_khz;
> -       u8 div = 1;
> -
> -       switch (cfg->pixclk) {
> -       case  22250000 ...  47500000:
> -               div = 1;
> -               break;
> -       case  50349650 ...  99000000:
> -               div = 2;
> -               break;
> -       case 100699300 ... 198000000:
> -               div = 4;
> -               break;
> -       case 205000000 ... 297000000:
> -               div = 8;
> -               break;
> +       u32 int_pllclk;
> +       u8 div;
> +
> +       /* Find int_pllclk speed */
> +       for (div = 0; div < 4; div++) {
> +               int_pllclk = pclk / (1 << div);
> +               if (int_pllclk < (50 * MHZ))
> +                       break;
>         }
>
> -       writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG(12));
> +       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));

This causes a build failure on m68k with gcc version 9.5.0 (Ubuntu
9.5.0-1ubuntu1~22.04):

  CC [M]  drivers/phy/freescale/phy-fsl-samsung-hdmi.o
In file included from ./arch/m68k/include/asm/io_mm.h:25,
                 from ./arch/m68k/include/asm/io.h:8,
                 from ./include/linux/io.h:14,
                 from ./include/linux/iopoll.h:14,
                 from drivers/phy/freescale/phy-fsl-samsung-hdmi.c:12:
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_147’ declared with attribute error: FIELD_PREP:
value too large for the field
  542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |                                      ^
./arch/m68k/include/asm/raw_io.h:30:82: note: in definition of macro ‘out_8’
   30 | #define out_8(addr,b) (void)((*(__force volatile u8 *)
(unsigned long)(addr)) = (b))
      |
                  ^
drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:2: note: in expansion
of macro ‘writeb’
  344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
      |  ^~~~~~
././include/linux/compiler_types.h:530:2: note: in expansion of macro
‘__compiletime_assert’
  530 |  __compiletime_assert(condition, msg, prefix, suffix)
      |  ^~~~~~~~~~~~~~~~~~~~
././include/linux/compiler_types.h:542:2: note: in expansion of macro
‘_compiletime_assert’
  542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
      |  ^~~~~~~~~~~~~~~~~~~
./include/linux/build_bug.h:39:37: note: in expansion of macro
‘compiletime_assert’
   39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
      |                                     ^~~~~~~~~~~~~~~~~~
./include/linux/bitfield.h:68:3: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
   68 |   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
      |   ^~~~~~~~~~~~~~~~
./include/linux/bitfield.h:115:3: note: in expansion of macro ‘__BF_FIELD_CHECK’
  115 |   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
      |   ^~~~~~~~~~~~~~~~
drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:9: note: in expansion
of macro ‘FIELD_PREP’
  344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
      |         ^~~~~~~~~~

As it builds fine on i386, I looked at the preprocessed source files,
but didn't see any differences that could explain this.

I changed cross-compiler to gcc version 10.5.0 (Ubuntu 10.5.0-1ubuntu1~22.04),
and that fixed the issue on m68k.
I changed the native compiler to gcc-9, and the build started failing
on i386 and x86_64, too....

>
>         /*
>          * Calculation for the frequency lock detector target code (fld_tg_code)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V2 3/3] phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation

[Adam Ford]

On Fri, Dec 13, 2024 at 8:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Adam,
>
> On Sat, Oct 26, 2024 at 3:21 PM Adam Ford <aford173@gmail.com> wrote:
> > Currently, the calcuation for fld_tg_code is based on a lookup table,
>
> calculation (everywhere)
>
> > but there are gaps in the lookup table, and frequencies in these
> > gaps may not properly use the correct divider.  Based on the description
> > of FLD_CK_DIV, the internal PLL frequency should be less than 50 MHz,
> > so directly calcuate the value of FLD_CK_DIV from pixclk.
> > This allow for proper calcuation of any pixel clock and eliminates a
> > few gaps in the LUT.
> >
> > Since the value of the int_pllclk is in Hz, do the fixed-point
> > math in Hz to achieve a more accurate value and reduces the complexity
> > of the caluation to 24MHz * (256 / int_pllclk).
> >
> > Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
>
> Thanks for your patch, which is now commit d567679f2b6a8bce ("phy:
> freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") in
> next-20241209 and later.
>
> > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > @@ -331,25 +331,17 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
> >  {
> >         u32 pclk = cfg->pixclk;
> >         u32 fld_tg_code;
> > -       u32 pclk_khz;
> > -       u8 div = 1;
> > -
> > -       switch (cfg->pixclk) {
> > -       case  22250000 ...  47500000:
> > -               div = 1;
> > -               break;
> > -       case  50349650 ...  99000000:
> > -               div = 2;
> > -               break;
> > -       case 100699300 ... 198000000:
> > -               div = 4;
> > -               break;
> > -       case 205000000 ... 297000000:
> > -               div = 8;
> > -               break;
> > +       u32 int_pllclk;
> > +       u8 div;
> > +
> > +       /* Find int_pllclk speed */
> > +       for (div = 0; div < 4; div++) {
> > +               int_pllclk = pclk / (1 << div);
> > +               if (int_pllclk < (50 * MHZ))
> > +                       break;
> >         }
> >
> > -       writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG(12));
> > +       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
>
> This causes a build failure on m68k with gcc version 9.5.0 (Ubuntu
> 9.5.0-1ubuntu1~22.04):
>
>   CC [M]  drivers/phy/freescale/phy-fsl-samsung-hdmi.o
> In file included from ./arch/m68k/include/asm/io_mm.h:25,
>                  from ./arch/m68k/include/asm/io.h:8,
>                  from ./include/linux/io.h:14,
>                  from ./include/linux/iopoll.h:14,
>                  from drivers/phy/freescale/phy-fsl-samsung-hdmi.c:12:
> 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_147’ declared with attribute error: FIELD_PREP:
> value too large for the field
>   542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |                                      ^
> ./arch/m68k/include/asm/raw_io.h:30:82: note: in definition of macro ‘out_8’
>    30 | #define out_8(addr,b) (void)((*(__force volatile u8 *)
> (unsigned long)(addr)) = (b))
>       |
>                   ^
> drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:2: note: in expansion
> of macro ‘writeb’
>   344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
>       |  ^~~~~~
> ././include/linux/compiler_types.h:530:2: note: in expansion of macro
> ‘__compiletime_assert’
>   530 |  __compiletime_assert(condition, msg, prefix, suffix)
>       |  ^~~~~~~~~~~~~~~~~~~~
> ././include/linux/compiler_types.h:542:2: note: in expansion of macro
> ‘_compiletime_assert’
>   542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
>       |  ^~~~~~~~~~~~~~~~~~~
> ./include/linux/build_bug.h:39:37: note: in expansion of macro
> ‘compiletime_assert’
>    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
>       |                                     ^~~~~~~~~~~~~~~~~~
> ./include/linux/bitfield.h:68:3: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
>    68 |   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
>       |   ^~~~~~~~~~~~~~~~
> ./include/linux/bitfield.h:115:3: note: in expansion of macro ‘__BF_FIELD_CHECK’
>   115 |   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
>       |   ^~~~~~~~~~~~~~~~
> drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:9: note: in expansion
> of macro ‘FIELD_PREP’
>   344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
>       |         ^~~~~~~~~~
>
> As it builds fine on i386, I looked at the preprocessed source files,
> but didn't see any differences that could explain this.
>
> I changed cross-compiler to gcc version 10.5.0 (Ubuntu 10.5.0-1ubuntu1~22.04),
> and that fixed the issue on m68k.
> I changed the native compiler to gcc-9, and the build started failing
> on i386 and x86_64, too....

I use the default compiler on Ubuntu 24.04.
Do you think it's best that I just replace the FIELD_PREP  macro with
manual code for that line?

adam
>
> >
> >         /*
> >          * Calculation for the frequency lock detector target code (fld_tg_code)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH V2 3/3] phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation

[Geert Uytterhoeven]

Hi Adam,

On Fri, Dec 13, 2024 at 3:57 PM Adam Ford <aford173@gmail.com> wrote:
> On Fri, Dec 13, 2024 at 8:13 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Sat, Oct 26, 2024 at 3:21 PM Adam Ford <aford173@gmail.com> wrote:
> > > Currently, the calcuation for fld_tg_code is based on a lookup table,
> >
> > calculation (everywhere)
> >
> > > but there are gaps in the lookup table, and frequencies in these
> > > gaps may not properly use the correct divider.  Based on the description
> > > of FLD_CK_DIV, the internal PLL frequency should be less than 50 MHz,
> > > so directly calcuate the value of FLD_CK_DIV from pixclk.
> > > This allow for proper calcuation of any pixel clock and eliminates a
> > > few gaps in the LUT.
> > >
> > > Since the value of the int_pllclk is in Hz, do the fixed-point
> > > math in Hz to achieve a more accurate value and reduces the complexity
> > > of the caluation to 24MHz * (256 / int_pllclk).
> > >
> > > Fixes: 6ad082bee902 ("phy: freescale: add Samsung HDMI PHY")
> > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> >
> > Thanks for your patch, which is now commit d567679f2b6a8bce ("phy:
> > freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation") in
> > next-20241209 and later.
> >
> > > --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > > +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > > @@ -331,25 +331,17 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
> > >  {
> > >         u32 pclk = cfg->pixclk;
> > >         u32 fld_tg_code;
> > > -       u32 pclk_khz;
> > > -       u8 div = 1;
> > > -
> > > -       switch (cfg->pixclk) {
> > > -       case  22250000 ...  47500000:
> > > -               div = 1;
> > > -               break;
> > > -       case  50349650 ...  99000000:
> > > -               div = 2;
> > > -               break;
> > > -       case 100699300 ... 198000000:
> > > -               div = 4;
> > > -               break;
> > > -       case 205000000 ... 297000000:
> > > -               div = 8;
> > > -               break;
> > > +       u32 int_pllclk;
> > > +       u8 div;
> > > +
> > > +       /* Find int_pllclk speed */
> > > +       for (div = 0; div < 4; div++) {
> > > +               int_pllclk = pclk / (1 << div);
> > > +               if (int_pllclk < (50 * MHZ))
> > > +                       break;
> > >         }
> > >
> > > -       writeb(FIELD_PREP(REG12_CK_DIV_MASK, ilog2(div)), phy->regs + PHY_REG(12));
> > > +       writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
> >
> > This causes a build failure on m68k with gcc version 9.5.0 (Ubuntu
> > 9.5.0-1ubuntu1~22.04):
> >
> >   CC [M]  drivers/phy/freescale/phy-fsl-samsung-hdmi.o
> > In file included from ./arch/m68k/include/asm/io_mm.h:25,
> >                  from ./arch/m68k/include/asm/io.h:8,
> >                  from ./include/linux/io.h:14,
> >                  from ./include/linux/iopoll.h:14,
> >                  from drivers/phy/freescale/phy-fsl-samsung-hdmi.c:12:
> > 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_147’ declared with attribute error: FIELD_PREP:
> > value too large for the field
> >   542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >       |                                      ^
> > ./arch/m68k/include/asm/raw_io.h:30:82: note: in definition of macro ‘out_8’
> >    30 | #define out_8(addr,b) (void)((*(__force volatile u8 *)
> > (unsigned long)(addr)) = (b))
> >       |
> >                   ^
> > drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:2: note: in expansion
> > of macro ‘writeb’
> >   344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
> >       |  ^~~~~~
> > ././include/linux/compiler_types.h:530:2: note: in expansion of macro
> > ‘__compiletime_assert’
> >   530 |  __compiletime_assert(condition, msg, prefix, suffix)
> >       |  ^~~~~~~~~~~~~~~~~~~~
> > ././include/linux/compiler_types.h:542:2: note: in expansion of macro
> > ‘_compiletime_assert’
> >   542 |  _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >       |  ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:39:37: note: in expansion of macro
> > ‘compiletime_assert’
> >    39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> >       |                                     ^~~~~~~~~~~~~~~~~~
> > ./include/linux/bitfield.h:68:3: note: in expansion of macro ‘BUILD_BUG_ON_MSG’
> >    68 |   BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?  \
> >       |   ^~~~~~~~~~~~~~~~
> > ./include/linux/bitfield.h:115:3: note: in expansion of macro ‘__BF_FIELD_CHECK’
> >   115 |   __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \
> >       |   ^~~~~~~~~~~~~~~~
> > drivers/phy/freescale/phy-fsl-samsung-hdmi.c:344:9: note: in expansion
> > of macro ‘FIELD_PREP’
> >   344 |  writeb(FIELD_PREP(REG12_CK_DIV_MASK, div), phy->regs + PHY_REG(12));
> >       |         ^~~~~~~~~~
> >
> > As it builds fine on i386, I looked at the preprocessed source files,
> > but didn't see any differences that could explain this.
> >
> > I changed cross-compiler to gcc version 10.5.0 (Ubuntu 10.5.0-1ubuntu1~22.04),
> > and that fixed the issue on m68k.
> > I changed the native compiler to gcc-9, and the build started failing
> > on i386 and x86_64, too....
>
> I use the default compiler on Ubuntu 24.04.
> Do you think it's best that I just replace the FIELD_PREP  macro with
> manual code for that line?

I think let's wait and see...
Perhaps this (compiler bug?) can be worked around in the
FIELD_PREP() macro itself.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Pei Xiao]

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));
 
 	/*
 	 * Calculation for the frequency lock detector target code (fld_tg_code)
-- 
2.25.1

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

[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);

adam
>         /*
>          * Calculation for the frequency lock detector target code (fld_tg_code)
> --
> 2.25.1
>

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

[Pei Xiao]

在 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
>>

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

[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.

adam
>
> thanks!
>
> Pei.
>
> >
> > adam
> >>          /*
> >>           * Calculation for the frequency lock detector target code (fld_tg_code)
> >> --
> >> 2.25.1
> >>

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

[Pei Xiao]

在 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
>>>>

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

[Pei Xiao]

FIELD_PREP() checks that a value 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__)

REG12_CK_DIV_MASK only two bit, add logical AND to limit range 0~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>
---
V2: change to use logical AND
---
 drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
index 5eac70a1e858..7b70ea7b1599 100644
--- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
+++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
@@ -341,7 +341,8 @@ 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));
+	/*REG12_CK_DIV_MASK only two bit, logical AND to limit range  */
+	writeb(FIELD_PREP(REG12_CK_DIV_MASK, div & 0x03), phy->regs + PHY_REG(12));
 
 	/*
 	 * Calculation for the frequency lock detector target code (fld_tg_code)
-- 
2.25.1

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

[Dominique Martinet]

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,
-- 
Dominique

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

[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);

> (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

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

[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)); 
>> (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

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

[Pei Xiao]

在 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

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

[Adam Ford]

On Thu, Jan 9, 2025 at 2:45 AM Pei Xiao <xiaopei01@kylinos.cn> wrote:
>
>
> 在 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?

Sorry for the delayed responses, I was traveling for the last week.

What about replacing the for loop with a do-while loop where the
criteria is int_pllclk < (50 * MHZ) && div <= 3.  Might that work?

adam


>
> 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

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

[Geert Uytterhoeven]

Hi Adam,

On Thu, Jan 9, 2025 at 4:03 PM Adam Ford <aford173@gmail.com> wrote:
> On Thu, Jan 9, 2025 at 2:45 AM Pei Xiao <xiaopei01@kylinos.cn> wrote:
> > 在 2025/1/3 09:34, Pei Xiao 写道:
> > > 在 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?
>
> Sorry for the delayed responses, I was traveling for the last week.
>
> What about replacing the for loop with a do-while loop where the
> criteria is int_pllclk < (50 * MHZ) && div <= 3.  Might that work?

Such a loop might never complete, although in practice that does not
seem to be possible (until someone modifies the tables in the driver
and adds a too-large cfg->pixclk?).

Probably the safest solution is to change the return type of
fsl_samsung_hdmi_phy_configure_pll_lock_det() from void to int, and
let it return an error code:

    if (div == 4)
            return -EINVAL;

 and propagate that. Its sole caller already returns an error code.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Adam Ford]

On Fri, Jan 10, 2025 at 4:04 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Adam,
>
> On Thu, Jan 9, 2025 at 4:03 PM Adam Ford <aford173@gmail.com> wrote:
> > On Thu, Jan 9, 2025 at 2:45 AM Pei Xiao <xiaopei01@kylinos.cn> wrote:
> > > 在 2025/1/3 09:34, Pei Xiao 写道:
> > > > 在 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?
> >
> > Sorry for the delayed responses, I was traveling for the last week.
> >
> > What about replacing the for loop with a do-while loop where the
> > criteria is int_pllclk < (50 * MHZ) && div <= 3.  Might that work?
>
> Such a loop might never complete, although in practice that does not
> seem to be possible (until someone modifies the tables in the driver
> and adds a too-large cfg->pixclk?).
>
> Probably the safest solution is to change the return type of
> fsl_samsung_hdmi_phy_configure_pll_lock_det() from void to int, and
> let it return an error code:
>
>     if (div == 4)
>             return -EINVAL;
>
>  and propagate that. Its sole caller already returns an error code.

That works for me.

Pei,

Do you want me to submit a patch, or did you want to do it?  I don't
know how to replicate the compiler error, so I can't verify if this
would fix it or not.

adam
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH V2 1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range

[Adam Ford]

On Sat, Oct 26, 2024 at 8:20 AM Adam Ford <aford173@gmail.com> wrote:
>
> The Integer divder uses values of P,M, and S to determine the PLL
> rate.  Currently, the range of M was set based on a series of
> table entries where the range was limited.  Since the ref manual
> shows it is 8-bit wide, expand the range to be up to 255.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>
> ---
> V2:  Fix typo in comment

Vinod,

Is there any chance this series could also be applied for the next
release?  I don't know what the cutoff deadline is, so if I'm too
late, I apologize.

adam
>
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 2c8038864357..412c03b7dcd6 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -406,16 +406,15 @@ static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u1
>                                 continue;
>
>                         /*
> -                        * TODO: Ref Manual doesn't state the range of _m
> -                        * so this should be further refined if possible.
> -                        * This range was set based on the original values
> -                        * in the lookup table
> +                        * The Ref manual doesn't explicitly state the range of M,
> +                        * but it does show it as an 8-bit value, so reject
> +                        * any value above 255.
>                          */
>                         tmp = (u64)fout * (_p * _s);
>                         do_div(tmp, 24 * MHZ);
> -                       _m = tmp;
> -                       if (_m < 0x30 || _m > 0x7b)
> +                       if (tmp > 255)
>                                 continue;
> +                       _m = tmp;
>
>                         /*
>                          * Rev 2 of the Ref Manual states the
> --
> 2.45.2
>

Re: [PATCH V2 1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range

[Adam Ford]

On Sat, Oct 26, 2024 at 8:20 AM Adam Ford <aford173@gmail.com> wrote:
>
> The Integer divder uses values of P,M, and S to determine the PLL
> rate.  Currently, the range of M was set based on a series of
> table entries where the range was limited.  Since the ref manual
> shows it is 8-bit wide, expand the range to be up to 255.
>
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@kontron.de>

Vinod,

Do you have any feedback on this series?  RC1 is available, and it
would be nice to see this series merged soon unless you have changes
you want implemented.

Thank you,

adam
> ---
> V2:  Fix typo in comment
>
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 2c8038864357..412c03b7dcd6 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -406,16 +406,15 @@ static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u1
>                                 continue;
>
>                         /*
> -                        * TODO: Ref Manual doesn't state the range of _m
> -                        * so this should be further refined if possible.
> -                        * This range was set based on the original values
> -                        * in the lookup table
> +                        * The Ref manual doesn't explicitly state the range of M,
> +                        * but it does show it as an 8-bit value, so reject
> +                        * any value above 255.
>                          */
>                         tmp = (u64)fout * (_p * _s);
>                         do_div(tmp, 24 * MHZ);
> -                       _m = tmp;
> -                       if (_m < 0x30 || _m > 0x7b)
> +                       if (tmp > 255)
>                                 continue;
> +                       _m = tmp;
>
>                         /*
>                          * Rev 2 of the Ref Manual states the
> --
> 2.45.2
>

Re: [PATCH V2 1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range

[Vinod Koul]

On Sat, 26 Oct 2024 08:19:57 -0500, Adam Ford wrote:
> The Integer divder uses values of P,M, and S to determine the PLL
> rate.  Currently, the range of M was set based on a series of
> table entries where the range was limited.  Since the ref manual
> shows it is 8-bit wide, expand the range to be up to 255.
> 
> 

Applied, thanks!

[1/3] phy: freescale: fsl-samsung-hdmi: Expand Integer divider range
      commit: 2a9868d69be26e623dd0bf4231d5175f0ccf5d6f
[2/3] phy: freescale: fsl-samsung-hdmi: Stop searching when exact match is found
      commit: 1b9b8b159601d174526ce1c3a62ebe3a7286003b
[3/3] phy: freescale: fsl-samsung-hdmi: Clean up fld_tg_code calculation
      commit: d567679f2b6a8bcea20589bbea6488c0236886cd

Best regards,
-- 
~Vinod