Re: [PATCH v4 4/4] dt-bindings: syscon: Add StarFive syscon doc

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 28/02/2023 15:59, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>> On 28/02/2023 12:02, Emil Renner Berthing wrote:
>>> On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 28/02/2023 10:05, William Qiu wrote:
>>>>>
>>>>>
>>>>> On 2023/2/28 6:29, Rob Herring wrote:
>>>>>> On Tue, Feb 21, 2023 at 10:44:02AM +0800, William Qiu wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 2023/2/21 7:43, Rob Herring wrote:
>>>>>>>> On Wed, Feb 15, 2023 at 07:32:49PM +0800, William Qiu wrote:
>>>>>>>>> Add documentation to describe StarFive System Controller Registers.
>>>>>>>>>
>>>>>>>>> Signed-off-by: William Qiu <william.qiu@starfivetech.com>
>>>>>>>>> ---
>>>>>>>>>  .../bindings/soc/starfive/jh7110-syscon.yaml  | 51 +++++++++++++++++++
>>>>>>>>>  MAINTAINERS                                   |  5 ++
>>>>>>>>>  2 files changed, 56 insertions(+)
>>>>>>>>>  create mode 100644 Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>>
>>>>>>>>> diff --git a/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>> new file mode 100644
>>>>>>>>> index 000000000000..fa4d8522a454
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/Documentation/devicetree/bindings/soc/starfive/jh7110-syscon.yaml
>>>>>>>>> @@ -0,0 +1,51 @@
>>>>>>>>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>>>>>>>>> +%YAML 1.2
>>>>>>>>> +---
>>>>>>>>> +$id: http://devicetree.org/schemas/soc/starfive/jh7110-syscon.yaml#
>>>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>>>> +
>>>>>>>>> +title: StarFive JH7110 SoC system controller
>>>>>>>>> +
>>>>>>>>> +maintainers:
>>>>>>>>> +  - William Qiu <william.qiu@starfivetech.com>
>>>>>>>>> +
>>>>>>>>> +description: |
>>>>>>>>> +  The StarFive JH7110 SoC system controller provides register information such
>>>>>>>>> +  as offset, mask and shift to configure related modules such as MMC and PCIe.
>>>>>>>>> +
>>>>>>>>> +properties:
>>>>>>>>> +  compatible:
>>>>>>>>> +    items:
>>>>>>>>> +      - enum:
>>>>>>>>> +          - starfive,jh7110-stg-syscon
>>>>>>>>> +          - starfive,jh7110-sys-syscon
>>>>>>>>> +          - starfive,jh7110-aon-syscon
>>>>>>>>
>>>>>>>> Is 'syscon' really part of what the blocks are called? Is just 'stg',
>>>>>>>> 'sys' and 'aon' not unique enough?
>>>>>>>>
>>>>>>>> Rob
>>>>>>> Hi Rob,
>>>>>>>
>>>>>>> In StarFive SoC, we do have syscrg/aoncrg/stgcrg, which is uesd to be the clock
>>>>>>> controller, so 'syscon' is added to avoid confusion.
>>>>>>
>>>>>> You've only added to my confusion. 'syscrg' and 'sys-syscon' are 2
>>>>>> different h/w blocks and unrelated to each other? Or 'syscrg' is the
>>>>>> clock portion of 'sys-syscon'? In that case, 'syscrg' should be a child
>>>>>> of 'sys-syscon' or possibly just all one node. Please provide details on
>>>>>> the entire h/w block so we can provide better input on the bindings.
>>>>>>
>>>>>> Rob
>>>>>
>>>>> Hi Rob,
>>>>>
>>>>> It's my description that's problematic.'syscon' here refers to the hardware module
>>>>> inside our JH7110, which is different from the syscon interface in linux. The syscon
>>>>> I added now uses the syscon interface of linux to read and write the syscon register
>>>>> in our JH7110. So we decided to name it that way.
>>>>
>>>> You didn't really answer Rob's questions.
>>>>
>>>> Also, syscon is Linux term, so are you sure hardware module is called
>>>> like this? Hardware engineers took pure Linux name and used it?
>>>
>>> Yes, from the documentation I could find[1] there are CRG blocks
>>> (Clock and Reset Generator) and SYSCON blocks:
>>> SYS CRG
>>> STG CRG
>>> AON CRG
>>> SYS SYSCON
>>> STG SYSCON
>>> AON SYSCON
>>>
>>> The CRG blocks contain registers to control clocks and resets that
>>> follow a pattern used by the clock and reset drivers. The SYSCON
>>> blocks just seem to contain registers to control whatever didn't fit
>>> in any other blocks, but might be vaguely related to the peripherals
>>> that run off clocks controlled by the corresponding CRG block.
>>
>> The memory map [1] suggests these are indeed separate address spaces,
>> e.g. AON CRG, AON SYSCON and AON GPIO, but now I would argue that this
>> might be still one device - AON (or STG, SYS). Just like PCIE0 has four
>> address spaces, it does not mean you have four separate PCIE0 devices.
>> You have only one PCIE0, just like you have only one AON, one STG and
>> one SYS (System).
> 
> I see what you mean, but if you look into what the registers in the
> SYSCON blocks actually do it's not clear to me that they should be
> grouped with the clocks/resets any more than say the pinctrl/GPIO
> node. Maybe it's my fault for not giving you the full picture. Eg. for
> "system" and "always-on" there are blocks:
> 
> SYS CRG
> SYS SYSCON
> SYS IOMUX
> AON CRG
> AON SYSCON
> AON IOMUX
> 
> ..and it really don't see why eg. SYS CRG and SYS SYSCON should be
> thought of as one device, but not include SYS IOMUX then.

... include sys iomux as well, just like GPIO is included for AON.

> 
> As an examly the SYS SYSCON includes registers to control:
> - remapping of different peripherals from SD controller to video encoders
> - voltage select for certain GPIO pins
> - phy interface selection for ethernet and CAN
> - QuadSPI delay chain and SRAM configuration
> - PLL configuration
> - endian selection for the SD controller
> 
> To me this is pretty much exactly described by the syscon device tree binding:
> "System controller node represents a register region containing a set
> of miscellaneous registers. The registers are not cohesive enough to
> represent as any specific type of device. [..]"
> In any case it's clear that however the SYSCON blocks are represented
> in the device tree, a driver for it would need to export registers in
> the SYSCON block for other drivers to use.

You started entire sentence with "but" so you disagree but with what
exactly? The naming? But syscon is fine - hardware manual calls it like
that.

The point was that AON is one device (consisting of multiple blocks).

Best regards,
Krzysztof