linux-kernel - Re: [PATCH v10 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7f2bf860ffaccd10899b72fdda6a3ca9c50d2107.camel@maquefel.me>
Date: Wed, 10 Jul 2024 13:34:12 +0300
From: Nikita Shubin <nikita.shubin@...uefel.me>
To: Stephen Boyd <sboyd@...nel.org>, Michael Turquette
 <mturquette@...libre.com>,  Nikita Shubin via B4 Relay
 <devnull+nikita.shubin.maquefel.me@...nel.org>
Cc: linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org, Arnd Bergmann
	 <arnd@...db.de>
Subject: Re: [PATCH v10 03/38] clk: ep93xx: add DT support for Cirrus EP93xx

Hello Stephen!

On Mon, 2024-07-08 at 15:18 -0700, Stephen Boyd wrote:
> Quoting Nikita Shubin via B4 Relay (2024-06-17 02:36:37)
> > diff --git a/drivers/clk/clk-ep93xx.c b/drivers/clk/clk-ep93xx.c
> > new file mode 100644
> > index 000000000000..a0430a5ae4da
> > --- /dev/null
> > +++ b/drivers/clk/clk-ep93xx.c
> > @@ -0,0 +1,834 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> [...]
> > +
> > +static int ep93xx_clk_enable(struct clk_hw *hw)
> > +{
> > +       struct ep93xx_clk *clk = ep93xx_clk_from(hw);
> > +       struct ep93xx_clk_priv *priv = ep93xx_priv_from(clk);
> > +       u32 val;
> > +
> > +       guard(spinlock_irqsave)(&priv->lock);
> 
> I thought guard() was most important when there were multiple exit
> paths
> from a function, but OK.
> 
> > +
> [...]
> > +
> > +static int ep93xx_plls_init(struct ep93xx_clk_priv *priv)
> > +{
> > +       const char fclk_divisors[] = { 1, 2, 4, 8, 16, 1, 1, 1 };
> > +       const char hclk_divisors[] = { 1, 2, 4, 5, 6, 8, 16, 32 };
> [...]
> > +       if (!(value & EP93XX_SYSCON_CLKSET2_NBYP2))
> > +               clk_pll2_rate = EP93XX_EXT_CLK_RATE;
> > +       else if (value & EP93XX_SYSCON_CLKSET2_PLL2_EN)
> > +               clk_pll2_rate = calc_pll_rate(EP93XX_EXT_CLK_RATE,
> > value);
> > +       else
> > +               clk_pll2_rate = 0;
> > +
> > +       hw = devm_clk_hw_register_fixed_rate(dev, "pll2", "xtali",
> > 0, clk_pll2_rate);
> 
> Please use clk_parent_data for topology descriptions.

It's look like clk_hw_register_fixed_rate_parent_hw() is missing devm_
version of this function. Is it ok to provide wrapper for
__clk_hw_register_fixed_rate() in drivers/clk/clk-ep93xx.c ?

> 
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       priv->fixed[EP93XX_CLK_PLL2] = hw;
> > +
> > +       return 0;
> > +}
> > +
> > +static int ep93xx_clk_probe(struct auxiliary_device *adev,
> > +                              const struct auxiliary_device_id
> > *id)
> > +{
> > +       struct ep93xx_regmap_adev *rdev =
> > to_ep93xx_regmap_adev(adev);
> > +       struct clk_parent_data xtali = { .index = 0 };
> > +       struct clk_parent_data ddiv_pdata[3] = { };
> > +       unsigned int clk_spi_div, clk_usb_div;
> > +       struct clk_parent_data pdata = {};
> > +       struct device *dev = &adev->dev;
> > +       struct ep93xx_clk_priv *priv;
> > +       struct ep93xx_clk *clk;
> > +       struct clk_hw *hw;
> > +       unsigned int idx;
> > +       int ret;
> > +       u32 value;
> > +
> > +       priv = devm_kzalloc(dev, struct_size(priv, reg, 10),
> > GFP_KERNEL);
> > +       if (!priv)
> > +               return -ENOMEM;
> > +
> > +       spin_lock_init(&priv->lock);
> > +       priv->dev = dev;
> > +       priv->aux_dev = rdev;
> > +       priv->map = rdev->map;
> > +       priv->base = rdev->base;
> > +
> > +       ret = ep93xx_plls_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       regmap_read(priv->map, EP93XX_SYSCON_CLKSET2, &value);
> > +       clk_usb_div = (value >> 28 & GENMASK(3, 0)) + 1;
> > +       hw = devm_clk_hw_register_fixed_factor(dev, "usb_clk",
> > "pll2", 0, 1, clk_usb_div);
> 
> This one can use clk_hw to reference pll2.
> 
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       priv->fixed[EP93XX_CLK_USB] = hw;
> > +
> > +       ret = ep93xx_uart_clock_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = ep93xx_dma_clock_init(priv);
> > +       if (ret)
> > +               return ret;
> > +
> > +       clk_spi_div = id->driver_data;
> > +       hw = devm_clk_hw_register_fixed_factor(dev, "ep93xx-spi.0",
> > "xtali",
> 
> Are these clk names trying to match device names?
> 
> > +                                              0, 1, clk_spi_div);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       priv->fixed[EP93XX_CLK_SPI] = hw;
> > +
> > +       /* PWM clock */
> > +       hw = devm_clk_hw_register_fixed_factor(dev, "pwm_clk",
> > "xtali", 0, 1, 1);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +
> > +       priv->fixed[EP93XX_CLK_PWM] = hw;
> > +
> > +       /* USB clock */
> > +       hw = devm_clk_hw_register_gate(priv->dev, "ohci-platform",
> > "usb_clk",
> > +                                      0, priv->base +
> > EP93XX_SYSCON_PWRCNT,
> > +                                      EP93XX_SYSCON_PWRCNT_USH_EN,
> > 0,
> > +                                      &priv->lock);
> > +       if (IS_ERR(hw))
> > +               return PTR_ERR(hw);
> > +