[v10,2/2] usb: typec: ucsi: add support for Cypress CCGx

[Peter Rosin <peda@axentia.se>]

On 2018-09-11 06:30, Ajay Gupta wrote:
> Hi Peter,
> 
>>>>>>>>>> +static int ucsi_ccg_send_data(struct ucsi_ccg *uc) {
>>>>>>>>>> +	unsigned char buf1[USBC_MSG_OUT_SIZE];
>>>>>>>>>> +	unsigned char buf2[USBC_CONTROL_SIZE];
>>>>>>>>>> +	int status;
>>>>>>>>>> +	u16 rab;
>>>>>>>>>> +
>>>>>>>>>> +	memcpy(buf1, (u8 *)(uc->ppm.data) +
>>>> USBC_MSG_OUT_OFFSET,
>>>>>>>>> sizeof(buf1));
>>>>>>>>>> +	memcpy(buf2, (u8 *)(uc->ppm.data) +
>>>> USBC_CONTROL_OFFSET,
>>>>>>>>>> +sizeof(buf2));
>>>>>>>>>
>>>>>>>>> Hmm, now that I see what this function does, instead of just
>>>>>>>>> seeing a bunch of magic numbers, I wonder why you make copies
>>>>>>>>> instead of feeding the correct section of the ppm.data buffer
>>>>>>>>> directly to ccg_write, like you do below for recv?
>>>>>>>> Ok, will fix.
>>>>>>>
>>>>>>> Hmm, now that I see this again, it makes me wonder why you
>>>>>>> complained about copying the buffer to fix the misunderstanding of
>>>>>>> the i2c_transfer interface, when you already copy the buffer in
>>>>>>> the first
>>>> place?
>>>>>> Copy is indeed not needed. I will fix it in next version.
>>>>>> We will have to do copy in ccg_write()if we try to combine two
>>>>>> write i2c_msg into one and I want to rather stay with two i2c_msg
>>>>>> to avoid
>>>> copy.
>>>>>> Also master_xfer() will become tricky since rab write for read alsp
>>>>>> has to go
>>>>> first.
>>>>>
>>>>> You are stuck with the construction of the extended buffer. See my
>>>>> mail in the
>>>>> 1/2 thread.
>>>>>
>>>>>>>>>> +	rab =
>>>> CCGX_I2C_RAB_UCSI_DATA_BLOCK(USBC_VERSION_OFFSET);
>>>>>>>>>> +	status = ccg_read(uc, rab, (u8 *)(uc->ppm.data) +
>>>>>>>>> USBC_VERSION_OFFSET,
>>>>>>>>>> +			  USBC_VERSION_SIZE);
>>>>>>>>>
>>>>>>>>> E.g.
>>>>>>>>> 	rab = CCGX_I2C_RAB_UCSI_DATA_BLOCK(offsetof(struct
>> ucsi_data,
>>>>>>>>> version));
>>>>>>>>> 	status = ccg_read(uc, rab, (u8 *)&uc->ppm.data->version,
>>>>>>>>> 			  sizeof(uc->ppm.data->version));
>>>>>>>>>
>>>>>>>>> Hmm, but this highlights that you are not doing any endian
>>>>>>>>> conversion of the fields in that struct as you read/write it.
>>>>>>>>
>>>>>>>>> Do you need to in case you have an endian mismatch?
>>>>>>>> Looks like don't need it. I have tested it and it works as is.
>>>>>>>
>>>>>>> Yeah, but have you tested the driver on a machine with the other
>>>>>>> byte-
>>>> sex?
>>>>>> No, I think better to convert to desired endian.
>>>>>
>>>>> The device has a specific endianess. The host cpu has a specific endianess.
>>>>> You transfer data byte-by-byte to/from a struct that appears to have
>>>>> multi- byte integers, e.g. the 16-bit version. You do not do any
>>>>> conversion that I see and you report that it works. So, there are
>>>>> two cases. Either
>>>>>
>>>>> 1. your host cpu and the device has the same endianess, and it all just
>>>>>    works by accident
>>>>>
>>>>> or
>>>>>
>>>>> 2. whatever is consuming the ppm data does the endian conversion for
>> you
>>>>>    on "the other side", and it all just works by design.
>>>>>
>>>>> I have no idea which it is since I know nothing about whatever
>>>>> handles the ppm data on the other side of that ucsi_register_ppm call. So,
>> I asked.
>>>> UCSI specification requires the ppm data to be in little-endian format.
>>>>
>>>> Below is from the UCSI specification.
>>>> "All multiple byte fields in this specification are interpreted as
>>>> and moved over the bus in little-endian order, i.e., LSB to MSB unless
>> otherwise specified"

Taking another peek into the UCSI spec, and I do not find any mention of
any rab. So, I think the rab is out of scope for that spec. I.e. that the
rab falls into this bucket:

	This specification does not define the method to use
	(PCIe/ACPI/I2C/etc.) in order to interface with the PPM.
	It is left to individual system manufacturers to determine 
	what bus/protocol they use to expose the PPM.

What abbreviation is rab anyway? Register Address Block?

>>>
>>> Do we still need any conversion here? The ppm data is now directly fed
>>> for read and write both and rab should be in little endian as per macro.
>>> #define CCGX_RAB_UCSI_DATA_BLOCK(offset)        (0xf000 | ((offset) & 0xff))
>>
>> What do you mean by "in little endian as per macro"?
>> Should not the non-offset 0xf0 byte of CCGX_RAB_UCSI_DATA_BLOCK 
>> be in the other byte of rab
> Shouldn't it be always in bits D[8:15] of rab so that it gets written to 
> buf[1] (2nd byte) in ccg_read/write() ?
> 
>> compared to e.g. the 0x06 byte of CCGX_RAB_INTR_REG?
>>
>> I assumed *all* CCGX_RAB_... defines to be in cpu-native endian. 
>> Are they not?
> How to know/confirm this?

I don't know what you want to have on the wire, but for the code as
written, a rab that is CCGX_RAB_UCSI_DATA_BLOCK(0x10) gets the bytes
0x10,0xf0 and for a rab that is CCGX_RAB_INTR_REG you get 0x06,0x00

What makes me a little worried is that the offset of the data-block
rab appears where the significant bits of the other rabs are.
However, that might not be a problem at all since the second byte
of the data-block rab (0xf0) appears to be distinct from all other
rabs. It just looks like the data-block rab might have been
byte-swapped when comparing it to all the other rabs you have defined.

If you can run the code and see that it works, then obviously you
are getting the byte-ordering of the rabs correct. No?

Cheers,
Peter