Hi Ilias, On Tue, 15 Mar 2022 at 00:34, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote: > > Hi Simon, > > On Mon, 14 Mar 2022 at 20:24, Simon Glass <s...@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