Re: [PATCH v2 11/18] crypto: rockhip: do not handle dma clock

[LABBE Corentin <clabbe@baylibre.com>]

Le Fri, Mar 04, 2022 at 04:01:58PM +0100, Johan Jonker a écrit :
> 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.

Hello

Thanks for the hint of devm_clk_bulk_get_all, I will switch to it as it simplify clock handling.

> Drop reset-names requirement for devm_reset_control_array_get_exclusive.
> 
> ===
> 
> Use a patch order to prevent the scripts generate notifications.

which scripts ?

> 
> - 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.

You are right, I will change them.


> 
> ===
> 
> 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);
> 

The problem is that I dont see any hints for this in TRM, and their rockchips source are inconsistent, they do this in uboot not in linux....

> ===
> 
> 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;


For handling rk3399, I have no hardware so I cannot do anything for it easily.
I have asked on IRC for some tests, so let's see if it works.
Anyway we can always add support for it later, the priority is to fix the driver breakage.

Regards