Re: [PATCH v3 1/2] tpm: Add tpm_tis_i2c backend for tpm_tis_core

From: Alexander Steffen
Date: Mon May 23 2022 - 12:10:31 EST

On 23.05.22 11:44, Krzysztof Kozlowski wrote:
On 23/05/2022 11:32, Krzysztof Kozlowski wrote:
On 23/05/2022 10:55, Alexander Steffen wrote:
On 22.05.22 10:30, Krzysztof Kozlowski wrote:
Without bindings, new compatibles and properties cannot be accepted, so NAK.

Could you be more specific as to what the correct solution is here?
Usually, I'd just look at what the existing code does, but that is a
little messy:



* socionext,synquacer-tpm-mmio is documented only in
Documentation/devicetree/bindings/trivial-devices.yaml

* nuvoton,npct601 is documented in trivial-devices.yaml and is also
mentioned in Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt

* nuvoton,npct650 is only mentioned in tpm-i2c.txt, but appears nowhere
in the code

* infineon,tpm_i2c_infineon appears only in tpm_i2c_infineon.c, but is
documented nowhere

* tpm_tis_spi_main.c has all its compatibles documented in
tpm_tis_spi.txt, except google,cr50, which is documented in
google,cr50.txt, even though it has the same properties

* tpm_tis_i2c_cr50.c uses the exact same google,cr50, even though that
is explicitly documented as a device "on SPI Bus" and lists
spi-max-frequency as one of its required properties, which does not make
any sense for an I2C device

* According to the feedback in
https://patchwork.kernel.org/project/linux-integrity/patch/20220404081835.495-4-johannes.holland@xxxxxxxxxxxx/#24801807,
the text format, that is currently used everywhere in
Documentation/devicetree/bindings/security/tpm/, is deprecated anyway
and should be replaced by YAML



So would you be okay with just adding the compatibles from tpm_tis_i2c.c
to trivial-devices.yaml, so that checkpatch does not complain anymore,
and leave the cleanup of the mess above for later?

To trivial-devices you should add only bindings of really trivial
devices, which do not have any other properties, even when the bindings
are finished. This means you describe fully the hardware and still have
only reg+compatible.

If this device fits such case - no other hardware properties than reg -
then, feel free to document it in trivial-devices. However I am not sure
that TPM devices are that trivial... For example tpm-i2c.txt defines
also interrupts and label.

If the device is not trivial, it should be documented in bindings,
either dedicated or some existing ones.

Ok, let's see whether I understood that correctly:



I think, in general, TPMs are trivial devices: They sit on the I2C or SPI bus waiting for commands, but don't do much else. They might have TPM-specific properties, like whether they implement the 1.2 or 2.0 command set, but we don't encode those in the device tree, since the driver tries to detect the available functionality dynamically (which makes sense, since some TPMs can be upgraded from 1.2 to 2.0, so that is not a static property of the device). Other properties, such as the maximum SPI frequency, are not TPM-specific, but apply to every SPI device and might be limited by either the SPI device itself or the SPI controller (or the user, wishing to run at lower frequencies, for whatever reason).



Looking at the code, there are some TPM-specific properties in use though: There is the powered-while-suspended flag, which only the TPM driver looks at (in drivers/char/tpm/eventlog/of.c). It is not specific to a single TPM (a single compatible string), but can be set for all the TPMs. Also, the linux,sml-* properties might be TPM-specific, though they get set in arch/powerpc/kernel/prom_init.c to communicate some information to the TPM driver. And there is lpcpd-gpios, which is only used by st33zp24.



Now the purpose of trivial-devices.yaml is to specify a schema of valid device definitions. It only allows the properties reg, interrupts and spi-max-frequency in addition to the compatible strings. So, strictly speaking, none of the TPMs should be listed there, since all the TPMs can, in theory, use the powered-while-suspended flag, which is not allowed by the schema. With other properties the schema does not seem to be too strict, since it applies to both I2C and SPI devices, but allows the spi-max-frequency property, even though that does not make sense for I2C devices.



So the correct solution could be this: Replace all the text files in Documentation/devicetree/bindings/security/tpm/ with a single trivial-tpms.yaml (similar to trivial-devices.yaml) and also move all the TPMs from trivial-devices.yaml there. This allows to properly document the powered-while-suspended flag and other generic TPM properties. st33zp24 should get its own YAML, to document lpcpd-gpios, that is only used by this driver. I'm not quite sure what to do with ibmvtpm.txt, since that seems to document several properties, that are not referenced in the TPM driver at all but instead get used by some scsi driver (e.g. ibm,loc-code), so I'd probably ignore it for now. What do you think?



As for tpm_tis_i2c, if there are no other objections to its current state, I'd add its compatible strings to trivial-devices.yaml for now, then do the cleanup as described above later. It does not make the code more wrong, since trivial-devices.yaml already contains some TPM devices, that are very similar to what tpm_tis_i2c now supports (i.e. that also don't have special properties), but allows for more time to do the cleanup properly, without holding up tpm_tis_i2c.



Kind regards

Alexander