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

[Sinan Kaya <okaya@codeaurora.org>]

On 4/12/2018 7:49 AM, Auger Eric wrote:
> Hi Geert,
> On 12/04/18 13:32, Geert Uytterhoeven wrote:
>> Hi Eric,
>>
>> On Thu, Apr 12, 2018 at 12:31 PM, Auger Eric <eric.auger@redhat.com> wrote:
>>> On 11/04/18 11:15, Geert Uytterhoeven wrote:
>>>> Vfio-platform requires reset support, provided either by ACPI, or, on DT
>>>> platforms, by a device-specific reset driver matching against the
>>>> device's compatible value.
>>>>
>>>> On many SoCs, devices are connected to an SoC-internal reset controller.
>>>> If the reset hierarchy is described in DT using "resets" properties,
>>>> such devices can be reset in a generic way through the reset controller
>>>> subsystem.  Hence add support for this, avoiding the need to write
>>>> device-specific reset drivers for each single device on affected SoCs.
>>>>
>>>> Devices that do require a more complex reset procedure can still provide
>>>> a device-specific reset driver, as that takes precedence.
>>>>
>>>> Note that this functionality depends on CONFIG_RESET_CONTROLLER=y, and
>>>> becomes a no-op (as in: "No reset function found for device") if reset
>>>> controller support is disabled.
>>>>
>>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>>> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
>>
>>>> --- a/drivers/vfio/platform/vfio_platform_common.c
>>>> +++ b/drivers/vfio/platform/vfio_platform_common.c
>>
>>>> @@ -127,8 +130,16 @@ static int vfio_platform_get_reset(struct vfio_platform_device *vdev)
>>>>               vdev->of_reset = vfio_platform_lookup_reset(vdev->compat,
>>>>                                                       &vdev->reset_module);
>>>>       }
>>>> +     if (vdev->of_reset)
>>>> +             return 0;
>>>> +
>>>> +     rstc = of_reset_control_get_exclusive(vdev->device->of_node, NULL);
>>>
>>> Shouldn't we prefer the top level reset_control_get_exclusive()?
>>
>> I guess that should work, too.
>>
>>> To make sure about the exclusive/shared terminology, does
>>> get_reset_control_get_exclusive() check we have an exclusive wire
>>> between this device and the reset controller?
>>
>> AFAIU, the "exclusive" means that only a single user can obtain access to
>> the reset, and it does not guarantee that we have an exclusive wire between
>> the device and the reset controller.
>>
>> The latter depends on the SoC's reset topology. If a reset wire is shared
>> by multiple devices (e.g. resets shared by PWM or Display Unit devices on
>> R-Car SoCs), exporting a subset of these devices to a guest is a bad idea,
>> indeed.
> 
> So who's going to check this assigned device will not trigger a reset of
> other non assigned devices sharing the same reset controller?

I like the direction in general. I was hoping that you'd call it of_reset_control
rather than reset_control.

Is there anything in the OF spec about what to expect from DT's reset?

> 
>>
>> I guess the same thing can happen with the ACPI "_RST" method?
> 
> ACPI spec _RST chapter says about _RST object:
> 
> "This object executes a reset on the associated device
>  or devices. If included in a device context, the
> reset must not affect any other ACPI-described de
> vices; if included in a power resource for reset
> (_PRR, Section 7.3.26) the reset must affect all ACPI-described devices
> that reference it. When this object is described in
> a device context, it executes a function level reset that only affects
> the device it is associated with; neither parent nor children should be
> affected by the execution of this reset. Executing this must only result
> in this device resetting without the device appearing as if it
> has been removed from the bus altogether, to prevent OSPM re-enumeration
> of devices on hot-pluggable buses (e.g. USB)."

ACPI spec is clear. We are doing a device specific reset aka function level
reset here. It does not impact other devices in the system. 

In fact, ACPI does not have a clock controller concept. All clock/reset details
are hidden from the OS.

> 
> Adding Sinan in copy for clarification.
> 
> Thanks
> 
> Eric
>>
>> Gr{oetje,eeting}s,
>>
>>                         Geert
>>
> 


-- 
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.