Messages in this thread Patch in this message | | | Date | Wed, 19 Jul 2023 18:06:40 +0530 | Subject | Re: [PATCH] cdx: add support for bus mastering | From | "Gupta, Nipun" <> |
| |
On 7/18/2023 7:16 PM, Greg KH wrote: > On Tue, Jul 18, 2023 at 03:36:51PM +0530, Nipun Gupta wrote: >> Introduce cdx_set_master() and cdx_clear_master() APIs >> to support enable and disable of bus mastering. Drivers >> need to use these APIs to enable/disable DMAs from the >> CDX devices. > > You do have a full 72 columns, why not use that?
sure, will update accordingly.
> >> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> >> Reviewed-by: Pieter Jansen van Vuuren <pieter.jansen-van-vuuren@amd.com> >> --- >> drivers/cdx/cdx.c | 32 ++++++++++++++ >> drivers/cdx/controller/cdx_controller.c | 4 ++ >> drivers/cdx/controller/mcdi_functions.c | 57 +++++++++++++++++++++++++ >> drivers/cdx/controller/mcdi_functions.h | 13 ++++++ >> include/linux/cdx/cdx_bus.h | 16 +++++++ >> 5 files changed, 122 insertions(+) >> >> diff --git a/drivers/cdx/cdx.c b/drivers/cdx/cdx.c >> index d2cad4c670a0..efb24672b7d9 100644 >> --- a/drivers/cdx/cdx.c >> +++ b/drivers/cdx/cdx.c >> @@ -182,6 +182,38 @@ cdx_match_id(const struct cdx_device_id *ids, struct cdx_device *dev) >> return NULL; >> } >> >> +int cdx_set_master(struct cdx_device *cdx_dev) >> +{ >> + struct cdx_controller *cdx = cdx_dev->cdx; >> + struct cdx_device_config dev_config; >> + int ret; >> + >> + dev_config.type = CDX_DEV_BUS_MASTER_CONF; >> + dev_config.bme = true; > > What is "bme"?
This is bus master enable. I will add a comment on the structure definition.
> > And are you sure that the config can be on the stack?
Yes, as this information is used by the controller and a new created command is sent to the Firmware/Hardware.
> >> + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, >> + cdx_dev->dev_num, &dev_config); > > Will dev_configure always be there?
This is expected to be there, but will add a check for further assurance.
> >> + if (ret) >> + dev_err(&cdx_dev->dev, "device master enable failed\n"); > > Why error out here, shouldn't the call have printed something if it > wanted to?
Agree, will remove
> >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(cdx_set_master); >> + >> +void cdx_clear_master(struct cdx_device *cdx_dev) >> +{ >> + struct cdx_controller *cdx = cdx_dev->cdx; >> + struct cdx_device_config dev_config; >> + int ret; >> + >> + dev_config.type = CDX_DEV_BUS_MASTER_CONF; >> + dev_config.bme = false; >> + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, >> + cdx_dev->dev_num, &dev_config); >> + if (ret) >> + dev_err(&cdx_dev->dev, "device master disable failed\n"); > > Same question as above.
sure will remove
> >> +} >> +EXPORT_SYMBOL_GPL(cdx_clear_master); >> + >> /** >> * cdx_bus_match - device to driver matching callback >> * @dev: the cdx device to match against >> diff --git a/drivers/cdx/controller/cdx_controller.c b/drivers/cdx/controller/cdx_controller.c >> index dc52f95f8978..ba670f0427d3 100644 >> --- a/drivers/cdx/controller/cdx_controller.c >> +++ b/drivers/cdx/controller/cdx_controller.c >> @@ -55,6 +55,10 @@ static int cdx_configure_device(struct cdx_controller *cdx, >> case CDX_DEV_RESET_CONF: >> ret = cdx_mcdi_reset_device(cdx->priv, bus_num, dev_num); >> break; >> + case CDX_DEV_BUS_MASTER_CONF: >> + ret = cdx_mcdi_bus_master_enable(cdx->priv, bus_num, dev_num, >> + dev_config->bme); >> + break; >> default: >> ret = -EINVAL; >> } >> diff --git a/drivers/cdx/controller/mcdi_functions.c b/drivers/cdx/controller/mcdi_functions.c >> index 0158f26533dd..c4c00f376006 100644 >> --- a/drivers/cdx/controller/mcdi_functions.c >> +++ b/drivers/cdx/controller/mcdi_functions.c >> @@ -137,3 +137,60 @@ int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num) >> >> return ret; >> } >> + >> +static int cdx_mcdi_ctrl_flag_get(struct cdx_mcdi *cdx, u8 bus_num, >> + u8 dev_num, u32 *flags) >> +{ >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_IN_LEN); >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN); > > How big are these buffers?
The defines are as follows in mc_cdx_pcol.h: #define MC_CMD_CDX_DEVICE_CONTROL_GET_IN_LEN 8 #define MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN 4
where MCDI_DECLARE_BUF leads to creation of a variable of data structure: /* A doubleword (i.e. 4 byte) datatype - little-endian in HW */ struct cdx_dword { __le32 cdx_u32; };
so inbuf is of 32 bytes (8 * 4) and outbuf is 16 bytes (4 * 4).
> >> + size_t outlen; >> + int ret; >> + >> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_BUS, bus_num); >> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_GET_IN_DEVICE, dev_num); >> + ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_GET, inbuf, >> + sizeof(inbuf), outbuf, sizeof(outbuf), &outlen); >> + if (ret) >> + return ret; >> + >> + if (outlen != MC_CMD_CDX_DEVICE_CONTROL_GET_OUT_LEN) >> + return -EIO; >> + >> + *flags = MCDI_DWORD(outbuf, CDX_DEVICE_CONTROL_GET_OUT_FLAGS); >> + >> + return 0; >> +} >> + >> +static int cdx_mcdi_ctrl_flag_set(struct cdx_mcdi *cdx, u8 bus_num, >> + u8 dev_num, bool enable, int lbn) >> +{ >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_CONTROL_SET_IN_LEN); >> + int ret, en; >> + u32 flags; >> + >> + /* >> + * Get flags and then set/reset BUS_MASTER_BIT according to >> + * the input params. >> + */ >> + ret = cdx_mcdi_ctrl_flag_get(cdx, bus_num, dev_num, &flags); >> + if (ret) >> + return ret; >> + >> + en = enable ? 1 : 0; > > if/then/else?
okay. will update
> > >> + flags = (flags & (u32)(~(BIT(lbn)))) | (en << lbn); >> + >> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_BUS, bus_num); >> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_DEVICE, dev_num); >> + MCDI_SET_DWORD(inbuf, CDX_DEVICE_CONTROL_SET_IN_FLAGS, flags); >> + ret = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_CONTROL_SET, inbuf, >> + sizeof(inbuf), NULL, 0, NULL); >> + >> + return ret; >> +} >> + >> +int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num, >> + u8 dev_num, bool enable) >> +{ >> + return cdx_mcdi_ctrl_flag_set(cdx, bus_num, dev_num, enable, >> + MC_CMD_CDX_DEVICE_CONTROL_SET_IN_BUS_MASTER_ENABLE_LBN); >> +} >> diff --git a/drivers/cdx/controller/mcdi_functions.h b/drivers/cdx/controller/mcdi_functions.h >> index 7440ace5539a..a448d6581eb4 100644 >> --- a/drivers/cdx/controller/mcdi_functions.h >> +++ b/drivers/cdx/controller/mcdi_functions.h >> @@ -58,4 +58,17 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx, >> int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, >> u8 bus_num, u8 dev_num); >> >> +/** >> + * cdx_mcdi_bus_master_enable - Set/Reset bus mastering for cdx device >> + * represented by bus_num:dev_num >> + * @cdx: pointer to MCDI interface. >> + * @bus_num: Bus number. >> + * @dev_num: Device number. >> + * @enable: Enable bus mastering if set, disable otherwise. >> + * >> + * Return: 0 on success, <0 on failure >> + */ >> +int cdx_mcdi_bus_master_enable(struct cdx_mcdi *cdx, u8 bus_num, >> + u8 dev_num, bool enable); >> + >> #endif /* CDX_MCDI_FUNCTIONS_H */ >> diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h >> index bead71b7bc73..acb4e40bd20e 100644 >> --- a/include/linux/cdx/cdx_bus.h >> +++ b/include/linux/cdx/cdx_bus.h >> @@ -21,11 +21,13 @@ >> struct cdx_controller; >> >> enum { >> + CDX_DEV_BUS_MASTER_CONF, >> CDX_DEV_RESET_CONF, >> }; >> >> struct cdx_device_config { >> u8 type; >> + bool bme; > > What does "bme" mean?
This is bus master enable bit. Will add comment.
Thanks, Nipun
> > thanks, > > greg k-h
| |