* [PATCH] vfio: platform: Fix using devices in PM Domains @ 2018-04-11 9:15 Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 0/2] vfio: platform: Improve reset support Geert Uytterhoeven ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-11 9:15 UTC (permalink / raw) To: Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel, Geert Uytterhoeven If a device is part of a PM Domain (e.g. power and/or clock domain), its power state is managed using Runtime PM. Without Runtime PM, the device may not be powered up, causing subtle failures, crashes, or system lock-ups when the device is accessed by the guest. Fix this by adding Runtime PM support, powering the device when the VFIO device is opened by the guest. Note that while more fine-grained power management could be implemented on the guest side, if exported, this would be inherently unsafe, as abusing it may kill the whole system. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/vfio/platform/vfio_platform_common.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 3c13327b2777f8ec..9d46fce2446daebe 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -17,6 +17,7 @@ #include <linux/iommu.h> #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> @@ -255,6 +256,8 @@ static void vfio_platform_release(void *device_data) const char *extra_dbg = NULL; int ret; + pm_runtime_put(vdev->device); + ret = vfio_platform_call_reset(vdev, &extra_dbg); if (ret && vdev->reset_required) { dev_warn(vdev->device, "reset driver is required and reset call failed in release (%d) %s\n", @@ -297,6 +300,10 @@ static int vfio_platform_open(void *device_data) ret, extra_dbg ? extra_dbg : ""); goto err_rst; } + + ret = pm_runtime_get_sync(vdev->device); + if (ret < 0) + goto err_rst; } vdev->refcnt++; @@ -712,6 +719,7 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, mutex_init(&vdev->igate); + pm_runtime_enable(vdev->device); return 0; put_iommu: @@ -729,6 +737,7 @@ struct vfio_platform_device *vfio_platform_remove_common(struct device *dev) vdev = vfio_del_group_dev(dev); if (vdev) { + pm_runtime_disable(vdev->device); vfio_platform_put_reset(vdev); vfio_iommu_group_put(dev->iommu_group, dev); } -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v3 0/2] vfio: platform: Improve reset support 2018-04-11 9:15 [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven @ 2018-04-11 9:15 ` Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path Geert Uytterhoeven ` (2 subsequent siblings) 3 siblings, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-11 9:15 UTC (permalink / raw) To: Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel, Geert Uytterhoeven Hi all, This patch series improves reset support for vfio-platform: - The first patch fixes a bug I ran into while working on this. - The second patch implements generic DT reset controller support, for devices that are connected to an SoC-internal reset controller and can be reset in a generic way. This avoids having to write/change a vfio-specific reset driver for each and every device to be passed-through to a guest. Changes compared to v2: - Add Reviewed-by, - Merge similar checks in vfio_platform_has_reset(). Changes compared to v1: - Add Reviewed-by, - Don't store error values in vdev->reset_control, - Use of_reset_control_get_exclusive() instead of __of_reset_control_get(), - Improve description. This has been tested on R-Car H3/Salvator-XS by exporting a GPIO module to a QEMU+KWM guest: the GPIO module is reset before QEMU opens the vfio device, and reset again after QEMU has released it, as can be witnessed by the LEDs connected to the GPIOs. Thanks! Geert Uytterhoeven (2): vfio: platform: Fix reset module leak in error path vfio: platform: Add generic DT reset support drivers/vfio/platform/vfio_platform_common.c | 35 +++++++++++++++++++++------ drivers/vfio/platform/vfio_platform_private.h | 1 + 2 files changed, 29 insertions(+), 7 deletions(-) -- 2.7.4 Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path 2018-04-11 9:15 [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 0/2] vfio: platform: Improve reset support Geert Uytterhoeven @ 2018-04-11 9:15 ` Geert Uytterhoeven 2018-04-13 8:55 ` Auger Eric 2018-05-11 19:45 ` Alex Williamson 2018-04-11 9:15 ` [PATCH v3 2/2] vfio: platform: Add generic DT reset support Geert Uytterhoeven 2018-04-11 9:21 ` [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven 3 siblings, 2 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-11 9:15 UTC (permalink / raw) To: Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel, Geert Uytterhoeven If the IOMMU group setup fails, the reset module is not released. Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Eric Auger <eric.auger@redhat.com> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> --- v3: - Add Reviewed-by, v2: - Add Reviewed-by. --- drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 35469af87f88678e..b60bb5326668498c 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, group = vfio_iommu_group_get(dev); if (!group) { pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); - return -EINVAL; + ret = -EINVAL; + goto put_reset; } ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); - if (ret) { - vfio_iommu_group_put(group, dev); - return ret; - } + if (ret) + goto put_iommu; mutex_init(&vdev->igate); return 0; + +put_iommu: + vfio_iommu_group_put(group, dev); +put_reset: + vfio_platform_put_reset(vdev); + return ret; } EXPORT_SYMBOL_GPL(vfio_platform_probe_common); -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path 2018-04-11 9:15 ` [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path Geert Uytterhoeven @ 2018-04-13 8:55 ` Auger Eric 2018-05-11 19:45 ` Alex Williamson 1 sibling, 0 replies; 20+ messages in thread From: Auger Eric @ 2018-04-13 8:55 UTC (permalink / raw) To: Geert Uytterhoeven, Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel Hi Geert, On 11/04/18 11:15, Geert Uytterhoeven wrote: > If the IOMMU group setup fails, the reset module is not released. > > Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> Acked-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > --- > v3: > - Add Reviewed-by, > > v2: > - Add Reviewed-by. > --- > drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 35469af87f88678e..b60bb5326668498c 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > group = vfio_iommu_group_get(dev); > if (!group) { > pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); > - return -EINVAL; > + ret = -EINVAL; > + goto put_reset; > } > > ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); > - if (ret) { > - vfio_iommu_group_put(group, dev); > - return ret; > - } > + if (ret) > + goto put_iommu; > > mutex_init(&vdev->igate); > > return 0; > + > +put_iommu: > + vfio_iommu_group_put(group, dev); > +put_reset: > + vfio_platform_put_reset(vdev); > + return ret; > } > EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path 2018-04-11 9:15 ` [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path Geert Uytterhoeven 2018-04-13 8:55 ` Auger Eric @ 2018-05-11 19:45 ` Alex Williamson 1 sibling, 0 replies; 20+ messages in thread From: Alex Williamson @ 2018-05-11 19:45 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Baptiste Reynal, Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel On Wed, 11 Apr 2018 11:15:48 +0200 Geert Uytterhoeven <geert+renesas@glider.be> wrote: > If the IOMMU group setup fails, the reset module is not released. > > Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Eric Auger <eric.auger@redhat.com> > Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > --- > v3: > - Add Reviewed-by, > > v2: > - Add Reviewed-by. > --- > drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) Applied to next branch for v4.18 with Eric's ack. Thanks, Alex ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-11 9:15 [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 0/2] vfio: platform: Improve reset support Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path Geert Uytterhoeven @ 2018-04-11 9:15 ` Geert Uytterhoeven 2018-04-12 7:00 ` Simon Horman 2018-04-12 10:31 ` Auger Eric 2018-04-11 9:21 ` [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven 3 siblings, 2 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-11 9:15 UTC (permalink / raw) To: Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel, Geert Uytterhoeven Vfio-platform requires 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, 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@glider.be> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> --- 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 | 20 ++++++++++++++++++-- drivers/vfio/platform/vfio_platform_private.h | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index b60bb5326668498c..ef9b9e3220ebe939 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -17,6 +17,7 @@ #include <linux/iommu.h> #include <linux/module.h> #include <linux/mutex.h> +#include <linux/reset.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/uaccess.h> @@ -112,11 +113,13 @@ 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; + if (VFIO_PLATFORM_IS_ACPI(vdev)) return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; @@ -127,8 +130,16 @@ 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; + + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); + if (!IS_ERR(rstc)) { + vdev->reset_control = rstc; + return 0; + } - return vdev->of_reset ? 0 : -ENOENT; + return PTR_ERR(rstc); } static void vfio_platform_put_reset(struct vfio_platform_device *vdev) @@ -138,6 +149,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); } static int vfio_platform_regions_init(struct vfio_platform_device *vdev) @@ -217,6 +230,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev, } else if (vdev->of_reset) { dev_info(vdev->device, "reset\n"); return vdev->of_reset(vdev); + } else if (vdev->reset_control) { + dev_info(vdev->device, "reset\n"); + return reset_control_reset(vdev->reset_control); } dev_warn(vdev->device, "no reset function found!\n"); diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h index 85ffe5d9d1abd94e..a56e80ae5986540b 100644 --- a/drivers/vfio/platform/vfio_platform_private.h +++ b/drivers/vfio/platform/vfio_platform_private.h @@ -60,6 +60,7 @@ struct vfio_platform_device { const char *compat; const char *acpihid; struct module *reset_module; + struct reset_control *reset_control; struct device *device; /* -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-11 9:15 ` [PATCH v3 2/2] vfio: platform: Add generic DT reset support Geert Uytterhoeven @ 2018-04-12 7:00 ` Simon Horman 2018-04-12 10:31 ` Auger Eric 1 sibling, 0 replies; 20+ messages in thread From: Simon Horman @ 2018-04-12 7:00 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel On Wed, Apr 11, 2018 at 11:15:49AM +0200, Geert Uytterhoeven wrote: > Vfio-platform requires 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, > 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@glider.be> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> Reviewed-by: Simon Horman <horms+renesas@verge.net.au> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-11 9:15 ` [PATCH v3 2/2] vfio: platform: Add generic DT reset support Geert Uytterhoeven 2018-04-12 7:00 ` Simon Horman @ 2018-04-12 10:31 ` Auger Eric 2018-04-12 11:32 ` Geert Uytterhoeven 1 sibling, 1 reply; 20+ messages in thread From: Auger Eric @ 2018-04-12 10:31 UTC (permalink / raw) To: Geert Uytterhoeven, Baptiste Reynal, Alex Williamson Cc: Philipp Zabel, Rob Herring, Mark Rutland, kvm, devicetree, linux-renesas-soc, linux-kernel Hi Geert, Philipp, On 11/04/18 11:15, Geert Uytterhoeven wrote: > Vfio-platform requires 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, > 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@glider.be> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > --- > 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 | 20 ++++++++++++++++++-- > drivers/vfio/platform/vfio_platform_private.h | 1 + > 2 files changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index b60bb5326668498c..ef9b9e3220ebe939 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -17,6 +17,7 @@ > #include <linux/iommu.h> > #include <linux/module.h> > #include <linux/mutex.h> > +#include <linux/reset.h> > #include <linux/slab.h> > #include <linux/types.h> > #include <linux/uaccess.h> > @@ -112,11 +113,13 @@ 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; > + > if (VFIO_PLATFORM_IS_ACPI(vdev)) > return vfio_platform_acpi_has_reset(vdev) ? 0 : -ENOENT; > > @@ -127,8 +130,16 @@ 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; > + > + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); Shouldn't we prefer the top level reset_control_get_exclusive()? To make sure about the exclusive/shared terminology, does get_reset_control_get_exclusive() check we have an exclusive wire between this device and the reset controller? Thanks Eric > + if (!IS_ERR(rstc)) { > + vdev->reset_control = rstc; > + return 0; > + } > > - return vdev->of_reset ? 0 : -ENOENT; > + return PTR_ERR(rstc); > } > > static void vfio_platform_put_reset(struct vfio_platform_device *vdev) > @@ -138,6 +149,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); > } > > static int vfio_platform_regions_init(struct vfio_platform_device *vdev) > @@ -217,6 +230,9 @@ static int vfio_platform_call_reset(struct vfio_platform_device *vdev, > } else if (vdev->of_reset) { > dev_info(vdev->device, "reset\n"); > return vdev->of_reset(vdev); > + } else if (vdev->reset_control) { > + dev_info(vdev->device, "reset\n"); > + return reset_control_reset(vdev->reset_control); > } > > dev_warn(vdev->device, "no reset function found!\n"); > diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h > index 85ffe5d9d1abd94e..a56e80ae5986540b 100644 > --- a/drivers/vfio/platform/vfio_platform_private.h > +++ b/drivers/vfio/platform/vfio_platform_private.h > @@ -60,6 +60,7 @@ struct vfio_platform_device { > const char *compat; > const char *acpihid; > struct module *reset_module; > + struct reset_control *reset_control; > struct device *device; > > /* > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 10:31 ` Auger Eric @ 2018-04-12 11:32 ` Geert Uytterhoeven 2018-04-12 11:49 ` Auger Eric 0 siblings, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-12 11:32 UTC (permalink / raw) To: Auger Eric Cc: Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Eric, On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: > On 11/04/18 11:15, Geert Uytterhoeven wrote: >> Vfio-platform requires 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, >> 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@glider.be> >> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c >> @@ -127,8 +130,16 @@ 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; >> + >> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); > > Shouldn't we prefer the top level reset_control_get_exclusive()? I guess that should work, too. > To make sure about the exclusive/shared terminology, does > get_reset_control_get_exclusive() check we have an exclusive wire > between this device and the reset controller? AFAIU, the "exclusive" means that only a single user can obtain access to the reset, and it does not guarantee that we have an exclusive wire between the device and the reset controller. The latter depends on the SoC's reset topology. If a reset wire is shared by multiple devices (e.g. resets shared by PWM or Display Unit devices on R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, indeed. I guess the same thing can happen with the ACPI "_RST" method? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 11:32 ` Geert Uytterhoeven @ 2018-04-12 11:49 ` Auger Eric 2018-04-12 12:36 ` Sinan Kaya 0 siblings, 1 reply; 20+ messages in thread From: Auger Eric @ 2018-04-12 11:49 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List, Kaya Sinan Hi Geert, On 12/04/18 13:32, Geert Uytterhoeven wrote: > Hi Eric, > > On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >> On 11/04/18 11:15, Geert Uytterhoeven wrote: >>> Vfio-platform requires 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, >>> 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@glider.be> >>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > >>> --- a/drivers/vfio/platform/vfio_platform_common.c >>> +++ b/drivers/vfio/platform/vfio_platform_common.c > >>> @@ -127,8 +130,16 @@ 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; >>> + >>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >> >> Shouldn't we prefer the top level reset_control_get_exclusive()? > > I guess that should work, too. > >> To make sure about the exclusive/shared terminology, does >> get_reset_control_get_exclusive() check we have an exclusive wire >> between this device and the reset controller? > > AFAIU, the "exclusive" means that only a single user can obtain access to > the reset, and it does not guarantee that we have an exclusive wire between > the device and the reset controller. > > The latter depends on the SoC's reset topology. If a reset wire is shared > by multiple devices (e.g. resets shared by PWM or Display Unit devices on > R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, > indeed. So who's going to check this assigned device will not trigger a reset of other non assigned devices sharing the same reset controller? > > I guess the same thing can happen with the ACPI "_RST" method? ACPI spec _RST chapter says about _RST object: "This object executes a reset on the associated device or devices. If included in a device context, the reset must not affect any other ACPI-described de vices; if included in a power resource for reset (_PRR, Section 7.3.26) the reset must affect all ACPI-described devices that reference it. When this object is described in a device context, it executes a function level reset that only affects the device it is associated with; neither parent nor children should be affected by the execution of this reset. Executing this must only result in this device resetting without the device appearing as if it has been removed from the bus altogether, to prevent OSPM re-enumeration of devices on hot-pluggable buses (e.g. USB)." Adding Sinan in copy for clarification. Thanks Eric > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 11:49 ` Auger Eric @ 2018-04-12 12:36 ` Sinan Kaya 2018-04-12 13:12 ` Geert Uytterhoeven 0 siblings, 1 reply; 20+ messages in thread From: Sinan Kaya @ 2018-04-12 12:36 UTC (permalink / raw) To: Auger Eric, Geert Uytterhoeven Cc: Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List On 4/12/2018 7:49 AM, Auger Eric wrote: > Hi Geert, > On 12/04/18 13:32, Geert Uytterhoeven wrote: >> Hi Eric, >> >> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >>> On 11/04/18 11:15, Geert Uytterhoeven wrote: >>>> Vfio-platform requires 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, >>>> 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@glider.be> >>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >> >>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >> >>>> @@ -127,8 +130,16 @@ 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; >>>> + >>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >>> >>> Shouldn't we prefer the top level reset_control_get_exclusive()? >> >> I guess that should work, too. >> >>> To make sure about the exclusive/shared terminology, does >>> get_reset_control_get_exclusive() check we have an exclusive wire >>> between this device and the reset controller? >> >> AFAIU, the "exclusive" means that only a single user can obtain access to >> the reset, and it does not guarantee that we have an exclusive wire between >> the device and the reset controller. >> >> The latter depends on the SoC's reset topology. If a reset wire is shared >> by multiple devices (e.g. resets shared by PWM or Display Unit devices on >> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >> indeed. > > So who's going to check this assigned device will not trigger a reset of > other non assigned devices sharing the same reset controller? I like the direction in general. I was hoping that you'd call it of_reset_control rather than reset_control. Is there anything in the OF spec about what to expect from DT's reset? > >> >> I guess the same thing can happen with the ACPI "_RST" method? > > ACPI spec _RST chapter says about _RST object: > > "This object executes a reset on the associated device > or devices. If included in a device context, the > reset must not affect any other ACPI-described de > vices; if included in a power resource for reset > (_PRR, Section 7.3.26) the reset must affect all ACPI-described devices > that reference it. When this object is described in > a device context, it executes a function level reset that only affects > the device it is associated with; neither parent nor children should be > affected by the execution of this reset. Executing this must only result > in this device resetting without the device appearing as if it > has been removed from the bus altogether, to prevent OSPM re-enumeration > of devices on hot-pluggable buses (e.g. USB)." ACPI spec is clear. We are doing a device specific reset aka function level reset here. It does not impact other devices in the system. In fact, ACPI does not have a clock controller concept. All clock/reset details are hidden from the OS. > > Adding Sinan in copy for clarification. > > Thanks > > Eric >> >> Gr{oetje,eeting}s, >> >> Geert >> > -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 12:36 ` Sinan Kaya @ 2018-04-12 13:12 ` Geert Uytterhoeven 2018-04-12 14:10 ` Philipp Zabel 0 siblings, 1 reply; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-12 13:12 UTC (permalink / raw) To: Sinan Kaya Cc: Auger Eric, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Sinan, On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > On 4/12/2018 7:49 AM, Auger Eric wrote: >> On 12/04/18 13:32, Geert Uytterhoeven wrote: >>> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >>>> On 11/04/18 11:15, Geert Uytterhoeven wrote: >>>>> Vfio-platform requires 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, >>>>> 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@glider.be> >>>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >>> >>>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>> >>>>> @@ -127,8 +130,16 @@ 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; >>>>> + >>>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >>>> >>>> Shouldn't we prefer the top level reset_control_get_exclusive()? >>> >>> I guess that should work, too. >>> >>>> To make sure about the exclusive/shared terminology, does >>>> get_reset_control_get_exclusive() check we have an exclusive wire >>>> between this device and the reset controller? >>> >>> AFAIU, the "exclusive" means that only a single user can obtain access to >>> the reset, and it does not guarantee that we have an exclusive wire between >>> the device and the reset controller. >>> >>> The latter depends on the SoC's reset topology. If a reset wire is shared >>> by multiple devices (e.g. resets shared by PWM or Display Unit devices on >>> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >>> indeed. >> >> So who's going to check this assigned device will not trigger a reset of >> other non assigned devices sharing the same reset controller? > > I like the direction in general. I was hoping that you'd call it of_reset_control > rather than reset_control. You mean vfio_platform_device.of_reset_control? If we switch to reset_control_get_exclusive(), that doesn't make much sense... > Is there anything in the OF spec about what to expect from DT's reset? Documentation/devicetree/bindings/reset/reset.txt says: "A word on where to place reset signal consumers in device tree: It is possible in hardware for a reset signal to affect multiple logically separate HW blocks at once. In this case, it would be unwise to represent this reset signal in the DT node of each affected HW block, since if activated, an unrelated block may be reset. Instead, reset signals should be represented in the DT node where it makes most sense to control it; this may be a bus node if all children of the bus are affected by the reset signal, or an individual HW block node for dedicated reset signals. The intent of this binding is to give appropriate software access to the reset signals in order to manage the HW, rather than to slavishly enumerate the reset signal that affects each HW block." So according to the bindings, a specific reset should apply to a single device only. A quick check shows there are several violators: $ for i in $(git grep -lw resets -- "*dts*"); do grep -w resets $i | sort | uniq -c | sed -e "s@^@$i:@g"; done | grep -v ": 1 " arch/arm/boot/dts/aspeed-g4.dtsi: 14 resets = <&syscon ASPEED_RESET_I2C>; arch/arm/boot/dts/aspeed-g5.dtsi: 14 resets = <&syscon ASPEED_RESET_I2C>; arch/arm/boot/dts/atlas7.dtsi: 2 resets = <&car 29>; arch/arm/boot/dts/gemini.dtsi: 2 resets = <&syscon GEMINI_RESET_IDE>; arch/arm/boot/dts/meson8.dtsi: 2 resets = <&reset RESET_USB_OTG>; arch/arm/boot/dts/meson8b.dtsi: 2 resets = <&reset RESET_USB_OTG>; arch/arm/boot/dts/r8a7743.dtsi: 7 resets = <&cpg 523>; arch/arm/boot/dts/r8a7743.dtsi: 2 resets = <&cpg 703>; arch/arm/boot/dts/r8a7743.dtsi: 2 resets = <&cpg 704>; arch/arm/boot/dts/r8a7745.dtsi: 7 resets = <&cpg 523>; arch/arm/boot/dts/r8a7745.dtsi: 2 resets = <&cpg 703>; arch/arm/boot/dts/r8a7745.dtsi: 2 resets = <&cpg 704>; arch/arm/boot/dts/r8a7790.dtsi: 3 resets = <&cpg 703>; arch/arm/boot/dts/r8a7790.dtsi: 2 resets = <&cpg 704>; arch/arm/boot/dts/r8a7791.dtsi: 2 resets = <&cpg 703>; arch/arm/boot/dts/r8a7791.dtsi: 2 resets = <&cpg 704>; arch/arm/boot/dts/r8a7794.dtsi: 2 resets = <&cpg 703>; arch/arm/boot/dts/r8a7794.dtsi: 2 resets = <&cpg 704>; arch/arm/boot/dts/stih410.dtsi: 2 resets = <&powerdown STIH407_USB2_PORT0_POWERDOWN>, arch/arm/boot/dts/stih410.dtsi: 2 resets = <&powerdown STIH407_USB2_PORT1_POWERDOWN>, arch/arm/boot/dts/stih410.dtsi: 2 resets = <&softreset STIH407_PICOPHY_SOFTRESET>, arch/arm/boot/dts/stih418.dtsi: 2 resets = <&powerdown STIH407_USB2_PORT0_POWERDOWN>, arch/arm/boot/dts/stih418.dtsi: 2 resets = <&powerdown STIH407_USB2_PORT1_POWERDOWN>, arch/arm/boot/dts/stih418.dtsi: 2 resets = <&softreset STIH407_PICOPHY_SOFTRESET>, arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&de_clocks RST_FE0>; arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&usb_clocks RST_USB0_HCI>; arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&usb_clocks RST_USB2_HCI>; arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu RST_BUS_EHCI0>, <&ccu RST_BUS_OHCI0>; arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu RST_BUS_EHCI1>, <&ccu RST_BUS_OHCI1>; arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu RST_BUS_EHCI2>, <&ccu RST_BUS_OHCI2>; arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu RST_BUS_EHCI3>, <&ccu RST_BUS_OHCI3>; arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi: 2 resets = <&reset RESET_USB_OTG>; arch/arm64/boot/dts/nvidia/tegra186.dtsi: 2 resets = <&bpmp TEGRA186_RESET_DSI>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 328>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 7 resets = <&cpg 523>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 700>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 701>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 702>; arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 703>; arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 328>; arch/arm64/boot/dts/renesas/r8a7796.dtsi: 7 resets = <&cpg 523>; arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 702>; arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 703>; arch/arm64/boot/dts/renesas/r8a77965.dtsi: 3 resets = <&cpg 328>; arch/arm64/boot/dts/renesas/r8a77965.dtsi: 7 resets = <&cpg 523>; arch/arm64/boot/dts/renesas/r8a77965.dtsi: 2 resets = <&cpg 702>; arch/arm64/boot/dts/renesas/r8a77965.dtsi: 4 resets = <&cpg 703>; arch/arm64/boot/dts/renesas/r8a77995.dtsi: 4 resets = <&cpg 523>; arch/arm64/boot/dts/renesas/r8a77995.dtsi: 3 resets = <&cpg 703>; $ Perhaps we should start grouping devices sharing a reset signal in a "simple-bus" node? Phillip: any comments? > ACPI spec is clear. We are doing a device specific reset aka function level > reset here. It does not impact other devices in the system. Assumed all ACPI implementations comply, of course. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 13:12 ` Geert Uytterhoeven @ 2018-04-12 14:10 ` Philipp Zabel 2018-04-12 16:02 ` Geert Uytterhoeven 0 siblings, 1 reply; 20+ messages in thread From: Philipp Zabel @ 2018-04-12 14:10 UTC (permalink / raw) To: Geert Uytterhoeven, Sinan Kaya Cc: Auger Eric, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: > Hi Sinan, > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > > On 4/12/2018 7:49 AM, Auger Eric wrote: > > > On 12/04/18 13:32, Geert Uytterhoeven wrote: > > > > On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: > > > > > On 11/04/18 11:15, Geert Uytterhoeven wrote: > > > > > > Vfio-platform requires 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, > > > > > > 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@glider.be> > > > > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > > --- a/drivers/vfio/platform/vfio_platform_common.c > > > > > > +++ b/drivers/vfio/platform/vfio_platform_common.c > > > > > > @@ -127,8 +130,16 @@ 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; > > > > > > + > > > > > > + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); > > > > > > > > > > Shouldn't we prefer the top level reset_control_get_exclusive()? > > > > > > > > I guess that should work, too. > > > > > > > > > To make sure about the exclusive/shared terminology, does > > > > > get_reset_control_get_exclusive() check we have an exclusive wire > > > > > between this device and the reset controller? > > > > > > > > AFAIU, the "exclusive" means that only a single user can obtain access to > > > > the reset, and it does not guarantee that we have an exclusive wire between > > > > the device and the reset controller. > > > > > > > > The latter depends on the SoC's reset topology. If a reset wire is shared > > > > by multiple devices (e.g. resets shared by PWM or Display Unit devices on > > > > R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, > > > > indeed. > > > > > > So who's going to check this assigned device will not trigger a reset of > > > other non assigned devices sharing the same reset controller? If the reset control is requested as exclusive, any other driver requesting the same reset control will fail (or this reset_control_get will fail, whichever comes last). > > I like the direction in general. I was hoping that you'd call it of_reset_control > > rather than reset_control. > > You mean vfio_platform_device.of_reset_control? > If we switch to reset_control_get_exclusive(), that doesn't make much sense... > > > Is there anything in the OF spec about what to expect from DT's reset? > > Documentation/devicetree/bindings/reset/reset.txt says: > > "A word on where to place reset signal consumers in device tree: It is possible > in hardware for a reset signal to affect multiple logically separate HW blocks > at once. In this case, it would be unwise to represent this reset signal in > the DT node of each affected HW block, since if activated, an unrelated block > may be reset. Instead, reset signals should be represented in the DT node > where it makes most sense to control it; this may be a bus node if all > children of the bus are affected by the reset signal, or an individual HW > block node for dedicated reset signals. The intent of this binding is to give > appropriate software access to the reset signals in order to manage the HW, > rather than to slavishly enumerate the reset signal that affects each HW > block." This was written in 2012, and unfortunately the DT binding documentation does not inform hardware development, and has not been updated since. There's generally two kinds of reset uses: - either to bring a device into a known state at a given point in time, which is often done using a timed assert/deassert sequence, - or just to park a device while not in active use (must deassert any time before use, may or may not assert any time after use) For the former case, the above paragraph makes a lot of sense, because when it is necessary to reset a device that shares the reset line with another, either choice between disturbing the other device, or not being able to reset when necessary, is a bad one. The reset controller framework supports those use cases via the reset_control_get_exclusive function variants. But for the latter case, there is absolutely no need to forbid sharing reset lines among multiple devices, as deassertion/assertion can just be handled reference counted, like clocks or power management. The reset controller framework supports those use cases via the reset_control_get_shared function variants. The case we are talking about here is the first one. > So according to the bindings, a specific reset should apply to a single > device only. Indeed sharing reset lines between peripherals has become unexpectedly common, making it impractical to forbid shared resets in the device tree. > A quick check shows there are several violators: > > $ for i in $(git grep -lw resets -- "*dts*"); do grep -w resets $i | > sort | uniq -c | sed -e "s@^@$i:@g"; done | grep -v ": 1 " > arch/arm/boot/dts/aspeed-g4.dtsi: 14 resets = <&syscon ASPEED_RESET_I2C>; > arch/arm/boot/dts/aspeed-g5.dtsi: 14 resets = <&syscon ASPEED_RESET_I2C>; > arch/arm/boot/dts/atlas7.dtsi: 2 resets = <&car 29>; > arch/arm/boot/dts/gemini.dtsi: 2 resets = <&syscon GEMINI_RESET_IDE>; > arch/arm/boot/dts/meson8.dtsi: 2 resets = <&reset RESET_USB_OTG>; > arch/arm/boot/dts/meson8b.dtsi: 2 resets = <&reset RESET_USB_OTG>; > arch/arm/boot/dts/r8a7743.dtsi: 7 resets = <&cpg 523>; > arch/arm/boot/dts/r8a7743.dtsi: 2 resets = <&cpg 703>; > arch/arm/boot/dts/r8a7743.dtsi: 2 resets = <&cpg 704>; > arch/arm/boot/dts/r8a7745.dtsi: 7 resets = <&cpg 523>; > arch/arm/boot/dts/r8a7745.dtsi: 2 resets = <&cpg 703>; > arch/arm/boot/dts/r8a7745.dtsi: 2 resets = <&cpg 704>; > arch/arm/boot/dts/r8a7790.dtsi: 3 resets = <&cpg 703>; > arch/arm/boot/dts/r8a7790.dtsi: 2 resets = <&cpg 704>; > arch/arm/boot/dts/r8a7791.dtsi: 2 resets = <&cpg 703>; > arch/arm/boot/dts/r8a7791.dtsi: 2 resets = <&cpg 704>; > arch/arm/boot/dts/r8a7794.dtsi: 2 resets = <&cpg 703>; > arch/arm/boot/dts/r8a7794.dtsi: 2 resets = <&cpg 704>; > arch/arm/boot/dts/stih410.dtsi: 2 resets = <&powerdown > STIH407_USB2_PORT0_POWERDOWN>, > arch/arm/boot/dts/stih410.dtsi: 2 resets = <&powerdown > STIH407_USB2_PORT1_POWERDOWN>, > arch/arm/boot/dts/stih410.dtsi: 2 resets = <&softreset > STIH407_PICOPHY_SOFTRESET>, > arch/arm/boot/dts/stih418.dtsi: 2 resets = <&powerdown > STIH407_USB2_PORT0_POWERDOWN>, > arch/arm/boot/dts/stih418.dtsi: 2 resets = <&powerdown > STIH407_USB2_PORT1_POWERDOWN>, > arch/arm/boot/dts/stih418.dtsi: 2 resets = <&softreset > STIH407_PICOPHY_SOFTRESET>, > arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&de_clocks RST_FE0>; > arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&usb_clocks RST_USB0_HCI>; > arch/arm/boot/dts/sun9i-a80.dtsi: 2 resets = <&usb_clocks RST_USB2_HCI>; > arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu > RST_BUS_EHCI0>, <&ccu RST_BUS_OHCI0>; > arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu > RST_BUS_EHCI1>, <&ccu RST_BUS_OHCI1>; > arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu > RST_BUS_EHCI2>, <&ccu RST_BUS_OHCI2>; > arch/arm/boot/dts/sunxi-h3-h5.dtsi: 2 resets = <&ccu > RST_BUS_EHCI3>, <&ccu RST_BUS_OHCI3>; > arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi: 2 resets = <&reset > RESET_USB_OTG>; > arch/arm64/boot/dts/nvidia/tegra186.dtsi: 2 resets = <&bpmp > TEGRA186_RESET_DSI>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 328>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 7 resets = <&cpg 523>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 700>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 701>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 702>; > arch/arm64/boot/dts/renesas/r8a7795.dtsi: 3 resets = <&cpg 703>; > arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 328>; > arch/arm64/boot/dts/renesas/r8a7796.dtsi: 7 resets = <&cpg 523>; > arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 702>; > arch/arm64/boot/dts/renesas/r8a7796.dtsi: 3 resets = <&cpg 703>; > arch/arm64/boot/dts/renesas/r8a77965.dtsi: 3 resets = <&cpg 328>; > arch/arm64/boot/dts/renesas/r8a77965.dtsi: 7 resets = <&cpg 523>; > arch/arm64/boot/dts/renesas/r8a77965.dtsi: 2 resets = <&cpg 702>; > arch/arm64/boot/dts/renesas/r8a77965.dtsi: 4 resets = <&cpg 703>; > arch/arm64/boot/dts/renesas/r8a77995.dtsi: 4 resets = <&cpg 523>; > arch/arm64/boot/dts/renesas/r8a77995.dtsi: 3 resets = <&cpg 703>; > $ > > Perhaps we should start grouping devices sharing a reset signal in a > "simple-bus" node? > > Phillip: any comments? For some of those it may be possible, but that is basically just a work- around for reality not matching expectations. There may be other cases where devices sharing a reset line are not even in the same parent node because they are controlled via a different bus. In general, I don't think it is feasible or desirable to force grouping of devices that share the same reset line into a common parent node. My suggestion would be to relax the language in the reset.txt DT bindings doc. regards Philipp ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 14:10 ` Philipp Zabel @ 2018-04-12 16:02 ` Geert Uytterhoeven 2018-04-13 8:52 ` Auger Eric 2018-04-13 9:22 ` Philipp Zabel 0 siblings, 2 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-12 16:02 UTC (permalink / raw) To: Philipp Zabel Cc: Sinan Kaya, Auger Eric, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Philipp, On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: >> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> > On 4/12/2018 7:49 AM, Auger Eric wrote: >> > > On 12/04/18 13:32, Geert Uytterhoeven wrote: >> > > > On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >> > > > > On 11/04/18 11:15, Geert Uytterhoeven wrote: >> > > > > > Vfio-platform requires 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, >> > > > > > 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@glider.be> >> > > > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >> > > > > > --- a/drivers/vfio/platform/vfio_platform_common.c >> > > > > > +++ b/drivers/vfio/platform/vfio_platform_common.c >> > > > > > @@ -127,8 +130,16 @@ 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; >> > > > > > + >> > > > > > + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >> > > > > >> > > > > Shouldn't we prefer the top level reset_control_get_exclusive()? >> > > > >> > > > I guess that should work, too. >> > > > >> > > > > To make sure about the exclusive/shared terminology, does >> > > > > get_reset_control_get_exclusive() check we have an exclusive wire >> > > > > between this device and the reset controller? >> > > > >> > > > AFAIU, the "exclusive" means that only a single user can obtain access to >> > > > the reset, and it does not guarantee that we have an exclusive wire between >> > > > the device and the reset controller. >> > > > >> > > > The latter depends on the SoC's reset topology. If a reset wire is shared >> > > > by multiple devices (e.g. resets shared by PWM or Display Unit devices on >> > > > R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >> > > > indeed. >> > > >> > > So who's going to check this assigned device will not trigger a reset of >> > > other non assigned devices sharing the same reset controller? > > If the reset control is requested as exclusive, any other driver > requesting the same reset control will fail (or this reset_control_get > will fail, whichever comes last). > >> > I like the direction in general. I was hoping that you'd call it of_reset_control >> > rather than reset_control. >> >> You mean vfio_platform_device.of_reset_control? >> If we switch to reset_control_get_exclusive(), that doesn't make much sense... >> >> > Is there anything in the OF spec about what to expect from DT's reset? >> >> Documentation/devicetree/bindings/reset/reset.txt says: >> >> "A word on where to place reset signal consumers in device tree: It is possible >> in hardware for a reset signal to affect multiple logically separate HW blocks >> at once. In this case, it would be unwise to represent this reset signal in >> the DT node of each affected HW block, since if activated, an unrelated block >> may be reset. Instead, reset signals should be represented in the DT node >> where it makes most sense to control it; this may be a bus node if all >> children of the bus are affected by the reset signal, or an individual HW >> block node for dedicated reset signals. The intent of this binding is to give >> appropriate software access to the reset signals in order to manage the HW, >> rather than to slavishly enumerate the reset signal that affects each HW >> block." > > This was written in 2012, and unfortunately the DT binding documentation > does not inform hardware development, and has not been updated since. > > There's generally two kinds of reset uses: > - either to bring a device into a known state at a given point in time, > which is often done using a timed assert/deassert sequence, > - or just to park a device while not in active use (must deassert any > time before use, may or may not assert any time after use) > > For the former case, the above paragraph makes a lot of sense, because > when it is necessary to reset a device that shares the reset line with > another, either choice between disturbing the other device, or not > being able to reset when necessary, is a bad one. The reset controller > framework supports those use cases via the reset_control_get_exclusive > function variants. > > But for the latter case, there is absolutely no need to forbid sharing > reset lines among multiple devices, as deassertion/assertion can just be > handled reference counted, like clocks or power management. The reset > controller framework supports those use cases via the > reset_control_get_shared function variants. > > The case we are talking about here is the first one. Except that vfio-platform wants to reset the device before and after its use by the guest, for isolation reasons, which does cause a major disturbance in case of a shared reset. >> So according to the bindings, a specific reset should apply to a single >> device only. > > Indeed sharing reset lines between peripherals has become unexpectedly > common, making it impractical to forbid shared resets in the device > tree. > >> A quick check shows there are several violators: [...] >> Perhaps we should start grouping devices sharing a reset signal in a >> "simple-bus" node? >> >> Phillip: any comments? > > For some of those it may be possible, but that is basically just a work- > around for reality not matching expectations. There may be other cases > where devices sharing a reset line are not even in the same parent node > because they are controlled via a different bus. In general, I don't > think it is feasible or desirable to force grouping of devices that > share the same reset line into a common parent node. At least for Renesas R-Car SoCs, I think this is feasible, as all affected devices are currently grouped under the same /soc node. I added subnodes for all devices sharing resets (one for pwm, 4 for USB2, and one for USB3; display doesn't have resets yet), and it still boots ;-) However, ehci_platform_probe() cannot get its (optional) resets anymore. Probably the reset controller framework needs to be taught to look for shared resets in the parent node, too? > My suggestion would be to relax the language in the reset.txt DT > bindings doc. Which is fine to keep the status quo with the hardware designers, but makes it less likely for non-whitelisted generic reset controller support to become acceptable for the vfio people... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 16:02 ` Geert Uytterhoeven @ 2018-04-13 8:52 ` Auger Eric 2018-04-13 9:02 ` Geert Uytterhoeven 2018-04-13 9:22 ` Philipp Zabel 1 sibling, 1 reply; 20+ messages in thread From: Auger Eric @ 2018-04-13 8:52 UTC (permalink / raw) To: Geert Uytterhoeven, Philipp Zabel Cc: Sinan Kaya, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Geert, Philipp, On 12/04/18 18:02, Geert Uytterhoeven wrote: > Hi Philipp, > > On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: >>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>>> On 4/12/2018 7:49 AM, Auger Eric wrote: >>>>> On 12/04/18 13:32, Geert Uytterhoeven wrote: >>>>>> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >>>>>>> On 11/04/18 11:15, Geert Uytterhoeven wrote: >>>>>>>> Vfio-platform requires 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, >>>>>>>> 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@glider.be> >>>>>>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >>>>>>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>>>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>>>>>>> @@ -127,8 +130,16 @@ 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; >>>>>>>> + >>>>>>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >>>>>>> >>>>>>> Shouldn't we prefer the top level reset_control_get_exclusive()? >>>>>> >>>>>> I guess that should work, too. >>>>>> >>>>>>> To make sure about the exclusive/shared terminology, does >>>>>>> get_reset_control_get_exclusive() check we have an exclusive wire >>>>>>> between this device and the reset controller? >>>>>> >>>>>> AFAIU, the "exclusive" means that only a single user can obtain access to >>>>>> the reset, and it does not guarantee that we have an exclusive wire between >>>>>> the device and the reset controller. >>>>>> >>>>>> The latter depends on the SoC's reset topology. If a reset wire is shared >>>>>> by multiple devices (e.g. resets shared by PWM or Display Unit devices on >>>>>> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >>>>>> indeed. >>>>> >>>>> So who's going to check this assigned device will not trigger a reset of >>>>> other non assigned devices sharing the same reset controller? >> >> If the reset control is requested as exclusive, any other driver >> requesting the same reset control will fail (or this reset_control_get >> will fail, whichever comes last). >> >>>> I like the direction in general. I was hoping that you'd call it of_reset_control >>>> rather than reset_control. >>> >>> You mean vfio_platform_device.of_reset_control? >>> If we switch to reset_control_get_exclusive(), that doesn't make much sense... >>> >>>> Is there anything in the OF spec about what to expect from DT's reset? >>> >>> Documentation/devicetree/bindings/reset/reset.txt says: >>> >>> "A word on where to place reset signal consumers in device tree: It is possible >>> in hardware for a reset signal to affect multiple logically separate HW blocks >>> at once. In this case, it would be unwise to represent this reset signal in >>> the DT node of each affected HW block, since if activated, an unrelated block >>> may be reset. Instead, reset signals should be represented in the DT node >>> where it makes most sense to control it; this may be a bus node if all >>> children of the bus are affected by the reset signal, or an individual HW >>> block node for dedicated reset signals. The intent of this binding is to give >>> appropriate software access to the reset signals in order to manage the HW, >>> rather than to slavishly enumerate the reset signal that affects each HW >>> block." >> >> This was written in 2012, and unfortunately the DT binding documentation >> does not inform hardware development, and has not been updated since. >> >> There's generally two kinds of reset uses: >> - either to bring a device into a known state at a given point in time, >> which is often done using a timed assert/deassert sequence, >> - or just to park a device while not in active use (must deassert any >> time before use, may or may not assert any time after use) >> >> For the former case, the above paragraph makes a lot of sense, because >> when it is necessary to reset a device that shares the reset line with >> another, either choice between disturbing the other device, or not >> being able to reset when necessary, is a bad one. The reset controller >> framework supports those use cases via the reset_control_get_exclusive >> function variants. >> >> But for the latter case, there is absolutely no need to forbid sharing >> reset lines among multiple devices, as deassertion/assertion can just be >> handled reference counted, like clocks or power management. The reset >> controller framework supports those use cases via the >> reset_control_get_shared function variants. >> >> The case we are talking about here is the first one. > > Except that vfio-platform wants to reset the device before and after its > use by the guest, for isolation reasons, which does cause a major > disturbance in case of a shared reset. Do we have any guarantee that drivers whose device are sharing the reset signal will have got the reset control when the vfio-platform driver calls reset_control_get_exclusive(). In such a case vfio-platform reset_control_get_exclusive() will fail and this is what we want. Otherwise it is unsafe, right. Doesn't this assumption look a little risky? Thanks Eric > >>> So according to the bindings, a specific reset should apply to a single >>> device only. >> >> Indeed sharing reset lines between peripherals has become unexpectedly >> common, making it impractical to forbid shared resets in the device >> tree. >> >>> A quick check shows there are several violators: > > [...] > >>> Perhaps we should start grouping devices sharing a reset signal in a >>> "simple-bus" node? >>> >>> Phillip: any comments? >> >> For some of those it may be possible, but that is basically just a work- >> around for reality not matching expectations. There may be other cases >> where devices sharing a reset line are not even in the same parent node >> because they are controlled via a different bus. In general, I don't >> think it is feasible or desirable to force grouping of devices that >> share the same reset line into a common parent node. > > At least for Renesas R-Car SoCs, I think this is feasible, as all affected > devices are currently grouped under the same /soc node. > I added subnodes for all devices sharing resets (one for pwm, 4 for USB2, > and one for USB3; display doesn't have resets yet), and it still boots ;-) > > However, ehci_platform_probe() cannot get its (optional) resets anymore. > Probably the reset controller framework needs to be taught to look for > shared resets in the parent node, too? > >> My suggestion would be to relax the language in the reset.txt DT >> bindings doc. > > Which is fine to keep the status quo with the hardware designers, but makes > it less likely for non-whitelisted generic reset controller support to > become acceptable for the vfio people... > > Gr{oetje,eeting}s, > > Geert > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-13 8:52 ` Auger Eric @ 2018-04-13 9:02 ` Geert Uytterhoeven 0 siblings, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-13 9:02 UTC (permalink / raw) To: Auger Eric Cc: Philipp Zabel, Sinan Kaya, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Eric, On Fri, Apr 13, 2018 at 10:52 AM, Auger Eric <eric.auger@redhat.com> wrote: > On 12/04/18 18:02, Geert Uytterhoeven wrote: >> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: >>>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >>>>> On 4/12/2018 7:49 AM, Auger Eric wrote: >>>>>> On 12/04/18 13:32, Geert Uytterhoeven wrote: >>>>>>> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >>>>>>>> On 11/04/18 11:15, Geert Uytterhoeven wrote: >>>>>>>>> Vfio-platform requires 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, >>>>>>>>> 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@glider.be> >>>>>>>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >>>>>>>>> --- a/drivers/vfio/platform/vfio_platform_common.c >>>>>>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c >>>>>>>>> @@ -127,8 +130,16 @@ 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; >>>>>>>>> + >>>>>>>>> + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >>>>>>>> >>>>>>>> Shouldn't we prefer the top level reset_control_get_exclusive()? >>>>>>> >>>>>>> I guess that should work, too. >>>>>>> >>>>>>>> To make sure about the exclusive/shared terminology, does >>>>>>>> get_reset_control_get_exclusive() check we have an exclusive wire >>>>>>>> between this device and the reset controller? >>>>>>> >>>>>>> AFAIU, the "exclusive" means that only a single user can obtain access to >>>>>>> the reset, and it does not guarantee that we have an exclusive wire between >>>>>>> the device and the reset controller. >>>>>>> >>>>>>> The latter depends on the SoC's reset topology. If a reset wire is shared >>>>>>> by multiple devices (e.g. resets shared by PWM or Display Unit devices on >>>>>>> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >>>>>>> indeed. >>>>>> >>>>>> So who's going to check this assigned device will not trigger a reset of >>>>>> other non assigned devices sharing the same reset controller? >>> >>> If the reset control is requested as exclusive, any other driver >>> requesting the same reset control will fail (or this reset_control_get >>> will fail, whichever comes last). >>> >>>>> I like the direction in general. I was hoping that you'd call it of_reset_control >>>>> rather than reset_control. >>>> >>>> You mean vfio_platform_device.of_reset_control? >>>> If we switch to reset_control_get_exclusive(), that doesn't make much sense... >>>> >>>>> Is there anything in the OF spec about what to expect from DT's reset? >>>> >>>> Documentation/devicetree/bindings/reset/reset.txt says: >>>> >>>> "A word on where to place reset signal consumers in device tree: It is possible >>>> in hardware for a reset signal to affect multiple logically separate HW blocks >>>> at once. In this case, it would be unwise to represent this reset signal in >>>> the DT node of each affected HW block, since if activated, an unrelated block >>>> may be reset. Instead, reset signals should be represented in the DT node >>>> where it makes most sense to control it; this may be a bus node if all >>>> children of the bus are affected by the reset signal, or an individual HW >>>> block node for dedicated reset signals. The intent of this binding is to give >>>> appropriate software access to the reset signals in order to manage the HW, >>>> rather than to slavishly enumerate the reset signal that affects each HW >>>> block." >>> >>> This was written in 2012, and unfortunately the DT binding documentation >>> does not inform hardware development, and has not been updated since. >>> >>> There's generally two kinds of reset uses: >>> - either to bring a device into a known state at a given point in time, >>> which is often done using a timed assert/deassert sequence, >>> - or just to park a device while not in active use (must deassert any >>> time before use, may or may not assert any time after use) >>> >>> For the former case, the above paragraph makes a lot of sense, because >>> when it is necessary to reset a device that shares the reset line with >>> another, either choice between disturbing the other device, or not >>> being able to reset when necessary, is a bad one. The reset controller >>> framework supports those use cases via the reset_control_get_exclusive >>> function variants. >>> >>> But for the latter case, there is absolutely no need to forbid sharing >>> reset lines among multiple devices, as deassertion/assertion can just be >>> handled reference counted, like clocks or power management. The reset >>> controller framework supports those use cases via the >>> reset_control_get_shared function variants. >>> >>> The case we are talking about here is the first one. >> >> Except that vfio-platform wants to reset the device before and after its >> use by the guest, for isolation reasons, which does cause a major >> disturbance in case of a shared reset. > > Do we have any guarantee that drivers whose device are sharing the reset > signal will have got the reset control when the vfio-platform driver > calls reset_control_get_exclusive(). In such a case vfio-platform > reset_control_get_exclusive() will fail and this is what we want. > Otherwise it is unsafe, right. Doesn't this assumption look a little risky? No we don't: most drivers don't use reset_control at all. I think on the Renesas SoCs only USB and Ethernet PHY (which is BTW external, and thus not covered by the on-SoC reset controller) do. A few more users may be added in the future. But all module resets are described in DT. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-12 16:02 ` Geert Uytterhoeven 2018-04-13 8:52 ` Auger Eric @ 2018-04-13 9:22 ` Philipp Zabel 2018-04-13 10:05 ` Auger Eric 2018-04-13 11:56 ` Geert Uytterhoeven 1 sibling, 2 replies; 20+ messages in thread From: Philipp Zabel @ 2018-04-13 9:22 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Sinan Kaya, Auger Eric, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Geert, On Thu, 2018-04-12 at 18:02 +0200, Geert Uytterhoeven wrote: > Hi Philipp, > > On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: > > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: > > > > On 4/12/2018 7:49 AM, Auger Eric wrote: > > > > > On 12/04/18 13:32, Geert Uytterhoeven wrote: > > > > > > On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: > > > > > > > On 11/04/18 11:15, Geert Uytterhoeven wrote: > > > > > > > > Vfio-platform requires 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, > > > > > > > > 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@glider.be> > > > > > > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> > > > > > > > > --- a/drivers/vfio/platform/vfio_platform_common.c > > > > > > > > +++ b/drivers/vfio/platform/vfio_platform_common.c > > > > > > > > @@ -127,8 +130,16 @@ 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; > > > > > > > > + > > > > > > > > + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); > > > > > > > > > > > > > > Shouldn't we prefer the top level reset_control_get_exclusive()? > > > > > > > > > > > > I guess that should work, too. > > > > > > > > > > > > > To make sure about the exclusive/shared terminology, does > > > > > > > get_reset_control_get_exclusive() check we have an exclusive wire > > > > > > > between this device and the reset controller? > > > > > > > > > > > > AFAIU, the "exclusive" means that only a single user can obtain access to > > > > > > the reset, and it does not guarantee that we have an exclusive wire between > > > > > > the device and the reset controller. > > > > > > > > > > > > The latter depends on the SoC's reset topology. If a reset wire is shared > > > > > > by multiple devices (e.g. resets shared by PWM or Display Unit devices on > > > > > > R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, > > > > > > indeed. > > > > > > > > > > So who's going to check this assigned device will not trigger a reset of > > > > > other non assigned devices sharing the same reset controller? > > > > If the reset control is requested as exclusive, any other driver > > requesting the same reset control will fail (or this reset_control_get > > will fail, whichever comes last). > > > > > > I like the direction in general. I was hoping that you'd call it of_reset_control > > > > rather than reset_control. > > > > > > You mean vfio_platform_device.of_reset_control? > > > If we switch to reset_control_get_exclusive(), that doesn't make much sense... > > > > > > > Is there anything in the OF spec about what to expect from DT's reset? > > > > > > Documentation/devicetree/bindings/reset/reset.txt says: > > > > > > "A word on where to place reset signal consumers in device tree: It is possible > > > in hardware for a reset signal to affect multiple logically separate HW blocks > > > at once. In this case, it would be unwise to represent this reset signal in > > > the DT node of each affected HW block, since if activated, an unrelated block > > > may be reset. Instead, reset signals should be represented in the DT node > > > where it makes most sense to control it; this may be a bus node if all > > > children of the bus are affected by the reset signal, or an individual HW > > > block node for dedicated reset signals. The intent of this binding is to give > > > appropriate software access to the reset signals in order to manage the HW, > > > rather than to slavishly enumerate the reset signal that affects each HW > > > block." > > > > This was written in 2012, and unfortunately the DT binding documentation > > does not inform hardware development, and has not been updated since. > > > > There's generally two kinds of reset uses: > > - either to bring a device into a known state at a given point in time, > > which is often done using a timed assert/deassert sequence, > > - or just to park a device while not in active use (must deassert any > > time before use, may or may not assert any time after use) > > > > For the former case, the above paragraph makes a lot of sense, because > > when it is necessary to reset a device that shares the reset line with > > another, either choice between disturbing the other device, or not > > being able to reset when necessary, is a bad one. The reset controller > > framework supports those use cases via the reset_control_get_exclusive > > function variants. > > > > But for the latter case, there is absolutely no need to forbid sharing > > reset lines among multiple devices, as deassertion/assertion can just be > > handled reference counted, like clocks or power management. The reset > > controller framework supports those use cases via the > > reset_control_get_shared function variants. > > > > The case we are talking about here is the first one. > > Except that vfio-platform wants to reset the device before and after its > use by the guest, for isolation reasons, which does cause a major > disturbance in case of a shared reset. Right. So suddenly we are making devices / drivers that usually would fall into the latter case add a requirement from the first case. That also means it is impossible to use just one of the devices that share a reset line for vfio individually, while the other ones are still in use by the host. Currently the reset line is a shared resource similar to the iommu for devices in the same iommu_group. Is there any mechanism in vfio that would allow modeling other shared resources apart from iommu? [...] > > For some of those it may be possible, but that is basically just a work- > > around for reality not matching expectations. There may be other cases > > where devices sharing a reset line are not even in the same parent node > > because they are controlled via a different bus. In general, I don't > > think it is feasible or desirable to force grouping of devices that > > share the same reset line into a common parent node. > > At least for Renesas R-Car SoCs, I think this is feasible, as all affected > devices are currently grouped under the same /soc node. > I added subnodes for all devices sharing resets (one for pwm, 4 for USB2, > and one for USB3; display doesn't have resets yet), and it still boots ;-) Is this grouping enough to make sure all of the pwm/usb2/usb3 devices are only ever configured for vfio use together? Assuming I have pwm[1-4] all sharing the same reset line, and I want pwm2 to be used by a vfio guest, I first have to make sure that all of pwm[1-4] are unbound, releasing their shared resets, before vfio- platform can request the same reset line as exclusive. Thinking about it, if the pwm drivers keep their requested reset control around for the duration the device is bound, the reset controller framework should already kind of handle this - while any of the shared reset control handles is kept around, any exclusive request for the same reset control will fail with -EBUSY (and the other way around). But that requires all drivers to request the reset control during probe and release it during remove. > However, ehci_platform_probe() cannot get its (optional) resets anymore. > Probably the reset controller framework needs to be taught to look for > shared resets in the parent node, too? Hm, a generic framework shouldn't do such a thing, the parent node could be covered by a completely different binding. > > My suggestion would be to relax the language in the reset.txt DT > > bindings doc. > > Which is fine to keep the status quo with the hardware designers, but makes > it less likely for non-whitelisted generic reset controller support to > become acceptable for the vfio people... I still may be missing context, but I fail to see how pwm@0 { resets = <&shared_reset_control>; }; pwm@1 { resets = <&shared_reset_control>; }; -> pwms { resets = <&shared_reset_control>; pwm@0 { }; pwm@1 { }; }; makes any difference here, unless pwms gets bound to an actual driver that is used for vfio? regards Philipp ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-13 9:22 ` Philipp Zabel @ 2018-04-13 10:05 ` Auger Eric 2018-04-13 11:56 ` Geert Uytterhoeven 1 sibling, 0 replies; 20+ messages in thread From: Auger Eric @ 2018-04-13 10:05 UTC (permalink / raw) To: Philipp Zabel, Geert Uytterhoeven Cc: Sinan Kaya, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Philipp, On 13/04/18 11:22, Philipp Zabel wrote: [..] > That also means it is impossible to use just one of the devices that > share a reset line for vfio individually, while the other ones are still > in use by the host. Currently the reset line is a shared resource > similar to the iommu for devices in the same iommu_group. > > Is there any mechanism in vfio that would allow modeling other shared > resources apart from iommu? No we only check the VFIO group viability at IOMMU level. > > [...] >>> For some of those it may be possible, but that is basically just a work- >>> around for reality not matching expectations. There may be other cases >>> where devices sharing a reset line are not even in the same parent node >>> because they are controlled via a different bus. In general, I don't >>> think it is feasible or desirable to force grouping of devices that >>> share the same reset line into a common parent node. >> >> At least for Renesas R-Car SoCs, I think this is feasible, as all affected >> devices are currently grouped under the same /soc node. >> I added subnodes for all devices sharing resets (one for pwm, 4 for USB2, >> and one for USB3; display doesn't have resets yet), and it still boots ;-) > > Is this grouping enough to make sure all of the pwm/usb2/usb3 devices > are only ever configured for vfio use together? > > Assuming I have pwm[1-4] all sharing the same reset line, and I want > pwm2 to be used by a vfio guest, I first have to make sure that all of > pwm[1-4] are unbound, releasing their shared resets, before vfio- > platform can request the same reset line as exclusive. > > Thinking about it, if the pwm drivers keep their requested reset control > around for the duration the device is bound, the reset controller > framework should already kind of handle this - while any of the shared > reset control handles is kept around, any exclusive request for the same > reset control will fail with -EBUSY (and the other way around). > But that requires all drivers to request the reset control during probe > and release it during remove. > >> However, ehci_platform_probe() cannot get its (optional) resets anymore. >> Probably the reset controller framework needs to be taught to look for >> shared resets in the parent node, too? > > Hm, a generic framework shouldn't do such a thing, the parent node could > be covered by a completely different binding. > >>> My suggestion would be to relax the language in the reset.txt DT >>> bindings doc. >> >> Which is fine to keep the status quo with the hardware designers, but makes >> it less likely for non-whitelisted generic reset controller support to >> become acceptable for the vfio people... > > I still may be missing context, but I fail to see how > > pwm@0 { > resets = <&shared_reset_control>; > }; > > pwm@1 { > resets = <&shared_reset_control>; > }; > > -> > > pwms { > resets = <&shared_reset_control>; > > pwm@0 { > }; > > pwm@1 { > }; > }; > > makes any difference here, unless pwms gets bound to an actual driver > that is used for vfio? I don't think we are ready to assign pwms with VFIO as Alex emphasized VFIO was meant to be used with IOMMU and I guess those devices do not belong to any iommu group. Thanks Eric > > regards > Philipp > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support 2018-04-13 9:22 ` Philipp Zabel 2018-04-13 10:05 ` Auger Eric @ 2018-04-13 11:56 ` Geert Uytterhoeven 1 sibling, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-13 11:56 UTC (permalink / raw) To: Philipp Zabel Cc: Sinan Kaya, Auger Eric, Geert Uytterhoeven, Baptiste Reynal, Alex Williamson, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List Hi Philipp, On Fri, Apr 13, 2018 at 11:22 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Thu, 2018-04-12 at 18:02 +0200, Geert Uytterhoeven wrote: >> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote: >> > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote: >> > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya <okaya@codeaurora.org> wrote: >> > > > On 4/12/2018 7:49 AM, Auger Eric wrote: >> > > > > On 12/04/18 13:32, Geert Uytterhoeven wrote: >> > > > > > On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote: >> > > > > > > On 11/04/18 11:15, Geert Uytterhoeven wrote: >> > > > > > > > Vfio-platform requires 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, >> > > > > > > > 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@glider.be> >> > > > > > > > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de> >> > > > > > > > --- a/drivers/vfio/platform/vfio_platform_common.c >> > > > > > > > +++ b/drivers/vfio/platform/vfio_platform_common.c >> > > > > > > > @@ -127,8 +130,16 @@ 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; >> > > > > > > > + >> > > > > > > > + rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL); >> > > > > > > >> > > > > > > Shouldn't we prefer the top level reset_control_get_exclusive()? >> > > > > > >> > > > > > I guess that should work, too. >> > > > > > >> > > > > > > To make sure about the exclusive/shared terminology, does >> > > > > > > get_reset_control_get_exclusive() check we have an exclusive wire >> > > > > > > between this device and the reset controller? >> > > > > > >> > > > > > AFAIU, the "exclusive" means that only a single user can obtain access to >> > > > > > the reset, and it does not guarantee that we have an exclusive wire between >> > > > > > the device and the reset controller. >> > > > > > >> > > > > > The latter depends on the SoC's reset topology. If a reset wire is shared >> > > > > > by multiple devices (e.g. resets shared by PWM or Display Unit devices on >> > > > > > R-Car SoCs), exporting a subset of these devices to a guest is a bad idea, >> > > > > > indeed. >> > > > > >> > > > > So who's going to check this assigned device will not trigger a reset of >> > > > > other non assigned devices sharing the same reset controller? >> > >> > If the reset control is requested as exclusive, any other driver >> > requesting the same reset control will fail (or this reset_control_get >> > will fail, whichever comes last). >> > >> > > > I like the direction in general. I was hoping that you'd call it of_reset_control >> > > > rather than reset_control. >> > > >> > > You mean vfio_platform_device.of_reset_control? >> > > If we switch to reset_control_get_exclusive(), that doesn't make much sense... >> > > >> > > > Is there anything in the OF spec about what to expect from DT's reset? >> > > >> > > Documentation/devicetree/bindings/reset/reset.txt says: >> > > >> > > "A word on where to place reset signal consumers in device tree: It is possible >> > > in hardware for a reset signal to affect multiple logically separate HW blocks >> > > at once. In this case, it would be unwise to represent this reset signal in >> > > the DT node of each affected HW block, since if activated, an unrelated block >> > > may be reset. Instead, reset signals should be represented in the DT node >> > > where it makes most sense to control it; this may be a bus node if all >> > > children of the bus are affected by the reset signal, or an individual HW >> > > block node for dedicated reset signals. The intent of this binding is to give >> > > appropriate software access to the reset signals in order to manage the HW, >> > > rather than to slavishly enumerate the reset signal that affects each HW >> > > block." >> > >> > This was written in 2012, and unfortunately the DT binding documentation >> > does not inform hardware development, and has not been updated since. >> > >> > There's generally two kinds of reset uses: >> > - either to bring a device into a known state at a given point in time, >> > which is often done using a timed assert/deassert sequence, >> > - or just to park a device while not in active use (must deassert any >> > time before use, may or may not assert any time after use) >> > >> > For the former case, the above paragraph makes a lot of sense, because >> > when it is necessary to reset a device that shares the reset line with >> > another, either choice between disturbing the other device, or not >> > being able to reset when necessary, is a bad one. The reset controller >> > framework supports those use cases via the reset_control_get_exclusive >> > function variants. >> > >> > But for the latter case, there is absolutely no need to forbid sharing >> > reset lines among multiple devices, as deassertion/assertion can just be >> > handled reference counted, like clocks or power management. The reset >> > controller framework supports those use cases via the >> > reset_control_get_shared function variants. >> > >> > The case we are talking about here is the first one. >> >> Except that vfio-platform wants to reset the device before and after its >> use by the guest, for isolation reasons, which does cause a major >> disturbance in case of a shared reset. > > Right. So suddenly we are making devices / drivers that usually would > fall into the latter case add a requirement from the first case. > > That also means it is impossible to use just one of the devices that > share a reset line for vfio individually, while the other ones are still > in use by the host. Currently the reset line is a shared resource > similar to the iommu for devices in the same iommu_group. Correct. > [...] >> > For some of those it may be possible, but that is basically just a work- >> > around for reality not matching expectations. There may be other cases >> > where devices sharing a reset line are not even in the same parent node >> > because they are controlled via a different bus. In general, I don't >> > think it is feasible or desirable to force grouping of devices that >> > share the same reset line into a common parent node. >> >> At least for Renesas R-Car SoCs, I think this is feasible, as all affected >> devices are currently grouped under the same /soc node. >> I added subnodes for all devices sharing resets (one for pwm, 4 for USB2, >> and one for USB3; display doesn't have resets yet), and it still boots ;-) > > Is this grouping enough to make sure all of the pwm/usb2/usb3 devices > are only ever configured for vfio use together? Currently exporting to a guest using vfio-platform is done on a per-device basis. VFIO has to be taught to consider the parent reset, and export them as a group. > Assuming I have pwm[1-4] all sharing the same reset line, and I want > pwm2 to be used by a vfio guest, I first have to make sure that all of > pwm[1-4] are unbound, releasing their shared resets, before vfio- > platform can request the same reset line as exclusive. Right. > Thinking about it, if the pwm drivers keep their requested reset control > around for the duration the device is bound, the reset controller > framework should already kind of handle this - while any of the shared > reset control handles is kept around, any exclusive request for the same > reset control will fail with -EBUSY (and the other way around). > But that requires all drivers to request the reset control during probe > and release it during remove. All of that assumes the pwm driver uses resets, which it currently doesn't. Note that pwm is a bad example, as you probably want to use para-virtualization instead of vfio-platform. >> However, ehci_platform_probe() cannot get its (optional) resets anymore. >> Probably the reset controller framework needs to be taught to look for >> shared resets in the parent node, too? > > Hm, a generic framework shouldn't do such a thing, the parent node could > be covered by a completely different binding. True. So grouping is not an option? Else every single driver needs to be aware of it, and start looking for parent resets if the device resets cannot be found? >> > My suggestion would be to relax the language in the reset.txt DT >> > bindings doc. >> >> Which is fine to keep the status quo with the hardware designers, but makes >> it less likely for non-whitelisted generic reset controller support to >> become acceptable for the vfio people... > > I still may be missing context, but I fail to see how > > pwm@0 { > resets = <&shared_reset_control>; > }; > > pwm@1 { > resets = <&shared_reset_control>; > }; > > -> > > pwms { > resets = <&shared_reset_control>; > > pwm@0 { > }; > > pwm@1 { > }; > }; > > makes any difference here, unless pwms gets bound to an actual driver > that is used for vfio? I just suggested the grouping to comply with the current DT reset bindings, i.e. to avoid adding the same resets property to multiple nodes. Doing so has the side effect that even after my patch, a single pwm device can still not be exported to a guest, as it doesn't have a resets property anymore. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] vfio: platform: Fix using devices in PM Domains 2018-04-11 9:15 [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven ` (2 preceding siblings ...) 2018-04-11 9:15 ` [PATCH v3 2/2] vfio: platform: Add generic DT reset support Geert Uytterhoeven @ 2018-04-11 9:21 ` Geert Uytterhoeven 3 siblings, 0 replies; 20+ messages in thread From: Geert Uytterhoeven @ 2018-04-11 9:21 UTC (permalink / raw) To: Geert Uytterhoeven Cc: Baptiste Reynal, Alex Williamson, Philipp Zabel, Rob Herring, Mark Rutland, KVM list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Linux-Renesas, Linux Kernel Mailing List On Wed, Apr 11, 2018 at 11:15 AM, Geert Uytterhoeven <geert+renesas@glider.be> wrote: > If a device is part of a PM Domain (e.g. power and/or clock domain), its > power state is managed using Runtime PM. Without Runtime PM, the device > may not be powered up, causing subtle failures, crashes, or system > lock-ups when the device is accessed by the guest. > > Fix this by adding Runtime PM support, powering the device when the VFIO > device is opened by the guest. > > Note that while more fine-grained power management could be implemented > on the guest side, if exported, this would be inherently unsafe, as > abusing it may kill the whole system. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Please ignore, this was sent accidentally with another series. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2018-05-11 19:45 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-11 9:15 [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 0/2] vfio: platform: Improve reset support Geert Uytterhoeven 2018-04-11 9:15 ` [PATCH v3 1/2] vfio: platform: Fix reset module leak in error path Geert Uytterhoeven 2018-04-13 8:55 ` Auger Eric 2018-05-11 19:45 ` Alex Williamson 2018-04-11 9:15 ` [PATCH v3 2/2] vfio: platform: Add generic DT reset support Geert Uytterhoeven 2018-04-12 7:00 ` Simon Horman 2018-04-12 10:31 ` Auger Eric 2018-04-12 11:32 ` Geert Uytterhoeven 2018-04-12 11:49 ` Auger Eric 2018-04-12 12:36 ` Sinan Kaya 2018-04-12 13:12 ` Geert Uytterhoeven 2018-04-12 14:10 ` Philipp Zabel 2018-04-12 16:02 ` Geert Uytterhoeven 2018-04-13 8:52 ` Auger Eric 2018-04-13 9:02 ` Geert Uytterhoeven 2018-04-13 9:22 ` Philipp Zabel 2018-04-13 10:05 ` Auger Eric 2018-04-13 11:56 ` Geert Uytterhoeven 2018-04-11 9:21 ` [PATCH] vfio: platform: Fix using devices in PM Domains Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.