Hi Geert, On 11/19/18 8:24 PM, Geert Uytterhoeven wrote: > Hi Eric, > > On Mon, Nov 19, 2018 at 11:52 AM Auger Eric <eric.auger@xxxxxxxxxx> wrote: >> On 11/13/18 2:15 PM, Geert Uytterhoeven wrote: >>> Vfio-platform requires dedicated reset support, provided either by ACPI, >>> or, on DT platforms, by a device-specific reset driver matching against >>> the device's compatible value. >>> >>> On many SoCs, devices are connected to an SoC-internal reset controller. >>> If the reset hierarchy is described in DT using "resets" properties, or >>> in lookup tables in platform code, and devices have exactly one >>> dedicated reset each, such devices can be reset in a generic way through >>> the reset controller subsystem. Hence add support for this, avoiding >>> the need to write device-specific reset drivers for each single device >>> on affected SoCs. >>> >>> Devices that do require a more complex reset procedure can still provide >>> a device-specific reset driver, as that takes precedence. >>> >>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and >>> becomes a no-op (as in: "No reset function found for device") if reset >>> controller support is disabled. >>> >>> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> >>> Reviewed-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx> >>> Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx> >>> --- >>> This depends on "[PATCH] reset: Add reset_control_get_count()" >>> (https://lore.kernel.org/lkml/20181113124744.7769-1-geert+renesas@xxxxxxxxx/). >>> >>> v5: >>> - Use reset_control_get_exclusive() instead of rejected >>> reset_control_get_dedicated(), as exclusive already implies a >>> dedicated reset line, >>> - Ensure the device has exactly one reset, >>> >>> v4: >>> - Add Reviewed-by, >>> - Use new RFC reset_control_get_dedicated() instead of >>> of_reset_control_get_exclusive(), to (a) make it more generic, and >>> (b) make sure the reset returned is really a dedicated reset, and >>> does not affect other devices, >>> >>> v3: >>> - Add Reviewed-by, >>> - Merge similar checks in vfio_platform_has_reset(), >>> >>> v2: >>> - Don't store error values in vdev->reset_control, >>> - Use of_reset_control_get_exclusive() instead of >>> __of_reset_control_get(), >>> - Improve description. >>> --- >>> drivers/vfio/platform/vfio_platform_common.c | 29 +++++++++++++++++-- >>> drivers/vfio/platform/vfio_platform_private.h | 1 + >>> 2 files changed, 28 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c >>> index c0cd824be2b767be..ce2aad8e0a8159f9 100644 >>> --- a/drivers/vfio/platform/vfio_platform_common.c >>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>> @@ -18,6 +18,7 @@ >>> #include <linux/module.h> >>> #include <linux/mutex.h> >>> #include <linux/pm_runtime.h> >>> +#include <linux/reset.h> >>> #include <linux/slab.h> >>> #include <linux/types.h> >>> #include <linux/uaccess.h> >>> @@ -113,11 +114,14 @@ static bool vfio_platform_has_reset(struct vfio_platform_device *vdev) >>> if (VFIO_PLATFORM_IS_ACPI(vdev)) >>> return vfio_platform_acpi_has_reset(vdev); >>> >>> - return vdev->of_reset ? true : false; >>> + return vdev->of_reset || vdev->reset_control; >>> } >>> >>> static int vfio_platform_get_reset(struct vfio_platform_device *vdev) >>> { >>> + struct reset_control *rstc; >>> + int count; >>> + >>> if (VFIO_PLATFORM_IS_ACPI(vdev)) >>> return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; >>> >>> @@ -128,8 +132,24 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev) >>> vdev->of_reset = vfio_platform_lookup_reset(vdev->compat, >>> &vdev->reset_module); >>> } >>> + if (vdev->of_reset) >>> + return 0; >>> + >>> + /* Generic reset handling needs a single, dedicated reset line */ >>> + count = reset_control_get_count(vdev->device); >>> + if (count < 0) >>> + return count; >>> + >>> + if (count != 1) >>> + return -EINVAL; >>> >>> - return vdev->of_reset ? 0 : -ENOENT; >>> + rstc = reset_control_get_exclusive(vdev->device, NULL); >> >> So I understand the semantics of reset_control_get_exclusive() is now >> agreed and this means the reset line is not shared with other devices, >> correct? > > Yes, it has been clarified that the intended semantics of an exclusive > reset are that the reset line is not shared with other devices. > However, currently the reset controller subsystem does not enforce this. > > Given the clarification, this is something to fix in the reset > controller subsystem, and thus orthogonal to this vfio patch. > My patch "[PATCH v3] reset: Exclusive resets must be dedicated to a > single hardware block" > (https://lore.kernel.org/lkml/20181113133520.20889-1-geert+renesas@xxxxxxxxx/) > is meant to fix this. OK thank you for the clarification. So from a functional point of view I would consider the above patch is a dependency that needs to be resolved before getting this upstreamed. > >> A question about the usage of reset_control_get_count(). Why is it an >> issue if the count is > 1? Does it check there are several reset lines, >> each of then being used for resetting something different or does it >> check there are several reset controllers are able to do the reset job? >> My point behind is what's the issue as long as one non shared line can >> be grabbed with reset_control_get_exclusive(). > > A device may have multiple resets[*]. If this is the case, a specific > procedure (e.g. reset order) may be needed to reset the device fully. > Assuming that just asserting the first reset will do the job may lead > to failure. > Hence the generic method, which is the target of my patch, should only > be applied for devices with a single reset. > Devices that do require a more complex reset procedure can still provide > a device-specific reset driver, as that takes precedence. > > [*] git grep "\<resets\>.*=.*," -- "*dts*" gives +200 hits. OK understood, this fully clarifies. > >>> + if (!IS_ERR(rstc)) { >>> + vdev->reset_control = rstc; >>> + return 0; >>> + } >>> + >>> + return PTR_ERR(rstc); >>> } >>> >>> static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >>> @@ -139,6 +159,8 @@ static void vfio_platform_put_reset(struct vfio_platform_device *vdev) >>> >>> if (vdev->of_reset) >>> module_put(vdev->reset_module); >>> + >>> + reset_control_put(vdev->reset_control); >> >> Most of the drivers use devm_reset_control_get_exclusive(), can't we use >> that instead and benefit from the fact the reset_control_put() is called >> automatically on driver detach? > > Yes, we could use devm_reset_control_get_exclusive() instead. > Given we still need manual release of a device-specific reset_module, > and a manual call to vfio_iommu_group_put(), it wouldn't simplify the error > and removal path much, though, and even lead to confusion. Yep I don't have a strong opinion here, as long as the put is done ;-) With respect to that patch and as long as "[PATCH v3] reset: Exclusive resets must be dedicated to a single hardware block" functionality is enforced, Acked-by: Eric Auger <eric.auger@xxxxxxxxxx> Thanks Eric > > Gr{oetje,eeting}s, > > Geert >
