Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> Basically what I was asking is whether I could use i2c_generic_scl_recovery
> in the case where SCL is hung.

The name is a bit misleading, I am afraid. Recovery can only be used when
SDA is stuck low. And to fix this it *uses* SCL toggling to get out of
it. And 'generic_scl' means 'gimme some SCL to control and I will toggle
it'. Compared to 'gpio_recovery' which will do all the GPIO handling for
you.

When SCL is hung, you can only reset the device which forces SCL low.

> I think I have a pretty good idea of what to do, I should probably just put
> together an RFC patch.

Sure.



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> Basically what I was asking is whether I could use i2c_generic_scl_recovery
> in the case where SCL is hung.

The name is a bit misleading, I am afraid. Recovery can only be used when
SDA is stuck low. And to fix this it *uses* SCL toggling to get out of
it. And 'generic_scl' means 'gimme some SCL to control and I will toggle
it'. Compared to 'gpio_recovery' which will do all the GPIO handling for
you.

When SCL is hung, you can only reset the device which forces SCL low.

> I think I have a pretty good idea of what to do, I should probably just put
> together an RFC patch.

Sure.



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Sorry, went on vacation and then forgot about our conversion.

>> the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
>> to be part of the user interface, or is it just intended to help put the
>> main recovery function together?
>
> Sorry, I don't understand the question. What do you mean?
>

What I meant is that it looks like the only use of it is putting
together a default
recovery function, but I was wondering if it is fair to use it on its own.
Basically what I was asking is whether I could use i2c_generic_scl_recovery
in the case where SCL is hung.

I think I have a pretty good idea of what to do, I should probably just put
together an RFC patch.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

Sorry, went on vacation and then forgot about our conversion.

>> the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
>> to be part of the user interface, or is it just intended to help put the
>> main recovery function together?
>
> Sorry, I don't understand the question. What do you mean?
>

What I meant is that it looks like the only use of it is putting
together a default
recovery function, but I was wondering if it is fair to use it on its own.
Basically what I was asking is whether I could use i2c_generic_scl_recovery
in the case where SCL is hung.

I think I have a pretty good idea of what to do, I should probably just put
together an RFC patch.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
> to be part of the user interface, or is it just intended to help put the
> main recovery function together?

Sorry, I don't understand the question. What do you mean?



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
> to be part of the user interface, or is it just intended to help put the
> main recovery function together?

Sorry, I don't understand the question. What do you mean?



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Tue, Jun 27, 2017 at 2:00 AM, Wolfram Sang  wrote:
>
>> > If SCL is stuck low, how do you want to send a STOP?
>> >
>>
>> Fair point. I should probably drop that in the future and just do a
>> reset, and even then, doing a
>> reset is probably just wishful thinking. If a slave is holding down
>> SCL, we are pretty screwed.
>
> Yes, you'd need to reset the client device. And that has to be done in
> the client driver. I wonder if i2c_transfer and friends should return a
> specific error value in that case... need to think about it.
>

Well, a good first step would be to make my recovery code conform to
the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
to be part of the user interface, or is it just intended to help put the
main recovery function together?

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Tue, Jun 27, 2017 at 2:00 AM, Wolfram Sang  wrote:
>
>> > If SCL is stuck low, how do you want to send a STOP?
>> >
>>
>> Fair point. I should probably drop that in the future and just do a
>> reset, and even then, doing a
>> reset is probably just wishful thinking. If a slave is holding down
>> SCL, we are pretty screwed.
>
> Yes, you'd need to reset the client device. And that has to be done in
> the client driver. I wonder if i2c_transfer and friends should return a
> specific error value in that case... need to think about it.
>

Well, a good first step would be to make my recovery code conform to
the struct i2c_bus_recovery_info. Is i2c_generic_scl_recovery supposed
to be part of the user interface, or is it just intended to help put the
main recovery function together?

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> > If SCL is stuck low, how do you want to send a STOP?
> >
> 
> Fair point. I should probably drop that in the future and just do a
> reset, and even then, doing a
> reset is probably just wishful thinking. If a slave is holding down
> SCL, we are pretty screwed.

Yes, you'd need to reset the client device. And that has to be done in
the client driver. I wonder if i2c_transfer and friends should return a
specific error value in that case... need to think about it.



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

> > If SCL is stuck low, how do you want to send a STOP?
> >
> 
> Fair point. I should probably drop that in the future and just do a
> reset, and even then, doing a
> reset is probably just wishful thinking. If a slave is holding down
> SCL, we are pretty screwed.

Yes, you'd need to reset the client device. And that has to be done in
the client driver. I wonder if i2c_transfer and friends should return a
specific error value in that case... need to think about it.



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Sun, Jun 25, 2017 at 11:18 PM, Joel Stanley  wrote:
> On Sat, Jun 24, 2017 at 4:13 AM, Wolfram Sang  wrote:
>> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>>> Added initial master support for Aspeed I2C controller. Supports
>>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>>
>>> Signed-off-by: Brendan Higgins 
>>
>> Applied to for-next, thanks for all the hard work!
>
> Congratulations Brendan. It's great to see this driver in the tree.
>
> Cheers,
>
> Joel

Thanks! And thank you to everyone who helped! I am not the only person who
put in a lot of hard work.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Sun, Jun 25, 2017 at 11:18 PM, Joel Stanley  wrote:
> On Sat, Jun 24, 2017 at 4:13 AM, Wolfram Sang  wrote:
>> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>>> Added initial master support for Aspeed I2C controller. Supports
>>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>>
>>> Signed-off-by: Brendan Higgins 
>>
>> Applied to for-next, thanks for all the hard work!
>
> Congratulations Brendan. It's great to see this driver in the tree.
>
> Cheers,
>
> Joel

Thanks! And thank you to everyone who helped! I am not the only person who
put in a lot of hard work.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Fri, Jun 23, 2017 at 11:43 AM, Wolfram Sang  wrote:
> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>> Added initial master support for Aspeed I2C controller. Supports
>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>
>> Signed-off-by: Brendan Higgins 
>
> Applied to for-next, thanks for all the hard work!

Thanks for the patience!

>
> One question however which can be solved incrementally if needed:
>
>> + if (command & ASPEED_I2CD_SDA_LINE_STS) {
>> + /* Bus is idle: no recovery needed. */
>> + if (command & ASPEED_I2CD_SCL_LINE_STS)
>> + goto out;
>> + dev_dbg(bus->dev, "SCL hung (state %x), attempting recovery\n",
>> + command);
>> +
>> + reinit_completion(>cmd_complete);
>> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
>
> If SCL is stuck low, how do you want to send a STOP?
>

Fair point. I should probably drop that in the future and just do a
reset, and even then, doing a
reset is probably just wishful thinking. If a slave is holding down
SCL, we are pretty screwed.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Brendan Higgins]

On Fri, Jun 23, 2017 at 11:43 AM, Wolfram Sang  wrote:
> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>> Added initial master support for Aspeed I2C controller. Supports
>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>
>> Signed-off-by: Brendan Higgins 
>
> Applied to for-next, thanks for all the hard work!

Thanks for the patience!

>
> One question however which can be solved incrementally if needed:
>
>> + if (command & ASPEED_I2CD_SDA_LINE_STS) {
>> + /* Bus is idle: no recovery needed. */
>> + if (command & ASPEED_I2CD_SCL_LINE_STS)
>> + goto out;
>> + dev_dbg(bus->dev, "SCL hung (state %x), attempting recovery\n",
>> + command);
>> +
>> + reinit_completion(>cmd_complete);
>> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);
>
> If SCL is stuck low, how do you want to send a STOP?
>

Fair point. I should probably drop that in the future and just do a
reset, and even then, doing a
reset is probably just wishful thinking. If a slave is holding down
SCL, we are pretty screwed.

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Joel Stanley]

On Sat, Jun 24, 2017 at 4:13 AM, Wolfram Sang  wrote:
> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>> Added initial master support for Aspeed I2C controller. Supports
>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>
>> Signed-off-by: Brendan Higgins 
>
> Applied to for-next, thanks for all the hard work!

Congratulations Brendan. It's great to see this driver in the tree.

Cheers,

Joel

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Joel Stanley]

On Sat, Jun 24, 2017 at 4:13 AM, Wolfram Sang  wrote:
> On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
>> Added initial master support for Aspeed I2C controller. Supports
>> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
>>
>> Signed-off-by: Brendan Higgins 
>
> Applied to for-next, thanks for all the hard work!

Congratulations Brendan. It's great to see this driver in the tree.

Cheers,

Joel

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
> Added initial master support for Aspeed I2C controller. Supports
> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
> 
> Signed-off-by: Brendan Higgins 

Applied to for-next, thanks for all the hard work!

One question however which can be solved incrementally if needed:

> + if (command & ASPEED_I2CD_SDA_LINE_STS) {
> + /* Bus is idle: no recovery needed. */
> + if (command & ASPEED_I2CD_SCL_LINE_STS)
> + goto out;
> + dev_dbg(bus->dev, "SCL hung (state %x), attempting recovery\n",
> + command);
> +
> + reinit_completion(>cmd_complete);
> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);

If SCL is stuck low, how do you want to send a STOP?



signature.asc
Description: PGP signature

Re: [PATCH v11 3/4] i2c: aspeed: added driver for Aspeed I2C

[Wolfram Sang]

On Tue, Jun 20, 2017 at 02:15:15PM -0700, Brendan Higgins wrote:
> Added initial master support for Aspeed I2C controller. Supports
> fourteen busses present in AST24XX and AST25XX BMC SoCs by Aspeed.
> 
> Signed-off-by: Brendan Higgins 

Applied to for-next, thanks for all the hard work!

One question however which can be solved incrementally if needed:

> + if (command & ASPEED_I2CD_SDA_LINE_STS) {
> + /* Bus is idle: no recovery needed. */
> + if (command & ASPEED_I2CD_SCL_LINE_STS)
> + goto out;
> + dev_dbg(bus->dev, "SCL hung (state %x), attempting recovery\n",
> + command);
> +
> + reinit_completion(>cmd_complete);
> + writel(ASPEED_I2CD_M_STOP_CMD, bus->base + ASPEED_I2C_CMD_REG);

If SCL is stuck low, how do you want to send a STOP?



signature.asc
Description: PGP signature