Date:   Mon, 23 May 2022 18:10:03 +0200
From:   Alexander Steffen <Alexander.Steffen@...ineon.com>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        <jarkko@...nel.org>, <linux-kernel@...r.kernel.org>,
        <linux-integrity@...r.kernel.org>
CC:     <peterhuewe@....de>, <jgg@...pe.ca>,
        <krzysztof.kozlowski+dt@...aro.org>,
        Johannes Holland <johannes.holland@...ineon.com>,
        Amir Mizinski <amirmizi6@...il.com>
Subject: Re: [PATCH v3 1/2] tpm: Add tpm_tis_i2c backend for tpm_tis_core

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@infineon.com/#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

Powered by blists - more mailing lists