| Message ID | 20210124093946.103018-1-igor.opaniuk@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Peng Fan |
| Headers | show |
| Series | [RFC,v2,1/2] optee: obtain emmc rpmb info from dt | expand |
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 >
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 > >
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
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
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
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)