| Message ID | 20211103220133.1422879-4-wuhaotsh@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc NPCM7XX patches | expand |
Hao Wu <wuhaotsh@google.com> writes: > We made 3 changes to the at24c_eeprom_init function in > npcm7xx_boards.c: > > 1. We allow the function to take a I2CBus* as parameter. This allows > us to attach an EEPROM device behind an I2C mux which is not > possible with the old method. > > 2. We make at24c EEPROMs are backed by drives so that we can > specify the content of the EEPROMs. > > 3. Instead of using i2c address as unit number, This patch assigns > unique unit numbers for each eeproms in each board. This avoids > conflict in providing multiple eeprom contents with the same address. > In the old method if we specify two drives with the same unit number, > the following error will occur: `Device with id 'none85' exists`. When a commit does three things, chances are splitting it into three commits would be an improvement. This is *not* a demand. > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > --- > hw/arm/npcm7xx_boards.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c > index dec7d16ae5..9121e081fa 100644 > --- a/hw/arm/npcm7xx_boards.c > +++ b/hw/arm/npcm7xx_boards.c > @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num) > return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus")); > } > > -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr, > - uint32_t rsize) > +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr, > + uint32_t rsize, int unit_number) I'd keep bus and unit number together. I'd name the new parameter @unit, to match drive_get(). > { > - I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus); > I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr); > DeviceState *dev = DEVICE(i2c_dev); > + DriveInfo *dinfo; > > + dinfo = drive_get(IF_OTHER, bus, unit_number); > + if (dinfo) { > + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); > + } > qdev_prop_set_uint32(dev, "rom-size", rsize); > i2c_slave_realize_and_unref(i2c_dev, i2c_bus, &error_abort); > } > @@ -239,8 +243,8 @@ static void quanta_gsj_i2c_init(NPCM7xxState *soc) > i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x5c); > i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x5c); > > - at24c_eeprom_init(soc, 9, 0x55, 8192); > - at24c_eeprom_init(soc, 10, 0x55, 8192); > + at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192, 0); > + at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192, 1); > > /* > * i2c-11:
Am 03.11.2021 um 23:01 hat Hao Wu geschrieben: > We made 3 changes to the at24c_eeprom_init function in > npcm7xx_boards.c: > > 1. We allow the function to take a I2CBus* as parameter. This allows > us to attach an EEPROM device behind an I2C mux which is not > possible with the old method. > > 2. We make at24c EEPROMs are backed by drives so that we can > specify the content of the EEPROMs. > > 3. Instead of using i2c address as unit number, This patch assigns > unique unit numbers for each eeproms in each board. This avoids > conflict in providing multiple eeprom contents with the same address. > In the old method if we specify two drives with the same unit number, > the following error will occur: `Device with id 'none85' exists`. > > Signed-off-by: Hao Wu <wuhaotsh@google.com> > --- > hw/arm/npcm7xx_boards.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c > index dec7d16ae5..9121e081fa 100644 > --- a/hw/arm/npcm7xx_boards.c > +++ b/hw/arm/npcm7xx_boards.c > @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num) > return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus")); > } > > -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr, > - uint32_t rsize) > +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr, > + uint32_t rsize, int unit_number) > { > - I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus); > I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr); > DeviceState *dev = DEVICE(i2c_dev); > + DriveInfo *dinfo; > > + dinfo = drive_get(IF_OTHER, bus, unit_number); > + if (dinfo) { > + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); > + } I may be missing the history of this series, but why do we have to use a legacy DriveInfo here instead of adding a drive property to the board to make this configurable with -blockdev? Adding even more devices that can only be configured with -drive feels like a step backwards to me. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 03.11.2021 um 23:01 hat Hao Wu geschrieben: >> We made 3 changes to the at24c_eeprom_init function in >> npcm7xx_boards.c: >> >> 1. We allow the function to take a I2CBus* as parameter. This allows >> us to attach an EEPROM device behind an I2C mux which is not >> possible with the old method. >> >> 2. We make at24c EEPROMs are backed by drives so that we can >> specify the content of the EEPROMs. >> >> 3. Instead of using i2c address as unit number, This patch assigns >> unique unit numbers for each eeproms in each board. This avoids >> conflict in providing multiple eeprom contents with the same address. >> In the old method if we specify two drives with the same unit number, >> the following error will occur: `Device with id 'none85' exists`. >> >> Signed-off-by: Hao Wu <wuhaotsh@google.com> >> --- >> hw/arm/npcm7xx_boards.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c >> index dec7d16ae5..9121e081fa 100644 >> --- a/hw/arm/npcm7xx_boards.c >> +++ b/hw/arm/npcm7xx_boards.c >> @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num) >> return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus")); >> } >> >> -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr, >> - uint32_t rsize) >> +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr, >> + uint32_t rsize, int unit_number) >> { >> - I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus); >> I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr); >> DeviceState *dev = DEVICE(i2c_dev); >> + DriveInfo *dinfo; >> >> + dinfo = drive_get(IF_OTHER, bus, unit_number); >> + if (dinfo) { >> + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); >> + } > > I may be missing the history of this series, but why do we have to use a > legacy DriveInfo here instead of adding a drive property to the board to > make this configurable with -blockdev? > > Adding even more devices that can only be configured with -drive feels > like a step backwards to me. More like sideways. The big unfinished part of -blockdev is configuring onboard devices with it. I took a stab at one instance in commit ebc29e1bea "pc: Support firmware configuration with -blockdev", 2019-03-11. Quoting from the commit message: Mapping -drive if=none,... to -blockdev is a solved problem. With if=T other than if=none, -drive additionally configures a block device frontend. For non-onboard devices, that part maps to -device. Also a solved problem. For onboard devices such as PC flash memory, we have an unsolved problem. This is actually an instance of a wider problem: our general device configuration interface doesn't cover onboard devices. Instead, we have a zoo of ad hoc interfaces that are much more limited. One of them is -drive, which we'd rather deprecate, but can't until we have suitable replacements for all its uses. Sadly, I can't attack the wider problem today. So back to the narrow problem. My first idea was to reduce it to its solved buddy by using pluggable instead of onboard devices for the flash memory. Workable, but it requires some extra smarts in firmware descriptors and libvirt. Paolo had an idea that is simpler for libvirt: keep the devices onboard, and add machine properties for their block backends. The implementation is less than straightforward, I'm afraid. First, block backend properties are *qdev* properties. Machines can't have those, as they're not devices. I could duplicate these qdev properties as QOM properties, but I hate that. More seriously, the properties do not belong to the machine, they belong to the onboard flash devices. Adding them to the machine would then require bad magic to somehow transfer them to the flash devices. Fortunately, QOM provides the means to handle exactly this case: add alias properties to the machine that forward to the onboard devices' properties. Properties need to be created in .instance_init() methods. For PC machines, that's pc_machine_initfn(). To make alias properties work, we need to create the onboard flash devices there, too. Requires several bug fixes, in the previous commits. We also have to realize the devices. More on that below. The need to create onboard devices differently results in a non-trivial refactoring. The need for keeping -drive working complicates things further. The "several bug fixes" took me 26 preparatory patches, plus at least three more to fix regressions caused by one of them. I then did the same for ARM virt in commit e0561e60f1 "hw/arm/virt: Support firmware configuration with -blockdev", 2019-05-07. Only a few preparatory patches, but the patch again includes non-trivial refactoring. Two new machines (ARM sbsa-ref and RISC-V virt) have since done it this way from the start, both in 2019. Definitely easier than the refactoring I did for PC machines and ARM virt. More than a hundred drive_get() remain in some 70 files hw/. These should all be for onboard block devices. I'm not aware of progress since 2019. If "replace them all by machine properties" is the way forward, we need to get going. At the current rate of one file a year (measured charitably), we'll be done around 2090, provided we don't add more (we've added quite a few since I did the first replacement). If it isn't a practical way forward, then what is? And this is what makes me go "more like sideways". Hao Wu is doing what the vast majority of the code does. Yes, that's not moving us forward on this thorny set of problems. But it's hardly setting us back, either. Before we can ask people to help us move forward, we need to find the way forward. I'm not sure we have.
diff --git a/hw/arm/npcm7xx_boards.c b/hw/arm/npcm7xx_boards.c index dec7d16ae5..9121e081fa 100644 --- a/hw/arm/npcm7xx_boards.c +++ b/hw/arm/npcm7xx_boards.c @@ -126,13 +126,17 @@ static I2CBus *npcm7xx_i2c_get_bus(NPCM7xxState *soc, uint32_t num) return I2C_BUS(qdev_get_child_bus(DEVICE(&soc->smbus[num]), "i2c-bus")); } -static void at24c_eeprom_init(NPCM7xxState *soc, int bus, uint8_t addr, - uint32_t rsize) +static void at24c_eeprom_init(I2CBus *i2c_bus, int bus, uint8_t addr, + uint32_t rsize, int unit_number) { - I2CBus *i2c_bus = npcm7xx_i2c_get_bus(soc, bus); I2CSlave *i2c_dev = i2c_slave_new("at24c-eeprom", addr); DeviceState *dev = DEVICE(i2c_dev); + DriveInfo *dinfo; + dinfo = drive_get(IF_OTHER, bus, unit_number); + if (dinfo) { + qdev_prop_set_drive(dev, "drive", blk_by_legacy_dinfo(dinfo)); + } qdev_prop_set_uint32(dev, "rom-size", rsize); i2c_slave_realize_and_unref(i2c_dev, i2c_bus, &error_abort); } @@ -239,8 +243,8 @@ static void quanta_gsj_i2c_init(NPCM7xxState *soc) i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 3), "tmp105", 0x5c); i2c_slave_create_simple(npcm7xx_i2c_get_bus(soc, 4), "tmp105", 0x5c); - at24c_eeprom_init(soc, 9, 0x55, 8192); - at24c_eeprom_init(soc, 10, 0x55, 8192); + at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 9), 9, 0x55, 8192, 0); + at24c_eeprom_init(npcm7xx_i2c_get_bus(soc, 10), 10, 0x55, 8192, 1); /* * i2c-11:
We made 3 changes to the at24c_eeprom_init function in npcm7xx_boards.c: 1. We allow the function to take a I2CBus* as parameter. This allows us to attach an EEPROM device behind an I2C mux which is not possible with the old method. 2. We make at24c EEPROMs are backed by drives so that we can specify the content of the EEPROMs. 3. Instead of using i2c address as unit number, This patch assigns unique unit numbers for each eeproms in each board. This avoids conflict in providing multiple eeprom contents with the same address. In the old method if we specify two drives with the same unit number, the following error will occur: `Device with id 'none85' exists`. Signed-off-by: Hao Wu <wuhaotsh@google.com> --- hw/arm/npcm7xx_boards.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)