On Mon, Aug 20, 2018 at 10:01 PM, Emmanuel Vadot <manu@xxxxxxxxxxxxxxxx> wrote:
> On Mon, 20 Aug 2018 16:41:09 +0800
> Chen-Yu Tsai <wens@xxxxxxxx> wrote:
>
>> On Mon, Aug 20, 2018 at 3:42 PM, Emmanuel Vadot <manu@xxxxxxxxxxxxxxxx> wrote:
>> >
>> > Hello Chen-Yu,
>> >
>> > On Thu, 14 Jun 2018 23:35:44 +0800
>> > Chen-Yu Tsai <wens@xxxxxxxx> wrote:
>> >
>> >> Hi,
>> >>
>> >> This series is the remaining A64 syscon changes from the R40 DWMAC
>> >> series. The series aligns how the A64 system control exports a regmap
>> >> for the sun8i DWMAC driver to access with what we've done for the R40.
>> >>
>> >> Originally the A64 used the generic syscon for this bit of hardware.
>> >> But this block also contains mapping bits for the onboard SRAM, used
>> >> by various peripherals, and other vendor specific bits we may use in
>> >> the future. It is by no means generic. And we already have a device
>> >> tree binding and driver for the SRAM part.
>> >>
>> >> The first patch make the SRAM control device export a regmap, exposing
>> >> a single EMAC control register, for the DWMAC driver to consume.
>> >>
>> >> The second and third patches rename the A64 compatible string to read
>> >> "system control", which is what the block is named in the user manual.
>> >>
>> >> The last patch fixes up the device node, and also adds the lone mappable
>> >> SRAM block, which is needed by the Display Engine.
>> >>
>> >> Changes since v2:
>> >>
>> >> - changed the compatible string from "*-sram-controller" to
>> >> "*-system-control"
>> >>
>> >>
>> >> ChenYu
>> >>
>> >> Chen-Yu Tsai (2):
>> >> dt-bindings: sram: Rename A64 SRAM controller compatible
>> >> soc: sunxi: sram: Add updated compatible string for A64 system control
>> >>
>> >> Icenowy Zheng (2):
>> >> soc: sunxi: export a regmap for EMAC clock reg on A64
>> >> arm64: dts: allwinner: a64: add SRAM controller device tree node
>> >>
>> >> .../devicetree/bindings/sram/sunxi-sram.txt | 3 +-
>> >> arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 19 +++++-
>> >> drivers/soc/sunxi/sunxi_sram.c | 61 ++++++++++++++++++-
>> >> 3 files changed, 78 insertions(+), 5 deletions(-)
>> >>
>> >> --
>> >> 2.17.1
>> >
>> > I wish to have seen this serie before as it have some inconsistencies.
>> >
>> > In patch 2 you renamed allwinner,sun50i-a64-sram-controller to
>> > allwinner,sun50i-a64-system-control but the former was never used in
>> > the DTS, the compatible used was allwinner,sun50i-a64-system-controller.
>> > You also say that you've never seen use of it. How can you make that
>> > claim ? There is a lot of downstream users of DTS now (FreeBSD, NetBSD,
>> > OpenBSD and even RiscOS and Haiku iirc), it's not just Linux.
>>
>> The original "a64-sram-controller" was never used in any upstream device
>> tree, be it Linux or U-boot. Since the projects you listed all derive
>> their device trees from U-boot, with maybe some extra devices on top,
>> they would either have had the "system-controller" version, or the even
>> earlier version in U-boot where the emac node just lists the syscon
>> address range as its own.
>>
>> U-boot has the latter "system-controller" because it was copied from Linux.
>> The original use-case was a SoC-specific compatible string followed by the
>> very generic "syscon". The latter "syscon" binding is what we actually
>> support in Linux and U-boot. Same goes for NetBSD and OpenBSD.
>>
>> IMHO FreeBSD could also move to a generic syscon API, instead of
>> "system-controller" for sunxi and "grf" for rockchip.
>
> We decided to always implement a specific driver if the node have
> another compatible than syscon (I don't see the point of having a
> syscon compatible plus another one)
In most cases, the fallback is there in situations when support for the
older or more generic device type can work with the new device, albeit
at lower efficiency or without full functionality.
Using a specific one with the "syscon" doesn't quite make sense to me
either, but some platforms, like Rockchip, do use it. I suppose it would
help them identify the type of syscon, but I think that is really tied
to the consumer.
>> You can continue to support old device trees and the unlisted compatible
>> through the generic "syscon" fallback. For the updated device tree you will
>> have to support the new compatible, along with supporting the SRAM mappings.
>> The SRAM mappings are why the "syscon" compatible was removed. It just
>> doesn't fit the semantics described by the "syscon" binding.
>
> I understand the removal of syscon since the sram addition, I just
> don't understand the strict renaming and why adding a new one couldn't
> work.
You mean having something like the following?
compatible = "allwinner,sun50i-a64-system-control",
"allwinner,sun50i-a64-system-controller";
Well, the "system-controller" compatible was never listed in any binding,
and it shouldn't have been there in the first place. And I think having
the compatibles match up with the datasheet is better. Last, it is not
an actual "controller". It is just a set of control registers.
ChenYu
>> > Also this compatible is currently the one used in the u-boot dts,
>> > which mean that users of the embedded DTB use or can use it (which is
>> > the default for EFI users of U-Boot).
>>
>> As mentioned above, the old device tree uses the syscon binding, which
>> you can continue to support. The new binding is add support for the SRAM
>> mapping system. In addition, you probably don't want two device drivers
>> supporting the same compatible string. On Linux it's even worse, because
>> the "syscon" driver sidesteps the device model and isn't an actual device
>> driver.
>>
>> Regards
>> ChenYu
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
>
> --
> Emmanuel Vadot <manu@xxxxxxxxxxxxxxxx> <manu@xxxxxxxxxxx>
