[U-Boot] [PATCH 026/126] pci: sandbox: Move the emulators into their own node

[Bin Meng <bmeng.cn@gmail.com>]

On Sat, Oct 5, 2019 at 1:03 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Wed, Sep 25, 2019 at 10:58 PM Simon Glass <sjg@chromium.org> wrote:
> >
> > Sandbox i2c works using emulation drivers which are currently children of
>
> pci
>
> > the i2c device:
>
> pci

Fixed these, and

>
> >
> >         pci-controller {
> >                 pci at 1f,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0xf800 0 0 0 0>;
> >                         emul at 1f,0 {
> >                                 compatible = "sandbox,swap-case";
> >                         };
> >                 };
> >         };
> >
> > In this case the emulation device is attached to pci device on address
> > f800 (device 1f, function 0) and provides the swap-case functionality.
> >
> > However this is not ideal, since every device on a PCI bus has a child
> > device. This is only really the case for sandbox, but we want to avoid
> > special-case code for sandbox.
> >
> > Worse, child devices cannot be probed before their parents. This forces
> > us to use 'find' rather than 'get' to obtain the emulator device. In fact
> > the emulator devices are never probed. There is code in
> > sandbox_pci_emul_post_probe() which tries to track when emulators are
> > active, but at present this does not work.
> >
> > A better approach seems to be to add a separate node elsewhere in the
> > device tree, an 'emulation parent'. This could be given a bogus address
> > (such as -1) to hide the emulators away from the 'pci' command, but it
> > seems better to keep it at the root node to avoid such hacks.
> >
> > Then we can use a phandle to point from the  evice to the correct
>
> typo: device

Fixed this, and

>
>
> > emulator, and only on sandbox. The code to find an emulator does not
> > interfere with normal pci operation.
> >
> > Add a new UCLASS_PCI_EMUL_PARENT uclass which allows finding an emulator
> > given a bus, and finding a bus given an emulator. Update the existing
> > device trees and the code for finding an emulator.
> >
> > This brings PCI emulators more into line with I2C.
> >
> > Signed-off-by: Simon Glass <sjg@chromium.org>
> > ---
> >
> >  arch/sandbox/dts/sandbox.dtsi | 11 +++++++---
> >  arch/sandbox/dts/test.dts     | 38 +++++++++++++++++++++++------------
> >  doc/driver-model/pci-info.rst | 23 +++++++++++----------
> >  drivers/pci/pci-emul-uclass.c | 37 ++++++++++++++++++++++++++++------
> >  include/dm/uclass-id.h        |  1 +
> >  5 files changed, 77 insertions(+), 33 deletions(-)
> >
> > diff --git a/arch/sandbox/dts/sandbox.dtsi b/arch/sandbox/dts/sandbox.dtsi
> > index c6d5650c20b..f09bc70b0da 100644
> > --- a/arch/sandbox/dts/sandbox.dtsi
> > +++ b/arch/sandbox/dts/sandbox.dtsi
> > @@ -103,9 +103,14 @@
> >                 pci at 1f,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0xf800 0 0 0 0>;
> > -                       emul at 1f,0 {
> > -                               compatible = "sandbox,swap-case";
> > -                       };
> > +                       sandbox,emul = <&swap_case_emul>;
> > +               };
> > +       };
> > +
> > +       emul {
> > +               compatible = "sandbox,pci-emul-parent";
> > +               swap_case_emul: emul at 1f,0 {
> > +                       compatible = "sandbox,swap-case";
> >                 };
> >         };
> >
> > diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
> > index 5669ede7051..a2e75981f0b 100644
> > --- a/arch/sandbox/dts/test.dts
> > +++ b/arch/sandbox/dts/test.dts
> > @@ -452,24 +452,31 @@
> >                 pci at 0,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0x0000 0 0 0 0>;
> > -                       emul at 0,0 {
> > -                               compatible = "sandbox,swap-case";
> > -                       };
> > +                       sandbox,emul = <&swap_case_emul0>;
>
> &swap_case_emul0_0
>
> >                 };
> >                 pci at 1,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0x0800 0 0 0 0>;
> > -                       emul at 0,0 {
> > -                               compatible = "sandbox,swap-case";
> > -                               use-ea;
> > -                       };
> > +                       sandbox,emul = <&swap_case_emul1>;
>
> &swap_case_emul0_1
>
> >                 };
> >                 pci at 1f,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0xf800 0 0 0 0>;
> > -                       emul at 1f,0 {
> > -                               compatible = "sandbox,swap-case";
> > -                       };
> > +                       sandbox,emul = <&swap_case_emul1f>;
>
> &swap_case_emul0_1f

Fixed the label names, and

>
> > +               };
> > +       };
> > +
> > +       pci-emul0 {
> > +               compatible = "sandbox,pci-emul-parent";
> > +               swap_case_emul0: emul at 0,0 {
> > +                       compatible = "sandbox,swap-case";
> > +               };
> > +               swap_case_emul1: emul at 1,0 {
> > +                       compatible = "sandbox,swap-case";
> > +                       use-ea;
> > +               };
> > +               swap_case_emul1f: emul at 1f,0 {
> > +                       compatible = "sandbox,swap-case";
> >                 };
> >         };
> >
> > @@ -499,9 +506,14 @@
> >                 pci at 1f,0 {
> >                         compatible = "pci-generic";
> >                         reg = <0xf800 0 0 0 0>;
> > -                       emul at 1f,0 {
> > -                               compatible = "sandbox,swap-case";
> > -                       };
> > +                       sandbox,emul = <&swap_case_emul2_1f>;
> > +               };
> > +       };
> > +
> > +       pci-emul2 {
> > +               compatible = "sandbox,pci-emul-parent";
> > +               swap_case_emul2_1f: emul2 at 1f,0 {
> > +                       compatible = "sandbox,swap-case";
> >                 };
> >         };
> >
> > diff --git a/doc/driver-model/pci-info.rst b/doc/driver-model/pci-info.rst
> > index d93ab8b61d5..f39ff990a67 100644
> > --- a/doc/driver-model/pci-info.rst
> > +++ b/doc/driver-model/pci-info.rst
> > @@ -113,14 +113,17 @@ Sandbox
> >  -------
> >
> >  With sandbox we need a device emulator for each device on the bus since there
> > -is no real PCI bus. This works by looking in the device tree node for a
> > -driver. For example::
> > -
> > +is no real PCI bus. This works by looking in the device tree node for an
> > +emulator driver. For example::
> >
> >         pci at 1f,0 {
> >                 compatible = "pci-generic";
> >                 reg = <0xf800 0 0 0 0>;
> > -               emul at 1f,0 {
> > +               sandbox,emul = <&emul_1f>;
> > +       };
> > +       pci-emul {
> > +               compatible = "sandbox,pci-emul-parent";
> > +               emul_1f: emul at 1f,0 {
> >                         compatible = "sandbox,swap-case";
> >                 };
> >         };
> > @@ -130,14 +133,16 @@ Note that the first cell in the 'reg' value is the bus/device/function. See
> >  PCI_BDF() for the encoding (it is also specified in the IEEE Std 1275-1994
> >  PCI bus binding document, v2.1)
> >
> > +The pci-emul node should go outside the pci bus node, since otherwise it will
> > +be scanned as a PCI device, causing confusion.
> > +
> >  When this bus is scanned we will end up with something like this::
> >
> >     `- * pci-controller @ 05c660c8, 0
> >      `-   pci at 1f,0 @ 05c661c8, 63488
> > -     `-   emul at 1f,0 @ 05c662c8
> > +   `-   emul at 1f,0 @ 05c662c8
> >
> > -When accesses go to the pci at 1f,0 device they are forwarded to its child, the
> > -emulator.
> > +When accesses go to the pci at 1f,0 device they are forwarded to its emulator.
> >
> >  The sandbox PCI drivers also support dynamic driver binding, allowing device
> >  driver to declare the driver binding information via U_BOOT_PCI_DEVICE(),
> > @@ -164,7 +169,3 @@ When this bus is scanned we will end up with something like this::
> >   pci        [ + ]   pci_sandbo  |-- pci-controller1
> >   pci_emul   [   ]   sandbox_sw  |   |-- sandbox_swap_case_emul
> >   pci_emul   [   ]   sandbox_sw  |   `-- sandbox_swap_case_emul
> > -
> > -Note the difference from the statically declared device nodes is that the
> > -device is directly attached to the host controller, instead of via a container
> > -device like pci at 1f,0.
> > diff --git a/drivers/pci/pci-emul-uclass.c b/drivers/pci/pci-emul-uclass.c
> > index 38227583547..f918e9a52fd 100644
> > --- a/drivers/pci/pci-emul-uclass.c
> > +++ b/drivers/pci/pci-emul-uclass.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/libfdt.h>
> >  #include <pci.h>
> >  #include <dm/lists.h>
> > +#include <dm/uclass-internal.h>
> >
> >  struct sandbox_pci_emul_priv {
> >         int dev_count;
> > @@ -30,13 +31,15 @@ int sandbox_pci_get_emul(struct udevice *bus, pci_dev_t find_devfn,
> >         }
> >         *containerp = dev;
> >
> > -       if (device_get_uclass_id(dev) == UCLASS_PCI_GENERIC) {
> > -               ret = device_find_first_child(dev, emulp);
> > -               if (ret)
> > -                       return ret;
> > -       } else {
> > +       /*
> > +        * TODO(sjg at chromium.org): This code needs a comment as I'm not sure
> > +        * why UCLASS_PCI_GENERIC devices end up being their own emulators. I
> > +        * left this code as is.
> > +        */
>
> This code comes from:
>
> commit 4345998ae9dfad7ba0beb54ad4322134557504a9
> Author: Bin Meng <bmeng.cn@gmail.com>
> Date:   Fri Aug 3 01:14:45 2018 -0700
>
>     pci: sandbox: Support dynamically binding device driver
>
> I think the comments can be removed :)

Updated the comment to mention commit
4345998ae9dfad7ba0beb54ad4322134557504a9, and

>
> > +       ret = uclass_find_device_by_phandle(UCLASS_PCI_EMUL, dev,
> > +                                           "sandbox,emul", emulp);
> > +       if (ret && device_get_uclass_id(dev) != UCLASS_PCI_GENERIC)
> >                 *emulp = dev;
> > -       }
> >
> >         return *emulp ? 0 : -ENODEV;
> >  }
> > @@ -68,3 +71,25 @@ UCLASS_DRIVER(pci_emul) = {
> >         .pre_remove     = sandbox_pci_emul_pre_remove,
> >         .priv_auto_alloc_size   = sizeof(struct sandbox_pci_emul_priv),
> >  };
> > +
> > +/*
> > + * This uclass is a child of the pci bus. Its platdata is not defined here so
> > + * is defined by its parent, UCLASS_PCI, which uses struct pci_child_platdata.
> > + * See per_child_platdata_auto_alloc_size in UCLASS_DRIVER(pci).
> > + */
> > +UCLASS_DRIVER(pci_emul_parent) = {
> > +       .id             = UCLASS_PCI_EMUL_PARENT,
> > +       .name           = "pci_emul_parent",
> > +       .post_bind      = dm_scan_fdt_dev,
> > +};
> > +
> > +static const struct udevice_id pci_emul_parent_ids[] = {
> > +       { .compatible = "sandbox,pci-emul-parent" },
> > +       { }
> > +};
> > +
> > +U_BOOT_DRIVER(pci_emul_parent_drv) = {
> > +       .name           = "pci_emul_parent_drv",
> > +       .id             = UCLASS_PCI_EMUL_PARENT,
> > +       .of_match       = pci_emul_parent_ids,
> > +};
> > diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> > index d4d96106b37..f431f3bf294 100644
> > --- a/include/dm/uclass-id.h
> > +++ b/include/dm/uclass-id.h
> > @@ -23,6 +23,7 @@ enum uclass_id {
> >         UCLASS_I2C_EMUL,        /* sandbox I2C device emulator */
> >         UCLASS_I2C_EMUL_PARENT, /* parent for I2C device emulators */
> >         UCLASS_PCI_EMUL,        /* sandbox PCI device emulator */
> > +       UCLASS_PCI_EMUL_PARENT, /* parent for PCI device emulators */
> >         UCLASS_USB_EMUL,        /* sandbox USB bus device emulator */
> >         UCLASS_AXI_EMUL,        /* sandbox AXI bus device emulator */
> >
> > --
>
> Other than the issues above,
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> Tested-by: Bin Meng <bmeng.cn@gmail.com>

applied to u-boot-x86/next, thanks!