| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHCN7xJL-aS+kFp2YwtSSUUMoTBqZCsXb0qvCpYQGpJVM_qJYg@mail.gmail.com>
Date: Mon, 2 Sep 2024 16:20:11 -0500
From: Adam Ford <aford173@...il.com>
To: Dominique Martinet <dominique.martinet@...ark-techno.com>
Cc: linux-phy@...ts.infradead.org, linux-imx@....com, festevam@...il.com,
frieder.schrempf@...tron.de, aford@...conembedded.com,
Vinod Koul <vkoul@...nel.org>, Kishon Vijay Abraham I <kishon@...nel.org>,
Marco Felsch <m.felsch@...gutronix.de>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Lucas Stach <l.stach@...gutronix.de>, linux-kernel@...r.kernel.org,
Makoto Sato <makoto.sato@...ark-techno.com>
Subject: Re: [RFC V3 3/3] phy: freescale: fsl-samsung-hdmi: Support dynamic integer
On Fri, Aug 30, 2024 at 12:56 AM Dominique Martinet
<dominique.martinet@...ark-techno.com> wrote:
>
> Adam Ford wrote on Thu, Aug 29, 2024 at 10:24:27PM -0500:
> > 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
> > 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.
> >
> > 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.
>
> Thank you!
>
> I've updated to v3 and we must have missed something fiddling with v1
> but our 31.5MHz-only screen turns on with this!
>
> Unfortunately among the other odd devices we support, there's one
> whose native resolution only supports 83.5MHz, and that doesn't come out
> right with the integer divider (that function returns 83.2MHz, which is
> 0.4%off)
> If we force the round/set rate functions to prefer the calculated value
> and allow that in imx8mp_hdmi_check_clk_rate (dw_hdmi-imx.c) then it
> also works, so I don't think we actually need to affine the model...
> Coming back to what Lucas replied to my initial mail HDMI would also a
> rate mismatch of ±0.5%, so the integer calculator works for all the
> frequencies we've currently added manually if we fix the check to allow
> that as well:
> 32000000: found 32000000 (100.0% match): p 1 / m 80 / s 12
> 51200000: found 51200000 (100.0% match): p 1 / m 64 / s 6
> 65000000: found 64800000 (99.6% match): p 1 / m 54 / s 4
> 71000000: found 70800000 (99.7% match): p 1 / m 59 / s 4
> 83500000: found 83200000 (99.6% match): p 1 / m 104 / s 6
>
> (only actually tested 51.2 and 83.5 here, we don't have all the hardware
> available; I'll try to play with normal monitors that support more modes
> once the patch gets further finalized)
>
>
> So, as far as I'm concerned I'd be happy to move forward with that and
> will backport this to our tree/remove our kludged values, would "just"
> need to properly pick the closest value if no exact match instead of
> always falling back to the table (or just remove the table altogether if
> we can test a few monitors?)
>
> A couple of style nitpicks below
>
> > diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> > index a700a300dc6f..3fab40cde40d 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)
> > #define REG14_TOL_MASK GENMASK(7, 4)
> > @@ -29,281 +31,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,
> > + * meaning the last four entries will be fixed, but the first three will
> > + * be calculated by the PMS calculator
> > + */
> > +static struct phy_config calculated_phy_pll_cfg = {
>
> I'd change cur_cfg from pointer to the struct itself like this (partial
> patch that probably won't even apply on your branch:)
> ----
> diff --git a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> index 9048cdc760c2..d7124604819c 100644
> --- a/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> +++ b/drivers/phy/freescale/phy-fsl-samsung-hdmi.c
> @@ -402,7 +402,7 @@ struct fsl_samsung_hdmi_phy {
>
> /* clk provider */
> struct clk_hw hw;
> - const struct phy_config *cur_cfg;
> + struct phy_config cur_cfg;
Wouldn't converting this from a pointer require me to do a memcpy
later? It seems like that's more work than just pointing it to an
address.
> };
>
> static inline struct fsl_samsung_hdmi_phy *
> @@ -562,9 +562,9 @@ static int phy_clk_set_rate(struct clk_hw *hw,
> if (i < 0)
> return -EINVAL;
>
> - phy->cur_cfg = &phy_pll_cfg[i];
> + phy->cur_cfg = phy_pll_cfg[i];
I think this is would have to be a memcpy instead of just an equal
statement since phy->cur_cfg would no longer be a pointer.
>
> - return fsl_samsung_hdmi_phy_configure(phy, phy->cur_cfg);
> + return fsl_samsung_hdmi_phy_configure(phy, &phy->cur_cfg);
> }
>
> static const struct clk_ops phy_clk_ops = {
> ----
>
> Then you can just set it directly for calculated values.
> But conceptually it's the same, just one less indirection.
>
> > @@ -406,6 +424,76 @@ fsl_samsung_hdmi_phy_configure_pll_lock_det(struct fsl_samsung_hdmi_phy *phy,
> > phy->regs + PHY_REG(14));
> > }
> >
> > +static unsigned long fsl_samsung_hdmi_phy_find_pms(unsigned long fout, u8 *p, u16 *m, u8 *s)
> > +{
> > + unsigned long best_freq = 0;
> > + u32 min_delta = 0xffffffff;
> > + u8 _p, best_p;
> > + u16 _m, best_m;
> > + u8 _s, best_s;
> > +
> > + /* The ref manual states the values of 'P' rannge from 1 to 11 */
>
> typo: range
Thanks. I can fix.
>
> > + for (_p = 1; _p <= 11; ++_p) {
> > + for (_s = 1; _s <= 16; ++_s) {
> > + u64 tmp;
> > + u32 delta;
> > +
> > + /* s must be one or even */
> > + if (_s > 1 && (_s & 0x01) == 1)
> > + _s++;
> > +
> > + /* _s cannot be 14 per the TRM */
> > + if (_s == 14)
> > + 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 look-up table
> > + */
> > + tmp = (u64)fout * (_p * _s);
> > + do_div(tmp, 24 * MHZ);
> > + _m = tmp;
> > + if (_m < 0x30 || _m > 0x7b)
> > + continue;
> > +
> > + /*
> > + * Rev 2 of the Ref Manual states the
> > + * VCO can range between 750MHz and
> > + * 3GHz. The VCO is assumed to be _m x
> > + * the reference frequency of 24MHz divided
> > + * by the prescaler, _p
> > + */
> > + 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);
> > +
> > + delta = abs(fout - tmp);
> > + if (delta < min_delta) {
> > + best_p = _p;
> > + best_s = _s;
> > + best_m = _m;
> > + min_delta = delta;
> > + best_freq = tmp;
> > + }
> > + }
> > + }
> > +
> > + if (best_freq) {
> > + *p = best_p;
> > + *m = best_m;
> > + *s = best_s;
> > + }
> > +
> > + return best_freq;
> > +}
> > +
> > static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
> > const struct phy_config *cfg)
> > {
> > @@ -419,13 +507,13 @@ static int fsl_samsung_hdmi_phy_configure(struct fsl_samsung_hdmi_phy *phy,
> > for (i = 0; i < ARRAY_SIZE(common_phy_cfg); i++)
> > writeb(common_phy_cfg[i].val, phy->regs + common_phy_cfg[i].reg);
> >
> > - /* set individual PLL registers PHY_REG2 ... PHY_REG7 */
> > + /* set individual PLL registers PHY_REG1 ... PHY_REG7 */
> > for (i = 0; i < PHY_PLL_DIV_REGS_NUM; i++)
> > - writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(2) + i * 4);
> > + writeb(cfg->pll_div_regs[i], phy->regs + PHY_REG(1) + i * 4);
> >
> > - /* High nibble of pll_div_regs[1] contains S which also gets written to REG21 */
> > + /* High nibble of PHY_REG3 and low nibble of PHY_REG21 both contain 'S' */
> > writeb(REG21_SEL_TX_CK_INV | FIELD_PREP(REG21_PMS_S_MASK,
> > - cfg->pll_div_regs[1] >> 4), phy->regs + PHY_REG(21));
> > + cfg->pll_div_regs[2] >> 4), phy->regs + PHY_REG(21));
> >
> > fsl_samsung_hdmi_phy_configure_pll_lock_det(phy, cfg);
> >
> > @@ -453,29 +541,70 @@ static unsigned long phy_clk_recalc_rate(struct clk_hw *hw,
> > static long phy_clk_round_rate(struct clk_hw *hw,
> > unsigned long rate, unsigned long *parent_rate)
> > {
> > + u32 int_div_clk;
> > int i;
> > + u16 m;
> > + u8 p, s;
> > +
> > + /* If the clock is out of range return error instead of searching */
> > + if (rate > 297000000 || rate < 22250000)
> > + return -EINVAL;
> >
> > + /* Check the look-up table */
> > for (i = ARRAY_SIZE(phy_pll_cfg) - 1; i >= 0; i--)
> > if (phy_pll_cfg[i].pixclk <= rate)
> > - return phy_pll_cfg[i].pixclk;
> > + break;
> > + /* If the rate is an exact match, return it now */
> > + if (rate == phy_pll_cfg[i].pixclk)
> > + return phy_pll_cfg[i].pixclk;
> > +
> > + /*
> > + * The math on the lookup table shows the PMS math yields a
> > + * frequency 5 x pixclk.
> > + * When we check the integer divider against the desired rate,
> > + * multiply the rate x 5 and then divide the outcome by 5.
> > + */
> > + int_div_clk = fsl_samsung_hdmi_phy_find_pms(rate * 5, &p, &m, &s) / 5;
>
> I'd move that comment and both multiplication and division inside
> fsl_samsung_hdmi_phy_find_pms, since it's a property of the computation
> (not having the comment made me ask last time, with the comment it's
> fine -- thanks for adding these comments, very helpful.)
>
> --
> Dominique
>
>
Powered by blists - more mailing lists