Hi Ilias,
On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Simon,
On Mon, 14 Mar 2022 at 20:24, Simon Glass sjg@chromium.org wrote:
[...]
}This really should be in the device tree so what you are doing here is quite strange.
Like I had mentioned in my earlier emails, the TPM device has a builtin RNG functionality, which is non-optional. So I don't understand why we need to use a device tree subnode here. Whether the device is being bound to the parent is being controlled by the TPM_RNG config that you asked me to put in my previous version, which I am doing.
See how PMICs work, for example. We have GPIOs, regulators and suchlike in the PMIC and we add subnodes for them in the DT. It is just how it is done.
Driver model is designed to automatically bind devices based on the device tree. There are cases where it is necessary to manually bind things, but we mustn't prevent people from doing it 'properly'.
There's a small difference here though. The RNG is not a device. The TPM is the device and an encoded command to that device returns a random number. There's no binding initiating etc.
A device is just something with a struct udevice, so I don't see the random number generator as anything different from another device. We might have a white-noise generator which produces random numbers. Just because the feature is packaged inside a single chip doesn't make it any less a device. Just like the PMIC.
Finally, I know you keep saying that random numbers are only needed in U-Boot proper, but if I want a random number in SPL, it may not work, since device_bind() is often not available, for code-size reasons.
And the entire tpm framework will fit?
Yes. For verified boot it has to, since you cannot init RAM until you have selected your A/B/recovery image.
So that is why I say that what you are doing is quite strange. Perhaps you are coming from a different project, which does things differently.
I don't personally find it strange. The device is already described in the DTS and I don't see a strong reason to deviate for the upstream version again.
Linux tends to rely a lot more on manually adding devices. It can have a pretty dramatic bloating effect on code size in U-Boot.
Anyway, so long as we can detect an existing device, as I explained below, it is fine to manually add it when it is missing.
Regards /Ilias
If you want to manually bind it, please call
device_find_first_child_by_uclass() first to make sure it isn't already there.
Okay
Also you should bind it in the bind() method, not in probe().
Okay
This is the code used for the same thing in the bootstd series:
struct udevice *bdev; int ret;
ret = device_find_first_child_by_uclass(parent, UCLASS_BOOTDEV, &bdev); if (ret) { if (ret != -ENODEV) { log_debug("Cannot access bootdev device\n"); return ret; }
ret = bootdev_bind(parent, drv_name, "bootdev", &bdev); if (ret) { log_debug("Cannot create bootdev device\n"); return ret; } }
return 0;+}
UCLASS_DRIVER(tpm) = {
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,
.id = UCLASS_TPM,.name = "tpm",.flags = DM_UC_FLAG_SEQ_ALIAS,#if CONFIG_IS_ENABLED(OF_REAL)
.post_bind = dm_scan_fdt_dev,
.post_bind = dm_scan_fdt_dev,#endif
.post_probe = tpm_uclass_post_probe,Should be post_bind.
Okay
[..]
Regards, Simon