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

[Ajay Gupta <ajaykuee@gmail.com>]

Hi Peter

> On Sep 10, 2018, at 11:29 PM, Peter Rosin <peda@axentia.se> wrote:
> 
>> 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?
Yes
> 
>>>> 
>>>> 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
I guess you meant
0x10 at lower byte D0:7 and
0xf0 at upper byte D8:15
Similarly,
0x06 at D0:7
0x00 at D8:15
I don’t see any issue here. Consider a 16 bit offset where CCGX_RAB_INTR_REG is at 0x0006 and other one at 0xf010
> 
> 
> 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?
Yes, they work as is on multiple systems.

Thanks
Ajay

—nvpublic
> 
> Cheers,
> Peter