[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter

> >>>>>>>>> +		if (msgs[i].flags & I2C_M_RD) {
> >>>>>>>>> +			/* gpu_i2c_read has implicit start and stop */
> >>>>>>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf,
> >> msgs[i].len);
> >>>>>>>>> +			if (status < 0)
> >>>>>>>>> +				return status;
> >>>>>>>>> +		} else {
> >>>>>>>>> +			/* start on first write message */
> >>>>>>>>> +			if (i == 0) {
> >>>>>>>>
> >>>>>>>> This "if (i == 0)" test is completely bogus to me. I fail to
> >>>>>>>> see why the meat of the block should not happen for both writes
> >>>>>>>> in a double-write transfer.
> >>>>>>>>
> >>>>>>>> If the second message is a write, you do not issue any start
> >>>>>>>> nor do you write out the address for the second message. You
> >>>>>>>> want to generate the following for a transfer consisting of 2 write-
> messages:
> >>>>>>>>
> >>>>>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
> >>>>>>>>                              =============
> >>>>>>>>
> >>>>>>>> (where "..." denotes further optional "Data [A]" instances)
> >>>>>>>>
> >>>>>>>> As is, the stuff underlined by equal signs are not generated,
> >>>>>>>> at least as I read the code.
> >>>>>>>>
> >>>>>>>> This is what I meant in my comment around this area for the v9
> patch.
> >>>>>>>
> >>>>>>> Oh, I just realized, this probably means that the ccg_write
> >>>>>>> function in patch
> >>>>>>> 2/2 asks for the wrong thing. If this code actually works, the
> >>>>>>> client driver should probably ask for a single-message transfer
> >>>>>>> consisting of the 2-byte rab concatenated with the data buffer.
> >>>>>>> And that actually makes sense, there is no reason to split the
> >>>>>>> two
> >>>>>>> (dependent) parts of the write into separate messages.
> >>>>>> That would require to create new buffer and copy data for each
> >>>>>> write request from UCSI core driver for sending UCSI command.
> >>>>>> This doesn't look proper way of doing it.
> >>>>>
> >>>>> Well, that's the way master_xfer works, so you will just have to
> >>>>> live with it if you do not want a stop in the middle.
> >>>>
> >>>> Bzzt, s/stop/restart/
> >>> Obviously, a stop in middle will not work. For any read or write
> >>> request,
> >>
> >> Yes, with "s/stop/restart/", I meant "replace stop with restart", and
> >> not "stop followed by restart". That's pretty standard notation, but
> >> sorry if that was not clear.
> >>
> >>> 2 byte rab write has to go first. We anyway have to separate rab
> >>> part in read transactions so write should be okay to have separate
> >>> rab write message.
> >>
> >> That is not at all certain. The two below transfers need not mean the
> >> same thing at all.
> >>
> >> S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P S Addr Wr [A] rab1 [A]
> >> rab2 [A] S Addr Wr [A] Data [A] ... P
> >>
> >> In fact, I strongly suspect that for the latter case, the device will
> >> interpret the first two Data bytes as rab and that you therefore have
> >> to make sure that the first transfer (w/o restart) is what you generate in
> ccg_write.
> >>
> >>> All the client driver of this needs to create messages in pairs
> >>> where first one will always be a rab write and second one either read or
> write.
> >>> I can add a check for this condition in master_xfer if needed.
> >>>
> >>> Do you still see any issue with this?
> >>
> >> Yes, this is very much an issue. master_xfer has to start each
> >> message with a start for the first message, and a restart for
> >> subsequent messages. And after the final message there should be a
> >> stop. All starts/restarts should be followed by the address and the
> >> read/write bit. Then data in the given direction. The master_xfer
> >> function should never do any device specific checks for device
> >> specific conditions. It simply should not care about which client
> >> driver is currently using it, and instead just stick to implementing the
> master_xfer api so that all client drivers know what to expect. No cludges.
> >
> > I didn't get how read will happen. Read will also require a 2 byte rab
> > write and followed by read.
> >
> > Are you saying to have two messages for reads [rab + read] and
> > combined (rab and write) single message for write?
> > Then for read: rab is written first followed by reads and then actual explicit
> stop?
> >
> > for (i = 0; i < num; i++)
> > 	if (Read) {
> > 		read (without stop);
> > 	} else  {
> > 		start;
> > 		addr;
> > 		write;
> > 	}
> > }
> > stop;
> 
> Yes, exactly like that (just missing a { for the for loop). If you can do reads
> without an implicit stop, that's excellent news!
> 
> My guess is that with the above, you can actually program the I2C_MST_ADDR
> register inside the "if (Read)" branch and then remove the I2C_AQ_COMB |
> I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.

We need I2C_MST_ADDR to be programmed even in a single write message.
If we move it inside "if (Read)" branch then a single write message will not get it.

How does the below look?

for (i = 0; i < num; i++) {
	write msg[i].addr to I2C_MST_ADDR;
 	if (Read) {
 		read (without stop);
 	} else  {
 		start;
 		addr;
 		write;
 	}
 }
 stop;

Thanks
Ajay

--
nvpublic
--
> Cheers,
> Peter
> 
> >
> > Thanks
> > Ajay
> >
> > --
> > nvpublic
> > --
> >>
> >> Cheers,
> >> Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 23:38, Ajay Gupta wrote:
>> My guess is that with the above, you can actually program the I2C_MST_ADDR
>> register inside the "if (Read)" branch and then remove the I2C_AQ_COMB |
>> I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.
> 
> We need I2C_MST_ADDR to be programmed even in a single write message.
> If we move it inside "if (Read)" branch then a single write message will not get it.
> 
> How does the below look?
> 
> for (i = 0; i < num; i++) {
> 	write msg[i].addr to I2C_MST_ADDR;
>  	if (Read) {
>  		read (without stop);
>  	} else  {
>  		start;
>  		addr;
>  		write;
>  	}
>  }
>  stop;

Sure, that's perfectly fine. I just don't see why the chip needs that for
writes when the address has to be written manually anyway, but if it does
need it, then so it is (and if it doesn't need it, the penalty for the
extra register write is probably not all that significant...)

Cheers,
Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 22:04, Ajay Gupta wrote:
> Hi Peter,
> 
>>>>>>>>> +		if (msgs[i].flags & I2C_M_RD) {
>>>>>>>>> +			/* gpu_i2c_read has implicit start and stop */
>>>>>>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf,
>> msgs[i].len);
>>>>>>>>> +			if (status < 0)
>>>>>>>>> +				return status;
>>>>>>>>> +		} else {
>>>>>>>>> +			/* start on first write message */
>>>>>>>>> +			if (i == 0) {
>>>>>>>>
>>>>>>>> This "if (i == 0)" test is completely bogus to me. I fail to see
>>>>>>>> why the meat of the block should not happen for both writes in a
>>>>>>>> double-write transfer.
>>>>>>>>
>>>>>>>> If the second message is a write, you do not issue any start nor
>>>>>>>> do you write out the address for the second message. You want to
>>>>>>>> generate the following for a transfer consisting of 2 write-messages:
>>>>>>>>
>>>>>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>>>>>>                              =============
>>>>>>>>
>>>>>>>> (where "..." denotes further optional "Data [A]" instances)
>>>>>>>>
>>>>>>>> As is, the stuff underlined by equal signs are not generated, at
>>>>>>>> least as I read the code.
>>>>>>>>
>>>>>>>> This is what I meant in my comment around this area for the v9 patch.
>>>>>>>
>>>>>>> Oh, I just realized, this probably means that the ccg_write
>>>>>>> function in patch
>>>>>>> 2/2 asks for the wrong thing. If this code actually works, the
>>>>>>> client driver should probably ask for a single-message transfer
>>>>>>> consisting of the 2-byte rab concatenated with the data buffer.
>>>>>>> And that actually makes sense, there is no reason to split the two
>>>>>>> (dependent) parts of the write into separate messages.
>>>>>> That would require to create new buffer and copy data for each
>>>>>> write request from UCSI core driver for sending UCSI command. This
>>>>>> doesn't look proper way of doing it.
>>>>>
>>>>> Well, that's the way master_xfer works, so you will just have to
>>>>> live with it if you do not want a stop in the middle.
>>>>
>>>> Bzzt, s/stop/restart/
>>> Obviously, a stop in middle will not work. For any read or write
>>> request,
>>
>> Yes, with "s/stop/restart/", I meant "replace stop with restart", and not "stop
>> followed by restart". That's pretty standard notation, but sorry if that was not
>> clear.
>>
>>> 2 byte rab write has to go first. We anyway have to separate rab part
>>> in read transactions so write should be okay to have separate rab
>>> write message.
>>
>> That is not at all certain. The two below transfers need not mean the same
>> thing at all.
>>
>> S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P S Addr Wr [A] rab1 [A] rab2 [A] S
>> Addr Wr [A] Data [A] ... P
>>
>> In fact, I strongly suspect that for the latter case, the device will interpret the
>> first two Data bytes as rab and that you therefore have to make sure that the
>> first transfer (w/o restart) is what you generate in ccg_write.
>>
>>> All the client driver of this needs to create messages in pairs where
>>> first one will always be a rab write and second one either read or write.
>>> I can add a check for this condition in master_xfer if needed.
>>>
>>> Do you still see any issue with this?
>>
>> Yes, this is very much an issue. master_xfer has to start each message with a
>> start for the first message, and a restart for subsequent messages. And after
>> the final message there should be a stop. All starts/restarts should be
>> followed by the address and the read/write bit. Then data in the given
>> direction. The master_xfer function should never do any device specific
>> checks for device specific conditions. It simply should not care about which
>> client driver is currently using it, and instead just stick to implementing the
>> master_xfer api so that all client drivers know what to expect. No cludges.
> 
> I didn't get how read will happen. Read will also require a 2 byte rab write and
> followed by read. 
> 
> Are you saying to have two messages for reads [rab + read]
> and combined (rab and write) single message for write?
> Then for read: rab is written first followed by reads and then actual explicit stop?
> 
> for (i = 0; i < num; i++)
> 	if (Read) {
> 		read (without stop);
> 	} else  {
> 		start;
> 		addr;
> 		write;
> 	}
> }
> stop;

Yes, exactly like that (just missing a { for the for loop). If you can do reads
without an implicit stop, that's excellent news!

My guess is that with the above, you can actually program the I2C_MST_ADDR
register inside the "if (Read)" branch and then remove the
I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR quirks.

Cheers,
Peter

> 
> Thanks
> Ajay
> 
> --
> nvpublic
> --
>>
>> Cheers,
>> Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter,

> >>>>>>> +		if (msgs[i].flags & I2C_M_RD) {
> >>>>>>> +			/* gpu_i2c_read has implicit start and stop */
> >>>>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf,
> msgs[i].len);
> >>>>>>> +			if (status < 0)
> >>>>>>> +				return status;
> >>>>>>> +		} else {
> >>>>>>> +			/* start on first write message */
> >>>>>>> +			if (i == 0) {
> >>>>>>
> >>>>>> This "if (i == 0)" test is completely bogus to me. I fail to see
> >>>>>> why the meat of the block should not happen for both writes in a
> >>>>>> double-write transfer.
> >>>>>>
> >>>>>> If the second message is a write, you do not issue any start nor
> >>>>>> do you write out the address for the second message. You want to
> >>>>>> generate the following for a transfer consisting of 2 write-messages:
> >>>>>>
> >>>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
> >>>>>>                              =============
> >>>>>>
> >>>>>> (where "..." denotes further optional "Data [A]" instances)
> >>>>>>
> >>>>>> As is, the stuff underlined by equal signs are not generated, at
> >>>>>> least as I read the code.
> >>>>>>
> >>>>>> This is what I meant in my comment around this area for the v9 patch.
> >>>>>
> >>>>> Oh, I just realized, this probably means that the ccg_write
> >>>>> function in patch
> >>>>> 2/2 asks for the wrong thing. If this code actually works, the
> >>>>> client driver should probably ask for a single-message transfer
> >>>>> consisting of the 2-byte rab concatenated with the data buffer.
> >>>>> And that actually makes sense, there is no reason to split the two
> >>>>> (dependent) parts of the write into separate messages.
> >>>> That would require to create new buffer and copy data for each
> >>>> write request from UCSI core driver for sending UCSI command. This
> >>>> doesn't look proper way of doing it.
> >>>
> >>> Well, that's the way master_xfer works, so you will just have to
> >>> live with it if you do not want a stop in the middle.
> >>
> >> Bzzt, s/stop/restart/
> > Obviously, a stop in middle will not work. For any read or write
> > request,
> 
> Yes, with "s/stop/restart/", I meant "replace stop with restart", and not "stop
> followed by restart". That's pretty standard notation, but sorry if that was not
> clear.
> 
> > 2 byte rab write has to go first. We anyway have to separate rab part
> > in read transactions so write should be okay to have separate rab
> > write message.
> 
> That is not at all certain. The two below transfers need not mean the same
> thing at all.
> 
> S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P S Addr Wr [A] rab1 [A] rab2 [A] S
> Addr Wr [A] Data [A] ... P
> 
> In fact, I strongly suspect that for the latter case, the device will interpret the
> first two Data bytes as rab and that you therefore have to make sure that the
> first transfer (w/o restart) is what you generate in ccg_write.
> 
> > All the client driver of this needs to create messages in pairs where
> > first one will always be a rab write and second one either read or write.
> > I can add a check for this condition in master_xfer if needed.
> >
> > Do you still see any issue with this?
> 
> Yes, this is very much an issue. master_xfer has to start each message with a
> start for the first message, and a restart for subsequent messages. And after
> the final message there should be a stop. All starts/restarts should be
> followed by the address and the read/write bit. Then data in the given
> direction. The master_xfer function should never do any device specific
> checks for device specific conditions. It simply should not care about which
> client driver is currently using it, and instead just stick to implementing the
> master_xfer api so that all client drivers know what to expect. No cludges.

I didn't get how read will happen. Read will also require a 2 byte rab write and
followed by read. 

Are you saying to have two messages for reads [rab + read]
and combined (rab and write) single message for write?
Then for read: rab is written first followed by reads and then actual explicit stop?

for (i = 0; i < num; i++)
	if (Read) {
		read (without stop);
	} else  {
		start;
		addr;
		write;
	}
}
stop;

Thanks
Ajay

--
nvpublic
--
> 
> Cheers,
> Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 19:43, Ajay Gupta wrote:
> Hi Peter,
> 
>>>>>>> +
>>>>>>> +		if (msgs[i].flags & I2C_M_RD) {
>>>>>>> +			/* gpu_i2c_read has implicit start and stop */
>>>>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>>>>>> +			if (status < 0)
>>>>>>> +				return status;
>>>>>>> +		} else {
>>>>>>> +			/* start on first write message */
>>>>>>> +			if (i == 0) {
>>>>>>
>>>>>> This "if (i == 0)" test is completely bogus to me. I fail to see
>>>>>> why the meat of the block should not happen for both writes in a
>>>>>> double-write transfer.
>>>>>>
>>>>>> If the second message is a write, you do not issue any start nor do
>>>>>> you write out the address for the second message. You want to
>>>>>> generate the following for a transfer consisting of 2 write-messages:
>>>>>>
>>>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>>>>                              =============
>>>>>>
>>>>>> (where "..." denotes further optional "Data [A]" instances)
>>>>>>
>>>>>> As is, the stuff underlined by equal signs are not generated, at
>>>>>> least as I read the code.
>>>>>>
>>>>>> This is what I meant in my comment around this area for the v9 patch.
>>>>>
>>>>> Oh, I just realized, this probably means that the ccg_write function
>>>>> in patch
>>>>> 2/2 asks for the wrong thing. If this code actually works, the
>>>>> client driver should probably ask for a single-message transfer
>>>>> consisting of the 2-byte rab concatenated with the data buffer.
>>>>> And that actually makes sense, there is no reason to split the two
>>>>> (dependent) parts of the write into separate messages.
>>>> That would require to create new buffer and copy data for each write
>>>> request from UCSI core driver for sending UCSI command. This doesn't
>>>> look proper way of doing it.
>>>
>>> Well, that's the way master_xfer works, so you will just have to live
>>> with it if you do not want a stop in the middle.
>>
>> Bzzt, s/stop/restart/
> Obviously, a stop in middle will not work. For any read or write request,

Yes, with "s/stop/restart/", I meant "replace stop with restart", and not
"stop followed by restart". That's pretty standard notation, but sorry if
that was not clear.

> 2 byte rab write has to go first. We anyway have to separate rab part
> in read transactions so write should be okay to have separate rab write
> message.

That is not at all certain. The two below transfers need not mean the same
thing at all.

S Addr Wr [A] rab1 [A] rab2 [A] Data [A] ... P
S Addr Wr [A] rab1 [A] rab2 [A] S Addr Wr [A] Data [A] ... P

In fact, I strongly suspect that for the latter case, the device will
interpret the first two Data bytes as rab and that you therefore have to
make sure that the first transfer (w/o restart) is what you generate in
ccg_write.

> All the client driver of this needs to create messages in pairs where first
> one will always be a rab write and second one either read or write.
> I can add a check for this condition in master_xfer if needed.
> 
> Do you still see any issue with this?

Yes, this is very much an issue. master_xfer has to start each message with
a start for the first message, and a restart for subsequent messages. And
after the final message there should be a stop. All starts/restarts should
be followed by the address and the read/write bit. Then data in the given
direction. The master_xfer function should never do any device specific
checks for device specific conditions. It simply should not care about
which client driver is currently using it, and instead just stick to
implementing the master_xfer api so that all client drivers know what to
expect. No cludges.

Cheers,
Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter,

> >>>>> +
> >>>>> +		if (msgs[i].flags & I2C_M_RD) {
> >>>>> +			/* gpu_i2c_read has implicit start and stop */
> >>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> >>>>> +			if (status < 0)
> >>>>> +				return status;
> >>>>> +		} else {
> >>>>> +			/* start on first write message */
> >>>>> +			if (i == 0) {
> >>>>
> >>>> This "if (i == 0)" test is completely bogus to me. I fail to see
> >>>> why the meat of the block should not happen for both writes in a
> >>>> double-write transfer.
> >>>>
> >>>> If the second message is a write, you do not issue any start nor do
> >>>> you write out the address for the second message. You want to
> >>>> generate the following for a transfer consisting of 2 write-messages:
> >>>>
> >>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
> >>>>                              =============
> >>>>
> >>>> (where "..." denotes further optional "Data [A]" instances)
> >>>>
> >>>> As is, the stuff underlined by equal signs are not generated, at
> >>>> least as I read the code.
> >>>>
> >>>> This is what I meant in my comment around this area for the v9 patch.
> >>>
> >>> Oh, I just realized, this probably means that the ccg_write function
> >>> in patch
> >>> 2/2 asks for the wrong thing. If this code actually works, the
> >>> client driver should probably ask for a single-message transfer
> >>> consisting of the 2-byte rab concatenated with the data buffer.
> >>> And that actually makes sense, there is no reason to split the two
> >>> (dependent) parts of the write into separate messages.
> >> That would require to create new buffer and copy data for each write
> >> request from UCSI core driver for sending UCSI command. This doesn't
> >> look proper way of doing it.
> >
> > Well, that's the way master_xfer works, so you will just have to live
> > with it if you do not want a stop in the middle.
> 
> Bzzt, s/stop/restart/
Obviously, a stop in middle will not work. For any read or write request,
2 byte rab write has to go first. We anyway have to separate rab part
in read transactions so write should be okay to have separate rab write
message.

All the client driver of this needs to create messages in pairs where first
one will always be a rab write and second one either read or write.

I can add a check for this condition in master_xfer if needed.

Do you still see any issue with this?

Thanks
Ajay

--
nvpublic
--
 
> Cheers,
> Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 19:28, Peter Rosin wrote:
> On 2018-09-10 18:08, Ajay Gupta wrote:
>> Hi Peter,
>>
>>>>> +
>>>>> +		if (msgs[i].flags & I2C_M_RD) {
>>>>> +			/* gpu_i2c_read has implicit start and stop */
>>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>>>> +			if (status < 0)
>>>>> +				return status;
>>>>> +		} else {
>>>>> +			/* start on first write message */
>>>>> +			if (i == 0) {
>>>>
>>>> This "if (i == 0)" test is completely bogus to me. I fail to see why
>>>> the meat of the block should not happen for both writes in a
>>>> double-write transfer.
>>>>
>>>> If the second message is a write, you do not issue any start nor do
>>>> you write out the address for the second message. You want to generate
>>>> the following for a transfer consisting of 2 write-messages:
>>>>
>>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>>                              =============
>>>>
>>>> (where "..." denotes further optional "Data [A]" instances)
>>>>
>>>> As is, the stuff underlined by equal signs are not generated, at least
>>>> as I read the code.
>>>>
>>>> This is what I meant in my comment around this area for the v9 patch.
>>>
>>> Oh, I just realized, this probably means that the ccg_write function in patch
>>> 2/2 asks for the wrong thing. If this code actually works, the client driver
>>> should probably ask for a single-message transfer consisting of the 2-byte rab
>>> concatenated with the data buffer. 
>>> And that actually makes sense, there is no
>>> reason to split the two (dependent) parts of the write into separate messages.
>> That would require to create new buffer and copy data for each write request
>> from UCSI core driver for sending UCSI command. This doesn't look proper way
>> of doing it.
> 
> Well, that's the way master_xfer works, so you will just have to live with it
> if you do not want a stop in the middle.

Bzzt, s/stop/restart/

Cheers,
Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-10 18:08, Ajay Gupta wrote:
> Hi Peter,
> 
>>>> +
>>>> +		if (msgs[i].flags & I2C_M_RD) {
>>>> +			/* gpu_i2c_read has implicit start and stop */
>>>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>>>> +			if (status < 0)
>>>> +				return status;
>>>> +		} else {
>>>> +			/* start on first write message */
>>>> +			if (i == 0) {
>>>
>>> This "if (i == 0)" test is completely bogus to me. I fail to see why
>>> the meat of the block should not happen for both writes in a
>>> double-write transfer.
>>>
>>> If the second message is a write, you do not issue any start nor do
>>> you write out the address for the second message. You want to generate
>>> the following for a transfer consisting of 2 write-messages:
>>>
>>>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>>>                              =============
>>>
>>> (where "..." denotes further optional "Data [A]" instances)
>>>
>>> As is, the stuff underlined by equal signs are not generated, at least
>>> as I read the code.
>>>
>>> This is what I meant in my comment around this area for the v9 patch.
>>
>> Oh, I just realized, this probably means that the ccg_write function in patch
>> 2/2 asks for the wrong thing. If this code actually works, the client driver
>> should probably ask for a single-message transfer consisting of the 2-byte rab
>> concatenated with the data buffer. 
>> And that actually makes sense, there is no
>> reason to split the two (dependent) parts of the write into separate messages.
> That would require to create new buffer and copy data for each write request
> from UCSI core driver for sending UCSI command. This doesn't look proper way
> of doing it.

Well, that's the way master_xfer works, so you will just have to live with it
if you do not want a stop in the middle.

Cheers,
Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Hi Peter,

> >> +
> >> +		if (msgs[i].flags & I2C_M_RD) {
> >> +			/* gpu_i2c_read has implicit start and stop */
> >> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> >> +			if (status < 0)
> >> +				return status;
> >> +		} else {
> >> +			/* start on first write message */
> >> +			if (i == 0) {
> >
> > This "if (i == 0)" test is completely bogus to me. I fail to see why
> > the meat of the block should not happen for both writes in a
> > double-write transfer.
> >
> > If the second message is a write, you do not issue any start nor do
> > you write out the address for the second message. You want to generate
> > the following for a transfer consisting of 2 write-messages:
> >
> >   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
> >                              =============
> >
> > (where "..." denotes further optional "Data [A]" instances)
> >
> > As is, the stuff underlined by equal signs are not generated, at least
> > as I read the code.
> >
> > This is what I meant in my comment around this area for the v9 patch.
> 
> Oh, I just realized, this probably means that the ccg_write function in patch
> 2/2 asks for the wrong thing. If this code actually works, the client driver
> should probably ask for a single-message transfer consisting of the 2-byte rab
> concatenated with the data buffer. 
> And that actually makes sense, there is no
> reason to split the two (dependent) parts of the write into separate messages.
That would require to create new buffer and copy data for each write request
from UCSI core driver for sending UCSI command. This doesn't look proper way
of doing it.

Thanks
Ajay


--
nvpublic
--
> 
> Otherwise, fixing the bug here will break your client driver, but two bugs that
> just happen to cancel each other out are not what we want...
> 
> All assuming my hunch is correct, of course.
> 
> Cheers,
> Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

On 2018-09-08 21:17, Peter Rosin wrote:
>> +
>> +		if (msgs[i].flags & I2C_M_RD) {
>> +			/* gpu_i2c_read has implicit start and stop */
>> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
>> +			if (status < 0)
>> +				return status;
>> +		} else {
>> +			/* start on first write message */
>> +			if (i == 0) {
> 
> This "if (i == 0)" test is completely bogus to me. I fail to see why the
> meat of the block should not happen for both writes in a double-write
> transfer.
> 
> If the second message is a write, you do not issue any start nor do you
> write out the address for the second message. You want to generate the
> following for a transfer consisting of 2 write-messages:
> 
>   S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
>                              =============
> 
> (where "..." denotes further optional "Data [A]" instances)
> 
> As is, the stuff underlined by equal signs are not generated, at least as
> I read the code.
> 
> This is what I meant in my comment around this area for the v9 patch.

Oh, I just realized, this probably means that the ccg_write function in
patch 2/2 asks for the wrong thing. If this code actually works, the client
driver should probably ask for a single-message transfer consisting of the
2-byte rab concatenated with the data buffer. And that actually makes sense,
there is no reason to split the two (dependent) parts of the write into
separate messages.

Otherwise, fixing the bug here will break your client driver, but two bugs
that just happen to cancel each other out are not what we want...

All assuming my hunch is correct, of course.

Cheers,
Peter

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Peter Rosin]

Andy, there's a question for you below.

On 2018-09-08 02:09, Ajay Gupta wrote:
> Latest NVIDIA GPU card has USB Type-C interface. There is a
> Type-C controller which can be accessed over I2C.
> 
> This driver adds I2C bus driver to communicate with Type-C controller.
> I2C client driver will be part of USB Type-C UCSI driver.
> 
> Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changes from v1 -> v2
> 	None
> Changes from v2 -> v3
> 	Fixed review comments from Andy and Thierry
> 	Rename i2c-gpu.c -> i2c-nvidia-gpu.c
> Changes from v3 -> v4
> 	Fixed review comments from Andy
> Changes from v4 -> v5
> 	Fixed review comments from Andy
> Changes from v5 -> v6
> 	None 
> Changes from v6 -> v7 -> v8
> 	Fixed review comments from Peter 
> 	- Add implicit STOP for last write message
> 	- Add i2c_adapter_quirks with max_read_len and
> 	  I2C_AQ_COMB flags
> Changes from v8 -> v9
> 	Fixed review comments from Peter
> 	- Drop do_start flag
> 	- Use i2c_8bit_addr_from_msg()
> Changes from v9 -> v10
> 	Fixed review comments from Peter
> 	- Dropped I2C_FUNC_SMBUS_EMUL
> 	- Dropped local mutex
> 	
>  Documentation/i2c/busses/i2c-nvidia-gpu |  18 ++
>  MAINTAINERS                             |   7 +
>  drivers/i2c/busses/Kconfig              |   9 +
>  drivers/i2c/busses/Makefile             |   1 +
>  drivers/i2c/busses/i2c-nvidia-gpu.c     | 372 ++++++++++++++++++++++++++++++++
>  5 files changed, 407 insertions(+)
>  create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
>  create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c
> 
> diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu
> new file mode 100644
> index 0000000..31884d2
> --- /dev/null
> +++ b/Documentation/i2c/busses/i2c-nvidia-gpu
> @@ -0,0 +1,18 @@
> +Kernel driver i2c-nvidia-gpu
> +
> +Datasheet: not publicly available.
> +
> +Authors:
> +	Ajay Gupta <ajayg@nvidia.com>
> +
> +Description
> +-----------
> +
> +i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing
> +and later GPUs and it is used to communicate with Type-C controller on GPUs.
> +
> +If your 'lspci -v' listing shows something like the following,
> +
> +01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1)
> +
> +then this driver should support the I2C controller of your GPU.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ad052a..2d1c5a1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6797,6 +6797,13 @@ L:	linux-acpi@vger.kernel.org
>  S:	Maintained
>  F:	drivers/i2c/i2c-core-acpi.c
>  
> +I2C CONTROLLER DRIVER FOR NVIDIA GPU
> +M:	Ajay Gupta <ajayg@nvidia.com>
> +L:	linux-i2c@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/i2c/busses/i2c-nvidia-gpu
> +F:	drivers/i2c/busses/i2c-nvidia-gpu.c
> +
>  I2C MUXES
>  M:	Peter Rosin <peda@axentia.se>
>  L:	linux-i2c@vger.kernel.org
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 451d4ae..eed827b 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-nforce2-s4985.
>  
> +config I2C_NVIDIA_GPU
> +	tristate "NVIDIA GPU I2C controller"
> +	depends on PCI
> +	help
> +	  If you say yes to this option, support will be included for the
> +	  NVIDIA GPU I2C controller which is used to communicate with the GPU's
> +	  Type-C controller. This driver can also be built as a module called
> +	  i2c-nvidia-gpu.
> +
>  config I2C_SIS5595
>  	tristate "SiS 5595"
>  	depends on PCI
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 18b26af..d499813 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
>  obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
>  obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
>  obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
> +obj-$(CONFIG_I2C_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
>  
>  ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
> diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
> new file mode 100644
> index 0000000..c231121
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
> @@ -0,0 +1,372 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nvidia GPU I2C controller Driver
> + *
> + * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
> + * Author: Ajay Gupta <ajayg@nvidia.com>
> + */
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm.h>
> +#include <linux/pm_runtime.h>
> +
> +#include <asm/unaligned.h>
> +
> +/* I2C definitions */
> +#define I2C_MST_CNTL				0x00
> +#define I2C_MST_CNTL_GEN_START			BIT(0)
> +#define I2C_MST_CNTL_GEN_STOP			BIT(1)
> +#define I2C_MST_CNTL_CMD_NONE			(0 << 2)
> +#define I2C_MST_CNTL_CMD_READ			(1 << 2)
> +#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
> +#define I2C_MST_CNTL_GEN_RAB			BIT(4)
> +#define I2C_MST_CNTL_BURST_SIZE_SHIFT		6
> +#define I2C_MST_CNTL_GEN_NACK			BIT(28)
> +#define I2C_MST_CNTL_STATUS			GENMASK(30, 29)
> +#define I2C_MST_CNTL_STATUS_OKAY		(0 << 29)
> +#define I2C_MST_CNTL_STATUS_NO_ACK		(1 << 29)
> +#define I2C_MST_CNTL_STATUS_TIMEOUT		(2 << 29)
> +#define I2C_MST_CNTL_STATUS_BUS_BUSY		(3 << 29)
> +#define I2C_MST_CNTL_CYCLE_TRIGGER		BIT(31)
> +
> +#define I2C_MST_ADDR				0x04
> +
> +#define I2C_MST_I2C0_TIMING				0x08
> +#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		0x10e
> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT		16
> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX		255
> +#define I2C_MST_I2C0_TIMING_TIMEOUT_CHECK		BIT(24)
> +
> +#define I2C_MST_DATA					0x0c
> +
> +#define I2C_MST_HYBRID_PADCTL				0x20
> +#define I2C_MST_HYBRID_PADCTL_MODE_I2C			BIT(0)
> +#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV		BIT(14)
> +#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV		BIT(15)
> +
> +struct gpu_i2c_dev {
> +	struct device *dev;
> +	void __iomem *regs;
> +	struct i2c_adapter adapter;
> +	struct i2c_client *client;
> +};
> +
> +static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	/* enable I2C */
> +	val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
> +	val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
> +		I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
> +		I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
> +	writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
> +
> +	/* enable 100KHZ mode */
> +	val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
> +	val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
> +	    << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
> +	val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
> +	writel(val, i2cd->regs + I2C_MST_I2C0_TIMING);
> +}
> +
> +static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
> +{
> +	unsigned long target = jiffies + msecs_to_jiffies(1000);
> +	u32 val;
> +
> +	do {
> +		val = readl(i2cd->regs + I2C_MST_CNTL);
> +		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
> +			break;
> +		if ((val & I2C_MST_CNTL_STATUS) !=
> +				I2C_MST_CNTL_STATUS_BUS_BUSY)
> +			break;
> +		usleep_range(1000, 2000);
> +	} while (time_is_after_jiffies(target));

Andy, if you're still following this thread I'm curious, why did you claim
that the empty line here was redundant[1]? I'm personally with Ajay on this
one and think it looks cramped as is. Or do Ajay and I misinterpret you?

[1] https://marc.info/?l=linux-i2c&m=153569761011089

> +	if (time_is_before_jiffies(target)) {
> +		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
> +		return -EIO;
> +	}
> +
> +	val = readl(i2cd->regs + I2C_MST_CNTL);
> +	switch (val & I2C_MST_CNTL_STATUS) {
> +	case I2C_MST_CNTL_STATUS_OKAY:
> +		return 0;
> +	case I2C_MST_CNTL_STATUS_NO_ACK:
> +		return -EIO;
> +	case I2C_MST_CNTL_STATUS_TIMEOUT:
> +		return -ETIME;
> +	case I2C_MST_CNTL_STATUS_BUS_BUSY:
> +		return -EBUSY;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
> +{
> +	int status;
> +	u32 val;
> +
> +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> +		I2C_MST_CNTL_CMD_READ | (len << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> +		I2C_MST_CNTL_CYCLE_TRIGGER | I2C_MST_CNTL_GEN_NACK;
> +	val &= ~I2C_MST_CNTL_GEN_RAB;
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	status = gpu_i2c_check_status(i2cd);
> +	if (status < 0)
> +		return status;
> +
> +	val = readl(i2cd->regs + I2C_MST_DATA);
> +	switch (len) {
> +	case 1:
> +		data[0] = val;
> +		break;
> +	case 2:
> +		put_unaligned_be16(val, data);
> +		break;
> +	case 3:
> +		put_unaligned_be16(val >> 8, data);
> +		data[2] = val;
> +		break;
> +	case 4:
> +		put_unaligned_be32(val, data);
> +		break;
> +	default:
> +		break;
> +	}
> +	return status;
> +}
> +
> +static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
> +		I2C_MST_CNTL_GEN_NACK;
> +	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
> +{
> +	u32 val;
> +
> +	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
> +		I2C_MST_CNTL_GEN_NACK;
> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
> +{
> +	u32 val;
> +
> +	writel(data, i2cd->regs + I2C_MST_DATA);
> +
> +	val = I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
> +		I2C_MST_CNTL_GEN_NACK;
> +	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
> +		 I2C_MST_CNTL_GEN_RAB);
> +	writel(val, i2cd->regs + I2C_MST_CNTL);
> +
> +	return gpu_i2c_check_status(i2cd);
> +}
> +
> +static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
> +			       struct i2c_msg *msgs, int num)
> +{
> +	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
> +	int status;
> +	int i, j;
> +
> +	for (i = 0; i < num; i++) {
> +		/* Write the client address before processing any message */
> +		writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);

Since you have I2C_AQ_COMB_SAME_ADDR, you know that the .addr of all messages
are the same, so you should be able to hoist this to before the loop and
use msgs[0].addr, which is safe since the I2C core never calls .master_xfer
with num <= 0.

I think this is so, since elsewhere you have claimed that I2C_MST_ADDR has
to be written once before transfers begins. And that's before the loop.
Inside the loop is once for every message, not for every transfer.

> +
> +		if (msgs[i].flags & I2C_M_RD) {
> +			/* gpu_i2c_read has implicit start and stop */
> +			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
> +			if (status < 0)
> +				return status;
> +		} else {
> +			/* start on first write message */
> +			if (i == 0) {

This "if (i == 0)" test is completely bogus to me. I fail to see why the
meat of the block should not happen for both writes in a double-write
transfer.

If the second message is a write, you do not issue any start nor do you
write out the address for the second message. You want to generate the
following for a transfer consisting of 2 write-messages:

  S Addr Wr [A] Data [A] ... S Addr Wr [A] Data [A] ... P
                             =============

(where "..." denotes further optional "Data [A]" instances)

As is, the stuff underlined by equal signs are not generated, at least as
I read the code.

This is what I meant in my comment around this area for the v9 patch.

> +				u8 addr = i2c_8bit_addr_from_msg(msgs + i);

Missing blank line here between declarations and code.

> +				status = gpu_i2c_start(i2cd);
> +				if (status < 0)
> +					return status;
> +
> +				status = gpu_i2c_write(i2cd, addr);
> +				if (status < 0)
> +					return gpu_i2c_stop(i2cd);

You should issue the stop and then return the original error (i.e. status)
instead of risking a return of zero (or some unrelated error).

> +			}
> +			for (j = 0; j < msgs[i].len; j++) {
> +				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
> +				if (status < 0)
> +					return gpu_i2c_stop(i2cd);

Dito.

> +			}
> +			/* stop if last write message */
> +			if (i == (num - 1)) {
> +				status = gpu_i2c_stop(i2cd);
> +				if (status < 0)
> +					return status;
> +			}
> +		}
> +	}
> +	return i;
> +}
> +
> +static const struct i2c_adapter_quirks gpu_i2c_quirks = {
> +	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR,
> +	.max_read_len = 4,
> +};
> +
> +static u32 gpu_i2c_functionality(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C;

I think you should add back the I2C_FUNC_SMBUS_EMUL flags (possibly
with I2C_FUNC_SMBUS_QUICK excluded if the hardware does not support
zero-length reads). But all other SMBUS emulations should work just
fine, methinks.

> +}
> +
> +static const struct i2c_algorithm gpu_i2c_algorithm = {
> +	.master_xfer	= gpu_i2c_master_xfer,
> +	.functionality	= gpu_i2c_functionality,
> +};
> +
> +/*
> + * This driver is for Nvidia GPU cards with USB Type-C interface.
> + * We want to identify the cards using vendor ID and class code only
> + * to avoid dependency of adding product id for any new card which
> + * requires this driver.
> + * Currently there is no class code defined for UCSI device over PCI
> + * so using UNKNOWN class for now and it will be updated when UCSI
> + * over PCI gets a class code.
> + * There is no other NVIDIA cards with UNKNOWN class code. Even if the
> + * driver gets loaded for an undesired card then eventually i2c_read()
> + * (initiated from UCSI i2c_client) will timeout or UCSI commands will
> + * timeout.
> + */
> +#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
> +static const struct pci_device_id gpu_i2c_ids[] = {
> +	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> +		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
> +
> +static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
> +{
> +	static struct i2c_board_info gpu_ccgx_ucsi = {
> +		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
> +	};
> +
> +	gpu_ccgx_ucsi.irq = irq;
> +	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
> +	if (!i2cd->client)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +	struct gpu_i2c_dev *i2cd;
> +	int status;
> +
> +	i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL);
> +	if (!i2cd)
> +		return -ENOMEM;
> +
> +	i2cd->dev = &pdev->dev;
> +	dev_set_drvdata(&pdev->dev, i2cd);
> +
> +	status = pcim_enable_device(pdev);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
> +		return status;
> +	}
> +
> +	pci_set_master(pdev);
> +
> +	i2cd->regs = pcim_iomap(pdev, 0, 0);
> +	if (!i2cd->regs) {
> +		dev_err(&pdev->dev, "pcim_iomap failed\n");
> +		return -ENOMEM;
> +	}
> +
> +	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
> +		return status;
> +	}
> +
> +	gpu_enable_i2c_bus(i2cd);
> +
> +	i2c_set_adapdata(&i2cd->adapter, i2cd);
> +	i2cd->adapter.owner = THIS_MODULE;
> +	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
> +		sizeof(i2cd->adapter.name));
> +	i2cd->adapter.algo = &gpu_i2c_algorithm;
> +	i2cd->adapter.quirks = &gpu_i2c_quirks;
> +	i2cd->adapter.dev.parent = &pdev->dev;
> +	status = i2c_add_adapter(&i2cd->adapter);
> +	if (status < 0)
> +		goto free_irq_vectors;
> +
> +	status = gpu_populate_client(i2cd, pdev->irq);
> +	if (status < 0) {
> +		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
> +		goto del_adapter;
> +	}
> +
> +	return 0;
> +
> +del_adapter:
> +	i2c_del_adapter(&i2cd->adapter);
> +free_irq_vectors:
> +	pci_free_irq_vectors(pdev);
> +	return status;
> +}
> +
> +static void gpu_i2c_remove(struct pci_dev *pdev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
> +
> +	i2c_del_adapter(&i2cd->adapter);
> +	pci_free_irq_vectors(pdev);
> +}
> +
> +static int gpu_i2c_resume(struct device *dev)
> +{
> +	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
> +
> +	gpu_enable_i2c_bus(i2cd);
> +	return 0;
> +}
> +
> +UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
> +
> +static struct pci_driver gpu_i2c_driver = {
> +	.name		= "nvidia-gpu",
> +	.id_table	= gpu_i2c_ids,
> +	.probe		= gpu_i2c_probe,
> +	.remove		= gpu_i2c_remove,
> +	.driver		= {
> +		.pm	= &gpu_i2c_driver_pm,
> +	},
> +};
> +
> +module_pci_driver(gpu_i2c_driver);
> +
> +MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
> +MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
> +MODULE_LICENSE("GPL v2");
>

[v10,1/2] i2c: buses: add i2c bus driver for NVIDIA GPU

[Ajay Gupta]

Latest NVIDIA GPU card has USB Type-C interface. There is a
Type-C controller which can be accessed over I2C.

This driver adds I2C bus driver to communicate with Type-C controller.
I2C client driver will be part of USB Type-C UCSI driver.

Signed-off-by: Ajay Gupta <ajayg@nvidia.com>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
Changes from v1 -> v2
	None
Changes from v2 -> v3
	Fixed review comments from Andy and Thierry
	Rename i2c-gpu.c -> i2c-nvidia-gpu.c
Changes from v3 -> v4
	Fixed review comments from Andy
Changes from v4 -> v5
	Fixed review comments from Andy
Changes from v5 -> v6
	None 
Changes from v6 -> v7 -> v8
	Fixed review comments from Peter 
	- Add implicit STOP for last write message
	- Add i2c_adapter_quirks with max_read_len and
	  I2C_AQ_COMB flags
Changes from v8 -> v9
	Fixed review comments from Peter
	- Drop do_start flag
	- Use i2c_8bit_addr_from_msg()
Changes from v9 -> v10
	Fixed review comments from Peter
	- Dropped I2C_FUNC_SMBUS_EMUL
	- Dropped local mutex
	
 Documentation/i2c/busses/i2c-nvidia-gpu |  18 ++
 MAINTAINERS                             |   7 +
 drivers/i2c/busses/Kconfig              |   9 +
 drivers/i2c/busses/Makefile             |   1 +
 drivers/i2c/busses/i2c-nvidia-gpu.c     | 372 ++++++++++++++++++++++++++++++++
 5 files changed, 407 insertions(+)
 create mode 100644 Documentation/i2c/busses/i2c-nvidia-gpu
 create mode 100644 drivers/i2c/busses/i2c-nvidia-gpu.c

diff --git a/Documentation/i2c/busses/i2c-nvidia-gpu b/Documentation/i2c/busses/i2c-nvidia-gpu
new file mode 100644
index 0000000..31884d2
--- /dev/null
+++ b/Documentation/i2c/busses/i2c-nvidia-gpu
@@ -0,0 +1,18 @@
+Kernel driver i2c-nvidia-gpu
+
+Datasheet: not publicly available.
+
+Authors:
+	Ajay Gupta <ajayg@nvidia.com>
+
+Description
+-----------
+
+i2c-nvidia-gpu is a driver for I2C controller included in NVIDIA Turing
+and later GPUs and it is used to communicate with Type-C controller on GPUs.
+
+If your 'lspci -v' listing shows something like the following,
+
+01:00.3 Serial bus controller [0c80]: NVIDIA Corporation Device 1ad9 (rev a1)
+
+then this driver should support the I2C controller of your GPU.
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ad052a..2d1c5a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6797,6 +6797,13 @@ L:	linux-acpi@vger.kernel.org
 S:	Maintained
 F:	drivers/i2c/i2c-core-acpi.c
 
+I2C CONTROLLER DRIVER FOR NVIDIA GPU
+M:	Ajay Gupta <ajayg@nvidia.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/i2c/busses/i2c-nvidia-gpu
+F:	drivers/i2c/busses/i2c-nvidia-gpu.c
+
 I2C MUXES
 M:	Peter Rosin <peda@axentia.se>
 L:	linux-i2c@vger.kernel.org
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 451d4ae..eed827b 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -224,6 +224,15 @@ config I2C_NFORCE2_S4985
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-nforce2-s4985.
 
+config I2C_NVIDIA_GPU
+	tristate "NVIDIA GPU I2C controller"
+	depends on PCI
+	help
+	  If you say yes to this option, support will be included for the
+	  NVIDIA GPU I2C controller which is used to communicate with the GPU's
+	  Type-C controller. This driver can also be built as a module called
+	  i2c-nvidia-gpu.
+
 config I2C_SIS5595
 	tristate "SiS 5595"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 18b26af..d499813 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -140,5 +140,6 @@ obj-$(CONFIG_I2C_SIBYTE)	+= i2c-sibyte.o
 obj-$(CONFIG_I2C_XGENE_SLIMPRO) += i2c-xgene-slimpro.o
 obj-$(CONFIG_SCx200_ACB)	+= scx200_acb.o
 obj-$(CONFIG_I2C_FSI)		+= i2c-fsi.o
+obj-$(CONFIG_I2C_NVIDIA_GPU)	+= i2c-nvidia-gpu.o
 
 ccflags-$(CONFIG_I2C_DEBUG_BUS) := -DDEBUG
diff --git a/drivers/i2c/busses/i2c-nvidia-gpu.c b/drivers/i2c/busses/i2c-nvidia-gpu.c
new file mode 100644
index 0000000..c231121
--- /dev/null
+++ b/drivers/i2c/busses/i2c-nvidia-gpu.c
@@ -0,0 +1,372 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nvidia GPU I2C controller Driver
+ *
+ * Copyright (C) 2018 NVIDIA Corporation. All rights reserved.
+ * Author: Ajay Gupta <ajayg@nvidia.com>
+ */
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+
+#include <asm/unaligned.h>
+
+/* I2C definitions */
+#define I2C_MST_CNTL				0x00
+#define I2C_MST_CNTL_GEN_START			BIT(0)
+#define I2C_MST_CNTL_GEN_STOP			BIT(1)
+#define I2C_MST_CNTL_CMD_NONE			(0 << 2)
+#define I2C_MST_CNTL_CMD_READ			(1 << 2)
+#define I2C_MST_CNTL_CMD_WRITE			(2 << 2)
+#define I2C_MST_CNTL_GEN_RAB			BIT(4)
+#define I2C_MST_CNTL_BURST_SIZE_SHIFT		6
+#define I2C_MST_CNTL_GEN_NACK			BIT(28)
+#define I2C_MST_CNTL_STATUS			GENMASK(30, 29)
+#define I2C_MST_CNTL_STATUS_OKAY		(0 << 29)
+#define I2C_MST_CNTL_STATUS_NO_ACK		(1 << 29)
+#define I2C_MST_CNTL_STATUS_TIMEOUT		(2 << 29)
+#define I2C_MST_CNTL_STATUS_BUS_BUSY		(3 << 29)
+#define I2C_MST_CNTL_CYCLE_TRIGGER		BIT(31)
+
+#define I2C_MST_ADDR				0x04
+
+#define I2C_MST_I2C0_TIMING				0x08
+#define I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ		0x10e
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT		16
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX		255
+#define I2C_MST_I2C0_TIMING_TIMEOUT_CHECK		BIT(24)
+
+#define I2C_MST_DATA					0x0c
+
+#define I2C_MST_HYBRID_PADCTL				0x20
+#define I2C_MST_HYBRID_PADCTL_MODE_I2C			BIT(0)
+#define I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV		BIT(14)
+#define I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV		BIT(15)
+
+struct gpu_i2c_dev {
+	struct device *dev;
+	void __iomem *regs;
+	struct i2c_adapter adapter;
+	struct i2c_client *client;
+};
+
+static void gpu_enable_i2c_bus(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	/* enable I2C */
+	val = readl(i2cd->regs + I2C_MST_HYBRID_PADCTL);
+	val |= I2C_MST_HYBRID_PADCTL_MODE_I2C |
+		I2C_MST_HYBRID_PADCTL_I2C_SCL_INPUT_RCV |
+		I2C_MST_HYBRID_PADCTL_I2C_SDA_INPUT_RCV;
+	writel(val, i2cd->regs + I2C_MST_HYBRID_PADCTL);
+
+	/* enable 100KHZ mode */
+	val = I2C_MST_I2C0_TIMING_SCL_PERIOD_100KHZ;
+	val |= (I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT_MAX
+	    << I2C_MST_I2C0_TIMING_TIMEOUT_CLK_CNT);
+	val |= I2C_MST_I2C0_TIMING_TIMEOUT_CHECK;
+	writel(val, i2cd->regs + I2C_MST_I2C0_TIMING);
+}
+
+static int gpu_i2c_check_status(struct gpu_i2c_dev *i2cd)
+{
+	unsigned long target = jiffies + msecs_to_jiffies(1000);
+	u32 val;
+
+	do {
+		val = readl(i2cd->regs + I2C_MST_CNTL);
+		if (!(val & I2C_MST_CNTL_CYCLE_TRIGGER))
+			break;
+		if ((val & I2C_MST_CNTL_STATUS) !=
+				I2C_MST_CNTL_STATUS_BUS_BUSY)
+			break;
+		usleep_range(1000, 2000);
+	} while (time_is_after_jiffies(target));
+	if (time_is_before_jiffies(target)) {
+		dev_err(i2cd->dev, "i2c timeout error %x\n", val);
+		return -EIO;
+	}
+
+	val = readl(i2cd->regs + I2C_MST_CNTL);
+	switch (val & I2C_MST_CNTL_STATUS) {
+	case I2C_MST_CNTL_STATUS_OKAY:
+		return 0;
+	case I2C_MST_CNTL_STATUS_NO_ACK:
+		return -EIO;
+	case I2C_MST_CNTL_STATUS_TIMEOUT:
+		return -ETIME;
+	case I2C_MST_CNTL_STATUS_BUS_BUSY:
+		return -EBUSY;
+	default:
+		return 0;
+	}
+}
+
+static int gpu_i2c_read(struct gpu_i2c_dev *i2cd, u8 *data, u16 len)
+{
+	int status;
+	u32 val;
+
+	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
+		I2C_MST_CNTL_CMD_READ | (len << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
+		I2C_MST_CNTL_CYCLE_TRIGGER | I2C_MST_CNTL_GEN_NACK;
+	val &= ~I2C_MST_CNTL_GEN_RAB;
+	writel(val, i2cd->regs + I2C_MST_CNTL);
+
+	status = gpu_i2c_check_status(i2cd);
+	if (status < 0)
+		return status;
+
+	val = readl(i2cd->regs + I2C_MST_DATA);
+	switch (len) {
+	case 1:
+		data[0] = val;
+		break;
+	case 2:
+		put_unaligned_be16(val, data);
+		break;
+	case 3:
+		put_unaligned_be16(val >> 8, data);
+		data[2] = val;
+		break;
+	case 4:
+		put_unaligned_be32(val, data);
+		break;
+	default:
+		break;
+	}
+	return status;
+}
+
+static int gpu_i2c_start(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	val = I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_CMD_NONE |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_GEN_RAB);
+	writel(val, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_stop(struct gpu_i2c_dev *i2cd)
+{
+	u32 val;
+
+	val = I2C_MST_CNTL_GEN_STOP | I2C_MST_CNTL_CMD_NONE |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_RAB);
+	writel(val, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_write(struct gpu_i2c_dev *i2cd, u8 data)
+{
+	u32 val;
+
+	writel(data, i2cd->regs + I2C_MST_DATA);
+
+	val = I2C_MST_CNTL_CMD_WRITE | (1 << I2C_MST_CNTL_BURST_SIZE_SHIFT) |
+		I2C_MST_CNTL_GEN_NACK;
+	val &= ~(I2C_MST_CNTL_GEN_START | I2C_MST_CNTL_GEN_STOP |
+		 I2C_MST_CNTL_GEN_RAB);
+	writel(val, i2cd->regs + I2C_MST_CNTL);
+
+	return gpu_i2c_check_status(i2cd);
+}
+
+static int gpu_i2c_master_xfer(struct i2c_adapter *adap,
+			       struct i2c_msg *msgs, int num)
+{
+	struct gpu_i2c_dev *i2cd = i2c_get_adapdata(adap);
+	int status;
+	int i, j;
+
+	for (i = 0; i < num; i++) {
+		/* Write the client address before processing any message */
+		writel(msgs[i].addr, i2cd->regs + I2C_MST_ADDR);
+
+		if (msgs[i].flags & I2C_M_RD) {
+			/* gpu_i2c_read has implicit start and stop */
+			status = gpu_i2c_read(i2cd, msgs[i].buf, msgs[i].len);
+			if (status < 0)
+				return status;
+		} else {
+			/* start on first write message */
+			if (i == 0) {
+				u8 addr = i2c_8bit_addr_from_msg(msgs + i);
+				status = gpu_i2c_start(i2cd);
+				if (status < 0)
+					return status;
+
+				status = gpu_i2c_write(i2cd, addr);
+				if (status < 0)
+					return gpu_i2c_stop(i2cd);
+			}
+			for (j = 0; j < msgs[i].len; j++) {
+				status = gpu_i2c_write(i2cd, msgs[i].buf[j]);
+				if (status < 0)
+					return gpu_i2c_stop(i2cd);
+			}
+			/* stop if last write message */
+			if (i == (num - 1)) {
+				status = gpu_i2c_stop(i2cd);
+				if (status < 0)
+					return status;
+			}
+		}
+	}
+	return i;
+}
+
+static const struct i2c_adapter_quirks gpu_i2c_quirks = {
+	.flags = I2C_AQ_COMB | I2C_AQ_COMB_WRITE_FIRST | I2C_AQ_COMB_SAME_ADDR,
+	.max_read_len = 4,
+};
+
+static u32 gpu_i2c_functionality(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C;
+}
+
+static const struct i2c_algorithm gpu_i2c_algorithm = {
+	.master_xfer	= gpu_i2c_master_xfer,
+	.functionality	= gpu_i2c_functionality,
+};
+
+/*
+ * This driver is for Nvidia GPU cards with USB Type-C interface.
+ * We want to identify the cards using vendor ID and class code only
+ * to avoid dependency of adding product id for any new card which
+ * requires this driver.
+ * Currently there is no class code defined for UCSI device over PCI
+ * so using UNKNOWN class for now and it will be updated when UCSI
+ * over PCI gets a class code.
+ * There is no other NVIDIA cards with UNKNOWN class code. Even if the
+ * driver gets loaded for an undesired card then eventually i2c_read()
+ * (initiated from UCSI i2c_client) will timeout or UCSI commands will
+ * timeout.
+ */
+#define PCI_CLASS_SERIAL_UNKNOWN	0x0c80
+static const struct pci_device_id gpu_i2c_ids[] = {
+	{ PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
+		PCI_CLASS_SERIAL_UNKNOWN << 8, 0xffffff00},
+	{ }
+};
+MODULE_DEVICE_TABLE(pci, gpu_i2c_ids);
+
+static int gpu_populate_client(struct gpu_i2c_dev *i2cd, int irq)
+{
+	static struct i2c_board_info gpu_ccgx_ucsi = {
+		I2C_BOARD_INFO("ccgx-ucsi", 0x8),
+	};
+
+	gpu_ccgx_ucsi.irq = irq;
+	i2cd->client = i2c_new_device(&i2cd->adapter, &gpu_ccgx_ucsi);
+	if (!i2cd->client)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int gpu_i2c_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	struct gpu_i2c_dev *i2cd;
+	int status;
+
+	i2cd = devm_kzalloc(&pdev->dev, sizeof(struct gpu_i2c_dev), GFP_KERNEL);
+	if (!i2cd)
+		return -ENOMEM;
+
+	i2cd->dev = &pdev->dev;
+	dev_set_drvdata(&pdev->dev, i2cd);
+
+	status = pcim_enable_device(pdev);
+	if (status < 0) {
+		dev_err(&pdev->dev, "pcim_enable_device failed %d\n", status);
+		return status;
+	}
+
+	pci_set_master(pdev);
+
+	i2cd->regs = pcim_iomap(pdev, 0, 0);
+	if (!i2cd->regs) {
+		dev_err(&pdev->dev, "pcim_iomap failed\n");
+		return -ENOMEM;
+	}
+
+	status = pci_alloc_irq_vectors(pdev, 1, 1, PCI_IRQ_MSI);
+	if (status < 0) {
+		dev_err(&pdev->dev, "pci_alloc_irq_vectors err %d\n", status);
+		return status;
+	}
+
+	gpu_enable_i2c_bus(i2cd);
+
+	i2c_set_adapdata(&i2cd->adapter, i2cd);
+	i2cd->adapter.owner = THIS_MODULE;
+	strlcpy(i2cd->adapter.name, "NVIDIA GPU I2C adapter",
+		sizeof(i2cd->adapter.name));
+	i2cd->adapter.algo = &gpu_i2c_algorithm;
+	i2cd->adapter.quirks = &gpu_i2c_quirks;
+	i2cd->adapter.dev.parent = &pdev->dev;
+	status = i2c_add_adapter(&i2cd->adapter);
+	if (status < 0)
+		goto free_irq_vectors;
+
+	status = gpu_populate_client(i2cd, pdev->irq);
+	if (status < 0) {
+		dev_err(&pdev->dev, "gpu_populate_client failed %d\n", status);
+		goto del_adapter;
+	}
+
+	return 0;
+
+del_adapter:
+	i2c_del_adapter(&i2cd->adapter);
+free_irq_vectors:
+	pci_free_irq_vectors(pdev);
+	return status;
+}
+
+static void gpu_i2c_remove(struct pci_dev *pdev)
+{
+	struct gpu_i2c_dev *i2cd = dev_get_drvdata(&pdev->dev);
+
+	i2c_del_adapter(&i2cd->adapter);
+	pci_free_irq_vectors(pdev);
+}
+
+static int gpu_i2c_resume(struct device *dev)
+{
+	struct gpu_i2c_dev *i2cd = dev_get_drvdata(dev);
+
+	gpu_enable_i2c_bus(i2cd);
+	return 0;
+}
+
+UNIVERSAL_DEV_PM_OPS(gpu_i2c_driver_pm, NULL, gpu_i2c_resume, NULL);
+
+static struct pci_driver gpu_i2c_driver = {
+	.name		= "nvidia-gpu",
+	.id_table	= gpu_i2c_ids,
+	.probe		= gpu_i2c_probe,
+	.remove		= gpu_i2c_remove,
+	.driver		= {
+		.pm	= &gpu_i2c_driver_pm,
+	},
+};
+
+module_pci_driver(gpu_i2c_driver);
+
+MODULE_AUTHOR("Ajay Gupta <ajayg@nvidia.com>");
+MODULE_DESCRIPTION("Nvidia GPU I2C controller Driver");
+MODULE_LICENSE("GPL v2");