| Message ID | 20211022183709.1199701-10-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL Region Creation / HDM decoder programming | expand |
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. > 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. > > 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. > + if (crb) { > + cxl_probe_component_regs(&root_port->dev, crb, &map); > + iounmap(crb); > + if (map.hdm_decoder.valid) { > + dev_dbg(host, > + "Found single port host bridge with component registers\n"); > + return 0; > + } > + } > > /* > * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability > -- > 2.33.1 >
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? > > > 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? 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? > > + if (crb) { > > + cxl_probe_component_regs(&root_port->dev, crb, &map); > > + iounmap(crb); > > + if (map.hdm_decoder.valid) { > > + dev_dbg(host, > > + "Found single port host bridge with component registers\n"); > > + return 0; > > + } > > + } > > > > /* > > * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability > > -- > > 2.33.1 > >
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.
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.
On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: [..] > 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. I'm honestly asking the question why this is needed and more specifically why this is needed in this location? In other words, how is this case different than typical component register probing that is done in the port driver itself? I would expect the TODO just gets deleted by the port driver addition.
On 21-11-02 10:46:41, Dan Williams wrote: > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > [..] > > 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. > > I'm honestly asking the question why this is needed and more > specifically why this is needed in this location? > > In other words, how is this case different than typical component > register probing that is done in the port driver itself? I would > expect the TODO just gets deleted by the port driver addition. Without this code, a decoder is added for the full address range regardless if there is an existing programmed HDM decoder. With this code when the port driver probes this host bridge HDM component registers, everything will be enumerated by the normal flow. This seemed easier than trying to have the port driver determine what cxl_acpi driver did and unwind that if there is a programmed decoder. But now you're making me think I've missed something big...
On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-11-02 10:46:41, Dan Williams wrote: > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > [..] > > > 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. > > > > I'm honestly asking the question why this is needed and more > > specifically why this is needed in this location? > > > > In other words, how is this case different than typical component > > register probing that is done in the port driver itself? I would > > expect the TODO just gets deleted by the port driver addition. > > Without this code, a decoder is added for the full address range regardless if > there is an existing programmed HDM decoder. With this code when the port driver > probes this host bridge HDM component registers, everything will be enumerated > by the normal flow. > > This seemed easier than trying to have the port driver determine what cxl_acpi > driver did and unwind that if there is a programmed decoder. I think this is more an argument to unify all decoder creation in the port driver. When the port driver sees a single dport with no HDM decoder registers it can create the passthrough decoder, if it sees single dport with HDM decoder registers then it can create a programmable decoder.
On 21-11-02 11:10:05, Dan Williams wrote: > On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > On 21-11-02 10:46:41, Dan Williams wrote: > > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > [..] > > > > 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. > > > > > > I'm honestly asking the question why this is needed and more > > > specifically why this is needed in this location? > > > > > > In other words, how is this case different than typical component > > > register probing that is done in the port driver itself? I would > > > expect the TODO just gets deleted by the port driver addition. > > > > Without this code, a decoder is added for the full address range regardless if > > there is an existing programmed HDM decoder. With this code when the port driver > > probes this host bridge HDM component registers, everything will be enumerated > > by the normal flow. > > > > This seemed easier than trying to have the port driver determine what cxl_acpi > > driver did and unwind that if there is a programmed decoder. > > I think this is more an argument to unify all decoder creation in the > port driver. When the port driver sees a single dport with no HDM > decoder registers it can create the passthrough decoder, if it sees > single dport with HDM decoder registers then it can create a > programmable decoder. That too, though I'm not sure how useful that will be for us. I think the more likely case is BIOS is trying to hide some device (or switch) address space.
On Tue, Nov 2, 2021 at 11:27 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-11-02 11:10:05, Dan Williams wrote: > > On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > On 21-11-02 10:46:41, Dan Williams wrote: > > > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > [..] > > > > > 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. > > > > > > > > I'm honestly asking the question why this is needed and more > > > > specifically why this is needed in this location? > > > > > > > > In other words, how is this case different than typical component > > > > register probing that is done in the port driver itself? I would > > > > expect the TODO just gets deleted by the port driver addition. > > > > > > Without this code, a decoder is added for the full address range regardless if > > > there is an existing programmed HDM decoder. With this code when the port driver > > > probes this host bridge HDM component registers, everything will be enumerated > > > by the normal flow. > > > > > > This seemed easier than trying to have the port driver determine what cxl_acpi > > > driver did and unwind that if there is a programmed decoder. > > > > I think this is more an argument to unify all decoder creation in the > > port driver. When the port driver sees a single dport with no HDM > > decoder registers it can create the passthrough decoder, if it sees > > single dport with HDM decoder registers then it can create a > > programmable decoder. > > That too, though I'm not sure how useful that will be for us. ? This is about the proper place to enumerate decoders. The proposal in this patch is to do some part of the enumeration in cxl_acpi and some part in cxl_port, I'm saying push it all to cxl_port. I.e. it was a layering violation in retrospect to create a default passthrough decoder from cxl_acpi. > I think the more > likely case is BIOS is trying to hide some device (or switch) address space. Not sure why that is relevant for the decision about where to place component register probing and passthrough decoder creation?
On 21-11-02 11:49:14, Dan Williams wrote: > On Tue, Nov 2, 2021 at 11:27 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > On 21-11-02 11:10:05, Dan Williams wrote: > > > On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > On 21-11-02 10:46:41, Dan Williams wrote: > > > > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > [..] > > > > > > 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. > > > > > > > > > > I'm honestly asking the question why this is needed and more > > > > > specifically why this is needed in this location? > > > > > > > > > > In other words, how is this case different than typical component > > > > > register probing that is done in the port driver itself? I would > > > > > expect the TODO just gets deleted by the port driver addition. > > > > > > > > Without this code, a decoder is added for the full address range regardless if > > > > there is an existing programmed HDM decoder. With this code when the port driver > > > > probes this host bridge HDM component registers, everything will be enumerated > > > > by the normal flow. > > > > > > > > This seemed easier than trying to have the port driver determine what cxl_acpi > > > > driver did and unwind that if there is a programmed decoder. > > > > > > I think this is more an argument to unify all decoder creation in the > > > port driver. When the port driver sees a single dport with no HDM > > > decoder registers it can create the passthrough decoder, if it sees > > > single dport with HDM decoder registers then it can create a > > > programmable decoder. > > > > That too, though I'm not sure how useful that will be for us. > > ? > > This is about the proper place to enumerate decoders. The proposal in > this patch is to do some part of the enumeration in cxl_acpi and some > part in cxl_port, I'm saying push it all to cxl_port. I.e. it was a > layering violation in retrospect to create a default passthrough > decoder from cxl_acpi. I think it makes sense to have cxl_decoder_add() not called from anything but the port driver. So long as there's a way to determine it's a CXL 2.0 Root Port, which, it seems we'll have, it should be straight forward. > > > I think the more > > likely case is BIOS is trying to hide some device (or switch) address space. > > Not sure why that is relevant for the decision about where to place > component register probing and passthrough decoder creation? The comment wasn't about where, it was just a response as to how there's more motivation than just having a programmable decoder, you might find a locked decoder that has unexpected constraints.
On Tue, Nov 2, 2021 at 2:16 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > On 21-11-02 11:49:14, Dan Williams wrote: > > On Tue, Nov 2, 2021 at 11:27 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > On 21-11-02 11:10:05, Dan Williams wrote: > > > > On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > > > On 21-11-02 10:46:41, Dan Williams wrote: > > > > > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > [..] > > > > > > > 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. > > > > > > > > > > > > I'm honestly asking the question why this is needed and more > > > > > > specifically why this is needed in this location? > > > > > > > > > > > > In other words, how is this case different than typical component > > > > > > register probing that is done in the port driver itself? I would > > > > > > expect the TODO just gets deleted by the port driver addition. > > > > > > > > > > Without this code, a decoder is added for the full address range regardless if > > > > > there is an existing programmed HDM decoder. With this code when the port driver > > > > > probes this host bridge HDM component registers, everything will be enumerated > > > > > by the normal flow. > > > > > > > > > > This seemed easier than trying to have the port driver determine what cxl_acpi > > > > > driver did and unwind that if there is a programmed decoder. > > > > > > > > I think this is more an argument to unify all decoder creation in the > > > > port driver. When the port driver sees a single dport with no HDM > > > > decoder registers it can create the passthrough decoder, if it sees > > > > single dport with HDM decoder registers then it can create a > > > > programmable decoder. > > > > > > That too, though I'm not sure how useful that will be for us. > > > > ? > > > > This is about the proper place to enumerate decoders. The proposal in > > this patch is to do some part of the enumeration in cxl_acpi and some > > part in cxl_port, I'm saying push it all to cxl_port. I.e. it was a > > layering violation in retrospect to create a default passthrough > > decoder from cxl_acpi. > > I think it makes sense to have cxl_decoder_add() not called from anything but > the port driver. So long as there's a way to determine it's a CXL 2.0 Root Port, > which, it seems we'll have, it should be straight forward. Does it need to be root port aware? I expect any single ported switch is treated the same, not that I expect single-ported switches to be a real thing, but I don't expect our port driver would care.
On 21-11-02 14:34:19, Dan Williams wrote: > On Tue, Nov 2, 2021 at 2:16 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > On 21-11-02 11:49:14, Dan Williams wrote: > > > On Tue, Nov 2, 2021 at 11:27 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > On 21-11-02 11:10:05, Dan Williams wrote: > > > > > On Tue, Nov 2, 2021 at 10:57 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > > > > > > On 21-11-02 10:46:41, Dan Williams wrote: > > > > > > > On Tue, Nov 2, 2021 at 9:31 AM Ben Widawsky <ben.widawsky@intel.com> wrote: > > > > > > > [..] > > > > > > > > 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. > > > > > > > > > > > > > > I'm honestly asking the question why this is needed and more > > > > > > > specifically why this is needed in this location? > > > > > > > > > > > > > > In other words, how is this case different than typical component > > > > > > > register probing that is done in the port driver itself? I would > > > > > > > expect the TODO just gets deleted by the port driver addition. > > > > > > > > > > > > Without this code, a decoder is added for the full address range regardless if > > > > > > there is an existing programmed HDM decoder. With this code when the port driver > > > > > > probes this host bridge HDM component registers, everything will be enumerated > > > > > > by the normal flow. > > > > > > > > > > > > This seemed easier than trying to have the port driver determine what cxl_acpi > > > > > > driver did and unwind that if there is a programmed decoder. > > > > > > > > > > I think this is more an argument to unify all decoder creation in the > > > > > port driver. When the port driver sees a single dport with no HDM > > > > > decoder registers it can create the passthrough decoder, if it sees > > > > > single dport with HDM decoder registers then it can create a > > > > > programmable decoder. > > > > > > > > That too, though I'm not sure how useful that will be for us. > > > > > > ? > > > > > > This is about the proper place to enumerate decoders. The proposal in > > > this patch is to do some part of the enumeration in cxl_acpi and some > > > part in cxl_port, I'm saying push it all to cxl_port. I.e. it was a > > > layering violation in retrospect to create a default passthrough > > > decoder from cxl_acpi. > > > > I think it makes sense to have cxl_decoder_add() not called from anything but > > the port driver. So long as there's a way to determine it's a CXL 2.0 Root Port, > > which, it seems we'll have, it should be straight forward. > > Does it need to be root port aware? I expect any single ported switch > is treated the same, not that I expect single-ported switches to be a > real thing, but I don't expect our port driver would care. You're correct... I thought switch ports required HDM decoder registers even if single port, but I just checked and they are the same as the host bridge. So indeed the enumeration should only be special cased for the endpoint.
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); + if (crb) { + cxl_probe_component_regs(&root_port->dev, crb, &map); + iounmap(crb); + if (map.hdm_decoder.valid) { + dev_dbg(host, + "Found single port host bridge with component registers\n"); + return 0; + } + } /* * Per the CXL specification (8.2.5.12 CXL HDM Decoder Capability
Now that the port driver exists and is able to do proper decoder enumeration of the component registers, it becomes trivial to use that 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. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/acpi.c | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)