| Message ID | 20220906093703.35658-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] tee: optee: rework TA bus scanning code | expand |
Hi, On 9/6/22 11:37, Ilias Apalodimas wrote: > Late versions of OP-TEE support a pseudo bus. TAs that behave as > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > that we already have a workaround for RNG. The details are in > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > So let's add a list of devices based on U-Boot Kconfig options that we will > scan until we properly implement the tee-bus functionality. > > While at it change the behaviour of the tee core itself wrt to device > binding. If some device binding fails, print a warning instead of > disabling OP-TEE. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > --- > Changes since v3: > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > really needed > - Changed the style of the optee_bus_probe[] definition to > {.drv_name = xxx, .dev_name = yyy } > > Changes since v2: > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > Changes since v1: > - remove a macro and use ARRAY_SIZE directly > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > Reviewed-by: Patrick Delaunay <patrick.delaunay@foss.st.com> Thanks Patrick
Hi, On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > that we already have a workaround for RNG. The details are in > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > So let's add a list of devices based on U-Boot Kconfig options that we will > scan until we properly implement the tee-bus functionality. > > While at it change the behaviour of the tee core itself wrt to device > binding. If some device binding fails, print a warning instead of > disabling OP-TEE. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > --- > Changes since v3: > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > really needed > - Changed the style of the optee_bus_probe[] definition to > {.drv_name = xxx, .dev_name = yyy } > > Changes since v2: > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > Changes since v1: > - remove a macro and use ARRAY_SIZE directly > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > 1 file changed, 19 insertions(+), 5 deletions(-) > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > index a89d62aaf0b3..c201a4635e6b 100644 > --- a/drivers/tee/optee/core.c > +++ b/drivers/tee/optee/core.c > @@ -31,6 +31,18 @@ struct optee_pdata { > optee_invoke_fn *invoke_fn; > }; > > +static const struct { > + const char *drv_name; > + const char *dev_name; > +} optee_bus_probe[] = { > +#ifdef CONFIG_RNG_OPTEE > + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, > +#endif > +#ifdef CONFIG_TPM2_FTPM_TEE > + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, > +#endif > +}; > + > struct rpc_param { > u32 a0; > u32 a1; > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) > { > struct optee_pdata *pdata = dev_get_plat(dev); > u32 sec_caps; > - struct udevice *child; > - int ret; > + int ret, i; > > if (!is_optee_api(pdata->invoke_fn)) { > dev_err(dev, "OP-TEE api uid mismatch\n"); > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) > * in U-Boot, the discovery of TA on the TEE bus is not supported: > * only bind the drivers associated to the supported OP-TEE TA > */ > - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { > - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); > + > + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { > + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, > + optee_bus_probe[i].dev_name, NULL); > if (ret) > - return ret; > + dev_warn(dev, "Failed to bind device %s\n", > + optee_bus_probe[i].dev_name); Please add device tree nodes for these and all this code can go away. Regards, Simon
Hi Simon, On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote: > Hi, > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > > that we already have a workaround for RNG. The details are in > > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > > > So let's add a list of devices based on U-Boot Kconfig options that we will > > scan until we properly implement the tee-bus functionality. > > > > While at it change the behaviour of the tee core itself wrt to device > > binding. If some device binding fails, print a warning instead of > > disabling OP-TEE. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > > --- > > Changes since v3: > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > > really needed > > - Changed the style of the optee_bus_probe[] definition to > > {.drv_name = xxx, .dev_name = yyy } > > > > Changes since v2: > > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > > > Changes since v1: > > - remove a macro and use ARRAY_SIZE directly > > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > index a89d62aaf0b3..c201a4635e6b 100644 > > --- a/drivers/tee/optee/core.c > > +++ b/drivers/tee/optee/core.c > > @@ -31,6 +31,18 @@ struct optee_pdata { > > optee_invoke_fn *invoke_fn; > > }; > > > > +static const struct { > > + const char *drv_name; > > + const char *dev_name; > > +} optee_bus_probe[] = { > > +#ifdef CONFIG_RNG_OPTEE > > + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, > > +#endif > > +#ifdef CONFIG_TPM2_FTPM_TEE > > + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, > > +#endif > > +}; > > + > > struct rpc_param { > > u32 a0; > > u32 a1; > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) > > { > > struct optee_pdata *pdata = dev_get_plat(dev); > > u32 sec_caps; > > - struct udevice *child; > > - int ret; > > + int ret, i; > > > > if (!is_optee_api(pdata->invoke_fn)) { > > dev_err(dev, "OP-TEE api uid mismatch\n"); > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) > > * in U-Boot, the discovery of TA on the TEE bus is not supported: > > * only bind the drivers associated to the supported OP-TEE TA > > */ > > - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { > > - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); > > + > > + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { > > + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, > > + optee_bus_probe[i].dev_name, NULL); > > if (ret) > > - return ret; > > + dev_warn(dev, "Failed to bind device %s\n", > > + optee_bus_probe[i].dev_name); > > Please add device tree nodes for these and all this code can go away. That's the exact opposite of what the commit message describes. OP-TEE supports a scannable bus ifor TAs that behave like hardware blocks and doesn't need a DT entry. Since it's really the TAs compilation decision to support that or not having them as a DT node is not always the right choice. Thanks /Ilias > > Regards, > Simon
Hi Ilias, On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote: > > Hi, > > > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > > > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > > > that we already have a workaround for RNG. The details are in > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > > > > > So let's add a list of devices based on U-Boot Kconfig options that we will > > > scan until we properly implement the tee-bus functionality. > > > > > > While at it change the behaviour of the tee core itself wrt to device > > > binding. If some device binding fails, print a warning instead of > > > disabling OP-TEE. > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > > > --- > > > Changes since v3: > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > > > really needed > > > - Changed the style of the optee_bus_probe[] definition to > > > {.drv_name = xxx, .dev_name = yyy } > > > > > > Changes since v2: > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > > > > > Changes since v1: > > > - remove a macro and use ARRAY_SIZE directly > > > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > > index a89d62aaf0b3..c201a4635e6b 100644 > > > --- a/drivers/tee/optee/core.c > > > +++ b/drivers/tee/optee/core.c > > > @@ -31,6 +31,18 @@ struct optee_pdata { > > > optee_invoke_fn *invoke_fn; > > > }; > > > > > > +static const struct { > > > + const char *drv_name; > > > + const char *dev_name; > > > +} optee_bus_probe[] = { > > > +#ifdef CONFIG_RNG_OPTEE > > > + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, > > > +#endif > > > +#ifdef CONFIG_TPM2_FTPM_TEE > > > + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, > > > +#endif > > > +}; > > > + > > > struct rpc_param { > > > u32 a0; > > > u32 a1; > > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) > > > { > > > struct optee_pdata *pdata = dev_get_plat(dev); > > > u32 sec_caps; > > > - struct udevice *child; > > > - int ret; > > > + int ret, i; > > > > > > if (!is_optee_api(pdata->invoke_fn)) { > > > dev_err(dev, "OP-TEE api uid mismatch\n"); > > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) > > > * in U-Boot, the discovery of TA on the TEE bus is not supported: > > > * only bind the drivers associated to the supported OP-TEE TA > > > */ > > > - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { > > > - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); > > > + > > > + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { > > > + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, > > > + optee_bus_probe[i].dev_name, NULL); > > > if (ret) > > > - return ret; > > > + dev_warn(dev, "Failed to bind device %s\n", > > > + optee_bus_probe[i].dev_name); > > > > Please add device tree nodes for these and all this code can go away. > > That's the exact opposite of what the commit message describes. OP-TEE > supports a scannable bus ifor TAs that behave like hardware blocks and > doesn't need a DT entry. Since it's really the TAs compilation decision > to support that or not having them as a DT node is not always the right > choice. This is continuing the perversion of how things are supposed to work in driver model. We need to talk about this because it is simply the wrong way to be approaching this. There is nothing wrong with putting things in the DT and this is how U-Boot works. For now, please create a binding and get it reviewed. You don't need all the internal objects but you do need an OP-TEE driver and node, as we have with PCI. Regards, Simon
Hi Simon, On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas > <ilias.apalodimas@linaro.org> wrote: > > > > Hi Simon, > > > > On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote: > > > Hi, > > > > > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > > > > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > > > > that we already have a workaround for RNG. The details are in > > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > > > > > > > So let's add a list of devices based on U-Boot Kconfig options that we will > > > > scan until we properly implement the tee-bus functionality. > > > > > > > > While at it change the behaviour of the tee core itself wrt to device > > > > binding. If some device binding fails, print a warning instead of > > > > disabling OP-TEE. > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > > > > --- > > > > Changes since v3: > > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > > > > really needed > > > > - Changed the style of the optee_bus_probe[] definition to > > > > {.drv_name = xxx, .dev_name = yyy } > > > > > > > > Changes since v2: > > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > > > > > > > Changes since v1: > > > > - remove a macro and use ARRAY_SIZE directly > > > > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > > > index a89d62aaf0b3..c201a4635e6b 100644 > > > > --- a/drivers/tee/optee/core.c > > > > +++ b/drivers/tee/optee/core.c > > > > @@ -31,6 +31,18 @@ struct optee_pdata { > > > > optee_invoke_fn *invoke_fn; > > > > }; > > > > > > > > +static const struct { > > > > + const char *drv_name; > > > > + const char *dev_name; > > > > +} optee_bus_probe[] = { > > > > +#ifdef CONFIG_RNG_OPTEE > > > > + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, > > > > +#endif > > > > +#ifdef CONFIG_TPM2_FTPM_TEE > > > > + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, > > > > +#endif > > > > +}; > > > > + > > > > struct rpc_param { > > > > u32 a0; > > > > u32 a1; > > > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) > > > > { > > > > struct optee_pdata *pdata = dev_get_plat(dev); > > > > u32 sec_caps; > > > > - struct udevice *child; > > > > - int ret; > > > > + int ret, i; > > > > > > > > if (!is_optee_api(pdata->invoke_fn)) { > > > > dev_err(dev, "OP-TEE api uid mismatch\n"); > > > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) > > > > * in U-Boot, the discovery of TA on the TEE bus is not supported: > > > > * only bind the drivers associated to the supported OP-TEE TA > > > > */ > > > > - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { > > > > - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); > > > > + > > > > + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { > > > > + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, > > > > + optee_bus_probe[i].dev_name, NULL); > > > > if (ret) > > > > - return ret; > > > > + dev_warn(dev, "Failed to bind device %s\n", > > > > + optee_bus_probe[i].dev_name); > > > > > > Please add device tree nodes for these and all this code can go away. > > > > That's the exact opposite of what the commit message describes. OP-TEE > > supports a scannable bus ifor TAs that behave like hardware blocks and > > doesn't need a DT entry. Since it's really the TAs compilation decision > > to support that or not having them as a DT node is not always the right > > choice. > > This is continuing the perversion of how things are supposed to work > in driver model. Which is not the only thing we need to keep in mind though. > > We need to talk about this because it is simply the wrong way to be > approaching this. This is already part of other software components though, e.g it's already in the kernel. So I don't think it's the wrong approach. > There is nothing wrong with putting things in the DT > and this is how U-Boot works. For now, please create a binding and get > it reviewed. You don't need all the internal objects but you do need > an OP-TEE driver and node, as we have with PCI. Some things *are* working without a DT entry. You had similar concerns on FF-A (where you requested a DT node again) and people gave the exact same response. As long as a bus is scanable in any way, it's preferable to than adding a DT entry. Moreover this code does not prevent anyone from adding a DT entry. To make things even worse if the TA is compiled as 'scanable' and has a DT entry, it might cause issues down the road when being probed by the kernel. So really this is just a patch that makes u-boot behave and plug in properly to the rest of the ecosystem Thanks /Ilias > > Regards, > Simon
Hi Ilias, On Wed, 7 Sept 2022 at 15:32, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Thu, 8 Sept 2022 at 00:11, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Tue, 6 Sept 2022 at 15:23, Ilias Apalodimas > > <ilias.apalodimas@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Tue, Sep 06, 2022 at 03:18:28PM -0600, Simon Glass wrote: > > > > Hi, > > > > > > > > On Tue, 6 Sept 2022 at 03:37, Ilias Apalodimas > > > > <ilias.apalodimas@linaro.org> wrote: > > > > > > > > > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > > > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > > > > > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > > > > > that we already have a workaround for RNG. The details are in > > > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > > > > > > > > > So let's add a list of devices based on U-Boot Kconfig options that we will > > > > > scan until we properly implement the tee-bus functionality. > > > > > > > > > > While at it change the behaviour of the tee core itself wrt to device > > > > > binding. If some device binding fails, print a warning instead of > > > > > disabling OP-TEE. > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > > > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > > > > > --- > > > > > Changes since v3: > > > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > > > > > really needed > > > > > - Changed the style of the optee_bus_probe[] definition to > > > > > {.drv_name = xxx, .dev_name = yyy } > > > > > > > > > > Changes since v2: > > > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > > > > > > > > > Changes since v1: > > > > > - remove a macro and use ARRAY_SIZE directly > > > > > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > > > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > > > > > diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c > > > > > index a89d62aaf0b3..c201a4635e6b 100644 > > > > > --- a/drivers/tee/optee/core.c > > > > > +++ b/drivers/tee/optee/core.c > > > > > @@ -31,6 +31,18 @@ struct optee_pdata { > > > > > optee_invoke_fn *invoke_fn; > > > > > }; > > > > > > > > > > +static const struct { > > > > > + const char *drv_name; > > > > > + const char *dev_name; > > > > > +} optee_bus_probe[] = { > > > > > +#ifdef CONFIG_RNG_OPTEE > > > > > + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, > > > > > +#endif > > > > > +#ifdef CONFIG_TPM2_FTPM_TEE > > > > > + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, > > > > > +#endif > > > > > +}; > > > > > + > > > > > struct rpc_param { > > > > > u32 a0; > > > > > u32 a1; > > > > > @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) > > > > > { > > > > > struct optee_pdata *pdata = dev_get_plat(dev); > > > > > u32 sec_caps; > > > > > - struct udevice *child; > > > > > - int ret; > > > > > + int ret, i; > > > > > > > > > > if (!is_optee_api(pdata->invoke_fn)) { > > > > > dev_err(dev, "OP-TEE api uid mismatch\n"); > > > > > @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) > > > > > * in U-Boot, the discovery of TA on the TEE bus is not supported: > > > > > * only bind the drivers associated to the supported OP-TEE TA > > > > > */ > > > > > - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { > > > > > - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); > > > > > + > > > > > + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { > > > > > + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, > > > > > + optee_bus_probe[i].dev_name, NULL); > > > > > if (ret) > > > > > - return ret; > > > > > + dev_warn(dev, "Failed to bind device %s\n", > > > > > + optee_bus_probe[i].dev_name); > > > > > > > > Please add device tree nodes for these and all this code can go away. > > > > > > That's the exact opposite of what the commit message describes. OP-TEE > > > supports a scannable bus ifor TAs that behave like hardware blocks and > > > doesn't need a DT entry. Since it's really the TAs compilation decision > > > to support that or not having them as a DT node is not always the right > > > choice. > > > > This is continuing the perversion of how things are supposed to work > > in driver model. > > Which is not the only thing we need to keep in mind though. > > > > > We need to talk about this because it is simply the wrong way to be > > approaching this. > > This is already part of other software components though, e.g it's > already in the kernel. So I don't think it's the wrong approach. > > > There is nothing wrong with putting things in the DT > > and this is how U-Boot works. For now, please create a binding and get > > it reviewed. You don't need all the internal objects but you do need > > an OP-TEE driver and node, as we have with PCI. > > Some things *are* working without a DT entry. You had similar > concerns on FF-A (where you requested a DT node again) and people gave > the exact same response. As long as a bus is scanable in any way, > it's preferable to than adding a DT entry. Moreover this code does > not prevent anyone from adding a DT entry. > > To make things even worse if the TA is compiled as 'scanable' and has > a DT entry, it might cause issues down the road when being probed by > the kernel. So really this is just a patch that makes u-boot behave > and plug in properly to the rest of the ecosystem Calling device_bind() is supposed to be used in extremis. I don't see any scanning of an OP-TEE bus here. I just see it binding two child devices which are hard-coded in U-Boot. What am I missing? This appears to be a Linaro binding, so you should be able to update it easily enough. Regards, Simon
Hi Simon, > > > > > > > > > > > > Late versions of OP-TEE support a pseudo bus. TAs that behave as > > > > > > hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can > > > > > > scan. Unfortunately U-Boot doesn't support that yet. It's worth noting > > > > > > that we already have a workaround for RNG. The details are in > > > > > > commit 70812bb83da6 ("tee: optee: bind rng optee driver") > > > > > > > > > > > > So let's add a list of devices based on U-Boot Kconfig options that we will > > > > > > scan until we properly implement the tee-bus functionality. > > > > > > > > > > > > While at it change the behaviour of the tee core itself wrt to device > > > > > > binding. If some device binding fails, print a warning instead of > > > > > > disabling OP-TEE. > > > > > > > > > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > > > Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> > > > > > > Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> > > > > > > --- > > > > > > Changes since v3: > > > > > > - Use NULL instead of a child ptr on device_bind_driver(), since it's not > > > > > > really needed > > > > > > - Changed the style of the optee_bus_probe[] definition to > > > > > > {.drv_name = xxx, .dev_name = yyy } > > > > > > > > > > > > Changes since v2: > > > > > > - Fixed typo on driver name ftpm-tee -> ftpm_tee > > > > > > > > > > > > Changes since v1: > > > > > > - remove a macro and use ARRAY_SIZE directly > > > > > > drivers/tee/optee/core.c | 24 +++++++++++++++++++----- > > > > > > 1 file changed, 19 insertions(+), 5 deletions(-) > > > > > > [...] > > > > Some things *are* working without a DT entry. You had similar > > concerns on FF-A (where you requested a DT node again) and people gave > > the exact same response. As long as a bus is scanable in any way, > > it's preferable to than adding a DT entry. Moreover this code does > > not prevent anyone from adding a DT entry. > > > > To make things even worse if the TA is compiled as 'scanable' and has > > a DT entry, it might cause issues down the road when being probed by > > the kernel. So really this is just a patch that makes u-boot behave > > and plug in properly to the rest of the ecosystem > > Calling device_bind() is supposed to be used in extremis. I don't see > any scanning of an OP-TEE bus here. I just see it binding two child > devices which are hard-coded in U-Boot. What am I missing? The commit description describes the current state of U-Boot > > This appears to be a Linaro binding, so you should be able to update > it easily enough. Linaro binding? The DT is governed by a spec, we commit everything upstream there. OTOH I still don't see what we need to put in there. As we discussed this is a bus that can be used to scan devices. Thanks /Ilias > > Regards, > Simon
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index a89d62aaf0b3..c201a4635e6b 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -31,6 +31,18 @@ struct optee_pdata { optee_invoke_fn *invoke_fn; }; +static const struct { + const char *drv_name; + const char *dev_name; +} optee_bus_probe[] = { +#ifdef CONFIG_RNG_OPTEE + { .drv_name = "optee-rng", .dev_name = "optee-rng" }, +#endif +#ifdef CONFIG_TPM2_FTPM_TEE + { .drv_name = "ftpm_tee", .dev_name = "ftpm_tee" }, +#endif +}; + struct rpc_param { u32 a0; u32 a1; @@ -642,8 +654,7 @@ static int optee_probe(struct udevice *dev) { struct optee_pdata *pdata = dev_get_plat(dev); u32 sec_caps; - struct udevice *child; - int ret; + int ret, i; if (!is_optee_api(pdata->invoke_fn)) { dev_err(dev, "OP-TEE api uid mismatch\n"); @@ -672,10 +683,13 @@ static int optee_probe(struct udevice *dev) * in U-Boot, the discovery of TA on the TEE bus is not supported: * only bind the drivers associated to the supported OP-TEE TA */ - if (IS_ENABLED(CONFIG_RNG_OPTEE)) { - ret = device_bind_driver(dev, "optee-rng", "optee-rng", &child); + + for (i = 0; i < ARRAY_SIZE(optee_bus_probe); i++) { + ret = device_bind_driver(dev, optee_bus_probe[i].drv_name, + optee_bus_probe[i].dev_name, NULL); if (ret) - return ret; + dev_warn(dev, "Failed to bind device %s\n", + optee_bus_probe[i].dev_name); } return 0;
Late versions of OP-TEE support a pseudo bus. TAs that behave as hardware blocks (e.g TPM, RNG etc) present themselves on a bus which we can scan. Unfortunately U-Boot doesn't support that yet. It's worth noting that we already have a workaround for RNG. The details are in commit 70812bb83da6 ("tee: optee: bind rng optee driver") So let's add a list of devices based on U-Boot Kconfig options that we will scan until we properly implement the tee-bus functionality. While at it change the behaviour of the tee core itself wrt to device binding. If some device binding fails, print a warning instead of disabling OP-TEE. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> --- Changes since v3: - Use NULL instead of a child ptr on device_bind_driver(), since it's not really needed - Changed the style of the optee_bus_probe[] definition to {.drv_name = xxx, .dev_name = yyy } Changes since v2: - Fixed typo on driver name ftpm-tee -> ftpm_tee Changes since v1: - remove a macro and use ARRAY_SIZE directly drivers/tee/optee/core.c | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-)