On Mon, Jan 31, 2022 at 9:52 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Tue, 25 Jan 2022 19:09:25 -0800
> Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
> > Unlike the decoder enumeration for "root decoders" described by platform
> > firmware, standard coders can be enumerated from the component registers
> > space once the base address has been identified (via PCI, ACPI, or
> > another mechanism).
> >
> > Add common infrastructure for HDM (Host-managed-Device-Memory) Decoder
> > enumeration and share it between host-bridge, upstream switch port, and
> > cxl_test defined decoders.
> >
> > The locking model for switch level decoders is to hold the port lock
> > over the enumeration. This facilitates moving the dport and decoder
> > enumeration to a 'port' driver. For now, the only enumerator of decoder
> > resources is the cxl_acpi root driver.
> >
> > [ben: fixup kdoc]
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> Mostly looks nice. A couple of queries inline.
>
> Jonathan
>
> > ---
> > Changes since v3:
> > - Fixup kdoc for devm_cxl_enumerate_decoders() (Ben)
> > - Cleanup a sparse warning around __iomem usage (Ben)
> >
> > drivers/cxl/acpi.c | 43 ++-----
> > drivers/cxl/core/Makefile | 1
> > drivers/cxl/core/core.h | 2
> > drivers/cxl/core/hdm.c | 248 +++++++++++++++++++++++++++++++++++++++++
> > drivers/cxl/core/port.c | 57 +++++++--
> > drivers/cxl/core/regs.c | 5 -
> > drivers/cxl/cxl.h | 33 ++++-
> > drivers/cxl/cxlmem.h | 8 +
> > tools/testing/cxl/Kbuild | 4 +
> > tools/testing/cxl/test/cxl.c | 29 +++++
> > tools/testing/cxl/test/mock.c | 50 ++++++++
> > tools/testing/cxl/test/mock.h | 3
> > 12 files changed, 434 insertions(+), 49 deletions(-)
> > create mode 100644 drivers/cxl/core/hdm.c
> >
> > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > index 259441245687..8c2ced91518b 100644
> > --- a/drivers/cxl/acpi.c
> > +++ b/drivers/cxl/acpi.c
> > @@ -168,10 +168,10 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > struct device *host = root_port->dev.parent;
> > struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > struct acpi_pci_root *pci_root;
> > - int single_port_map[1], rc;
> > - struct cxl_decoder *cxld;
> > struct cxl_dport *dport;
> > + struct cxl_hdm *cxlhdm;
> > struct cxl_port *port;
> > + int rc;
> >
> > if (!bridge)
> > return 0;
> > @@ -200,37 +200,24 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > rc = devm_cxl_port_enumerate_dports(host, port);
> > if (rc < 0)
> > return rc;
> > - if (rc > 1)
> > - return 0;
> > -
> > - /* TODO: Scan CHBCR for HDM Decoder resources */
> > -
> > - /*
> > - * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
> > - * Structure) single ported host-bridges need not publish a decoder
> > - * capability when a passthrough decode can be assumed, i.e. all
> > - * transactions that the uport sees are claimed and passed to the single
> > - * dport. Disable the range until the first CXL region is enumerated /
> > - * activated.
> > - */
> > - cxld = cxl_switch_decoder_alloc(port, 1);
> > - if (IS_ERR(cxld))
> > - return PTR_ERR(cxld);
> > -
> > cxl_device_lock(&port->dev);
> > - dport = list_first_entry(&port->dports, typeof(*dport), list);
> > - cxl_device_unlock(&port->dev);
> > + if (rc == 1) {
> > + rc = devm_cxl_add_passthrough_decoder(host, port);
> > + goto out;
> > + }
> >
> > - single_port_map[0] = dport->port_id;
> > + cxlhdm = devm_cxl_setup_hdm(host, port);
> > + if (IS_ERR(cxlhdm)) {
> > + rc = PTR_ERR(cxlhdm);
> > + goto out;
> > + }
> >
> > - rc = cxl_decoder_add(cxld, single_port_map);
> > + rc = devm_cxl_enumerate_decoders(host, cxlhdm);
> > if (rc)
> > - put_device(&cxld->dev);
> > - else
> > - rc = cxl_decoder_autoremove(host, cxld);
> > + dev_err(&port->dev, "Couldn't enumerate decoders (%d)\n", rc);
> >
> > - if (rc == 0)
> > - dev_dbg(host, "add: %s\n", dev_name(&cxld->dev));
> > +out:
> > + cxl_device_unlock(&port->dev);
> > return rc;
> > }
> >
>
> ...
>
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > new file mode 100644
> > index 000000000000..fd9782269c56
> > --- /dev/null
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -0,0 +1,248 @@
>
>
> ...
>
> > +
> > +static void __iomem *map_hdm_decoder_regs(struct cxl_port *port,
> > + void __iomem *crb)
> > +{
> > + struct cxl_register_map map;
> > + struct cxl_component_reg_map *comp_map = &map.component_map;
>
> Why can't we use a cxl_register_map directly in here?
> Doesn't seem to make use of the containing structure.
Yeah, I don't see a reason for cxl_register_map to be used here since
that was built for cxl_find_regblock(). The cxl_find_regblock() work
was already done.
>
> > +
> > + cxl_probe_component_regs(&port->dev, crb, comp_map);
> > + if (!comp_map->hdm_decoder.valid) {
> > + dev_err(&port->dev, "HDM decoder registers invalid\n");
> > + return IOMEM_ERR_PTR(-ENXIO);
> > + }
> > +
> > + return crb + comp_map->hdm_decoder.offset;
> > +}
> > +
> > +/**
> > + * devm_cxl_setup_hdm - map HDM decoder component registers
> > + * @port: cxl_port to map
> > + */
> > +struct cxl_hdm *devm_cxl_setup_hdm(struct device *host, struct cxl_port *port)
>
> Mentioned this in earlier reply, but good to keep docs in sync with
> code even if going to change it shortly.
Yeah, I meant to fix that up, looks like I didn't commit the hunk on the resend.
