[PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Andre Przywara]

Commit 1f1f5183981d ("arm64: dts: allwinner: a64: add SRAM controller
device tree node") changed the (so far unused) compatible name of the
sytem controller node, to match a specific SRAM controller driver
instead of relying on the generic syscon driver. The Ethernet driver
needs a register in there, so it got amended to use the new driver instead
of the generic syscon approach. Consequently the "syscon" compatible has
been dropped, as it's not needed anymore and would compete with the
new SRAM driver.

However this breaks existing DT consumers like older kernels, which
expect the node pointed to by the syscon property to contain the string
"syscon" somewhere in the compatible list. When such a DT is given to an
older kernel (<4.19, for instance one on a distro installer image),
the Ethernet will not probe successfully:
----------
dwmac-sun8i 1c30000.ethernet: PTP uses main clock
dwmac-sun8i 1c30000.ethernet: Linked as a consumer to regulator.6
dwmac-sun8i 1c30000.ethernet: Unable to map syscon: -22
dwmac-sun8i 1c30000.ethernet: Dropping the link to regulator.6
dwmac-sun8i: probe of 1c30000.ethernet failed with error -22
----------

To avoid this problem, (re-)add the "syscon" string to the compatible
list of the sytem controller. This should make both older and newer
kernels happy:
- A newer kernel will try to find an existing *device* offering the regmap
  first. The "syscon" string itself will not trigger a driver probe on
  its own, the node must be explicitly referenced by another driver, wanting
  to use the regmap. Newer kernels won't do it this way and will use the
  regmap offered by the SRAM driver.
- An older kernel will try the syscon way, and the system controller /
  SRAM driver DT node is still fully syscon compatible.

So by adding "syscon" we make everyone happy: Newer kernels will ignore
it (knowing a better way), and older kernels will still work.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
Hi,

I think we need to amend the Ethernet driver now to not too easily use
syscon as a fallback, in case dwmac-sun8i probed earlier than the SRAM driver.
Will try this later tonight, but just wanted to start the discussion.

Thanks,
Andre.

 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index d3daf90a8715..8f2dad7e5d06 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -197,7 +197,8 @@
 		};
 
 		syscon: syscon@1c00000 {
-			compatible = "allwinner,sun50i-a64-system-control";
+			compatible = "allwinner,sun50i-a64-system-control",
+				     "syscon";
 			reg = <0x01c00000 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
-- 
2.17.1

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Chen-Yu Tsai]

On Mon, Sep 17, 2018 at 11:29 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> Commit 1f1f5183981d ("arm64: dts: allwinner: a64: add SRAM controller
> device tree node") changed the (so far unused) compatible name of the
> sytem controller node, to match a specific SRAM controller driver
> instead of relying on the generic syscon driver. The Ethernet driver
> needs a register in there, so it got amended to use the new driver instead
> of the generic syscon approach. Consequently the "syscon" compatible has
> been dropped, as it's not needed anymore and would compete with the
> new SRAM driver.
>
> However this breaks existing DT consumers like older kernels, which
> expect the node pointed to by the syscon property to contain the string
> "syscon" somewhere in the compatible list. When such a DT is given to an
> older kernel (<4.19, for instance one on a distro installer image),
> the Ethernet will not probe successfully:
> ----------
> dwmac-sun8i 1c30000.ethernet: PTP uses main clock
> dwmac-sun8i 1c30000.ethernet: Linked as a consumer to regulator.6
> dwmac-sun8i 1c30000.ethernet: Unable to map syscon: -22
> dwmac-sun8i 1c30000.ethernet: Dropping the link to regulator.6
> dwmac-sun8i: probe of 1c30000.ethernet failed with error -22
> ----------
>
> To avoid this problem, (re-)add the "syscon" string to the compatible
> list of the sytem controller. This should make both older and newer
> kernels happy:
> - A newer kernel will try to find an existing *device* offering the regmap
>   first. The "syscon" string itself will not trigger a driver probe on
>   its own, the node must be explicitly referenced by another driver, wanting
>   to use the regmap. Newer kernels won't do it this way and will use the
>   regmap offered by the SRAM driver.
> - An older kernel will try the syscon way, and the system controller /
>   SRAM driver DT node is still fully syscon compatible.
>
> So by adding "syscon" we make everyone happy: Newer kernels will ignore
> it (knowing a better way), and older kernels will still work.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
> Hi,
>
> I think we need to amend the Ethernet driver now to not too easily use
> syscon as a fallback, in case dwmac-sun8i probed earlier than the SRAM driver.
> Will try this later tonight, but just wanted to start the discussion.

There is a good reason for removing the syscon compatible. It signals
that the block is a "generic" collection of random registers, which in
this case is not. This is the main reason. A supplimental one follows.

Implementations follow this "generic" concept and offer unrestricted,
concurrent access to the syscon region. Linux and one of the BSDs AFAIK
are the same in that syscon is just an API and not a full-blown device.
Thus the other sram controller driver that we bind to it has no way of
knowing about other accesses happening under its nose. U-boot, IIRC,
makes syscon an actual device driver, so you can't even have both drivers
bind to the same node.

Yes these are implementation issues, but other drivers might depend on it.
The syscon driver is simple and nice to use, until it isn't.

Regards
ChenYu

> Thanks,
> Andre.
>
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index d3daf90a8715..8f2dad7e5d06 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -197,7 +197,8 @@
>                 };
>
>                 syscon: syscon@1c00000 {
> -                       compatible = "allwinner,sun50i-a64-system-control";
> +                       compatible = "allwinner,sun50i-a64-system-control",
> +                                    "syscon";
>                         reg = <0x01c00000 0x1000>;
>                         #address-cells = <1>;
>                         #size-cells = <1>;
> --
> 2.17.1
>

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Andre Przywara]

On Fri, 21 Sep 2018 17:47:50 +0800
Chen-Yu Tsai <wens@csie.org> wrote:

Hi,

(sorry if I steer the discussion in the wrong direction, I am not quite
sure if you dismiss the idea of staying compatible in general or if you
just point to an issue with my particular solution. Trying to reason
on both below.)

> On Mon, Sep 17, 2018 at 11:29 PM Andre Przywara
> <andre.przywara@arm.com> wrote:
> >
> > Commit 1f1f5183981d ("arm64: dts: allwinner: a64: add SRAM
> > controller device tree node") changed the (so far unused)
> > compatible name of the sytem controller node, to match a specific
> > SRAM controller driver instead of relying on the generic syscon
> > driver. The Ethernet driver needs a register in there, so it got
> > amended to use the new driver instead of the generic syscon
> > approach. Consequently the "syscon" compatible has been dropped, as
> > it's not needed anymore and would compete with the new SRAM driver.
> >
> > However this breaks existing DT consumers like older kernels, which
> > expect the node pointed to by the syscon property to contain the
> > string "syscon" somewhere in the compatible list. When such a DT is
> > given to an older kernel (<4.19, for instance one on a distro
> > installer image), the Ethernet will not probe successfully:
> > ----------
> > dwmac-sun8i 1c30000.ethernet: PTP uses main clock
> > dwmac-sun8i 1c30000.ethernet: Linked as a consumer to regulator.6
> > dwmac-sun8i 1c30000.ethernet: Unable to map syscon: -22
> > dwmac-sun8i 1c30000.ethernet: Dropping the link to regulator.6
> > dwmac-sun8i: probe of 1c30000.ethernet failed with error -22
> > ----------
> >
> > To avoid this problem, (re-)add the "syscon" string to the
> > compatible list of the sytem controller. This should make both
> > older and newer kernels happy:
> > - A newer kernel will try to find an existing *device* offering the
> > regmap first. The "syscon" string itself will not trigger a driver
> > probe on its own, the node must be explicitly referenced by another
> > driver, wanting to use the regmap. Newer kernels won't do it this
> > way and will use the regmap offered by the SRAM driver.
> > - An older kernel will try the syscon way, and the system
> > controller / SRAM driver DT node is still fully syscon compatible.
> >
> > So by adding "syscon" we make everyone happy: Newer kernels will
> > ignore it (knowing a better way), and older kernels will still work.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > Hi,
> >
> > I think we need to amend the Ethernet driver now to not too easily
> > use syscon as a fallback, in case dwmac-sun8i probed earlier than
> > the SRAM driver. Will try this later tonight, but just wanted to
> > start the discussion.  
> 
> There is a good reason for removing the syscon compatible. It signals
> that the block is a "generic" collection of random registers, which in
> this case is not. This is the main reason. A supplimental one follows.

But syscon would only be the fallback, following the DT compatible
string semantics: If a kernel doesn't have a specific driver, it could
use "syscon" to get at least some basic functionality.
In this case I would expect the SRAM driver to be coupled with the DE2
driver requiring it, so we have the SRAM with it and don't need it
without it. So syscon is a reasonable fallback for older kernels.

I understand that from a "designer's" point of view syscon is not really
applicable (see the end of the first paragraph of the commit message).
The problem is: we broke compatibility with older kernels. I missed that
point back when the change was posted, because I thought older kernels
would just follow the syscon phandle. I missed that they require a
"syscon" compatible string in there.
So I am looking for a simple solution to have all the new features but
keep the new DT compatible with older kernels.
(If the reason for staying compatible in this direction is not clear,
see below.)

> Implementations follow this "generic" concept and offer unrestricted,
> concurrent access to the syscon region. Linux and one of the BSDs
> AFAIK are the same in that syscon is just an API and not a full-blown
> device. Thus the other sram controller driver that we bind to it has
> no way of knowing about other accesses happening under its nose.
> U-boot, IIRC, makes syscon an actual device driver, so you can't even
> have both drivers bind to the same node.

I don't think that the drivers would conflict on the hardware level,
would they? Even if both would match: A kernel would check the
compatible strings in order: If it matches on the specific string, the
new driver initialises and claims the MMIO region. Any attempts from
other drivers (including syscon) after this point would just fail, on
the ioremap() in Linux for instance. Similar on the other hand: If
syscon is the first getting probed, its ioremap() wins and the specific
driver would fail on probing. But DT's compatible string list semantics
would ensure the proper order. With that slightly awkward behavior for
syscon, which we can fix.

So we won't have the issue of two drivers competing for the same
hardware. I believe this is true for every halfway sane implementation.

> Yes these are implementation issues, but other drivers might depend
> on it. The syscon driver is simple and nice to use, until it isn't.

I see that and I understand that it probably should have been a
specific driver from the beginning, but that this is always easy to say
at hindsight ;-) And since I don't have a time machine, we need to fix
it somehow differently.

So the practical problem at hand: I can't reasonably push this DT into
U-Boot anymore, so that it gets passed on to EFI apps (grub) and on from
there to kernels: Anyone wanting to boot a kernel <4.19 (Ubuntu 18.04
installer) or some FreeBSD, for instance, will now no longer
have Ethernet. Which worked before, the 4.18 DTs we have in U-Boot
boot >=4.15 kernels just fine. I could keep the 4.18 DTs in U-Boot, but
will loose all the new features and can't update anymore easily.

I see that for the traditional embedded Linux use case this isn't an
issue, but I believe all those SBC boards are beyond that and users
want to boot of the shelf distros (installers) with them. Which they
can right now, but can't anymore with this new DT.

Cheers,
Andre.
 
> >
> >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
> > d3daf90a8715..8f2dad7e5d06 100644 ---
> > a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++
> > b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -197,7 +197,8 @@
> >                 };
> >
> >                 syscon: syscon@1c00000 {
> > -                       compatible =
> > "allwinner,sun50i-a64-system-control";
> > +                       compatible =
> > "allwinner,sun50i-a64-system-control",
> > +                                    "syscon";
> >                         reg = <0x01c00000 0x1000>;
> >                         #address-cells = <1>;
> >                         #size-cells = <1>;
> > --
> > 2.17.1
> >  

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Chen-Yu Tsai]

On Fri, Sep 21, 2018 at 10:35 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Fri, 21 Sep 2018 17:47:50 +0800
> Chen-Yu Tsai <wens@csie.org> wrote:
>
> Hi,
>
> (sorry if I steer the discussion in the wrong direction, I am not quite
> sure if you dismiss the idea of staying compatible in general or if you
> just point to an issue with my particular solution. Trying to reason
> on both below.)
>
> > On Mon, Sep 17, 2018 at 11:29 PM Andre Przywara
> > <andre.przywara@arm.com> wrote:
> > >
> > > Commit 1f1f5183981d ("arm64: dts: allwinner: a64: add SRAM
> > > controller device tree node") changed the (so far unused)
> > > compatible name of the sytem controller node, to match a specific
> > > SRAM controller driver instead of relying on the generic syscon
> > > driver. The Ethernet driver needs a register in there, so it got
> > > amended to use the new driver instead of the generic syscon
> > > approach. Consequently the "syscon" compatible has been dropped, as
> > > it's not needed anymore and would compete with the new SRAM driver.
> > >
> > > However this breaks existing DT consumers like older kernels, which
> > > expect the node pointed to by the syscon property to contain the
> > > string "syscon" somewhere in the compatible list. When such a DT is
> > > given to an older kernel (<4.19, for instance one on a distro
> > > installer image), the Ethernet will not probe successfully:
> > > ----------
> > > dwmac-sun8i 1c30000.ethernet: PTP uses main clock
> > > dwmac-sun8i 1c30000.ethernet: Linked as a consumer to regulator.6
> > > dwmac-sun8i 1c30000.ethernet: Unable to map syscon: -22
> > > dwmac-sun8i 1c30000.ethernet: Dropping the link to regulator.6
> > > dwmac-sun8i: probe of 1c30000.ethernet failed with error -22
> > > ----------
> > >
> > > To avoid this problem, (re-)add the "syscon" string to the
> > > compatible list of the sytem controller. This should make both
> > > older and newer kernels happy:
> > > - A newer kernel will try to find an existing *device* offering the
> > > regmap first. The "syscon" string itself will not trigger a driver
> > > probe on its own, the node must be explicitly referenced by another
> > > driver, wanting to use the regmap. Newer kernels won't do it this
> > > way and will use the regmap offered by the SRAM driver.
> > > - An older kernel will try the syscon way, and the system
> > > controller / SRAM driver DT node is still fully syscon compatible.
> > >
> > > So by adding "syscon" we make everyone happy: Newer kernels will
> > > ignore it (knowing a better way), and older kernels will still work.
> > >
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > > ---
> > > Hi,
> > >
> > > I think we need to amend the Ethernet driver now to not too easily
> > > use syscon as a fallback, in case dwmac-sun8i probed earlier than
> > > the SRAM driver. Will try this later tonight, but just wanted to
> > > start the discussion.
> >
> > There is a good reason for removing the syscon compatible. It signals
> > that the block is a "generic" collection of random registers, which in
> > this case is not. This is the main reason. A supplimental one follows.
>
> But syscon would only be the fallback, following the DT compatible
> string semantics: If a kernel doesn't have a specific driver, it could
> use "syscon" to get at least some basic functionality.

Unfortunately that is not how syscon actually works. See below.

> In this case I would expect the SRAM driver to be coupled with the DE2
> driver requiring it, so we have the SRAM with it and don't need it
> without it. So syscon is a reasonable fallback for older kernels.
>
> I understand that from a "designer's" point of view syscon is not really
> applicable (see the end of the first paragraph of the commit message).
> The problem is: we broke compatibility with older kernels. I missed that
> point back when the change was posted, because I thought older kernels
> would just follow the syscon phandle. I missed that they require a
> "syscon" compatible string in there.
> So I am looking for a simple solution to have all the new features but
> keep the new DT compatible with older kernels.
> (If the reason for staying compatible in this direction is not clear,
> see below.)
>
> > Implementations follow this "generic" concept and offer unrestricted,
> > concurrent access to the syscon region. Linux and one of the BSDs
> > AFAIK are the same in that syscon is just an API and not a full-blown
> > device. Thus the other sram controller driver that we bind to it has
> > no way of knowing about other accesses happening under its nose.
> > U-boot, IIRC, makes syscon an actual device driver, so you can't even
> > have both drivers bind to the same node.
>
> I don't think that the drivers would conflict on the hardware level,
> would they? Even if both would match: A kernel would check the
> compatible strings in order: If it matches on the specific string, the
> new driver initialises and claims the MMIO region. Any attempts from
> other drivers (including syscon) after this point would just fail, on
> the ioremap() in Linux for instance. Similar on the other hand: If
> syscon is the first getting probed, its ioremap() wins and the specific
> driver would fail on probing. But DT's compatible string list semantics
> would ensure the proper order. With that slightly awkward behavior for
> syscon, which we can fix.
>
> So we won't have the issue of two drivers competing for the same
> hardware. I believe this is true for every halfway sane implementation.

Actually, no. syscon is not a "device driver" and does not follow the
driver model. Not in Linux at least. syscons are registered on first use.
When a consumer looks up a syscon phandle, if the phandle is valid and
the node pointed to has a "syscon" compatible, and its syscon has not
been registered, the syscon framework just goes ahead and creates an
MMIO regmap. There is no exclusion.

Recap: syscon does not follow the driver model or compatible string list
parsing semantics.

Besides, ioremap doesn't guarantee the region is only used by one consumer.
Requesting the region does. (IIRC there are ways around it, like being a
sub-device of the one that first requested it, but that's off topic.)

> > Yes these are implementation issues, but other drivers might depend
> > on it. The syscon driver is simple and nice to use, until it isn't.
>
> I see that and I understand that it probably should have been a
> specific driver from the beginning, but that this is always easy to say
> at hindsight ;-) And since I don't have a time machine, we need to fix
> it somehow differently.
>
> So the practical problem at hand: I can't reasonably push this DT into
> U-Boot anymore, so that it gets passed on to EFI apps (grub) and on from
> there to kernels: Anyone wanting to boot a kernel <4.19 (Ubuntu 18.04
> installer) or some FreeBSD, for instance, will now no longer
> have Ethernet. Which worked before, the 4.18 DTs we have in U-Boot
> boot >=4.15 kernels just fine. I could keep the 4.18 DTs in U-Boot, but
> will loose all the new features and can't update anymore easily.

I see your point, and I can't offer any good solutions right now.

ChenYu

> I see that for the traditional embedded Linux use case this isn't an
> issue, but I believe all those SBC boards are beyond that and users
> want to boot of the shelf distros (installers) with them. Which they
> can right now, but can't anymore with this new DT.
>
> Cheers,
> Andre.
>
> > >
> > >  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> > > b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
> > > d3daf90a8715..8f2dad7e5d06 100644 ---
> > > a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++
> > > b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -197,7 +197,8 @@
> > >                 };
> > >
> > >                 syscon: syscon@1c00000 {
> > > -                       compatible =
> > > "allwinner,sun50i-a64-system-control";
> > > +                       compatible =
> > > "allwinner,sun50i-a64-system-control",
> > > +                                    "syscon";
> > >                         reg = <0x01c00000 0x1000>;
> > >                         #address-cells = <1>;
> > >                         #size-cells = <1>;
> > > --
> > > 2.17.1
> > >
>

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[André Przywara]

On 9/21/18 3:57 PM, Chen-Yu Tsai wrote:

Hi,

> On Fri, Sep 21, 2018 at 10:35 PM Andre Przywara <andre.przywara@arm.com> wrote:
>>
>> On Fri, 21 Sep 2018 17:47:50 +0800
>> Chen-Yu Tsai <wens@csie.org> wrote:
>>
>> Hi,
>>
>> (sorry if I steer the discussion in the wrong direction, I am not quite
>> sure if you dismiss the idea of staying compatible in general or if you
>> just point to an issue with my particular solution. Trying to reason
>> on both below.)
>>
>>> On Mon, Sep 17, 2018 at 11:29 PM Andre Przywara
>>> <andre.przywara@arm.com> wrote:
>>>>
>>>> Commit 1f1f5183981d ("arm64: dts: allwinner: a64: add SRAM
>>>> controller device tree node") changed the (so far unused)
>>>> compatible name of the sytem controller node, to match a specific
>>>> SRAM controller driver instead of relying on the generic syscon
>>>> driver. The Ethernet driver needs a register in there, so it got
>>>> amended to use the new driver instead of the generic syscon
>>>> approach. Consequently the "syscon" compatible has been dropped, as
>>>> it's not needed anymore and would compete with the new SRAM driver.
>>>>
>>>> However this breaks existing DT consumers like older kernels, which
>>>> expect the node pointed to by the syscon property to contain the
>>>> string "syscon" somewhere in the compatible list. When such a DT is
>>>> given to an older kernel (<4.19, for instance one on a distro
>>>> installer image), the Ethernet will not probe successfully:
>>>> ----------
>>>> dwmac-sun8i 1c30000.ethernet: PTP uses main clock
>>>> dwmac-sun8i 1c30000.ethernet: Linked as a consumer to regulator.6
>>>> dwmac-sun8i 1c30000.ethernet: Unable to map syscon: -22
>>>> dwmac-sun8i 1c30000.ethernet: Dropping the link to regulator.6
>>>> dwmac-sun8i: probe of 1c30000.ethernet failed with error -22
>>>> ----------
>>>>
>>>> To avoid this problem, (re-)add the "syscon" string to the
>>>> compatible list of the sytem controller. This should make both
>>>> older and newer kernels happy:
>>>> - A newer kernel will try to find an existing *device* offering the
>>>> regmap first. The "syscon" string itself will not trigger a driver
>>>> probe on its own, the node must be explicitly referenced by another
>>>> driver, wanting to use the regmap. Newer kernels won't do it this
>>>> way and will use the regmap offered by the SRAM driver.
>>>> - An older kernel will try the syscon way, and the system
>>>> controller / SRAM driver DT node is still fully syscon compatible.
>>>>
>>>> So by adding "syscon" we make everyone happy: Newer kernels will
>>>> ignore it (knowing a better way), and older kernels will still work.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>> ---
>>>> Hi,
>>>>
>>>> I think we need to amend the Ethernet driver now to not too easily
>>>> use syscon as a fallback, in case dwmac-sun8i probed earlier than
>>>> the SRAM driver. Will try this later tonight, but just wanted to
>>>> start the discussion.
>>>
>>> There is a good reason for removing the syscon compatible. It signals
>>> that the block is a "generic" collection of random registers, which in
>>> this case is not. This is the main reason. A supplimental one follows.
>>
>> But syscon would only be the fallback, following the DT compatible
>> string semantics: If a kernel doesn't have a specific driver, it could
>> use "syscon" to get at least some basic functionality.
> 
> Unfortunately that is not how syscon actually works. See below.

I think I understand that syscon is different (doesn't probe triggered
by its compatible string). And in this case it's the responsibility of
the "consuming" driver to ensure the DT semantics (see below).

>> In this case I would expect the SRAM driver to be coupled with the DE2
>> driver requiring it, so we have the SRAM with it and don't need it
>> without it. So syscon is a reasonable fallback for older kernels.
>>
>> I understand that from a "designer's" point of view syscon is not really
>> applicable (see the end of the first paragraph of the commit message).
>> The problem is: we broke compatibility with older kernels. I missed that
>> point back when the change was posted, because I thought older kernels
>> would just follow the syscon phandle. I missed that they require a
>> "syscon" compatible string in there.
>> So I am looking for a simple solution to have all the new features but
>> keep the new DT compatible with older kernels.
>> (If the reason for staying compatible in this direction is not clear,
>> see below.)
>>
>>> Implementations follow this "generic" concept and offer unrestricted,
>>> concurrent access to the syscon region. Linux and one of the BSDs
>>> AFAIK are the same in that syscon is just an API and not a full-blown
>>> device. Thus the other sram controller driver that we bind to it has
>>> no way of knowing about other accesses happening under its nose.
>>> U-boot, IIRC, makes syscon an actual device driver, so you can't even
>>> have both drivers bind to the same node.
>>
>> I don't think that the drivers would conflict on the hardware level,
>> would they? Even if both would match: A kernel would check the
>> compatible strings in order: If it matches on the specific string, the
>> new driver initialises and claims the MMIO region. Any attempts from
>> other drivers (including syscon) after this point would just fail, on
>> the ioremap() in Linux for instance. Similar on the other hand: If
>> syscon is the first getting probed, its ioremap() wins and the specific
>> driver would fail on probing. But DT's compatible string list semantics
>> would ensure the proper order. With that slightly awkward behavior for
>> syscon, which we can fix.
>>
>> So we won't have the issue of two drivers competing for the same
>> hardware. I believe this is true for every halfway sane implementation.
> 
> Actually, no. syscon is not a "device driver" and does not follow the
> driver model. Not in Linux at least. syscons are registered on first use.
> When a consumer looks up a syscon phandle, if the phandle is valid and
> the node pointed to has a "syscon" compatible, and its syscon has not
> been registered, the syscon framework just goes ahead and creates an
> MMIO regmap. There is no exclusion.
> 
> Recap: syscon does not follow the driver model or compatible string list
> parsing semantics.

I understand that, but the consuming driver currently follows the
semantics: I checks for a regmap offered by a proper device first, so
anything offered by a non-syscon driver. Then it only reverts to the
syscon method otherwise - to keep backwards compatibility.

> Besides, ioremap doesn't guarantee the region is only used by one consumer.
> Requesting the region does. (IIRC there are ways around it, like being a
> sub-device of the one that first requested it, but that's off topic.)

OK, that seems to be true. ioremap doesn't seem to check for existing
users at all (that' probably due to the "re" in its name). But I don't
think the matters here:
- Either the kernel has support for the proper driver - then the device
should always get the regmap from there and will never try syscon.
- If the kernel does not have this driver, syscon will do the trick.
Without any proper driver there is also no concurrent user of the area
covered by syscon.
- If there is a consuming driver using syscon and another consumer
requiring the specific driver, that should be considered a DT bug. Or we
don't care, if both consumers don't conflict in their MMIO accesses.

So where would be the problem? I don't see a valid case with two
ioremaps on the same region.
I don't think we need to care about anyone adding a random syscon user
to the DT, which conflicts with the SRAM driver. This would be just wrong.

Cheers,
Andre.

>>> Yes these are implementation issues, but other drivers might depend
>>> on it. The syscon driver is simple and nice to use, until it isn't.
>>
>> I see that and I understand that it probably should have been a
>> specific driver from the beginning, but that this is always easy to say
>> at hindsight ;-) And since I don't have a time machine, we need to fix
>> it somehow differently.
>>
>> So the practical problem at hand: I can't reasonably push this DT into
>> U-Boot anymore, so that it gets passed on to EFI apps (grub) and on from
>> there to kernels: Anyone wanting to boot a kernel <4.19 (Ubuntu 18.04
>> installer) or some FreeBSD, for instance, will now no longer
>> have Ethernet. Which worked before, the 4.18 DTs we have in U-Boot
>> boot >=4.15 kernels just fine. I could keep the 4.18 DTs in U-Boot, but
>> will loose all the new features and can't update anymore easily.
> 
> I see your point, and I can't offer any good solutions right now.
> 
> ChenYu
> 
>> I see that for the traditional embedded Linux use case this isn't an
>> issue, but I believe all those SBC boards are beyond that and users
>> want to boot of the shelf distros (installers) with them. Which they
>> can right now, but can't anymore with this new DT.
>>
>> Cheers,
>> Andre.
>>
>>>>
>>>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
>>>> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi index
>>>> d3daf90a8715..8f2dad7e5d06 100644 ---
>>>> a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi +++
>>>> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi @@ -197,7 +197,8 @@
>>>>                 };
>>>>
>>>>                 syscon: syscon@1c00000 {
>>>> -                       compatible =
>>>> "allwinner,sun50i-a64-system-control";
>>>> +                       compatible =
>>>> "allwinner,sun50i-a64-system-control",
>>>> +                                    "syscon";
>>>>                         reg = <0x01c00000 0x1000>;
>>>>                         #address-cells = <1>;
>>>>                         #size-cells = <1>;
>>>> --
>>>> 2.17.1
>>>>
>>

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1375 bytes --]

On Fri, Sep 21, 2018 at 03:35:22PM +0100, Andre Przywara wrote:
> The problem is: we broke compatibility with older kernels.

The problem is that we never said we wouldn't.

We've had this discussion a number of times. You forced the backward
compatibility onto us without any warning, and now we have to support
it. And it's already a pain. Maybe ARM can fix that problem by just
assigning more engineers to that, but that's not something we can do.

The fundamental difference is that we're mostly just a bunch of spare
time programmers working on this platform, with a partial
documentation for the controllers, at best.

You forced me to ask these developpers to work on their weekends and
evenings on some crazy corner cases to maintain the backward
compatibility. And honestly, both from a technical and human
standpoint, I definitely understand if some of them are just leaving
and don't want to work on it anymore. I would probably do the same in
their position.

And having to ask that for companies like ARM or SUSE just makes it
more frustrating to be honest. So there's simply no way you have
forward compatibility while I'm there. Or you manage to convince all
the ARM maintainers and enforce that compatibility for all the
platforms.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Andre Przywara]

On Fri, 21 Sep 2018 17:16:25 +0200
Maxime Ripard <maxime.ripard@bootlin.com> wrote:

Hi,

> On Fri, Sep 21, 2018 at 03:35:22PM +0100, Andre Przywara wrote:
> > The problem is: we broke compatibility with older kernels.  
> 
> The problem is that we never said we wouldn't.
> 
> We've had this discussion a number of times. You forced the backward
> compatibility onto us without any warning, and now we have to support
> it. And it's already a pain. Maybe ARM can fix that problem by just
> assigning more engineers to that, but that's not something we can do.

Sorry, but please leave my employer out of this. As far as Allwinner
stuff is concerned, I am as volunteer and spare-time as all the other
developers. I just use my ARM address because this is ARM policy (to
not hide your identity and contribute to projects under the same
address, even if this is in your spare time. Strange, I know, but this
is how it is).

And please don't claim I am forcing anything. If you look back and into
/src/linux/Maintainers, I have no power in the Allwinner sphere and
lost many battles in the past.
Please be assured that I don't feel too well in this position of
being the pesky guy pointing out that things broke again, but I try to
send patches instead of complaints, to offer a solution.
The idea is to have technical discussions instead of "political"
ones (whatever that means).

Also I was not asking to rewrite everything or turn stuff upside down,
instead trying to offer an easy solution (it's a one-liner DT patch!)
for this kind of problem. So far (4.15..4.18) we are actually forwards-
and backwards compatible, so it's no black magic.

> The fundamental difference is that we're mostly just a bunch of spare
> time programmers working on this platform, with a partial
> documentation for the controllers, at best.
> 
> You forced me to ask these developpers to work on their weekends and
> evenings on some crazy corner cases to maintain the backward
> compatibility. And honestly, both from a technical and human
> standpoint, I definitely understand if some of them are just leaving
> and don't want to work on it anymore. I would probably do the same in
> their position.
> 
> And having to ask that for companies like ARM or SUSE just makes it
> more frustrating to be honest. So there's simply no way you have
> forward compatibility while I'm there. Or you manage to convince all
> the ARM maintainers and enforce that compatibility for all the
> platforms.

I understand that from your point of view there is no way of investing
huge efforts in staying forward compatible, but I am not asking for
that (and by no way forcing this!). Instead this suggestion is a small
tweak to achieve this.

So I am sorry if those things frustrate you (which I can understand
very well), but I believe fixing the DT in a proper way is
much more user friendly in the long term (actually this issue was
brought forward by a user[1]).

Cheers,
Andre.

[1]
https://github.com/apritzel/arm-trusted-firmware/issues/10/#issuecomment-421368057

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Maxime Ripard]

On Mon, Sep 24, 2018 at 11:22:30AM +0100, Andre Przywara wrote:
> > The fundamental difference is that we're mostly just a bunch of spare
> > time programmers working on this platform, with a partial
> > documentation for the controllers, at best.
> > 
> > You forced me to ask these developpers to work on their weekends and
> > evenings on some crazy corner cases to maintain the backward
> > compatibility. And honestly, both from a technical and human
> > standpoint, I definitely understand if some of them are just leaving
> > and don't want to work on it anymore. I would probably do the same in
> > their position.
> > 
> > And having to ask that for companies like ARM or SUSE just makes it
> > more frustrating to be honest. So there's simply no way you have
> > forward compatibility while I'm there. Or you manage to convince all
> > the ARM maintainers and enforce that compatibility for all the
> > platforms.
> 
> I understand that from your point of view there is no way of investing
> huge efforts in staying forward compatible, but I am not asking for
> that (and by no way forcing this!). Instead this suggestion is a small
> tweak to achieve this.

We're trying to remain compatible but if there's any technical reason,
then we won't be. I don't want anyone to assume we will, and to rely
on the fact that we are actually guaranteeing this.

> So I am sorry if those things frustrate you (which I can understand
> very well), but I believe fixing the DT in a proper way is
> much more user friendly in the long term (actually this issue was
> brought forward by a user[1]).

Given the current state of the industry, I don't really see how the DT
can allow you to do what you are trying to achieve.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[André Przywara]

On 9/26/18 10:52 AM, Maxime Ripard wrote:

Hi Maxime,

> On Mon, Sep 24, 2018 at 11:22:30AM +0100, Andre Przywara wrote:
>>> The fundamental difference is that we're mostly just a bunch of spare
>>> time programmers working on this platform, with a partial
>>> documentation for the controllers, at best.
>>>
>>> You forced me to ask these developpers to work on their weekends and
>>> evenings on some crazy corner cases to maintain the backward
>>> compatibility. And honestly, both from a technical and human
>>> standpoint, I definitely understand if some of them are just leaving
>>> and don't want to work on it anymore. I would probably do the same in
>>> their position.
>>>
>>> And having to ask that for companies like ARM or SUSE just makes it
>>> more frustrating to be honest. So there's simply no way you have
>>> forward compatibility while I'm there. Or you manage to convince all
>>> the ARM maintainers and enforce that compatibility for all the
>>> platforms.
>>
>> I understand that from your point of view there is no way of investing
>> huge efforts in staying forward compatible, but I am not asking for
>> that (and by no way forcing this!). Instead this suggestion is a small
>> tweak to achieve this.
> 
> We're trying to remain compatible but if there's any technical reason,
> then we won't be. I don't want anyone to assume we will, and to rely
> on the fact that we are actually guaranteeing this.

I understand that this is not the forum to discuss this, so have to
accept your position here. But I don't think this means we can't try to
solve those issues on case-by-case base? Just because we don't guarantee
this doesn't mean we shouldn't even try.

>> So I am sorry if those things frustrate you (which I can understand
>> very well), but I believe fixing the DT in a proper way is
>> much more user friendly in the long term (actually this issue was
>> brought forward by a user[1]).
> 
> Given the current state of the industry, I don't really see how the DT
> can allow you to do what you are trying to achieve.

I am not sure I do understand what you mean with "industry"? If you are
thinking about the SoC vendor or board vendors to push this endeavor and
create stable bindings and DTs from the beginning: I see that this won't
work with the prevalent attitude across many vendors today.

But in our case we (as the community) drive the DTs and the bindings
anyway, and we decided to mostly ignore Allwinner's DT effort - for good
reasons. So we can - given consensus on the goals and approach - make
this possible and don't need to rely on any company or "industry".

I find it a bit pity that we are stuck with this embedded approach,
where each board(!) vendor AND community members produce gazillions of
questionable images with all kind of distribution flavors.
At the moment (mainline U-Boot with EFI support and shipping with decent
DTs) we are very close to let people install distros with off-the-shelf
installers. I think this is a goal worthwhile fighting for.

Cheers,
Andre.

Re: [PATCH] arm64: dts: allwinner: a64: Re-add "syscon" compatible

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 1502 bytes --]

On Mon, Oct 01, 2018 at 01:55:19AM +0100, André Przywara wrote:
> >> So I am sorry if those things frustrate you (which I can understand
> >> very well), but I believe fixing the DT in a proper way is
> >> much more user friendly in the long term (actually this issue was
> >> brought forward by a user[1]).
> > 
> > Given the current state of the industry, I don't really see how the DT
> > can allow you to do what you are trying to achieve.
> 
> I am not sure I do understand what you mean with "industry"? If you are
> thinking about the SoC vendor or board vendors to push this endeavor and
> create stable bindings and DTs from the beginning: I see that this won't
> work with the prevalent attitude across many vendors today.

This is what I meant.

> But in our case we (as the community) drive the DTs and the bindings
> anyway, and we decided to mostly ignore Allwinner's DT effort - for good
> reasons. So we can - given consensus on the goals and approach - make
> this possible and don't need to rely on any company or "industry".

Actually, I think this is part of the reason. We like our DT bindings
so much that we are ready to spend monthes debating on it, while with
ACPI (or the DT from the older days, from what I understood) you're
stuck with whatever comes with the board. Whether one likes it or not
doesn't matter. And then you can use whatever kernel with it.

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel