On 28/02/2023 15:59, Emil Renner Berthing wrote: > On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski > <krzysztof.kozlowski@xxxxxxxxxx> wrote: >> On 28/02/2023 12:02, Emil Renner Berthing wrote: >>> On Tue, 28 Feb 2023 at 11:40, Krzysztof Kozlowski >>> <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> >>>>>>>>> --- >>>>>>>>> .../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@xxxxxxxxxxxxxxxx> >>>>>>>>> + >>>>>>>>> +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
