Re: [RFC PATCH v2 09/28] cxl/acpi: Map single port host bridge component registers

[Ben Widawsky <ben.widawsky@intel.com>]

On 21-11-01 19:15:33, Dan Williams wrote:
> On Mon, Nov 1, 2021 at 10:08 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> >
> > On 21-10-31 11:03:37, Dan Williams wrote:
> > > On Fri, Oct 22, 2021 at 11:37 AM Ben Widawsky <ben.widawsky@intel.com> wrote:
> > > >
> > > > Now that the port driver exists and is able to do proper decoder
> > > > enumeration of the component registers, it becomes trivial to use that
> > >
> > > This is the second occurrence of "becomes trivial" in this series,
> > > it's distracting because it immediately sets off alarm bells that the
> > > changelog is not in:
> > >
> > > Background
> > > Problem
> > > Solution
> > >
> > > ...format.
> >
> > Let me make sure I understand.
> >
> > Background: Now that the port driver exists...
> > Problem: host bridge uports can use the port driver [but don't yet]
> > Solution: This patch (the description is indeed missing).
> >
> > Is your point that I didn't document a solution, or something else?
> 
> Why do single port host bridge registers need to be mapped?
> 
> Why does the cxl_acpi driver need to do it and not the just introduced
> port driver?
> 
> This patch seems to be saying, map these because they can be mapped.
> 
> >
> >
> > >
> > > > for host bridge uports. For reasons out of scope, a functional change
> > > > would be visible if the HDM decoder was programmed by BIOS to values
> > > > other than the full address range. Similarly if a type2 device was
> > > > connected to this root port and programmed by BIOS, that can now be
> > > > acted upon accordingly.
> > >
> > > I would reserve discussion of "no functional change" for patches that
> > > are pure cleanup. In this case this patch will cause the kernel to
> > > behave differently when other conditions are met.
> >
> > True. I will remove that.
> >
> > >
> > > >
> > > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > > ---
> > > >  drivers/cxl/acpi.c | 25 ++++++++++++++++++++++++-
> > > >  1 file changed, 24 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c
> > > > index d61397055e9f..8cca0814dfb8 100644
> > > > --- a/drivers/cxl/acpi.c
> > > > +++ b/drivers/cxl/acpi.c
> > > > @@ -280,12 +280,14 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > > >         struct cxl_port *root_port = arg;
> > > >         struct device *host = root_port->dev.parent;
> > > >         struct acpi_device *bridge = to_cxl_host_bridge(host, match);
> > > > +       struct cxl_component_reg_map map;
> > > >         struct acpi_pci_root *pci_root;
> > > >         struct cxl_walk_context ctx;
> > > >         int single_port_map[1], rc;
> > > >         struct cxl_decoder *cxld;
> > > >         struct cxl_dport *dport;
> > > >         struct cxl_port *port;
> > > > +       void __iomem *crb;
> > > >
> > > >         if (!bridge)
> > > >                 return 0;
> > > > @@ -318,10 +320,31 @@ static int add_host_bridge_uport(struct device *match, void *arg)
> > > >                 return -ENODEV;
> > > >         if (ctx.error)
> > > >                 return ctx.error;
> > > > +       /*
> > > > +        * If the host bridge has more than 1 root port, it must have registers
> > > > +        * controlling the HDM decoders. Those will be enumerated by the port
> > > > +        * driver.
> > > > +        */
> > > >         if (ctx.count > 1)
> > > >                 return 0;
> > > >
> > > > -       /* TODO: Scan CHBCR for HDM Decoder resources */
> > > > +       /*
> > > > +        * If the single ported host bridge has a component register block,
> > > > +        * simply let the port driver handle the decoder enumeration.
> > > > +        *
> > > > +        * Host bridge component registers live in the system's physical address
> > > > +        * space.
> > > > +        */
> > > > +       crb = ioremap(dport->component_reg_phys, CXL_COMPONENT_REG_BLOCK_SIZE);
> > >
> > > This looks broken, the driver for the port should be mapping dport
> > > component registers to offer services to upper layers.
> > >
> > > There's also a missing iounmap.
> > >
> >
> > Where's the missing iounmap?
> 
> Oops, sorry, don't know why I missed that. Forgive the noise.
> 
> > The port driver does what you say, but I need a way
> > to shortcircuit the case where the root port doesn't have component registers
> > which you've previously documented as allowed by spec. How would you recommend
> > doing that?
> 
> Perhaps cxl_port_probe() needs to special case the single dport case
> and just say, "no registers needed, single port == HDM passthrough".
> 
> Until there's a need to look at non-HDM registers I'd hold off on
> enabling this case. Probably that comes soon when considering IDE
> support, but no need to pre-enable that yet.

It seemed like a very simple thing to support given the port driver's existence
so it was added to remove a TODO. However, I will drop it as you request.