| Message ID | 20230214171830.681594-1-clg@kaod.org |
|---|---|
| Headers | show |
| Series | aspeed: I2C fixes, -drive removal (first step) | expand |
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.
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.
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,
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. >
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