[1/2] crypto: caam: Change the i.MX8MQ check support all i.MX8M variants

Commit Message

Adam Ford Nov. 30, 2019, 10:51 p.m. UTC

The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
the driver is restricting the check to just the i.MX8MQ.

This patch lets the driver support all i.MX8M Variants if enabled.

Signed-off-by: Adam Ford <[e-mail]>

Comments

Frieder Schrempf Dec. 4, 2019, 11:38 a.m. UTC | #1

Hi Adam,

On 30.11.19 23:51, Adam Ford wrote:
> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> the driver is restricting the check to just the i.MX8MQ.
> 
> This patch lets the driver support all i.MX8M Variants if enabled.
> 
> Signed-off-by: Adam Ford <[e-mail]>

What about the following lines in run_descriptor_deco0()? Does this 
condition also apply to i.MX8MM?

drivers/crypto/caam/ctrl.c:

	if (ctrlpriv->virt_en == 1 ||
	    /*
	     * Apparently on i.MX8MQ it doesn't matter if virt_en == 1
	     * and the following steps should be performed regardless
	     */
	    of_machine_is_compatible("fsl,imx8mq")) {
		clrsetbits_32(&ctrl->deco_rsr, 0, DECORSR_JR0);

		while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
		       --timeout)
			cpu_relax();

		timeout = 100000;
	}

Regards,
Frieder

> 
> diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> index db22777d59b4..1ce03f8961b6 100644
> --- a/drivers/crypto/caam/ctrl.c
> +++ b/drivers/crypto/caam/ctrl.c
> @@ -527,7 +527,7 @@ static const struct soc_device_attribute caam_imx_soc_table[] = {
>   	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
>   	{ .soc_id = "i.MX6*",  .data = &caam_imx6_data },
>   	{ .soc_id = "i.MX7*",  .data = &caam_imx7_data },
> -	{ .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
> +	{ .soc_id = "i.MX8M*", .data = &caam_imx7_data },
>   	{ .family = "Freescale i.MX" },
>   	{ /* sentinel */ }
>   };
>

Adam Ford Dec. 6, 2019, 7:55 p.m. UTC | #2

On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
<[e-mail]> wrote:
>
> Hi Adam,
>
> On 30.11.19 23:51, Adam Ford wrote:
> > The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> > the driver is restricting the check to just the i.MX8MQ.
> >
> > This patch lets the driver support all i.MX8M Variants if enabled.
> >
> > Signed-off-by: Adam Ford <[e-mail]>
>
> What about the following lines in run_descriptor_deco0()? Does this
> condition also apply to i.MX8MM?

I think that's a question for NXP.  I am not seeing that in the NXP
Linux Release, and I don't have an 8MQ to compare.

I was able to get the driver working on the i.MXMM with the patch.

NXP  Team,

Do you have any opinions on this?

adam
>
> drivers/crypto/caam/ctrl.c:
>
>         if (ctrlpriv->virt_en == 1 ||
>             /*
>              * Apparently on i.MX8MQ it doesn't matter if virt_en == 1
>              * and the following steps should be performed regardless
>              */
>             of_machine_is_compatible("fsl,imx8mq")) {
>                 clrsetbits_32(&ctrl->deco_rsr, 0, DECORSR_JR0);
>
>                 while (!(rd_reg32(&ctrl->deco_rsr) & DECORSR_VALID) &&
>                        --timeout)
>                         cpu_relax();
>
>                 timeout = 100000;
>         }
>
> Regards,
> Frieder
>
> >
> > diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
> > index db22777d59b4..1ce03f8961b6 100644
> > --- a/drivers/crypto/caam/ctrl.c
> > +++ b/drivers/crypto/caam/ctrl.c
> > @@ -527,7 +527,7 @@ static const struct soc_device_attribute caam_imx_soc_table[] = {
> >       { .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
> >       { .soc_id = "i.MX6*",  .data = &caam_imx6_data },
> >       { .soc_id = "i.MX7*",  .data = &caam_imx7_data },
> > -     { .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
> > +     { .soc_id = "i.MX8M*", .data = &caam_imx7_data },
> >       { .family = "Freescale i.MX" },
> >       { /* sentinel */ }
> >   };
> >

Horia Geanta Dec. 10, 2019, 7:56 a.m. UTC | #3

On 12/6/2019 9:55 PM, Adam Ford wrote:
> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
> <[e-mail]> wrote:
>>
>> Hi Adam,
>>
>> On 30.11.19 23:51, Adam Ford wrote:
>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
>>> the driver is restricting the check to just the i.MX8MQ.
>>>
>>> This patch lets the driver support all i.MX8M Variants if enabled.
>>>
>>> Signed-off-by: Adam Ford <[e-mail]>
>>
>> What about the following lines in run_descriptor_deco0()? Does this
>> condition also apply to i.MX8MM?
> 
> I think that's a question for NXP.  I am not seeing that in the NXP
> Linux Release, and I don't have an 8MQ to compare.
> 
IIRC the i.MX BSP releases use the JRI for initializing the RNG,
and not the DECO register interface.

> I was able to get the driver working on the i.MXMM with the patch.
> 
You are probably using a newer U-boot, which includes
commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")

> NXP  Team,
> 
> Do you have any opinions on this?
> 
Since current U-boot initializes both RNG state handles, practically
instantiate_rng() is a no-op.

A simple experiment is to "lie" about the state_handle_mask, to exercise
the DECO acquire code (or, as mentioned above, to run with an older U-boot):

@@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
        struct caam_ctrl __iomem *ctrl;
        u32 *desc, status = 0, rdsta_val;
        int ret = 0, sh_idx;
+       static int force_init = 1;

        ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
        desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
        if (!desc)
                return -ENOMEM;

+       if (force_init && (state_handle_mask == 0x3)) {
+               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
+               force_init = 0;
+               state_handle_mask = 0x2;
+       }
+
        for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
                /*
                 * If the corresponding bit is set, this state handle

In this case boot log confirms the DECO cannot be acquired:
[    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
[    2.172293] caam 30900000.crypto: failed to acquire DECO 0
[    2.177786] caam 30900000.crypto: failed to instantiate RNG

To sum up, writing to DECORSR is mandatory.

Horia

Frieder Schrempf Dec. 11, 2019, 2:23 p.m. UTC | #4

On 10.12.19 08:56, Horia Geanta wrote:
> On 12/6/2019 9:55 PM, Adam Ford wrote:
>> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
>> <[e-mail]> wrote:
>>>
>>> Hi Adam,
>>>
>>> On 30.11.19 23:51, Adam Ford wrote:
>>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
>>>> the driver is restricting the check to just the i.MX8MQ.
>>>>
>>>> This patch lets the driver support all i.MX8M Variants if enabled.
>>>>
>>>> Signed-off-by: Adam Ford <[e-mail]>
>>>
>>> What about the following lines in run_descriptor_deco0()? Does this
>>> condition also apply to i.MX8MM?
>>
>> I think that's a question for NXP.  I am not seeing that in the NXP
>> Linux Release, and I don't have an 8MQ to compare.
>>
> IIRC the i.MX BSP releases use the JRI for initializing the RNG,
> and not the DECO register interface.
> 
>> I was able to get the driver working on the i.MXMM with the patch.
>>
> You are probably using a newer U-boot, which includes
> commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")
> 
>> NXP  Team,
>>
>> Do you have any opinions on this?
>>
> Since current U-boot initializes both RNG state handles, practically
> instantiate_rng() is a no-op.
> 
> A simple experiment is to "lie" about the state_handle_mask, to exercise
> the DECO acquire code (or, as mentioned above, to run with an older U-boot):
> 
> @@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
>          struct caam_ctrl __iomem *ctrl;
>          u32 *desc, status = 0, rdsta_val;
>          int ret = 0, sh_idx;
> +       static int force_init = 1;
> 
>          ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
>          desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
>          if (!desc)
>                  return -ENOMEM;
> 
> +       if (force_init && (state_handle_mask == 0x3)) {
> +               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
> +               force_init = 0;
> +               state_handle_mask = 0x2;
> +       }
> +
>          for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
>                  /*
>                   * If the corresponding bit is set, this state handle
> 
> In this case boot log confirms the DECO cannot be acquired:
> [    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
> [    2.172293] caam 30900000.crypto: failed to acquire DECO 0
> [    2.177786] caam 30900000.crypto: failed to instantiate RNG
> 
> To sum up, writing to DECORSR is mandatory.

Thanks Horia for providing the details.

Adam, can you update your patch to enable the code in 
run_descriptor_deco0() for i.MX8MM?

If I understand this correctly, this is necessary to have the RNG 
initialize correctly no matter what version of U-Boot is used.

Thanks,
Frieder

Adam Ford Dec. 11, 2019, 2:27 p.m. UTC | #5

On Wed, Dec 11, 2019 at 8:23 AM Schrempf Frieder
<[e-mail]> wrote:
>
> On 10.12.19 08:56, Horia Geanta wrote:
> > On 12/6/2019 9:55 PM, Adam Ford wrote:
> >> On Wed, Dec 4, 2019 at 5:38 AM Schrempf Frieder
> >> <[e-mail]> wrote:
> >>>
> >>> Hi Adam,
> >>>
> >>> On 30.11.19 23:51, Adam Ford wrote:
> >>>> The i.MX8M Mini uses the same crypto engine as the i.MX8MQ, but
> >>>> the driver is restricting the check to just the i.MX8MQ.
> >>>>
> >>>> This patch lets the driver support all i.MX8M Variants if enabled.
> >>>>
> >>>> Signed-off-by: Adam Ford <[e-mail]>
> >>>
> >>> What about the following lines in run_descriptor_deco0()? Does this
> >>> condition also apply to i.MX8MM?
> >>
> >> I think that's a question for NXP.  I am not seeing that in the NXP
> >> Linux Release, and I don't have an 8MQ to compare.
> >>
> > IIRC the i.MX BSP releases use the JRI for initializing the RNG,
> > and not the DECO register interface.
> >
> >> I was able to get the driver working on the i.MXMM with the patch.
> >>
> > You are probably using a newer U-boot, which includes
> > commit dfaec76029f2 ("crypto/fsl: instantiate all rng state handles")
> >
> >> NXP  Team,
> >>
> >> Do you have any opinions on this?
> >>
> > Since current U-boot initializes both RNG state handles, practically
> > instantiate_rng() is a no-op.
> >
> > A simple experiment is to "lie" about the state_handle_mask, to exercise
> > the DECO acquire code (or, as mentioned above, to run with an older U-boot):
> >
> > @@ -268,12 +272,19 @@ static int instantiate_rng(struct device *ctrldev, int state_handle_mask,
> >          struct caam_ctrl __iomem *ctrl;
> >          u32 *desc, status = 0, rdsta_val;
> >          int ret = 0, sh_idx;
> > +       static int force_init = 1;
> >
> >          ctrl = (struct caam_ctrl __iomem *)ctrlpriv->ctrl;
> >          desc = kmalloc(CAAM_CMD_SZ * 7, GFP_KERNEL);
> >          if (!desc)
> >                  return -ENOMEM;
> >
> > +       if (force_init && (state_handle_mask == 0x3)) {
> > +               dev_err(ctrldev, "Forcing reinit of RNG state handle 0!\n");
> > +               force_init = 0;
> > +               state_handle_mask = 0x2;
> > +       }
> > +
> >          for (sh_idx = 0; sh_idx < RNG4_MAX_HANDLES; sh_idx++) {
> >                  /*
> >                   * If the corresponding bit is set, this state handle
> >
> > In this case boot log confirms the DECO cannot be acquired:
> > [    2.137101] caam 30900000.crypto: Forcing reinit of RNG state handle 0!
> > [    2.172293] caam 30900000.crypto: failed to acquire DECO 0
> > [    2.177786] caam 30900000.crypto: failed to instantiate RNG
> >
> > To sum up, writing to DECORSR is mandatory.
>
> Thanks Horia for providing the details.

I appreciate it too.

>
> Adam, can you update your patch to enable the code in
> run_descriptor_deco0() for i.MX8MM?

I will work on that.  I have been trying to get the mainline U-Boot to
start correctly, because I wanted to see if/how this interacted. I'll
try to get a V2 pushed today.

>
> If I understand this correctly, this is necessary to have the RNG
> initialize correctly no matter what version of U-Boot is used.

That makes sense based on Horia's feedback.  With the holidays this
month, my spare time and weekends have been full.

adam
>
> Thanks,
> Frieder

Patch

diff --git a/drivers/crypto/caam/ctrl.c b/drivers/crypto/caam/ctrl.c
index db22777d59b4..1ce03f8961b6 100644
--- a/drivers/crypto/caam/ctrl.c
+++ b/drivers/crypto/caam/ctrl.c
@@ -527,7 +527,7 @@  static const struct soc_device_attribute caam_imx_soc_table[] = {
 	{ .soc_id = "i.MX6UL", .data = &caam_imx6ul_data },
 	{ .soc_id = "i.MX6*",  .data = &caam_imx6_data },
 	{ .soc_id = "i.MX7*",  .data = &caam_imx7_data },
-	{ .soc_id = "i.MX8MQ", .data = &caam_imx7_data },
+	{ .soc_id = "i.MX8M*", .data = &caam_imx7_data },
 	{ .family = "Freescale i.MX" },
 	{ /* sentinel */ }
 };