On 15 March 2012 07:35, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Create 9 exynos4210 i2c interfaces.>> Signed-off-by: Igor Mitsyanko <i.mitsyanko@samsung.com>
Mostly this looks OK but I still find the i2c slave stuff
odd -- should the controller really register itself as
a slave on its own bus? Doesn't this mean that the
controller can effectively try to talk to itself? Does
the hardware let you do that?
I suspect that what's happening here is that the hardware
lets you put the i2c controller into slave mode so some
other device on the bus can be a master. But QEMU's
i2c bus abstraction doesn't cover that use case at all...
Anyway, I don't know enough about i2c to really be able
to review this patch properly in that area. Anybody?
-- PMM

On 03/21/2012 03:55 PM, Peter Maydell wrote:
> On 15 March 2012 07:35, Igor Mitsyanko<i.mitsyanko@samsung.com> wrote:>> Create 9 exynos4210 i2c interfaces.>>>> Signed-off-by: Igor Mitsyanko<i.mitsyanko@samsung.com>>> Mostly this looks OK but I still find the i2c slave stuff> odd -- should the controller really register itself as> a slave on its own bus? Doesn't this mean that the> controller can effectively try to talk to itself? Does> the hardware let you do that?>
Controller's master and slave i2c interfaces operate on the same single
bus, so I think it should. It can't talk to itself because controller's
two modes of operation are mutually exclusive. As I said, I took this
approach from pxa i2c implementation, the only difference is that they
register slave interface on a separate bus, but I think it's not right
to do that.
> I suspect that what's happening here is that the hardware> lets you put the i2c controller into slave mode so some> other device on the bus can be a master. But QEMU's> i2c bus abstraction doesn't cover that use case at all...>
Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller
will never be used as i2c slave device by anyone), but I think we still
have to implement devices exactly like they described in documentation.

On 21 March 2012 13:07, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> On 03/21/2012 03:55 PM, Peter Maydell wrote:>> I suspect that what's happening here is that the hardware>> lets you put the i2c controller into slave mode so some>> other device on the bus can be a master. But QEMU's>> i2c bus abstraction doesn't cover that use case at all...
> Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller will> never be used as i2c slave device by anyone), but I think we still have to> implement devices exactly like they described in documentation.
I agree with the sentiment, I'm just not sure if the code you've
written is actually doing that. The right way to model this would
be if our i2c bus implementation provided an interface so you
could register as a device which is a master but can switch into
slave mode. Failing that, maybe we should just not support switching
into slave mode at all. Registering as two separate devices on the
i2c bus doesn't sound right to me.
-- PMM

On 03/21/2012 05:09 PM, Peter Maydell wrote:
> On 21 March 2012 13:07, Igor Mitsyanko<i.mitsyanko@samsung.com> wrote:>> On 03/21/2012 03:55 PM, Peter Maydell wrote:>>> I suspect that what's happening here is that the hardware>>> lets you put the i2c controller into slave mode so some>>> other device on the bus can be a master. But QEMU's>>> i2c bus abstraction doesn't cover that use case at all...>>> Yes, I saw this statement in hw/i2c.h (and probably cpu i2c controller will>> never be used as i2c slave device by anyone), but I think we still have to>> implement devices exactly like they described in documentation.>> I agree with the sentiment, I'm just not sure if the code you've> written is actually doing that. The right way to model this would> be if our i2c bus implementation provided an interface so you> could register as a device which is a master but can switch into> slave mode.
I don't think we can do that without multiple inheritance, and it
wouldn't worth an effort for a feature that never going to be used.
Failing that, maybe we should just not support switching
> into slave mode at all.
Do you mean we shouldn't register EXYNOS4_I2C_SLAVE at all so some
hypothetical bus master wouldn't even find EXYNOS4_I2C_SLAVE on a bus?
Maybe the best solution is to make exynos4210_i2c_slave_send() and
exynos4210_i2c_slave_recv() always return -1, so a hypothetical bus
master will treat EXYNOS4_I2C_SLAVE as a broken device. But that seems
to behave exactly like "not register at all" approach..
And are we really sure that slave interface wouldn't work correctly in a
current implementation? For example, emulated Exynos CPU issues some
command to a device A on SPI line and device A in turn issues data on
i2c line connected to Exynos i2c controller configured as slave.
EXYNOS4_I2C_SLAVE receives a data and raises interrupt flag.
Registering as two separate devices on the
> i2c bus doesn't sound right to me.>
Why? That's how it's done in hardware, I think we can roughly consider
it to be two separate devices multiplexing single bus.

On 21 March 2012 14:18, Igor Mitsyanko <i.mitsyanko@samsung.com> wrote:
> Do you mean we shouldn't register EXYNOS4_I2C_SLAVE at all so some> hypothetical bus master wouldn't even find EXYNOS4_I2C_SLAVE on a bus?> Maybe the best solution is to make exynos4210_i2c_slave_send() and> exynos4210_i2c_slave_recv() always return -1, so a hypothetical bus master> will treat EXYNOS4_I2C_SLAVE as a broken device. But that seems to behave> exactly like "not register at all" approach..> And are we really sure that slave interface wouldn't work correctly in a> current implementation? For example, emulated Exynos CPU issues some command> to a device A on SPI line and device A in turn issues data on i2c line> connected to Exynos i2c controller configured as slave. EXYNOS4_I2C_SLAVE> receives a data and raises interrupt flag.
If there's a valid configuration that works in the existing code
where we can end up receiving data correctly to the EXYNOS4_I2C_SLAVE
from some other device on the i2c bus, that's fine: we can test that
the code you have works OK.
If there is no valid configuration that will do that (because we
don't have any support for any other device being a bus master)
then the code is completely useless, untested and untestable and
we shouldn't put it in.
-- PMM