| Message ID | 20230117134139.1298-8-nipun.gupta@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add support for CDX bus | expand |
On Tue, Jan 17, 2023 at 07:11:39PM +0530, Nipun Gupta wrote: > Create sysfs entry for CDX devices. > > Sysfs entries provided in each of the CDX device detected by > the CDX controller > - vendor id > - device id > - remove > - reset of the device. > - driver override > > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com> > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com> > --- > Documentation/ABI/testing/sysfs-bus-cdx | 34 +++++ > drivers/bus/cdx/cdx.c | 144 ++++++++++++++++++++ > drivers/bus/cdx/controller/cdx_controller.c | 19 +++ > drivers/bus/cdx/controller/mcdi_functions.c | 14 ++ > drivers/bus/cdx/controller/mcdi_functions.h | 11 ++ > include/linux/cdx/cdx_bus.h | 23 ++++ > 6 files changed, 245 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx > index 8c2425fdb6d9..1e0fdce18cde 100644 > --- a/Documentation/ABI/testing/sysfs-bus-cdx > +++ b/Documentation/ABI/testing/sysfs-bus-cdx > @@ -10,3 +10,37 @@ Description: > For example:: > > # echo 1 > /sys/bus/cdx/rescan > + > +What: /sys/bus/cdx/devices/.../vendor > +Date: January 2023 > +Contact: nipun.gupta@amd.com > +Description: > + Vendor ID for this CDX device What format is this in? How big is it? Examples? > + > +What: /sys/bus/cdx/devices/.../device > +Date: January 2023 > +Contact: nipun.gupta@amd.com > +Description: > + Device ID for this CDX device Same here, be specific. > + > +What: /sys/bus/cdx/devices/.../reset > +Date: January 2023 > +Contact: nipun.gupta@amd.com > +Description: > + Writing a non-zero value to this file would reset the > + CDX device > + > + For example:: > + > + # echo 1 > /sys/bus/cdx/.../reset Will that remove the device from the driver too? Again, more documentation please. > + > +What: /sys/bus/cdx/devices/.../remove > +Date: January 2023 > +Contact: tarak.reddy@amd.com > +Description: > + Writing a non-zero value to this file would remove the > + corrosponding device from the CDX bus > + > + For example:: > + > + # echo 1 > /sys/bus/cdx/devices/.../remove Why would you want to remove a device from the bus like this? > diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c > index 2737470f08d3..1372d8dcaa4c 100644 > --- a/drivers/bus/cdx/cdx.c > +++ b/drivers/bus/cdx/cdx.c > @@ -72,6 +72,38 @@ static DECLARE_BITMAP(cdx_controller_id_map, MAX_CDX_CONTROLLERS); > /* List of CDX controllers registered with the CDX bus */ > static LIST_HEAD(cdx_controllers); > > +/** > + * reset_cdx_device - Reset a CDX device > + * @dev: CDX device > + * @data: This is always passed as NULL, and is not used in this API, > + * but is required here as the bus_for_each_dev() API expects > + * the passed function (reset_cdx_device) to have this > + * as an argument. > + * > + * @return: -errno on failure, 0 on success. I recommend this function actually be the one without the data pointer, that way you don't get confused here. > + */ > +static int reset_cdx_device(struct device *dev, void *data) > +{ > + struct cdx_device *cdx_dev = to_cdx_device(dev); > + struct cdx_controller *cdx = cdx_dev->cdx; > + struct cdx_device_config dev_config; > + int ret; > + > + dev_config.type = CDX_DEV_RESET_CONF; > + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, > + cdx_dev->dev_num, &dev_config); > + if (ret) > + dev_err(dev, "cdx device reset failed\n"); > + > + return ret; > +} > + > +int cdx_dev_reset(struct device *dev) > +{ > + return reset_cdx_device(dev, NULL); > +} > +EXPORT_SYMBOL_GPL(cdx_dev_reset); Remove the indirection as mentioned above please. Otherwise, very minor comments, nice work. greg k-h
[AMD Official Use Only - General] > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Tuesday, January 17, 2023 7:44 PM > To: Gupta, Nipun <Nipun.Gupta@amd.com> > Cc: robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; rafael@kernel.org; > eric.auger@redhat.com; alex.williamson@redhat.com; cohuck@redhat.com; > song.bao.hua@hisilicon.com; mchehab+huawei@kernel.org; maz@kernel.org; > f.fainelli@gmail.com; jeffrey.l.hugo@gmail.com; saravanak@google.com; > Michael.Srba@seznam.cz; mani@kernel.org; yishaih@nvidia.com; > jgg@ziepe.ca; jgg@nvidia.com; robin.murphy@arm.com; will@kernel.org; > joro@8bytes.org; masahiroy@kernel.org; ndesaulniers@google.com; linux-arm- > kernel@lists.infradead.org; linux-kbuild@vger.kernel.org; linux- > kernel@vger.kernel.org; devicetree@vger.kernel.org; okaya@kernel.org; > Anand, Harpreet <harpreet.anand@amd.com>; Agarwal, Nikhil > <nikhil.agarwal@amd.com>; Simek, Michal <michal.simek@amd.com>; git > (AMD-Xilinx) <git@amd.com> > Subject: Re: [PATCH 07/19] bus/cdx: add device attributes > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On Tue, Jan 17, 2023 at 07:11:39PM +0530, Nipun Gupta wrote: > > Create sysfs entry for CDX devices. > > > > Sysfs entries provided in each of the CDX device detected by > > the CDX controller > > - vendor id > > - device id > > - remove > > - reset of the device. > > - driver override > > > > Signed-off-by: Puneet Gupta <puneet.gupta@amd.com> > > Signed-off-by: Nipun Gupta <nipun.gupta@amd.com> > > Signed-off-by: Tarak Reddy <tarak.reddy@amd.com> > > --- > > Documentation/ABI/testing/sysfs-bus-cdx | 34 +++++ > > drivers/bus/cdx/cdx.c | 144 ++++++++++++++++++++ > > drivers/bus/cdx/controller/cdx_controller.c | 19 +++ > > drivers/bus/cdx/controller/mcdi_functions.c | 14 ++ > > drivers/bus/cdx/controller/mcdi_functions.h | 11 ++ > > include/linux/cdx/cdx_bus.h | 23 ++++ > > 6 files changed, 245 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-cdx > b/Documentation/ABI/testing/sysfs-bus-cdx > > index 8c2425fdb6d9..1e0fdce18cde 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-cdx > > +++ b/Documentation/ABI/testing/sysfs-bus-cdx > > @@ -10,3 +10,37 @@ Description: > > For example:: > > > > # echo 1 > /sys/bus/cdx/rescan > > + > > +What: /sys/bus/cdx/devices/.../vendor > > +Date: January 2023 > > +Contact: nipun.gupta@amd.com > > +Description: > > + Vendor ID for this CDX device > > What format is this in? How big is it? Examples? This is 16 bits. Agree need to add more info on this and other params. Will update in next version. > > > + > > +What: /sys/bus/cdx/devices/.../device > > +Date: January 2023 > > +Contact: nipun.gupta@amd.com > > +Description: > > + Device ID for this CDX device > > Same here, be specific. > > > + > > +What: /sys/bus/cdx/devices/.../reset > > +Date: January 2023 > > +Contact: nipun.gupta@amd.com > > +Description: > > + Writing a non-zero value to this file would reset the > > + CDX device > > + > > + For example:: > > + > > + # echo 1 > /sys/bus/cdx/.../reset > > Will that remove the device from the driver too? No, it will not remove the device from the driver. We would Introduce reset_done and reset_prepare callbacks which would inform the device driver about reset in next spin. > > Again, more documentation please. Sure. > > > + > > +What: /sys/bus/cdx/devices/.../remove > > +Date: January 2023 > > +Contact: tarak.reddy@amd.com > > +Description: > > + Writing a non-zero value to this file would remove the > > + corrosponding device from the CDX bus > > + > > + For example:: > > + > > + # echo 1 > /sys/bus/cdx/devices/.../remove > > Why would you want to remove a device from the bus like this? This is required to prepare the bus for applying new fabric configuration (pseudo hot plug). So that driver does not access the device while it is being reconfigured. > > > diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c > > index 2737470f08d3..1372d8dcaa4c 100644 > > --- a/drivers/bus/cdx/cdx.c > > +++ b/drivers/bus/cdx/cdx.c > > @@ -72,6 +72,38 @@ static DECLARE_BITMAP(cdx_controller_id_map, > MAX_CDX_CONTROLLERS); > > /* List of CDX controllers registered with the CDX bus */ > > static LIST_HEAD(cdx_controllers); > > > > +/** > > + * reset_cdx_device - Reset a CDX device > > + * @dev: CDX device > > + * @data: This is always passed as NULL, and is not used in this API, > > + * but is required here as the bus_for_each_dev() API expects > > + * the passed function (reset_cdx_device) to have this > > + * as an argument. > > + * > > + * @return: -errno on failure, 0 on success. > > I recommend this function actually be the one without the data pointer, > that way you don't get confused here. Agree will update this. > > > + */ > > +static int reset_cdx_device(struct device *dev, void *data) > > +{ > > + struct cdx_device *cdx_dev = to_cdx_device(dev); > > + struct cdx_controller *cdx = cdx_dev->cdx; > > + struct cdx_device_config dev_config; > > + int ret; > > + > > + dev_config.type = CDX_DEV_RESET_CONF; > > + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, > > + cdx_dev->dev_num, &dev_config); > > + if (ret) > > + dev_err(dev, "cdx device reset failed\n"); > > + > > + return ret; > > +} > > + > > +int cdx_dev_reset(struct device *dev) > > +{ > > + return reset_cdx_device(dev, NULL); > > +} > > +EXPORT_SYMBOL_GPL(cdx_dev_reset); > > Remove the indirection as mentioned above please. > > Otherwise, very minor comments, nice work. > > greg k-h
diff --git a/Documentation/ABI/testing/sysfs-bus-cdx b/Documentation/ABI/testing/sysfs-bus-cdx index 8c2425fdb6d9..1e0fdce18cde 100644 --- a/Documentation/ABI/testing/sysfs-bus-cdx +++ b/Documentation/ABI/testing/sysfs-bus-cdx @@ -10,3 +10,37 @@ Description: For example:: # echo 1 > /sys/bus/cdx/rescan + +What: /sys/bus/cdx/devices/.../vendor +Date: January 2023 +Contact: nipun.gupta@amd.com +Description: + Vendor ID for this CDX device + +What: /sys/bus/cdx/devices/.../device +Date: January 2023 +Contact: nipun.gupta@amd.com +Description: + Device ID for this CDX device + +What: /sys/bus/cdx/devices/.../reset +Date: January 2023 +Contact: nipun.gupta@amd.com +Description: + Writing a non-zero value to this file would reset the + CDX device + + For example:: + + # echo 1 > /sys/bus/cdx/.../reset + +What: /sys/bus/cdx/devices/.../remove +Date: January 2023 +Contact: tarak.reddy@amd.com +Description: + Writing a non-zero value to this file would remove the + corrosponding device from the CDX bus + + For example:: + + # echo 1 > /sys/bus/cdx/devices/.../remove diff --git a/drivers/bus/cdx/cdx.c b/drivers/bus/cdx/cdx.c index 2737470f08d3..1372d8dcaa4c 100644 --- a/drivers/bus/cdx/cdx.c +++ b/drivers/bus/cdx/cdx.c @@ -72,6 +72,38 @@ static DECLARE_BITMAP(cdx_controller_id_map, MAX_CDX_CONTROLLERS); /* List of CDX controllers registered with the CDX bus */ static LIST_HEAD(cdx_controllers); +/** + * reset_cdx_device - Reset a CDX device + * @dev: CDX device + * @data: This is always passed as NULL, and is not used in this API, + * but is required here as the bus_for_each_dev() API expects + * the passed function (reset_cdx_device) to have this + * as an argument. + * + * @return: -errno on failure, 0 on success. + */ +static int reset_cdx_device(struct device *dev, void *data) +{ + struct cdx_device *cdx_dev = to_cdx_device(dev); + struct cdx_controller *cdx = cdx_dev->cdx; + struct cdx_device_config dev_config; + int ret; + + dev_config.type = CDX_DEV_RESET_CONF; + ret = cdx->ops->dev_configure(cdx, cdx_dev->bus_num, + cdx_dev->dev_num, &dev_config); + if (ret) + dev_err(dev, "cdx device reset failed\n"); + + return ret; +} + +int cdx_dev_reset(struct device *dev) +{ + return reset_cdx_device(dev, NULL); +} +EXPORT_SYMBOL_GPL(cdx_dev_reset); + /** * cdx_unregister_device - Unregister a CDX device * @dev: CDX device @@ -238,6 +270,117 @@ static int cdx_dma_configure(struct device *dev) return 0; } +/* show configuration fields */ +#define cdx_config_attr(field, format_string) \ +static ssize_t \ +field##_show(struct device *dev, struct device_attribute *attr, char *buf) \ +{ \ + struct cdx_device *cdx_dev = to_cdx_device(dev); \ + return sysfs_emit(buf, format_string, cdx_dev->field); \ +} \ +static DEVICE_ATTR_RO(field) + +cdx_config_attr(vendor, "0x%04x\n"); +cdx_config_attr(device, "0x%04x\n"); + +static ssize_t remove_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + unsigned long val = 0; + + if (kstrtoul(buf, 0, &val) < 0) + return -EINVAL; + + if (!val) + return -EINVAL; + + if (device_remove_file_self(dev, attr)) { + int ret; + + ret = cdx_unregister_device(dev, NULL); + if (ret) + return ret; + } + + return count; +} +static DEVICE_ATTR_WO(remove); + +static ssize_t reset_store(struct device *dev, struct device_attribute *attr, + const char *buf, size_t count) +{ + unsigned long val = 0; + int ret = 0; + + if (kstrtoul(buf, 0, &val) < 0) + return -EINVAL; + + if (!val) + return -EINVAL; + + ret = reset_cdx_device(dev, NULL); + if (ret) + return ret; + + return count; +} +static DEVICE_ATTR_WO(reset); + +static ssize_t driver_override_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct cdx_device *cdx_dev = to_cdx_device(dev); + const char *old = cdx_dev->driver_override; + char *driver_override; + char *cp; + + if (WARN_ON(dev->bus != &cdx_bus_type)) + return -EINVAL; + + if (count >= (PAGE_SIZE - 1)) + return -EINVAL; + + driver_override = kstrndup(buf, count, GFP_KERNEL); + if (!driver_override) + return -ENOMEM; + + cp = strchr(driver_override, '\n'); + if (cp) + *cp = '\0'; + + if (strlen(driver_override)) { + cdx_dev->driver_override = driver_override; + } else { + kfree(driver_override); + cdx_dev->driver_override = NULL; + } + + kfree(old); + + return count; +} + +static ssize_t driver_override_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct cdx_device *cdx_dev = to_cdx_device(dev); + + return sysfs_emit(buf, "%s\n", cdx_dev->driver_override); +} +static DEVICE_ATTR_RW(driver_override); + +static struct attribute *cdx_dev_attrs[] = { + &dev_attr_remove.attr, + &dev_attr_reset.attr, + &dev_attr_vendor.attr, + &dev_attr_device.attr, + &dev_attr_driver_override.attr, + NULL, +}; +ATTRIBUTE_GROUPS(cdx_dev); + static ssize_t rescan_store(struct bus_type *bus, const char *buf, size_t count) { @@ -280,6 +423,7 @@ struct bus_type cdx_bus_type = { .shutdown = cdx_shutdown, .dma_configure = cdx_dma_configure, .bus_groups = cdx_bus_groups, + .dev_groups = cdx_dev_groups, }; EXPORT_SYMBOL_GPL(cdx_bus_type); diff --git a/drivers/bus/cdx/controller/cdx_controller.c b/drivers/bus/cdx/controller/cdx_controller.c index b62a6d3b7bd4..dbcd236b360d 100644 --- a/drivers/bus/cdx/controller/cdx_controller.c +++ b/drivers/bus/cdx/controller/cdx_controller.c @@ -85,6 +85,24 @@ void cdx_rpmsg_pre_remove(struct cdx_controller *cdx) cdx_mcdi_wait_for_quiescence(cdx->priv, MCDI_RPC_TIMEOUT); } +static int cdx_configure_device(struct cdx_controller *cdx, + u8 bus_num, u8 dev_num, + struct cdx_device_config *dev_config) +{ + int ret = 0; + + switch (dev_config->type) { + case CDX_DEV_RESET_CONF: + ret = cdx_mcdi_reset_device(cdx->priv, bus_num, dev_num); + break; + default: + dev_err(cdx->dev, "Invalid device configuration flag\n"); + ret = -EINVAL; + } + + return ret; +} + static int cdx_scan_devices(struct cdx_controller *cdx) { struct cdx_mcdi *cdx_mcdi = cdx->priv; @@ -144,6 +162,7 @@ static int cdx_scan_devices(struct cdx_controller *cdx) static struct cdx_ops cdx_ops = { .scan = cdx_scan_devices, + .dev_configure = cdx_configure_device, }; static int xlnx_cdx_probe(struct platform_device *pdev) diff --git a/drivers/bus/cdx/controller/mcdi_functions.c b/drivers/bus/cdx/controller/mcdi_functions.c index 3940a2c7919c..673b3896411e 100644 --- a/drivers/bus/cdx/controller/mcdi_functions.c +++ b/drivers/bus/cdx/controller/mcdi_functions.c @@ -123,3 +123,17 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx, return 0; } + +int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num) +{ + MCDI_DECLARE_BUF(inbuf, MC_CMD_CDX_DEVICE_RESET_IN_LEN); + int rc; + + MCDI_SET_DWORD(inbuf, CDX_DEVICE_RESET_IN_BUS, bus_num); + MCDI_SET_DWORD(inbuf, CDX_DEVICE_RESET_IN_DEVICE, dev_num); + + rc = cdx_mcdi_rpc(cdx, MC_CMD_CDX_DEVICE_RESET, inbuf, sizeof(inbuf), + NULL, 0, NULL); + + return rc; +} diff --git a/drivers/bus/cdx/controller/mcdi_functions.h b/drivers/bus/cdx/controller/mcdi_functions.h index 97dfde18592f..54d6017ab7d1 100644 --- a/drivers/bus/cdx/controller/mcdi_functions.h +++ b/drivers/bus/cdx/controller/mcdi_functions.h @@ -47,4 +47,15 @@ int cdx_mcdi_get_dev_config(struct cdx_mcdi *cdx, u8 bus_num, u8 dev_num, struct cdx_dev_params *dev_params); +/** + * cdx_mcdi_reset_device - Reset cdx device represented by bus_num:dev_num + * @cdx: pointer to MCDI interface. + * @bus_num: Bus number. + * @dev_num: Device number. + * + * @return: 0 on success, <0 on failure + */ +int cdx_mcdi_reset_device(struct cdx_mcdi *cdx, + u8 bus_num, u8 dev_num); + #endif /* CDX_MCDI_FUNCTIONS_H */ diff --git a/include/linux/cdx/cdx_bus.h b/include/linux/cdx/cdx_bus.h index 1d28f11da5e7..9f055739ed30 100644 --- a/include/linux/cdx/cdx_bus.h +++ b/include/linux/cdx/cdx_bus.h @@ -21,8 +21,20 @@ /* Forward declaration for CDX controller */ struct cdx_controller; +enum { + CDX_DEV_RESET_CONF, +}; + +struct cdx_device_config { + u8 type; +}; + typedef int (*cdx_scan_cb)(struct cdx_controller *cdx); +typedef int (*cdx_dev_configure_cb)(struct cdx_controller *cdx, + u8 bus_num, u8 dev_num, + struct cdx_device_config *dev_config); + /** * CDX_DEVICE_DRIVER_OVERRIDE - macro used to describe a CDX device with * override_only flags. @@ -39,9 +51,12 @@ typedef int (*cdx_scan_cb)(struct cdx_controller *cdx); /** * struct cdx_ops - Callbacks supported by CDX controller. * @scan: scan the devices on the controller + * @dev_configure: configuration like reset, master_enable, + * msi_config etc for a CDX device */ struct cdx_ops { cdx_scan_cb scan; + cdx_dev_configure_cb dev_configure; }; /** @@ -150,4 +165,12 @@ void cdx_driver_unregister(struct cdx_driver *cdx_driver); extern struct bus_type cdx_bus_type; +/** + * cdx_dev_reset - Reset CDX device + * @dev: device pointer + * + * @return: 0 for success, -errno on failure + */ +int cdx_dev_reset(struct device *dev); + #endif /* _CDX_BUS_H_ */