| Message ID | 1523375627-23746-3-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2018-04-10 at 17:53 +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> > --- > 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 | 26 ++++++++++++++++++++++++-- > drivers/vfio/platform/vfio_platform_private.h | 1 + > 2 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index b60bb5326668498c..3c13327b2777f8ec 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c [...] > @@ -127,8 +136,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 vdev->device->of_node == NULL, this will return -EINVAL ... > + if (!IS_ERR(rstc)) { > + vdev->reset_control = rstc; > + return 0; > + } > > - return vdev->of_reset ? 0 : -ENOENT; > + return PTR_ERR(rstc); ... instead of -ENOENT, if that makes any difference. regards Philipp
On Tue, Apr 10, 2018 at 05:53:47PM +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> > --- > 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 | 26 ++++++++++++++++++++++++-- > drivers/vfio/platform/vfio_platform_private.h | 1 + > 2 files changed, 25 insertions(+), 2 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index b60bb5326668498c..3c13327b2777f8ec 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,19 @@ 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; > + if (vdev->of_reset) > + return true; > + > + if (vdev->reset_control) > + return true; > + > + return false; I wonder if the above would be better expressed as: 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 +136,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 +155,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 +236,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"); Would it be useful to differentiate between the above two informational messages? > + 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 >
Hi Simon, On Wed, Apr 11, 2018 at 10:22 AM, Simon Horman <horms@verge.net.au> wrote: > On Tue, Apr 10, 2018 at 05:53:47PM +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> >> --- >> v2: >> - Don't store error values in vdev->reset_control, >> - Use of_reset_control_get_exclusive() instead of >> __of_reset_control_get(), >> - Improve description. >> --- 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,19 @@ 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; >> + if (vdev->of_reset) >> + return true; >> + >> + if (vdev->reset_control) >> + return true; >> + >> + return false; > > I wonder if the above would be better expressed as: > > return vdev->of_reset || vdev->reset_control; Makes sense, now both checks are of the same type. >> @@ -217,6 +236,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"); > > Would it be useful to differentiate between the above two informational > messages? Probably not, there's also no differentiation with the message for the ACPI case above (out of visible context). Gr{oetje,eeting}s, Geert
Hi Philipp, On Wed, Apr 11, 2018 at 10:22 AM, Philipp Zabel <p.zabel@pengutronix.de> wrote: > On Tue, 2018-04-10 at 17:53 +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> Thanks! >> --- a/drivers/vfio/platform/vfio_platform_common.c >> +++ b/drivers/vfio/platform/vfio_platform_common.c > [...] >> @@ -127,8 +136,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 vdev->device->of_node == NULL, this will return -EINVAL ... > >> + if (!IS_ERR(rstc)) { >> + vdev->reset_control = rstc; >> + return 0; >> + } >> >> - return vdev->of_reset ? 0 : -ENOENT; >> + return PTR_ERR(rstc); > > ... instead of -ENOENT, if that makes any difference. Not really. The single caller (vfio_platform_probe_common()) already returns -EINVAL if no IOMMU group is found, so this should be handled fine. Gr{oetje,eeting}s, Geert
Hi Geert, On Wed, Apr 11, 2018 at 10:39:19AM +0200, Geert Uytterhoeven wrote: > Hi Simon, > > On Wed, Apr 11, 2018 at 10:22 AM, Simon Horman <horms@verge.net.au> wrote: > > On Tue, Apr 10, 2018 at 05:53:47PM +0200, Geert Uytterhoeven wrote: ... > >> @@ -217,6 +236,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"); > > > > Would it be useful to differentiate between the above two informational > > messages? > > Probably not, there's also no differentiation with the message for the > ACPI case above (out of visible context). Thanks, I agree that it seems fine to leave things as you have them above.
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index b60bb5326668498c..3c13327b2777f8ec 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,19 @@ 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; + if (vdev->of_reset) + return true; + + if (vdev->reset_control) + return true; + + return false; } 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 +136,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 +155,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 +236,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; /*
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> --- 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 | 26 ++++++++++++++++++++++++-- drivers/vfio/platform/vfio_platform_private.h | 1 + 2 files changed, 25 insertions(+), 2 deletions(-)