Re: [PATCH 08/12] tpm/tpm_tis: Split tpm_tis driver into a core and TCG TIS compliant phy

[Christophe Ricard <christophe.ricard-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Jason,

My answers/questions below.

Best Regards
Christophe

On 21/03/2016 02:32, Jason Gunthorpe wrote:
> On Sun, Mar 20, 2016 at 08:34:39PM +0100, Christophe Ricard wrote:
>> +++ b/drivers/char/tpm/tpm.h
>> @@ -142,6 +142,7 @@ struct tpm_vendor_specific {
>>   
>>   	int region_size;
>>   	int have_region;
>> +	bool itpm;
>>   
>>   	struct list_head list;
>>   	int locality;
>> @@ -149,6 +150,7 @@ struct tpm_vendor_specific {
>>   	bool timeout_adjusted;
>>   	unsigned long duration[3]; /* jiffies */
>>   	bool duration_adjusted;
>> +	bool irq_tested;
>>   	void *priv;
> No adding driver specific stuff to this structure.
I suppose irq_tested is fine because it is common all drivers and 
related to irq management. I guess it can be moved below irq field.
I somewhat understand your mean for itpm.
I believe it is specific for the legacy/lpc/0xfed4xxxx addressing mode. 
Can you confirm ?

For itpm workaround, i would like to use 2 additional specific fields in 
tpm_class_ops called req_expected_val and req_expected_mask.
req_expected_val and req_expected_mask would be set in all phys with 
TPM_STS_DATA_EXPECT.
An additional tpm_class_ops ops called for example tpm_postprobe for 
additional phys specific operations handling in tpm_tis the itpm workaround.
req_expected_val and req_expected_mask would be set to 0 in tpm_tis in 
case itpm is set to true and probe_itpm set itpm to true.

This would remove the need of any itpm mention in the tpm_tis_core 
layer. What do you think ?
>
>>   	list_for_each_entry(id, &dev->pnp.ids, list)
>> -		if (!strcmp(hid, id->id))
>> -			return 1;
>> +	if (!strcmp(hid, id->id))
>> +		return 1;
> useless change
Ok
>> +static const struct tis_vendor_timeout_override vendor_timeout_overrides[] = {
>> +	/* Atmel 3204 */
>> +	{ 0x32041114, { (TIS_SHORT_TIMEOUT * 1000), (TIS_LONG_TIMEOUT * 1000),
>> +			(TIS_SHORT_TIMEOUT * 1000), (TIS_SHORT_TIMEOUT * 1000) } },
>> +};
> This initializer should not be in a header file
I will move it in tpm_tis_core.c
>
>> +/* Helpers - read */
>> +static inline int tpm_read_bytes(struct tpm_chip *chip, u32 addr,
>> +				 size_t len, u8 *res)
>> +{
>> +	return chip->ops->read_bytes(chip, addr, len, 1, res);
>> +}
>> +static inline u8 tpm_read_byte(struct tpm_chip *chip, u32 addr)
> Are these all actually used? I would have thought only read32 would be
> needed. Please use better names, tpm_read32,
> read8, etc.
I did a quick check of the complete set and found all of them are used.
> Jason


------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785351&iu=/4140