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
