Re: [PATCH] misc/pca955*: Move models under hw/gpio
On Mon, 2024-03-25 at 14:48 +0100, Cédric Le Goater wrote: > The PCA9552 and PCA9554 devices are both I2C GPIO controllers and the > PCA9552 also can drive LEDs. Do all the necessary adjustments to move > the models under hw/gpio. > > Cc: Glenn Miles > Signed-off-by: Cédric Le Goater Acked-by: Andrew Jeffery
Re: [PATCH 22/33] hw/arm/aspeed_ast2600: Let the A7MPcore create/wire the CPU cores
On Tue, 2023-12-12 at 17:29 +0100, Philippe Mathieu-Daudé wrote: > Set the properties on the a7mpcore object to let it create and > wire the CPU cores. Remove the redundant code. > > Signed-off-by: Philippe Mathieu-Daudé Acked-by: Andrew Jeffery
Re: [PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls
On Mon, 2023-11-20 at 22:32 +0100, Philippe Mathieu-Daudé wrote:
> Mechanical change using the following coccinelle script:
>
> @@
> expression mr, owner, arg3, arg4, errp;
> @@
> - memory_region_init_ram(mr, owner, arg3, arg4, );
> if (
> - errp
> + !memory_region_init_ram(mr, owner, arg3, arg4, )
> ) {
> ...
> return;
> }
>
> and removing the local Error variable.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> hw/arm/aspeed_ast2400.c | 6 ++
> hw/arm/aspeed_ast2600.c | 6 ++
For my own benefit it looks like the motivating thread for this series
is:
https://lore.kernel.org/qemu-devel/936e1ac4-cef8-08b4-c688-e5b1e0572...@linaro.org/
Anyway,
Reviewed-by: Andrew Jeffery # aspeed
Re: [PATCH v3 2/2] misc/pca9552: Let external devices set pca9552 inputs
On Tue, 2023-10-24 at 13:11 -0500, Glenn Miles wrote: > Allow external devices to drive pca9552 input pins by adding > input GPIO's to the model. This allows a device to connect > its output GPIO's to the pca9552 input GPIO's. > > In order for an external device to set the state of a pca9552 > pin, the pin must first be configured for high impedance (LED > is off). If the pca9552 pin is configured to drive the pin low > (LED is on), then external input will be ignored. > > Here is a table describing the logical state of a pca9552 pin > given the state being driven by the pca9552 and an external device: > >PCA9552 >Configured >State > > | Hi-Z | Low | > --+--+-+ > External Hi-Z | Hi | Low | > Device--+--+-+ > State Low | Low | Low | > --+--+-----+ > > Signed-off-by: Glenn Miles Reviewed-by: Andrew Jeffery
Re: [PATCH v2] misc/led: LED state is set opposite of what is expected
On Tue, 2023-10-24 at 14:11 -0500, Glenn Miles wrote: > Testing of the LED state showed that when the LED polarity was > set to GPIO_POLARITY_ACTIVE_LOW and a low logic value was set on > the input GPIO of the LED, the LED was being turn off when it was > expected to be turned on. > > Signed-off-by: Glenn Miles Reviewed-by: Andrew Jeffery
Re: [PATCH v6 06/10] hw/fsi: Aspeed APB2OPB interface
On Thu, 2023-10-26 at 10:27 -0500, Ninad Palsule wrote: > Hello Cedric, > > > On 10/24/23 10:21, Cédric Le Goater wrote: > > On 10/24/23 17:00, Ninad Palsule wrote: > > > Hello Cedric, > > > > > > On 10/24/23 02:46, Cédric Le Goater wrote: > > > > and the fsi_opb_* routines are useless to me. > > > We are trying to keep the separation between OPB implementation and > > > interface hence we have all those fsi_opb_*. I feel that we should > > > keep as it is so that future extensions will be easier. Please let me > > > know. > > > > Well, I can't really tell because I don't know enough about FSI :/ > > > > The models look fragile and I have spent already a lot of time trying > > to untangle what they are trying to do. Please ask your teammates or > > let's see in the next QEMU cycle. > > I have decided to go with the approach you suggested and it looks much > better. Fixed it. I intended to reply to this before Ninad sent out v7, but life intervened. If we can't justify it with the code we have now I think it's right to pull it out. Add the code to support the things we're trying to do when we need to do them. As long as we don't do anything that precludes us from adding that code later (and I can't really imagine how we'd corner ourselves like that). We should bear in mind I wrote the initial models several years ago in the space of about a week while I was trying to learn FSI (and more deeply about the QEMU bus and address space modelling). I think I was doing that to unblock some CI due to the introduction of the kernel driver for the Aspeed FSI hardware. The models were pretty rough - prior to all this review the code reflected my hazy understanding of the problems. I didn't get time to remove the complexities introduced by my misunderstandings, and now it's been so long that I'm not much help with fixing them. Andrew
Re: [PATCH] MAINTAINERS: aspeed: Update Andrew's email address
On Wed, 2023-10-25 at 10:57 +0200, Cédric Le Goater wrote: > On 10/24/23 11:36, Cédric Le Goater wrote: > > On 9/25/23 08:22, Andrew Jeffery wrote: > > > I've changed employers, have company email that deals with patch-based > > > workflows without too much of a headache, and am trying to steer some > > > content out of my personal mail. > > > > > > Signed-off-by: Andrew Jeffery > > > --- > > > > > > Hi Cédric, do you mind including this in your Aspeed queue? > > > > > > I am not sure to send an Aspeed PR. May be this change could go > > through qemu-trivial ? > > After all, I will send an Aspeed PR. Don't bother. Thanks!
Re: [PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices
On Wed, 2023-10-25 at 11:22 +0200, Klaus Jensen wrote: > On Oct 25 11:14, Cédric Le Goater wrote: > > It seems that the "at24c-eeprom" model doesn't have a maintainer. Until > > this is sorted out, may be this change could go through the NVMe queue > > since it is related. > > > > I can, but I'm not that confident on determining if we choose to > implement this behavior broadly. I have no qualms, but someone who works > more with embedded stuff might? What are the feelings on putting the behaviour behind a flag? We could add it as a property that we can set, e.g. when defining a machine. Andrew
Re: [PATCH v2 1/3] misc/pca9552: Fix inverted input status
On Fri, 2023-10-20 at 13:23 -0500, Glenn Miles wrote: > The pca9552 INPUT0 and INPUT1 registers are supposed to > hold the logical values of the LED pins. A logical 0 > should be seen in the INPUT0/1 registers for a pin when > its corresponding LSn bits are set to 0, which is also > the state needed for turning on an LED in a typical > usage scenario. Existing code was doing the opposite > and setting INPUT0/1 bit to a 1 when the LSn bit was > set to 0, so this commit fixes that. > > Signed-off-by: Glenn Miles I sent a Reviewed-by tag for v1, don't forget to collect those on your patches before sending out a new set. Something for next time :) Anyway, Reviewed-by: Andrew Jeffery
Re: [PATCH v2 3/3] misc/pca9552: Only update output GPIOs if state changed
On Fri, 2023-10-20 at 13:23 -0500, Glenn Miles wrote:
> The pca9552 code was updating output GPIO states whenever
> the pin state was updated even if the state did not change.
> This commit adds a check so that we only update the GPIO
> output when the pin state actually changes.
Given this is intertwined with patch 2/3 that adds the input mode
capability, I think they should be squashed together?
>
> Signed-off-by: Glenn Miles
> ---
>
> New commit in v2
>
> hw/misc/pca9552.c | 25 ++---
> 1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index ed814d1f98..4ed6903404 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -112,14 +112,15 @@ static void pca955x_update_pin_input(PCA955xState *s)
>
> for (i = 0; i < k->pin_count; i++) {
> uint8_t input_reg = PCA9552_INPUT0 + (i / 8);
> -uint8_t input_shift = (i % 8);
> +uint8_t bit_mask = 1 << (i % 8);
> uint8_t config = pca955x_pin_get_config(s, i);
> +uint8_t old_value = s->regs[input_reg] & bit_mask;
> +uint8_t new_value;
>
> switch (config) {
> case PCA9552_LED_ON:
> /* Pin is set to 0V to turn on LED */
> -qemu_set_irq(s->gpio_out[i], 0);
> -s->regs[input_reg] &= ~(1 << input_shift);
> +s->regs[input_reg] &= ~bit_mask;
> break;
> case PCA9552_LED_OFF:
> /*
> @@ -128,11 +129,9 @@ static void pca955x_update_pin_input(PCA955xState *s)
> * external device drives it low.
> */
> if (s->ext_state[i] == PCA9552_PIN_LOW) {
> -qemu_set_irq(s->gpio_out[i], 0);
> -s->regs[input_reg] &= ~(1 << input_shift);
> +s->regs[input_reg] &= ~bit_mask;
> } else {
> -qemu_set_irq(s->gpio_out[i], 1);
> -s->regs[input_reg] |= 1 << input_shift;
> +s->regs[input_reg] |= bit_mask;
> }
> break;
> case PCA9552_LED_PWM0:
> @@ -141,6 +140,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
> default:
> break;
> }
> +
> +/* update irq state only if pin state changed */
> +new_value = s->regs[input_reg] & bit_mask;
> +if (new_value != old_value) {
> +if (new_value) {
> +/* changed from 0 to 1 */
> +qemu_set_irq(s->gpio_out[i], 1);
> +} else {
> +/* changed from 1 to 0 */
> +qemu_set_irq(s->gpio_out[i], 0);
> +}
Slightly code-golf-y, but the inner if-else here may be compressed to:
qemu_set_irq(s->gpio_out[i], !!new_value);
Andrew
Re: [PATCH 1/2] misc/pca9552: Fix inverted input status
On Fri, 2023-10-20 at 11:32 -0500, Miles Glenn wrote:
> On Fri, 2023-10-20 at 11:42 +0200, Philippe Mathieu-Daudé wrote:
> > On 20/10/23 04:51, Andrew Jeffery wrote:
> > > On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > > > > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > > > > hold the logical values of the LED pins. A logical 0
> > > > > should be seen in the INPUT0/1 registers for a pin when
> > > > > its corresponding LSn bits are set to 0, which is also
> > > > > the state needed for turning on an LED in a typical
> > > > > usage scenario. Existing code was doing the opposite
> > > > > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > > > > set to 0, so this commit fixes that.
> > > > >
> > > > > Signed-off-by: Glenn Miles
> > > > > ---
> > > > >
> > > > > Changes from prior version:
> > > > > - Added comment regarding pca953X
> > > > > - Added cover letter
> > > > >
> > > > > hw/misc/pca9552.c | 18 +-
> > > > > tests/qtest/pca9552-test.c | 6 +++---
> > > > > 2 files changed, 16 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > > > > index fff19e369a..445f56a9e8 100644
> > > > > --- a/hw/misc/pca9552.c
> > > > > +++ b/hw/misc/pca9552.c
> > > > > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> > > > >
> > > > > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > > > > TYPE_PCA955X)
> > > > > -
> > > > > +/*
> > > > > + * Note: The LED_ON and LED_OFF configuration values for the
> > > > > PCA955X
> > > > > + *chips are the reverse of the PCA953X family of
> > > > > chips.
> > > > > + */
> > > > > #define PCA9552_LED_ON 0x0
> > > > > #define PCA9552_LED_OFF 0x1
> > > > > #define PCA9552_LED_PWM0 0x2
> > > > > @@ -112,13 +115,18 @@ static void
> > > > > pca955x_update_pin_input(PCA955xState *s)
> > > > >
> > > > > switch (config) {
> > > > > case PCA9552_LED_ON:
> > > > > -qemu_set_irq(s->gpio[i], 1);
> > > > > -s->regs[input_reg] |= 1 << input_shift;
> > > > > -break;
> > > > > -case PCA9552_LED_OFF:
> > > > > +/* Pin is set to 0V to turn on LED */
> > > > > qemu_set_irq(s->gpio[i], 0);
> > > > > s->regs[input_reg] &= ~(1 << input_shift);
> > > > > break;
> > > > > +case PCA9552_LED_OFF:
> > > > > +/*
> > > > > + * Pin is set to Hi-Z to turn off LED and
> > > > > + * pullup sets it to a logical 1.
> > > > > + */
> > > > > +qemu_set_irq(s->gpio[i], 1);
> > > > > +s->regs[input_reg] |= 1 << input_shift;
> > > > > +break;
> > >
> > > So the witherspoon-bmc machine was a user of the PCA9552 outputs as
> > > LEDs. I guess its LEDs were in the wrong state the whole time? That
> > > looks like the only user though, and shouldn't be negatively
> > > affected.
> >
> > Usually GPIO polarity is a machine/board property, not a device
> > one.
> >
> > We could use the LED API (hw/misc/led.h) which initialize each
> > output with GpioPolarity.
> >
>
> Thanks for your comments! This piqued my curiosity so I decided to
> run a test with the witherspoon-bmc machine. Without my changes, I ran
> the following command to turn off LED13 on the pca9552 which I had
> previously set to "on":
>
> qom-set /machine/unattached/device[18] led13 off
>
> I had GDB connected at the time with a breakpoint set on
> led_set_state() so that I could see what was happening. Due to the
> inversion bug, I expected the pca9552 code to set the pin low and also
> set the irq low, which it did. The connected LED's on this pca9552
> were all configured as GPIO_POLARITY_ACTIVE_LOW, so I expected that
> setting the irq state low would actually turn on the LED. Instead it
Re: [PATCH 2/2] misc/pca9552: Let external devices set pca9552 inputs
On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> Allow external devices to drive pca9552 input pins by adding
> input GPIO's to the model. This allows a device to connect
> its output GPIO's to the pca9552 input GPIO's.
>
> In order for an external device to set the state of a pca9552
> pin, the pin must first be configured for high impedance (LED
> is off). If the pca9552 pin is configured to drive the pin low
> (LED is on), then external input will be ignored.
>
> Here is a table describing the logical state of a pca9552 pin
> given the state being driven by the pca9552 and an external device:
>
>PCA9552
>Configured
>State
>
> | Hi-Z | Low |
> --+--+-+
> External Hi-Z | Hi | Low |
> Device--+--+-+
> State Low | Low | Low |
> --+--+-+
>
> Signed-off-by: Glenn Miles
> ---
>
> Changes from previous version:
> - Added #define's for external state values
> - Added logic state table to commit message
> - Added cover letter
>
> hw/misc/pca9552.c | 41 ++-
> include/hw/misc/pca9552.h | 3 ++-
> 2 files changed, 38 insertions(+), 6 deletions(-)
>
> diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> index 445f56a9e8..ed814d1f98 100644
> --- a/hw/misc/pca9552.c
> +++ b/hw/misc/pca9552.c
> @@ -44,6 +44,8 @@ DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> #define PCA9552_LED_OFF 0x1
> #define PCA9552_LED_PWM0 0x2
> #define PCA9552_LED_PWM1 0x3
> +#define PCA9552_PIN_LOW 0x0
> +#define PCA9552_PIN_HIZ 0x1
>
> static const char *led_state[] = {"on", "off", "pwm0", "pwm1"};
>
> @@ -116,16 +118,22 @@ static void pca955x_update_pin_input(PCA955xState *s)
> switch (config) {
> case PCA9552_LED_ON:
> /* Pin is set to 0V to turn on LED */
> -qemu_set_irq(s->gpio[i], 0);
> +qemu_set_irq(s->gpio_out[i], 0);
pca955x_update_pin_input() is called by the external GPIO handling path
as well as the I2C command handling path. If the I2C path sets the line
low followed by the device attached to the GPIO setting the line low
then I think each change will issue an event on the outbound GPIO. Is
that desired behaviour? Does it matter?
> s->regs[input_reg] &= ~(1 << input_shift);
> break;
> case PCA9552_LED_OFF:
> /*
> * Pin is set to Hi-Z to turn off LED and
> - * pullup sets it to a logical 1.
> + * pullup sets it to a logical 1 unless
> + * external device drives it low.
> */
> -qemu_set_irq(s->gpio[i], 1);
> -s->regs[input_reg] |= 1 << input_shift;
> +if (s->ext_state[i] == PCA9552_PIN_LOW) {
> +qemu_set_irq(s->gpio_out[i], 0);
Similarly here - it might be the case that both devices have pulled the
line low and now the I2C path is releasing it. Given the external
device had already pulled the line low as well should we expect an
event to occur issued here? Should it matter?
Andrew
> +s->regs[input_reg] &= ~(1 << input_shift);
> +} else {
> +qemu_set_irq(s->gpio_out[i], 1);
> +s->regs[input_reg] |= 1 << input_shift;
> +}
> break;
> case PCA9552_LED_PWM0:
> case PCA9552_LED_PWM1:
> @@ -340,6 +348,7 @@ static const VMStateDescription pca9552_vmstate = {
> VMSTATE_UINT8(len, PCA955xState),
> VMSTATE_UINT8(pointer, PCA955xState),
> VMSTATE_UINT8_ARRAY(regs, PCA955xState, PCA955X_NR_REGS),
> +VMSTATE_UINT8_ARRAY(ext_state, PCA955xState, PCA955X_PIN_COUNT_MAX),
> VMSTATE_I2C_SLAVE(i2c, PCA955xState),
> VMSTATE_END_OF_LIST()
> }
> @@ -358,6 +367,7 @@ static void pca9552_reset(DeviceState *dev)
> s->regs[PCA9552_LS2] = 0x55;
> s->regs[PCA9552_LS3] = 0x55;
>
> +memset(s->ext_state, PCA9552_PIN_HIZ, PCA955X_PIN_COUNT_MAX);
> pca955x_update_pin_input(s);
>
> s->pointer = 0xFF;
> @@ -380,6 +390,26 @@ static void pca955x_initfn(Object *obj)
> }
> }
>
> +static void pca955x_set_ext_state(PCA955xState *s, int pin, int level)
> +{
> +if (s->ext_state[pin] != level) {
> +uint16_t pins_status = pca955x_pins_get_status(s);
> +s->ext_state[pin] = level;
> +pca955x_update_pin_input(s);
> +pca955x_display_pins_status(s, pins_status);
> +}
> +}
> +
> +static void pca955x_gpio_in_handler(void *opaque, int pin, int level)
> +{
> +
> +PCA955xState *s = PCA955X(opaque);
> +PCA955xClass *k = PCA955X_GET_CLASS(s);
> +
> +assert((pin >= 0) && (pin < k->pin_count));
> +pca955x_set_ext_state(s, pin, level);
> +}
> +
> static void pca955x_realize(DeviceState *dev, Error **errp)
> {
> PCA955xClass *k = PCA955X_GET_CLASS(dev);
> @@ -389,7 +419,8 @@ static void
Re: [PATCH 1/2] misc/pca9552: Fix inverted input status
On Thu, 2023-10-19 at 15:40 -0500, Glenn Miles wrote:
> > The pca9552 INPUT0 and INPUT1 registers are supposed to
> > hold the logical values of the LED pins. A logical 0
> > should be seen in the INPUT0/1 registers for a pin when
> > its corresponding LSn bits are set to 0, which is also
> > the state needed for turning on an LED in a typical
> > usage scenario. Existing code was doing the opposite
> > and setting INPUT0/1 bit to a 1 when the LSn bit was
> > set to 0, so this commit fixes that.
> >
> > Signed-off-by: Glenn Miles
> > ---
> >
> > Changes from prior version:
> > - Added comment regarding pca953X
> > - Added cover letter
> >
> > hw/misc/pca9552.c | 18 +-
> > tests/qtest/pca9552-test.c | 6 +++---
> > 2 files changed, 16 insertions(+), 8 deletions(-)
> >
> > diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
> > index fff19e369a..445f56a9e8 100644
> > --- a/hw/misc/pca9552.c
> > +++ b/hw/misc/pca9552.c
> > @@ -36,7 +36,10 @@ typedef struct PCA955xClass PCA955xClass;
> >
> > DECLARE_CLASS_CHECKERS(PCA955xClass, PCA955X,
> > TYPE_PCA955X)
> > -
> > +/*
> > + * Note: The LED_ON and LED_OFF configuration values for the PCA955X
> > + *chips are the reverse of the PCA953X family of chips.
> > + */
> > #define PCA9552_LED_ON 0x0
> > #define PCA9552_LED_OFF 0x1
> > #define PCA9552_LED_PWM0 0x2
> > @@ -112,13 +115,18 @@ static void pca955x_update_pin_input(PCA955xState *s)
> >
> > switch (config) {
> > case PCA9552_LED_ON:
> > -qemu_set_irq(s->gpio[i], 1);
> > -s->regs[input_reg] |= 1 << input_shift;
> > -break;
> > -case PCA9552_LED_OFF:
> > +/* Pin is set to 0V to turn on LED */
> > qemu_set_irq(s->gpio[i], 0);
> > s->regs[input_reg] &= ~(1 << input_shift);
> > break;
> > +case PCA9552_LED_OFF:
> > +/*
> > + * Pin is set to Hi-Z to turn off LED and
> > + * pullup sets it to a logical 1.
> > + */
> > + qemu_set_irq(s->gpio[i], 1);
> > +s->regs[input_reg] |= 1 << input_shift;
> > +break;
So the witherspoon-bmc machine was a user of the PCA9552 outputs as
LEDs. I guess its LEDs were in the wrong state the whole time? That
looks like the only user though, and shouldn't be negatively affected.
Reviewed-by: Andrew Jeffery
> > case PCA9552_LED_PWM0:
> > case PCA9552_LED_PWM1:
> > /* TODO */
> > diff --git a/tests/qtest/pca9552-test.c b/tests/qtest/pca9552-test.c
> > index d80ed93cd3..ccca2b3d91 100644
> > --- a/tests/qtest/pca9552-test.c
> > +++ b/tests/qtest/pca9552-test.c
> > @@ -60,7 +60,7 @@ static void send_and_receive(void *obj, void *data,
> > QGuestAllocator *alloc)
> > g_assert_cmphex(value, ==, 0x55);
> >
> > value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -g_assert_cmphex(value, ==, 0x0);
> > +g_assert_cmphex(value, ==, 0xFF);
> >
> > pca9552_init(i2cdev);
> >
> > @@ -68,13 +68,13 @@ static void send_and_receive(void *obj, void *data,
> > QGuestAllocator *alloc)
> > g_assert_cmphex(value, ==, 0x54);
> >
> > value = i2c_get8(i2cdev, PCA9552_INPUT0);
> > -g_assert_cmphex(value, ==, 0x01);
> > +g_assert_cmphex(value, ==, 0xFE);
> >
> > value = i2c_get8(i2cdev, PCA9552_LS3);
> > g_assert_cmphex(value, ==, 0x54);
> >
> > value = i2c_get8(i2cdev, PCA9552_INPUT1);
> > -g_assert_cmphex(value, ==, 0x10);
> > +g_assert_cmphex(value, ==, 0xEF);
> > }
> >
> > static void pca9552_register_nodes(void)
Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
On Mon, 25 Sep 2023, at 18:50, Cédric Le Goater wrote:
> On 9/25/23 09:54, Andrew Jeffery wrote:
>>
>>
>> On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
>>> Joel, Andrew,
>>>
>>> On 5/25/19 17:12, Cédric Le Goater wrote:
>>>> From: Joel Stanley
>>>>
>>>> The Linux kernel driver was updated in commit 4451d3f59f2a
>>>> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an
>>>> issue observed on hardware:
>>>>
>>>>> RELOAD register is loaded into COUNT register when the aspeed timer
>>>>> is enabled, which means the next event may be delayed because timer
>>>>> interrupt won't be generated until <0x - current_count +
>>>>> cycles>.
>>>>
>>>> When running under Qemu, the system appeared "laggy". The guest is now
>>>> scheduling timer events too regularly, starving the host of CPU time.
>>>>
>>>> This patch modifies the timer model to attempt to schedule the timer
>>>> expiry as the guest requests, but if we have missed the deadline we
>>>> re interrupt and try again, which allows the guest to catch up.
>>>>
>>>> Provides expected behaviour with old and new guest code.
>>>>
>>>> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model")
>>>> Signed-off-by: Joel Stanley
>>>> [clg: - merged a fix from Andrew Jeffery
>>>> "Fire interrupt on failure to meet deadline"
>>>>
>>>> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html
>>>> - adapted commit log
>>>> - checkpatch fixes ]
>>>> Signed-off-by: Cédric Le Goater
>>>> ---
>>>>hw/timer/aspeed_timer.c | 59 ++---
>>>>1 file changed, 31 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>>>> index 5c786e512815..9ffd8e09f670 100644
>>>> --- a/hw/timer/aspeed_timer.c
>>>> +++ b/hw/timer/aspeed_timer.c
>>>> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct
>>>> AspeedTimer *t, uint32_t ticks)
>>>>
>>>>static uint64_t calculate_next(struct AspeedTimer *t)
>>>>{
>>>> -uint64_t next = 0;
>>>> -uint32_t rate = calculate_rate(t);
>>>> +uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> +uint64_t next;
>>>>
>>>> -while (!next) {
>>>> -/* We don't know the relationship between the values in the match
>>>> - * registers, so sort using MAX/MIN/zero. We sort in that order
>>>> as the
>>>> - * timer counts down to zero. */
>>>> -uint64_t seq[] = {
>>>> -calculate_time(t, MAX(t->match[0], t->match[1])),
>>>> -calculate_time(t, MIN(t->match[0], t->match[1])),
>>>> -calculate_time(t, 0),
>>>> -};
>>>> -uint64_t reload_ns;
>>>> -uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> -
>>>> -if (now < seq[0]) {
>>>> -next = seq[0];
>>>> -} else if (now < seq[1]) {
>>>> -next = seq[1];
>>>> -} else if (now < seq[2]) {
>>>> -next = seq[2];
>>>> -} else if (t->reload) {
>>>> -reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>>>> -t->start = now - ((now - t->start) % reload_ns);
>>>> -} else {
>>>> -/* no reload value, return 0 */
>>>> -break;
>>>> -}
>>>> +/*
>>>> + * We don't know the relationship between the values in the match
>>>> + * registers, so sort using MAX/MIN/zero. We sort in that order as
>>>> + * the timer counts down to zero.
>>>> + */
>>>> +
>>>> +next = calculate_time(t, MAX(t->match[0], t->match[1]));
>>>> +if (now < next) {
>>>> +return next;
>>>> +}
>>>> +
>>>> +next = calculate_time(t, MIN(t->match[0], t->match[1]));
>>>> +if (now < next) {
>>>> +return next;
>>>> +}
>>>> +
>>>> +next = calculate_time(t, 0);
>>>> +if (now < next) {
>>>> +return next;
>>>> +}
>>>> +
>>>> +/* We've missed all deadlines, fire interrupt and try again */
>>>> +timer_del(>timer);
>>>> +
>>>> +if (timer_overflow_interrupt(t)) {
>>>> +t->level = !t->level;
>>>> +qemu_set_irq(t->irq, t->level);
>>>>}
>>>>
>>>> -return next;
>>>> +t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>>>> +return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>>>
>>> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it
>>> comes
>>> from ? Thanks,
>>
>> The inner MAX() deals with the lack of ordering constraints between the
>> match values. I think the outer MAX() is redundant. We should probably
>> remove it. The match member is type uint32_t so it can't be negative. You
>> did steal that from an RFC patch :D
>
> I did ! Fixed there :
>
>
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20230922155924.1172019-5-...@kaod.org/
>
Thanks. That one might be further down in my review queue
Andrew
Re: [PATCH] MAINTAINERS: aspeed: Update Andrew's email address
On Mon, 25 Sep 2023, at 17:28, Cédric Le Goater wrote: > On 9/25/23 08:22, Andrew Jeffery wrote: >> I've changed employers, have company email that deals with patch-based >> workflows without too much of a headache, and am trying to steer some >> content out of my personal mail. >> >> Signed-off-by: Andrew Jeffery >> --- >> >> Hi Cédric, do you mind including this in your Aspeed queue? > > Sure. It's on aspeed-8.2 for now. I will send a PR when I have more > material for QEMU 8.2 > Thanks!
Re: [Qemu-devel] [PATCH 08/19] aspeed/timer: Fix behaviour running Linux
On Fri, 22 Sep 2023, at 22:51, Cédric Le Goater wrote:
> Joel, Andrew,
>
> On 5/25/19 17:12, Cédric Le Goater wrote:
>> From: Joel Stanley
>>
>> The Linux kernel driver was updated in commit 4451d3f59f2a
>> ("clocksource/drivers/fttmr010: Fix set_next_event handler) to fix an
>> issue observed on hardware:
>>
>> > RELOAD register is loaded into COUNT register when the aspeed timer
>> > is enabled, which means the next event may be delayed because timer
>> > interrupt won't be generated until <0x - current_count +
>> > cycles>.
>>
>> When running under Qemu, the system appeared "laggy". The guest is now
>> scheduling timer events too regularly, starving the host of CPU time.
>>
>> This patch modifies the timer model to attempt to schedule the timer
>> expiry as the guest requests, but if we have missed the deadline we
>> re interrupt and try again, which allows the guest to catch up.
>>
>> Provides expected behaviour with old and new guest code.
>>
>> Fixes: c04bd47db6b9 ("hw/timer: Add ASPEED timer device model")
>> Signed-off-by: Joel Stanley
>> [clg: - merged a fix from Andrew Jeffery
>> "Fire interrupt on failure to meet deadline"
>> https://lists.ozlabs.org/pipermail/openbmc/2019-January/014641.html
>>- adapted commit log
>>- checkpatch fixes ]
>> Signed-off-by: Cédric Le Goater
>> ---
>> hw/timer/aspeed_timer.c | 59 ++---
>> 1 file changed, 31 insertions(+), 28 deletions(-)
>>
>> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
>> index 5c786e512815..9ffd8e09f670 100644
>> --- a/hw/timer/aspeed_timer.c
>> +++ b/hw/timer/aspeed_timer.c
>> @@ -109,37 +109,40 @@ static inline uint64_t calculate_time(struct
>> AspeedTimer *t, uint32_t ticks)
>>
>> static uint64_t calculate_next(struct AspeedTimer *t)
>> {
>> -uint64_t next = 0;
>> -uint32_t rate = calculate_rate(t);
>> +uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +uint64_t next;
>>
>> -while (!next) {
>> -/* We don't know the relationship between the values in the match
>> - * registers, so sort using MAX/MIN/zero. We sort in that order as
>> the
>> - * timer counts down to zero. */
>> -uint64_t seq[] = {
>> -calculate_time(t, MAX(t->match[0], t->match[1])),
>> -calculate_time(t, MIN(t->match[0], t->match[1])),
>> -calculate_time(t, 0),
>> -};
>> -uint64_t reload_ns;
>> -uint64_t now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> -
>> -if (now < seq[0]) {
>> -next = seq[0];
>> -} else if (now < seq[1]) {
>> -next = seq[1];
>> -} else if (now < seq[2]) {
>> -next = seq[2];
>> -} else if (t->reload) {
>> -reload_ns = muldiv64(t->reload, NANOSECONDS_PER_SECOND, rate);
>> -t->start = now - ((now - t->start) % reload_ns);
>> -} else {
>> -/* no reload value, return 0 */
>> -break;
>> -}
>> +/*
>> + * We don't know the relationship between the values in the match
>> + * registers, so sort using MAX/MIN/zero. We sort in that order as
>> + * the timer counts down to zero.
>> + */
>> +
>> +next = calculate_time(t, MAX(t->match[0], t->match[1]));
>> +if (now < next) {
>> +return next;
>> +}
>> +
>> +next = calculate_time(t, MIN(t->match[0], t->match[1]));
>> +if (now < next) {
>> +return next;
>> +}
>> +
>> +next = calculate_time(t, 0);
>> +if (now < next) {
>> +return next;
>> +}
>> +
>> +/* We've missed all deadlines, fire interrupt and try again */
>> +timer_del(>timer);
>> +
>> +if (timer_overflow_interrupt(t)) {
>> +t->level = !t->level;
>> +qemu_set_irq(t->irq, t->level);
>> }
>>
>> -return next;
>> +t->start = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
>> +return calculate_time(t, MAX(MAX(t->match[0], t->match[1]), 0));
>
> This MAX(MAX(x, y), 0) looks strange to me. Would you remember where it comes
> from ? Thanks,
The inner MAX() deals with the lack of ordering constraints between the match
values. I think the outer MAX() is redundant. We should probably remove it. The
match member is type uint32_t so it can't be negative. You did steal that from
an RFC patch :D
Andrew
Re: [PATCH 3/4] aspeed/i3c: Rename variable shadowing a local
On Sat, 23 Sep 2023, at 03:58, Philippe Mathieu-Daudé wrote:
> On 22/9/23 17:59, Cédric Le Goater wrote:
>> to fix warning :
>>
>>../hw/i3c/aspeed_i3c.c: In function ‘aspeed_i3c_realize’:
>>../hw/i3c/aspeed_i3c.c:1959:17: warning: declaration of ‘dev’ shadows a
>> parameter [-Wshadow=local]
>> 1959 | Object *dev = OBJECT(>devices[i]);
>> | ^~~
>>../hw/i3c/aspeed_i3c.c:1942:45: note: shadowed declaration is here
>> 1942 | static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
>> |~^~~
>>
>> Signed-off-by: Cédric Le Goater
>> ---
>> hw/misc/aspeed_i3c.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/misc/aspeed_i3c.c b/hw/misc/aspeed_i3c.c
>> index f54f5da522b3..d1ff61767167 100644
>> --- a/hw/misc/aspeed_i3c.c
>> +++ b/hw/misc/aspeed_i3c.c
>> @@ -296,13 +296,13 @@ static void aspeed_i3c_realize(DeviceState *dev, Error
>> **errp)
>
> Alternatively:
>
> -- >8 --
>
> -static void aspeed_i3c_realize(DeviceState *dev, Error **errp)
> +static void aspeed_i3c_realize(DeviceState *ctrl, Error **errp)
> {
> int i;
> -AspeedI3CState *s = ASPEED_I3C(dev);
> -SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
> +AspeedI3CState *s = ASPEED_I3C(ctrl);
> +SysBusDevice *sbd = SYS_BUS_DEVICE(ctrl);
Hmm, I feel like `dev` isn't an unreasonable or unusual name for the formal
parameter? Switching to `ctrl` isn't my immediate preference.
>
> ---
>
>> memory_region_add_subregion(>iomem_container, 0x0, >iomem);
>>
>> for (i = 0; i < ASPEED_I3C_NR_DEVICES; ++i) {
>> -Object *dev = OBJECT(>devices[i]);
>> +Object *i3c_dev = OBJECT(>devices[i]);
Maybe `s/i3c_dev/subdev`? I dunno. That bikeshed isn't gonna get painted on its
own.
Reviewed-by: Andrew Jeffery
>>
>> -if (!object_property_set_uint(dev, "device-id", i, errp)) {
>> +if (!object_property_set_uint(i3c_dev, "device-id", i, errp)) {
>> return;
>> }
>>
>> -if (!sysbus_realize(SYS_BUS_DEVICE(dev), errp)) {
>> +if (!sysbus_realize(SYS_BUS_DEVICE(i3c_dev), errp)) {
>> return;
>> }
>>
>
> Reviewed-by: Philippe Mathieu-Daudé
[PATCH] MAINTAINERS: aspeed: Update Andrew's email address
I've changed employers, have company email that deals with patch-based workflows without too much of a headache, and am trying to steer some content out of my personal mail. Signed-off-by: Andrew Jeffery --- Hi Cédric, do you mind including this in your Aspeed queue? MAINTAINERS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 355b1960ce46..b142c09628b9 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1108,7 +1108,7 @@ F: docs/system/arm/emcraft-sf2.rst ASPEED BMCs M: Cédric Le Goater M: Peter Maydell -R: Andrew Jeffery +R: Andrew Jeffery R: Joel Stanley L: qemu-...@nongnu.org S: Maintained -- 2.39.2
Re: [PATCH v6 1/3] hw/i2c: add smbus pec utility function
On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Add i2c_smbus_pec() to calculate the SMBus Packet Error Code for a
> message.
>
> Reviewed-by: Jonathan Cameron
> Signed-off-by: Klaus Jensen
It at least looks a lot like the linux implementation :)
Reviewed-by: Andrew Jeffery
> ---
> hw/i2c/smbus_master.c | 26 ++
> include/hw/i2c/smbus_master.h | 2 ++
> 2 files changed, 28 insertions(+)
>
> diff --git a/hw/i2c/smbus_master.c b/hw/i2c/smbus_master.c
> index 6a53c34e70b7..01a8e4700222 100644
> --- a/hw/i2c/smbus_master.c
> +++ b/hw/i2c/smbus_master.c
> @@ -15,6 +15,32 @@
> #include "hw/i2c/i2c.h"
> #include "hw/i2c/smbus_master.h"
>
> +static uint8_t crc8(uint16_t data)
> +{
> +int i;
> +
> +for (i = 0; i < 8; i++) {
> +if (data & 0x8000) {
> +data ^= 0x1070U << 3;
> +}
> +
> +data <<= 1;
> +}
> +
> +return (uint8_t)(data >> 8);
> +}
> +
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len)
> +{
> +int i;
> +
> +for (i = 0; i < len; i++) {
> +crc = crc8((crc ^ buf[i]) << 8);
> +}
> +
> +return crc;
> +}
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read)
> {
> diff --git a/include/hw/i2c/smbus_master.h b/include/hw/i2c/smbus_master.h
> index bb13bc423c22..d90f81767d86 100644
> --- a/include/hw/i2c/smbus_master.h
> +++ b/include/hw/i2c/smbus_master.h
> @@ -27,6 +27,8 @@
>
> #include "hw/i2c/i2c.h"
>
> +uint8_t i2c_smbus_pec(uint8_t crc, uint8_t *buf, size_t len);
> +
> /* Master device commands. */
> int smbus_quick_command(I2CBus *bus, uint8_t addr, int read);
> int smbus_receive_byte(I2CBus *bus, uint8_t addr);
>
Re: [PATCH v6 2/3] hw/i2c: add mctp core
On Thu, 2023-09-14 at 11:53 +0200, Klaus Jensen wrote: > From: Klaus Jensen > > Add an abstract MCTP over I2C endpoint model. This implements MCTP > control message handling as well as handling the actual I2C transport > (packetization). > > Devices are intended to derive from this and implement the class > methods. > > Parts of this implementation is inspired by code[1] previously posted by > Jonathan Cameron. > > Squashed a fix[2] from Matt Johnston. > > [1]: > https://lore.kernel.org/qemu-devel/20220520170128.4436-1-jonathan.came...@huawei.com/ > [2]: > https://lore.kernel.org/qemu-devel/20221121080445.ga29...@codeconstruct.com.au/ > > Tested-by: Jonathan Cameron > Reviewed-by: Jonathan Cameron > Signed-off-by: Klaus Jensen Nice! Reviewed-by: Andrew Jeffery
Re: [PATCH v6 3/3] hw/nvme: add nvme management interface model
+switch (request->opc) {
> +case NMI_CMD_READ_NMI_DS:
> +nmi_handle_mi_read_nmi_ds(nmi, request);
> +break;
> +
> +case NMI_CMD_CONFIGURATION_GET:
> +nmi_handle_mi_config_get(nmi, request);
> +break;
> +
> +default:
> +nmi_set_parameter_error(nmi, 0x0, 0x0);
> +fprintf(stderr, "nmi command 0x%x not handled\n", request->opc);
> +
> +break;
> +}
> +}
> +
> +static void nmi_reset(MCTPI2CEndpoint *mctp)
> +{
> +NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
> +nmi->len = 0;
> +}
> +
> +static void nmi_handle(MCTPI2CEndpoint *mctp)
> +{
> +NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
> +NMIMessage *msg = (NMIMessage *)nmi->buffer;
> +uint32_t crc;
> +uint8_t nmimt;
> +
> +const uint8_t buf[] = {
> +msg->mctpd,
> +FIELD_DP8(msg->nmp, NMI_NMP, ROR, 1),
> +0x0, 0x0,
> +};
> +
> +if (FIELD_EX8(msg->mctpd, NMI_MCTPD, MT) != NMI_MCTPD_MT_NMI) {
> +goto drop;
> +}
> +
> +if (FIELD_EX8(msg->mctpd, NMI_MCTPD, IC) != NMI_MCTPD_IC_ENABLED) {
> +goto drop;
> +}
> +
> +nmi->pos = 0;
> +nmi_scratch_append(nmi, buf, sizeof(buf));
> +
> +nmimt = FIELD_EX8(msg->nmp, NMI_NMP, NMIMT);
> +
> +trace_nmi_handle_msg(nmimt);
> +
> +switch (nmimt) {
> +case NMI_NMP_NMIMT_NVME_MI:
> +nmi_handle_mi(nmi, msg);
> +break;
> +
> +default:
> +fprintf(stderr, "nmi message type 0x%x not handled\n", nmimt);
> +
> +nmi_set_error(nmi, 0x3);
0x3 is Invalid Command Opcode? I feel it would be be helpful to give it
a name.
Otherwise, this looks good to me. I realise I haven't contributed much
upstream recently, but FWIW:
Reviewed-by: Andrew Jeffery
> +
> +break;
> +}
> +
> +crc = crc32c(0x, nmi->scratch, nmi->pos);
> +nmi_scratch_append(nmi, , sizeof(crc));
> +
> +nmi->len = nmi->pos;
> +nmi->pos = 0;
> +
> +i2c_mctp_schedule_send(mctp);
> +
> +return;
> +
> +drop:
> +nmi_reset(mctp);
> +}
> +
> +static size_t nmi_get_buf(MCTPI2CEndpoint *mctp, const uint8_t **buf,
> + size_t maxlen, uint8_t *mctp_flags)
> +{
> +NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
> +size_t len;
> +
> +len = MIN(maxlen, nmi->len - nmi->pos);
> +
> +if (len == 0) {
> +return 0;
> +}
> +
> +if (nmi->pos == 0) {
> +*mctp_flags = FIELD_DP8(*mctp_flags, MCTP_H_FLAGS, SOM, 1);
> +}
> +
> +*buf = nmi->scratch + nmi->pos;
> +nmi->pos += len;
> +
> +if (nmi->pos == nmi->len) {
> +*mctp_flags = FIELD_DP8(*mctp_flags, MCTP_H_FLAGS, EOM, 1);
> +
> +nmi->pos = nmi->len = 0;
> +}
> +
> +return len;
> +}
> +
> +static int nmi_put_buf(MCTPI2CEndpoint *mctp, uint8_t *buf, size_t len)
> +{
> +NMIDevice *nmi = NMI_I2C_DEVICE(mctp);
> +
> +if (nmi->len + len > NMI_MAX_MESSAGE_LENGTH) {
> +return -1;
> +}
> +
> +memcpy(nmi->buffer + nmi->len, buf, len);
> +nmi->len += len;
> +
> +return 0;
> +}
> +
> +static size_t nmi_get_types(MCTPI2CEndpoint *mctp, const uint8_t **data)
> +{
> +/**
> + * DSP0236 1.3.0, Table 19.
> + *
> + * This only includes message types that are supported *in addition* to
> the
> + * MCTP control message type.
> + */
> +static const uint8_t buf[] = {
> +0x0,/* success */
> +0x1,/* number of message types in list (supported) */
> +NMI_MCTPD_MT_NMI,
> +};
> +
> +*data = buf;
> +
> +return sizeof(buf);
> +}
> +
> +static void nvme_mi_class_init(ObjectClass *oc, void *data)
> +{
> +MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_CLASS(oc);
> +
> +mc->get_types = nmi_get_types;
> +
> +mc->get_buf = nmi_get_buf;
> +mc->put_buf = nmi_put_buf;
> +
> +mc->handle = nmi_handle;
> +mc->reset = nmi_reset;
> +}
> +
> +static const TypeInfo nvme_mi = {
> +.name = TYPE_NMI_I2C_DEVICE,
> +.parent = TYPE_MCTP_I2C_ENDPOINT,
> +.instance_size = sizeof(NMIDevice),
> +.class_init = nvme_mi_class_init,
> +};
> +
> +static void register_types(void)
> +{
> +type_register_static(_mi);
> +}
> +
> +type_init(register_types);
> diff --git a/hw/nvme/trace-events b/hw/nvme/trace-events
> index 3a67680c6ad1..41e2c540dcf2 100644
> --- a/hw/nvme/trace-events
> +++ b/hw/nvme/trace-events
> @@ -216,3 +216,9 @@ pci_nvme_ub_db_wr_invalid_sq(uint32_t qid) "submission
> queue doorbell write for
> pci_nvme_ub_db_wr_invalid_sqtail(uint32_t qid, uint16_t new_tail)
> "submission queue doorbell write value beyond queue size, sqid=%"PRIu32",
> new_head=%"PRIu16", ignoring"
> pci_nvme_ub_unknown_css_value(void) "unknown value in cc.css field"
> pci_nvme_ub_too_many_mappings(void) "too many prp/sgl mappings"
> +
> +# nmi-i2c
> +nmi_handle_mi_read_nmi_ds(uint8_t dtyp) "dtyp 0x%"PRIx8""
> +nmi_handle_mi_config_get(uint8_t identifier) "identifier 0x%"PRIx8""
> +nmi_handle_mi(uint8_t opc) "opc 0x%"PRIx8""
> +nmi_handle_msg(uint8_t nmint) "nmint 0x%"PRIx8""
>
[PATCH] eeprom_at24c: Model 8-bit data addressing for 16-bit devices
It appears some (many?) EEPROMs that implement 16-bit data addressing
will accept an 8-bit address and clock out non-uniform data for the
read. This behaviour is exploited by an EEPROM detection routine in part
of OpenBMC userspace with a reasonably broad user base:
https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229
The diversity of the set of EEPROMs that it operates against is unclear,
but this code has been around for a while now.
Separately, The NVM Express Management Interface Specification dictates
the provided behaviour in section 8.2 Vital Product Data:
> If only one byte of the Command Offset is provided by the Management
> Controller, then the least significant byte of the internal offset
> shall be set to that value and the most-significant byte of the
> internal offset shall be cleared to 0h
https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf
This change makes it possible to expose NVMe VPD in a manner that can be
dynamically detected by OpenBMC.
Signed-off-by: Andrew Jeffery
---
hw/nvram/eeprom_at24c.c | 18 +-
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/hw/nvram/eeprom_at24c.c b/hw/nvram/eeprom_at24c.c
index 613c4929e327..64a61cc0e468 100644
--- a/hw/nvram/eeprom_at24c.c
+++ b/hw/nvram/eeprom_at24c.c
@@ -98,12 +98,20 @@ uint8_t at24c_eeprom_recv(I2CSlave *s)
EEPROMState *ee = AT24C_EE(s);
uint8_t ret;
-/*
- * If got the byte address but not completely with address size
- * will return the invalid value
- */
if (ee->haveaddr > 0 && ee->haveaddr < ee->asize) {
-return 0xff;
+/*
+ * Provide behaviour that aligns with NVMe MI 1.2c, section 8.2.
+ *
+ *
https://nvmexpress.org/wp-content/uploads/NVM-Express-Management-Interface-Specification-1.2c-2022.10.06-Ratified.pdf
+ *
+ * Otherwise, the clocked-out data is meaningless anyway, and so
reading
+ * off memory is as good a behaviour as anything. This also happens to
+ * help the address-width detection heuristic in OpenBMC's userspace.
+ *
+ *
https://github.com/openbmc/entity-manager/blob/0422a24bb6033605ce75479f675fedc76abb1167/src/fru_device.cpp#L197-L229
+ */
+ee->haveaddr = ee->asize;
+ee->cur %= ee->rsize;
}
ret = ee->mem[ee->cur];
--
2.39.2
Re: [PATCH v5 3/3] hw/nvme: add nvme management interface model
Hi Klaus,
On Thu, 2023-09-14 at 08:51 +0200, Klaus Jensen wrote:
> On Sep 12 13:50, Andrew Jeffery wrote:
> > Hi Klaus,
> >
> > On Tue, 2023-09-05 at 10:38 +0200, Klaus Jensen wrote:
> > > >
> > > > +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest
> > > > *request)
> > > > +{
> > > > + uint32_t dw0 = le32_to_cpu(request->dw0);
> > > > + uint8_t identifier = FIELD_EX32(dw0,
> > > > NMI_CMD_CONFIGURATION_GET_DW0,
> > > > + IDENTIFIER);
> > > > + const uint8_t *buf;
> > > > +
> > > > + static const uint8_t smbus_freq[4] = {
> > > > + 0x00, /* success */
> > > > + 0x01, 0x00, 0x00, /* 100 kHz */
> > > > + };
> > > > +
> > > > + static const uint8_t mtu[4] = {
> > > > + 0x00, /* success */
> > > > + 0x40, 0x00, /* 64 */
> > > > + 0x00, /* reserved */
> > > > + };
> > > > +
> > > > + trace_nmi_handle_mi_config_get(identifier);
> > > > +
> > > > + switch (identifier) {
> > > > + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> > > > + buf = smbus_freq;
> > > > + break;
> > > > +
> > > > + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> > > > + buf = mtu;
> > > > + break;
> > > > +
> > > > + default:
> > > > + nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest,
> > > > dw0));
> > > > + return;
> > > > + }
> > > > +
> > > > + nmi_scratch_append(nmi, buf, sizeof(buf));
> > > > +}
> >
> > When I tried to build this patch I got:
> >
> > ```
> > In file included from /usr/include/string.h:535,
> > from
> > /home/andrew/src/qemu.org/qemu/include/qemu/osdep.h:112,
> > from ../hw/nvme/nmi-i2c.c:12:
> > In function ‘memcpy’,
> > inlined from ‘nmi_scratch_append’ at ../hw/nvme/nmi-i2c.c:80:5,
> > inlined from ‘nmi_handle_mi_config_get’ at ../hw/nvme/nmi-i2c.c:246:5,
> > inlined from ‘nmi_handle_mi’ at ../hw/nvme/nmi-i2c.c:266:9,
> > inlined from ‘nmi_handle’ at ../hw/nvme/nmi-i2c.c:313:9:
> > /usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error:
> > ‘__builtin_memcpy’ forming offset [4, 7] is out of the bounds [0, 4]
> > [-Werror=array-bounds=]
> >29 | return __builtin___memcpy_chk (__dest, __src, __len,
> > | ^
> >30 | __glibc_objsize0 (__dest));
> > | ~~
> > ```
> >
> > It wasn't clear initially from the error that the source of the problem
> > was the size associated with the source buffer, especially as there is
> > some pointer arithmetic being done to derive `__dest`.
> >
> > Anyway, what we're trying to express is that the size to copy from buf
> > is the size of the array pointed to by buf. However, buf is declared as
> > a pointer to uint8_t, which loses the length information. To fix that I
> > think we need:
> >
> > - const uint8_t *buf;
> > + const uint8_t (*buf)[4];
> >
> > and then:
> >
> > - nmi_scratch_append(nmi, buf, sizeof(buf));
> > + nmi_scratch_append(nmi, buf, sizeof(*buf));
> >
> > Andrew
> >
>
> Hi Andrew,
>
> Nice (and important) catch! Just curious, are you massaging QEMU's build
> system into adding additional checks or how did your compiler catch
> this?
No tricks to be honest, I just applied your patches on top of
9ef497755afc ("Merge tag 'pull-vfio-20230911' of
https://github.com/legoater/qemu into staging") using `b4 shazam ...`.
I'm building on Debian Bookworm:
$ gcc --version | head -n1
gcc (Debian 12.2.0-14) 12.2.0
Andrew
Re: [PATCH v5 3/3] hw/nvme: add nvme management interface model
Hi Klaus,
On Tue, 2023-09-05 at 10:38 +0200, Klaus Jensen wrote:
> >
> > +static void nmi_handle_mi_config_get(NMIDevice *nmi, NMIRequest
> > *request)
> > +{
> > + uint32_t dw0 = le32_to_cpu(request->dw0);
> > + uint8_t identifier = FIELD_EX32(dw0,
> > NMI_CMD_CONFIGURATION_GET_DW0,
> > + IDENTIFIER);
> > + const uint8_t *buf;
> > +
> > + static const uint8_t smbus_freq[4] = {
> > + 0x00, /* success */
> > + 0x01, 0x00, 0x00, /* 100 kHz */
> > + };
> > +
> > + static const uint8_t mtu[4] = {
> > + 0x00, /* success */
> > + 0x40, 0x00, /* 64 */
> > + 0x00, /* reserved */
> > + };
> > +
> > + trace_nmi_handle_mi_config_get(identifier);
> > +
> > + switch (identifier) {
> > + case NMI_CMD_CONFIGURATION_GET_SMBUS_FREQ:
> > + buf = smbus_freq;
> > + break;
> > +
> > + case NMI_CMD_CONFIGURATION_GET_MCTP_TRANSMISSION_UNIT:
> > + buf = mtu;
> > + break;
> > +
> > + default:
> > + nmi_set_parameter_error(nmi, 0x0, offsetof(NMIRequest,
> > dw0));
> > + return;
> > + }
> > +
> > + nmi_scratch_append(nmi, buf, sizeof(buf));
> > +}
When I tried to build this patch I got:
```
In file included from /usr/include/string.h:535,
from /home/andrew/src/qemu.org/qemu/include/qemu/osdep.h:112,
from ../hw/nvme/nmi-i2c.c:12:
In function ‘memcpy’,
inlined from ‘nmi_scratch_append’ at ../hw/nvme/nmi-i2c.c:80:5,
inlined from ‘nmi_handle_mi_config_get’ at ../hw/nvme/nmi-i2c.c:246:5,
inlined from ‘nmi_handle_mi’ at ../hw/nvme/nmi-i2c.c:266:9,
inlined from ‘nmi_handle’ at ../hw/nvme/nmi-i2c.c:313:9:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:10: error:
‘__builtin_memcpy’ forming offset [4, 7] is out of the bounds [0, 4]
[-Werror=array-bounds=]
29 | return __builtin___memcpy_chk (__dest, __src, __len,
| ^
30 | __glibc_objsize0 (__dest));
| ~~
```
It wasn't clear initially from the error that the source of the problem
was the size associated with the source buffer, especially as there is
some pointer arithmetic being done to derive `__dest`.
Anyway, what we're trying to express is that the size to copy from buf
is the size of the array pointed to by buf. However, buf is declared as
a pointer to uint8_t, which loses the length information. To fix that I
think we need:
- const uint8_t *buf;
+ const uint8_t (*buf)[4];
and then:
- nmi_scratch_append(nmi, buf, sizeof(buf));
+ nmi_scratch_append(nmi, buf, sizeof(*buf));
Andrew
Re: [PATCH v3 1/1] hw/arm/aspeed:Add vpd data for Rainier machine
On Wed, 24 May 2023, at 17:14, Joel Stanley wrote: > On Wed, 24 May 2023 at 06:38, Cédric Le Goater wrote: >> >> But, I also got this : >> >>root@p10bmc:~# [ 91.656331] watchdog: watchdog0: watchdog did not stop! >>[ 91.734858] systemd-shutdown[1]: Using hardware watchdog 'aspeed_wdt', >> version 0, device /dev/watchdog0 >>[ 91.735766] systemd-shutdown[1]: Watchdog running with a timeout of >> 1min. >>[ 91.987363] systemd-shutdown[1]: Syncing filesystems and block devices. >>[ 93.471897] systemd-shutdown[1]: Sending SIGTERM to remaining >> processes... >> >> and the machine rebooted. >> >> Joel had the same problem : >> >>https://github.com/openbmc/qemu/issues/39 >> >> Is it unrelated ? I haven't started a rainier in 2 years at least so I can >> not tell. > > I don't think it's related to Ninad's patches. > > I am able to reproduce the issue on my old Skylake x86 machine, but it > doesn't happen on my M1 mac mini. > > I suspect the emulation is moving too slowly, but the host's wall > clock is still ticking, so all of a sudden the BMC finds out that time > has passed an the watchdog bites. I could be wrong. Yeah, I also suspect this is occurring. In some cases we see output like: ``` [ 295.474921] hrtimer: interrupt took 411679950 ns Watchdog timer 0 expired. ``` This suggests that we've exhausted the three iterations of the hrtimer callback list from the hrtimer interrupt[1]. Too much stuff happening for the time-keeping we have. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/time/hrtimer.c?h=v6.3#n1869 However, regarding Cédric's log above, a reboot is expected on the first boot of a fresh image when there's valid VPD available. For the first boot of a fresh image we configure the kernel with a minimal devicetree that allows us to read the VPD data. This determines the system we're actually on and configures an appropriate devicetree for subsequent boots. We then reboot to pick up the new devicetree. The devicetree to use is stored in the u-boot environment (in `fitconfig`). Andrew
[PATCH 0/2] linux-user: Clarify error on failure to map guest address space
Hello, This series is a couple of trivial improvements to the error message from linux-user's ELF loader when it fails to mmap() the guest's address space. Both issues caused me brief confusion when trying to sort myself out after hitting https://gitlab.com/qemu-project/qemu/-/issues/447 I've build tested the two as a sanity check. Cheers, Andrew Andrew Jeffery (2): linux-user: elfload: s/min_mmap_addr/mmap_min_addr/ linux-user: elfload: Specify -R is an option for qemu-user binaries linux-user/elfload.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) -- 2.39.2
[PATCH 1/2] linux-user: elfload: s/min_mmap_addr/mmap_min_addr/
As-is the error message can cause some confusion as the mentioned sysctl
attribute name is wrong:
https://www.kernel.org/doc/html/latest/admin-guide/sysctl/vm.html#mmap-min-addr
Signed-off-by: Andrew Jeffery
---
linux-user/elfload.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 1dbc1f0f9baa..601b156b476b 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2771,7 +2771,7 @@ static void pgb_reserved_va(const char *image_name,
abi_ulong guest_loaddr,
if (addr == MAP_FAILED || addr != test) {
error_report("Unable to reserve 0x%lx bytes of virtual address "
"space at %p (%s) for use as guest address space (check
your "
- "virtual memory ulimit setting, min_mmap_addr or reserve
less "
+ "virtual memory ulimit setting, mmap_min_addr or reserve
less "
"using -R option)", reserved_va, test, strerror(errno));
exit(EXIT_FAILURE);
}
--
2.39.2
[PATCH 2/2] linux-user: elfload: Specify -R is an option for qemu-user binaries
Given several different concepts are suggested for investigation, let's
not confuse e.g. ulimit's -R with what was actually intended.
Signed-off-by: Andrew Jeffery
---
linux-user/elfload.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index 601b156b476b..694794f97202 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -2772,7 +2772,8 @@ static void pgb_reserved_va(const char *image_name,
abi_ulong guest_loaddr,
error_report("Unable to reserve 0x%lx bytes of virtual address "
"space at %p (%s) for use as guest address space (check
your "
"virtual memory ulimit setting, mmap_min_addr or reserve
less "
- "using -R option)", reserved_va, test, strerror(errno));
+ "using qemu-user's -R option)",
+ reserved_va, test, strerror(errno));
exit(EXIT_FAILURE);
}
--
2.39.2
Re: [PATCH] hw/misc: Add basic Aspeed GFX model
On Thu, 19 Jan 2023, at 23:14, Joel Stanley wrote:
> Enough model to capture the pinmux writes to enable correct operation of
> the parts of pinmux that depend on GFX registers.
>
> Signed-off-by: Joel Stanley
> ---
> include/hw/arm/aspeed_soc.h | 3 +
> include/hw/misc/aspeed_gfx.h | 31 +
> hw/arm/aspeed_ast2600.c | 11
> hw/arm/aspeed_soc.c | 12
> hw/misc/aspeed_gfx.c | 121 +++
> hw/misc/meson.build | 1 +
> hw/misc/trace-events | 4 ++
> 7 files changed, 183 insertions(+)
> create mode 100644 include/hw/misc/aspeed_gfx.h
> create mode 100644 hw/misc/aspeed_gfx.c
>
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 8389200b2d01..7084e0efeb97 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -26,6 +26,7 @@
> #include "hw/ssi/aspeed_smc.h"
> #include "hw/misc/aspeed_hace.h"
> #include "hw/misc/aspeed_sbc.h"
> +#include "hw/misc/aspeed_gfx.h"
> #include "hw/watchdog/wdt_aspeed.h"
> #include "hw/net/ftgmac100.h"
> #include "target/arm/cpu.h"
> @@ -81,6 +82,7 @@ struct AspeedSoCState {
> AspeedSDHCIState emmc;
> AspeedLPCState lpc;
> AspeedPECIState peci;
> +AspeedGFXState gfx;
> SerialMM uart[ASPEED_UARTS_NUM];
> Clock *sysclk;
> UnimplementedDeviceState iomem;
> @@ -171,6 +173,7 @@ enum {
> ASPEED_DEV_EMMC,
> ASPEED_DEV_KCS,
> ASPEED_DEV_HACE,
> +ASPEED_DEV_GFX,
> ASPEED_DEV_DPMCU,
> ASPEED_DEV_DP,
> ASPEED_DEV_I3C,
> diff --git a/include/hw/misc/aspeed_gfx.h b/include/hw/misc/aspeed_gfx.h
> new file mode 100644
> index ..b0736a53f577
> --- /dev/null
> +++ b/include/hw/misc/aspeed_gfx.h
> @@ -0,0 +1,31 @@
> +/*
> + * ASPEED GFX Controller
> + *
> + * Copyright (C) 2023 IBM Corp.
> + *
> + * This code is licensed under the GPL version 2 or later. See
> + * the COPYING file in the top-level directory.
> + */
Use SPDX here?
> +
> +#ifndef ASPEED_GFX_H
> +#define ASPEED_GFX_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_GFX "aspeed.gfx"
> +#define ASPEED_GFX(obj) OBJECT_CHECK(AspeedGFXState, (obj), TYPE_ASPEED_GFX)
> +
> +#define ASPEED_GFX_NR_REGS (0xFC >> 2)
> +
> +typedef struct AspeedGFXState {
> +/* */
> +SysBusDevice parent;
> +
> +/*< public >*/
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_GFX_NR_REGS];
> +} AspeedGFXState;
> +
> +#endif /* _ASPEED_GFX_H_ */
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index cd75465c2bdd..10e4a13655cc 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -43,6 +43,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> [ASPEED_DEV_HACE] = 0x1E6D,
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU] = 0x1E6E2000,
> +[ASPEED_DEV_GFX] = 0x1E6E6000,
> [ASPEED_DEV_XDMA] = 0x1E6E7000,
> [ASPEED_DEV_ADC] = 0x1E6E9000,
> [ASPEED_DEV_DP]= 0x1E6EB000,
> @@ -255,6 +256,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>
> object_initialize_child(obj, "sbc", >sbc, TYPE_ASPEED_SBC);
>
> +object_initialize_child(obj, "gfx", >gfx, TYPE_ASPEED_GFX);
> +
> object_initialize_child(obj, "iomem", >iomem,
> TYPE_UNIMPLEMENTED_DEVICE);
> object_initialize_child(obj, "video", >video,
> TYPE_UNIMPLEMENTED_DEVICE);
> object_initialize_child(obj, "dpmcu", >dpmcu,
> TYPE_UNIMPLEMENTED_DEVICE);
> @@ -607,6 +610,14 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
> return;
> }
> aspeed_mmio_map(s, SYS_BUS_DEVICE(>sbc), 0,
> sc->memmap[ASPEED_DEV_SBC]);
> +
> +/* GFX */
> +if (!sysbus_realize(SYS_BUS_DEVICE(>gfx), errp)) {
> +return;
> +}
> +aspeed_mmio_map(s, SYS_BUS_DEVICE(>gfx), 0,
> sc->memmap[ASPEED_DEV_GFX]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>gfx), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_GFX));
I think we're missing an entry for ASPEED_DEV_GFX in the irqmap array?
> }
>
> static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index b05b9dd41602..053149f9ccdf 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -33,6 +33,7 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU]= 0x1E6E2000,
> [ASPEED_DEV_HACE] = 0x1E6E3000,
> +[ASPEED_DEV_GFX]= 0x1E6E6000,
> [ASPEED_DEV_XDMA] = 0x1E6E7000,
> [ASPEED_DEV_VIDEO] = 0x1E70,
> [ASPEED_DEV_ADC]= 0x1E6E9000,
> @@ -69,6 +70,7 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
> [ASPEED_DEV_SDMC] = 0x1E6E,
> [ASPEED_DEV_SCU]= 0x1E6E2000,
> [ASPEED_DEV_HACE] = 0x1E6E3000,
> +[ASPEED_DEV_GFX]= 0x1E6E6000,
> [ASPEED_DEV_XDMA] =
Re: [RFC 0/3] Add Generic SPI GPIO model
On Sun, 31 Jul 2022, at 06:48, Cédric Le Goater wrote: > On 7/29/22 19:30, Peter Delevoryas wrote: >> Certainly we'd like to use IRQ's instead, but she ran into correctness >> problems. Maybe we can investigate that further and fix it. Yes, let's not work around problems that we have the ability to fix. > > So much is happening in that mode. Yes, though while there's no-doubt accidental complexity in terms of its implementation, the underlying hardware is also quite haphazard and we need an approach that scales to the large number of GPIOs it provides. So I'd argue there's a bunch of essential complexity involved as well. Do we start with a fresh implementation that allows us to get the expected behaviour and then build that out to replace the current implementation? Or, can we add more tests for the existing model to pin down where the bugs are? > We need more trace events in the Aspeed > GPIO model at least an extra in aspeed_gpio_update() We can always fall back to this but my hope is we can get better test coverage to iron out the bugs. Maybe that gets us a more refined and maintainable model implementation along the way. Cheers, Andrew
Re: [PATCH v3 8/8] arm/aspeed: Replace mx25l25635e chip model
On Mon, 25 Jul 2022, at 16:02, Cédric Le Goater wrote: > On 7/25/22 04:08, Andrew Jeffery wrote: >> >> >> On Fri, 22 Jul 2022, at 16:06, Cédric Le Goater wrote: >>> aspeed_machine_witherspoon_class_init(ObjectClass *oc, void *data) >>> mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)"; >>> amc->soc_name = "ast2500-a1"; >>> amc->hw_strap1 = WITHERSPOON_BMC_HW_STRAP1; >>> -amc->fmc_model = "mx25l25635e"; >>> +amc->fmc_model = "mx25l25635f"; >> >> The witherspoon I checked also reported mx25l25635e in dmesg for the >> FMC. >> >> You do say "generally" in the commit message though. > > You can not tell from dmesg. > > The MX25L25635F and MX25L25635E models share the same JEDEC ID and > the spi-nor flash device table only contains a mx25l25635e entry. > The MX25L25635F is detected in mx25l25635_post_bfpt_fixups using SFDP. > > That's why I added this warning : > > > https://github.com/legoater/linux/commit/934f0708ac597022cbf6c8d6f2ce91d55025e943 > Oh righto, sorry for the noise.
Re: [PATCH v3 8/8] arm/aspeed: Replace mx25l25635e chip model
On Fri, 22 Jul 2022, at 16:06, Cédric Le Goater wrote: > A mx25l25635f chip model is generally found on these machines. It's > newer and uses 4B opcodes which is better to exercise the support in > the Linux kernel. > > Signed-off-by: Cédric Le Goater > --- > hw/arm/aspeed.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 1c611284819d..7e95abc55b09 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -1157,7 +1157,7 @@ static void > aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data) > amc->soc_name = "ast2400-a1"; > amc->hw_strap1 = PALMETTO_BMC_HW_STRAP1; > amc->fmc_model = "n25q256a"; > -amc->spi_model = "mx25l25635e"; > +amc->spi_model = "mx25l25635f"; Hmm, dmesg reported mx25l25635e on the palmetto I checked > amc->num_cs= 1; > amc->i2c_init = palmetto_bmc_i2c_init; > mc->default_ram_size = 256 * MiB; > @@ -1208,7 +1208,7 @@ static void > aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data) > amc->soc_name = "ast2500-a1"; > amc->hw_strap1 = AST2500_EVB_HW_STRAP1; > amc->fmc_model = "mx25l25635e"; > -amc->spi_model = "mx25l25635e"; > +amc->spi_model = "mx25l25635f"; > amc->num_cs= 1; > amc->i2c_init = ast2500_evb_i2c_init; > mc->default_ram_size = 512 * MiB; > @@ -1258,7 +1258,7 @@ static void > aspeed_machine_witherspoon_class_init(ObjectClass *oc, void *data) > mc->desc = "OpenPOWER Witherspoon BMC (ARM1176)"; > amc->soc_name = "ast2500-a1"; > amc->hw_strap1 = WITHERSPOON_BMC_HW_STRAP1; > -amc->fmc_model = "mx25l25635e"; > +amc->fmc_model = "mx25l25635f"; The witherspoon I checked also reported mx25l25635e in dmesg for the FMC. You do say "generally" in the commit message though. Andrew
Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins
I think we've sorted this out, but replying to finalise the conversation On Tue, 12 Jul 2022, at 11:27, Peter Delevoryas wrote: > On Mon, Jul 11, 2022 at 10:56:08PM +0930, Andrew Jeffery wrote: >> >> /* >> @@ -607,7 +608,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr >> offset, uint64_t data, >> data &= props->output; >> data = update_value_control_source(set, set->data_value, data); >> set->data_read = data; >> -aspeed_gpio_update(s, set, data); >> +aspeed_gpio_update(s, set, data, set->direction); >> return; >> case gpio_reg_direction: >> /* >> @@ -683,7 +684,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr >> offset, uint64_t data, >>HWADDR_PRIx"\n", __func__, offset); >> return; >> } >> -aspeed_gpio_update(s, set, set->data_value); >> +aspeed_gpio_update(s, set, set->data_value, UINT32_MAX); > > Looks great overall, but why is the mode_mask UINT32_MAX here? Shouldn't it be > set->direction? We only want to let the guest OS write to output pins, right? > Or is that not how the register works? The set->direction case is handled in the top hunk which handles the data register write. Note that it performs an early return. The bottom hunk deals with making the value register consistent when we've updated any register that isn't the data register. Andrew
Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins
On Fri, 8 Jul 2022, at 04:34, Peter Delevoryas wrote:
> On Thu, Jul 07, 2022 at 10:53:57AM -0700, Peter Delevoryas wrote:
>> On Thu, Jul 07, 2022 at 10:56:02AM +0200, Cédric Le Goater wrote:
>> > On 7/7/22 09:17, Peter Delevoryas wrote:
>> > > It seems that aspeed_gpio_update is allowing the value for input pins to
>> > > be
>> > > modified through register writes and QOM property modification.
>> > >
>> > > The QOM property modification is fine, but modifying the value through
>> > > register writes from the guest OS seems wrong if the pin's direction is
>> > > set
>> > > to input.
>> > >
>> > > The datasheet specifies that "0" bits in the direction register select
>> > > input
>> > > mode, and "1" selects output mode.
>> > >
>> > > OpenBMC userspace code is accidentally writing 0's to the GPIO data
>> > > registers somewhere (or perhaps the driver is doing it through a reset or
>> > > something), and this is overwriting GPIO FRU information (board ID, slot
>> > > presence pins) that is initialized in Aspeed machine reset code (see
>> > > fby35_reset() in hw/arm/aspeed.c).
>> >
>> > It might be good to log a GUEST_ERROR in that case, when writing to an
>> > input GPIO and when reading from an output GPIO.
>>
>> Good idea, I'll include a GUEST_ERROR for writing to an input GPIO.
>>
>> I'm actually not totally certain about emitting an error when reading from an
>> output GPIO, because the driver can only do 8-bit reads at the finest
>> granularity, and if 1 of the 8 pins' direction is output, then it will be
>> reading the value of an output pin. But, that's not really bad, because
>> presumably the value will be ignored. Maybe I can go test this out on
>> hardware and figure out what happens though.
>
> Did a small experiment, I was looking at some of the most significant
> bits:
>
> root@dhcp-100-96-192-133:~# devmem 0x1e78
> 0x3CFF303E
> root@dhcp-100-96-192-133:~# devmem 0x1e780004
> 0x280C
> root@dhcp-100-96-192-133:~# devmem 0x1e78 32 0x
> root@dhcp-100-96-192-133:~# devmem 0x1e780004
> 0x280C
> root@dhcp-100-96-192-133:~# devmem 0x1e78
> 0x3CFF303E
> root@dhcp-100-96-192-133:~# devmem 0x1e78
> 0x3CFF303E
> root@dhcp-100-96-192-133:~# devmem 0x1e78 32 0
> root@dhcp-100-96-192-133:~# devmem 0x1e78
> 0x14FF303A
>
> Seems like the output pin 0x2000 was initially high, and the input
> pin right next to it 0x1000 was also high. After writing 0 to the
> data register, the value in the data register changed for the output
> pin, but not the input pin. Which matches what we're planning on doing
> in the controller.
>
> So yeah, I'll add GUEST_ERROR for writes to input pins but not output
> pins. The driver should probably be doing a read-modify-update.
> Although...if it's not, that technically wouldn't matter, behavior
> wise...maybe GUEST_ERROR isn't appropriate for writes to input pins
> either, for the same reason as I mentioned with reads of output pins.
> I'll let you guys comment on what you think we should do.
>
With the quick hack below I think I got sensible behaviour?
```
# devmem 0x1e78
0x
# devmem 0x1e780004
0x
# devmem 0x1e780004 32 1
# devmem 0x1e78 32 1
# devmem 0x1e78
0x0001
# devmem 0x1e78 32 3
# devmem 0x1e78
0x0001
# QEMU 7.0.0 monitor - type 'help' for more information
(qemu) qom-set gpio gpioA1 on
(qemu)
# devmem 0x1e78
0x0003
# (qemu) qom-set gpio gpioA1 off
(qemu)
# devmem 0x1e78
0x0001
# (qemu) qom-set gpio gpioA0 off
(qemu)
# devmem 0x1e78
0x0001
#
```
That was with the patch below. However, I think there's a deeper issue
with the input masking that needs to be addressed. Essentially we lack
modelling for the actual line state, we were proxying that with
register state. As it stands if we input-mask a line and use qom-set to
change its state the state update will go missing. However, as Joel
notes, I don't think we have anything configuring input masking.
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c63634d3d3e2..a1aa6504a8d8 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -244,7 +244,7 @@ static ptrdiff_t aspeed_gpio_set_idx(AspeedGPIOState *s,
GPIOSets *regs)
}
static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
- uint32_t value)
+ uint32_t value, uint32_t mode_mask)
{
uint32_t input_mask = regs->input_mask;
uint32_t direction = regs->direction;
@@ -253,7 +253,8 @@ static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets
*regs,
uint32_t diff;
int gpio;
-diff = old ^ new;
+diff = (old ^ new);
+diff &= mode_mask;
if (diff) {
for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
uint32_t mask = 1 << gpio;
@@ -315,7 +316,7 @@ static void aspeed_gpio_set_pin_level(AspeedGPIOState *s,
uint32_t set_idx,
value &= !pin_mask;
}
-
Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins
On Thu, 7 Jul 2022, at 17:50, Joel Stanley wrote:
> On Thu, 7 Jul 2022 at 07:17, Peter Delevoryas wrote:
>>
>> It seems that aspeed_gpio_update is allowing the value for input pins to be
>> modified through register writes and QOM property modification.
>>
>> The QOM property modification is fine, but modifying the value through
>> register writes from the guest OS seems wrong if the pin's direction is set
>> to input.
>>
>> The datasheet specifies that "0" bits in the direction register select input
>> mode, and "1" selects output mode.
>>
>> OpenBMC userspace code is accidentally writing 0's to the GPIO data
>> registers somewhere (or perhaps the driver is doing it through a reset or
>> something), and this is overwriting GPIO FRU information (board ID, slot
>> presence pins) that is initialized in Aspeed machine reset code (see
>> fby35_reset() in hw/arm/aspeed.c).
>>
>> Signed-off-by: Peter Delevoryas
>> Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 and
>> AST2500")
>> ---
>> hw/gpio/aspeed_gpio.c | 22 --
>> 1 file changed, 12 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
>> index a62a673857..2eae427201 100644
>> --- a/hw/gpio/aspeed_gpio.c
>> +++ b/hw/gpio/aspeed_gpio.c
>> @@ -268,7 +268,7 @@ static ptrdiff_t aspeed_gpio_set_idx(AspeedGPIOState *s,
>> GPIOSets *regs)
>> }
>>
>> static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
>> - uint32_t value)
>> + uint32_t value, bool force)
>> {
>> uint32_t input_mask = regs->input_mask;
>> uint32_t direction = regs->direction;
>> @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s,
>> GPIOSets *regs,
>> }
>>
>
> Reading this model hurts my head a little. Perhaps we also need to add
> a test for this case to make it clear what's going on.
>
> The test above the code you've changed does this:
>
>>/* ...and we're output or not input-masked... */
>>if (!(direction & mask) && (input_mask & mask)) {
>>continue;
>>}
>
> For clarity, !(direction & mask) means "is input".
>
> The comment confuses me because it says "or", but the code has "and".
The comment documents what conditions we need before we actually go and set the
output value.
The test is whether we fail to meet those conditions.
If the condition evaluates true we don't want to modify the GPIO value, so we
`continue` to the next loop iteration to test the next GPIO.
>
> input_mask doesn't seem to feature in the Linux driver, so that will
> always be zero. The test will be X && 0, which is always 0.
>
> If you changed it to || then we would do the test that the comment
> suggests. When the pin is input, we will skip updating the value.
The condition shouldn't change, rather if the comment makes things more
confusing rather than less, we should change that instead.
Andrew
Re: [PATCH v2 0/5] hw: aspeed: Init all UART's with serial devices
On Mon, 16 May 2022, at 16:48, Cédric Le Goater wrote: > On 5/16/22 08:23, Peter Delevoryas wrote: >> v2: >> - Rebased on Cedric's irq proposal. [1] >> - Added "Introduce common UART init function" patch >> - Added "Add uarts_num SoC attribute" patch >> - Rewrote last commit's message for clarity > > Looks good to me. > >> I tried testing this by running acceptance tests, particularly the >> boot_linux_console.py file, but I had to disable the raspi2_initrd case. >> It's not related to my changes (A/B tested and it fails on upstream/master >> too), but thought I would mention that.> >> I also manually tested several machines: >> >> AST2400: >> https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd >> AST2500: >> https://github.com/facebook/openbmc/releases/download/v2021.49.0/fby3.mtd >> AST2600: >> https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd >> AST1030: >> https://github.com/peterdelevoryas/OpenBIC/releases/download/oby35-cl-2022.13.01/Y35BCL.elf >> >> [1] https://lore.kernel.org/qemu-devel/20220516055620.2380197-1-...@kaod.org/ > I have quite a few images which I run manually. OpenBMC is providing > images, Aspeed also. Joel did a small tool for the IBM rainier : > >https://github.com/shenki/qemu-boot-test > > Having an automated framework for Aspeed machines pulling images > from different places would be nice but we cannot put all under > QEMU. For what it's worth I run this as a smoke test before pushing updates to openbmc/qemu: https://github.com/openbmc/openbmc-build-scripts/blob/master/scripts/test-qemu Andrew
Re: [PATCH 0/3] hw: aspeed_gpio: Model new interface for the AST2600
On Tue, 8 Feb 2022, at 01:34, Andrew Jeffery wrote: > Hello, > > This series adds support for a new register interface supported by the > Aspeed GPIO controller, present in at least the AST2600. > > The new interface is a single register implementing an indirect command > protocol that allows us to manipulate up to (at least) 208 GPIOs. This > makes it possible to write very simple drivers for e.g. u-boot and > jettison the need for the tedious data model required to deal with the > old register layout. > > I've lightly tested the device consistency under Linux. The Linux > driver is implemented in terms of the old interface, so data model > consistency can be tested one way by poking the driver using the > libgpiod tools and then the other using devmem to drive the new > interface. > > Please review! Naturally further testing revealed some quirks that require further enhancements to the modelling. Hold off on doing anything with this series for the moment. Cheers, Andrew
[PATCH 1/3] hw: aspeed_gpio: Cleanup stray semicolon after switch
Not sure how that got there. Signed-off-by: Andrew Jeffery --- hw/gpio/aspeed_gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c index 911d21c8cfbe..c63634d3d3e2 100644 --- a/hw/gpio/aspeed_gpio.c +++ b/hw/gpio/aspeed_gpio.c @@ -571,7 +571,7 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size) qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%" HWADDR_PRIx"\n", __func__, offset); return 0; -}; +} } static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data, -- 2.32.0
[PATCH 0/3] hw: aspeed_gpio: Model new interface for the AST2600
Hello, This series adds support for a new register interface supported by the Aspeed GPIO controller, present in at least the AST2600. The new interface is a single register implementing an indirect command protocol that allows us to manipulate up to (at least) 208 GPIOs. This makes it possible to write very simple drivers for e.g. u-boot and jettison the need for the tedious data model required to deal with the old register layout. I've lightly tested the device consistency under Linux. The Linux driver is implemented in terms of the old interface, so data model consistency can be tested one way by poking the driver using the libgpiod tools and then the other using devmem to drive the new interface. Please review! Andrew Andrew Jeffery (3): hw: aspeed_gpio: Cleanup stray semicolon after switch hw: aspeed_gpio: Split GPIOSet handling from accessors hw: aspeed_gpio: Support the AST2600's indexed register interface hw/gpio/aspeed_gpio.c | 305 -- include/hw/gpio/aspeed_gpio.h | 3 + 2 files changed, 261 insertions(+), 47 deletions(-) -- 2.32.0
[PATCH 3/3] hw: aspeed_gpio: Support the AST2600's indexed register interface
A new register interface was added to the AST2600 GPIO controller that
allows a single 32 bit register to drive configuration of up to 208
GPIOs. This makes way for a very simple driver implementation in
early-boot firmware such as u-boot. The old register interface required
drivers implement a tedious data model, but allowed efficient multi-line
bit-banging.
Either way, the hardware model in qemu becomes quite complex, though it
would have been less so had the new interface been the only one
available.
Signed-off-by: Andrew Jeffery
---
hw/gpio/aspeed_gpio.c | 202 +-
include/hw/gpio/aspeed_gpio.h | 3 +
2 files changed, 202 insertions(+), 3 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index 1d4d1aedc4b5..cee1a9a2e065 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -160,7 +160,42 @@
#define GPIO_YZAAAB_DIRECTION (0x1E4 >> 2)
#define GPIO_AC_DATA_VALUE (0x1E8 >> 2)
#define GPIO_AC_DIRECTION (0x1EC >> 2)
-#define GPIO_3_3V_MEM_SIZE 0x1F0
+#define GPIO_INDEX (0x2AC >> 2)
+#define GPIO_INDEX_DATA_SHIFT 20
+#define GPIO_INDEX_DATA_LEN 12
+#define GPIO_INDEX_DATA_DATA 20
+#define GPIO_INDEX_DATA_DIR 20
+#define GPIO_INDEX_DATA_IRQ_EN 20
+#define GPIO_INDEX_DATA_IRQ_TY0 21
+#define GPIO_INDEX_DATA_IRQ_TY1 22
+#define GPIO_INDEX_DATA_IRQ_TY2 23
+#define GPIO_INDEX_DATA_IRQ_STS 24
+#define GPIO_INDEX_DATA_DB1 20
+#define GPIO_INDEX_DATA_DB2 21
+#define GPIO_INDEX_DATA_TOL 20
+#define GPIO_INDEX_DATA_SRC0 20
+#define GPIO_INDEX_DATA_SRC1 20
+#define GPIO_INDEX_DATA_INPUT20
+#define GPIO_INDEX_DATA_WR_SRC 20
+#define GPIO_INDEX_TYPE_SHIFT 16
+#define GPIO_INDEX_TYPE_LEN 4
+#define GPIO_INDEX_TYPE_DATA 0
+#define GPIO_INDEX_TYPE_DIR 1
+#define GPIO_INDEX_TYPE_IRQ 2
+#define GPIO_INDEX_TYPE_DEBOUNCE 3
+#define GPIO_INDEX_TYPE_TOL 4
+#define GPIO_INDEX_TYPE_SRC 5
+#define GPIO_INDEX_TYPE_INPUT6
+#define GPIO_INDEX_TYPE_RSVD 7
+#define GPIO_INDEX_TYPE_WR_SRC 8
+#define GPIO_INDEX_TYPE_RD_SRC 9
+#define GPIO_INDEX_CMD_SHIFT 12
+#define GPIO_INDEX_CMD_LEN1
+#define GPIO_INDEX_CMD_WRITE 0
+#define GPIO_INDEX_CMD_READ 1
+#define GPIO_INDEX_NR_SHIFT 0
+#define GPIO_INDEX_NR_LEN 8
+#define GPIO_3_3V_MEM_SIZE 0x2B0
#define GPIO_3_3V_REG_ARRAY_SIZE (GPIO_3_3V_MEM_SIZE >> 2)
/* AST2600 only - 1.8V gpios */
@@ -571,6 +606,11 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr
offset, uint32_t size)
return (uint64_t) s->debounce_regs[idx];
}
+/* This is a (new, indirect) register interface for configuring GPIOs */
+if (agc->have_index_reg && idx == GPIO_INDEX) {
+return (uint64_t) s->index;
+}
+
reg = >reg_table[idx];
if (reg->set_idx >= agc->nr_gpio_sets) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
@@ -581,8 +621,73 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr
offset, uint32_t size)
return aspeed_gpio_set_read(s, reg);
}
-static void aspeed_gpio_set_write(AspeedGPIOState *s, const AspeedGPIOReg *reg,
- uint32_t data)
+static int aspeed_gpio_set_offset_read(AspeedGPIOState *s, int set, enum
GPIORegType reg,
+ int offset)
+{
+return !!(aspeed_gpio_set_read(s, &(AspeedGPIOReg){set, reg}) &
BIT(offset));
+}
+
+static const enum GPIORegType aspeed_gpio_index_type_map[] = {
+ [GPIO_INDEX_TYPE_DATA] = gpio_reg_data_value,
+ [GPIO_INDEX_TYPE_DIR] = gpio_reg_direction,
+ [GPIO_INDEX_TYPE_TOL] = gpio_reg_reset_tolerant,
+ [GPIO_INDEX_TYPE_INPUT] = gpio_reg_input_mask,
+ [GPIO_INDEX_TYPE_WR_SRC] = gpio_reg_input_mask /* See GPIO2AC doc */
+};
+
+static void
+aspeed_gpio_index_read(AspeedGPIOState *s, uint32_t type, uint32_t number)
+{
+int pin = number % 32;
+int set = number / 32;
+
+/* Clear the data field so we can OR into it without further data
dependencies */
+s->index = deposit32(s->index, GPIO_INDEX_DATA_SHIFT, GPIO_INDEX_DATA_LEN,
0);
+
+switch (type) {
+case GPIO_INDEX_TYPE_DATA:
+case GPIO_INDEX_TYPE_DIR:
+case GPIO_INDEX_TYPE_TOL:
+case GPIO_INDEX_TYPE_INPUT:
+case GPIO_INDEX_TYPE_WR_SRC:
+{
+enum GPIORegType reg = aspeed_gpio_index_type_map[type];
+s->index |= deposit32(0, GPIO_INDEX_DATA_SHIFT, GPIO_INDEX_DATA_LEN,
+ aspeed_gpio_set_offset_read(s, set, reg, pin));
+break;
+}
+case GPIO_INDEX_TYPE_IRQ:
+s->index |= deposit32(0, GPIO_INDEX_DATA_IRQ_EN, 1,
+aspeed_gpio_set_offset_read(s, set,
gpio_reg_int_enable, pin));
+s->index |= deposit32(0, GPIO_INDEX_DATA_I
[PATCH 2/3] hw: aspeed_gpio: Split GPIOSet handling from accessors
Pave the way for implementing the new register interface for GPIO
control provided by the AST2600. We need a consistent data model, so
do some work to enable use of the AspeedGPIOReg / GPIOSets data
structures for both.
Signed-off-by: Andrew Jeffery
---
hw/gpio/aspeed_gpio.c | 105 --
1 file changed, 60 insertions(+), 45 deletions(-)
diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
index c63634d3d3e2..1d4d1aedc4b5 100644
--- a/hw/gpio/aspeed_gpio.c
+++ b/hw/gpio/aspeed_gpio.c
@@ -516,28 +516,11 @@ static const AspeedGPIOReg
aspeed_1_8v_gpios[GPIO_1_8V_REG_ARRAY_SIZE] = {
[GPIO_1_8V_E_INPUT_MASK] = {1, gpio_reg_input_mask},
};
-static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
+static uint64_t
+aspeed_gpio_set_read(const AspeedGPIOState *s, const AspeedGPIOReg *reg)
{
-AspeedGPIOState *s = ASPEED_GPIO(opaque);
-AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
-uint64_t idx = -1;
-const AspeedGPIOReg *reg;
-GPIOSets *set;
+const GPIOSets *set = >sets[reg->set_idx];
-idx = offset >> 2;
-if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
-idx -= GPIO_DEBOUNCE_TIME_1;
-return (uint64_t) s->debounce_regs[idx];
-}
-
-reg = >reg_table[idx];
-if (reg->set_idx >= agc->nr_gpio_sets) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
- HWADDR_PRIx"\n", __func__, offset);
-return 0;
-}
-
-set = >sets[reg->set_idx];
switch (reg->type) {
case gpio_reg_data_value:
return set->data_value;
@@ -567,37 +550,44 @@ static uint64_t aspeed_gpio_read(void *opaque, hwaddr
offset, uint32_t size)
return set->data_read;
case gpio_reg_input_mask:
return set->input_mask;
-default:
+case gpio_not_a_reg:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid register: %d\n", __func__,
+ reg->type);
+}
+
+return 0;
+}
+
+static uint64_t aspeed_gpio_read(void *opaque, hwaddr offset, uint32_t size)
+{
+AspeedGPIOState *s = ASPEED_GPIO(opaque);
+AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+const AspeedGPIOReg *reg;
+uint64_t idx = -1;
+
+idx = offset >> 2;
+if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
+idx -= GPIO_DEBOUNCE_TIME_1;
+return (uint64_t) s->debounce_regs[idx];
+}
+
+reg = >reg_table[idx];
+if (reg->set_idx >= agc->nr_gpio_sets) {
qemu_log_mask(LOG_GUEST_ERROR, "%s: no getter for offset 0x%"
HWADDR_PRIx"\n", __func__, offset);
return 0;
}
+
+return aspeed_gpio_set_read(s, reg);
}
-static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
- uint32_t size)
+static void aspeed_gpio_set_write(AspeedGPIOState *s, const AspeedGPIOReg *reg,
+ uint32_t data)
{
-AspeedGPIOState *s = ASPEED_GPIO(opaque);
AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
const GPIOSetProperties *props;
-uint64_t idx = -1;
-const AspeedGPIOReg *reg;
-GPIOSets *set;
uint32_t cleared;
-
-idx = offset >> 2;
-if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
-idx -= GPIO_DEBOUNCE_TIME_1;
-s->debounce_regs[idx] = (uint32_t) data;
-return;
-}
-
-reg = >reg_table[idx];
-if (reg->set_idx >= agc->nr_gpio_sets) {
-qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
- HWADDR_PRIx"\n", __func__, offset);
-return;
-}
+GPIOSets *set;
set = >sets[reg->set_idx];
props = >props[reg->set_idx];
@@ -678,13 +668,38 @@ static void aspeed_gpio_write(void *opaque, hwaddr
offset, uint64_t data,
*/
set->input_mask = data & props->input;
break;
-default:
-qemu_log_mask(LOG_GUEST_ERROR, "%s: no setter for offset 0x%"
- HWADDR_PRIx"\n", __func__, offset);
+case gpio_not_a_reg:
+qemu_log_mask(LOG_GUEST_ERROR, "%s: Invalid register: %d\n", __func__,
+ reg->type);
return;
}
+
aspeed_gpio_update(s, set, set->data_value);
-return;
+}
+
+static void aspeed_gpio_write(void *opaque, hwaddr offset, uint64_t data,
+ uint32_t size)
+{
+AspeedGPIOState *s = ASPEED_GPIO(opaque);
+AspeedGPIOClass *agc = ASPEED_GPIO_GET_CLASS(s);
+const AspeedGPIOReg *reg;
+uint64_t idx = -1;
+
+idx = offset >> 2;
+if (idx >= GPIO_DEBOUNCE_TIME_1 && idx <= GPIO_DEBOUNCE_TIME_3) {
+idx -= GPIO_DEBOUN
Re: [PATCH] MAINTAINERS: Change status to Odd Fixes
On Wed, 3 Nov 2021, at 02:48, Peter Maydell wrote: > On Mon, 1 Nov 2021 at 18:31, Cédric Le Goater wrote: >> >> I haven't done any Aspeed development for a couple of years now and >> maintaining the Aspeed QEMU machines has been a side project since. >> I don't have time anymore. > > Thanks for the work you've done on this over the years. Second this! > > I guess this means I should start picking up aspeed-related > patches into the target-arm queue again ? I'd struggle taking it on - my qemu development is pretty sporadic and hackish at best and my patches need the review. Unless Joel wants to take the role I think it would be best if our patches went via target-arm again. A general thanks for your help here too Peter :) Andrew
Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
Hi Cédric, Peter, On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote: > On 8/28/21 5:58 PM, Peter Delevoryas wrote: > > I think I’m a little confused on this part. What I meant by “most machines > > just use UART5” was that most DTS’s use “stdout-path=”, but fuji uses > > “stdout-path=”. I /do/ see that SCU510 includes a bit related to > > UART, but it’s for disabling booting from UART1 and UART5. I just care > > about the console aspect, not booting. > > The UART can be switched with SCU70[29] on the AST2500, btw. If it helps, neither the AST2600's "Boot from UART" feature nor the AST2[456]00's "Debug UART" feature are related to which UART is used as the BMC console by u-boot and/or the kernel - the latter is entirely a software thing. The "Debug UART" is a hardware backdoor, a UART-to-AHB bridge implemented by the SoC. It provides a shell environment that allows you to issue transactions directly on the AHB if you perform a magic knock. I have a driver for it implemented here: https://github.com/amboar/cve-2019-6260/blob/master/src/debug.c SCU70[29] on the AST2500 selects whether this backdoor is exposed on UART1 or UART5. The "Boot from UART" feature is implemented in the AST2600 ROM code as a fallback for loading the SPL if fetching it from SPI-NOR or the eMMC fails, or the SPL is incorrectly signed for secure-boot. I think Peter is on the right track with this patch? Andrew
[PATCH] misc/pca9552: Fix LED status register indexing in pca955x_get_led()
There was a bit of a thinko in the state calculation where every odd pin
in was reported in e.g. "pwm0" mode rather than "off". This was the
result of an incorrect bit shift for the 2-bit field representing each
LED state.
Fixes: a90d8f84674d ("misc/pca9552: Add qom set and get")
Signed-off-by: Andrew Jeffery
---
hw/misc/pca9552.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/misc/pca9552.c b/hw/misc/pca9552.c
index b7686e27d7fa..fff19e369a39 100644
--- a/hw/misc/pca9552.c
+++ b/hw/misc/pca9552.c
@@ -272,7 +272,7 @@ static void pca955x_get_led(Object *obj, Visitor *v, const
char *name,
* reading the INPUTx reg
*/
reg = PCA9552_LS0 + led / 4;
-state = (pca955x_read(s, reg) >> (led % 8)) & 0x3;
+state = (pca955x_read(s, reg) >> ((led % 4) * 2)) & 0x3;
visit_type_str(v, name, (char **)_state[state], errp);
}
--
2.30.2
Re: [PATCH 2/2] watchdog: aspeed: Fix sequential control writes
On Fri, 9 Jul 2021, at 16:59, Philippe Mathieu-Daudé wrote:
> On 7/9/21 7:31 AM, Andrew Jeffery wrote:
> > The logic in the handling for the control register required toggling the
> > enable state for writes to stick. Rework the condition chain to allow
> > sequential writes that do not update the enable state.
> >
> > Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
> > Signed-off-by: Andrew Jeffery
> > ---
> > hw/watchdog/wdt_aspeed.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
> > index faa3d35fdf21..69c37af9a6e9 100644
> > --- a/hw/watchdog/wdt_aspeed.c
> > +++ b/hw/watchdog/wdt_aspeed.c
> > @@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr
> > offset, uint64_t data,
> > } else if (!enable && aspeed_wdt_is_enabled(s)) {
> > s->regs[WDT_CTRL] = data;
> > timer_del(s->timer);
> > +} else {
> > +s->regs[WDT_CTRL] = data;
>
> What about simplifying by moving here:
>
>if (!enable && aspeed_wdt_is_enabled(s)) {
>timer_del(s->timer);
>}
>
I don't think that works, as aspeed_wdt_is_enabled() tests the value of
s->regs[WDT_CTRL]. If you set it before you test then you end up in the
wrong state.
Andrew
[PATCH 1/2] watchdog: aspeed: Sanitize control register values
While some of the critical fields remain the same, there is variation in
the definition of the control register across the SoC generations.
Reserved regions are adjusted, while in other cases the mutability or
behaviour of fields change.
Introduce a callback to sanitize the value on writes to ensure model
behaviour reflects the hardware.
Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
Signed-off-by: Andrew Jeffery
---
hw/watchdog/wdt_aspeed.c | 24 ++--
include/hw/watchdog/wdt_aspeed.h | 1 +
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index 6352ba1b0e5b..faa3d35fdf21 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -118,13 +118,27 @@ static void aspeed_wdt_reload_1mhz(AspeedWDTState *s)
}
}
+static uint64_t aspeed_2400_sanitize_ctrl(uint64_t data)
+{
+return data & 0x;
+}
+
+static uint64_t aspeed_2500_sanitize_ctrl(uint64_t data)
+{
+return (data & ~(0xfUL << 8)) | WDT_CTRL_1MHZ_CLK;
+}
+
+static uint64_t aspeed_2600_sanitize_ctrl(uint64_t data)
+{
+return data & ~(0x7UL << 7);
+}
static void aspeed_wdt_write(void *opaque, hwaddr offset, uint64_t data,
unsigned size)
{
AspeedWDTState *s = ASPEED_WDT(opaque);
AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
-bool enable = data & WDT_CTRL_ENABLE;
+bool enable;
offset >>= 2;
@@ -144,6 +158,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset,
uint64_t data,
}
break;
case WDT_CTRL:
+data = awc->sanitize_ctrl(data);
+enable = data & WDT_CTRL_ENABLE;
if (enable && !aspeed_wdt_is_enabled(s)) {
s->regs[WDT_CTRL] = data;
awc->wdt_reload(s);
@@ -207,11 +223,12 @@ static const MemoryRegionOps aspeed_wdt_ops = {
static void aspeed_wdt_reset(DeviceState *dev)
{
AspeedWDTState *s = ASPEED_WDT(dev);
+AspeedWDTClass *awc = ASPEED_WDT_GET_CLASS(s);
s->regs[WDT_STATUS] = 0x3EF1480;
s->regs[WDT_RELOAD_VALUE] = 0x03EF1480;
s->regs[WDT_RESTART] = 0;
-s->regs[WDT_CTRL] = 0;
+s->regs[WDT_CTRL] = awc->sanitize_ctrl(0);
s->regs[WDT_RESET_WIDTH] = 0xFF;
timer_del(s->timer);
@@ -293,6 +310,7 @@ static void aspeed_2400_wdt_class_init(ObjectClass *klass,
void *data)
awc->ext_pulse_width_mask = 0xff;
awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
awc->wdt_reload = aspeed_wdt_reload;
+awc->sanitize_ctrl = aspeed_2400_sanitize_ctrl;
}
static const TypeInfo aspeed_2400_wdt_info = {
@@ -328,6 +346,7 @@ static void aspeed_2500_wdt_class_init(ObjectClass *klass,
void *data)
awc->reset_ctrl_reg = SCU_RESET_CONTROL1;
awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
awc->wdt_reload = aspeed_wdt_reload_1mhz;
+awc->sanitize_ctrl = aspeed_2500_sanitize_ctrl;
}
static const TypeInfo aspeed_2500_wdt_info = {
@@ -348,6 +367,7 @@ static void aspeed_2600_wdt_class_init(ObjectClass *klass,
void *data)
awc->reset_ctrl_reg = AST2600_SCU_RESET_CONTROL1;
awc->reset_pulse = aspeed_2500_wdt_reset_pulse;
awc->wdt_reload = aspeed_wdt_reload_1mhz;
+awc->sanitize_ctrl = aspeed_2600_sanitize_ctrl;
}
static const TypeInfo aspeed_2600_wdt_info = {
diff --git a/include/hw/watchdog/wdt_aspeed.h b/include/hw/watchdog/wdt_aspeed.h
index 80b03661e303..f945cd6c5833 100644
--- a/include/hw/watchdog/wdt_aspeed.h
+++ b/include/hw/watchdog/wdt_aspeed.h
@@ -44,6 +44,7 @@ struct AspeedWDTClass {
uint32_t reset_ctrl_reg;
void (*reset_pulse)(AspeedWDTState *s, uint32_t property);
void (*wdt_reload)(AspeedWDTState *s);
+uint64_t (*sanitize_ctrl)(uint64_t data);
};
#endif /* WDT_ASPEED_H */
--
2.30.2
[PATCH 2/2] watchdog: aspeed: Fix sequential control writes
The logic in the handling for the control register required toggling the
enable state for writes to stick. Rework the condition chain to allow
sequential writes that do not update the enable state.
Fixes: 854123bf8d4b ("wdt: Add Aspeed watchdog device model")
Signed-off-by: Andrew Jeffery
---
hw/watchdog/wdt_aspeed.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/hw/watchdog/wdt_aspeed.c b/hw/watchdog/wdt_aspeed.c
index faa3d35fdf21..69c37af9a6e9 100644
--- a/hw/watchdog/wdt_aspeed.c
+++ b/hw/watchdog/wdt_aspeed.c
@@ -166,6 +166,8 @@ static void aspeed_wdt_write(void *opaque, hwaddr offset,
uint64_t data,
} else if (!enable && aspeed_wdt_is_enabled(s)) {
s->regs[WDT_CTRL] = data;
timer_del(s->timer);
+} else {
+s->regs[WDT_CTRL] = data;
}
break;
case WDT_RESET_WIDTH:
--
2.30.2
[PATCH 0/2] wdt_aspeed: Fix behaviour of control register
Hello, I discovered a couple of bugs in the watchdog while testing a tool to poke Aspeed BMCs over their various AHB bridges. The immediate observation was that the model for the 2500 wasn't signalling use of the fixed 1MHz clock, which is resolved in the first patch. The other observation was that sequential writes to control weren't sticking if the enable bit wasn't toggled, which is fixed in the second patch. Please review. Andrew Andrew Jeffery (2): watchdog: aspeed: Sanitize control register values watchdog: aspeed: Fix sequential control writes hw/watchdog/wdt_aspeed.c | 26 -- include/hw/watchdog/wdt_aspeed.h | 1 + 2 files changed, 25 insertions(+), 2 deletions(-) -- 2.30.2
Re: [PATCH] arm/aspeed: rainier: Add i2c eeproms and muxes
On Fri, 25 Jun 2021, at 14:36, Joel Stanley wrote:
> These are the devices documented by the Rainier device tree. With this
> we can see the guest discovering the multiplexers and probing the eeprom
> devices:
>
> i2c i2c-2: Added multiplexed i2c bus 16
> i2c i2c-2: Added multiplexed i2c bus 17
> i2c i2c-2: Added multiplexed i2c bus 18
> i2c i2c-2: Added multiplexed i2c bus 19
> i2c-mux-gpio i2cmux: 4 port mux on 1e78a180.i2c-bus adapter
> at24 20-0050: 8192 byte 24c64 EEPROM, writable, 1 bytes/write
> i2c i2c-4: Added multiplexed i2c bus 20
> at24 21-0051: 8192 byte 24c64 EEPROM, writable, 1 bytes/write
> i2c i2c-4: Added multiplexed i2c bus 21
> at24 22-0052: 8192 byte 24c64 EEPROM, writable, 1 bytes/write
>
> Signed-off-by: Joel Stanley
> ---
> hw/arm/aspeed.c | 56 +
> 1 file changed, 56 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 1301e8fdffb2..7ed22294c6eb 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -677,6 +677,10 @@ static void g220a_bmc_i2c_init(AspeedMachineState *bmc)
> static void rainier_bmc_i2c_init(AspeedMachineState *bmc)
> {
> AspeedSoCState *soc = >soc;
> +I2CSlave *i2c_mux;
> +
> +smbus_eeprom_init_one(aspeed_i2c_get_bus(>i2c, 0), 0x51,
> + g_malloc0(32 * 1024));
>
> /* The rainier expects a TMP275 but a TMP105 is compatible */
> i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 4), TYPE_TMP105,
> @@ -685,11 +689,25 @@ static void rainier_bmc_i2c_init(AspeedMachineState
> *bmc)
> 0x49);
> i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 4), TYPE_TMP105,
> 0x4a);
> +i2c_mux = i2c_slave_create_simple(aspeed_i2c_get_bus(>i2c, 4),
> + "pca9546", 0x70);
> +smbus_eeprom_init_one(pca954x_i2c_get_bus(i2c_mux, 0), 0x50,
> + g_malloc0(64 * 1024));
The EEPROMs described in the Rainier devicetree are Atmel AT2x devices and
not SMBus EEPROMs. The SMBus EEPROM model uses SMBus block reads and
writes which are not what the AT2x driver issues. If you try to read or
write data under Linux from the EEPROMs in this patch you just get
corrupt results. So as far as I can see the patch needs to be reworked.
Andrew
Re: RFC: Memory region accesses where .valid.min_access_size < .impl.min_access_size
On Thu, 13 May 2021, at 22:30, Jonathan Cameron wrote: > On Thu, 13 May 2021 14:36:27 +0200 > Philippe Mathieu-Daudé wrote: > > > On 5/13/21 2:23 PM, Peter Maydell wrote: > > > On Thu, 13 May 2021 at 12:49, Jonathan Cameron > > > wrote: > > >> My initial suggestion was to fix this by adding the relatively > > >> simple code needed in the driver to implement byte read / write, > > >> but Ben pointed at the QEMU docs - docs/devel/memory.rst which > > >> says > > >> " > > >> .impl.min_access_size, .impl.max_access_size define the access sizes > > >>(in bytes) supported by the *implementation*; other access sizes will > > >> be > > >>emulated using the ones available. For example a 4-byte write will be > > >>emulated using four 1-byte writes, if .impl.max_access_size = 1. > > >> " > > >> > > >> This isn't true when we have the situation where > > >> .valid.min_access_size < .imp.min_access_size > > >> > > >> So change the docs or try to make this work? > > > > See also this patch from Francisco: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg636935.html > > > > And full unaligned access support from Andrew: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg461247.html Much better to use lore.kernel.org: https://lore.kernel.org/qemu-devel/20170630030058.28943-1-and...@aj.id.au/ because... > > Thanks - that's very similar to what I was carrying, but I think it > only covers the read case. That's backed up by the comment: > /* XXX: Can't do this hack for writes */ It becomes easier to find Paolo's suggestion to fix that here: https://lore.kernel.org/qemu-devel/cd1aba90-176f-9ec6-3e2b-d1135156a...@redhat.com/ Would love to see this resolved! Unfortunately I haven't had the bandwidth to fix it all up for ... a long time now. Andrew
Re: [PATCH] aspeed: Emulate the AST2600A3
On Tue, 13 Apr 2021, at 00:57, Cédric Le Goater wrote: > On 3/4/21 1:43 PM, Joel Stanley wrote: > > This is the latest revision of the ASPEED 2600 SoC. > > Should we change all machines to use the new SoC ? > > I would prefer if we introduced an "ast2600-a3" Aspeed SoC, that we would > use for the newer rainier machine, and leave the tacoma-bmc and ast2600-evb > machines as they are. I think we just change them all. We're not going to see pre-A3 chips in production systems. Andrew
Re: [PATCH v5 1/3] hw: Model ASPEED's Hash and Crypto Engine
On Fri, 9 Apr 2021, at 09:32, Joel Stanley wrote: > The HACE (Hash and Crypto Engine) is a device that offloads MD5, SHA1, > SHA2, RSA and other cryptographic algorithms. > > This initial model implements a subset of the device's functionality; > currently only MD5/SHA hashing, and on the ast2600's scatter gather > engine. > > Co-developed-by: Klaus Heinrich Kiwi > Reviewed-by: Cédric Le Goater > Reviewed-by: Philippe Mathieu-Daudé > Signed-off-by: Joel Stanley Reviewed-by: Andrew Jeffery
Re: [PATCH v3 1/3] hw: Model ASPEED's Hash and Crypto Engine
On Fri, 12 Mar 2021, at 21:27, Joel Stanley wrote:
> The HACE (Hash and Crypto Engine) is a device that offloads MD5, SHA1,
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley
> Signed-off-by: Cédric Le Goater
> ---
> v3:
> - rebase on upstream to fix meson.build conflict
> v2:
> - reorder register defines
> - mask src/dest/len registers according to hardware
> ---
> include/hw/misc/aspeed_hace.h | 33
> hw/misc/aspeed_hace.c | 312 ++
> hw/misc/meson.build | 1 +
> 3 files changed, 346 insertions(+)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
>
> diff --git a/include/hw/misc/aspeed_hace.h
> b/include/hw/misc/aspeed_hace.h
> new file mode 100644
> index ..e1fce670ef9e
> --- /dev/null
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -0,0 +1,33 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ASPEED_HACE_H
> +#define ASPEED_HACE_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_HACE "aspeed.hace"
> +#define ASPEED_HACE(obj) OBJECT_CHECK(AspeedHACEState, (obj),
> TYPE_ASPEED_HACE)
> +
> +#define ASPEED_HACE_NR_REGS (0x64 >> 2)
> +
> +typedef struct AspeedHACEState {
> +/* */
> +SysBusDevice parent;
> +
> +/*< public >*/
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_HACE_NR_REGS];
> +
> +MemoryRegion *dram_mr;
> +AddressSpace dram_as;
> +} AspeedHACEState;
> +
> +#endif /* _ASPEED_HACE_H_ */
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> new file mode 100644
> index ..3d02fae2dd62
> --- /dev/null
> +++ b/hw/misc/aspeed_hace.c
> @@ -0,0 +1,312 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * Joel Stanley
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_hace.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "crypto/hash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/irq.h"
> +
> +#define R_CRYPT_CMD (0x10 / 4)
> +
> +#define R_STATUS(0x1c / 4)
> +#define HASH_IRQBIT(9)
> +#define CRYPT_IRQ BIT(12)
> +#define TAG_IRQ BIT(15)
> +
> +#define R_HASH_SRC (0x20 / 4)
> +#define R_HASH_DEST (0x24 / 4)
> +#define R_HASH_SRC_LEN (0x2c / 4)
> +
> +#define R_HASH_CMD (0x30 / 4)
> +/* Hash algorithim selection */
> +#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
> +#define HASH_ALGO_MD5 0
> +#define HASH_ALGO_SHA1 BIT(5)
> +#define HASH_ALGO_SHA224 BIT(6)
> +#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
> +#define HASH_ALGO_SHA512_SERIES(BIT(5) | BIT(6))
> +/* SHA512 algorithim selection */
> +#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
> +#define HASH_ALGO_SHA512_SHA5120
> +#define HASH_ALGO_SHA512_SHA384BIT(10)
> +#define HASH_ALGO_SHA512_SHA256BIT(11)
> +#define HASH_ALGO_SHA512_SHA224(BIT(10) | BIT(11))
> +/* HMAC modes */
> +#define HASH_HMAC_MASK (BIT(7) | BIT(8))
> +#define HASH_DIGEST0
> +#define HASH_DIGEST_HMAC BIT(7)
> +#define HASH_DIGEST_ACCUM BIT(8)
> +#define HASH_HMAC_KEY (BIT(7) | BIT(8))
> +/* Cascscaed operation modes */
> +#define HASH_ONLY 0
> +#define HASH_ONLY2 BIT(0)
> +#define HASH_CRYPT_THEN_HASH BIT(1)
> +#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
> +/* Other cmd bits */
> +#define HASH_IRQ_ENBIT(9)
> +#define HASH_SG_EN BIT(18)
> +
> +
> +static int do_hash_operation(AspeedHACEState *s, int algo)
> +{
> +hwaddr src, len, dest;
> +uint8_t *digest_buf = NULL;
> +size_t digest_len = 0;
> +char *src_buf;
> +int rc;
> +
> +src = 0x8000 | s->regs[R_HASH_SRC];
Tricky. Also doesn't work on the AST2400 where SDRAM is based at
0x4000?
Other than that it looks good to me.
Andrew
Re: [PATCH 2/2] aspeed: Integrate HACE
On Wed, 3 Mar 2021, at 17:33, Joel Stanley wrote: > Add the hash and crypto engine model to the aspeed socs. > > Signed-off-by: Joel Stanley > [ clg: documentation update ] > Signed-off-by: Cédric Le Goater Reviewed-by: Andrew Jeffery
Re: [PATCH 1/2] hw: Model ASPEED's Hash and Crypto Engine
On Wed, 3 Mar 2021, at 17:33, Joel Stanley wrote:
> The HACE (Hash and Crpyto Engine) is a device that offloads MD5, SHA1,
> SHA2, RSA and other cryptographic algorithms.
>
> This initial model implements a subset of the device's functionality;
> currently only direct access (non-scatter gather) hashing.
>
> Signed-off-by: Joel Stanley
> Signed-off-by: Cédric Le Goater
> ---
> include/hw/misc/aspeed_hace.h | 33
> hw/misc/aspeed_hace.c | 302 ++
> hw/misc/meson.build | 2 +-
> 3 files changed, 336 insertions(+), 1 deletion(-)
> create mode 100644 include/hw/misc/aspeed_hace.h
> create mode 100644 hw/misc/aspeed_hace.c
>
> diff --git a/include/hw/misc/aspeed_hace.h
> b/include/hw/misc/aspeed_hace.h
> new file mode 100644
> index ..e1fce670ef9e
> --- /dev/null
> +++ b/include/hw/misc/aspeed_hace.h
> @@ -0,0 +1,33 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#ifndef ASPEED_HACE_H
> +#define ASPEED_HACE_H
> +
> +#include "hw/sysbus.h"
> +
> +#define TYPE_ASPEED_HACE "aspeed.hace"
> +#define ASPEED_HACE(obj) OBJECT_CHECK(AspeedHACEState, (obj),
> TYPE_ASPEED_HACE)
> +
> +#define ASPEED_HACE_NR_REGS (0x64 >> 2)
0x64 is the offset of the last defined register, so this should be (0x68 >> 2)
> +
> +typedef struct AspeedHACEState {
> +/* */
> +SysBusDevice parent;
> +
> +/*< public >*/
> +MemoryRegion iomem;
> +qemu_irq irq;
> +
> +uint32_t regs[ASPEED_HACE_NR_REGS];
> +
> +MemoryRegion *dram_mr;
> +AddressSpace dram_as;
> +} AspeedHACEState;
> +
> +#endif /* _ASPEED_HACE_H_ */
> diff --git a/hw/misc/aspeed_hace.c b/hw/misc/aspeed_hace.c
> new file mode 100644
> index ..0e402a0adea9
> --- /dev/null
> +++ b/hw/misc/aspeed_hace.c
> @@ -0,0 +1,302 @@
> +/*
> + * ASPEED Hash and Crypto Engine
> + *
> + * Copyright (C) 2021 IBM Corp.
> + *
> + * Joel Stanley
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/log.h"
> +#include "qemu/error-report.h"
> +#include "hw/misc/aspeed_hace.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +#include "crypto/hash.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/irq.h"
> +
> +#define R_STATUS(0x1c / 4)
> +#define HASH_IRQBIT(9)
> +#define CRYPT_IRQ BIT(12)
> +#define TAG_IRQ BIT(15)
> +
> +#define R_HASH_CMD (0x30 / 4)
> +/* Hash algorithim selection */
> +#define HASH_ALGO_MASK (BIT(4) | BIT(5) | BIT(6))
Hmm, feels GENMASK()-y, but looks like the tree is in a bit of a weird
state in that respect:
$ git grep GENMASK
include/hw/usb/dwc2-regs.h:#define GSNPSID_ID_MASK
GENMASK(31, 16)
include/standard-headers/asm-x86/kvm_para.h:#define KVM_ASYNC_PF_VEC_MASK
GENMASK(7, 0)
$
> +#define HASH_ALGO_MD5 0
> +#define HASH_ALGO_SHA1 BIT(5)
> +#define HASH_ALGO_SHA224 BIT(6)
> +#define HASH_ALGO_SHA256 (BIT(4) | BIT(6))
Not something you need to fix necessarily, but it would feel more
intuitive to me to order these MSB to LSB, so e.g. (BIT(6) | BIT(4)).
That way I can "see" the value without having to reverse the bits.
> +#define HASH_ALGO_SHA512_SERIES(BIT(5) | BIT(6))
> +/* SHA512 algorithim selection */
> +#define SHA512_HASH_ALGO_MASK (BIT(10) | BIT(11) | BIT(12))
> +#define HASH_ALGO_SHA512_SHA5120
> +#define HASH_ALGO_SHA512_SHA384BIT(10)
> +#define HASH_ALGO_SHA512_SHA256BIT(11)
> +#define HASH_ALGO_SHA512_SHA224(BIT(10) | BIT(11))
> +/* HMAC modes */
> +#define HASH_HMAC_MASK (BIT(7) | BIT(8))
> +#define HASH_DIGEST0
> +#define HASH_DIGEST_HMAC BIT(7)
> +#define HASH_DIGEST_ACCUM BIT(8)
> +#define HASH_HMAC_KEY (BIT(7) | BIT(8))
> +/* Cascscaed operation modes */
> +#define HASH_ONLY 0
> +#define HASH_ONLY2 BIT(0)
> +#define HASH_CRYPT_THEN_HASH BIT(1)
> +#define HASH_HASH_THEN_CRYPT (BIT(0) | BIT(1))
> +/* Other cmd bits */
> +#define HASH_IRQ_ENBIT(9)
> +#define HASH_SG_EN BIT(18)
> +
> +#define R_CRYPT_CMD (0x10 / 4)
> +
> +#define R_HASH_SRC (0x20 / 4)
> +#define R_HASH_DEST (0x24 / 4)
> +#define R_HASH_SRC_LEN (0x2c / 4)
In general, ordering the registers and fields in datasheet-order is
helpful to me as a reviewer...
> +
> +static int do_hash_operation(AspeedHACEState *s, int algo)
> +{
> +hwaddr src, len, dest;
> +uint8_t *digest_buf = NULL;
> +size_t digest_len = 0;
> +char *src_buf;
> +int rc;
> +
> +src = s->regs[R_HASH_SRC];
> +len = s->regs[R_HASH_SRC_LEN];
> +dest =
[PATCH v3 5/5] hw/misc: Model KCS devices in the Aspeed LPC controller
Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
IO cycles from the BMC to the host.
Expose support on the BMC side by implementing the usual MMIO
behaviours, and expose the ability to inspect the KCS registers in
"host" style by accessing QOM properties associated with each register.
The model caters to the IRQ style of both the AST2600 and the earlier
SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
sub-device, while there is a single IRQ shared across all subdevices on
the AST2400 and AST2500.
Signed-off-by: Andrew Jeffery
Reviewed-by: Cédric Le Goater
---
hw/arm/aspeed_ast2600.c | 28 ++-
hw/arm/aspeed_soc.c | 24 ++-
hw/misc/aspeed_lpc.c | 359 ++-
include/hw/arm/aspeed_soc.h | 1 +
include/hw/misc/aspeed_lpc.h | 17 +-
5 files changed, 424 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 5a7b8ba81c92..4f83097e4a26 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_ETH3] = 32,
[ASPEED_DEV_ETH4] = 33,
-
+[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
};
static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
@@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the GIC. It is otherwise unused. */
sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
+ * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
+ * shared across the subdevices, and the shared IRQ output to the VIC is at
+ * offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_4));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4f098da437ac..057d053c8478 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_WDT]= 27,
[ASPEED_DEV_PWM]= 28,
[ASPEED_DEV_LPC]= 8,
-[ASPEED_DEV_IBT]= 8, /* LPC */
[ASPEED_DEV_I2C]= 12,
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
@@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the VIC */
sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
+ * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
+ * contrast, on the AST2600, the subdevice IRQs are connected straight to
+ * the GIC).
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the shared IRQ
output
+ * to the VIC is at offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_4));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEM
[PATCH v3 4/5] hw/misc: Add a basic Aspeed LPC controller model
From: Cédric Le Goater
This is a very minimal framework to access registers which are used to
configure the AHB memory mapping of the flash chips on the LPC HC
Firmware address space.
Signed-off-by: Cédric Le Goater
Signed-off-by: Andrew Jeffery
---
docs/system/arm/aspeed.rst | 2 +-
hw/arm/aspeed_ast2600.c | 10 +++
hw/arm/aspeed_soc.c | 10 +++
hw/misc/aspeed_lpc.c | 131 +++
hw/misc/meson.build | 7 +-
include/hw/arm/aspeed_soc.h | 2 +
include/hw/misc/aspeed_lpc.h | 32 +
7 files changed, 192 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/aspeed_lpc.c
create mode 100644 include/hw/misc/aspeed_lpc.h
diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 690bada7842b..2f6fa8938d02 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -48,6 +48,7 @@ Supported devices
* UART
* Ethernet controllers
* Front LEDs (PCA9552 on I2C bus)
+ * LPC Peripheral Controller (a subset of subdevices are supported)
Missing devices
@@ -56,7 +57,6 @@ Missing devices
* Coprocessor support
* ADC (out of tree implementation)
* PWM and Fan Controller
- * LPC Bus Controller
* Slave GPIO Controller
* Super I/O Controller
* Hash/Crypto Engine
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 2125a96ef317..5a7b8ba81c92 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -211,6 +211,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "emmc-controller.sdhci", >emmc.slots[0],
TYPE_SYSBUS_SDHCI);
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
/*
@@ -469,6 +471,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
sysbus_mmio_map(SYS_BUS_DEVICE(>emmc), 0, sc->memmap[ASPEED_DEV_EMMC]);
sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07a..4f098da437ac 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -211,6 +211,8 @@ static void aspeed_soc_init(Object *obj)
object_initialize_child(obj, "sdhci[*]", >sdhci.slots[i],
TYPE_SYSBUS_SDHCI);
}
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -393,6 +395,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
sc->memmap[ASPEED_DEV_SDHCI]);
sysbus_connect_irq(SYS_BUS_DEVICE(>sdhci), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
new file mode 100644
index ..e668e985ff04
--- /dev/null
+++ b/hw/misc/aspeed_lpc.c
@@ -0,0 +1,131 @@
+/*
+ * ASPEED LPC Controller
+ *
+ * Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_lpc.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#define TO_REG(offset) ((offset) >> 2)
+
+#define HICR0TO_REG(0x00)
+#define HICR1TO_REG(0x04)
+#define HICR2TO_REG(0x08)
+#define HICR3TO_REG(0x0C)
+#define HICR4TO_REG(0x10)
+#define HICR5TO_REG(0x80)
+#define HICR6TO_REG(0x84)
+#define HICR7TO_REG(0x88)
+#define HICR8TO_REG(0x8C)
+
+static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
+{
+AspeedLPCState *s = ASPEED_LPC(opaque);
+int reg = TO_REG(offset);
+
+if (reg >= ARRAY_SIZE(s->regs)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at
[PATCH v3 1/5] hw/arm: ast2600: Force a multiple of 32 of IRQs for the GIC
This appears to be a requirement of the GIC model. The AST2600 allocates
197 GIC IRQs, which we will adjust shortly.
Signed-off-by: Andrew Jeffery
Reviewed-by: Cédric Le Goater
---
hw/arm/aspeed_ast2600.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bf31ca351feb..bc0eeb058b24 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define ASPEED_SOC_AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 128
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
@@ -267,7 +267,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
object_property_set_int(OBJECT(>a7mpcore), "num-cpu", sc->num_cpus,
_abort);
object_property_set_int(OBJECT(>a7mpcore), "num-irq",
-ASPEED_SOC_AST2600_MAX_IRQ + GIC_INTERNAL,
+ROUND_UP(AST2600_MAX_IRQ + GIC_INTERNAL, 32),
_abort);
sysbus_realize(SYS_BUS_DEVICE(>a7mpcore), _abort);
--
2.27.0
[PATCH v3 3/5] hw/arm: ast2600: Correct the iBT interrupt ID
The AST2600 allocates distinct GIC IRQs for the LPC subdevices such as
the iBT device. Previously on the AST2400 and AST2500 the LPC subdevices
shared a single LPC IRQ.
Signed-off-by: Andrew Jeffery
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
---
hw/arm/aspeed_ast2600.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 22fcb5b0edbe..2125a96ef317 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -98,7 +98,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_WDT] = 24,
[ASPEED_DEV_PWM] = 44,
[ASPEED_DEV_LPC] = 35,
-[ASPEED_DEV_IBT] = 35,/* LPC */
+[ASPEED_DEV_IBT] = 143,
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
--
2.27.0
[PATCH v3 2/5] hw/arm: ast2600: Set AST2600_MAX_IRQ to value from datasheet
The datasheet says we have 197 IRQs allocated, and we need more than 128
to describe IRQs from LPC devices. Raise the value now to allow
modelling of the LPC devices.
Signed-off-by: Andrew Jeffery
Reviewed-by: Philippe Mathieu-Daudé
Reviewed-by: Cédric Le Goater
---
hw/arm/aspeed_ast2600.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bc0eeb058b24..22fcb5b0edbe 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 197
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
--
2.27.0
[PATCH v3 0/5] aspeed: LPC peripheral controller devices
Hello, This series adds support for some of the LPC[1] peripherals found in Aspeed BMC SoCs. [1] https://www.intel.com/content/dam/www/program/design/us/en/documents/low-pin-count-interface-specification.pdf v3 fixes a copy/paste error hooking up the LPC IRQ for the AST2600, identified off-list. I've tested exercised the eMMC path to confirm the fix. v2 of the series can be found here: https://lore.kernel.org/qemu-devel/20210301010610.355702-1-and...@aj.id.au/T/#mccf00fea21d955d74de39dbc49af8451b447ff54 BMCs typically provide a number of features to their host via LPC that include but are not limited to: 1. Mapping LPC firmware cycles to BMC-controlled flash devices 2. UART(s) for system console routing 3. POST code routing 4. Keyboard-Controller-Style (KCS) IPMI devices 5. Block Transfer (BT) IPMI devices 6. A SuperIO controller for management of LPC devices and miscellaneous functionality Specifically, this series adds basic support for functions 1 and 4 above, handling the BMC firmware configuring the bridge mapping LPC firmware cycles onto its AHB as well as support for four KCS devices. Aspeed's LPC controller is not a straight-forward device by any stretch. It contains at least the capabilities outlined above, in the sense that it's not possible to cleanly separate the different functions into distinct MMIO sub-regions: Registers for the various bits of functionality have the feel of arbitrary placement with a nod to feature-creep and backwards compatibility. Further, the conceptually coherent pieces of functionality often come with the ability to issue interrupts, though for the AST2400 and AST2500 there is one shared VIC IRQ for all LPC "subdevices". By contrast the AST2600 gives each subdevice a distinct IRQ via the GIC. All this combined leads to some complexity regarding the interrupts and handling the MMIO accesses (in terms of mapping the access back to the function it's affecting). Finally, as a point of clarity, Aspeed BMCs also contain an LPC Host Controller to drive the LPC bus. This series does not concern itself with the LPC Host Controller function, only with a subset of the peripheral devices the BMC presents to the host. I've tested the series using a combination of the ast2600-evb, witherspoon-bmc and romulus-bmc machines along with a set of recently-posted patches for Linux[2]. Please review! Andrew [2] https://lore.kernel.org/openbmc/20210219142523.3464540-1-and...@aj.id.au/T/#m1e2029e7aa2be3056320e8d46b3b5b1539a776b4 Andrew Jeffery (4): hw/arm: ast2600: Force a multiple of 32 of IRQs for the GIC hw/arm: ast2600: Set AST2600_MAX_IRQ to value from datasheet hw/arm: ast2600: Correct the iBT interrupt ID hw/misc: Model KCS devices in the Aspeed LPC controller Cédric Le Goater (1): hw/misc: Add a basic Aspeed LPC controller model docs/system/arm/aspeed.rst | 2 +- hw/arm/aspeed_ast2600.c | 44 +++- hw/arm/aspeed_soc.c | 34 ++- hw/misc/aspeed_lpc.c | 486 +++ hw/misc/meson.build | 7 +- include/hw/arm/aspeed_soc.h | 3 + include/hw/misc/aspeed_lpc.h | 47 7 files changed, 616 insertions(+), 7 deletions(-) create mode 100644 hw/misc/aspeed_lpc.c create mode 100644 include/hw/misc/aspeed_lpc.h base-commit: 51db2d7cf26d05a961ec0ee0eb773594b32cc4a1 -- 2.27.0
Re: [PATCH v2 4/5] hw/misc: Add a basic Aspeed LPC controller model
On Mon, 1 Mar 2021, at 11:36, Andrew Jeffery wrote:
> From: Cédric Le Goater
>
> This is a very minimal framework to access registers which are used to
> configure the AHB memory mapping of the flash chips on the LPC HC
> Firmware address space.
>
> Signed-off-by: Cédric Le Goater
> Signed-off-by: Andrew Jeffery
> ---
> docs/system/arm/aspeed.rst | 2 +-
> hw/arm/aspeed_ast2600.c | 10 +++
> hw/arm/aspeed_soc.c | 10 +++
> hw/misc/aspeed_lpc.c | 131 +++
> hw/misc/meson.build | 7 +-
> include/hw/arm/aspeed_soc.h | 2 +
> include/hw/misc/aspeed_lpc.h | 32 +
> 7 files changed, 192 insertions(+), 2 deletions(-)
> create mode 100644 hw/misc/aspeed_lpc.c
> create mode 100644 include/hw/misc/aspeed_lpc.h
>
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index 690bada7842b..2f6fa8938d02 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -48,6 +48,7 @@ Supported devices
> * UART
> * Ethernet controllers
> * Front LEDs (PCA9552 on I2C bus)
> + * LPC Peripheral Controller (a subset of subdevices are supported)
>
>
> Missing devices
> @@ -56,7 +57,6 @@ Missing devices
> * Coprocessor support
> * ADC (out of tree implementation)
> * PWM and Fan Controller
> - * LPC Bus Controller
> * Slave GPIO Controller
> * Super I/O Controller
> * Hash/Crypto Engine
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 2125a96ef317..60152de001e6 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -211,6 +211,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
>
> object_initialize_child(obj, "emmc-controller.sdhci", >emmc.slots[0],
> TYPE_SYSBUS_SDHCI);
> +
> +object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
> }
>
> /*
> @@ -469,6 +471,14 @@ static void aspeed_soc_ast2600_realize(DeviceState
> *dev, Error **errp)
> sysbus_mmio_map(SYS_BUS_DEVICE(>emmc), 0,
> sc->memmap[ASPEED_DEV_EMMC]);
> sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
> +
> +/* LPC */
> +if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
> +return;
> +}
> +sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0,
> sc->memmap[ASPEED_DEV_LPC]);
> +sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
> + aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
Hah, this isn't right! We don't notice it wrt LPC devices because the
LPC IRQ is unused right now, but it will impact the eMMC.
Let me do a v3.
Andrew
[PATCH v2 5/5] hw/misc: Model KCS devices in the Aspeed LPC controller
Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
IO cycles from the BMC to the host.
Expose support on the BMC side by implementing the usual MMIO
behaviours, and expose the ability to inspect the KCS registers in
"host" style by accessing QOM properties associated with each register.
The model caters to the IRQ style of both the AST2600 and the earlier
SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
sub-device, while there is a single IRQ shared across all subdevices on
the AST2400 and AST2500.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 28 ++-
hw/arm/aspeed_soc.c | 24 ++-
hw/misc/aspeed_lpc.c | 359 ++-
include/hw/arm/aspeed_soc.h | 1 +
include/hw/misc/aspeed_lpc.h | 17 +-
5 files changed, 424 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 60152de001e6..fd463775d281 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_ETH3] = 32,
[ASPEED_DEV_ETH4] = 33,
-
+[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
};
static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
@@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the GIC. It is otherwise unused. */
sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
+ * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
+ * shared across the subdevices, and the shared IRQ output to the VIC is at
+ * offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_4));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4f098da437ac..057d053c8478 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_WDT]= 27,
[ASPEED_DEV_PWM]= 28,
[ASPEED_DEV_LPC]= 8,
-[ASPEED_DEV_IBT]= 8, /* LPC */
[ASPEED_DEV_I2C]= 12,
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
@@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the VIC */
sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
+ * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
+ * contrast, on the AST2600, the subdevice IRQs are connected straight to
+ * the GIC).
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the shared IRQ
output
+ * to the VIC is at offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_4));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/a
[PATCH v2 3/5] hw/arm: ast2600: Correct the iBT interrupt ID
The AST2600 allocates distinct GIC IRQs for the LPC subdevices such as
the iBT device. Previously on the AST2400 and AST2500 the LPC subdevices
shared a single LPC IRQ.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 22fcb5b0edbe..2125a96ef317 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -98,7 +98,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_WDT] = 24,
[ASPEED_DEV_PWM] = 44,
[ASPEED_DEV_LPC] = 35,
-[ASPEED_DEV_IBT] = 35,/* LPC */
+[ASPEED_DEV_IBT] = 143,
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
--
2.27.0
[PATCH v2 4/5] hw/misc: Add a basic Aspeed LPC controller model
From: Cédric Le Goater
This is a very minimal framework to access registers which are used to
configure the AHB memory mapping of the flash chips on the LPC HC
Firmware address space.
Signed-off-by: Cédric Le Goater
Signed-off-by: Andrew Jeffery
---
docs/system/arm/aspeed.rst | 2 +-
hw/arm/aspeed_ast2600.c | 10 +++
hw/arm/aspeed_soc.c | 10 +++
hw/misc/aspeed_lpc.c | 131 +++
hw/misc/meson.build | 7 +-
include/hw/arm/aspeed_soc.h | 2 +
include/hw/misc/aspeed_lpc.h | 32 +
7 files changed, 192 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/aspeed_lpc.c
create mode 100644 include/hw/misc/aspeed_lpc.h
diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 690bada7842b..2f6fa8938d02 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -48,6 +48,7 @@ Supported devices
* UART
* Ethernet controllers
* Front LEDs (PCA9552 on I2C bus)
+ * LPC Peripheral Controller (a subset of subdevices are supported)
Missing devices
@@ -56,7 +57,6 @@ Missing devices
* Coprocessor support
* ADC (out of tree implementation)
* PWM and Fan Controller
- * LPC Bus Controller
* Slave GPIO Controller
* Super I/O Controller
* Hash/Crypto Engine
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 2125a96ef317..60152de001e6 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -211,6 +211,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "emmc-controller.sdhci", >emmc.slots[0],
TYPE_SYSBUS_SDHCI);
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
/*
@@ -469,6 +471,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
sysbus_mmio_map(SYS_BUS_DEVICE(>emmc), 0, sc->memmap[ASPEED_DEV_EMMC]);
sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07a..4f098da437ac 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -211,6 +211,8 @@ static void aspeed_soc_init(Object *obj)
object_initialize_child(obj, "sdhci[*]", >sdhci.slots[i],
TYPE_SYSBUS_SDHCI);
}
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -393,6 +395,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
sc->memmap[ASPEED_DEV_SDHCI]);
sysbus_connect_irq(SYS_BUS_DEVICE(>sdhci), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
new file mode 100644
index ..e668e985ff04
--- /dev/null
+++ b/hw/misc/aspeed_lpc.c
@@ -0,0 +1,131 @@
+/*
+ * ASPEED LPC Controller
+ *
+ * Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_lpc.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#define TO_REG(offset) ((offset) >> 2)
+
+#define HICR0TO_REG(0x00)
+#define HICR1TO_REG(0x04)
+#define HICR2TO_REG(0x08)
+#define HICR3TO_REG(0x0C)
+#define HICR4TO_REG(0x10)
+#define HICR5TO_REG(0x80)
+#define HICR6TO_REG(0x84)
+#define HICR7TO_REG(0x88)
+#define HICR8TO_REG(0x8C)
+
+static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
+{
+AspeedLPCState *s = ASPEED_LPC(opaque);
+int reg = TO_REG(offset);
+
+if (reg >= ARRAY_SIZE(s->regs)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at
[PATCH v2 0/5] aspeed: LPC peripheral controller devices
Hello, This series adds support for some of the LPC[1] peripherals found in Aspeed BMC SoCs. v2 addresses some minor feedback from Philippe and Cédric. v1 can be found here: https://lore.kernel.org/qemu-devel/20210226065758.547824-1-and...@aj.id.au/T/#m28b4392d0672e85fbfaaf6565a2da2e82de1691d BMCs typically provide a number of features to their host via LPC that include but are not limited to: 1. Mapping LPC firmware cycles to BMC-controlled flash devices 2. UART(s) for system console routing 3. POST code routing 4. Keyboard-Controller-Style (KCS) IPMI devices 5. Block Transfer (BT) IPMI devices 6. A SuperIO controller for management of LPC devices and miscellaneous functionality [1] https://www.intel.com/content/dam/www/program/design/us/en/documents/low-pin-count-interface-specification.pdf Specifically, this series adds basic support for functions 1 and 4 above, handling the BMC firmware configuring the bridge mapping LPC firmware cycles onto its AHB as well as support for four KCS devices. Aspeed's LPC controller is not a straight-forward device by any stretch. It contains at least the capabilities outlined above, in the sense that it's not possible to cleanly separate the different functions into distinct MMIO sub-regions: Registers for the various bits of functionality have the feel of arbitrary placement with a nod to feature-creep and backwards compatibility. Further, the conceptually coherent pieces of functionality often come with the ability to issue interrupts, though for the AST2400 and AST2500 there is one shared VIC IRQ for all LPC "subdevices". By contrast the AST2600 gives each subdevice a distinct IRQ via the GIC. All this combined leads to some complexity regarding the interrupts and handling the MMIO accesses (in terms of mapping the access back to the function it's affecting). Finally, as a point of clarity, Aspeed BMCs also contain an LPC Host Controller to drive the LPC bus. This series does not concern itself with the LPC Host Controller function, only with a subset of the peripheral devices the BMC presents to the host. I've tested the series using a combination of the ast2600-evb, witherspoon-bmc and romulus-bmc machines along with a set of recently-posted patches for Linux[2]. Please review! Andrew [2] https://lore.kernel.org/openbmc/20210219142523.3464540-1-and...@aj.id.au/T/#m1e2029e7aa2be3056320e8d46b3b5b1539a776b4 Andrew Jeffery (4): arm: ast2600: Force a multiple of 32 of IRQs for the GIC hw/arm: ast2600: Set AST2600_MAX_IRQ to value from datasheet hw/arm: ast2600: Correct the iBT interrupt ID hw/misc: Model KCS devices in the Aspeed LPC controller Cédric Le Goater (1): hw/misc: Add a basic Aspeed LPC controller model docs/system/arm/aspeed.rst | 2 +- hw/arm/aspeed_ast2600.c | 44 +++- hw/arm/aspeed_soc.c | 34 ++- hw/misc/aspeed_lpc.c | 486 +++ hw/misc/meson.build | 7 +- include/hw/arm/aspeed_soc.h | 3 + include/hw/misc/aspeed_lpc.h | 47 7 files changed, 616 insertions(+), 7 deletions(-) create mode 100644 hw/misc/aspeed_lpc.c create mode 100644 include/hw/misc/aspeed_lpc.h base-commit: 51db2d7cf26d05a961ec0ee0eb773594b32cc4a1 -- 2.27.0
[PATCH v2 2/5] hw/arm: ast2600: Set AST2600_MAX_IRQ to value from datasheet
The datasheet says we have 197 IRQs allocated, and we need more than 128
to describe IRQs from LPC devices. Raise the value now to allow
modelling of the LPC devices.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bc0eeb058b24..22fcb5b0edbe 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 197
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
--
2.27.0
[PATCH v2 1/5] arm: ast2600: Force a multiple of 32 of IRQs for the GIC
This appears to be a requirement of the GIC model. The AST2600 allocates
197 GIC IRQs, which we will adjust shortly.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bf31ca351feb..bc0eeb058b24 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define ASPEED_SOC_AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 128
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
@@ -267,7 +267,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
object_property_set_int(OBJECT(>a7mpcore), "num-cpu", sc->num_cpus,
_abort);
object_property_set_int(OBJECT(>a7mpcore), "num-irq",
-ASPEED_SOC_AST2600_MAX_IRQ + GIC_INTERNAL,
+ROUND_UP(AST2600_MAX_IRQ + GIC_INTERNAL, 32),
_abort);
sysbus_realize(SYS_BUS_DEVICE(>a7mpcore), _abort);
--
2.27.0
Re: [PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC
On Mon, 1 Mar 2021, at 09:37, Andrew Jeffery wrote:
>
>
> On Fri, 26 Feb 2021, at 19:26, Philippe Mathieu-Daudé wrote:
> > On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > > This appears to be a requirement of the GIC model.
> >
> > If so this should be adjusted in the GIC or a15mp_priv_realize(),
> > not in each caller, isn't it?
> >
>
> Maybe, let me look into it. I'll clean it up in v2 if it makes sense.
So the current behaviour has been around since 2009, originating in
41c1e2f54e6f ("arm: make sure that number of irqs can be represented in
GICD_TYPER."). The GIC architecture specification says:
"The GICD_TYPER.ITLinesNumber field identifies the number of
implemented GICD_ISENABLERns, and therefore the maximum number of SPIs
that might be supported."
While the code says:
/* ITLinesNumber is represented as (N / 32) - 1 (see
* gic_dist_readb) so this is an implementation imposed
* restriction, not an architectural one:
*/
if (s->num_irq < 32 || (s->num_irq % 32)) {
error_setg(errp,
"%d interrupt lines unsupported: not divisible by
32",
num_irq);
return;
}
My feeling is that it's better to be explicit in the models that are
affected (i.e. leave the ROUND_UP() as I have it in this patch). This
way if the implementation restriction is ever lifted, we know which
models we can clean up. I won't be reworking the GIC to remove the
restriction in this series, so unless you have a particularly strong
preference/justification for the implicit ROUND_UP(), I plan to leave
it as is.
Cheers,
Andrew
Re: [PATCH 4/4] hw/misc: Model KCS devices in the Aspeed LPC controller
On Fri, 26 Feb 2021, at 20:21, Cédric Le Goater wrote:
> On 2/26/21 7:57 AM, Andrew Jeffery wrote:
> > Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
> > IO cycles from the BMC to the host.
> >
> > Expose support on the BMC side by implementing the usual MMIO
> > behaviours, and expose the ability to inspect the KCS registers in
> > "host" style by accessing QOM properties associated with each register.
> >
> > The model caters to the IRQ style of both the AST2600 and the earlier
> > SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
> > sub-device, while there is a single IRQ shared across all subdevices on
> > the AST2400 and AST2500.
> >
> > Signed-off-by: Andrew Jeffery
> > ---
> > hw/arm/aspeed_ast2600.c | 28 ++-
> > hw/arm/aspeed_soc.c | 24 ++-
> > hw/misc/aspeed_lpc.c | 354 +++
> > include/hw/arm/aspeed_soc.h | 1 +
> > include/hw/misc/aspeed_lpc.h | 17 +-
> > 5 files changed, 421 insertions(+), 3 deletions(-)
> >
> > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> > index 60152de001e6..fd463775d281 100644
> > --- a/hw/arm/aspeed_ast2600.c
> > +++ b/hw/arm/aspeed_ast2600.c
> > @@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> > [ASPEED_DEV_ETH2] = 3,
> > [ASPEED_DEV_ETH3] = 32,
> > [ASPEED_DEV_ETH4] = 33,
> > -
> > +[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
> > };
> >
> > static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
> > @@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState
> > *dev, Error **errp)
> > return;
> > }
> > sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0,
> > sc->memmap[ASPEED_DEV_LPC]);
> > +
> > +/* Connect the LPC IRQ to the GIC. It is otherwise unused. */
> > sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
> > aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> > +
> > +/*
> > + * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
> > + *
> > + * LPC subdevice IRQ sources are offset from 1 because the LPC model
> > caters
> > + * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
> > + * shared across the subdevices, and the shared IRQ output to the VIC
> > is at
> > + * offset 0.
> > + */
> > +sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
> > + qdev_get_gpio_in(DEVICE(>a7mpcore),
> > +sc->irqmap[ASPEED_DEV_KCS] +
> > aspeed_lpc_kcs_1));
> > +
> > +sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
> > + qdev_get_gpio_in(DEVICE(>a7mpcore),
> > +sc->irqmap[ASPEED_DEV_KCS] +
> > aspeed_lpc_kcs_2));
> > +
> > +sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
> > + qdev_get_gpio_in(DEVICE(>a7mpcore),
> > +sc->irqmap[ASPEED_DEV_KCS] +
> > aspeed_lpc_kcs_3));
> > +
> > +sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
> > + qdev_get_gpio_in(DEVICE(>a7mpcore),
> > +sc->irqmap[ASPEED_DEV_KCS] +
> > aspeed_lpc_kcs_4));
> > }
> >
> > static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
> > diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> > index 4f098da437ac..057d053c8478 100644
> > --- a/hw/arm/aspeed_soc.c
> > +++ b/hw/arm/aspeed_soc.c
> > @@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
> > [ASPEED_DEV_WDT]= 27,
> > [ASPEED_DEV_PWM]= 28,
> > [ASPEED_DEV_LPC]= 8,
> > -[ASPEED_DEV_IBT]= 8, /* LPC */
> > [ASPEED_DEV_I2C]= 12,
> > [ASPEED_DEV_ETH1] = 2,
> > [ASPEED_DEV_ETH2] = 3,
> > @@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error
> > **errp)
> > return;
> > }
> > sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0,
> > sc->memmap[ASPEED_DEV_LPC]);
> > +
> > +/* Connect the LPC IRQ to the VIC */
> > sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
> > aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
> > +
> > +/*
Re: [PATCH 2/4] arm: ast2600: Fix iBT IRQ ID
On Fri, 26 Feb 2021, at 19:28, Philippe Mathieu-Daudé wrote: > On 2/26/21 7:57 AM, Andrew Jeffery wrote: > > The AST2600 allocates individual GIC IRQ lines for the LPC sub-devices. > > This is a contrast to the AST2400 and AST2500 which use one shared VIC > > IRQ line for the LPC sub-devices. Switch the iBT device to use the > > GIC IRQ ID documented in the datasheet. > > [*] > > > > > While we're here, set the number of IRQs to the allocated number of IRQs > > in the datasheet. > > Please do one change per patch. This would be the first change, > and [*] is the second. Given that I had to change the current value to support the iBT device I figured it would be fine in the same patch, but sure, I can split this out in v2. Andrew
Re: [PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC
On Fri, 26 Feb 2021, at 19:26, Philippe Mathieu-Daudé wrote: > On 2/26/21 7:57 AM, Andrew Jeffery wrote: > > This appears to be a requirement of the GIC model. > > If so this should be adjusted in the GIC or a15mp_priv_realize(), > not in each caller, isn't it? > Maybe, let me look into it. I'll clean it up in v2 if it makes sense. Cheers, Andrew
[PATCH 4/4] hw/misc: Model KCS devices in the Aspeed LPC controller
Keyboard-Controller-Style devices for IPMI purposes are exposed via LPC
IO cycles from the BMC to the host.
Expose support on the BMC side by implementing the usual MMIO
behaviours, and expose the ability to inspect the KCS registers in
"host" style by accessing QOM properties associated with each register.
The model caters to the IRQ style of both the AST2600 and the earlier
SoCs (AST2400 and AST2500). The AST2600 allocates an IRQ for each LPC
sub-device, while there is a single IRQ shared across all subdevices on
the AST2400 and AST2500.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 28 ++-
hw/arm/aspeed_soc.c | 24 ++-
hw/misc/aspeed_lpc.c | 354 +++
include/hw/arm/aspeed_soc.h | 1 +
include/hw/misc/aspeed_lpc.h | 17 +-
5 files changed, 421 insertions(+), 3 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 60152de001e6..fd463775d281 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -104,7 +104,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_ETH2] = 3,
[ASPEED_DEV_ETH3] = 32,
[ASPEED_DEV_ETH4] = 33,
-
+[ASPEED_DEV_KCS] = 138, /* 138 -> 142 */
};
static qemu_irq aspeed_soc_get_irq(AspeedSoCState *s, int ctrl)
@@ -477,8 +477,34 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the GIC. It is otherwise unused. */
sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2600 LPC subdevice IRQs are connected straight to the GIC.
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the LPC model caters
+ * to the AST2400 and AST2500. SoCs before the AST2600 have one LPC IRQ
+ * shared across the subdevices, and the shared IRQ output to the VIC is at
+ * offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>a7mpcore),
+sc->irqmap[ASPEED_DEV_KCS] +
aspeed_lpc_kcs_4));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 4f098da437ac..057d053c8478 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -112,7 +112,6 @@ static const int aspeed_soc_ast2400_irqmap[] = {
[ASPEED_DEV_WDT]= 27,
[ASPEED_DEV_PWM]= 28,
[ASPEED_DEV_LPC]= 8,
-[ASPEED_DEV_IBT]= 8, /* LPC */
[ASPEED_DEV_I2C]= 12,
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
@@ -401,8 +400,31 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
return;
}
sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+
+/* Connect the LPC IRQ to the VIC */
sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
+
+/*
+ * On the AST2400 and AST2500 the one LPC IRQ is shared between all of the
+ * subdevices. Connect the LPC subdevice IRQs to the LPC controller IRQ (by
+ * contrast, on the AST2600, the subdevice IRQs are connected straight to
+ * the GIC).
+ *
+ * LPC subdevice IRQ sources are offset from 1 because the shared IRQ
output
+ * to the VIC is at offset 0.
+ */
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_1,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_1));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_2,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_2));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_3,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_3));
+
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 1 + aspeed_lpc_kcs_4,
+ qdev_get_gpio_in(DEVICE(>lpc), aspeed_lpc_kcs_4));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/a
[PATCH 3/4] hw/misc: Add a basic Aspeed LPC controller model
From: Cédric Le Goater
This is a very minimal framework to access registers which are used to
configure the AHB memory mapping of the flash chips on the LPC HC
Firmware address space.
Signed-off-by: Cédric Le Goater
Signed-off-by: Andrew Jeffery
---
docs/system/arm/aspeed.rst | 2 +-
hw/arm/aspeed_ast2600.c | 10 +++
hw/arm/aspeed_soc.c | 10 +++
hw/misc/aspeed_lpc.c | 131 +++
hw/misc/meson.build | 7 +-
include/hw/arm/aspeed_soc.h | 2 +
include/hw/misc/aspeed_lpc.h | 32 +
7 files changed, 192 insertions(+), 2 deletions(-)
create mode 100644 hw/misc/aspeed_lpc.c
create mode 100644 include/hw/misc/aspeed_lpc.h
diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index 690bada7842b..2f6fa8938d02 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -48,6 +48,7 @@ Supported devices
* UART
* Ethernet controllers
* Front LEDs (PCA9552 on I2C bus)
+ * LPC Peripheral Controller (a subset of subdevices are supported)
Missing devices
@@ -56,7 +57,6 @@ Missing devices
* Coprocessor support
* ADC (out of tree implementation)
* PWM and Fan Controller
- * LPC Bus Controller
* Slave GPIO Controller
* Super I/O Controller
* Hash/Crypto Engine
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 2125a96ef317..60152de001e6 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -211,6 +211,8 @@ static void aspeed_soc_ast2600_init(Object *obj)
object_initialize_child(obj, "emmc-controller.sdhci", >emmc.slots[0],
TYPE_SYSBUS_SDHCI);
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
/*
@@ -469,6 +471,14 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
sysbus_mmio_map(SYS_BUS_DEVICE(>emmc), 0, sc->memmap[ASPEED_DEV_EMMC]);
sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_EMMC));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>emmc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 7eefd54ac07a..4f098da437ac 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -211,6 +211,8 @@ static void aspeed_soc_init(Object *obj)
object_initialize_child(obj, "sdhci[*]", >sdhci.slots[i],
TYPE_SYSBUS_SDHCI);
}
+
+object_initialize_child(obj, "lpc", >lpc, TYPE_ASPEED_LPC);
}
static void aspeed_soc_realize(DeviceState *dev, Error **errp)
@@ -393,6 +395,14 @@ static void aspeed_soc_realize(DeviceState *dev, Error
**errp)
sc->memmap[ASPEED_DEV_SDHCI]);
sysbus_connect_irq(SYS_BUS_DEVICE(>sdhci), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+
+/* LPC */
+if (!sysbus_realize(SYS_BUS_DEVICE(>lpc), errp)) {
+return;
+}
+sysbus_mmio_map(SYS_BUS_DEVICE(>lpc), 0, sc->memmap[ASPEED_DEV_LPC]);
+sysbus_connect_irq(SYS_BUS_DEVICE(>lpc), 0,
+ aspeed_soc_get_irq(s, ASPEED_DEV_LPC));
}
static Property aspeed_soc_properties[] = {
DEFINE_PROP_LINK("dram", AspeedSoCState, dram_mr, TYPE_MEMORY_REGION,
diff --git a/hw/misc/aspeed_lpc.c b/hw/misc/aspeed_lpc.c
new file mode 100644
index ..e668e985ff04
--- /dev/null
+++ b/hw/misc/aspeed_lpc.c
@@ -0,0 +1,131 @@
+/*
+ * ASPEED LPC Controller
+ *
+ * Copyright (C) 2017-2018 IBM Corp.
+ *
+ * This code is licensed under the GPL version 2 or later. See
+ * the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qemu/log.h"
+#include "qemu/error-report.h"
+#include "hw/misc/aspeed_lpc.h"
+#include "qapi/error.h"
+#include "hw/qdev-properties.h"
+#include "migration/vmstate.h"
+
+#define TO_REG(offset) ((offset) >> 2)
+
+#define HICR0TO_REG(0x00)
+#define HICR1TO_REG(0x04)
+#define HICR2TO_REG(0x08)
+#define HICR3TO_REG(0x0C)
+#define HICR4TO_REG(0x10)
+#define HICR5TO_REG(0x80)
+#define HICR6TO_REG(0x84)
+#define HICR7TO_REG(0x88)
+#define HICR8TO_REG(0x8C)
+
+static uint64_t aspeed_lpc_read(void *opaque, hwaddr offset, unsigned size)
+{
+AspeedLPCState *s = ASPEED_LPC(opaque);
+int reg = TO_REG(offset);
+
+if (reg >= ARRAY_SIZE(s->regs)) {
+qemu_log_mask(LOG_GUEST_ERROR,
+ "%s: Out-of-bounds read at
[PATCH 1/4] arm: ast2600: Force a multiple of 32 of IRQs for the GIC
This appears to be a requirement of the GIC model. The AST2600 allocates
197 GIC IRQs, which we will adjust shortly.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bf31ca351feb..bc0eeb058b24 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define ASPEED_SOC_AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 128
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
@@ -267,7 +267,7 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev,
Error **errp)
object_property_set_int(OBJECT(>a7mpcore), "num-cpu", sc->num_cpus,
_abort);
object_property_set_int(OBJECT(>a7mpcore), "num-irq",
-ASPEED_SOC_AST2600_MAX_IRQ + GIC_INTERNAL,
+ROUND_UP(AST2600_MAX_IRQ + GIC_INTERNAL, 32),
_abort);
sysbus_realize(SYS_BUS_DEVICE(>a7mpcore), _abort);
--
2.27.0
[PATCH 2/4] arm: ast2600: Fix iBT IRQ ID
The AST2600 allocates individual GIC IRQ lines for the LPC sub-devices.
This is a contrast to the AST2400 and AST2500 which use one shared VIC
IRQ line for the LPC sub-devices. Switch the iBT device to use the
GIC IRQ ID documented in the datasheet.
While we're here, set the number of IRQs to the allocated number of IRQs
in the datasheet.
Signed-off-by: Andrew Jeffery
---
hw/arm/aspeed_ast2600.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index bc0eeb058b24..2125a96ef317 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -65,7 +65,7 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
#define ASPEED_A7MPCORE_ADDR 0x4046
-#define AST2600_MAX_IRQ 128
+#define AST2600_MAX_IRQ 197
/* Shared Peripheral Interrupt values below are offset by -32 from datasheet */
static const int aspeed_soc_ast2600_irqmap[] = {
@@ -98,7 +98,7 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_WDT] = 24,
[ASPEED_DEV_PWM] = 44,
[ASPEED_DEV_LPC] = 35,
-[ASPEED_DEV_IBT] = 35,/* LPC */
+[ASPEED_DEV_IBT] = 143,
[ASPEED_DEV_I2C] = 110, /* 110 -> 125 */
[ASPEED_DEV_ETH1] = 2,
[ASPEED_DEV_ETH2] = 3,
--
2.27.0
[PATCH 0/4] aspeed: LPC peripheral controller devices
Hello, This series adds support for some of the LPC[1] peripherals found in Aspeed BMC SoCs. BMCs typically provide a number of features to their host via LPC that include but are not limited to: 1. Mapping LPC firmware cycles to BMC-controlled flash devices 2. UART(s) for system console routing 3. POST code routing 4. Keyboard-Controller-Style (KCS) IPMI devices 5. Block Transfer (BT) IPMI devices 6. A SuperIO controller for management of LPC devices and miscellaneous functionality [1] https://www.intel.com/content/dam/www/program/design/us/en/documents/low-pin-count-interface-specification.pdf Specifically, this series adds basic support for functions 1 and 4 above, handling the BMC firmware configuring the bridge mapping LPC firmware cycles onto its AHB as well as support for four KCS devices. Aspeed's LPC controller is not a straight-forward device by any stretch. It contains at least the capabilities outlined above, in the sense that it's not possible to cleanly separate the different functions into distinct MMIO sub-regions: Registers for the various bits of functionality have the feel of arbitrary placement with a nod to feature-creep and backwards compatibility. Further, the conceptually coherent pieces of functionality often come with the ability to issue interrupts, though for the AST2400 and AST2500 there is one shared VIC IRQ for all LPC "subdevices". By contrast the AST2600 gives each subdevice a distinct IRQ via the GIC. All this combined leads to some complexity regarding the interrupts and handling the MMIO accesses (in terms of mapping the access back to the function it's affecting). Finally, as a point of clarity, Aspeed BMCs also contain an LPC Host Controller to master the LPC bus. This series does not concern itself with the LPC Host Controller function, only with a subset of the peripheral devices the BMC presents to the host. I've tested the series using a combination of the ast2600-evb, witherspoon-bmc and romulus-bmc machines along with a set of recently-posted patches for Linux[2]. Please review! Andrew [2] https://lore.kernel.org/openbmc/20210219142523.3464540-1-and...@aj.id.au/T/#m1e2029e7aa2be3056320e8d46b3b5b1539a776b4 Andrew Jeffery (3): arm: ast2600: Force a multiple of 32 of IRQs for the GIC arm: ast2600: Fix iBT IRQ ID hw/misc: Model KCS devices in the Aspeed LPC controller Cédric Le Goater (1): hw/misc: Add a basic Aspeed LPC controller model docs/system/arm/aspeed.rst | 2 +- hw/arm/aspeed_ast2600.c | 44 +++- hw/arm/aspeed_soc.c | 34 ++- hw/misc/aspeed_lpc.c | 485 +++ hw/misc/meson.build | 7 +- include/hw/arm/aspeed_soc.h | 3 + include/hw/misc/aspeed_lpc.h | 47 7 files changed, 615 insertions(+), 7 deletions(-) create mode 100644 hw/misc/aspeed_lpc.c create mode 100644 include/hw/misc/aspeed_lpc.h base-commit: 51db2d7cf26d05a961ec0ee0eb773594b32cc4a1 -- 2.27.0
Re: [PATCH 4/7] ppc/pnv: Simplify pnv_bmc_create()
On Fri, 29 Jan 2021, at 19:09, Cédric Le Goater wrote: > On 1/28/21 11:40 PM, David Gibson wrote: > > On Thu, Jan 28, 2021 at 08:46:01AM +0100, Cédric Le Goater wrote: > >> On 1/28/21 1:46 AM, Joel Stanley wrote: > >>> On Tue, 26 Jan 2021 at 17:14, Cédric Le Goater wrote: > > and reuse pnv_bmc_set_pnor() to share the setting of the PNOR. > > Signed-off-by: Cédric Le Goater > --- > hw/ppc/pnv_bmc.c | 7 +-- > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/hw/ppc/pnv_bmc.c b/hw/ppc/pnv_bmc.c > index 67ebb16c4d5f..86d16b493539 100644 > --- a/hw/ppc/pnv_bmc.c > +++ b/hw/ppc/pnv_bmc.c > @@ -260,13 +260,8 @@ IPMIBmc *pnv_bmc_create(PnvPnor *pnor) > Object *obj; > > obj = object_new(TYPE_IPMI_BMC_SIMULATOR); > -object_ref(OBJECT(pnor)); > -object_property_add_const_link(obj, "pnor", OBJECT(pnor)); > >>> > >>> I assume it's ok to move the link set to after the realise of the BMC > >>> object? > >> > >> > >> When 2 objects need to be linked, one has to be realized first. > >> I suppose this is why it is allowed but I am not expert in that area. > >> > >> Greg ? > >> > >> That was the case already when defining a "ipmi-bmc-sim" device on the > >> command line. > > > > Well, the other thing here is that the IPMI_BMC_SIMULATOR isn't a > > POWER specific object, and doesn't actually know anything about pnor, > > so it never looks at that property. Do we even need it? > > It does through hiomap_cmd() which handles HIOMAP commands related > to the PNOR. The link was introduced to remove a reference to the > global machine (qdev_get_machine()). The PNOR device is instantiated > at the machine level but conceptually, this is incorrect. > > The PNOR is a device controlled by the BMC and accessed by the host > through a mapping on the LPC FW address space. It used to be controlled > from the host also, through the iLPC2AHB device and mboxes, but these > "doors" were closed sometime ago. Right, so rehashing what Cédric said about the context for the PNOR and IPMI: On PowerNV platforms, the host firmware accesses the data in the PNOR by sending commands over IPMI to the BMC to change the mappings in the LPC FW space. HIOMAP (Host I/O Map)[1] is the name of the little spec/protocol that defines the layout of data in the FW space and the commands and ABI for manipulating the mappings. [1] https://github.com/openbmc/hiomapd/blob/master/Documentation/protocol.md It's all terrible, but IPMI was the most well-trodden path I had at my disposal for improving the security of the (Aspeed) BMCs' hardware configuration for Power platform designs. From BMC reset the host has unrestricted access to the BMC's AHB via bridges on the LPC and PCIe buses until the BMC firmware disables them. Leaving the bridges enabled is not great for security or stability of the BMC firmware, so this meant cooking up some magic to allow the host to write back to the PNOR (owned by the BMC as Cédric mentions above) without exposing the BMC in unacceptable ways. Andrew
Re: [PATCH] hw/arm/aspeed: Map the UART5 device unconditionally
On Wed, 30 Sep 2020, at 19:37, Cédric Le Goater wrote: > On 9/30/20 7:29 AM, Andrew Jeffery wrote: > > > > > > On Fri, 18 Sep 2020, at 02:33, Cédric Le Goater wrote: > >> On 9/17/20 6:57 PM, Philippe Mathieu-Daudé wrote: > >>> On 9/16/20 7:51 AM, Cédric Le Goater wrote: > >>>> On 9/15/20 7:23 PM, Philippe Mathieu-Daudé wrote: > >>>>> ping? > >>>> > >>>> It's reviewed : > >>>> > >>>> > >>>> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200905212415.760452-1-f4...@amsat.org/ > >>>> > >>> > >>> Yes I know :) This is part of my routine to check if a > >>> patch hasn't been confirmed to be queued 1 week after the > >>> last review, to ping the maintainer (because some > >>> automatically flush patches older than 1month in their > >>> mailbox). > >> > >> ooh. That's brutal. > >> > >>>> I will send a PR when I have more patches. > >>> > >>> Ah OK. I didn't know you would keep merging the Aspeed > >>> patches. Since this was a single patch, I thought it would > >>> go via the usual qemu-arm queue from Peter. > >> > >> sure. It could also. Fine with me. I have only three for the > >> moment. > >> > >>> No rush, I just wanted to be sure the patch was not lost. > >>> Also, once a patch is queued, I understand it is the > >>> maintainer responsibility to keep rebasing the patch > >>> queued. > >> > >> yes. I know. I have been taking care of Andrew's ADC patches > >> since 2017 ... cough cough :) > > > > Agh! > > Does that mean "I will work on it !" ? :) I'll see what I can do, I've recently started to rearrange my task queue. Andrew
Re: [PATCH] hw/arm/aspeed: Map the UART5 device unconditionally
On Fri, 18 Sep 2020, at 02:33, Cédric Le Goater wrote: > On 9/17/20 6:57 PM, Philippe Mathieu-Daudé wrote: > > On 9/16/20 7:51 AM, Cédric Le Goater wrote: > >> On 9/15/20 7:23 PM, Philippe Mathieu-Daudé wrote: > >>> ping? > >> > >> It's reviewed : > >> > >> > >> http://patchwork.ozlabs.org/project/qemu-devel/patch/20200905212415.760452-1-f4...@amsat.org/ > >> > > > > Yes I know :) This is part of my routine to check if a > > patch hasn't been confirmed to be queued 1 week after the > > last review, to ping the maintainer (because some > > automatically flush patches older than 1month in their > > mailbox). > > ooh. That's brutal. > > >> I will send a PR when I have more patches. > > > > Ah OK. I didn't know you would keep merging the Aspeed > > patches. Since this was a single patch, I thought it would > > go via the usual qemu-arm queue from Peter. > > sure. It could also. Fine with me. I have only three for the > moment. > > > No rush, I just wanted to be sure the patch was not lost. > > Also, once a patch is queued, I understand it is the > > maintainer responsibility to keep rebasing the patch > > queued. > > yes. I know. I have been taking care of Andrew's ADC patches > since 2017 ... cough cough :) Agh!
Re: [PATCH v2] ftgmac100: fix dblac write test
On Sun, 28 Jun 2020, at 23:56, erik-smit wrote: > The test of the write of the dblac register was testing the old value > instead of the new value. This would accept the write of an invalid value > but subsequently refuse any following valid writes. > > Signed-off-by: erik-smit > --- > Changes since v1: > > Changed %ld to HWADDR_PRIx to fix building on mingw Bit of a nitpick, but the type of the value argument is uint64_t, so shouldn't the result of the expression captured by FTGMAC100_DBLAC_TXDES_SIZE() and FTGMAC100_DBLAC_RXDES_SIZE() be printed with a straight PRIx64 rather than HWADDR_PRIx? Otherwise the change seems sensible, so: Reviewed-by: Andrew Jeffery
Re: [PATCH v2 1/5] hw/i2c/aspeed_i2c: Simplify aspeed_i2c_get_bus()
On Mon, 6 Jul 2020, at 08:11, Philippe Mathieu-Daudé wrote: > All the callers of aspeed_i2c_get_bus() have a AspeedI2CState and > cast it to a DeviceState with DEVICE(), then aspeed_i2c_get_bus() > cast the DeviceState to an AspeedI2CState with ASPEED_I2C()... > > Simplify aspeed_i2c_get_bus() callers by using AspeedI2CState > argument. > > Reviewed-by: Markus Armbruster > Signed-off-by: Philippe Mathieu-Daudé Reviewed-by: Andrew Jeffery
Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second
On Mon, 22 Jun 2020, at 18:13, Philippe Mathieu-Daudé wrote: > On 6/22/20 2:21 AM, Andrew Jeffery wrote: > > On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote: > >> Hi Andrew, > >> > >> On 6/17/20 3:18 AM, Andrew Jeffery wrote: > >>> On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > >>>> The current implementation uses nano-second precision, while > >>>> the watchdog can not be more precise than a micro-second. > >>> > >>> What's the basis for this assertion? It's true for the AST2500 and > >>> AST2600, but > >>> the AST2400 can run the watchdog from either a 1MHz clock source or the > >>> APB > >>> clock (which must be at least 16.5MHz on palmetto). The reset state on the > >>> AST2400 configures the watchdog for the APB clock rate. > >>> > >>> The Linux driver will eventually configure the watchdog for 1MHz mode > >>> regardless so perhaps the AST2400 reset state is a bit of a corner case, > >>> but > >>> I feel the assertion should be watered down a bit? > >> > >> What about this description? > >> > >> "The current implementation uses nano-second precision, but > >> is not more precise than micro-second precision. > >> Simplify by using a micro-second based timer. > >> Rename the timer 'timer_us' to have the unit explicit." > > > > So is this a limitation of QEMUTimer? I was establishing that the hardware > > can > > operate at greater than 1 micro-second precision. > > No, I misread your comment about the AST2400 timer which can run > at more than 1Mhz. > > The QEMUTimer doesn't have a such limitation; this patch > aimed to simplify the code for reviewers, but you proved > it incorrect, so let's disregard it. > > Thanks for your careful review! Ah, great, I was wondering where my misunderstanding was. Thanks for clearing that up. Andrew
Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second
On Wed, 17 Jun 2020, at 13:11, Philippe Mathieu-Daudé wrote: > Hi Andrew, > > On 6/17/20 3:18 AM, Andrew Jeffery wrote: > > On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > >> The current implementation uses nano-second precision, while > >> the watchdog can not be more precise than a micro-second. > > > > What's the basis for this assertion? It's true for the AST2500 and AST2600, > > but > > the AST2400 can run the watchdog from either a 1MHz clock source or the APB > > clock (which must be at least 16.5MHz on palmetto). The reset state on the > > AST2400 configures the watchdog for the APB clock rate. > > > > The Linux driver will eventually configure the watchdog for 1MHz mode > > regardless so perhaps the AST2400 reset state is a bit of a corner case, but > > I feel the assertion should be watered down a bit? > > What about this description? > > "The current implementation uses nano-second precision, but > is not more precise than micro-second precision. > Simplify by using a micro-second based timer. > Rename the timer 'timer_us' to have the unit explicit." So is this a limitation of QEMUTimer? I was establishing that the hardware can operate at greater than 1 micro-second precision. Andrew
Re: [PATCH 7/7] hw/watchdog/wdt_aspeed: Reduce timer precision to micro-second
On Tue, 16 Jun 2020, at 17:21, Philippe Mathieu-Daudé wrote: > The current implementation uses nano-second precision, while > the watchdog can not be more precise than a micro-second. What's the basis for this assertion? It's true for the AST2500 and AST2600, but the AST2400 can run the watchdog from either a 1MHz clock source or the APB clock (which must be at least 16.5MHz on palmetto). The reset state on the AST2400 configures the watchdog for the APB clock rate. The Linux driver will eventually configure the watchdog for 1MHz mode regardless so perhaps the AST2400 reset state is a bit of a corner case, but I feel the assertion should be watered down a bit? Andrew
Re: [PATCH] docs/system: Document Aspeed boards
> > >> + * Hash/Crypto Engine > >> + * PCI-Express 1 Controller > >> + * Graphic Display Controller > >> + * PECI Controller > >> + * MCTP Controller > >> + * Mailbox Controller > >> + * Virtual UART > > > > Uh what is that? :) > > It is the host console. > To explain a little more, a 16550-compatible set of registers are exposed to both the host (via e.g. the LPC bus) and the BMC, but the FIFOs are shared and the transmissions are nothing more than register writes/reads from each side. There's no RS-232 involved, hence "Virtual" I guess. Andrew
Re: [PATCH 04/24] aspeed: Don't create unwanted "ftgmac100", "aspeed-mmi" devices
On Mon, 18 May 2020, at 21:49, Cédric Le Goater wrote: > On 5/18/20 7:03 AM, Markus Armbruster wrote: > > These devices are optional, and controlled by @nb_nics. > > aspeed_soc_ast2600_init() and aspeed_soc_init() create the maximum > > supported number. aspeed_soc_ast2600_realize() and > > aspeed_soc_realize() realize only the wanted number. Works, although > > it can leave unrealized devices hanging around in the QOM composition > > tree. Affects machines ast2500-evb, ast2600-evb, palmetto-bmc, > > romulus-bmc, swift-bmc, tacoma-bmc, and witherspoon-bmc. > > > > Make the init functions create only the wanted ones. Visible in "info > > qom-tree"; here's the change for ast2600-evb: > > > > /machine (ast2600-evb-machine) > >[...] > >/soc (ast2600-a1) > > [...] > > /ftgmac100[0] (ftgmac100) > >/ftgmac100[0] (qemu:memory-region) > > -/ftgmac100[1] (ftgmac100) > > -/ftgmac100[2] (ftgmac100) > > -/ftgmac100[3] (ftgmac100) > > /gpio (aspeed.gpio-ast2600) > > [...] > > /mii[0] (aspeed-mmi) > >/aspeed-mmi[0] (qemu:memory-region) > > -/mii[1] (aspeed-mmi) > > -/mii[2] (aspeed-mmi) > > -/mii[3] (aspeed-mmi) > > /rtc (aspeed.rtc) > > > > I'm not sure creating @nb_nics devices makes sense. How many does the > > physical chip provide? > > The AST2400, AST2500 SoC have 2 macs and the AST2600 has 4. Each machine > define the one it uses, generally MAC0 but the tacoma board uses MAC3. > > Shouldn't the model reflect the real address space independently from > the NIC backends defined on the command line ? That's my feeling too, though I'm not sure what to make of the unrealised devices in the QOM tree. Does it matter? It hasn't bothered me. Andrew
Re: [PATCH] aspeed: Support AST2600A1 silicon revision
On Mon, 4 May 2020, at 19:07, Joel Stanley wrote:
> There are minimal differences from Qemu's point of view between the A0
> and A1 silicon revisions.
>
> As the A1 exercises different code paths in u-boot it is desirable to
> emulate that instead.
>
> Signed-off-by: Joel Stanley
> ---
> hw/arm/aspeed.c | 8
> hw/arm/aspeed_ast2600.c | 6 +++---
> hw/misc/aspeed_scu.c | 11 +--
> include/hw/misc/aspeed_scu.h | 1 +
> 4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 99a0f3fcf36e..91301efab32d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -93,7 +93,7 @@ struct AspeedBoardState {
>
> /* Tacoma hardware value */
> #define TACOMA_BMC_HW_STRAP1 0x
> -#define TACOMA_BMC_HW_STRAP2 0x
> +#define TACOMA_BMC_HW_STRAP2 0x0040
>
> /*
> * The max ram region is for firmwares that scan the address space
> @@ -585,7 +585,7 @@ static void
> aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
> AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>
> mc->desc = "Aspeed AST2600 EVB (Cortex A7)";
> -amc->soc_name = "ast2600-a0";
> +amc->soc_name = "ast2600-a1";
> amc->hw_strap1 = AST2600_EVB_HW_STRAP1;
> amc->hw_strap2 = AST2600_EVB_HW_STRAP2;
> amc->fmc_model = "w25q512jv";
> @@ -600,8 +600,8 @@ static void
> aspeed_machine_tacoma_class_init(ObjectClass *oc, void *data)
> MachineClass *mc = MACHINE_CLASS(oc);
> AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>
> - mc->desc = "Aspeed AST2600 EVB (Cortex A7)";
> -amc->soc_name = "ast2600-a0";
> +mc->desc = "OpenPOWER Tacoma BMC (Cortex A7)";
Lol, whoops.
Reviewed-by: Andrew Jeffery
Re: [PATCH v1] nrf51: Fix last GPIO CNF address
On Fri, 10 Apr 2020, at 21:56, Peter Maydell wrote: > On Fri, 10 Apr 2020 at 04:42, Andrew Jeffery wrote: > > IIRC Phil wanted to enable sub-word accesses to the sample value > > registers but I didn't want to spread logic dealing with different access > > widths through the model. We already had a mechanism to describe the > > model's supported access sizes (as opposed to the valid access sizes > > allowed by hardware) in the `impl` member of the MemoryRegionOps, so > > I was trying to use that, but it didn't work as I needed. > > > > The accesses generated at the point of the guest MMIO need to be > > expanded to the access width supported by the model and then the > > resulting data trimmed again upon returning the data (in the case of a > > read) via the MMIO operation. > > > > So the intent was less about unaligned accesses than enabling models > > implementations that only have to handle certain-sized access widths. > > It happens to help the unaligned access case as well. > > Yeah, we definitely could do with improving things here, it is annoying > to have to write code for handling some of the oddball cases when you > have just one register that allows byte accesses or whatever. > The thing I have in the back of my mind as a concern is that we have > had several "is this a buffer overrun" questions where the answer has > been "it can't be, because the core code doesn't allow a call to the > read/write function for a 4 byte access where the address is not 4-aligned, > so my_byte_array[addr] is always in-bounds". > So if we change the core code we need to make sure we don't > inadvertently remove a restriction that was protecting us from a guest > escape... Oh for sure. The patch was very RFC, as mentioned I just sent it to check whether I was on the right track or off in the weeds, and from there it has hung around in Cedric's tree without much progress. Feels like the time is right to sort the problem out properly, which might mean starting from scratch to make sure we don't miss any of the details. Andrew
Re: [PATCH v1] nrf51: Fix last GPIO CNF address
On Tue, 7 Apr 2020, at 18:20, Peter Maydell wrote: > On Tue, 7 Apr 2020 at 09:45, Joel Stanley wrote: > > On Tue, 7 Apr 2020 at 08:41, Peter Maydell wrote: > > > Do you have a link to this patch, please? I had a quick search through > > > my mailing list articles but couldn't see anything obviously relevant. > > > > There is a reference in this thread: > > > > https://lore.kernel.org/qemu-devel/dd8fc1f7-56d9-4d9f-96a4-0fdcafdc8...@www.fastmail.com/ > > > > The patch is here: > > > > https://lore.kernel.org/qemu-devel/20170630030058.28943-1-and...@aj.id.au/ > > Oh, that's from 2017, no wonder I couldn't find it! Yeah, I never quite got back to finishing it :( It's development was driven by development of the ASPEED ADC model, which I hacked up in the interest of getting the ASPEED SDK booting under qemu (the SDK kernel had an infinite spin waiting for the ADC-ready bit). IIRC Phil wanted to enable sub-word accesses to the sample value registers but I didn't want to spread logic dealing with different access widths through the model. We already had a mechanism to describe the model's supported access sizes (as opposed to the valid access sizes allowed by hardware) in the `impl` member of the MemoryRegionOps, so I was trying to use that, but it didn't work as I needed. The accesses generated at the point of the guest MMIO need to be expanded to the access width supported by the model and then the resulting data trimmed again upon returning the data (in the case of a read) via the MMIO operation. So the intent was less about unaligned accesses than enabling models implementations that only have to handle certain-sized access widths. It happens to help the unaligned access case as well. > > Does somebody who's already reviewed the patch want to summarize > what the effects on devices are -- i.e. what calls the device's read/write > methods used to get if the guest did an unaligned access, including an > unaligned access half off-the-end of the memory region, and what > calls the read/write methods get after the patch ? The patch's commit > message doesn't really describe what it's doing... Honestly any of that information has well left my memory at this point, I'd have to analyse the patch to recover it. I was hoping that my turn-around time would be shorter than 3 years but there hasn't been a shortage of fires to put out in the mean time. Andrew
Re: [PATCH for-5.0] hw/gpio/aspeed_gpio.c: Don't directly include assert.h
On Fri, 3 Apr 2020, at 23:17, Peter Maydell wrote: > Remove a direct include of assert.h -- this is already > provided by qemu/osdep.h, and it breaks our rule that the > first include must always be osdep.h. > > In particular we must get the assert() macro via osdep.h > to avoid compile failures on mingw (see the comment in > osdep.h where we redefine assert() for that platform). > > Signed-off-by: Peter Maydell Acked-by: Andrew Jeffery
Re: [PATCH v1] usb: Add read support for HCIVERSION register to XHCI
On Wed, 1 Apr 2020, at 21:53, Cédric Le Goater wrote: > Hello, > > On 3/31/20 1:02 PM, Philippe Mathieu-Daudé wrote: > > On 3/31/20 11:57 AM, Cameron Esfahani wrote: > >> Philippe - > >> From what I've seen, access size has nothing to do with alignment. > > > > Yes, I was wondering if you were using unaligned accesses. > > > > I *think* the correct fix is in the "memory: Support unaligned accesses on > > aligned-only models" patch from Andrew Jeffery: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg461247.html > > Here is an updated version I have been keeping since : > > > https://github.com/legoater/qemu/commit/d57ac950c4be47a2bafd6c6a96dec2922c2ecd65 > > which breaks qtest : > > microbit-test.c:307:test_nrf51_gpio: assertion failed (actual == > 0x01): (0 == 1) > > I haven't had time to look at this closely but it would be nice to get this > patch merged. Second this! I never had the chance to circle back and fix it up. Andrew
Re: [PATCH 2/2] aspeed/smc: Fix User mode select/unselect scheme
On Thu, 6 Feb 2020, at 21:56, Cédric Le Goater wrote:
> The Aspeed SMC Controller can operate in different modes : Read, Fast
> Read, Write and User modes. When the User mode is configured, it
> selects automatically the SPI slave device until the CE_STOP_ACTIVE
> bit is set to 1. When any other modes are configured the device is
> unselected. The HW logic handles the chip select automatically when
> the flash is accessed through its AHB window.
>
> When configuring the CEx Control Register, the User mode logic to
> select and unselect the slave is incorrect and data corruption can be
> seen on machines using two chips, witherspoon and romulus.
>
> Rework the handler setting the CEx Control Register to fix this issue.
>
> Fixes: 7c1c69bca43c ("ast2400: add SMC controllers (FMC and SPI)")
> Signed-off-by: Cédric Le Goater
Champion!
Reviewed-by: Andrew Jeffery
Re: [PATCH 1/2] aspeed/smc: Add some tracing
On Thu, 6 Feb 2020, at 21:56, Cédric Le Goater wrote: > Signed-off-by: Cédric Le Goater Reviewed-by: Andrew Jeffery
Re: [PATCH 2/2] aspeed/scu: Implement chip ID register
On Tue, 21 Jan 2020, at 12:03, Joel Stanley wrote:
> This returns a fixed but non-zero value for the chip id.
>
> Signed-off-by: Joel Stanley
Reviewed-by: Andrew Jeffery
> ---
> hw/misc/aspeed_scu.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 7108cad8c6a7..19d1780a40da 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -77,6 +77,8 @@
> #define CPU2_BASE_SEG4 TO_REG(0x110)
> #define CPU2_BASE_SEG5 TO_REG(0x114)
> #define CPU2_CACHE_CTRL TO_REG(0x118)
> +#define CHIP_ID0 TO_REG(0x150)
> +#define CHIP_ID1 TO_REG(0x154)
> #define UART_HPLL_CLKTO_REG(0x160)
> #define PCIE_CTRLTO_REG(0x180)
> #define BMC_MMIO_CTRLTO_REG(0x184)
> @@ -115,6 +117,8 @@
> #define AST2600_HW_STRAP2_PROTTO_REG(0x518)
> #define AST2600_RNG_CTRL TO_REG(0x524)
> #define AST2600_RNG_DATA TO_REG(0x540)
> +#define AST2600_CHIP_ID0 TO_REG(0x5B0)
> +#define AST2600_CHIP_ID1 TO_REG(0x5B4)
>
> #define AST2600_CLK TO_REG(0x40)
>
> @@ -182,6 +186,8 @@ static const uint32_t
> ast2500_a1_resets[ASPEED_SCU_NR_REGS] = {
> [CPU2_BASE_SEG1] = 0x8000U,
> [CPU2_BASE_SEG4] = 0x1E60U,
> [CPU2_BASE_SEG5] = 0xC000U,
> + [CHIP_ID0]= 0x1234ABCDU,
> + [CHIP_ID1]= 0xU,
> [UART_HPLL_CLK] = 0x1903U,
> [PCIE_CTRL] = 0x007BU,
> [BMC_DEV_ID] = 0x2402U
> @@ -307,6 +313,8 @@ static void aspeed_ast2500_scu_write(void *opaque,
> hwaddr offset,
> case RNG_DATA:
> case FREE_CNTR4:
> case FREE_CNTR4_EXT:
> +case CHIP_ID0:
> +case CHIP_ID1:
> qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx
> "\n",
>__func__, offset);
> @@ -620,6 +628,8 @@ static void aspeed_ast2600_scu_write(void *opaque,
> hwaddr offset,
> case AST2600_RNG_DATA:
> case AST2600_SILICON_REV:
> case AST2600_SILICON_REV2:
> +case AST2600_CHIP_ID0:
> +case AST2600_CHIP_ID1:
> /* Add read only registers here */
> qemu_log_mask(LOG_GUEST_ERROR,
>"%s: Write to read-only offset 0x%" HWADDR_PRIx
> "\n",
> @@ -648,6 +658,9 @@ static const uint32_t
> ast2600_a0_resets[ASPEED_AST2600_SCU_NR_REGS] = {
> [AST2600_CLK_STOP_CTRL2]= 0xFFF0FFF0,
> [AST2600_SDRAM_HANDSHAKE] = 0x0040, /* SoC completed DRAM
> init */
> [AST2600_HPLL_PARAM]= 0x1000405F,
> +[AST2600_CHIP_ID0] = 0x1234ABCD,
> +[AST2600_CHIP_ID1] = 0x,
Probably should add the explicit trailing 'U' to the constants at some point.
Reviewed-by: Andrew Jeffery
Re: [PATCH 1/2] aspeed/scu: Create separate write callbacks
On Tue, 21 Jan 2020, at 12:03, Joel Stanley wrote: > This splits the common write callback into separate ast2400 and ast2500 > implementations. This makes it clearer when implementing differing > behaviour. > > Signed-off-by: Joel Stanley Reviewed-by: Andrew Jeffery
Re: [PATCH 2/5] hw/arm: ast2600: Wire up the eMMC controller
On Fri, 10 Jan 2020, at 22:26, Cédric Le Goater wrote: > >> + > >> + sysbus_init_child_obj(obj, "emmc", OBJECT(>emmc), sizeof(s->emmc), > >> + TYPE_ASPEED_SDHCI); > >> + > >> + object_property_set_int(OBJECT(>emmc), 1, "num-slots", > >> _abort); > >> + > >> + sysbus_init_child_obj(obj, "emmc[*]", OBJECT(>emmc.slots[0]), > > > > Single block, so use "emmc" instead. > > Andrew, how should we call the objects in the slots ? "sdhci" ? I think that's the right way to go, but maybe we need to rethink the naming at the controller level. Andrew
Re: [PATCH v3 0/4] Expose GT CNTFRQ as a CPU property to support AST2600
On Wed, 18 Dec 2019, at 01:55, Peter Maydell wrote: > On Fri, 13 Dec 2019 at 05:48, Andrew Jeffery wrote: > > > > Hello, > > > > This is a v3 of the belated follow-up from a few of my earlier attempts to > > fix > > up the ARM generic timer for correct behaviour on the ASPEED AST2600 SoC. > > The > > AST2600 clocks the generic timer at the rate of HPLL, which is configured to > > 1125MHz. This is significantly quicker than the currently hard-coded > > generic > > timer rate of 62.5MHz and so we see "sticky" behaviour in the guest. > > > > v2 can be found here: > > > > https://patchwork.ozlabs.org/cover/1203474/ > > > > Changes since v2: > > > > * Address some minor review comments from Philippe and add tags > > > > Changes since v1: > > > > * Fix a user mode build failure from partial renaming of > > gt_cntfrq_period_ns() > > * Add tags from Cedric and Richard > > > > Please review. > > > > Andrew > > > > Andrew Jeffery (4): > > target/arm: Remove redundant scaling of nexttick > > target/arm: Abstract the generic timer frequency > > target/arm: Prepare generic timer for per-platform CNTFRQ > > ast2600: Configure CNTFRQ at 1125MHz > > > > > Applied to target-arm.next, thanks. Thanks for your feedback throughout. Andrew
Re: [PATCH v2 0/2] hw/arm: ast2600: Wire up eMMC controller
On Fri, 13 Dec 2019, at 18:03, Cédric Le Goater wrote: > On 13/12/2019 05:28, Andrew Jeffery wrote: > > Hello, > > > > The AST2600 has an additional SDHCI intended for use as an eMMC boot source. > > These two patches rework the existing ASPEED SDHCI model to accommodate the > > single-slot nature of the eMMC controller and wire it into the AST2600 SoC. > > > > v2 contains some minor refactorings in response to issues pointed out by > > Cedric. > > > I think these patches are based on mainline. I fixed them locally on > my aspeed 5.0 branch and I plan to send them along with other aspeed > changes in the 5.0 timeframe. Yeah, they're based on Peter's tree. I'll base future patches on yours. Andrew
[PATCH v3 4/4] ast2600: Configure CNTFRQ at 1125MHz
This matches the configuration set by u-boot on the AST2600. Signed-off-by: Andrew Jeffery Reviewed-by: Richard Henderson Reviewed-by: Cédric Le Goater Reviewed-by: Philippe Mathieu-Daudé --- hw/arm/aspeed_ast2600.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index 931887ac681f..5aecc3b3caec 100644 --- a/hw/arm/aspeed_ast2600.c +++ b/hw/arm/aspeed_ast2600.c @@ -259,6 +259,9 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp) object_property_set_int(OBJECT(>cpu[i]), aspeed_calc_affinity(i), "mp-affinity", _abort); +object_property_set_int(OBJECT(>cpu[i]), 112500, "cntfrq", +_abort); + /* * TODO: the secondary CPUs are started and a boot helper * is needed when using -kernel -- git-series 0.9.1
