[PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

[PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

[Nam Cao]

Set link width to x1 and link speed to 2.5 Gb/s as specified by the
datasheet. Without this, these fields in the link status register read
zero, which is incorrect.

This problem appeared since 3d67447fe7c2 ("pcie: Fill PCIESlot link fields
to support higher speeds and widths"), which allows PCIe slot to set link
width and link speed. However, if PCIe slot does not explicitly set these
properties, they will be zero. Before this commit, the width and speed
default to x1 and 2.5 Gb/s.

Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher speeds and widths")
Signed-off-by: Nam Cao <namcao@linutronix.de>
---
 hw/pci-bridge/xio3130_downstream.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
index 38a2361fa2..d949431191 100644
--- a/hw/pci-bridge/xio3130_downstream.c
+++ b/hw/pci-bridge/xio3130_downstream.c
@@ -172,10 +172,18 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
     device_class_set_props(dc, xio3130_downstream_props);
 }
 
+static void xio3130_downstream_instance_post_init(Object *obj)
+{
+    PCIESlot *s = PCIE_SLOT(obj);
+    s->speed = QEMU_PCI_EXP_LNK_2_5GT;
+    s->width = QEMU_PCI_EXP_LNK_X1;
+}
+
 static const TypeInfo xio3130_downstream_info = {
     .name          = TYPE_XIO3130_DOWNSTREAM,
     .parent        = TYPE_PCIE_SLOT,
     .class_init    = xio3130_downstream_class_init,
+    .instance_post_init = xio3130_downstream_instance_post_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_PCIE_DEVICE },
         { }
-- 
2.39.2

Re: [PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

[Philippe Mathieu-Daudé]

Hi Nam,

On 29/5/24 15:21, Nam Cao wrote:
> Set link width to x1 and link speed to 2.5 Gb/s as specified by the
> datasheet. Without this, these fields in the link status register read
> zero, which is incorrect.
> 
> This problem appeared since 3d67447fe7c2 ("pcie: Fill PCIESlot link fields
> to support higher speeds and widths"), which allows PCIe slot to set link
> width and link speed. However, if PCIe slot does not explicitly set these
> properties, they will be zero. Before this commit, the width and speed
> default to x1 and 2.5 Gb/s.
> 
> Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher speeds and widths")
> Signed-off-by: Nam Cao <namcao@linutronix.de>
> ---
>   hw/pci-bridge/xio3130_downstream.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> index 38a2361fa2..d949431191 100644
> --- a/hw/pci-bridge/xio3130_downstream.c
> +++ b/hw/pci-bridge/xio3130_downstream.c
> @@ -172,10 +172,18 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
>       device_class_set_props(dc, xio3130_downstream_props);
>   }
>   
> +static void xio3130_downstream_instance_post_init(Object *obj)
> +{
> +    PCIESlot *s = PCIE_SLOT(obj);
> +    s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> +    s->width = QEMU_PCI_EXP_LNK_X1;

- You ignore previous value and overwrite
- You don't warn the user about missing config value

Is post_init() the correct way to deal with that? Usually
this is done in realize(), where you can set *errp to
notify the user of invalid values.

> +}
> +
>   static const TypeInfo xio3130_downstream_info = {
>       .name          = TYPE_XIO3130_DOWNSTREAM,
>       .parent        = TYPE_PCIE_SLOT,
>       .class_init    = xio3130_downstream_class_init,
> +    .instance_post_init = xio3130_downstream_instance_post_init,
>       .interfaces = (InterfaceInfo[]) {
>           { INTERFACE_PCIE_DEVICE },
>           { }

Re: [PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

[Nam Cao]

On Wed, May 29, 2024 at 03:50:14PM +0200, Philippe Mathieu-Daudé wrote:
> On 29/5/24 15:21, Nam Cao wrote:
> > Set link width to x1 and link speed to 2.5 Gb/s as specified by the
> > datasheet. Without this, these fields in the link status register read
> > zero, which is incorrect.
> > 
> > This problem appeared since 3d67447fe7c2 ("pcie: Fill PCIESlot link fields
> > to support higher speeds and widths"), which allows PCIe slot to set link
> > width and link speed. However, if PCIe slot does not explicitly set these
> > properties, they will be zero. Before this commit, the width and speed
> > default to x1 and 2.5 Gb/s.
> > 
> > Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher speeds and widths")
> > Signed-off-by: Nam Cao <namcao@linutronix.de>
> > ---
> >   hw/pci-bridge/xio3130_downstream.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> > 
> > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> > index 38a2361fa2..d949431191 100644
> > --- a/hw/pci-bridge/xio3130_downstream.c
> > +++ b/hw/pci-bridge/xio3130_downstream.c
> > @@ -172,10 +172,18 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
> >       device_class_set_props(dc, xio3130_downstream_props);
> >   }
> > +static void xio3130_downstream_instance_post_init(Object *obj)
> > +{
> > +    PCIESlot *s = PCIE_SLOT(obj);
> > +    s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> > +    s->width = QEMU_PCI_EXP_LNK_X1;
> 
> - You ignore previous value and overwrite
> - You don't warn the user about missing config value

Users cannot set speed/width for this device. At the moment, only PCI root
port supports that.

It wouldn't make sense for users to set these properties nonetheless, these
properties are fixed according to datasheet.

> Is post_init() the correct way to deal with that? Usually
> this is done in realize(), where you can set *errp to
> notify the user of invalid values.

I stole this from hw/pci-bridge/cxl_downstream.c and
hw/pci-bridge/pcie_root_port.c. So I think it's correct.

I am new to QEMU code, so please tell me if you think this is not the best
approach.

Best regards,
Nam

Re: [PATCH] pci-bridge/xio3130_downstream: fix invalid link speed and link width

[Nam Cao]

On Wed, May 29, 2024 at 04:17:04PM +0200, Nam Cao wrote:
> On Wed, May 29, 2024 at 03:50:14PM +0200, Philippe Mathieu-Daudé wrote:
> > On 29/5/24 15:21, Nam Cao wrote:
> > > Set link width to x1 and link speed to 2.5 Gb/s as specified by the
> > > datasheet. Without this, these fields in the link status register read
> > > zero, which is incorrect.
> > > 
> > > This problem appeared since 3d67447fe7c2 ("pcie: Fill PCIESlot link fields
> > > to support higher speeds and widths"), which allows PCIe slot to set link
> > > width and link speed. However, if PCIe slot does not explicitly set these
> > > properties, they will be zero. Before this commit, the width and speed
> > > default to x1 and 2.5 Gb/s.
> > > 
> > > Fixes: 3d67447fe7c2 ("pcie: Fill PCIESlot link fields to support higher speeds and widths")
> > > Signed-off-by: Nam Cao <namcao@linutronix.de>
> > > ---
> > >   hw/pci-bridge/xio3130_downstream.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3130_downstream.c
> > > index 38a2361fa2..d949431191 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -172,10 +172,18 @@ static void xio3130_downstream_class_init(ObjectClass *klass, void *data)
> > >       device_class_set_props(dc, xio3130_downstream_props);
> > >   }
> > > +static void xio3130_downstream_instance_post_init(Object *obj)
> > > +{
> > > +    PCIESlot *s = PCIE_SLOT(obj);
> > > +    s->speed = QEMU_PCI_EXP_LNK_2_5GT;
> > > +    s->width = QEMU_PCI_EXP_LNK_X1;
> > 
> > - You ignore previous value and overwrite
> > - You don't warn the user about missing config value
> 
> Users cannot set speed/width for this device. At the moment, only PCI root
> port supports that.
> 
> It wouldn't make sense for users to set these properties nonetheless, these
> properties are fixed according to datasheet.
> 
> > Is post_init() the correct way to deal with that? Usually
> > this is done in realize(), where you can set *errp to
> > notify the user of invalid values.
> 
> I stole this from hw/pci-bridge/cxl_downstream.c and
> hw/pci-bridge/pcie_root_port.c. So I think it's correct.

Ah wait, now I read those files again, I think those devices do it this way
because they want to have default values.

For this case, I think you are right that realize() is the more suitable
place because I am not setting the default, I am setting the fixed values.

I can send a v2 with that change, unless you have other comments?

Best regards,
Nam

> I am new to QEMU code, so please tell me if you think this is not the best
> approach.
> 
> Best regards,
> Nam
>