| Message ID | 168357884996.2756219.11067819935569345137.stgit@djiang5-mobl3 |
|---|---|
| State | Superseded |
| Headers | show |
| Series | cxl: Add support for QTG ID retrieval for CXL subsystem | expand |
On Mon, 08 May 2023 13:47:29 -0700 Dave Jiang <dave.jiang@intel.com> wrote: > Each CXL host bridge is represented by an ACPI0016 device. A generic port > device handle that is an ACPI device is represented by a string of > ACPI0016 device HID and UID. Create a device handle from the ACPI device > and retrieve the access coordinates from the stored memory targets. The > access coordinates are stored under the cxl_dport that is associated with > the CXL host bridge. > > Signed-off-by: Dave Jiang <dave.jiang@intel.com> > --- > drivers/cxl/acpi.c | 28 ++++++++++++++++++++++++++++ > drivers/cxl/cxl.h | 2 ++ > 2 files changed, 30 insertions(+) > > diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > index f9b35e8fe810..675a4f423f4b 100644 > --- a/drivers/cxl/acpi.c > +++ b/drivers/cxl/acpi.c > @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, > return 0; > } > > +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport) > +{ > + struct acpi_device *hb = to_cxl_host_bridge(NULL, dev); > + u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 }; > + int rc; > + > + /* ACPI spec 6.5 tABLE 5.65 */ tABLE? > + memcpy(handle, acpi_device_hid(hb), 8); > + memcpy(&handle[8], acpi_device_uid(hb), 4); > + > + rc = acpi_get_genport_coordinates(handle, dport->genport_coord); > + if (rc) > + return rc; > + > + return 0; > +} > + > static int add_host_bridge_dport(struct device *match, void *arg) > { > + int ret; > acpi_status rc; > struct device *bridge; > unsigned long long uid; > @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg) > if (IS_ERR(dport)) > return PTR_ERR(dport); > > + dport->genport_coord = devm_kzalloc(dport->dport, > + sizeof(*dport->genport_coord), > + GFP_KERNEL); It's pretty small - worth allocating separately? Maybe add something on why to the patch description if there is another reason for this dance. > + if (!dport->genport_coord) > + return -ENOMEM; > + > + ret = get_genport_coordinates(match, dport); > + if (ret) > + dev_dbg(match, "Failed to get generic port perf coordinates.\n"); > + > return 0; > } > > diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h > index c1e2c3703a63..033b822a20f2 100644 > --- a/drivers/cxl/cxl.h > +++ b/drivers/cxl/cxl.h > @@ -626,6 +626,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) > * @rcrb: base address for the Root Complex Register Block > * @rch: Indicate whether this dport was enumerated in RCH or VH mode > * @port: reference to cxl_port that contains this downstream port > + * @genport_coord: access coordinates (performance) from ACPI generic port > * @coord: access coordinates (performance) for switch from CDAT > * @link_latency: calculated PCIe downstream latency > */ > @@ -636,6 +637,7 @@ struct cxl_dport { > resource_size_t rcrb; > bool rch; > struct cxl_port *port; > + struct access_coordinate *genport_coord; > struct access_coordinate coord; > long link_latency; > }; > >
On 5/12/23 7:59 AM, Jonathan Cameron wrote: > On Mon, 08 May 2023 13:47:29 -0700 > Dave Jiang <dave.jiang@intel.com> wrote: > >> Each CXL host bridge is represented by an ACPI0016 device. A generic port >> device handle that is an ACPI device is represented by a string of >> ACPI0016 device HID and UID. Create a device handle from the ACPI device >> and retrieve the access coordinates from the stored memory targets. The >> access coordinates are stored under the cxl_dport that is associated with >> the CXL host bridge. >> >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >> --- >> drivers/cxl/acpi.c | 28 ++++++++++++++++++++++++++++ >> drivers/cxl/cxl.h | 2 ++ >> 2 files changed, 30 insertions(+) >> >> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c >> index f9b35e8fe810..675a4f423f4b 100644 >> --- a/drivers/cxl/acpi.c >> +++ b/drivers/cxl/acpi.c >> @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, >> return 0; >> } >> >> +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport) >> +{ >> + struct acpi_device *hb = to_cxl_host_bridge(NULL, dev); >> + u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 }; >> + int rc; >> + >> + /* ACPI spec 6.5 tABLE 5.65 */ > > tABLE? ooops caps lock > >> + memcpy(handle, acpi_device_hid(hb), 8); >> + memcpy(&handle[8], acpi_device_uid(hb), 4); >> + >> + rc = acpi_get_genport_coordinates(handle, dport->genport_coord); >> + if (rc) >> + return rc; >> + >> + return 0; >> +} >> + >> static int add_host_bridge_dport(struct device *match, void *arg) >> { >> + int ret; >> acpi_status rc; >> struct device *bridge; >> unsigned long long uid; >> @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg) >> if (IS_ERR(dport)) >> return PTR_ERR(dport); >> >> + dport->genport_coord = devm_kzalloc(dport->dport, >> + sizeof(*dport->genport_coord), >> + GFP_KERNEL); > > It's pretty small - worth allocating separately? > > Maybe add something on why to the patch description if there is another reason > for this dance. My intention was to allow detection of whether the data exists or not based on if the ptr is NULL. I'll add explanation in patch description. > >> + if (!dport->genport_coord) >> + return -ENOMEM; >> + >> + ret = get_genport_coordinates(match, dport); >> + if (ret) >> + dev_dbg(match, "Failed to get generic port perf coordinates.\n"); >> + >> return 0; >> } >> >> diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h >> index c1e2c3703a63..033b822a20f2 100644 >> --- a/drivers/cxl/cxl.h >> +++ b/drivers/cxl/cxl.h >> @@ -626,6 +626,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) >> * @rcrb: base address for the Root Complex Register Block >> * @rch: Indicate whether this dport was enumerated in RCH or VH mode >> * @port: reference to cxl_port that contains this downstream port >> + * @genport_coord: access coordinates (performance) from ACPI generic port >> * @coord: access coordinates (performance) for switch from CDAT >> * @link_latency: calculated PCIe downstream latency >> */ >> @@ -636,6 +637,7 @@ struct cxl_dport { >> resource_size_t rcrb; >> bool rch; >> struct cxl_port *port; >> + struct access_coordinate *genport_coord; >> struct access_coordinate coord; >> long link_latency; >> }; >> >> >
Dave Jiang wrote: > > > On 5/12/23 7:59 AM, Jonathan Cameron wrote: > > On Mon, 08 May 2023 13:47:29 -0700 > > Dave Jiang <dave.jiang@intel.com> wrote: > > > >> Each CXL host bridge is represented by an ACPI0016 device. A generic port > >> device handle that is an ACPI device is represented by a string of > >> ACPI0016 device HID and UID. Create a device handle from the ACPI device > >> and retrieve the access coordinates from the stored memory targets. The > >> access coordinates are stored under the cxl_dport that is associated with > >> the CXL host bridge. > >> > >> Signed-off-by: Dave Jiang <dave.jiang@intel.com> > >> --- > >> drivers/cxl/acpi.c | 28 ++++++++++++++++++++++++++++ > >> drivers/cxl/cxl.h | 2 ++ > >> 2 files changed, 30 insertions(+) > >> > >> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c > >> index f9b35e8fe810..675a4f423f4b 100644 > >> --- a/drivers/cxl/acpi.c > >> +++ b/drivers/cxl/acpi.c > >> @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, > >> return 0; > >> } > >> > >> +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport) > >> +{ > >> + struct acpi_device *hb = to_cxl_host_bridge(NULL, dev); > >> + u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 }; > >> + int rc; > >> + > >> + /* ACPI spec 6.5 tABLE 5.65 */ > > > > tABLE? > > ooops caps lock > > > > >> + memcpy(handle, acpi_device_hid(hb), 8); > >> + memcpy(&handle[8], acpi_device_uid(hb), 4); > >> + > >> + rc = acpi_get_genport_coordinates(handle, dport->genport_coord); > >> + if (rc) > >> + return rc; > >> + > >> + return 0; > >> +} > >> + > >> static int add_host_bridge_dport(struct device *match, void *arg) > >> { > >> + int ret; > >> acpi_status rc; > >> struct device *bridge; > >> unsigned long long uid; > >> @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg) > >> if (IS_ERR(dport)) > >> return PTR_ERR(dport); > >> > >> + dport->genport_coord = devm_kzalloc(dport->dport, This needs to be the same device as was used to allocate @dport, and that's not @dport->dport. > >> + sizeof(*dport->genport_coord), > >> + GFP_KERNEL); > > > > It's pretty small - worth allocating separately? > > > > Maybe add something on why to the patch description if there is another reason > > for this dance. > > My intention was to allow detection of whether the data exists or not > based on if the ptr is NULL. I'll add explanation in patch description. A couple reactions, is a "zero" access coordinate not sufficient for that? Also, "genport" is an ACPI-ism and I was aiming for cxl-core objects like cxl_dport to remain platform implementation independent. The concept of a dport having a host-bridge access_coordinate is generic though, so I would just rename this to "hb_access" or some other host-bridge generic naming.
On 5/16/23 2:13 PM, Dan Williams wrote: > Dave Jiang wrote: >> >> >> On 5/12/23 7:59 AM, Jonathan Cameron wrote: >>> On Mon, 08 May 2023 13:47:29 -0700 >>> Dave Jiang <dave.jiang@intel.com> wrote: >>> >>>> Each CXL host bridge is represented by an ACPI0016 device. A generic port >>>> device handle that is an ACPI device is represented by a string of >>>> ACPI0016 device HID and UID. Create a device handle from the ACPI device >>>> and retrieve the access coordinates from the stored memory targets. The >>>> access coordinates are stored under the cxl_dport that is associated with >>>> the CXL host bridge. >>>> >>>> Signed-off-by: Dave Jiang <dave.jiang@intel.com> >>>> --- >>>> drivers/cxl/acpi.c | 28 ++++++++++++++++++++++++++++ >>>> drivers/cxl/cxl.h | 2 ++ >>>> 2 files changed, 30 insertions(+) >>>> >>>> diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c >>>> index f9b35e8fe810..675a4f423f4b 100644 >>>> --- a/drivers/cxl/acpi.c >>>> +++ b/drivers/cxl/acpi.c >>>> @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, >>>> return 0; >>>> } >>>> >>>> +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport) >>>> +{ >>>> + struct acpi_device *hb = to_cxl_host_bridge(NULL, dev); >>>> + u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 }; >>>> + int rc; >>>> + >>>> + /* ACPI spec 6.5 tABLE 5.65 */ >>> >>> tABLE? >> >> ooops caps lock >> >>> >>>> + memcpy(handle, acpi_device_hid(hb), 8); >>>> + memcpy(&handle[8], acpi_device_uid(hb), 4); >>>> + >>>> + rc = acpi_get_genport_coordinates(handle, dport->genport_coord); >>>> + if (rc) >>>> + return rc; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int add_host_bridge_dport(struct device *match, void *arg) >>>> { >>>> + int ret; >>>> acpi_status rc; >>>> struct device *bridge; >>>> unsigned long long uid; >>>> @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg) >>>> if (IS_ERR(dport)) >>>> return PTR_ERR(dport); >>>> >>>> + dport->genport_coord = devm_kzalloc(dport->dport, > > This needs to be the same device as was used to allocate @dport, and > that's not @dport->dport. ok > >>>> + sizeof(*dport->genport_coord), >>>> + GFP_KERNEL); >>> >>> It's pretty small - worth allocating separately? >>> >>> Maybe add something on why to the patch description if there is another reason >>> for this dance. >> >> My intention was to allow detection of whether the data exists or not >> based on if the ptr is NULL. I'll add explanation in patch description. > > A couple reactions, is a "zero" access coordinate not sufficient for > that? Probably. I can change. I was being paranoid. > > Also, "genport" is an ACPI-ism and I was aiming for cxl-core objects > like cxl_dport to remain platform implementation independent. The > concept of a dport having a host-bridge access_coordinate is generic > though, so I would just rename this to "hb_access" or some other > host-bridge generic naming. Ok
diff --git a/drivers/cxl/acpi.c b/drivers/cxl/acpi.c index f9b35e8fe810..675a4f423f4b 100644 --- a/drivers/cxl/acpi.c +++ b/drivers/cxl/acpi.c @@ -537,8 +537,26 @@ static int cxl_get_chbcr(union acpi_subtable_headers *header, void *arg, return 0; } +static int get_genport_coordinates(struct device *dev, struct cxl_dport *dport) +{ + struct acpi_device *hb = to_cxl_host_bridge(NULL, dev); + u8 handle[ACPI_SRAT_DEVICE_HANDLE_SIZE] = { 0 }; + int rc; + + /* ACPI spec 6.5 tABLE 5.65 */ + memcpy(handle, acpi_device_hid(hb), 8); + memcpy(&handle[8], acpi_device_uid(hb), 4); + + rc = acpi_get_genport_coordinates(handle, dport->genport_coord); + if (rc) + return rc; + + return 0; +} + static int add_host_bridge_dport(struct device *match, void *arg) { + int ret; acpi_status rc; struct device *bridge; unsigned long long uid; @@ -594,6 +612,16 @@ static int add_host_bridge_dport(struct device *match, void *arg) if (IS_ERR(dport)) return PTR_ERR(dport); + dport->genport_coord = devm_kzalloc(dport->dport, + sizeof(*dport->genport_coord), + GFP_KERNEL); + if (!dport->genport_coord) + return -ENOMEM; + + ret = get_genport_coordinates(match, dport); + if (ret) + dev_dbg(match, "Failed to get generic port perf coordinates.\n"); + return 0; } diff --git a/drivers/cxl/cxl.h b/drivers/cxl/cxl.h index c1e2c3703a63..033b822a20f2 100644 --- a/drivers/cxl/cxl.h +++ b/drivers/cxl/cxl.h @@ -626,6 +626,7 @@ cxl_find_dport_by_dev(struct cxl_port *port, const struct device *dport_dev) * @rcrb: base address for the Root Complex Register Block * @rch: Indicate whether this dport was enumerated in RCH or VH mode * @port: reference to cxl_port that contains this downstream port + * @genport_coord: access coordinates (performance) from ACPI generic port * @coord: access coordinates (performance) for switch from CDAT * @link_latency: calculated PCIe downstream latency */ @@ -636,6 +637,7 @@ struct cxl_dport { resource_size_t rcrb; bool rch; struct cxl_port *port; + struct access_coordinate *genport_coord; struct access_coordinate coord; long link_latency; };
Each CXL host bridge is represented by an ACPI0016 device. A generic port device handle that is an ACPI device is represented by a string of ACPI0016 device HID and UID. Create a device handle from the ACPI device and retrieve the access coordinates from the stored memory targets. The access coordinates are stored under the cxl_dport that is associated with the CXL host bridge. Signed-off-by: Dave Jiang <dave.jiang@intel.com> --- drivers/cxl/acpi.c | 28 ++++++++++++++++++++++++++++ drivers/cxl/cxl.h | 2 ++ 2 files changed, 30 insertions(+)