Re: [PATCH v5 03/21] drivers: crypto: meson: make CLK controller optional

[Alexey Romanov]

On Mon, Mar 04, 2024 at 01:49:46PM +0000, Alexey Romanov wrote:
> Hello Jerome,
> 
> On Fri, Mar 01, 2024 at 04:21:20PM +0100, Jerome Brunet wrote:
> > 
> > On Fri 01 Mar 2024 at 16:29, Alexey Romanov <avromanov@xxxxxxxxxxxxxxxxx> wrote:
> > 
> > > Amlogic crypto IP doesn't take a clock input on some
> > > SoCs: AXG / A1 / S4 / G12. So make it optional.
> > >
> > 
> > I commented this patch on v2 and the comment keep on being un-addressed.
> > 
> > The SoC either:
> > * has a clock that is required for the IP to work
> > * Or does not
> > 
> > It is not something you are free to provide or not.
> > 
> > For the record, I find very hard believe that some SoC would have clock,
> > and other would not, for the same HW.
> > 
> > Isn't it more likely that the clock just happens to be left enabled by
> > the bootloader on some SoC and it conviently allows to ignore it ?
> 
> 
> S905X and newer SoC's uses DMA engine for crypto HW and they
> don't required clock input to work. Clock input is needed for
> blkmv engine.
> 
> Therefore, I'm not sure that it is needed for GXL too (and the second
> interrupt line). I tested it on vim1 board witouht them and everything
> works correctly.

Amlogic says that the crypto HW based on DMA engine doesn't require
a clock input. GXL uses DMA engine, so, I think we have to remove
whole clock controller calls in the next series from driver/dts/bindings.
And the second interrupt line from crypto node in meson-gxl.dtsi too.

> 
> > 
> > > Signed-off-by: Alexey Romanov <avromanov@xxxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/crypto/amlogic/amlogic-gxl-core.c | 14 +++-----------
> > >  1 file changed, 3 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/crypto/amlogic/amlogic-gxl-core.c b/drivers/crypto/amlogic/amlogic-gxl-core.c
> > > index e9e733ed98e0..a3a69a59f476 100644
> > > --- a/drivers/crypto/amlogic/amlogic-gxl-core.c
> > > +++ b/drivers/crypto/amlogic/amlogic-gxl-core.c
> > > @@ -269,16 +269,11 @@ static int meson_crypto_probe(struct platform_device *pdev)
> > >  		dev_err(&pdev->dev, "Cannot request MMIO err=%d\n", err);
> > >  		return err;
> > >  	}
> > > -	mc->busclk = devm_clk_get(&pdev->dev, "blkmv");
> > > +
> > > +	mc->busclk = devm_clk_get_optional_enabled(&pdev->dev, "blkmv");
> > >  	if (IS_ERR(mc->busclk)) {
> > >  		err = PTR_ERR(mc->busclk);
> > > -		dev_err(&pdev->dev, "Cannot get core clock err=%d\n", err);
> > > -		return err;
> > > -	}
> > > -
> > > -	err = clk_prepare_enable(mc->busclk);
> > > -	if (err != 0) {
> > > -		dev_err(&pdev->dev, "Cannot prepare_enable busclk\n");
> > > +		dev_err(&pdev->dev, "Cannot get and enable core clock err=%d\n", err);
> > >  		return err;
> > >  	}
> > >  
> > > @@ -306,7 +301,6 @@ static int meson_crypto_probe(struct platform_device *pdev)
> > >  	meson_unregister_algs(mc);
> > >  error_flow:
> > >  	meson_free_chanlist(mc, mc->flow_cnt - 1);
> > > -	clk_disable_unprepare(mc->busclk);
> > >  	return err;
> > >  }
> > >  
> > > @@ -321,8 +315,6 @@ static void meson_crypto_remove(struct platform_device *pdev)
> > >  	meson_unregister_algs(mc);
> > >  
> > >  	meson_free_chanlist(mc, mc->flow_cnt - 1);
> > > -
> > > -	clk_disable_unprepare(mc->busclk);
> > >  }
> > >  
> > >  static const struct meson_pdata meson_gxl_pdata = {
> > 
> > 
> > -- 
> > Jerome
> 
> -- 
> Thank you,
> Alexey

-- 
Thank you,
Alexey