| Message ID | 20240202204847.2062798-2-komlodi@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/i2c: smbus: Reset fixes | expand |
On Fri, 2 Feb 2024 at 20:48, Joe Komlodi <komlodi@google.com> wrote: > > It's possible for a reset to come in the middle of a transaction, which > causes the bus to be in an old state when a new transaction comes in. > > Signed-off-by: Joe Komlodi <komlodi@google.com> > --- > hw/i2c/core.c | 19 +++++++++++++++++++ > include/hw/i2c/i2c.h | 2 +- > 2 files changed, 20 insertions(+), 1 deletion(-) > > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > index 4cf30b2c86..3128067bba 100644 > --- a/hw/i2c/core.c > +++ b/hw/i2c/core.c > @@ -23,10 +23,29 @@ static Property i2c_props[] = { > DEFINE_PROP_END_OF_LIST(), > }; > > +static void i2c_bus_hold_reset(Object *obj) > +{ > + I2CBus *bus = I2C_BUS(obj); > + I2CNode *node, *next; > + > + bus->broadcast = false; > + QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { > + QLIST_REMOVE(node, next); > + g_free(node); > + } > +} This does what it says it's going to do; but I think it would be good to hear from Corey whether it's better to do this, or instead to call i2c_end_transfer() in the reset-enter phase. Mostly QEMU's "reset" is like power-cycling, in which case I guess that what we have here where we just forget about the in-progress transfer and assume the device on the other end is also going to reset back to a neutral state is what we want. Does i2c have a concept of a bus-level "reset" operation? > + > +static void i2c_bus_class_init(ObjectClass *klass, void *data) > +{ > + ResettableClass *rc = RESETTABLE_CLASS(klass); > + rc->phases.hold = i2c_bus_hold_reset; > +} > + > static const TypeInfo i2c_bus_info = { > .name = TYPE_I2C_BUS, > .parent = TYPE_BUS, > .instance_size = sizeof(I2CBus), > + .class_init = i2c_bus_class_init, > }; > static int i2c_bus_pre_save(void *opaque) > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > index 2a3abacd1b..49580e30e2 100644 > --- a/include/hw/i2c/i2c.h > +++ b/include/hw/i2c/i2c.h > @@ -64,7 +64,7 @@ struct I2CSlave { > }; > > #define TYPE_I2C_BUS "i2c-bus" > -OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS) > +OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS) I don't think you need this change any more ? thanks -- PMM
On Thu, Feb 8, 2024 at 8:39 AM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Fri, 2 Feb 2024 at 20:48, Joe Komlodi <komlodi@google.com> wrote: > > > > It's possible for a reset to come in the middle of a transaction, which > > causes the bus to be in an old state when a new transaction comes in. > > > > Signed-off-by: Joe Komlodi <komlodi@google.com> > > --- > > hw/i2c/core.c | 19 +++++++++++++++++++ > > include/hw/i2c/i2c.h | 2 +- > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > > index 4cf30b2c86..3128067bba 100644 > > --- a/hw/i2c/core.c > > +++ b/hw/i2c/core.c > > @@ -23,10 +23,29 @@ static Property i2c_props[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +static void i2c_bus_hold_reset(Object *obj) > > +{ > > + I2CBus *bus = I2C_BUS(obj); > > + I2CNode *node, *next; > > + > > + bus->broadcast = false; > > + QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { > > + QLIST_REMOVE(node, next); > > + g_free(node); > > + } > > +} > > This does what it says it's going to do; but I think it > would be good to hear from Corey whether it's better to > do this, or instead to call i2c_end_transfer() in the > reset-enter phase. i2c_end_transfer() might actually make more sense (explained a little more below). I'll see what Corey says though. > > Mostly QEMU's "reset" is like power-cycling, in which case > I guess that what we have here where we just forget about > the in-progress transfer and assume the device on the other > end is also going to reset back to a neutral state is what > we want. > > Does i2c have a concept of a bus-level "reset" operation? > Not really, as far as I know. On hardware I believe if a reset happened in the middle of a transaction it would just look like a transaction ending from the target's PoV. > > + > > +static void i2c_bus_class_init(ObjectClass *klass, void *data) > > +{ > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > + rc->phases.hold = i2c_bus_hold_reset; > > +} > > + > > static const TypeInfo i2c_bus_info = { > > .name = TYPE_I2C_BUS, > > .parent = TYPE_BUS, > > .instance_size = sizeof(I2CBus), > > + .class_init = i2c_bus_class_init, > > }; > > > > > static int i2c_bus_pre_save(void *opaque) > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > > index 2a3abacd1b..49580e30e2 100644 > > --- a/include/hw/i2c/i2c.h > > +++ b/include/hw/i2c/i2c.h > > @@ -64,7 +64,7 @@ struct I2CSlave { > > }; > > > > #define TYPE_I2C_BUS "i2c-bus" > > -OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS) > > +OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS) > > I don't think you need this change any more ? Oops, will fix in v4. I'll hold off on sending it until Corey gives input on the reset behavior. Thanks, Joe > > thanks > -- PMM
On Thu, Feb 08, 2024 at 04:39:10PM +0000, Peter Maydell wrote: > On Fri, 2 Feb 2024 at 20:48, Joe Komlodi <komlodi@google.com> wrote: > > > > It's possible for a reset to come in the middle of a transaction, which > > causes the bus to be in an old state when a new transaction comes in. > > > > Signed-off-by: Joe Komlodi <komlodi@google.com> > > --- > > hw/i2c/core.c | 19 +++++++++++++++++++ > > include/hw/i2c/i2c.h | 2 +- > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > > index 4cf30b2c86..3128067bba 100644 > > --- a/hw/i2c/core.c > > +++ b/hw/i2c/core.c > > @@ -23,10 +23,29 @@ static Property i2c_props[] = { > > DEFINE_PROP_END_OF_LIST(), > > }; > > > > +static void i2c_bus_hold_reset(Object *obj) > > +{ > > + I2CBus *bus = I2C_BUS(obj); > > + I2CNode *node, *next; > > + > > + bus->broadcast = false; > > + QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { > > + QLIST_REMOVE(node, next); > > + g_free(node); > > + } > > +} > > This does what it says it's going to do; but I think it > would be good to hear from Corey whether it's better to > do this, or instead to call i2c_end_transfer() in the > reset-enter phase. Sorry, I missed this, I'm having major chaos going on right now in my life. I don't think i2c_end_transfer() is the right thing to do. The transfer has not cleanly ended, it is just forgotten. > > Mostly QEMU's "reset" is like power-cycling, in which case > I guess that what we have here where we just forget about > the in-progress transfer and assume the device on the other > end is also going to reset back to a neutral state is what > we want. > > Does i2c have a concept of a bus-level "reset" operation? No, it does not. Most I2C devices don't even have a reset pin. In a reset situation in real hardware, the operation would be aborted by the lines drifting high after the bus master has been reset. So I think this is fine as is. -corey > > > + > > +static void i2c_bus_class_init(ObjectClass *klass, void *data) > > +{ > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > + rc->phases.hold = i2c_bus_hold_reset; > > +} > > + > > static const TypeInfo i2c_bus_info = { > > .name = TYPE_I2C_BUS, > > .parent = TYPE_BUS, > > .instance_size = sizeof(I2CBus), > > + .class_init = i2c_bus_class_init, > > }; > > > > > static int i2c_bus_pre_save(void *opaque) > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > > index 2a3abacd1b..49580e30e2 100644 > > --- a/include/hw/i2c/i2c.h > > +++ b/include/hw/i2c/i2c.h > > @@ -64,7 +64,7 @@ struct I2CSlave { > > }; > > > > #define TYPE_I2C_BUS "i2c-bus" > > -OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS) > > +OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS) > > I don't think you need this change any more ? > > thanks > -- PMM >
On Fri, Feb 16, 2024 at 5:04 PM Corey Minyard <minyard@acm.org> wrote: > > On Thu, Feb 08, 2024 at 04:39:10PM +0000, Peter Maydell wrote: > > On Fri, 2 Feb 2024 at 20:48, Joe Komlodi <komlodi@google.com> wrote: > > > > > > It's possible for a reset to come in the middle of a transaction, which > > > causes the bus to be in an old state when a new transaction comes in. > > > > > > Signed-off-by: Joe Komlodi <komlodi@google.com> > > > --- > > > hw/i2c/core.c | 19 +++++++++++++++++++ > > > include/hw/i2c/i2c.h | 2 +- > > > 2 files changed, 20 insertions(+), 1 deletion(-) > > > > > > diff --git a/hw/i2c/core.c b/hw/i2c/core.c > > > index 4cf30b2c86..3128067bba 100644 > > > --- a/hw/i2c/core.c > > > +++ b/hw/i2c/core.c > > > @@ -23,10 +23,29 @@ static Property i2c_props[] = { > > > DEFINE_PROP_END_OF_LIST(), > > > }; > > > > > > +static void i2c_bus_hold_reset(Object *obj) > > > +{ > > > + I2CBus *bus = I2C_BUS(obj); > > > + I2CNode *node, *next; > > > + > > > + bus->broadcast = false; > > > + QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { > > > + QLIST_REMOVE(node, next); > > > + g_free(node); > > > + } > > > +} > > > > This does what it says it's going to do; but I think it > > would be good to hear from Corey whether it's better to > > do this, or instead to call i2c_end_transfer() in the > > reset-enter phase. > > Sorry, I missed this, I'm having major chaos going on right now in my > life. No worries! I also missed this for a bit. > > I don't think i2c_end_transfer() is the right thing to do. The transfer > has not cleanly ended, it is just forgotten. Sounds good to me, I'll send up v4 with the change Peter pointed out. Thanks, Joe > > > > > Mostly QEMU's "reset" is like power-cycling, in which case > > I guess that what we have here where we just forget about > > the in-progress transfer and assume the device on the other > > end is also going to reset back to a neutral state is what > > we want. > > > > Does i2c have a concept of a bus-level "reset" operation? > > No, it does not. Most I2C devices don't even have a reset pin. In a > reset situation in real hardware, the operation would be aborted by > the lines drifting high after the bus master has been reset. > > So I think this is fine as is. > > -corey > > > > > > + > > > +static void i2c_bus_class_init(ObjectClass *klass, void *data) > > > +{ > > > + ResettableClass *rc = RESETTABLE_CLASS(klass); > > > + rc->phases.hold = i2c_bus_hold_reset; > > > +} > > > + > > > static const TypeInfo i2c_bus_info = { > > > .name = TYPE_I2C_BUS, > > > .parent = TYPE_BUS, > > > .instance_size = sizeof(I2CBus), > > > + .class_init = i2c_bus_class_init, > > > }; > > > > > > > > > static int i2c_bus_pre_save(void *opaque) > > > diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h > > > index 2a3abacd1b..49580e30e2 100644 > > > --- a/include/hw/i2c/i2c.h > > > +++ b/include/hw/i2c/i2c.h > > > @@ -64,7 +64,7 @@ struct I2CSlave { > > > }; > > > > > > #define TYPE_I2C_BUS "i2c-bus" > > > -OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS) > > > +OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS) > > > > I don't think you need this change any more ? > > > > thanks > > -- PMM > >
diff --git a/hw/i2c/core.c b/hw/i2c/core.c index 4cf30b2c86..3128067bba 100644 --- a/hw/i2c/core.c +++ b/hw/i2c/core.c @@ -23,10 +23,29 @@ static Property i2c_props[] = { DEFINE_PROP_END_OF_LIST(), }; +static void i2c_bus_hold_reset(Object *obj) +{ + I2CBus *bus = I2C_BUS(obj); + I2CNode *node, *next; + + bus->broadcast = false; + QLIST_FOREACH_SAFE(node, &bus->current_devs, next, next) { + QLIST_REMOVE(node, next); + g_free(node); + } +} + +static void i2c_bus_class_init(ObjectClass *klass, void *data) +{ + ResettableClass *rc = RESETTABLE_CLASS(klass); + rc->phases.hold = i2c_bus_hold_reset; +} + static const TypeInfo i2c_bus_info = { .name = TYPE_I2C_BUS, .parent = TYPE_BUS, .instance_size = sizeof(I2CBus), + .class_init = i2c_bus_class_init, }; static int i2c_bus_pre_save(void *opaque) diff --git a/include/hw/i2c/i2c.h b/include/hw/i2c/i2c.h index 2a3abacd1b..49580e30e2 100644 --- a/include/hw/i2c/i2c.h +++ b/include/hw/i2c/i2c.h @@ -64,7 +64,7 @@ struct I2CSlave { }; #define TYPE_I2C_BUS "i2c-bus" -OBJECT_DECLARE_SIMPLE_TYPE(I2CBus, I2C_BUS) +OBJECT_DECLARE_TYPE(I2CBus, I2CBusClass, I2C_BUS) typedef struct I2CNode I2CNode;
It's possible for a reset to come in the middle of a transaction, which causes the bus to be in an old state when a new transaction comes in. Signed-off-by: Joe Komlodi <komlodi@google.com> --- hw/i2c/core.c | 19 +++++++++++++++++++ include/hw/i2c/i2c.h | 2 +- 2 files changed, 20 insertions(+), 1 deletion(-)