Messages in this thread Patch in this message | | | Date | Fri, 4 Mar 2022 16:01:58 +0100 | Subject | Re: [PATCH v2 11/18] crypto: rockhip: do not handle dma clock | From | Johan Jonker <> |
| |
Hi Corentin,
Make your clock driver parsing portable.
=== oneOf: - const: rockchip,rk3288-crypto - items: - enum: - rockchip,rk3328-crypto - const: rockchip,rk3288-crypto
Compatible string must be SoC related!
rk3288 was the first in line that had support, so we use that as fall back string.
===
Make binding fit for more SoC types. Allow more clocks by using devm_clk_bulk_get_all. Drop reset-names requirement for devm_reset_control_array_get_exclusive.
===
Use a patch order to prevent the scripts generate notifications.
- dt-bindings conversion
- add rk3328 compatible string in a separate patch
- your driver changes
- dts patches
A proposed maintainer must be able to submit patch series without errors. ;)
===
When you remove a clock in a YAML conversion you must add a note to the DT maintainer.
===
Johan
On 3/2/22 22:11, Corentin Labbe wrote: > The DMA clock is handled by the DMA controller, so the crypto does not > have to touch it. > > Signed-off-by: Corentin Labbe <clabbe@baylibre.com> > --- > drivers/crypto/rockchip/rk3288_crypto.c | 16 +--------------- > drivers/crypto/rockchip/rk3288_crypto.h | 1 - > 2 files changed, 1 insertion(+), 16 deletions(-) > > diff --git a/drivers/crypto/rockchip/rk3288_crypto.c b/drivers/crypto/rockchip/rk3288_crypto.c > index 94ef1283789f..645855d2651b 100644 > --- a/drivers/crypto/rockchip/rk3288_crypto.c > +++ b/drivers/crypto/rockchip/rk3288_crypto.c > @@ -40,15 +40,8 @@ static int rk_crypto_enable_clk(struct rk_crypto_info *dev) > __func__, __LINE__); > goto err_hclk; > } > - err = clk_prepare_enable(dev->dmaclk); > - if (err) { > - dev_err(dev->dev, "[%s:%d], Couldn't enable clock dmaclk\n", > - __func__, __LINE__); > - goto err_dmaclk; > - } > + > return err; > -err_dmaclk: > - clk_disable_unprepare(dev->hclk); > err_hclk: > clk_disable_unprepare(dev->aclk); > err_aclk: > @@ -59,7 +52,6 @@ static int rk_crypto_enable_clk(struct rk_crypto_info *dev) > > static void rk_crypto_disable_clk(struct rk_crypto_info *dev) > { > - clk_disable_unprepare(dev->dmaclk); > clk_disable_unprepare(dev->hclk); > clk_disable_unprepare(dev->aclk); > clk_disable_unprepare(dev->sclk); > @@ -199,12 +191,6 @@ static int rk_crypto_probe(struct platform_device *pdev) > goto err_crypto; > } >
> - crypto_info->dmaclk = devm_clk_get(&pdev->dev, "apb_pclk"); > - if (IS_ERR(crypto_info->dmaclk)) { > - err = PTR_ERR(crypto_info->dmaclk); > - goto err_crypto; > - } > -
rk3288: clocks = <&cru ACLK_CRYPTO>, <&cru HCLK_CRYPTO>, - <&cru SCLK_CRYPTO>, <&cru ACLK_DMAC1>; - clock-names = "aclk", "hclk", "sclk", "apb_pclk"; + <&cru SCLK_CRYPTO>; + clock-names = "aclk", "hclk", "sclk";
rk3328: + clocks = <&cru HCLK_CRYPTO_MST>, <&cru HCLK_CRYPTO_SLV>, + <&cru SCLK_CRYPTO>; + clock-names = "aclk", "hclk", "sclk";
The HCLK_CRYPTO_MST not is related to ACLK_CRYPTO. You are reusing rk3288 names to not to change the driver. Give it an other name.
===
The sclk goes through a crypto_div_con. Does that need a frequency set? Or does that come from nowhere?
From crypto_v1.c priv->frequency = dev_read_u32_default(dev, "clock-frequency", CRYPTO_V1_DEFAULT_RATE);
ret = clk_set_rate(&priv->sclk, priv->frequency);
===
Could you make this portable? Example:
int i;
priv->num_clks = devm_clk_bulk_get_all(dev, &priv->clks); if (priv->num_clks < 1) return -EINVAL;
priv->sclk = NULL; for (i = 0; i < priv->num_clks; i++) { if (!strncmp(priv->clks[i].id, "sclk", 3)) { priv->sclk = priv->clks[i].clk; break; } }
if (!priv->sclk) { dev_err(dev, "no sclk found\n"); return -EINVAL; }
Also add optional "sclk1" clock for rk3399. Use "sclk" and not "sclk0" to be backwards compatible.
===
Also make the resets portable for rk3399. Remove the requirement for "reset-names".
Example: priv->phy_rst = devm_reset_control_array_get_exclusive(dev); if (IS_ERR(priv->phy_rst)) return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
> crypto_info->irq = platform_get_irq(pdev, 0); > if (crypto_info->irq < 0) { > dev_err(&pdev->dev, "control Interrupt is not available.\n"); > diff --git a/drivers/crypto/rockchip/rk3288_crypto.h b/drivers/crypto/rockchip/rk3288_crypto.h > index c741e97057dc..963fbfc4d14e 100644 > --- a/drivers/crypto/rockchip/rk3288_crypto.h > +++ b/drivers/crypto/rockchip/rk3288_crypto.h > @@ -191,7 +191,6 @@ struct rk_crypto_info {
> struct clk *aclk; > struct clk *hclk; > struct clk *sclk; > - struct clk *dmaclk;
int num_clks; struct clk_bulk_data *clks; struct clk *sclk; struct clk *sclk1;
> struct reset_control *rst; > void __iomem *reg; > int irq;
| |