Re: [PATCH v2 1/8] dt-bindings: pinctrl: mvebu: Document bindings for AC5

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 16/03/2022 21:21, Chris Packham wrote:
> 
> On 16/03/22 21:16, Krzysztof Kozlowski wrote:
>> On 15/03/2022 22:12, Chris Packham wrote:
>>> (trimmed cc list to the arm, pinctrl and dt people)
>>>
>>> On 15/03/22 23:46, Krzysztof Kozlowski wrote:
>>>> On 14/03/2022 22:31, Chris Packham wrote:
>>>>> Add JSON schema for marvell,ac5-pinctrl present on the Marvell 98DX2530
>>>>> SoC.
>>>>>
>>>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>       Changes in v2:
>>>>>       - Remove syscon and simple-mfd compatibles
>>>>>
>>>>>    .../bindings/pinctrl/marvell,ac5-pinctrl.yaml | 70 +++++++++++++++++++
>>>>>    1 file changed, 70 insertions(+)
>>>>>    create mode 100644 Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..65af1d5f5fe0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/pinctrl/marvell,ac5-pinctrl.yaml
>>>>> @@ -0,0 +1,70 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6CXu-YC1w&u=http%3a%2f%2fdevicetree%2eorg%2fschemas%2fpinctrl%2fmarvell%2cac5-pinctrl%2eyaml%23
>>>>> +$schema: http://scanmail.trustwave.com/?c=20988&d=1pyx4kv4KTrTfE5fXNs54mLZmOgk87Uim6TAvbEE2Q&u=http%3a%2f%2fdevicetree%2eorg%2fmeta-schemas%2fcore%2eyaml%23
>>>>> +
>>>>> +title: Marvell AC5 pin controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Chris Packham <chris.packham@alliedtelesis.co.nz>
>>>>> +
>>>>> +description:
>>>>> +  Bindings for Marvell's AC5 memory-mapped pin controller.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: marvell,ac5-pinctrl
>>>>> +
>>>>> +patternProperties:
>>>>> +  '-pins$':
>>>>> +    type: object
>>>>> +    $ref: pinmux-node.yaml#
>>>>> +
>>>>> +    properties:
>>>>> +      marvell,function:
>>>>> +        $ref: "/schemas/types.yaml#/definitions/string"
>>>>> +        description:
>>>>> +          Indicates the function to select.
>>>>> +        enum: [ gpio, i2c0, i2c1, nand, sdio, spi0, spi1, uart0, uart1, uart2, uart3 ]
>>>>> +
>>>>> +      marvell,pins:
>>>>> +        $ref: /schemas/types.yaml#/definitions/string-array
>>>>> +        description:
>>>>> +          Array of MPP pins to be used for the given function.
>>>>> +        minItems: 1
>>>>> +        items:
>>>>> +          enum: [ mpp0, mpp1, mpp2, mpp3, mpp4, mpp5, mpp6, mpp7, mpp8, mpp9,
>>>>> +                  mpp10, mpp11, mpp12, mpp13, mpp14, mpp15, mpp16, mpp17, mpp18, mpp19,
>>>>> +                  mpp20, mpp21, mpp22, mpp23, mpp24, mpp25, mpp26, mpp27, mpp28, mpp29,
>>>>> +                  mpp30, mpp31, mpp32, mpp33, mpp34, mpp35, mpp36, mpp37, mpp38, mpp39,
>>>>> +                  mpp40, mpp41, mpp42, mpp43, mpp44, mpp45 ]
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: "pinctrl.yaml#"
>>>>> +
>>>>> +required:
>>>>> +  - compatible
>>>>> +
>>>>> +additionalProperties: false
>>>>> +
>>>>> +examples:
>>>>> +  - |
>>>>> +    system-controller@80020100 {
>>>>> +      compatible = "syscon", "simple-mfd";
>>>>> +      reg = <0x80020000 0x20>;
>>>> This is unusual. Usually the pinctrl should be a device @80020100, not
>>>> child of syscon node. Why do you need it? In v1 you mentioned that
>>>> vendor sources do like this, but it's not correct to copy wrong DTS. :)
>>> The vendor dts has this
>>>
>>>           pinctrl0: pinctrl@80020100 {
>>>               compatible = "marvell,ac5-pinctrl",
>>>                        "syscon", "simple-mfd";
>>>               reg = <0 0x80020100 0 0x20>;
>>>               i2c_mpps: i2c-mpps {
>>>                   marvell,pins = "mpp26", "mpp27";
>>>                   marvell,function = "i2c0-opt";
>>>               };
>>>        };
>>>
>>> Rob pointed out that "syscon", "simple-mfd" don't belong. I went looking
>>> and found marvell,armada-7k-pinctrl which has the pinctrl as a child of
>>> a syscon node and what you see in v2 is the result.
>>>
>>> I probably went a bit too far off the deep end and should have just
>>> dropped the "syscon", "simple-mfd" compatibles. I even wrote that
>>> version but decided to add some gold plating before I submitted it.
>> More or less it is explained in
>> Documentation/devicetree/bindings/arm/marvell/cp110-system-controller.txt why
>> armada-7k uses it that way. The pinctrl is part of system registers
>> which apparently has to be shared with others (on shared SFR range).
>>
>> It depends on your case, your SFR ranges for pinctrl and other blocks.
>>
> I can tell you that without a syscon node in the mix somewhere the 
> driver will fail to load. And when I switch to 
> mvebu_pinctrl_simple_mmio_probe() the driver loads but then kernel 
> panics when something tries to use one of the pin functions.
> 
> So I think the syscon is needed. I just need to come up with a better 
> justification than "because it's needed".

What do you mean "driver fails to load"? You control the driver, don't
you? You wrote it? If you write a driver which is not compatible with
bindings, it won't work obviously, so after changing bindings you need
to revisit the driver.

There is no need for syscon because driver "fails to load". You need to
fix your driver. Currently the driver code is definitely not a proper
platform driver.

Different question is whether something else requires here syscon
because it accesses these registers but this requires knowledge of
architecture and other components.


Best regards,
Krzysztof