Message ID | 20240907081836.5801-6-alejandro.lucero-palau@amd.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | cxl: add Type2 device support | expand |
On 9/7/2024 4:18 PM, alejandro.lucero-palau@amd.com wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Create a new function for a type2 device initialising > cxl_dev_state struct regarding cxl regs setup and mapping. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_cxl.c | 6 ++++++ > include/linux/cxl/cxl.h | 2 ++ > 3 files changed, 38 insertions(+) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index bf57f081ef8f..9afcdd643866 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, > } > EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); > > +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds) > +{ > + struct cxl_register_map map; > + int rc; > + > + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, > + &cxlds->capabilities); > + if (!rc) { > + rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs); > + if (rc) > + return rc; > + } > + > + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, > + &cxlds->reg_map, &cxlds->capabilities); > + if (rc) > + dev_warn(&pdev->dev, "No component registers (%d)\n", rc); > + > + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { > + rc = cxl_map_component_regs(&cxlds->reg_map, > + &cxlds->regs.component, > + BIT(CXL_CM_CAP_CAP_ID_RAS)); > + if (rc) > + dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); > + } > + > + return rc; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); > + I thought this function should be implemented in efx driver, just like what cxl_pci driver does, because I think it is not a generic setup flow for all CXL type-2 devices. > bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, > u32 *current_caps) > { > diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c > index bba36cbbab22..fee143e94c1f 100644 > --- a/drivers/net/ethernet/sfc/efx_cxl.c > +++ b/drivers/net/ethernet/sfc/efx_cxl.c > @@ -66,6 +66,12 @@ int efx_cxl_init(struct efx_nic *efx) > goto err; > } > > + rc = cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds); > + if (rc) { > + pci_err(pci_dev, "CXL accel setup regs failed"); > + goto err; > + } > + > return 0; > err: > kfree(cxl->cxlds); > diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h > index 4a57bf60403d..f2dcba6cdc22 100644 > --- a/include/linux/cxl/cxl.h > +++ b/include/linux/cxl/cxl.h > @@ -5,6 +5,7 @@ > #define __CXL_H > > #include <linux/device.h> > +#include <linux/pci.h> > > enum cxl_resource { > CXL_ACCEL_RES_DPA, > @@ -50,4 +51,5 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, > enum cxl_resource); > bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, > u32 *current_caps); > +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds); > #endif
On 9/10/24 07:00, Li, Ming4 wrote: > On 9/7/2024 4:18 PM, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alucerop@amd.com> >> >> Create a new function for a type2 device initialising >> cxl_dev_state struct regarding cxl regs setup and mapping. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_cxl.c | 6 ++++++ >> include/linux/cxl/cxl.h | 2 ++ >> 3 files changed, 38 insertions(+) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index bf57f081ef8f..9afcdd643866 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, >> } >> EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); >> >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds) >> +{ >> + struct cxl_register_map map; >> + int rc; >> + >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, >> + &cxlds->capabilities); >> + if (!rc) { >> + rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs); >> + if (rc) >> + return rc; >> + } >> + >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, >> + &cxlds->reg_map, &cxlds->capabilities); >> + if (rc) >> + dev_warn(&pdev->dev, "No component registers (%d)\n", rc); >> + >> + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { >> + rc = cxl_map_component_regs(&cxlds->reg_map, >> + &cxlds->regs.component, >> + BIT(CXL_CM_CAP_CAP_ID_RAS)); >> + if (rc) >> + dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); >> + } >> + >> + return rc; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); >> + > I thought this function should be implemented in efx driver, just like what cxl_pci driver does, because I think it is not a generic setup flow for all CXL type-2 devices. > The idea here is to have a single function for discovering the registers, both Device and Component registers. If an accel has not all of them, as in the sfc case, not a problem with the last changes added. Keeping with the idea of avoiding an accel driver to manipulate cxl_dev_state, this accessor is created. >> bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, >> u32 *current_caps) >> { >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c >> index bba36cbbab22..fee143e94c1f 100644 >> --- a/drivers/net/ethernet/sfc/efx_cxl.c >> +++ b/drivers/net/ethernet/sfc/efx_cxl.c >> @@ -66,6 +66,12 @@ int efx_cxl_init(struct efx_nic *efx) >> goto err; >> } >> >> + rc = cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds); >> + if (rc) { >> + pci_err(pci_dev, "CXL accel setup regs failed"); >> + goto err; >> + } >> + >> return 0; >> err: >> kfree(cxl->cxlds); >> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h >> index 4a57bf60403d..f2dcba6cdc22 100644 >> --- a/include/linux/cxl/cxl.h >> +++ b/include/linux/cxl/cxl.h >> @@ -5,6 +5,7 @@ >> #define __CXL_H >> >> #include <linux/device.h> >> +#include <linux/pci.h> >> >> enum cxl_resource { >> CXL_ACCEL_RES_DPA, >> @@ -50,4 +51,5 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, >> enum cxl_resource); >> bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, >> u32 *current_caps); >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds); >> #endif >
On Tue, 10 Sep 2024 08:24:33 +0100 Alejandro Lucero Palau <alucerop@amd.com> wrote: > > On 9/10/24 07:00, Li, Ming4 wrote: > > On 9/7/2024 4:18 PM, alejandro.lucero-palau@amd.com wrote: > >> From: Alejandro Lucero <alucerop@amd.com> > >> > >> Create a new function for a type2 device initialising > >> cxl_dev_state struct regarding cxl regs setup and mapping. > >> > >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> > >> --- > >> drivers/cxl/core/pci.c | 30 > >> ++++++++++++++++++++++++++++++ drivers/net/ethernet/sfc/efx_cxl.c > >> | 6 ++++++ include/linux/cxl/cxl.h | 2 ++ > >> 3 files changed, 38 insertions(+) > >> > >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > >> index bf57f081ef8f..9afcdd643866 100644 > >> --- a/drivers/cxl/core/pci.c > >> +++ b/drivers/cxl/core/pci.c > >> @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev > >> *pdev, enum cxl_regloc_type type, } > >> EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); > >> > >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct > >> cxl_dev_state *cxlds) +{ > >> + struct cxl_register_map map; > >> + int rc; > >> + > >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, > >> + &cxlds->capabilities); > >> + if (!rc) { > >> + rc = cxl_map_device_regs(&map, > >> &cxlds->regs.device_regs); > >> + if (rc) > >> + return rc; > >> + } > >> + > >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, > >> + &cxlds->reg_map, > >> &cxlds->capabilities); > >> + if (rc) > >> + dev_warn(&pdev->dev, "No component registers > >> (%d)\n", rc); + > >> + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { > >> + rc = cxl_map_component_regs(&cxlds->reg_map, > >> + > >> &cxlds->regs.component, > >> + > >> BIT(CXL_CM_CAP_CAP_ID_RAS)); > >> + if (rc) > >> + dev_dbg(&pdev->dev, "Failed to map RAS > >> capability.\n"); > >> + } > >> + > >> + return rc; > >> +} > >> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); > >> + > > I thought this function should be implemented in efx driver, just > > like what cxl_pci driver does, because I think it is not a generic > > setup flow for all CXL type-2 devices. > > > > The idea here is to have a single function for discovering the > registers, both Device and Component registers. If an accel has not > all of them, as in the sfc case, not a problem with the last changes > added. > > Keeping with the idea of avoiding an accel driver to manipulate > cxl_dev_state, this accessor is created. > Agree. Let's keep this function. > > >> bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 > >> expected_caps, u32 *current_caps) > >> { > >> diff --git a/drivers/net/ethernet/sfc/efx_cxl.c > >> b/drivers/net/ethernet/sfc/efx_cxl.c index > >> bba36cbbab22..fee143e94c1f 100644 --- > >> a/drivers/net/ethernet/sfc/efx_cxl.c +++ > >> b/drivers/net/ethernet/sfc/efx_cxl.c @@ -66,6 +66,12 @@ int > >> efx_cxl_init(struct efx_nic *efx) goto err; > >> } > >> > >> + rc = cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds); > >> + if (rc) { > >> + pci_err(pci_dev, "CXL accel setup regs failed"); > >> + goto err; > >> + } > >> + > >> return 0; > >> err: > >> kfree(cxl->cxlds); > >> diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h > >> index 4a57bf60403d..f2dcba6cdc22 100644 > >> --- a/include/linux/cxl/cxl.h > >> +++ b/include/linux/cxl/cxl.h > >> @@ -5,6 +5,7 @@ > >> #define __CXL_H > >> > >> #include <linux/device.h> > >> +#include <linux/pci.h> > >> > >> enum cxl_resource { > >> CXL_ACCEL_RES_DPA, > >> @@ -50,4 +51,5 @@ int cxl_set_resource(struct cxl_dev_state > >> *cxlds, struct resource res, enum cxl_resource); > >> bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 > >> expected_caps, u32 *current_caps); > >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct > >> cxl_dev_state *cxlds); #endif > > >
On Sat, 7 Sep 2024 09:18:21 +0100 <alejandro.lucero-palau@amd.com> wrote: > From: Alejandro Lucero <alucerop@amd.com> > > Create a new function for a type2 device initialising > cxl_dev_state struct regarding cxl regs setup and mapping. > > Signed-off-by: Alejandro Lucero <alucerop@amd.com> > --- > drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++++++ > drivers/net/ethernet/sfc/efx_cxl.c | 6 ++++++ > include/linux/cxl/cxl.h | 2 ++ > 3 files changed, 38 insertions(+) > > diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c > index bf57f081ef8f..9afcdd643866 100644 > --- a/drivers/cxl/core/pci.c > +++ b/drivers/cxl/core/pci.c > @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, > } > EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); > > +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds) > +{ > + struct cxl_register_map map; > + int rc; > + > + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, > + &cxlds->capabilities); > + if (!rc) { I'd be tempted to wrap these two up in a separate function called from here as the out of line good path is less than ideal from readability point of view. > + rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs); > + if (rc) > + return rc; > + } > + > + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, > + &cxlds->reg_map, &cxlds->capabilities); > + if (rc) > + dev_warn(&pdev->dev, "No component registers (%d)\n", rc); > + > + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { If there are no component registers this isn't going to work yet this tries anyway? > + rc = cxl_map_component_regs(&cxlds->reg_map, > + &cxlds->regs.component, > + BIT(CXL_CM_CAP_CAP_ID_RAS)); > + if (rc) > + dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); > + } > + > + return rc; > +} > +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); > +
On 9/13/24 18:32, Jonathan Cameron wrote: > On Sat, 7 Sep 2024 09:18:21 +0100 > <alejandro.lucero-palau@amd.com> wrote: > >> From: Alejandro Lucero <alucerop@amd.com> >> >> Create a new function for a type2 device initialising >> cxl_dev_state struct regarding cxl regs setup and mapping. >> >> Signed-off-by: Alejandro Lucero <alucerop@amd.com> >> --- >> drivers/cxl/core/pci.c | 30 ++++++++++++++++++++++++++++++ >> drivers/net/ethernet/sfc/efx_cxl.c | 6 ++++++ >> include/linux/cxl/cxl.h | 2 ++ >> 3 files changed, 38 insertions(+) >> >> diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c >> index bf57f081ef8f..9afcdd643866 100644 >> --- a/drivers/cxl/core/pci.c >> +++ b/drivers/cxl/core/pci.c >> @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, >> } >> EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); >> >> +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds) >> +{ >> + struct cxl_register_map map; >> + int rc; >> + >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, >> + &cxlds->capabilities); >> + if (!rc) { > I'd be tempted to wrap these two up in a separate function called > from here as the out of line good path is less than ideal from > readability point of view. OK. It makes sense. >> + rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs); >> + if (rc) >> + return rc; >> + } >> + >> + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, >> + &cxlds->reg_map, &cxlds->capabilities); >> + if (rc) >> + dev_warn(&pdev->dev, "No component registers (%d)\n", rc); >> + >> + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { > If there are no component registers this isn't going to work yet this > tries anyway? Right. I should return if no component registers. Thanks! >> + rc = cxl_map_component_regs(&cxlds->reg_map, >> + &cxlds->regs.component, >> + BIT(CXL_CM_CAP_CAP_ID_RAS)); >> + if (rc) >> + dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); >> + } >> + >> + return rc; >> +} >> +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); >> +
diff --git a/drivers/cxl/core/pci.c b/drivers/cxl/core/pci.c index bf57f081ef8f..9afcdd643866 100644 --- a/drivers/cxl/core/pci.c +++ b/drivers/cxl/core/pci.c @@ -1142,6 +1142,36 @@ int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type, } EXPORT_SYMBOL_NS_GPL(cxl_pci_setup_regs, CXL); +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds) +{ + struct cxl_register_map map; + int rc; + + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_MEMDEV, &map, + &cxlds->capabilities); + if (!rc) { + rc = cxl_map_device_regs(&map, &cxlds->regs.device_regs); + if (rc) + return rc; + } + + rc = cxl_pci_setup_regs(pdev, CXL_REGLOC_RBI_COMPONENT, + &cxlds->reg_map, &cxlds->capabilities); + if (rc) + dev_warn(&pdev->dev, "No component registers (%d)\n", rc); + + if (cxlds->capabilities & BIT(CXL_CM_CAP_CAP_ID_RAS)) { + rc = cxl_map_component_regs(&cxlds->reg_map, + &cxlds->regs.component, + BIT(CXL_CM_CAP_CAP_ID_RAS)); + if (rc) + dev_dbg(&pdev->dev, "Failed to map RAS capability.\n"); + } + + return rc; +} +EXPORT_SYMBOL_NS_GPL(cxl_pci_accel_setup_regs, CXL); + bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, u32 *current_caps) { diff --git a/drivers/net/ethernet/sfc/efx_cxl.c b/drivers/net/ethernet/sfc/efx_cxl.c index bba36cbbab22..fee143e94c1f 100644 --- a/drivers/net/ethernet/sfc/efx_cxl.c +++ b/drivers/net/ethernet/sfc/efx_cxl.c @@ -66,6 +66,12 @@ int efx_cxl_init(struct efx_nic *efx) goto err; } + rc = cxl_pci_accel_setup_regs(pci_dev, cxl->cxlds); + if (rc) { + pci_err(pci_dev, "CXL accel setup regs failed"); + goto err; + } + return 0; err: kfree(cxl->cxlds); diff --git a/include/linux/cxl/cxl.h b/include/linux/cxl/cxl.h index 4a57bf60403d..f2dcba6cdc22 100644 --- a/include/linux/cxl/cxl.h +++ b/include/linux/cxl/cxl.h @@ -5,6 +5,7 @@ #define __CXL_H #include <linux/device.h> +#include <linux/pci.h> enum cxl_resource { CXL_ACCEL_RES_DPA, @@ -50,4 +51,5 @@ int cxl_set_resource(struct cxl_dev_state *cxlds, struct resource res, enum cxl_resource); bool cxl_pci_check_caps(struct cxl_dev_state *cxlds, u32 expected_caps, u32 *current_caps); +int cxl_pci_accel_setup_regs(struct pci_dev *pdev, struct cxl_dev_state *cxlds); #endif