On Thu, Jul 30, 2015 at 11:32:44PM -0700, Victoria Milhoan wrote:
> Hi Sascha,
>
> Thank you for the responses. Comments inline. Changes will be
> in the next version of the patch set.
>
> -Victoria
>
> On Thu, 30 Jul 2015 08:02:14 +0200
> Sascha Hauer <s.hauer@xxxxxxxxxxxxxx> wrote:
>
> > Hi Victoria,
> >
> > comments inline.
> >
> > On Wed, Jul 29, 2015 at 08:58:20PM -0700, Victoria Milhoan wrote:
> > > ARM-based systems may disable clocking to the CAAM device on the
> > > Freescale i.MX platform for power management purposes. This patch
> > > enables the required clocks when the CAAM module is initialized and
> > > disables the required clocks when the CAAM module is shut down.
> > >
> > > Signed-off-by: Victoria Milhoan <vicki.milhoan@xxxxxxxxxxxxx>
> > > ---
> > > drivers/crypto/caam/compat.h | 1 +
> > > drivers/crypto/caam/ctrl.c | 191 +++++++++++++++++++++++++++++++++++++++++++
> > > drivers/crypto/caam/intern.h | 5 ++
> > > 3 files changed, 197 insertions(+)
> > >
> > > diff --git a/drivers/crypto/caam/compat.h b/drivers/crypto/caam/compat.h
> > > index f57f395..b6955ec 100644
> > > --- a/drivers/crypto/caam/compat.h
> > > +++ b/drivers/crypto/caam/compat.h
> > > @@ -23,6 +23,7 @@
> > > #include <linux/types.h>
> > > #include <linux/debugfs.h>
> > > #include <linux/circ_buf.h>
> > > +#include <linux/clk.h>
> > > #include <net/xfrm.h>
> > >
> > > #include <crypto/algapi.h>
> > > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > > index 660cc3e..cfd8c9e 100644
> > > --- a/drivers/crypto/caam/ctrl.c
> > > +++ b/drivers/crypto/caam/ctrl.c
> > > @@ -16,6 +16,121 @@
> > > #include "error.h"
> > >
> > > /*
> > > + * ARM targets tend to have clock control subsystems that can
> > > + * enable/disable clocking to our device. Support clocking
> > > + * with the following functions.
> > > + */
> > > +#ifdef CONFIG_ARM
> > > +static inline struct clk *caam_drv_get_clk_ipg(struct caam_drv_private *drv)
> > > +{
> > > + return drv->caam_ipg;
> > > +}
> >
> > You return drv->caam_ipg on ARM and NULL on powerpc. drv->caam_ipg is
> > NULL on powerpc anyway which makes this different implementations for
> > ARM and powerpc unnecessary. Just access drv->caam_ipg directly where
> > needed.
>
> Agreed. I've reworked the patch to use direct references to the clocks.
>
> >
> > > +
> > > +static inline struct clk *caam_drv_get_clk_mem(struct caam_drv_private *drv)
> > > +{
> > > + return drv->caam_mem;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_get_clk_aclk(struct caam_drv_private *drv)
> > > +{
> > > + return drv->caam_aclk;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_get_clk_emislow(struct caam_drv_private *drv)
> > > +{
> > > + return drv->caam_emi_slow;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_ipg(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_ipg = clk;
> > > +}
> >
> > Ditto, just access drv->caam_ipg when needed.
> >
> > > +
> > > +static inline void caam_drv_set_clk_mem(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_mem = clk;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_aclk(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_aclk = clk;
> > > +}
> > > +
> > > +static inline void caam_drv_set_clk_emislow(struct caam_drv_private *drv,
> > > + struct clk *clk)
> > > +{
> > > + drv->caam_emi_slow = clk;
> > > +}
> > > +
> > > +static inline struct clk *caam_drv_identify_clk(struct device *dev,
> > > + char *clk_name)
> > > +{
> > > + return devm_clk_get(dev, clk_name);
> > > +}
> >
> > devm_clk_get() returns NULL when the architecture does not have clk
> > support, so it also seems unnecessary to have architecture specific
> > implementations for this.
> >
>
> Some of the QorIQ architectures have clock support enabled but do not
> support clocking to CAAM. This causes devm_clk_get() to return an error
> for these clocks instead of NULL. So, the only architecture-specific code
> left in the reworked patch is caam_drv_identify_clk().
I see. Wouldn't it be better to add CAAM clk support for QorIQ then?
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
