Message-ID: <a40d8846-6a6b-4da0-b7d0-dbea1fa76c63@kontron.de>
Date: Wed, 4 Sep 2024 16:09:39 +0200
From: Frieder Schrempf <frieder.schrempf@...tron.de>
To: Adam Ford <aford173@...il.com>
Cc: linux-phy@...ts.infradead.org, dominique.martinet@...ark-techno.com,
 linux-imx@....com, festevam@...il.com, aford@...conembedded.com,
 Sandor.yu@....com, Vinod Koul <vkoul@...nel.org>,
 Kishon Vijay Abraham I <kishon@...nel.org>,
 Marco Felsch <m.felsch@...gutronix.de>, Lucas Stach
 <l.stach@...gutronix.de>, Uwe Kleine-König
 <u.kleine-koenig@...gutronix.de>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V5 3/5] phy: freescale: fsl-samsung-hdmi: Support dynamic
 integer

On 04.09.24 4:05 PM, Adam Ford wrote:
> On Wed, Sep 4, 2024 at 9:00 AM Frieder Schrempf
> <frieder.schrempf@...tron.de> wrote:
>>
>> On 04.09.24 3:52 PM, Adam Ford wrote:
>>> On Wed, Sep 4, 2024 at 8:35 AM Frieder Schrempf
>>> <frieder.schrempf@...tron.de> wrote:
>>>>
>>>> On 04.09.24 4:32 AM, Adam Ford wrote:
>>>>> There is currently a look-up table for a variety of resolutions.
>>>>> Since the phy has the ability to dynamically calculate the values
>>>>> necessary to use the intger divider which should allow more
>>>>
>>>>                        ^ integer
>>>>
>>>>> resolutions without having to update the look-up-table.
>>>>>
>>>>> If the lookup table cannot find an exact match, fall back to the
>>>>> dynamic calculator of the integer divider.
>>>>
>>>> Nitpick: You have thre different versions of how to spell "lookup table"
>>>> in the paragraphs above. Maybe you can decide on one... ;)
>>>>
>>>>>
>>>>> Previously, the value of P was hard-coded to 1, this required an
>>>>> update to the phy_pll_cfg table to add in the extra value into the
>>>>> table, so if the value of P is calculated to be something else
>>>>> by the PMS calculator, the calculated_phy_pll_cfg structure
>>>>> can be used instead without having to keep track of which method
>>>>> was used.
>>>>>
>>>>> Signed-off-by: Adam Ford <aford173@...il.com>
>>>>
>>>> The comments I have are only nitpicks and the patch seems to work fine
>>>> for me. So feel free to add:
>>>>
>>>> Reviewed-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>>>> Tested-by: Frieder Schrempf <frieder.schrempf@...tron.de>
>>>>
>>>>> ---
>>>>> V5:  No Change
>>>>> V4:  No Change
>>>>> V3:  Change size of pll_div_regs to include PHY_REG01 (P)
>>>>>      Create calculated_phy_pll_cfg to containe the values
>>>>>      Eliminate the PMS calculation from fsl_samsung_hdmi_phy_configure
>>>>>      Make the LUT primary and fall back to integer calculator in
>>>>>      phy_clk_round_rate.
>>>>>      Check the range right away to ensure it's reaonsable rather than
>>>>>      trying to find a clock only to learn it's outside the range.
>>>>>      Overall added notes and comments where stuff may not be intuitive.
>>>>>
>>>>> V2:  Update phy_clk_round_rate and phy_clk_set_rate to both support
>>>>>      the integer clock PMS calculator.
>>>>> ---
>>>>>  drivers/phy/freescale/phy-fsl-samsung-hdmi.c | 341 +++++++++++++------
>>>>>  1 file changed, 235 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>>>> index 4f6874226f9a..8f2c0082aa12 100644
>>>>> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>>>> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
>>>>> @@ -16,6 +16,8 @@
>>>>>
>>>>>  #define PHY_REG(reg)         (reg * 4)
>>>>>
>>>>> +#define REG01_PMS_P_MASK     GENMASK(3, 0)
>>>>> +#define REG03_PMS_S_MASK     GENMASK(7, 4)
>>>>>  #define REG12_CK_DIV_MASK    GENMASK(5, 4)
>>>>>
>>>>>  #define REG13_TG_CODE_LOW_MASK       GENMASK(7, 0)
>>>>> @@ -38,281 +40,296 @@
>>>>>  #define REG34_PLL_LOCK               BIT(6)
>>>>>  #define REG34_PHY_CLK_READY  BIT(5)
>>>>>
>>>>> -#define PHY_PLL_DIV_REGS_NUM 6
>>>>> +#ifndef MHZ
>>>>> +#define MHZ  (1000UL * 1000UL)
>>>>> +#endif
>>>>> +
>>>>> +#define PHY_PLL_DIV_REGS_NUM 7
>>>>>
>>>>>  struct phy_config {
>>>>>       u32     pixclk;
>>>>>       u8      pll_div_regs[PHY_PLL_DIV_REGS_NUM];
>>>>>  };
>>>>>
>>>>> +/*
>>>>> + * The calculated_phy_pll_cfg only handles integer divider for PMS only,
>>>>
>>>> Nitpick: Remove duplicate 'only'
>>>>
>>>>> + * meaning the last four entries will be fixed, but the first three will
>>>>> + * be calculated by the PMS calculator
>>>>
>>>> Nitpick: Period at the end of the sentence
>>>
>>>
>>> Good catch.  I ran these through checkpatch, but I need to tell myself
>>> not do work on this stuff at night when I am tired.
>>> Sorry about that.  My grammar isn't normally this bad.  :-)
>>
>> I know. I already assumed that you were tired as the commit messages get
>> worse towards the end of the series ;)
> 
> I do this stuff as a hobby at night after my day-job is done, so my
> brain is somewhat shot.  Most of the actual work that I do for this is
> done on weekends in the morning when I am fresh, but I do the cleanup
> at night.  I should probably reevaluate that decision.  :-)
> 
> Thanks for the understanding.  :-)

No worries. You are doing great work. You have my full understanding for
any of such flaws. :)

[...]
>>>>> +                      */
>>>>> +                     tmp = (u64)_m * 24 * MHZ;
>>>>> +                     do_div(tmp, _p);
>>>>> +                     if (tmp < 750 * MHZ ||
>>>>> +                         tmp > 3000 * MHZ)
>>>>> +                             continue;
>>>>> +
>>>>> +                     tmp = (u64)_m * 24 * MHZ;
>>>>> +                     do_div(tmp, _p * _s);
>>>>
>>>> tmp already contains (_m * f_ref) / _p, so we sould be able to reuse
>>>> that value here and simply do do_div(tmp, _s) without recalculating tmp, no?
>>
>> Any reply to this comment? Can this be optimized?
> 
> I was going to investigate it before agreeing to do it. On the
> surface, it makes sense.

Great, I just thought you may have missed it. Thanks!

[...]

Powered by blists - more mailing lists