| Message ID | 20180917163955.19023-3-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio: platform: Add generic reset controller support | expand |
Hi Geert, On 9/17/18 6:39 PM, Geert Uytterhoeven wrote: > Vfio-platform requires dedicated reset support, provided either by ACPI, > or, on DT platforms, by a device-specific reset driver matching against > the device's compatible value. > > On many SoCs, devices are connected to an SoC-internal reset controller. > If the reset hierarchy is described in DT using "resets" properties, or > in lookup tables in platform code, 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> > --- > v4: > - Add Reviewed-by, > - Use new RFC reset_control_get_dedicated() instead of > of_reset_control_get_exclusive(), to (a) make it more generic, and > (b) make sure the reset returned is really a dedicated reset, and > does not affect other devices, > > v3: > - Add Reviewed-by, > - Merge similar checks in vfio_platform_has_reset(), > > v2: > - Don't store error values in vdev->reset_control, > - Use of_reset_control_get_exclusive() instead of > __of_reset_control_get(), > - Improve description. > > --- > drivers/vfio/platform/vfio_platform_common.c | 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 c0cd824be2b767be..eb77fe87f3663e3e 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -18,6 +18,7 @@ > #include <linux/module.h> > #include <linux/mutex.h> > #include <linux/pm_runtime.h> > +#include <linux/reset.h> > #include <linux/slab.h> > #include <linux/types.h> > #include <linux/uaccess.h> > @@ -113,11 +114,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; > > @@ -128,8 +131,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 = reset_control_get_dedicated(vdev->device, NULL); > + if (!IS_ERR(rstc)) { > + vdev->reset_control = rstc; > + return 0; > + } > > - return vdev->of_reset ? 0 : -ENOENT; > + return PTR_ERR(rstc); This changes the returned value as seen by the user (probe returned valud). Can we keep -ENOENT in case of no reset controller found? Otherwise looks good to me with the new "dedicated" reset semantics. Thanks Eric > } > > static void vfio_platform_put_reset(struct vfio_platform_device *vdev) > @@ -139,6 +150,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) > @@ -218,6 +231,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; > > /* >
Hi Eric, On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote: > On 9/17/18 6:39 PM, Geert Uytterhoeven wrote: > > Vfio-platform requires dedicated reset support, provided either by ACPI, > > or, on DT platforms, by a device-specific reset driver matching against > > the device's compatible value. > > > > On many SoCs, devices are connected to an SoC-internal reset controller. > > If the reset hierarchy is described in DT using "resets" properties, or > > in lookup tables in platform code, 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> > > --- a/drivers/vfio/platform/vfio_platform_common.c > > +++ b/drivers/vfio/platform/vfio_platform_common.c > > @@ -128,8 +131,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 = reset_control_get_dedicated(vdev->device, NULL); > > + if (!IS_ERR(rstc)) { > > + vdev->reset_control = rstc; > > + return 0; > > + } > > > > - return vdev->of_reset ? 0 : -ENOENT; > > + return PTR_ERR(rstc); > This changes the returned value as seen by the user (probe returned > valud). Can we keep -ENOENT in case of no reset controller found? On success, it still returns 0. On failure, it forwards the error from reset_control_get_dedicated(), which is IMHO better than replacing it by -ENOENT: we try to propagate error codes as much as possible. It could e.g. return -EPROBE_DEFER. Is there anything that relies on the function returning -ENOENT? > Otherwise looks good to me with the new "dedicated" reset semantics. Thanks! Gr{oetje,eeting}s, Geert
Hi Geert, On 9/19/18 2:54 PM, Geert Uytterhoeven wrote: > Hi Eric, > > On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote: >> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote: >>> Vfio-platform requires dedicated reset support, provided either by ACPI, >>> or, on DT platforms, by a device-specific reset driver matching against >>> the device's compatible value. >>> >>> On many SoCs, devices are connected to an SoC-internal reset controller. >>> If the reset hierarchy is described in DT using "resets" properties, or >>> in lookup tables in platform code, 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> > >>> --- a/drivers/vfio/platform/vfio_platform_common.c >>> +++ b/drivers/vfio/platform/vfio_platform_common.c > >>> @@ -128,8 +131,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 = reset_control_get_dedicated(vdev->device, NULL); >>> + if (!IS_ERR(rstc)) { >>> + vdev->reset_control = rstc; >>> + return 0; >>> + } >>> >>> - return vdev->of_reset ? 0 : -ENOENT; >>> + return PTR_ERR(rstc); >> This changes the returned value as seen by the user (probe returned >> valud). Can we keep -ENOENT in case of no reset controller found? > > On success, it still returns 0. > On failure, it forwards the error from reset_control_get_dedicated(), which > is IMHO better than replacing it by -ENOENT: we try to propagate error > codes as much as possible. It could e.g. return -EPROBE_DEFER. > > Is there anything that relies on the function returning -ENOENT? None I am aware of actually. I was afraid about compatibility break but here we would change an errno by another one so maybe that's not a big deal at that stage of vfio_platform usage? Thanks Eric > >> Otherwise looks good to me with the new "dedicated" reset semantics. > > Thanks! > > Gr{oetje,eeting}s, > > Geert >
On Wed, 19 Sep 2018 17:31:43 +0200 Auger Eric <eric.auger@redhat.com> wrote: > Hi Geert, > > On 9/19/18 2:54 PM, Geert Uytterhoeven wrote: > > Hi Eric, > > > > On Wed, Sep 19, 2018 at 2:36 PM Auger Eric <eric.auger@redhat.com> wrote: > >> On 9/17/18 6:39 PM, Geert Uytterhoeven wrote: > >>> Vfio-platform requires dedicated reset support, provided either by ACPI, > >>> or, on DT platforms, by a device-specific reset driver matching against > >>> the device's compatible value. > >>> > >>> On many SoCs, devices are connected to an SoC-internal reset controller. > >>> If the reset hierarchy is described in DT using "resets" properties, or > >>> in lookup tables in platform code, 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> > > > >>> --- a/drivers/vfio/platform/vfio_platform_common.c > >>> +++ b/drivers/vfio/platform/vfio_platform_common.c > > > >>> @@ -128,8 +131,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 = reset_control_get_dedicated(vdev->device, NULL); > >>> + if (!IS_ERR(rstc)) { > >>> + vdev->reset_control = rstc; > >>> + return 0; > >>> + } > >>> > >>> - return vdev->of_reset ? 0 : -ENOENT; > >>> + return PTR_ERR(rstc); > >> This changes the returned value as seen by the user (probe returned > >> valud). Can we keep -ENOENT in case of no reset controller found? > > > > On success, it still returns 0. > > On failure, it forwards the error from reset_control_get_dedicated(), which > > is IMHO better than replacing it by -ENOENT: we try to propagate error > > codes as much as possible. It could e.g. return -EPROBE_DEFER. > > > > Is there anything that relies on the function returning -ENOENT? > None I am aware of actually. I was afraid about compatibility break but > here we would change an errno by another one so maybe that's not a big > deal at that stage of vfio_platform usage? Yeah, I don't see that one errno vs another really matters in the grand scheme of things. I also don't see that propagating this particular errno adds much value, but it is good general practice, so seems ok to me unless there are other concerns. Thanks, Alex
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index c0cd824be2b767be..eb77fe87f3663e3e 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -18,6 +18,7 @@ #include <linux/module.h> #include <linux/mutex.h> #include <linux/pm_runtime.h> +#include <linux/reset.h> #include <linux/slab.h> #include <linux/types.h> #include <linux/uaccess.h> @@ -113,11 +114,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; @@ -128,8 +131,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 = reset_control_get_dedicated(vdev->device, 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) @@ -139,6 +150,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) @@ -218,6 +231,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; /*