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@xxxxxxxxx> 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@xxxxxxxxx>
> Reviewed-by: Frieder Schrempf <frieder.schrempf@xxxxxxxxxx>

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

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