This patch series introduces a new i2c module, namely the designware one and further enables this (optionally) for the virt-arm machine.

Chris Rauer (2):
  hw/i2c: Add designware i2c controller.
  hw/arm: Enable smbus on arm virt machine.

 MAINTAINERS                     |   6 +
 docs/system/arm/virt.rst        |   4 +
 hw/arm/Kconfig                  |   1 +
 hw/arm/virt-acpi-build.c        |  24 +
 hw/arm/virt.c                   |  55 +++
 hw/i2c/Kconfig                  |   4 +
 hw/i2c/designware_i2c.c         | 821 ++++++++++++++++++++++++++++++++
 hw/i2c/meson.build              |   1 +
 include/hw/arm/virt.h           |   3 +
 include/hw/i2c/designware_i2c.h | 110 +++++
 10 files changed, 1029 insertions(+)
 create mode 100644 hw/i2c/designware_i2c.c
 create mode 100644 include/hw/i2c/designware_i2c.h

-- 
2.34.1.575.g55b058a8bb-goog

On Mon, 10 Jan 2022 at 21:47, Patrick Venture <venture@google.com> wrote:
>
> This patch series introduces a new i2c module, namely the designware one and further enables this (optionally) for the virt-arm machine.
>
> Chris Rauer (2):
>   hw/i2c: Add designware i2c controller.
>   hw/arm: Enable smbus on arm virt machine.

I need to see a pretty strong justification for why we
should be adding new kinds of devices to the virt machine,
given that it increases complexity and potential attack
surface for using it with KVM; this cover letter doesn't
seem to provide any...

thanks
-- PMM

On Thu, 13 Jan 2022 at 11:48, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 10 Jan 2022 at 21:47, Patrick Venture <venture@google.com> wrote:
> >
> > This patch series introduces a new i2c module, namely the designware one and further enables this (optionally) for the virt-arm machine.
> >
> > Chris Rauer (2):
> >   hw/i2c: Add designware i2c controller.
> >   hw/arm: Enable smbus on arm virt machine.
>
> I need to see a pretty strong justification for why we
> should be adding new kinds of devices to the virt machine,
> given that it increases complexity and potential attack
> surface for using it with KVM; this cover letter doesn't
> seem to provide any...

Forgot to mention, but my prefered approach for providing
an i2c controller on the virt board would be to have a
PCI i2c controller: that way users who do need it can plug it
in with a -device command line option, and users who don't
need it never have to worry about it. (We seem to have
an ICH9-SMB PCI device already; I have no idea if it's suitable.)

thanks
-- PMM

>I need to see a pretty strong justification for why we should be adding
new kinds of devices to the virt machine,
The designware i2c controller is a very common controller on many ARM
SoCs.  It has device tree bindings and ACPI bindings which makes it ideal
for this platform.

>Forgot to mention, but my prefered approach for providing
>an i2c controller on the virt board would be to have a
>PCI i2c controller: that way users who do need it can plug it
>in with a -device command line option, and users who don't
>need it never have to worry about it.
The device is enabled by a machine parameter, “-machine virt,smbus=true”,
and is disabled by default.

> (We seem to have an ICH9-SMB PCI device already; I have no idea if it's
suitable.)
I didn't find that device suitable because it is part of the Intel
Southbridge, which may have some Intel platform quirks, and we don't need
all the things in that IO hub.

-Chris

On Wed, 26 Jan 2022 at 17:12, Chris Rauer <crauer@google.com> wrote:
>
>> I need to see a pretty strong justification for why we should be
>> adding new kinds of devices to the virt machine,
>
> The designware i2c controller is a very common controller on many
>  ARM SoCs.  It has device tree bindings and ACPI bindings which
> makes it ideal for this platform.

No, I mean, why do we need an i2c controller on the virt
board at all ?

> >Forgot to mention, but my prefered approach for providing
> >an i2c controller on the virt board would be to have a
> >PCI i2c controller: that way users who do need it can plug it
> >in with a -device command line option, and users who don't
> >need it never have to worry about it.

> > (We seem to have an ICH9-SMB PCI device already; I have no idea if it's suitable.)
> I didn't find that device suitable because it is part of the Intel
> Southbridge, which may have some Intel platform quirks, and
> we don't need all the things in that IO hub.

That's a pity. Is there a different PCI I2C controller we could model ?

thanks
-- PMM

>>> I need to see a pretty strong justification for why we should be

>>> adding new kinds of devices to the virt machine,

>>

>> The designware i2c controller is a very common controller on many

>>  ARM SoCs.  It has device tree bindings and ACPI bindings which

>> makes it ideal for this platform.

>

>No, I mean, why do we need an i2c controller on the virt

>board at all ?

The use case we are interested in is to enable IPMI SSIF on the arm virt
machine which is why I was interested in something with ACPI support.  That
particular IPMI SSIF enablement patch is not in this series but I think it
could be pulled into the series if necessary.

> > >Forgot to mention, but my prefered approach for providing

> > >an i2c controller on the virt board would be to have a

> > >PCI i2c controller: that way users who do need it can plug it

> > >in with a -device command line option, and users who don't

> > >need it never have to worry about it.

>

> > > (We seem to have an ICH9-SMB PCI device already; I have no idea if
it's suitable.)

> > I didn't find that device suitable because it is part of the Intel

> > Southbridge, which may have some Intel platform quirks, and

> > we don't need all the things in that IO hub.

>

> That's a pity. Is there a different PCI I2C controller we could model ?

I’m not aware of any standalone PCI I2C controllers.  I’ve seen I2C
controllers on PCI devices with other things but I don’t think those could
be used for IPMI SSIF or other general I2C use cases.   Do you know of a
particular device I should take a look at?


-Chris


On Wed, Jan 26, 2022 at 10:03 AM Peter Maydell <peter.maydell@linaro.org>
wrote:

> On Wed, 26 Jan 2022 at 17:12, Chris Rauer <crauer@google.com> wrote:
> >
> >> I need to see a pretty strong justification for why we should be
> >> adding new kinds of devices to the virt machine,
> >
> > The designware i2c controller is a very common controller on many
> >  ARM SoCs.  It has device tree bindings and ACPI bindings which
> > makes it ideal for this platform.
>
> No, I mean, why do we need an i2c controller on the virt
> board at all ?
>
> > >Forgot to mention, but my prefered approach for providing
> > >an i2c controller on the virt board would be to have a
> > >PCI i2c controller: that way users who do need it can plug it
> > >in with a -device command line option, and users who don't
> > >need it never have to worry about it.
>
> > > (We seem to have an ICH9-SMB PCI device already; I have no idea if
> it's suitable.)
> > I didn't find that device suitable because it is part of the Intel
> > Southbridge, which may have some Intel platform quirks, and
> > we don't need all the things in that IO hub.
>
> That's a pity. Is there a different PCI I2C controller we could model ?
>
> thanks
> -- PMM
>

+Enrico Weigelt

On 26/1/22 19:03, Peter Maydell wrote:
> On Wed, 26 Jan 2022 at 17:12, Chris Rauer <crauer@google.com> wrote:
>>
>>> I need to see a pretty strong justification for why we should be
>>> adding new kinds of devices to the virt machine,
>>
>> The designware i2c controller is a very common controller on many
>>   ARM SoCs.  It has device tree bindings and ACPI bindings which
>> makes it ideal for this platform.
> 
> No, I mean, why do we need an i2c controller on the virt
> board at all ?
> 
>>> Forgot to mention, but my prefered approach for providing
>>> an i2c controller on the virt board would be to have a
>>> PCI i2c controller: that way users who do need it can plug it
>>> in with a -device command line option, and users who don't
>>> need it never have to worry about it.
> 
>>> (We seem to have an ICH9-SMB PCI device already; I have no idea if it's suitable.)
>> I didn't find that device suitable because it is part of the Intel
>> Southbridge, which may have some Intel platform quirks, and
>> we don't need all the things in that IO hub.
> 
> That's a pity. Is there a different PCI I2C controller we could model ?

What about using virtio-gpio & bitbang I2C?

- virtio-gpio
https://lore.kernel.org/qemu-devel/20201127182917.2387-5-info@metux.net/

- bitbang I2C already in: hw/i2c/bitbang_i2c.c

Regards,

Phil.

Hi Phil,

> What about using virtio-gpio & bitbang I2C?
>
> - virtio-gpio
> https://lore.kernel.org/qemu-devel/20201127182917.2387-5-info@metux.net/
>
> - bitbang I2C already in: hw/i2c/bitbang_i2c.c
Sorry for the delay.

That looks like it might be doable with a bit more work creating the ACPI
entries for the bitbang I2C.  This definitely seems more appropriate on the
ARM_VIRT platform instead of putting a specific controller in.

For my uses, I will have to stick with the designware controller since it
matches the system I'm emulating a little more closely.  We can hold back
the designware stuff until another SoC platform is interested in using this
controller (since it seems like it is a common one).  Hopefully someone
will find another use for the controller patches someday.

Thanks again for looking at our patches.

-Chris


On Wed, Jan 26, 2022 at 3:42 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
wrote:

> +Enrico Weigelt
>
> On 26/1/22 19:03, Peter Maydell wrote:
> > On Wed, 26 Jan 2022 at 17:12, Chris Rauer <crauer@google.com> wrote:
> >>
> >>> I need to see a pretty strong justification for why we should be
> >>> adding new kinds of devices to the virt machine,
> >>
> >> The designware i2c controller is a very common controller on many
> >>   ARM SoCs.  It has device tree bindings and ACPI bindings which
> >> makes it ideal for this platform.
> >
> > No, I mean, why do we need an i2c controller on the virt
> > board at all ?
> >
> >>> Forgot to mention, but my prefered approach for providing
> >>> an i2c controller on the virt board would be to have a
> >>> PCI i2c controller: that way users who do need it can plug it
> >>> in with a -device command line option, and users who don't
> >>> need it never have to worry about it.
> >
> >>> (We seem to have an ICH9-SMB PCI device already; I have no idea if
> it's suitable.)
> >> I didn't find that device suitable because it is part of the Intel
> >> Southbridge, which may have some Intel platform quirks, and
> >> we don't need all the things in that IO hub.
> >
> > That's a pity. Is there a different PCI I2C controller we could model ?
>
> What about using virtio-gpio & bitbang I2C?
>
> - virtio-gpio
> https://lore.kernel.org/qemu-devel/20201127182917.2387-5-info@metux.net/
>
> - bitbang I2C already in: hw/i2c/bitbang_i2c.c
>
> Regards,
>
> Phil.
>

On Mon, Feb 21, 2022 at 09:47:27AM -0800, Chris Rauer wrote:
> Hi Phil,
> 
> > What about using virtio-gpio & bitbang I2C?
> >
> > - virtio-gpio
> > https://lore.kernel.org/qemu-devel/20201127182917.2387-5-info@metux.net/
> >
> > - bitbang I2C already in: hw/i2c/bitbang_i2c.c
> Sorry for the delay.
> 
> That looks like it might be doable with a bit more work creating the ACPI
> entries for the bitbang I2C.  This definitely seems more appropriate on the
> ARM_VIRT platform instead of putting a specific controller in.

I would think the efficiency of this would be horrible.

> 
> For my uses, I will have to stick with the designware controller since it
> matches the system I'm emulating a little more closely.  We can hold back
> the designware stuff until another SoC platform is interested in using this
> controller (since it seems like it is a common one).  Hopefully someone
> will find another use for the controller patches someday.

I should have chimed in sooner.

The i.MX i2c device has ACPI and OF support, I believe, and it's already
available in qemu.  I don't think the Intel smbus device would work.

The designware one is a pretty common and general one, it's a little
suprising that it hasn't already been done on qemu.  I would be ok with
this but Peter has the big picture here.

-corey

> 
> Thanks again for looking at our patches.
> 
> -Chris
> 
> 
> On Wed, Jan 26, 2022 at 3:42 PM Philippe Mathieu-Daudé <f4bug@amsat.org>
> wrote:
> 
> > +Enrico Weigelt
> >
> > On 26/1/22 19:03, Peter Maydell wrote:
> > > On Wed, 26 Jan 2022 at 17:12, Chris Rauer <crauer@google.com> wrote:
> > >>
> > >>> I need to see a pretty strong justification for why we should be
> > >>> adding new kinds of devices to the virt machine,
> > >>
> > >> The designware i2c controller is a very common controller on many
> > >>   ARM SoCs.  It has device tree bindings and ACPI bindings which
> > >> makes it ideal for this platform.
> > >
> > > No, I mean, why do we need an i2c controller on the virt
> > > board at all ?
> > >
> > >>> Forgot to mention, but my prefered approach for providing
> > >>> an i2c controller on the virt board would be to have a
> > >>> PCI i2c controller: that way users who do need it can plug it
> > >>> in with a -device command line option, and users who don't
> > >>> need it never have to worry about it.
> > >
> > >>> (We seem to have an ICH9-SMB PCI device already; I have no idea if
> > it's suitable.)
> > >> I didn't find that device suitable because it is part of the Intel
> > >> Southbridge, which may have some Intel platform quirks, and
> > >> we don't need all the things in that IO hub.
> > >
> > > That's a pity. Is there a different PCI I2C controller we could model ?
> >
> > What about using virtio-gpio & bitbang I2C?
> >
> > - virtio-gpio
> > https://lore.kernel.org/qemu-devel/20201127182917.2387-5-info@metux.net/
> >
> > - bitbang I2C already in: hw/i2c/bitbang_i2c.c
> >
> > Regards,
> >
> > Phil.
> >