Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Philipp,

On Fri, Apr 13, 2018 at 11:22 AM, Philipp Zabel  wrote:
> On Thu, 2018-04-12 at 18:02 +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  
>> wrote:
>> > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>> > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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 
>> > > > > >  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 
>> > > > > > > > Reviewed-by: Philipp Zabel 
>> > > > > > > > --- 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,
>> > > > > > > >   
>> > > > > > > > >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

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Philipp,

On Fri, Apr 13, 2018 at 11:22 AM, Philipp Zabel  wrote:
> On Thu, 2018-04-12 at 18:02 +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  
>> wrote:
>> > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>> > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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 
>> > > > > >  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 
>> > > > > > > > Reviewed-by: Philipp Zabel 
>> > > > > > > > --- 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,
>> > > > > > > >   
>> > > > > > > > >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; 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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 = <_reset_control>;
>   };
> 
>   pwm@1 {
>   resets = <_reset_control>;
>   };
> 
> ->
> 
>   pwms {
>   resets = <_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
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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 = <_reset_control>;
>   };
> 
>   pwm@1 {
>   resets = <_reset_control>;
>   };
> 
> ->
> 
>   pwms {
>   resets = <_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
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

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  wrote:
> > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
> > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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 
> > > > > >  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 
> > > > > > > > Reviewed-by: Philipp Zabel 
> > > > > > > > --- 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,
> > > > > > > >   
> > > > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

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  wrote:
> > On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
> > > On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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 
> > > > > >  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 
> > > > > > > > Reviewed-by: Philipp Zabel 
> > > > > > > > --- 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,
> > > > > > > >   
> > > > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Fri, Apr 13, 2018 at 10:52 AM, Auger Eric  wrote:
> On 12/04/18 18:02, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  
>> wrote:
>>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
 On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>>> 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 
> Reviewed-by: Philipp Zabel 
> --- 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,
>   
> >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Fri, Apr 13, 2018 at 10:52 AM, Auger Eric  wrote:
> On 12/04/18 18:02, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  
>> wrote:
>>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
 On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>>> 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 
> Reviewed-by: Philipp Zabel 
> --- 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,
>   
> >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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  wrote:
>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>> 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 
 Reviewed-by: Philipp Zabel 
 --- 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,
   
 >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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  wrote:
>> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>> 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 
 Reviewed-by: Philipp Zabel 
 --- 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,
   
 >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Philipp,

On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  wrote:
> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>> > > > 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 
>> > > > > > Reviewed-by: Philipp Zabel 
>> > > > > > --- 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,
>> > > > > >   
>> > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Philipp,

On Thu, Apr 12, 2018 at 4:10 PM, Philipp Zabel  wrote:
> On Thu, 2018-04-12 at 15:12 +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  
>> > > > 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 
>> > > > > > Reviewed-by: Philipp Zabel 
>> > > > > > --- 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,
>> > > > > >   
>> > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

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  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  
> > > > 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 
> > > > > > Reviewed-by: Philipp Zabel 
> > > > > > --- 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,
> > > > > >   
> > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Philipp Zabel]

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  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  
> > > > 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 
> > > > > > Reviewed-by: Philipp Zabel 
> > > > > > --- 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,
> > > > > >   
> > > > > > >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 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Sinan,

On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  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 
> Reviewed-by: Philipp Zabel 
>>>
> --- 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,
>   
> >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 = < ASPEED_RESET_I2C>;
arch/arm/boot/dts/aspeed-g5.dtsi: 14 resets = < ASPEED_RESET_I2C>;
arch/arm/boot/dts/atlas7.dtsi:  2 resets = < 29>;
arch/arm/boot/dts/gemini.dtsi:  2 resets = < GEMINI_RESET_IDE>;
arch/arm/boot/dts/meson8.dtsi:  2 resets = < RESET_USB_OTG>;
arch/arm/boot/dts/meson8b.dtsi:  2 resets = < RESET_USB_OTG>;
arch/arm/boot/dts/r8a7743.dtsi:  7 resets = < 523>;
arch/arm/boot/dts/r8a7743.dtsi:  2 resets = < 703>;
arch/arm/boot/dts/r8a7743.dtsi:  2 resets = < 704>;
arch/arm/boot/dts/r8a7745.dtsi:  7 resets = < 523>;
arch/arm/boot/dts/r8a7745.dtsi:  2 resets = < 703>;
arch/arm/boot/dts/r8a7745.dtsi:  2 resets = < 704>;
arch/arm/boot/dts/r8a7790.dtsi:  

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Sinan,

On Thu, Apr 12, 2018 at 2:36 PM, Sinan Kaya  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  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 
> Reviewed-by: Philipp Zabel 
>>>
> --- 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,
>   
> >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 = < ASPEED_RESET_I2C>;
arch/arm/boot/dts/aspeed-g5.dtsi: 14 resets = < ASPEED_RESET_I2C>;
arch/arm/boot/dts/atlas7.dtsi:  2 resets = < 29>;
arch/arm/boot/dts/gemini.dtsi:  2 resets = < GEMINI_RESET_IDE>;
arch/arm/boot/dts/meson8.dtsi:  2 resets = < RESET_USB_OTG>;
arch/arm/boot/dts/meson8b.dtsi:  2 resets = < RESET_USB_OTG>;
arch/arm/boot/dts/r8a7743.dtsi:  7 resets = < 523>;
arch/arm/boot/dts/r8a7743.dtsi:  2 resets = < 703>;
arch/arm/boot/dts/r8a7743.dtsi:  2 resets = < 704>;
arch/arm/boot/dts/r8a7745.dtsi:  7 resets = < 523>;
arch/arm/boot/dts/r8a7745.dtsi:  2 resets = < 703>;
arch/arm/boot/dts/r8a7745.dtsi:  2 resets = < 704>;
arch/arm/boot/dts/r8a7790.dtsi:  3 resets = < 703>;
arch/arm/boot/dts/r8a7790.dtsi:  2 resets = < 704>;

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Sinan Kaya]

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  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 
 Reviewed-by: Philipp Zabel 
>>
 --- 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,
   >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.

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Sinan Kaya]

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  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 
 Reviewed-by: Philipp Zabel 
>>
 --- 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,
   >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.

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

Hi Geert,
On 12/04/18 13:32, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric  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 
>>> Reviewed-by: Philipp Zabel 
> 
>>> --- 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,
>>>   >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
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

Hi Geert,
On 12/04/18 13:32, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric  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 
>>> Reviewed-by: Philipp Zabel 
> 
>>> --- 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,
>>>   >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
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric  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 
>> Reviewed-by: Philipp Zabel 

>> --- 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,
>>   >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 -- ge...@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

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Geert Uytterhoeven]

Hi Eric,

On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric  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 
>> Reviewed-by: Philipp Zabel 

>> --- 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,
>>   >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 -- ge...@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

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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 
> Reviewed-by: Philipp Zabel 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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,
>   >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;
>  
>   /*
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Auger Eric]

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 
> Reviewed-by: Philipp Zabel 
> ---
> 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 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -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,
>   >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;
>  
>   /*
> 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Simon Horman]

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 
> Reviewed-by: Philipp Zabel 

Reviewed-by: Simon Horman 

Re: [PATCH v3 2/2] vfio: platform: Add generic DT reset support

[Simon Horman]

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 
> Reviewed-by: Philipp Zabel 

Reviewed-by: Simon Horman