Hello,

This series starts with a first set of patches fixing I2C slave mode
in the Aspeed I2C controller, a test device and its associated test in
avocado.

Follow some cleanups which allow the use of block devices instead of
drives. So that, instead of specifying :

  -drive file=./flash-ast2600-evb,format=raw,if=mtd
  -drive file=./ast2600-evb.pnor,format=raw,if=mtd
  ...

and guessing from the order which bus the device is attached to, we
can use :

  -blockdev node-name=fmc0,driver=file,filename=./bmc.img
  -device mx66u51235f,bus=ssi.0,drive=fmc0
  -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
  -device mx66u51235f,bus=ssi.0,drive=fmc1 
  -blockdev node-name=pnor,driver=file,filename=./pnor
  -device mx66l1g45g,bus=ssi.1,drive=pnor
  ...

It is not perfect, the CS index still depends on the order, but it is
now possible to run a machine without -drive ...,if=mtd.

This lacks the final patch enabling the '-nodefaults' option by not
creating the default devices if specified on the command line. It
needs some more evaluation of the possible undesired effects. 
Thanks,

C.

Cédric Le Goater (6):
  m25p80: Improve error when the backend file size does not match the
    device
  tests/avocado/machine_aspeed.py: Add I2C slave tests
  aspeed/smc: Replace SysBus IRQs with GPIO lines
  aspeed/smc: Wire CS lines at reset
  aspeed: Introduce a spi_boot region under the SoC
  aspeed: Add a boot_rom overlap region in the SoC spi_boot container

Klaus Jensen (2):
  hw/i2c: only schedule pending master when bus is idle
  hw/misc: add a toy i2c echo device

 include/hw/arm/aspeed_soc.h     |   3 +
 include/hw/i2c/i2c.h            |   2 +
 hw/arm/aspeed.c                 |  60 ++++++------
 hw/arm/aspeed_ast2600.c         |  13 +++
 hw/arm/aspeed_soc.c             |  14 +++
 hw/arm/fby35.c                  |   8 +-
 hw/block/m25p80.c               |   4 +-
 hw/i2c/aspeed_i2c.c             |   2 +
 hw/i2c/core.c                   |  37 +++++---
 hw/misc/i2c-echo.c              | 156 ++++++++++++++++++++++++++++++++
 hw/ssi/aspeed_smc.c             |  29 +++++-
 hw/misc/meson.build             |   2 +
 tests/avocado/machine_aspeed.py |  10 ++
 13 files changed, 279 insertions(+), 61 deletions(-)
 create mode 100644 hw/misc/i2c-echo.c

-- 
2.39.1

On 14/2/23 18:18, Cédric Le Goater wrote:
> Hello,
> 
> This series starts with a first set of patches fixing I2C slave mode
> in the Aspeed I2C controller, a test device and its associated test in
> avocado.
> 
> Follow some cleanups which allow the use of block devices instead of
> drives. So that, instead of specifying :
> 
>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>    ...
> 
> and guessing from the order which bus the device is attached to, we
> can use :
> 
>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>    -blockdev node-name=pnor,driver=file,filename=./pnor
>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>    ...
> 
> It is not perfect, the CS index still depends on the order

Quick thoughts here:

TYPE_SSI_PERIPHERAL devices have one input SSI_GPIO_CS.

TYPE_SSI_BUS could have a "cs-num" property (how many
CS line associated with this bus) and create an array of
#cs-num output SSI_GPIO_CS.

TYPE_SSI_PERIPHERAL could have a "cs" (index) property;
if set, upon ssi_peripheral_realize() when the device is
plugged on the bus, the GPIO line is wired.

So we could set the 'cs=' property from CLI:

   -blockdev node-name=fmc0,driver=file,filename=./bmc.img
   -device mx66u51235f,bus=ssi.0,cs=1,drive=fmc0
   -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
   -device mx66u51235f,bus=ssi.0,cs=0,drive=fmc1

> but it is
> now possible to run a machine without -drive ...,if=mtd.
> 
> This lacks the final patch enabling the '-nodefaults' option by not
> creating the default devices if specified on the command line. It
> needs some more evaluation of the possible undesired effects.
> Thanks,
> 
> C.

On 2/15/23 11:45, Philippe Mathieu-Daudé wrote:
> On 14/2/23 18:18, Cédric Le Goater wrote:
>> Hello,
>>
>> This series starts with a first set of patches fixing I2C slave mode
>> in the Aspeed I2C controller, a test device and its associated test in
>> avocado.
>>
>> Follow some cleanups which allow the use of block devices instead of
>> drives. So that, instead of specifying :
>>
>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>    ...
>>
>> and guessing from the order which bus the device is attached to, we
>> can use :
>>
>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>    ...
>>
>> It is not perfect, the CS index still depends on the order
> 
> Quick thoughts here:
> 
> TYPE_SSI_PERIPHERAL devices have one input SSI_GPIO_CS.
> 
> TYPE_SSI_BUS could have a "cs-num" property (how many
> CS line associated with this bus) and create an array of
> #cs-num output SSI_GPIO_CS.
> 
> TYPE_SSI_PERIPHERAL could have a "cs" (index) property;
> if set, upon ssi_peripheral_realize() when the device is
> plugged on the bus, the GPIO line is wired.

yes. I would like to check first the impact on migration compatibility.

Thanks,

C.

> So we could set the 'cs=' property from CLI:
> 
>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>    -device mx66u51235f,bus=ssi.0,cs=1,drive=fmc0
>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>    -device mx66u51235f,bus=ssi.0,cs=0,drive=fmc1
> 
>> but it is
>> now possible to run a machine without -drive ...,if=mtd.
>>
>> This lacks the final patch enabling the '-nodefaults' option by not
>> creating the default devices if specified on the command line. It
>> needs some more evaluation of the possible undesired effects.
>> Thanks,
>>
>> C.
> 

Cédric Le Goater <clg@kaod.org> writes:

> Hello,
>
> This series starts with a first set of patches fixing I2C slave mode
> in the Aspeed I2C controller, a test device and its associated test in
> avocado.
>
> Follow some cleanups which allow the use of block devices instead of
> drives. So that, instead of specifying :
>
>   -drive file=./flash-ast2600-evb,format=raw,if=mtd
>   -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>   ...
>
> and guessing from the order which bus the device is attached to, we
> can use :
>
>   -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>   -device mx66u51235f,bus=ssi.0,drive=fmc0
>   -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>   -device mx66u51235f,bus=ssi.0,drive=fmc1 
>   -blockdev node-name=pnor,driver=file,filename=./pnor
>   -device mx66l1g45g,bus=ssi.1,drive=pnor
>   ...
>
> It is not perfect, the CS index still depends on the order, but it is
> now possible to run a machine without -drive ...,if=mtd.

Lovely!

Does this cover all uses of IF_MTD, or only some?

> This lacks the final patch enabling the '-nodefaults' option by not
> creating the default devices if specified on the command line. It
> needs some more evaluation of the possible undesired effects. 

Are you thinking of something similar to the default CD-ROM, i.e. use
default_list to have -device suppress a certain kind of default devices,
and also have -nodefaults suppress them all?

On 2/15/23 07:38, Markus Armbruster wrote:
> Cédric Le Goater <clg@kaod.org> writes:
> 
>> Hello,
>>
>> This series starts with a first set of patches fixing I2C slave mode
>> in the Aspeed I2C controller, a test device and its associated test in
>> avocado.
>>
>> Follow some cleanups which allow the use of block devices instead of
>> drives. So that, instead of specifying :
>>
>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>    ...
>>
>> and guessing from the order which bus the device is attached to, we
>> can use :
>>
>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>    ...
>>
>> It is not perfect, the CS index still depends on the order, but it is
>> now possible to run a machine without -drive ...,if=mtd.
> 
> Lovely!
> 
> Does this cover all uses of IF_MTD, or only some?

All use for default devices in the aspeed machines. I think most
machines use IF_MTD in a similar way.

Yet, one extra use of IF_MTD is to fill a ROM region with the first
drive contents. It avoids fetching instructions from the SPI flash
mapping and speeds up boot quite significantly.

Once the default flash devices are created and attached to their
drive, it is possible to query the block backend without the
drive_get() API. I have a couple of patches for it and it shouldn't
be a problem.
  
>> This lacks the final patch enabling the '-nodefaults' option by not
>> creating the default devices if specified on the command line. It
>> needs some more evaluation of the possible undesired effects.
> 
> Are you thinking of something similar to the default CD-ROM, i.e. use
> default_list to have -device suppress a certain kind of default devices,
> and also have -nodefaults suppress them all?

I would have -nodefaults suppress all flash devices. There are also
I2C devices but they are less problematic for the machine definition.
(Well, eeprom contents can be complex to handle)

The next step would be to get rid of the drive_get(IF_MTD) internal
API, which means finding a way to attach block backend devices from
the command line to the default flash devices. This should be done
at machine init time and the blockdev should have some 'bus@addr'
identifier. I lack the knowledge on how this could be done.

Thanks,

C.

  

Cédric Le Goater <clg@kaod.org> writes:

> On 2/15/23 07:38, Markus Armbruster wrote:
>> Cédric Le Goater <clg@kaod.org> writes:
>> 
>>> Hello,
>>>
>>> This series starts with a first set of patches fixing I2C slave mode
>>> in the Aspeed I2C controller, a test device and its associated test in
>>> avocado.
>>>
>>> Follow some cleanups which allow the use of block devices instead of
>>> drives. So that, instead of specifying :
>>>
>>>    -drive file=./flash-ast2600-evb,format=raw,if=mtd
>>>    -drive file=./ast2600-evb.pnor,format=raw,if=mtd
>>>    ...
>>>
>>> and guessing from the order which bus the device is attached to, we
>>> can use :
>>>
>>>    -blockdev node-name=fmc0,driver=file,filename=./bmc.img
>>>    -device mx66u51235f,bus=ssi.0,drive=fmc0
>>>    -blockdev node-name=fmc1,driver=file,filename=./bmc-alt.img
>>>    -device mx66u51235f,bus=ssi.0,drive=fmc1
>>>    -blockdev node-name=pnor,driver=file,filename=./pnor
>>>    -device mx66l1g45g,bus=ssi.1,drive=pnor
>>>    ...
>>>
>>> It is not perfect, the CS index still depends on the order, but it is
>>> now possible to run a machine without -drive ...,if=mtd.
>>
>> Lovely!
>>
>> Does this cover all uses of IF_MTD, or only some?
>
> All use for default devices in the aspeed machines. I think most
> machines use IF_MTD in a similar way.
>
> Yet, one extra use of IF_MTD is to fill a ROM region with the first
> drive contents. It avoids fetching instructions from the SPI flash
> mapping and speeds up boot quite significantly.
>
> Once the default flash devices are created and attached to their
> drive, it is possible to query the block backend without the
> drive_get() API. I have a couple of patches for it and it shouldn't
> be a problem.
>>
>>> This lacks the final patch enabling the '-nodefaults' option by not
>>> creating the default devices if specified on the command line. It
>>> needs some more evaluation of the possible undesired effects.
>>
>> Are you thinking of something similar to the default CD-ROM, i.e. use
>> default_list to have -device suppress a certain kind of default devices,
>> and also have -nodefaults suppress them all?
>
> I would have -nodefaults suppress all flash devices. There are also
> I2C devices but they are less problematic for the machine definition.
> (Well, eeprom contents can be complex to handle)
>
> The next step would be to get rid of the drive_get(IF_MTD) internal
> API, which means finding a way to attach block backend devices from
> the command line to the default flash devices. This should be done
> at machine init time and the blockdev should have some 'bus@addr'
> identifier. I lack the knowledge on how this could be done.

Possibly interesting for you: commit e0561e60f17 "hw/arm/virt: Support
firmware configuration with -blockdev".

>> The next step would be to get rid of the drive_get(IF_MTD) internal
>> API, which means finding a way to attach block backend devices from
>> the command line to the default flash devices. This should be done
>> at machine init time and the blockdev should have some 'bus@addr'
>> identifier. I lack the knowledge on how this could be done.
> 
> Possibly interesting for you: commit e0561e60f17 "hw/arm/virt: Support
> firmware configuration with -blockdev".

I see. The mapping device<->blk is moved at the machine level with
an option. The same could be done for the Aspeed machines with "fmc0",
"spi0", identifiers.

I think we should deprecate the "fmc-model" and "spi-model" machine
options. They become useless with -nodefaults correctly implemented.

Thanks,