Re: [PATCH V3 2/3] dt-bindings: gpio: zynqmp: Add binding documentation for modepin

[Michal Simek <michal.simek@xxxxxxxxxx>]

On 8/18/21 11:55 AM, Ahmad Fatoum wrote:
> On 18.08.21 11:38, Michal Simek wrote:
>> Hi Ahmad,
>>
>> On 8/18/21 11:00 AM, Ahmad Fatoum wrote:
>>> On 18.08.21 10:10, Piyush Mehta wrote:
>>>> This patch adds DT binding document for zynqmp modepin GPIO controller.
>>>> Modepin GPIO controller has four GPIO pins which can be configurable
>>>> as input or output.
>>>>
>>>> Modepin driver is a bridge between the peripheral driver and GPIO pins.
>>>> It has set and get APIs for accessing GPIO pins, based on the device-tree
>>>> entry of reset-gpio property in the peripheral driver, every pin can be
>>>> configured as input/output and trigger GPIO pin.
>>>>
>>>> For more information please refer zynqMp TRM link:
>>>> Link: https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
>>>> Chapter 2: Signals, Interfaces, and Pins
>>>> Table 2-2: Clock, Reset, and Configuration Pins - PS_MODE
>>>>
>>>> Signed-off-by: Piyush Mehta <piyush.mehta@xxxxxxxxxx>
>>>> Acked-by: Michal Simek <michal.simek@xxxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>> - Addressed review comments: Update commit message
>>>>
>>>> Review Comments:
>>>> https://lore.kernel.org/linux-arm-kernel/20210615080553.2021061-2-piyush.mehta@xxxxxxxxxx/T/#mbd1fbda813e33b19397b350bde75747c92a0d7e1
>>>> https://lore.kernel.org/linux-arm-kernel/20210615080553.2021061-2-piyush.mehta@xxxxxxxxxx/T/#me82b1444ab3776162cdb0077dfc9256365c7e736
>>>>
>>>> Changes in v3:
>>>> - Addressed Rob and Michal review comments:
>>>>   - Update DT example. 
>>>>
>>>> Review Comments:
>>>> https://lore.kernel.org/linux-arm-kernel/YRbBnRS0VosXcZWz@xxxxxxxxxxxxxxxxxx/
>>>> https://lore.kernel.org/linux-arm-kernel/d71ad7f9-6972-8cc0-6dfb-b5306c9900d0@xxxxxxxxxx/
>>>> ---
>>>>  .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    | 41 ++++++++++++++++++++++
>>>>  .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    | 43 ++++++++++++++++++++++
>>>>  1 file changed, 43 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
>>>> new file mode 100644
>>>> index 0000000..1442815
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
>>>> @@ -0,0 +1,43 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: "http://devicetree.org/schemas/gpio/xlnx,zynqmp-gpio-modepin.yaml#";
>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#";
>>>> +
>>>> +title: ZynqMP Mode Pin GPIO controller
>>>> +
>>>> +description:
>>>> +  PS_MODE is 4-bits boot mode pins sampled on POR deassertion. Mode Pin
>>>> +  GPIO controller with configurable from numbers of pins (from 0 to 3 per
>>>> +  PS_MODE). Every pin can be configured as input/output.
>>> So, at Linux runtime, someone decides to boot the system into e.g. a USB
>>> recovery mode and then toggles the appropriate GPIOs and does a system
>>> reset?
>>>
>>> If so, are you aware of the reboot mode[1] infrastructure?
>>>
>>> A reboot-mode-gpio driver on top of this GPIO controller would allow you
>>> to describe the supported reboot modes in the device tree and instead of
>>> exporting GPIOs to userspace, users can then just do
>>>
>>> 	systemctl restart recovery
>>>
>>> to toggle the appropriate bits.
>>>
>>> Also to be sure: PS_MODE are actual GPIO pins that you could toggle
>>> board level components with, right? i.e. it's not just a register that
>>> overrides the values read from the boot mode pins? (In the latter case
>>> a syscon-reboot-mode without GPIO controller would be the correct
>>> abstraction).
>>>
>>> [1]: drivers/power/reset/reboot-mode.c
>>
>> Thanks for these links. I wasn't aware about it.
>> But this device/IP is not working like this. Changing gpios to certain
>> state won't ensure that on reboot/reset (done in whatever way) won't
>> stay on values you chose.
> 
> Ah, the "PS_MODE is 4-bits boot mode pins sampled on POR deassertion" part
> misled me. These pins are sampled on startup, but can afterwards be reused 
> via talking to firmware. Thanks for clearing this up.

yes

>> modepin gpio driver is at BOOT_PIN_CTRL 	0xFF5E0250
>>
>> (To be fair if you add additional external chip it could work like this
>> but I have never seen it).
> 
> Ye, that would've been strange, that's why I asked. :)

No issue at all.

> 
>> But when you bring this up. Xilinx ZynqMP is providing a way how to
>> setup alternative boot mode which is done via
>> BOOT_MODE_USER 	0xFF5E0200
>> Bit 8 and 15-12.
>> Then you can setup any bootmode.
>>
>> ZynqMP supports couple of modes listed here
>> https://source.denx.de/u-boot/u-boot/-/blob/master/arch/arm/mach-zynqmp/include/mach/hardware.h#L73
>>
>> but again routing to this register needs to be done via firmware
>> interface but it should be done via separate driver.
> 
> Yes.
> 
>> Is there an option to setup whatever modes you like?
>>
>> I mean to simply cover all modes like this?
>>
>> mode-jtag = <0>;
>> mode-sd = <3>;
>> mode-sd1 = <5>;
> 
> Yes, you can define the supported modes in the SoC dtsi
> and boards inherit that and can extend it as necessary.

ok.

> 
>> And then users/customers can say what normal/recovery/test modes are.
> 
> Yes, that would be nice. But after your clarification, I see that it's
> unrelated to this patch series. Binding is fine. Question on driver
> is still applicable.

I remember any discussion about it between Piyush and Linus and I will
let Piyush to handle it.

Thanks,
Michal