[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@foundries.io>

Add support for rpmb-dev property in optee node.
Prioritize that provided eMMC info from DT for RPMB operations over
the one provided by OP-TEE OS core in RPC calls.

Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
---

Changes in v2:
- Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
  is no rpmb-dev property or somemithing went wrong

 drivers/tee/optee/core.c          | 33 +++++++++++++++++
 drivers/tee/optee/optee_private.h |  2 +-
 drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
 3 files changed, 70 insertions(+), 25 deletions(-)

diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
index b898c32edc..828ab9b00a 100644
--- a/drivers/tee/optee/core.c
+++ b/drivers/tee/optee/core.c
@@ -12,6 +12,7 @@
 #include <linux/arm-smccc.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <mmc.h>
 
 #include "optee_smc.h"
 #include "optee_msg.h"
@@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
 	return ERR_PTR(-EINVAL);
 }
 
+static struct mmc *get_rpmb_dev(struct udevice *dev)
+{
+	struct udevice *mmc_dev;
+	const fdt32_t *phandle_p;
+	u32 phandle;
+	int ret = 0;
+
+	debug("optee: looking for rpmb device in DT.\n");
+
+	phandle_p  = ofnode_get_property(dev_ofnode(dev),
+					 "rpmb-dev", NULL);
+	if (!phandle_p) {
+		debug("optee: missing \"rpmb-dev\" property\n");
+		return NULL;
+	}
+
+	phandle = fdt32_to_cpu(*phandle_p);
+
+	ret = uclass_get_device_by_phandle_id(UCLASS_MMC, phandle, &mmc_dev);
+	if (ret) {
+		printf("optee: invalid phandle value in \"rpmb-dev\".\n");
+		return NULL;
+	}
+
+	debug("optee: using phandle %d from \"rpmd-dev\" property.\n",
+	      phandle);
+	return mmc_get_mmc_dev(mmc_dev);
+}
+
 static int optee_of_to_plat(struct udevice *dev)
 {
 	struct optee_pdata *pdata = dev_get_plat(dev);
+	struct optee_private *priv = dev_get_priv(dev);
 
 	pdata->invoke_fn = get_invoke_func(dev);
 	if (IS_ERR(pdata->invoke_fn))
 		return PTR_ERR(pdata->invoke_fn);
 
+	priv->rpmb_mmc = get_rpmb_dev(dev);
+
 	return 0;
 }
 
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 1f07a27ee4..8e5a280622 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -19,8 +19,8 @@
  */
 struct optee_private {
 	struct mmc *rpmb_mmc;
-	int rpmb_dev_id;
 	int rpmb_original_part;
+	bool rpmb_inited;
 };
 
 struct optee_msg_arg;
diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
index 0804fc963c..0137c44be1 100644
--- a/drivers/tee/optee/rpmb.c
+++ b/drivers/tee/optee/rpmb.c
@@ -45,55 +45,67 @@ static void release_mmc(struct optee_private *priv)
 {
 	int rc;
 
-	if (!priv->rpmb_mmc)
+	if (!priv->rpmb_mmc || !priv->rpmb_inited)
 		return;
 
-	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, priv->rpmb_dev_id,
-				      priv->rpmb_original_part);
+	rc = mmc_switch_part(priv->rpmb_mmc, priv->rpmb_original_part);
 	if (rc)
 		debug("%s: blk_select_hwpart_devnum() failed: %d\n",
 		      __func__, rc);
 
-	priv->rpmb_mmc = NULL;
+	priv->rpmb_inited = false;
+}
+
+static int check_mmc(struct mmc *mmc)
+{
+	if (!mmc) {
+		debug("Cannot find RPMB device\n");
+		return -ENODEV;
+	}
+	if (!(mmc->version & MMC_VERSION_MMC)) {
+		debug("Device id is not an eMMC device\n");
+		return -ENODEV;
+	}
+	if (mmc->version < MMC_VERSION_4_41) {
+		debug("RPMB is not supported before version 4.41\n");
+		return -ENODEV;
+	}
+
+	return 0;
 }
 
 static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
 {
-	struct mmc *mmc;
 	int rc;
 
-	if (priv->rpmb_mmc && priv->rpmb_dev_id == dev_id)
+	if (priv->rpmb_mmc && priv->rpmb_inited)
 		return priv->rpmb_mmc;
 
 	release_mmc(priv);
 
-	mmc = find_mmc_device(dev_id);
-	if (!mmc) {
-		debug("Cannot find RPMB device\n");
-		return NULL;
-	}
-	if (!(mmc->version & MMC_VERSION_MMC)) {
-		debug("Device id %d is not an eMMC device\n", dev_id);
-		return NULL;
-	}
-	if (mmc->version < MMC_VERSION_4_41) {
-		debug("Device id %d: RPMB not supported before version 4.41\n",
-		      dev_id);
+	/*
+	 * Check if priv->rpmb_mmc was already set from DT node,
+	 * otherwise use dev_id provided by OP-TEE OS
+	 * and find mmc device by its dev_id
+	 */
+	if (!priv->rpmb_mmc)
+		priv->rpmb_mmc = find_mmc_device(dev_id);
+
+	rc = check_mmc(priv->rpmb_mmc);
+	if (rc)
 		return NULL;
-	}
 
-	priv->rpmb_original_part = mmc_get_blk_desc(mmc)->hwpart;
+	priv->rpmb_original_part = mmc_get_blk_desc(priv->rpmb_mmc)->hwpart;
 
-	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, dev_id, MMC_PART_RPMB);
+	rc = mmc_switch_part(priv->rpmb_mmc, MMC_PART_RPMB);
 	if (rc) {
 		debug("Device id %d: cannot select RPMB partition: %d\n",
 		      dev_id, rc);
 		return NULL;
 	}
 
-	priv->rpmb_mmc = mmc;
-	priv->rpmb_dev_id = dev_id;
-	return mmc;
+	priv->rpmb_inited = true;
+	return priv->rpmb_mmc;
 }
 
 static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
-- 
2.25.1

[RFC PATCH v2 2/2] doc: device-tree-bindings: optee: support rpmb-dev property

[Igor Opaniuk]

From: Igor Opaniuk <igor.opaniuk@foundries.io>

Add support for "rpmb-dev" property which provides eMMC phandle
for RPMB-related operations.

This commit is created only to show DT node updates for RFC patch,
obviously that node should be updated in device-tree-bindings in
Linux first, that pulled here.

Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>

---

Changes v2:
- Add ";" after phandle

 doc/device-tree-bindings/firmware/linaro,optee-tz.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/doc/device-tree-bindings/firmware/linaro,optee-tz.txt b/doc/device-tree-bindings/firmware/linaro,optee-tz.txt
index d38834c67d..1776dd9606 100644
--- a/doc/device-tree-bindings/firmware/linaro,optee-tz.txt
+++ b/doc/device-tree-bindings/firmware/linaro,optee-tz.txt
@@ -20,6 +20,8 @@ the reference implementation maintained by Linaro.
                    "hvc" : HVC #0, with the register assignments specified
 		           in drivers/tee/optee/optee_smc.h
 
+- rpmb-dev	: contains phandle to the eMMC device, whose RPMB partition
+                  is used for secure storage.
 
 
 Example:
@@ -27,5 +29,6 @@ Example:
 		optee {
 			compatible = "linaro,optee-tz";
 			method = "smc";
+			rpmb-dev = <&mmc0>;
 		};
 	};
-- 
2.25.1

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Jens Wiklander]

Hi Igor,

On Sun, Jan 24, 2021 at 11:39:45AM +0200, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
> 
> Add support for rpmb-dev property in optee node.
> Prioritize that provided eMMC info from DT for RPMB operations over
> the one provided by OP-TEE OS core in RPC calls.
> 
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
> 
> Changes in v2:
> - Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
>   is no rpmb-dev property or somemithing went wrong
> 
>  drivers/tee/optee/core.c          | 33 +++++++++++++++++
>  drivers/tee/optee/optee_private.h |  2 +-
>  drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
>  3 files changed, 70 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index b898c32edc..828ab9b00a 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -12,6 +12,7 @@
>  #include <linux/arm-smccc.h>
>  #include <linux/err.h>
>  #include <linux/io.h>
> +#include <mmc.h>
>  
>  #include "optee_smc.h"
>  #include "optee_msg.h"
> @@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
>  	return ERR_PTR(-EINVAL);
>  }
>  
> +static struct mmc *get_rpmb_dev(struct udevice *dev)
> +{
> +	struct udevice *mmc_dev;
> +	const fdt32_t *phandle_p;
> +	u32 phandle;
> +	int ret = 0;
> +
> +	debug("optee: looking for rpmb device in DT.\n");
> +
> +	phandle_p  = ofnode_get_property(dev_ofnode(dev),
> +					 "rpmb-dev", NULL);
> +	if (!phandle_p) {
> +		debug("optee: missing \"rpmb-dev\" property\n");

With this we'll complain in the log each time "rpmb-dev" isn't found
even when OP-TEE isn't configured to use RPMB. This might cause some
confusion.

Cheers,
Jens

> +		return NULL;
> +	}
> +
> +	phandle = fdt32_to_cpu(*phandle_p);
> +
> +	ret = uclass_get_device_by_phandle_id(UCLASS_MMC, phandle, &mmc_dev);
> +	if (ret) {
> +		printf("optee: invalid phandle value in \"rpmb-dev\".\n");
> +		return NULL;
> +	}
> +
> +	debug("optee: using phandle %d from \"rpmd-dev\" property.\n",
> +	      phandle);
> +	return mmc_get_mmc_dev(mmc_dev);
> +}
> +
>  static int optee_of_to_plat(struct udevice *dev)
>  {
>  	struct optee_pdata *pdata = dev_get_plat(dev);
> +	struct optee_private *priv = dev_get_priv(dev);
>  
>  	pdata->invoke_fn = get_invoke_func(dev);
>  	if (IS_ERR(pdata->invoke_fn))
>  		return PTR_ERR(pdata->invoke_fn);
>  
> +	priv->rpmb_mmc = get_rpmb_dev(dev);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 1f07a27ee4..8e5a280622 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -19,8 +19,8 @@
>   */
>  struct optee_private {
>  	struct mmc *rpmb_mmc;
> -	int rpmb_dev_id;
>  	int rpmb_original_part;
> +	bool rpmb_inited;
>  };
>  
>  struct optee_msg_arg;
> diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> index 0804fc963c..0137c44be1 100644
> --- a/drivers/tee/optee/rpmb.c
> +++ b/drivers/tee/optee/rpmb.c
> @@ -45,55 +45,67 @@ static void release_mmc(struct optee_private *priv)
>  {
>  	int rc;
>  
> -	if (!priv->rpmb_mmc)
> +	if (!priv->rpmb_mmc || !priv->rpmb_inited)
>  		return;
>  
> -	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, priv->rpmb_dev_id,
> -				      priv->rpmb_original_part);
> +	rc = mmc_switch_part(priv->rpmb_mmc, priv->rpmb_original_part);
>  	if (rc)
>  		debug("%s: blk_select_hwpart_devnum() failed: %d\n",
>  		      __func__, rc);
>  
> -	priv->rpmb_mmc = NULL;
> +	priv->rpmb_inited = false;
> +}
> +
> +static int check_mmc(struct mmc *mmc)
> +{
> +	if (!mmc) {
> +		debug("Cannot find RPMB device\n");
> +		return -ENODEV;
> +	}
> +	if (!(mmc->version & MMC_VERSION_MMC)) {
> +		debug("Device id is not an eMMC device\n");
> +		return -ENODEV;
> +	}
> +	if (mmc->version < MMC_VERSION_4_41) {
> +		debug("RPMB is not supported before version 4.41\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
>  }
>  
>  static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
>  {
> -	struct mmc *mmc;
>  	int rc;
>  
> -	if (priv->rpmb_mmc && priv->rpmb_dev_id == dev_id)
> +	if (priv->rpmb_mmc && priv->rpmb_inited)
>  		return priv->rpmb_mmc;
>  
>  	release_mmc(priv);
>  
> -	mmc = find_mmc_device(dev_id);
> -	if (!mmc) {
> -		debug("Cannot find RPMB device\n");
> -		return NULL;
> -	}
> -	if (!(mmc->version & MMC_VERSION_MMC)) {
> -		debug("Device id %d is not an eMMC device\n", dev_id);
> -		return NULL;
> -	}
> -	if (mmc->version < MMC_VERSION_4_41) {
> -		debug("Device id %d: RPMB not supported before version 4.41\n",
> -		      dev_id);
> +	/*
> +	 * Check if priv->rpmb_mmc was already set from DT node,
> +	 * otherwise use dev_id provided by OP-TEE OS
> +	 * and find mmc device by its dev_id
> +	 */
> +	if (!priv->rpmb_mmc)
> +		priv->rpmb_mmc = find_mmc_device(dev_id);
> +
> +	rc = check_mmc(priv->rpmb_mmc);
> +	if (rc)
>  		return NULL;
> -	}
>  
> -	priv->rpmb_original_part = mmc_get_blk_desc(mmc)->hwpart;
> +	priv->rpmb_original_part = mmc_get_blk_desc(priv->rpmb_mmc)->hwpart;
>  
> -	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, dev_id, MMC_PART_RPMB);
> +	rc = mmc_switch_part(priv->rpmb_mmc, MMC_PART_RPMB);
>  	if (rc) {
>  		debug("Device id %d: cannot select RPMB partition: %d\n",
>  		      dev_id, rc);
>  		return NULL;
>  	}
>  
> -	priv->rpmb_mmc = mmc;
> -	priv->rpmb_dev_id = dev_id;
> -	return mmc;
> +	priv->rpmb_inited = true;
> +	return priv->rpmb_mmc;
>  }
>  
>  static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
> -- 
> 2.25.1
> 

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Igor Opaniuk]

Hi Jens,

On Mon, Jan 25, 2021 at 10:50 AM Jens Wiklander
<jens.wiklander@linaro.org> wrote:
>
> Hi Igor,
>
> On Sun, Jan 24, 2021 at 11:39:45AM +0200, Igor Opaniuk wrote:
> > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> >
> > Add support for rpmb-dev property in optee node.
> > Prioritize that provided eMMC info from DT for RPMB operations over
> > the one provided by OP-TEE OS core in RPC calls.
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > ---
> >
> > Changes in v2:
> > - Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
> >   is no rpmb-dev property or somemithing went wrong
> >
> >  drivers/tee/optee/core.c          | 33 +++++++++++++++++
> >  drivers/tee/optee/optee_private.h |  2 +-
> >  drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
> >  3 files changed, 70 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index b898c32edc..828ab9b00a 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/arm-smccc.h>
> >  #include <linux/err.h>
> >  #include <linux/io.h>
> > +#include <mmc.h>
> >
> >  #include "optee_smc.h"
> >  #include "optee_msg.h"
> > @@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
> >       return ERR_PTR(-EINVAL);
> >  }
> >
> > +static struct mmc *get_rpmb_dev(struct udevice *dev)
> > +{
> > +     struct udevice *mmc_dev;
> > +     const fdt32_t *phandle_p;
> > +     u32 phandle;
> > +     int ret = 0;
> > +
> > +     debug("optee: looking for rpmb device in DT.\n");
> > +
> > +     phandle_p  = ofnode_get_property(dev_ofnode(dev),
> > +                                      "rpmb-dev", NULL);
> > +     if (!phandle_p) {
> > +             debug("optee: missing \"rpmb-dev\" property\n");
>
> With this we'll complain in the log each time "rpmb-dev" isn't found
> even when OP-TEE isn't configured to use RPMB. This might cause some
> confusion.

Sure I'll wrap with "if (IS_ENABLED(CONFIG_SUPPORT_EMMC_RPMB))"
get_rpmb_dev() invocation in optee_of_to_plat() .

But as it's just a debug message, I would prefer to keep it for
debugging purposes
(I can adjust it, adding the phrase that the driver will use OP-TEE
provided dev id instead).

> Cheers,
> Jens
>
> > +             return NULL;
> > +     }
> > +
> > +     phandle = fdt32_to_cpu(*phandle_p);
> > +
> > +     ret = uclass_get_device_by_phandle_id(UCLASS_MMC, phandle, &mmc_dev);
> > +     if (ret) {
> > +             printf("optee: invalid phandle value in \"rpmb-dev\".\n");
> > +             return NULL;
> > +     }
> > +
> > +     debug("optee: using phandle %d from \"rpmd-dev\" property.\n",
> > +           phandle);
> > +     return mmc_get_mmc_dev(mmc_dev);
> > +}
> > +
> >  static int optee_of_to_plat(struct udevice *dev)
> >  {
> >       struct optee_pdata *pdata = dev_get_plat(dev);
> > +     struct optee_private *priv = dev_get_priv(dev);
> >
> >       pdata->invoke_fn = get_invoke_func(dev);
> >       if (IS_ERR(pdata->invoke_fn))
> >               return PTR_ERR(pdata->invoke_fn);
> >
> > +     priv->rpmb_mmc = get_rpmb_dev(dev);
> > +
> >       return 0;
> >  }
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 1f07a27ee4..8e5a280622 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -19,8 +19,8 @@
> >   */
> >  struct optee_private {
> >       struct mmc *rpmb_mmc;
> > -     int rpmb_dev_id;
> >       int rpmb_original_part;
> > +     bool rpmb_inited;
> >  };
> >
> >  struct optee_msg_arg;
> > diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> > index 0804fc963c..0137c44be1 100644
> > --- a/drivers/tee/optee/rpmb.c
> > +++ b/drivers/tee/optee/rpmb.c
> > @@ -45,55 +45,67 @@ static void release_mmc(struct optee_private *priv)
> >  {
> >       int rc;
> >
> > -     if (!priv->rpmb_mmc)
> > +     if (!priv->rpmb_mmc || !priv->rpmb_inited)
> >               return;
> >
> > -     rc = blk_select_hwpart_devnum(IF_TYPE_MMC, priv->rpmb_dev_id,
> > -                                   priv->rpmb_original_part);
> > +     rc = mmc_switch_part(priv->rpmb_mmc, priv->rpmb_original_part);
> >       if (rc)
> >               debug("%s: blk_select_hwpart_devnum() failed: %d\n",
> >                     __func__, rc);
> >
> > -     priv->rpmb_mmc = NULL;
> > +     priv->rpmb_inited = false;
> > +}
> > +
> > +static int check_mmc(struct mmc *mmc)
> > +{
> > +     if (!mmc) {
> > +             debug("Cannot find RPMB device\n");
> > +             return -ENODEV;
> > +     }
> > +     if (!(mmc->version & MMC_VERSION_MMC)) {
> > +             debug("Device id is not an eMMC device\n");
> > +             return -ENODEV;
> > +     }
> > +     if (mmc->version < MMC_VERSION_4_41) {
> > +             debug("RPMB is not supported before version 4.41\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> >  }
> >
> >  static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
> >  {
> > -     struct mmc *mmc;
> >       int rc;
> >
> > -     if (priv->rpmb_mmc && priv->rpmb_dev_id == dev_id)
> > +     if (priv->rpmb_mmc && priv->rpmb_inited)
> >               return priv->rpmb_mmc;
> >
> >       release_mmc(priv);
> >
> > -     mmc = find_mmc_device(dev_id);
> > -     if (!mmc) {
> > -             debug("Cannot find RPMB device\n");
> > -             return NULL;
> > -     }
> > -     if (!(mmc->version & MMC_VERSION_MMC)) {
> > -             debug("Device id %d is not an eMMC device\n", dev_id);
> > -             return NULL;
> > -     }
> > -     if (mmc->version < MMC_VERSION_4_41) {
> > -             debug("Device id %d: RPMB not supported before version 4.41\n",
> > -                   dev_id);
> > +     /*
> > +      * Check if priv->rpmb_mmc was already set from DT node,
> > +      * otherwise use dev_id provided by OP-TEE OS
> > +      * and find mmc device by its dev_id
> > +      */
> > +     if (!priv->rpmb_mmc)
> > +             priv->rpmb_mmc = find_mmc_device(dev_id);
> > +
> > +     rc = check_mmc(priv->rpmb_mmc);
> > +     if (rc)
> >               return NULL;
> > -     }
> >
> > -     priv->rpmb_original_part = mmc_get_blk_desc(mmc)->hwpart;
> > +     priv->rpmb_original_part = mmc_get_blk_desc(priv->rpmb_mmc)->hwpart;
> >
> > -     rc = blk_select_hwpart_devnum(IF_TYPE_MMC, dev_id, MMC_PART_RPMB);
> > +     rc = mmc_switch_part(priv->rpmb_mmc, MMC_PART_RPMB);
> >       if (rc) {
> >               debug("Device id %d: cannot select RPMB partition: %d\n",
> >                     dev_id, rc);
> >               return NULL;
> >       }
> >
> > -     priv->rpmb_mmc = mmc;
> > -     priv->rpmb_dev_id = dev_id;
> > -     return mmc;
> > +     priv->rpmb_inited = true;
> > +     return priv->rpmb_mmc;
> >  }
> >
> >  static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
> > --
> > 2.25.1
> >



-- 
Best regards - Freundliche Gr?sse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk at foundries.io
W: www.foundries.io

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Jens Wiklander]

On Mon, Jan 25, 2021 at 12:50 PM Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> Hi Jens,
>
> On Mon, Jan 25, 2021 at 10:50 AM Jens Wiklander
> <jens.wiklander@linaro.org> wrote:
> >
> > Hi Igor,
> >
> > On Sun, Jan 24, 2021 at 11:39:45AM +0200, Igor Opaniuk wrote:
> > > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> > >
> > > Add support for rpmb-dev property in optee node.
> > > Prioritize that provided eMMC info from DT for RPMB operations over
> > > the one provided by OP-TEE OS core in RPC calls.
> > >
> > > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > > ---
> > >
> > > Changes in v2:
> > > - Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
> > >   is no rpmb-dev property or somemithing went wrong
> > >
> > >  drivers/tee/optee/core.c          | 33 +++++++++++++++++
> > >  drivers/tee/optee/optee_private.h |  2 +-
> > >  drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
> > >  3 files changed, 70 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > > index b898c32edc..828ab9b00a 100644
> > > --- a/drivers/tee/optee/core.c
> > > +++ b/drivers/tee/optee/core.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/arm-smccc.h>
> > >  #include <linux/err.h>
> > >  #include <linux/io.h>
> > > +#include <mmc.h>
> > >
> > >  #include "optee_smc.h"
> > >  #include "optee_msg.h"
> > > @@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
> > >       return ERR_PTR(-EINVAL);
> > >  }
> > >
> > > +static struct mmc *get_rpmb_dev(struct udevice *dev)
> > > +{
> > > +     struct udevice *mmc_dev;
> > > +     const fdt32_t *phandle_p;
> > > +     u32 phandle;
> > > +     int ret = 0;
> > > +
> > > +     debug("optee: looking for rpmb device in DT.\n");
> > > +
> > > +     phandle_p  = ofnode_get_property(dev_ofnode(dev),
> > > +                                      "rpmb-dev", NULL);
> > > +     if (!phandle_p) {
> > > +             debug("optee: missing \"rpmb-dev\" property\n");
> >
> > With this we'll complain in the log each time "rpmb-dev" isn't found
> > even when OP-TEE isn't configured to use RPMB. This might cause some
> > confusion.
>
> Sure I'll wrap with "if (IS_ENABLED(CONFIG_SUPPORT_EMMC_RPMB))"
> get_rpmb_dev() invocation in optee_of_to_plat() .
>
> But as it's just a debug message, I would prefer to keep it for
> debugging purposes
> (I can adjust it, adding the phrase that the driver will use OP-TEE
> provided dev id instead).

Sounds good.

Cheers,
Jens

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Patrick DELAUNAY]

Hi Igor,

On 1/24/21 10:39 AM, Igor Opaniuk wrote:
> From: Igor Opaniuk <igor.opaniuk@foundries.io>
>
> Add support for rpmb-dev property in optee node.
> Prioritize that provided eMMC info from DT for RPMB operations over
> the one provided by OP-TEE OS core in RPC calls.
>
> Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> ---
>
> Changes in v2:
> - Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
>    is no rpmb-dev property or somemithing went wrong
>
>   drivers/tee/optee/core.c          | 33 +++++++++++++++++
>   drivers/tee/optee/optee_private.h |  2 +-
>   drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
>   3 files changed, 70 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> index b898c32edc..828ab9b00a 100644
> --- a/drivers/tee/optee/core.c
> +++ b/drivers/tee/optee/core.c
> @@ -12,6 +12,7 @@
>   #include <linux/arm-smccc.h>
>   #include <linux/err.h>
>   #include <linux/io.h>
> +#include <mmc.h>
>   
>   #include "optee_smc.h"
>   #include "optee_msg.h"
> @@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
>   	return ERR_PTR(-EINVAL);
>   }
>   
> +static struct mmc *get_rpmb_dev(struct udevice *dev)
> +{
> +	struct udevice *mmc_dev;
> +	const fdt32_t *phandle_p;
> +	u32 phandle;
> +	int ret = 0;
> +
> +	debug("optee: looking for rpmb device in DT.\n");
> +
> +	phandle_p  = ofnode_get_property(dev_ofnode(dev),
> +					 "rpmb-dev", NULL);
> +	if (!phandle_p) {
> +		debug("optee: missing \"rpmb-dev\" property\n");
> +		return NULL;
> +	}
> +
> +	phandle = fdt32_to_cpu(*phandle_p);
> +
> +	ret = uclass_get_device_by_phandle_id(UCLASS_MMC, phandle, &mmc_dev);
> +	if (ret) {
> +		printf("optee: invalid phandle value in \"rpmb-dev\".\n");
> +		return NULL;
> +	}
> +
> +	debug("optee: using phandle %d from \"rpmd-dev\" property.\n",
> +	      phandle);
> +	return mmc_get_mmc_dev(mmc_dev);
> +}
> +
>   static int optee_of_to_plat(struct udevice *dev)
>   {
>   	struct optee_pdata *pdata = dev_get_plat(dev);
> +	struct optee_private *priv = dev_get_priv(dev);
>   
>   	pdata->invoke_fn = get_invoke_func(dev);
>   	if (IS_ERR(pdata->invoke_fn))
>   		return PTR_ERR(pdata->invoke_fn);
>   

Normally optee_of_to_plat should initialized only the platdata and not the private date
(initialized during probe or driver execution)

And no need to initialize rpmb_mmcif CONFIG_CMD_OPTEE_RPMBI is not activated,

I proposed:

struct optee_pdata {
 ??? optee_invoke_fn *invoke_fn;
 ??? struct mmc *rpmb_mmc;
};

+ if (IS_ENABLED(CONFIG_CMD_OPTEE_RPMBI))

+??? pdata->rpmb_mmc = get_rpmb_dev(dev);

> +	priv->rpmb_mmc = get_rpmb_dev(dev);
> +
>   	return 0;
>   }
>   
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 1f07a27ee4..8e5a280622 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -19,8 +19,8 @@
>    */
>   struct optee_private {
>   	struct mmc *rpmb_mmc;
> -	int rpmb_dev_id;
>   	int rpmb_original_part;
> +	bool rpmb_inited;
>   };
>   
>   struct optee_msg_arg;
> diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> index 0804fc963c..0137c44be1 100644
> --- a/drivers/tee/optee/rpmb.c
> +++ b/drivers/tee/optee/rpmb.c
> @@ -45,55 +45,67 @@ static void release_mmc(struct optee_private *priv)
>   {
>   	int rc;
>   
> -	if (!priv->rpmb_mmc)
> +	if (!priv->rpmb_mmc || !priv->rpmb_inited)
>   		return;
>   
> -	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, priv->rpmb_dev_id,
> -				      priv->rpmb_original_part);
> +	rc = mmc_switch_part(priv->rpmb_mmc, priv->rpmb_original_part);
>   	if (rc)
>   		debug("%s: blk_select_hwpart_devnum() failed: %d\n",
>   		      __func__, rc);
>   
> -	priv->rpmb_mmc = NULL;
> +	priv->rpmb_inited = false;
> +}
> +
> +static int check_mmc(struct mmc *mmc)
> +{
> +	if (!mmc) {
> +		debug("Cannot find RPMB device\n");
> +		return -ENODEV;
> +	}
> +	if (!(mmc->version & MMC_VERSION_MMC)) {
> +		debug("Device id is not an eMMC device\n");
> +		return -ENODEV;
> +	}
> +	if (mmc->version < MMC_VERSION_4_41) {
> +		debug("RPMB is not supported before version 4.41\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
>   }
>   
>   static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
>   {
> -	struct mmc *mmc;
>   	int rc;
>   
> -	if (priv->rpmb_mmc && priv->rpmb_dev_id == dev_id)
> +	if (priv->rpmb_mmc && priv->rpmb_inited)
>   		return priv->rpmb_mmc;
>   
>   	release_mmc(priv);

really need to release mmc here (it is not initialized) as the first test of

the called function is:

 ??? if (!priv->rpmb_mmc || !priv->rpmb_inited)


>   
> -	mmc = find_mmc_device(dev_id);
> -	if (!mmc) {
> -		debug("Cannot find RPMB device\n");
> -		return NULL;
> -	}
> -	if (!(mmc->version & MMC_VERSION_MMC)) {
> -		debug("Device id %d is not an eMMC device\n", dev_id);
> -		return NULL;
> -	}
> -	if (mmc->version < MMC_VERSION_4_41) {
> -		debug("Device id %d: RPMB not supported before version 4.41\n",
> -		      dev_id);
> +	/*
> +	 * Check if priv->rpmb_mmc was already set from DT node,
> +	 * otherwise use dev_id provided by OP-TEE OS
> +	 * and find mmc device by its dev_id
> +	 */
> +	if (!priv->rpmb_mmc)
> +		priv->rpmb_mmc = find_mmc_device(dev_id);



if (plat->rpmb_mmc)

	priv->rpmb_mmc = plat->rpmb_mmc
else
	priv->rpmb_mmc = find_mmc_device(dev_id);


and priv->rpmb_inited is nore more required (if priv->rpmb_mmc = NULL; on each error)

> +
> +	rc = check_mmc(priv->rpmb_mmc);
> +	if (rc)
>   		return NULL;
> -	}
>   
> -	priv->rpmb_original_part = mmc_get_blk_desc(mmc)->hwpart;
> +	priv->rpmb_original_part = mmc_get_blk_desc(priv->rpmb_mmc)->hwpart;
>   
> -	rc = blk_select_hwpart_devnum(IF_TYPE_MMC, dev_id, MMC_PART_RPMB);
> +	rc = mmc_switch_part(priv->rpmb_mmc, MMC_PART_RPMB);
>   	if (rc) {
>   		debug("Device id %d: cannot select RPMB partition: %d\n",
>   		      dev_id, rc);
>   		return NULL;
>   	}
>   
> -	priv->rpmb_mmc = mmc;
> -	priv->rpmb_dev_id = dev_id;
> -	return mmc;
> +	priv->rpmb_inited = true;
> +	return priv->rpmb_mmc;
>   }
>   
>   static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)


regards


Patrick

[RFC PATCH v2 1/2] optee: obtain emmc rpmb info from dt

[Igor Opaniuk]

Hi Patrick,

On Mon, Feb 8, 2021 at 5:06 PM Patrick DELAUNAY
<patrick.delaunay@foss.st.com> wrote:
>
> Hi Igor,
>
> On 1/24/21 10:39 AM, Igor Opaniuk wrote:
> > From: Igor Opaniuk <igor.opaniuk@foundries.io>
> >
> > Add support for rpmb-dev property in optee node.
> > Prioritize that provided eMMC info from DT for RPMB operations over
> > the one provided by OP-TEE OS core in RPC calls.
> >
> > Signed-off-by: Igor Opaniuk <igor.opaniuk@foundries.io>
> > ---
> >
> > Changes in v2:
> > - Return NULL instead of ERR_PTR(-ENXIO) in get_rpmb_dev in case there
> >    is no rpmb-dev property or somemithing went wrong
> >
> >   drivers/tee/optee/core.c          | 33 +++++++++++++++++
> >   drivers/tee/optee/optee_private.h |  2 +-
> >   drivers/tee/optee/rpmb.c          | 60 ++++++++++++++++++-------------
> >   3 files changed, 70 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c
> > index b898c32edc..828ab9b00a 100644
> > --- a/drivers/tee/optee/core.c
> > +++ b/drivers/tee/optee/core.c
> > @@ -12,6 +12,7 @@
> >   #include <linux/arm-smccc.h>
> >   #include <linux/err.h>
> >   #include <linux/io.h>
> > +#include <mmc.h>
> >
> >   #include "optee_smc.h"
> >   #include "optee_msg.h"
> > @@ -607,14 +608,46 @@ static optee_invoke_fn *get_invoke_func(struct udevice *dev)
> >       return ERR_PTR(-EINVAL);
> >   }
> >
> > +static struct mmc *get_rpmb_dev(struct udevice *dev)
> > +{
> > +     struct udevice *mmc_dev;
> > +     const fdt32_t *phandle_p;
> > +     u32 phandle;
> > +     int ret = 0;
> > +
> > +     debug("optee: looking for rpmb device in DT.\n");
> > +
> > +     phandle_p  = ofnode_get_property(dev_ofnode(dev),
> > +                                      "rpmb-dev", NULL);
> > +     if (!phandle_p) {
> > +             debug("optee: missing \"rpmb-dev\" property\n");
> > +             return NULL;
> > +     }
> > +
> > +     phandle = fdt32_to_cpu(*phandle_p);
> > +
> > +     ret = uclass_get_device_by_phandle_id(UCLASS_MMC, phandle, &mmc_dev);
> > +     if (ret) {
> > +             printf("optee: invalid phandle value in \"rpmb-dev\".\n");
> > +             return NULL;
> > +     }
> > +
> > +     debug("optee: using phandle %d from \"rpmd-dev\" property.\n",
> > +           phandle);
> > +     return mmc_get_mmc_dev(mmc_dev);
> > +}
> > +
> >   static int optee_of_to_plat(struct udevice *dev)
> >   {
> >       struct optee_pdata *pdata = dev_get_plat(dev);
> > +     struct optee_private *priv = dev_get_priv(dev);
> >
> >       pdata->invoke_fn = get_invoke_func(dev);
> >       if (IS_ERR(pdata->invoke_fn))
> >               return PTR_ERR(pdata->invoke_fn);
> >
>
> Normally optee_of_to_plat should initialized only the platdata and not the private date
> (initialized during probe or driver execution)

To be honest that was my initial intention to do that way,
but that would have required a complete overhaul of all functions in
optee/rpmb.c,
so I just tried to find a "golden mean" between the approach you described
and something less intrusive.

>
> And no need to initialize rpmb_mmcif CONFIG_CMD_OPTEE_RPMBI is not activated,
Yeah, will fix that.

>
> I proposed:
>
> struct optee_pdata {
>      optee_invoke_fn *invoke_fn;
>      struct mmc *rpmb_mmc;
> };
>
> + if (IS_ENABLED(CONFIG_CMD_OPTEE_RPMBI))
>
> +    pdata->rpmb_mmc = get_rpmb_dev(dev);
>
> > +     priv->rpmb_mmc = get_rpmb_dev(dev);
> > +
> >       return 0;
> >   }
> >
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 1f07a27ee4..8e5a280622 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -19,8 +19,8 @@
> >    */
> >   struct optee_private {
> >       struct mmc *rpmb_mmc;
> > -     int rpmb_dev_id;
> >       int rpmb_original_part;
> > +     bool rpmb_inited;
> >   };
> >
> >   struct optee_msg_arg;
> > diff --git a/drivers/tee/optee/rpmb.c b/drivers/tee/optee/rpmb.c
> > index 0804fc963c..0137c44be1 100644
> > --- a/drivers/tee/optee/rpmb.c
> > +++ b/drivers/tee/optee/rpmb.c
> > @@ -45,55 +45,67 @@ static void release_mmc(struct optee_private *priv)
> >   {
> >       int rc;
> >
> > -     if (!priv->rpmb_mmc)
> > +     if (!priv->rpmb_mmc || !priv->rpmb_inited)
> >               return;
> >
> > -     rc = blk_select_hwpart_devnum(IF_TYPE_MMC, priv->rpmb_dev_id,
> > -                                   priv->rpmb_original_part);
> > +     rc = mmc_switch_part(priv->rpmb_mmc, priv->rpmb_original_part);
> >       if (rc)
> >               debug("%s: blk_select_hwpart_devnum() failed: %d\n",
> >                     __func__, rc);
> >
> > -     priv->rpmb_mmc = NULL;
> > +     priv->rpmb_inited = false;
> > +}
> > +
> > +static int check_mmc(struct mmc *mmc)
> > +{
> > +     if (!mmc) {
> > +             debug("Cannot find RPMB device\n");
> > +             return -ENODEV;
> > +     }
> > +     if (!(mmc->version & MMC_VERSION_MMC)) {
> > +             debug("Device id is not an eMMC device\n");
> > +             return -ENODEV;
> > +     }
> > +     if (mmc->version < MMC_VERSION_4_41) {
> > +             debug("RPMB is not supported before version 4.41\n");
> > +             return -ENODEV;
> > +     }
> > +
> > +     return 0;
> >   }
> >
> >   static struct mmc *get_mmc(struct optee_private *priv, int dev_id)
> >   {
> > -     struct mmc *mmc;
> >       int rc;
> >
> > -     if (priv->rpmb_mmc && priv->rpmb_dev_id == dev_id)
> > +     if (priv->rpmb_mmc && priv->rpmb_inited)
> >               return priv->rpmb_mmc;
> >
> >       release_mmc(priv);
>
> really need to release mmc here (it is not initialized) as the first test of
>
> the called function is:
>
>      if (!priv->rpmb_mmc || !priv->rpmb_inited)
>
>
> >
> > -     mmc = find_mmc_device(dev_id);
> > -     if (!mmc) {
> > -             debug("Cannot find RPMB device\n");
> > -             return NULL;
> > -     }
> > -     if (!(mmc->version & MMC_VERSION_MMC)) {
> > -             debug("Device id %d is not an eMMC device\n", dev_id);
> > -             return NULL;
> > -     }
> > -     if (mmc->version < MMC_VERSION_4_41) {
> > -             debug("Device id %d: RPMB not supported before version 4.41\n",
> > -                   dev_id);
> > +     /*
> > +      * Check if priv->rpmb_mmc was already set from DT node,
> > +      * otherwise use dev_id provided by OP-TEE OS
> > +      * and find mmc device by its dev_id
> > +      */
> > +     if (!priv->rpmb_mmc)
> > +             priv->rpmb_mmc = find_mmc_device(dev_id);
>
>
>
> if (plat->rpmb_mmc)
>
>         priv->rpmb_mmc = plat->rpmb_mmc
> else
>         priv->rpmb_mmc = find_mmc_device(dev_id);
>
>
> and priv->rpmb_inited is nore more required (if priv->rpmb_mmc = NULL; on each error)
>
> > +
> > +     rc = check_mmc(priv->rpmb_mmc);
> > +     if (rc)
> >               return NULL;
> > -     }
> >
> > -     priv->rpmb_original_part = mmc_get_blk_desc(mmc)->hwpart;
> > +     priv->rpmb_original_part = mmc_get_blk_desc(priv->rpmb_mmc)->hwpart;
> >
> > -     rc = blk_select_hwpart_devnum(IF_TYPE_MMC, dev_id, MMC_PART_RPMB);
> > +     rc = mmc_switch_part(priv->rpmb_mmc, MMC_PART_RPMB);
> >       if (rc) {
> >               debug("Device id %d: cannot select RPMB partition: %d\n",
> >                     dev_id, rc);
> >               return NULL;
> >       }
> >
> > -     priv->rpmb_mmc = mmc;
> > -     priv->rpmb_dev_id = dev_id;
> > -     return mmc;
> > +     priv->rpmb_inited = true;
> > +     return priv->rpmb_mmc;
> >   }
> >
> >   static u32 rpmb_get_dev_info(u16 dev_id, struct rpmb_dev_info *info)
>
>
> regards
>
>
> Patrick
>

The principal problem here is a new "rpmb-dev" property for optee DT
node which I introduced
in this patch set. As in Linux all RPMB RPC calls are handled by tee
supplicant running
in user space, unlikely I can somehow justify why we need that
additional property
if it is not going to be used by optee linux kernel driver.

We can still have a temporary approach with u-boot-specific property
(something like `u-boot,rpmb-dev` in u-boot specific dtsi),  but that will
create additional fragmentation and divergence between U-Boot/Linux,
which I want to avoid. So I'm still thinking about what could be a
better solution here.

For additional details please check [1]

[1] https://github.com/OP-TEE/optee_os/issues/4343

-- 
Best regards - Freundliche Gr?sse - Meilleures salutations

Igor Opaniuk
Embedded Software Engineer
T:  +380 938364067
E: igor.opaniuk at foundries.io
W: www.foundries.io