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

[Conor Dooley]

On Tue, Feb 28, 2023 at 06:31:46PM +0100, Emil Renner Berthing wrote:
> On Tue, 28 Feb 2023 at 17:59, Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> > On 28/02/2023 15:59, Emil Renner Berthing wrote:
> > > On Tue, 28 Feb 2023 at 12:28, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@xxxxxxxxxx> wrote:

> > > 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.
> 
> This would at least take the view that the blocks named alike should
> be thought of as a single device to its logical conclusion.
> Unfortunately we're a bit late for that. The pinctrl/GPiO bindings and
> drivers are already merged:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d6e0a660097dcdb80e7c5c859eb12f776060b02e
> 
> > >
> > > 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).
> 
> Yes, and what I'm trying to explain is that I'm not convinced that's
> the right model. The CRG blocks and IOMUX blocks don't really have
> anything in common other than the name StarFive gave them. You can
> argue that the CRG and IOMUX blocks overlap with the corresponding
> SYSCON block, but so do a lot of other peripherals as you can see from
> the list above.
> 
> I think the IOMUX and SYSCON blocks are just named after the clock
> domain they're under, but a lot of other peripherals are also under
> the SYS and AON clock domains and we don't model them as one big
> device.

I went and bothered Rob/Krzysztof on IRC about this.
Not gonna speak for them, but I think they're now okay with keeping the
SYS_CRG (clock+reset block) separate from the SYS_SYSCON block ("random
collection of registers"). Possibly there was just confusion due to the
naming used here, thinking that "SYS", "STG" and "AON" were devices with
two register blocks, as opposed to being the name of a clock/power domain
on the SoC.

I'll leave it up to them to confirm that though!

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature