Re: [PATCH net-next v2 15/15] arm64: dts: allwinner: a64: add SRAM controller device tree node

[Maxime Ripard]

Hi,

On Tue, May 15, 2018 at 11:47:16PM -0700, Chen-Yu Tsai wrote:
> On Mon, May 14, 2018 at 1:03 AM, Maxime Ripard
> <maxime.rip...@bootlin.com> wrote:
> > 1;5201;0c
> > On Sun, May 13, 2018 at 12:37:49PM -0700, Chen-Yu Tsai wrote:
> >> On Wed, May 2, 2018 at 4:54 AM, Maxime Ripard <maxime.rip...@bootlin.com> 
> >> wrote:
> >> > On Wed, May 02, 2018 at 06:19:51PM +0800, Icenowy Zheng wrote:
> >> >>
> >> >>
> >> >> 于 2018年5月2日 GMT+08:00 下午5:53:21, Chen-Yu Tsai <w...@csie.org> 写到:
> >> >> >On Wed, May 2, 2018 at 5:51 PM, Maxime Ripard
> >> >> ><maxime.rip...@bootlin.com> wrote:
> >> >> >> Hi,
> >> >> >>
> >> >> >> On Wed, May 02, 2018 at 12:12:27AM +0800, Chen-Yu Tsai wrote:
> >> >> >>> From: Icenowy Zheng <icen...@aosc.io>
> >> >> >>>
> >> >> >>> Allwinner A64 has a SRAM controller, and in the device tree
> >> >> >currently
> >> >> >>> we have a syscon node to enable EMAC driver to access the EMAC clock
> >> >> >>> register. As SRAM controller driver can now export regmap for this
> >> >> >>> register, replace the syscon node to the SRAM controller device
> >> >> >node,
> >> >> >>> and let EMAC driver to acquire its EMAC clock regmap.
> >> >> >>>
> >> >> >>> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >> >> >>> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >> >> >>> ---
> >> >> >>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23
> >> >> >+++
> >> >> >>>  1 file changed, 19 insertions(+), 4 deletions(-)
> >> >> >>>
> >> >> >>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >> >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >> >>> index 1b2ef28c42bd..1c37659d9d41 100644
> >> >> >>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >> >>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >> >>> @@ -168,10 +168,25 @@
> >> >> >>>   #size-cells = <1>;
> >> >> >>>   ranges;
> >> >> >>>
> >> >> >>> - syscon: syscon@1c0 {
> >> >> >>> - compatible =
> >> >> >"allwinner,sun50i-a64-system-controller",
> >> >> >>> - "syscon";
> >> >> >>> + sram_controller: sram-controller@1c0 {
> >> >> >>> + compatible =
> >> >> >"allwinner,sun50i-a64-sram-controller";
> >> >> >>
> >> >> >> I don't think there's anything preventing us from keeping the
> >> >> >> -system-controller compatible. It's what was in the DT before, and
> >> >> >> it's how it's called in the datasheet.
> >> >> >
> >> >> >I actually meant to ask you about this. The -system-controller
> >> >> >compatible matches the datasheet better. Maybe we should just
> >> >> >switch to that one?
> >> >>
> >> >> No, if we do the switch the system-controller compatible,
> >> >> the device will be probed on the same memory region with
> >> >> a syscon on old DTs.
> >> >
> >> > The device hasn't magically changed either. Maybe we just need to add
> >> > a check to make sure we don't have the syscon compatible in the SRAM
> >> > driver probe so that the double driver issue doesn't happen?
> >>
> >> The syscon interface (which is not even a full blown device driver)
> >> only looks at the "syscon" compatible. Either way we're removing that
> >> part from the device tree so things should be ok for new device trees.
> >> As Maxime mentioned we can do a check for the syscon compatible and
> >> either give a warning to the user asking them to update their device
> >> tree, or not register our custom regmap, or not probe the SRAM driver.
> >> Personally I prefer the first option. The system controller block is
> >> probed before any syscon users, so we should be fine, given the dwmac
> >> driver goes the custom regmap path first.
> >>
> >> BTW, I still might end up changing the compatible. The manual uses
> >> "system control", not "system controller", which I think makes sense,
> >> since it is just a bunch of register files, kind of like the GRF
> >> (General Register Files) block found in Rockchip SoCs [1], and not an
> >> actual "controller".
> >
> > I'm not really fond of that, but we should at least make it consistent
> > on the other patches Paul sent then.
> 
> For the A10s / A13 right?

And A33, yep.

> I think my naming is slightly better, but it's just a minor detail.

Let's do this then.

> While we're still debating this, can I merge the R40 stuff first?
> The driver bits are already in.

Yep, go ahead.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 15/15] arm64: dts: allwinner: a64: add SRAM controller device tree node

[Maxime Ripard]

1;5201;0c
On Sun, May 13, 2018 at 12:37:49PM -0700, Chen-Yu Tsai wrote:
> On Wed, May 2, 2018 at 4:54 AM, Maxime Ripard <maxime.rip...@bootlin.com> 
> wrote:
> > On Wed, May 02, 2018 at 06:19:51PM +0800, Icenowy Zheng wrote:
> >>
> >>
> >> 于 2018年5月2日 GMT+08:00 下午5:53:21, Chen-Yu Tsai <w...@csie.org> 写到:
> >> >On Wed, May 2, 2018 at 5:51 PM, Maxime Ripard
> >> ><maxime.rip...@bootlin.com> wrote:
> >> >> Hi,
> >> >>
> >> >> On Wed, May 02, 2018 at 12:12:27AM +0800, Chen-Yu Tsai wrote:
> >> >>> From: Icenowy Zheng <icen...@aosc.io>
> >> >>>
> >> >>> Allwinner A64 has a SRAM controller, and in the device tree
> >> >currently
> >> >>> we have a syscon node to enable EMAC driver to access the EMAC clock
> >> >>> register. As SRAM controller driver can now export regmap for this
> >> >>> register, replace the syscon node to the SRAM controller device
> >> >node,
> >> >>> and let EMAC driver to acquire its EMAC clock regmap.
> >> >>>
> >> >>> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >> >>> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >> >>> ---
> >> >>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23
> >> >+++
> >> >>>  1 file changed, 19 insertions(+), 4 deletions(-)
> >> >>>
> >> >>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >>> index 1b2ef28c42bd..1c37659d9d41 100644
> >> >>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >> >>> @@ -168,10 +168,25 @@
> >> >>>   #size-cells = <1>;
> >> >>>   ranges;
> >> >>>
> >> >>> - syscon: syscon@1c0 {
> >> >>> - compatible =
> >> >"allwinner,sun50i-a64-system-controller",
> >> >>> - "syscon";
> >> >>> + sram_controller: sram-controller@1c0 {
> >> >>> + compatible =
> >> >"allwinner,sun50i-a64-sram-controller";
> >> >>
> >> >> I don't think there's anything preventing us from keeping the
> >> >> -system-controller compatible. It's what was in the DT before, and
> >> >> it's how it's called in the datasheet.
> >> >
> >> >I actually meant to ask you about this. The -system-controller
> >> >compatible matches the datasheet better. Maybe we should just
> >> >switch to that one?
> >>
> >> No, if we do the switch the system-controller compatible,
> >> the device will be probed on the same memory region with
> >> a syscon on old DTs.
> >
> > The device hasn't magically changed either. Maybe we just need to add
> > a check to make sure we don't have the syscon compatible in the SRAM
> > driver probe so that the double driver issue doesn't happen?
> 
> The syscon interface (which is not even a full blown device driver)
> only looks at the "syscon" compatible. Either way we're removing that
> part from the device tree so things should be ok for new device trees.
> As Maxime mentioned we can do a check for the syscon compatible and
> either give a warning to the user asking them to update their device
> tree, or not register our custom regmap, or not probe the SRAM driver.
> Personally I prefer the first option. The system controller block is
> probed before any syscon users, so we should be fine, given the dwmac
> driver goes the custom regmap path first.
> 
> BTW, I still might end up changing the compatible. The manual uses
> "system control", not "system controller", which I think makes sense,
> since it is just a bunch of register files, kind of like the GRF
> (General Register Files) block found in Rockchip SoCs [1], and not an
> actual "controller".

I'm not really fond of that, but we should at least make it consistent
on the other patches Paul sent then.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH RESEND net-next v2 1/8] dt-bindings: net: dwmac-sun8i: Clean up clock delay chain descriptions

[Maxime Ripard]

On Sun, May 13, 2018 at 09:59:22PM -0700, Chen-Yu Tsai wrote:
> On Sun, May 13, 2018 at 1:29 PM, Andrew Lunn <and...@lunn.ch> wrote:
> > On Sun, May 13, 2018 at 01:11:08PM -0700, Chen-Yu Tsai wrote:
> >> On Sun, May 13, 2018 at 1:05 PM, Andrew Lunn <and...@lunn.ch> wrote:
> >> >> > Hi Chen-Yu
> >> >> >
> >> >> > Are these delays the MAC applies? Not the PHY. It would be good to
> >> >> > make it clear here these are MAC imposed delays.
> >> >>
> >> >> Yes these are applied on the MAC side. Being described in the device
> >> >> tree bindings for the MAC, I thought this was implied to be the case?
> >> >> Are there known exceptions?
> >> >
> >> > There is frequent confusion with this. Most of the time, the PHY does
> >> > the delay, not the MAC, based on the phy-mode. So the MAC doing it is
> >> > an exception in itself.
> >> >
> >> > Do you actually need these delays for the board you adding support
> >> > for? Does the PHY not support adding the needed delays? If you don't
> >> > need the delays, i would not even implement them.
> >>
> >> Yes this is already used on the Bananapi M3. This patch merely reformats
> >> the description and adds a note saying this only applies to RGMII mode.
> >
> > Yes, the current code is needed for the Bananapi M3. But you have
> > another patch which extends the code to support a smaller range. Do
> > you have a board which actually needs this? If not, i would not add
> > that new code.
> 
> IIRC the delay on the PHY side is either 2ns or none. The delay on the
> MAC side here is an order smaller, likely fine tuning to cope with board
> design deficiencies.
> 
> Currently no other board requires this, but this is already part of the
> binding. The new stuff limits the range for a specific SoC, simply because
> that is the range supported by the control register. Not implementing, i.e.
> supporting the whole range from the property, which might get truncated,
> doesn't make much sense to me.

With that driver we don't, but the previous design had the same
feature and it was used on more boards. It was simply initialized
statically in U-Boot, and not in Linux through the DT like it is done
here.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 00/15] ARM: sun8i: r40: Add Ethernet support

[Maxime Ripard]

On Thu, May 03, 2018 at 02:40:42PM -0400, David Miller wrote:
> From: Maxime Ripard <maxime.rip...@bootlin.com>
> Date: Thu, 3 May 2018 15:12:57 +0200
> 
> > Hi Dave,
> > 
> > On Wed, May 02, 2018 at 11:06:17AM -0400, David Miller wrote:
> >> From: Chen-Yu Tsai <w...@csie.org>
> >> Date: Wed, 2 May 2018 00:33:45 +0800
> >> 
> >> > I should've mentioned that patches 3 ~ 10, and only these, should go
> >> > through net-next. sunxi will handle the remaining clk, device tree, and
> >> > soc driver patches.
> >> 
> >> Ok, I just noticed this.
> >> 
> >> Why don't you just post those patches separately as a series on their
> >> own then, in order to avoid confusion?
> >> 
> >> Then you can adjust the patch series header posting to explain the
> >> non-net-next changes, where they got merged, and what they provide
> >> in order to faciliate the net-next changes.
> > 
> > I now that we usually have some feedback from non-net maintainers that
> > they actually prefer seeing the full picture (and I also tend to
> > prefer that as well) and having all the patches relevant to enable a
> > particular feature, even if it means getting multiple maintainers
> > involved.
> > 
> > Just to make sure we understood you fully, do you want Chen-Yu to
> > resend his serie following your comments, or was that just a general
> > remark for next time?
> 
> Yeah, good questions.
> 
> I think it can be argued either way.  For review having the complete
> context is important.
> 
> But from a maintainer's standpoint, when there is any ambiguity
> whatsoever about what patches go into this tree or that, it is really
> frowned upon and is quite error prone.
> 
> Also, that header posting is _SO_ important.  It explains the series.
> But for these 'partial apply' situations the header posting refers
> to patches not in the series.
> 
> This looks terrible in the logs, when, as I do, the header posting
> text is added to a marge commit for the series.  People will read it
> and say "where are all of these other changes mentioned in the text?
> was this series misapplied?"
> 
> That's why, maybe after the review is successful, I want the actual
> patch series standalone with appropriately updated header posting
> text.

Ok, thanks for the explanation, that makes sense :)

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 00/15] ARM: sun8i: r40: Add Ethernet support

[Maxime Ripard]

Hi Dave,

On Wed, May 02, 2018 at 11:06:17AM -0400, David Miller wrote:
> From: Chen-Yu Tsai <w...@csie.org>
> Date: Wed, 2 May 2018 00:33:45 +0800
> 
> > I should've mentioned that patches 3 ~ 10, and only these, should go
> > through net-next. sunxi will handle the remaining clk, device tree, and
> > soc driver patches.
> 
> Ok, I just noticed this.
> 
> Why don't you just post those patches separately as a series on their
> own then, in order to avoid confusion?
> 
> Then you can adjust the patch series header posting to explain the
> non-net-next changes, where they got merged, and what they provide
> in order to faciliate the net-next changes.

I now that we usually have some feedback from non-net maintainers that
they actually prefer seeing the full picture (and I also tend to
prefer that as well) and having all the patches relevant to enable a
particular feature, even if it means getting multiple maintainers
involved.

Just to make sure we understood you fully, do you want Chen-Yu to
resend his serie following your comments, or was that just a general
remark for next time?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 15/15] arm64: dts: allwinner: a64: add SRAM controller device tree node

[Maxime Ripard]

On Wed, May 02, 2018 at 06:19:51PM +0800, Icenowy Zheng wrote:
> 
> 
> 于 2018年5月2日 GMT+08:00 下午5:53:21, Chen-Yu Tsai <w...@csie.org> 写到:
> >On Wed, May 2, 2018 at 5:51 PM, Maxime Ripard
> ><maxime.rip...@bootlin.com> wrote:
> >> Hi,
> >>
> >> On Wed, May 02, 2018 at 12:12:27AM +0800, Chen-Yu Tsai wrote:
> >>> From: Icenowy Zheng <icen...@aosc.io>
> >>>
> >>> Allwinner A64 has a SRAM controller, and in the device tree
> >currently
> >>> we have a syscon node to enable EMAC driver to access the EMAC clock
> >>> register. As SRAM controller driver can now export regmap for this
> >>> register, replace the syscon node to the SRAM controller device
> >node,
> >>> and let EMAC driver to acquire its EMAC clock regmap.
> >>>
> >>> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >>> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >>> ---
> >>>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23
> >+++
> >>>  1 file changed, 19 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >>> index 1b2ef28c42bd..1c37659d9d41 100644
> >>> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >>> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> >>> @@ -168,10 +168,25 @@
> >>>   #size-cells = <1>;
> >>>   ranges;
> >>>
> >>> - syscon: syscon@1c0 {
> >>> - compatible =
> >"allwinner,sun50i-a64-system-controller",
> >>> - "syscon";
> >>> + sram_controller: sram-controller@1c0 {
> >>> + compatible =
> >"allwinner,sun50i-a64-sram-controller";
> >>
> >> I don't think there's anything preventing us from keeping the
> >> -system-controller compatible. It's what was in the DT before, and
> >> it's how it's called in the datasheet.
> >
> >I actually meant to ask you about this. The -system-controller
> >compatible matches the datasheet better. Maybe we should just
> >switch to that one?
> 
> No, if we do the switch the system-controller compatible,
> the device will be probed on the same memory region with
> a syscon on old DTs.

The device hasn't magically changed either. Maybe we just need to add
a check to make sure we don't have the syscon compatible in the SRAM
driver probe so that the double driver issue doesn't happen?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next v2 15/15] arm64: dts: allwinner: a64: add SRAM controller device tree node

[Maxime Ripard]

Hi,

On Wed, May 02, 2018 at 12:12:27AM +0800, Chen-Yu Tsai wrote:
> From: Icenowy Zheng <icen...@aosc.io>
> 
> Allwinner A64 has a SRAM controller, and in the device tree currently
> we have a syscon node to enable EMAC driver to access the EMAC clock
> register. As SRAM controller driver can now export regmap for this
> register, replace the syscon node to the SRAM controller device node,
> and let EMAC driver to acquire its EMAC clock regmap.
> 
> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi 
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 1b2ef28c42bd..1c37659d9d41 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -168,10 +168,25 @@
>   #size-cells = <1>;
>   ranges;
>  
> - syscon: syscon@1c0 {
> - compatible = "allwinner,sun50i-a64-system-controller",
> - "syscon";
> + sram_controller: sram-controller@1c0 {
> + compatible = "allwinner,sun50i-a64-sram-controller";

I don't think there's anything preventing us from keeping the
-system-controller compatible. It's what was in the DT before, and
it's how it's called in the datasheet.

Otherwise, the whole serie looks good to me:
Acked-by: Maxime Ripard <maxime.rip...@bootlin.com>

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

[Maxime Ripard]

On Mon, Apr 16, 2018 at 10:51:55PM +0800, Chen-Yu Tsai wrote:
> On Mon, Apr 16, 2018 at 10:31 PM, Maxime Ripard
> <maxime.rip...@bootlin.com> wrote:
> > On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote:
> >> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icen...@aosc.io> wrote:
> >> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
> >> > <maxime.rip...@bootlin.com> 写到:
> >> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> >> >>> From: Chen-Yu Tsai <w...@csie.org>
> >> >>>
> >> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU
> >> >>> address space; on the A64 SoC this register is in the SRAM controller
> >> >>> address space, and with a different offset.
> >> >>>
> >> >>> To access the register from another device and hide the internal
> >> >>> difference between the device, let it register a regmap named
> >> >>> "emac-clock". We can then get the device from the phandle, and
> >> >>> retrieve the regmap with dev_get_regmap(); in this situation the
> >> >>> regmap_field will be set up to access the only register in the
> >> >>regmap.
> >> >>>
> >> >>> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >> >>> [Icenowy: change to use regmaps with single register, change commit
> >> >>>  message]
> >> >>> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >> >>> ---
> >> >>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48
> >> >>++-
> >> >>>  1 file changed, 46 insertions(+), 2 deletions(-)
> >> >>>
> >> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >> >>> index 1037f6c78bca..b61210c0d415 100644
> >> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = {
> >> >>>  .msb = 31,
> >> >>>  };
> >> >>>
> >> >>> +/* Specially exported regmap which contains only EMAC register */
> >> >>> +const struct reg_field single_reg_field = {
> >> >>> +.reg = 0,
> >> >>> +.lsb = 0,
> >> >>> +.msb = 31,
> >> >>> +};
> >> >>> +
> >> >>
> >> >>I'm not sure this would be wise. If we ever need some other register
> >> >>exported through the regmap, will have to change all the calling sites
> >> >>everywhere in the kernel, which will be a pain and will break
> >> >>bisectability.
> >> >
> >> > In this situation the register can be exported as another
> >> >  regmap. Currently the code will access a regmap with name
> >> > "emac-clock" for this register.
> >> >
> >> >>
> >> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable
> >> >>hook only allowing a single register seemed like a better one.
> >> >
> >> > But I remember you mentioned that you want it to hide the
> >> > difference inside the device.
> >>
> >> The idea is that a device can export multiple regmaps. This one,
> >> the one named "gmac" (in my soon to come v2) or "emac-clock" here,
> >> is but one of many possible regmaps, and it only exports the register
> >> needed by the GMAC/EMAC.
> >
> > I'm not sure this would be wise either. There's a single register map,
> > and as far as I know we don't have a binding to express this in the
> > DT. This means that the customer and provider would have to use the
> > same name, but without anything actually enforcing it aside from
> > "someone in the community knows it".
> >
> > This is not a really good design, and I was actually preferring your
> > first option. We shouldn't rely on any undocumented rule. This will be
> > easy to break and hard to maintain.
> 
> So, one regmap per device covering the whole register range, and the
> consumer knows which register to poke by looking at its own compatible.
> 
> That sound right?

Yep. And ideally, sending a single serie for both the A64 and the R40
cases, in order to provide the big picture.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [linux-sunxi] Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

[Maxime Ripard]

On Thu, Apr 12, 2018 at 11:23:30PM +0800, Chen-Yu Tsai wrote:
> On Thu, Apr 12, 2018 at 11:11 PM, Icenowy Zheng <icen...@aosc.io> wrote:
> > 于 2018年4月12日 GMT+08:00 下午10:56:28, Maxime Ripard 
> > <maxime.rip...@bootlin.com> 写到:
> >>On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> >>> From: Chen-Yu Tsai <w...@csie.org>
> >>>
> >>> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU
> >>> address space; on the A64 SoC this register is in the SRAM controller
> >>> address space, and with a different offset.
> >>>
> >>> To access the register from another device and hide the internal
> >>> difference between the device, let it register a regmap named
> >>> "emac-clock". We can then get the device from the phandle, and
> >>> retrieve the regmap with dev_get_regmap(); in this situation the
> >>> regmap_field will be set up to access the only register in the
> >>regmap.
> >>>
> >>> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >>> [Icenowy: change to use regmaps with single register, change commit
> >>>  message]
> >>> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >>> ---
> >>>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48
> >>++-
> >>>  1 file changed, 46 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >>b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >>> index 1037f6c78bca..b61210c0d415 100644
> >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> >>> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = {
> >>>  .msb = 31,
> >>>  };
> >>>
> >>> +/* Specially exported regmap which contains only EMAC register */
> >>> +const struct reg_field single_reg_field = {
> >>> +.reg = 0,
> >>> +.lsb = 0,
> >>> +.msb = 31,
> >>> +};
> >>> +
> >>
> >>I'm not sure this would be wise. If we ever need some other register
> >>exported through the regmap, will have to change all the calling sites
> >>everywhere in the kernel, which will be a pain and will break
> >>bisectability.
> >
> > In this situation the register can be exported as another
> >  regmap. Currently the code will access a regmap with name
> > "emac-clock" for this register.
> >
> >>
> >>Chen-Yu's (or was it yours?) initial solution with a custom writeable
> >>hook only allowing a single register seemed like a better one.
> >
> > But I remember you mentioned that you want it to hide the
> > difference inside the device.
> 
> The idea is that a device can export multiple regmaps. This one,
> the one named "gmac" (in my soon to come v2) or "emac-clock" here,
> is but one of many possible regmaps, and it only exports the register
> needed by the GMAC/EMAC.

I'm not sure this would be wise either. There's a single register map,
and as far as I know we don't have a binding to express this in the
DT. This means that the customer and provider would have to use the
same name, but without anything actually enforcing it aside from
"someone in the community knows it".

This is not a really good design, and I was actually preferring your
first option. We shouldn't rely on any undocumented rule. This will be
easy to break and hard to maintain.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 5/5] arm64: allwinner: a64: add SRAM controller device tree node

[Maxime Ripard]

Hi,

On Wed, Apr 11, 2018 at 10:16:41PM +0800, Icenowy Zheng wrote:
> Allwinner A64 has a SRAM controller, and in the device tree currently
> we have a syscon node to enable EMAC driver to access the EMAC clock
> register. As SRAM controller driver can now export regmap for this
> register, replace the syscon node to the SRAM controller device node,
> and let EMAC driver to acquire its EMAC clock regmap.
> 
> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 23 +++
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi 
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 1b2ef28c42bd..1c37659d9d41 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -168,10 +168,25 @@
>   #size-cells = <1>;
>   ranges;
>  
> - syscon: syscon@1c0 {
> - compatible = "allwinner,sun50i-a64-system-controller",
> - "syscon";
> + sram_controller: sram-controller@1c0 {
> + compatible = "allwinner,sun50i-a64-sram-controller";
>   reg = <0x01c0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + sram_c: sram@18000 {
> + compatible = "mmio-sram";
> + reg = <0x00018000 0x28000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0 0x00018000 0x28000>;
> +
> + de2_sram: sram-section@0 {
> + compatible = 
> "allwinner,sun50i-a64-sram-c";
> + reg = <0x 0x28000>;
> + };
> +     };

That doesn't look related at all to what's being discussed here, so
you'd rather add it as part of your DE2-enablement serie (or amend
your commit log to say why this is important to do it in this patch).

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 3/5] net: stmmac: dwmac-sun8i: Allow getting syscon regmap from device

[Maxime Ripard]

On Wed, Apr 11, 2018 at 10:16:39PM +0800, Icenowy Zheng wrote:
> From: Chen-Yu Tsai <w...@csie.org>
> 
> On the Allwinner R40 SoC, the "GMAC clock" register is in the CCU
> address space; on the A64 SoC this register is in the SRAM controller
> address space, and with a different offset.
> 
> To access the register from another device and hide the internal
> difference between the device, let it register a regmap named
> "emac-clock". We can then get the device from the phandle, and
> retrieve the regmap with dev_get_regmap(); in this situation the
> regmap_field will be set up to access the only register in the regmap.
> 
> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> [Icenowy: change to use regmaps with single register, change commit
>  message]
> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 48 
> ++-
>  1 file changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 1037f6c78bca..b61210c0d415 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -85,6 +85,13 @@ const struct reg_field old_syscon_reg_field = {
>   .msb = 31,
>  };
>  
> +/* Specially exported regmap which contains only EMAC register */
> +const struct reg_field single_reg_field = {
> + .reg = 0,
> + .lsb = 0,
> + .msb = 31,
> +};
> +

I'm not sure this would be wise. If we ever need some other register
exported through the regmap, will have to change all the calling sites
everywhere in the kernel, which will be a pain and will break
bisectability.

Chen-Yu's (or was it yours?) initial solution with a custom writeable
hook only allowing a single register seemed like a better one.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

On Tue, Apr 03, 2018 at 05:58:05PM +0800, Chen-Yu Tsai wrote:
> On Tue, Apr 3, 2018 at 5:54 PM, Icenowy Zheng <icen...@aosc.io> wrote:
> >
> >
> > 于 2018年4月3日 GMT+08:00 下午5:53:08, Chen-Yu Tsai <w...@csie.org> 写到:
> >>On Tue, Apr 3, 2018 at 5:50 PM, Maxime Ripard
> >><maxime.rip...@bootlin.com> wrote:
> >>> On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
> >>>> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> >>>> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> >>>> > <maxime.rip...@bootlin.com> wrote:
> >>>> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> >>>> > >> From: Icenowy Zheng <icen...@aosc.io>
> >>>> > >>
> >>>> > >> There's a GMAC configuration register, which exists on
> >>A64/A83T/H3/H5 in
> >>>> > >> the syscon part, in the CCU of R40 SoC.
> >>>> > >>
> >>>> > >> Export a regmap of the CCU.
> >>>> > >>
> >>>> > >> Read access is not restricted to all registers, but only the
> >>GMAC
> >>>> > >> register is allowed to be written.
> >>>> > >>
> >>>> > >> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >>>> > >> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >>>> > >
> >>>> > > Gah, this is crazy. I'm really starting to regret letting that
> >>syscon
> >>>> > > in in the first place...
> >>>> >
> >>>> > IMHO syscon is really a better fit. It's part of the glue layer
> >>and
> >>>> > most other dwmac user platforms treat it as such and use a syscon.
> >>>> > Plus the controls encompass delays (phase), inverters (polarity),
> >>>> > and even signal routing. It's not really just a group of clock
> >>controls,
> >>>> > like what we poorly modeled for A20/A31. I think that was really a
> >>>> > mistake.
> >>>> >
> >>>> > As I mentioned in the cover letter, a slightly saner approach
> >>would
> >>>> > be to let drivers add custom syscon entries, which would then
> >>require
> >>>> > less custom plumbing.
> >>>>
> >>>> A syscon is convenient, sure, but it also bypasses any abstraction
> >>>> layer we have everywhere else, which means that we'll have to
> >>maintain
> >>>> the register layout in each and every driver that uses it.
> >>>>
> >>>> So far, it's only be the GMAC, but it can also be others (the SRAM
> >>>> controller comes to my mind), and then, if there's any difference in
> >>>> the design in a future SoC, we'll have to maintain that in the GMAC
> >>>> driver as well.
> >>>
> >>> I guess I forgot to say something, I'm fine with using a syscon we
> >>> already have.
> >>>
> >>> I'm just questionning if merging any other driver using one is the
> >>> right move.
> >>
> >>Right. So in this case, we are not actually going through the syscon
> >>API. Rather we are exporting a regmap whose properties we actually
> >>define. If it makes you more acceptable to it, we could map just
> >>the GMAC register in the new regmap, and also have it named. This
> >>is all plumbing within the kernel so the device tree stays the same.
> >
> > I think my driver has already restricted the write permission
> > only to GMAC register.
> 
> Correct, but it still maps the entire region out, which means the
> consumer needs to know which offset to use. Maxime is saying this
> is something that is troublesome to maintain. So my proposal was
> to create a regmap with a base at the GMAC register offset. That
> way, the consumer doesn't need to use an offset to access it.

I guess this is something we can keep in mind if it gets out of
control yse.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

On Tue, Apr 03, 2018 at 11:48:45AM +0200, Maxime Ripard wrote:
> On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> > On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> > <maxime.rip...@bootlin.com> wrote:
> > > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> > >> From: Icenowy Zheng <icen...@aosc.io>
> > >>
> > >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> > >> the syscon part, in the CCU of R40 SoC.
> > >>
> > >> Export a regmap of the CCU.
> > >>
> > >> Read access is not restricted to all registers, but only the GMAC
> > >> register is allowed to be written.
> > >>
> > >> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> > >> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> > >
> > > Gah, this is crazy. I'm really starting to regret letting that syscon
> > > in in the first place...
> > 
> > IMHO syscon is really a better fit. It's part of the glue layer and
> > most other dwmac user platforms treat it as such and use a syscon.
> > Plus the controls encompass delays (phase), inverters (polarity),
> > and even signal routing. It's not really just a group of clock controls,
> > like what we poorly modeled for A20/A31. I think that was really a
> > mistake.
> > 
> > As I mentioned in the cover letter, a slightly saner approach would
> > be to let drivers add custom syscon entries, which would then require
> > less custom plumbing.
> 
> A syscon is convenient, sure, but it also bypasses any abstraction
> layer we have everywhere else, which means that we'll have to maintain
> the register layout in each and every driver that uses it.
> 
> So far, it's only be the GMAC, but it can also be others (the SRAM
> controller comes to my mind), and then, if there's any difference in
> the design in a future SoC, we'll have to maintain that in the GMAC
> driver as well.

I guess I forgot to say something, I'm fine with using a syscon we
already have.

I'm just questionning if merging any other driver using one is the
right move.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

On Tue, Mar 20, 2018 at 03:15:02PM +0800, Chen-Yu Tsai wrote:
> On Mon, Mar 19, 2018 at 5:31 AM, Maxime Ripard
> <maxime.rip...@bootlin.com> wrote:
> > On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> >> From: Icenowy Zheng <icen...@aosc.io>
> >>
> >> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> >> the syscon part, in the CCU of R40 SoC.
> >>
> >> Export a regmap of the CCU.
> >>
> >> Read access is not restricted to all registers, but only the GMAC
> >> register is allowed to be written.
> >>
> >> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >> Signed-off-by: Chen-Yu Tsai <w...@csie.org>
> >
> > Gah, this is crazy. I'm really starting to regret letting that syscon
> > in in the first place...
> 
> IMHO syscon is really a better fit. It's part of the glue layer and
> most other dwmac user platforms treat it as such and use a syscon.
> Plus the controls encompass delays (phase), inverters (polarity),
> and even signal routing. It's not really just a group of clock controls,
> like what we poorly modeled for A20/A31. I think that was really a
> mistake.
> 
> As I mentioned in the cover letter, a slightly saner approach would
> be to let drivers add custom syscon entries, which would then require
> less custom plumbing.

A syscon is convenient, sure, but it also bypasses any abstraction
layer we have everywhere else, which means that we'll have to maintain
the register layout in each and every driver that uses it.

So far, it's only be the GMAC, but it can also be others (the SRAM
controller comes to my mind), and then, if there's any difference in
the design in a future SoC, we'll have to maintain that in the GMAC
driver as well.

> > And I'm not really looking forward the time where SCPI et al. will be
> > mature and we'll have the clock controller completely outside of our
> > control.
> 
> I don't think it's going to happen for any of the older SoCs. The R40
> only stands out because the GMAC controls are in the clock controller
> address space, presumably to be like the A20.

SCPI (or equivalent) is a really nice feature to have when it comes to
virtualization, so even if it's less likely, it doesn't make it less
relevant on other SoCs.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH net-next 02/12] clk: sunxi-ng: r40: export a regmap to access the GMAC register

[Maxime Ripard]

On Sat, Mar 17, 2018 at 05:28:47PM +0800, Chen-Yu Tsai wrote:
> From: Icenowy Zheng <icen...@aosc.io>
> 
> There's a GMAC configuration register, which exists on A64/A83T/H3/H5 in
> the syscon part, in the CCU of R40 SoC.
> 
> Export a regmap of the CCU.
> 
> Read access is not restricted to all registers, but only the GMAC
> register is allowed to be written.
> 
> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> Signed-off-by: Chen-Yu Tsai <w...@csie.org>

Gah, this is crazy. I'm really starting to regret letting that syscon
in in the first place...

And I'm not really looking forward the time where SCPI et al. will be
mature and we'll have the clock controller completely outside of our
control.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 1/3] net: stmmac: dwmac-sun8i: drop V3s compatible and add V3 one

[Maxime Ripard]

Hi,

On Sat, Feb 03, 2018 at 03:23:28PM +0800, Icenowy Zheng wrote:
> 于 2018年2月3日 GMT+08:00 上午6:13:01, Maxime Ripard <maxime.rip...@bootlin.com> 写到:
> >On Sat, Feb 03, 2018 at 02:04:54AM +0800, Icenowy Zheng wrote:
> >> The V3s is just a differently packaged version of the V3 chip, which
> >has
> >> a MAC with the same capability with H3. The V3s just doesn't wire out
> >> the external MII/RMII/RGMII bus. (V3 wired out it).
> >> 
> >> Drop the compatible string of V3s in the dwmac-sun8i driver, and add
> >a
> >> V3 compatible string, which has all capabilities.
> >> 
> >> Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> >
> >This breaks the DT ABI, so NAK.
> 
> I have asked this at IRC.

One more reason why no one should ask questions like this on IRC.

> The V3s compatible string is never used in any mainline
> kernel, even not in any RC version.

$ git grep allwinner,sun8i-v3s-emac v4.15 | wc -l 
5

It is there already, and the fact that we have or don't have a DT in
tree that use it doesn't matter. One could very well have written a DT
for it and never submitted it.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH 1/3] net: stmmac: dwmac-sun8i: drop V3s compatible and add V3 one

[Maxime Ripard]

On Sat, Feb 03, 2018 at 02:04:54AM +0800, Icenowy Zheng wrote:
> The V3s is just a differently packaged version of the V3 chip, which has
> a MAC with the same capability with H3. The V3s just doesn't wire out
> the external MII/RMII/RGMII bus. (V3 wired out it).
> 
> Drop the compatible string of V3s in the dwmac-sun8i driver, and add a
> V3 compatible string, which has all capabilities.
> 
> Signed-off-by: Icenowy Zheng <icen...@aosc.io>

This breaks the DT ABI, so NAK.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com


signature.asc
Description: PGP signature

Re: [PATCH] net: stmmac: dwmac-sun8i: fix allwinner,leds-active-low handling

[Maxime Ripard]

On Wed, Nov 29, 2017 at 04:37:12PM +0100, Andrew Lunn wrote:
> On Wed, Nov 29, 2017 at 10:02:40AM +0100, Corentin Labbe wrote:
> > On Tue, Nov 28, 2017 at 06:38:26PM +0100, Andrew Lunn wrote:
> > > On Tue, Nov 28, 2017 at 05:48:22PM +0100, Corentin Labbe wrote:
> > > > The driver expect "allwinner,leds-active-low" to be in PHY node, but
> > > > the binding doc expect it to be in MAC node.
> > > > 
> > > > Since all board DT use it also in MAC node, the driver need to search
> > > > allwinner,leds-active-low in MAC node.
> > > 
> > > Hi Corentin
> > > 
> > > I'm having trouble working out how this worked before. This is code
> > > you moved around, when adding external/internal MDIOs. But the very
> > > first version of this driver code used priv->plat->phy_node. Did that
> > > somehow point to the MAC node when the internal PHY is used? Or has it
> > > been broken all the time?
> > > 
> > 
> > Hello
> > 
> 
> > Since this feature control only when the activity LED need to blink,
> > nobody see that it was broken.
> 
> Hi Corentin
> 
> So it never worked?
> 
> If it never worked, moving the DT properties into the PHY node, where
> they belong, won't introduce a regression :-)

That's even truer since it's been queued for 4.15 which hasn't been
released yet.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v10 0/8] net: stmmac: dwmac-sun8i: Handle integrated PHY

[Maxime Ripard]

On Tue, Oct 31, 2017 at 09:19:07AM +0100, Corentin Labbe wrote:
> Hello
> 
> The current way to find if the PHY is internal is to compare DT phy-mode
> and emac_variant/internal_phy.
> But it will negate a possible future SoC where an external PHY use the
> same phy mode than the integrated one.
> 
> This patchs series adds a new way to handle this problem via a mdio-mux.
> 
> The first try was to create a new MDIO mux "mdio-mux-syscon".
> mdio-mux-syscon working the same way than mdio-mux-mmioreg with the exception
> that the register is used via syscon/regmap.
> But this solution does not work for two reason:
> - changing the MDIO selection need the reset of MAC which cannot be done by 
> the
> mdio-mux-syscon driver
> - There were driver loading order problem:
> - mdio-mux-syscon needing that stmmac register the parent MDIO
> - stmmac needing that child MDIO was registered just after 
> registering parent MDIO
> 
> So we cannot use any external MDIO-mux.
> 
> The final solution was to represent the mdio-mux in MAC node and let
> the MAC handle all things.
> 
> Since DT bits was reverted in 4.13, this patch series include the
> revert of the revert.
> 
> I have let patch splited for easy review. (for seeing what's new)
> But the final serie could have some patch squashed if someone want.
> Like squashing patch and 1 and 2 (documentation)

Applied the patches, and I'll send a PR for it tomorrow after one
linux-next run.

Hopefully it will be merged in time for 4.15.

Thanks for your persistence,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v9 04/10] arm: dts: sunxi: h3/h5: represent the mdio switch used by sun8i-h3-emac

[Maxime Ripard]

Hi,

The prefix should be ARM, uppercase.

On Tue, Oct 24, 2017 at 07:57:08PM +0200, Corentin Labbe wrote:
> Since dwmac-sun8i could use either an integrated PHY or an external PHY
> (which could be at same MDIO address), we need to represent this selection
> by a MDIO switch.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 32 +++-
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi 
> b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index d762098fc589..0e97df490aba 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -422,14 +422,36 @@
>   #size-cells = <0>;
>   status = "disabled";
>  
> - mdio: mdio {
> + mdio0: mdio {
>   #address-cells = <1>;
>   #size-cells = <0>;
> - int_mii_phy: ethernet-phy@1 {
> - compatible = 
> "ethernet-phy-ieee802.3-c22";
> + compatible = "snps,dwmac-mdio";
> + };
> +
> + mdio-mux {
> + compatible = "allwinner,sun8i-h3-mdio-mux";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + mdio-parent-bus = <>;

And you should have a line here.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v9 06/10] arm64: dts: allwinner: Restore EMAC changes

[Maxime Ripard]

On Tue, Oct 24, 2017 at 07:57:10PM +0200, Corentin Labbe wrote:
> The original dwmac-sun8i DT bindings have some issue on how to handle
> integrated PHY and was reverted in last RC of 4.13.
> But now we have a solution so we need to get back that was reverted.
> 
> This patch restore arm64 DT about dwmac-sun8i
> This reverts commit 87e1f5e8bb4b ("arm64: dts: allwinner: Revert EMAC 
> changes")
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts   | 16 
>  .../boot/dts/allwinner/sun50i-a64-pine64-plus.dts| 15 +++
>  arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts  | 17 +
>  .../dts/allwinner/sun50i-a64-sopine-baseboard.dts| 16 
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi| 20 
> 
>  .../boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts | 17 +
>  .../boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts| 17 +
>  .../boot/dts/allwinner/sun50i-h5-orangepi-prime.dts  | 17 +
>  8 files changed, 135 insertions(+)

Can you split the changes between the A64 and the H5? It's going to be
difficult to merge otherwise.

(You also forgot to add Florian's Acked-by on your whole serie).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v7 02/10] arm: dts: sunxi: Restore EMAC changes

[Maxime Ripard]

On Wed, Oct 18, 2017 at 08:50:49PM +0200, Corentin Labbe wrote:
> On Wed, Oct 18, 2017 at 06:44:50PM +0200, Andrew Lunn wrote:
> > On Wed, Oct 18, 2017 at 01:44:50PM +0200, Corentin Labbe wrote:
> > > The original dwmac-sun8i DT bindings have some issue on how to handle
> > > integrated PHY and was reverted in last RC of 4.13.
> > > But now we have a solution so we need to get back that was reverted.
> > > 
> > > This patch restore arm DT about dwmac-sun8i
> > > This reverts commit fe45174b72ae ("arm: dts: sunxi: Revert EMAC changes")
> > > 
> > > Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> > > ---
> > >  arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts |  9 
> > >  arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts   | 19 +
> > >  arch/arm/boot/dts/sun8i-h3-nanopi-m1-plus.dts | 19 +
> > >  arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts |  7 ++
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-2.dts |  8 +++
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-one.dts   |  8 +++
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts   |  5 +
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts|  8 +++
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts  | 22 
> > > +++
> > >  arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts| 16 ++
> > >  arch/arm/boot/dts/sunxi-h3-h5.dtsi| 26 
> > > +++
> > >  11 files changed, 147 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts 
> > > b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > index b1502df7b509..6713d0f2b3f4 100644
> > > --- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
> > > @@ -56,6 +56,8 @@
> > >  
> > >   aliases {
> > >   serial0 = 
> > > + /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
> > > + ethernet0 = 
> > >   ethernet1 = 
> > >   };
> > >  
> > > @@ -102,6 +104,13 @@
> > >   status = "okay";
> > >  };
> > >  
> > > + {
> > > + phy-handle = <_mii_phy>;
> > > + phy-mode = "mii";
> > > + allwinner,leds-active-low;
> > > + status = "okay";
> > > +};
> > > +
> > >   {
> > >   pinctrl-names = "default";
> > >   pinctrl-0 = <_pins_a>;
> > > diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts 
> > > b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > > index e1dba9ffa94b..f2292deaa590 100644
> > > --- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > > +++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
> > > @@ -52,6 +52,7 @@
> > >   compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
> > >  
> > >   aliases {
> > > + ethernet0 = 
> > >   serial0 = 
> > >   serial1 = 
> > >   };
> > > @@ -111,6 +112,24 @@
> > >   status = "okay";
> > >  };
> > >  
> > > + {
> > > + pinctrl-names = "default";
> > > + pinctrl-0 = <_rgmii_pins>;
> > > + phy-supply = <_gmac_3v3>;
> > > + phy-handle = <_rgmii_phy>;
> > > + phy-mode = "rgmii";
> > > +
> > > + allwinner,leds-active-low;
> > > + status = "okay";
> > > +};
> > > +
> > 
> > 
> > > +_mdio {
> > > + ext_rgmii_phy: ethernet-phy@1 {
> > > + compatible = "ethernet-phy-ieee802.3-c22";
> > > + reg = <0>;
> > > + };
> > > +};
> > > +
> > 
> > Hi Corentin
> > 
> > I'm wondering about the order of the patches. Does the external_mdio
> > node actually exist at this point? Or only later when other patches
> > are applied?
> > 
> 
> You are right order of patch are wrong, I need to cut this one in two.
> "Revert²" sunxi-h3-h5.dtsi
> apply mdiomux
> "Revert²" all board nodes

I'm not even sure why you're actually adding them that way. Can't you
just create the new binding file, support it in the driver, and add
the matching DT nodes ?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 05/11] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY

[Maxime Ripard]

On Sun, Oct 08, 2017 at 06:33:40PM +, Corentin Labbe wrote:
> On Thu, Sep 28, 2017 at 09:37:08AM +0200, Corentin Labbe wrote:
> > On Wed, Sep 27, 2017 at 04:02:10PM +0200, Andrew Lunn wrote:
> > > Hi Corentin
> > > 
> > > > +Required properties for the mdio-mux node:
> > > > +  - compatible = "mdio-mux"
> > > 
> > > This is too generic. Please add a more specific compatible for this
> > > particular mux. You can keep "mdio-mux", since that is what the MDIO
> > > subsystem will look for.
> > > 
> > 
> > I will add allwinner,sun8i-h3-mdio-mux
> > 
> > > > +Required properties of the integrated phy node:
> > > >  - clocks: a phandle to the reference clock for the EPHY
> > > >  - resets: a phandle to the reset control for the EPHY
> > > > +- phy-is-integrated
> > > 
> > > So the last thing you said is that the mux is not the problem
> > > here. Something else is locking up. Did you discover what?
> > > 
> > > I really would like phy-is-integrated to go away.
> > > 
> > 
> > I have found the problem: by enabling ephy clk/reset the timeout does not 
> > occur anymore.
> > So we could remove phy-is-integrated by:
> > Moving internal phy clk/reset handling in mdio_mux_syscon_switch_fn()
> > But this means:
> > - getting internalphy node always by manually get 
> > internal_mdio/internal_phy (and not by the given phyhandle)
> > - doing some unnecessary tasks (enable/scan/disable) when external_phy is 
> > needed
> > 
> > Regards
> 
> Hello all
> 
> Below is the current patch, as you can read, it does not use anymore the 
> phy-is-integrated property.
> So now, the mdio-mux must always enable the internal mdio when switch_fn ask 
> for it and so reset MAC and so need to enable ephy clk/reset.
> But for this I need a reference to thoses clock and reset. (this is done in 
> get_ephy_nodes)
> The current version set those clock in mdio-mux node, and as you can see it 
> is already ugly (lots of get next node),
> if the clk/rst nodes were as it should be, in phy nodes, it will be more bad.
> 
> So, since the MAC have a dependency on thoses clk/rst nodes for
> doing reset(), I seek a proper way to get references on it.
>
> OR do you agree that putting ephy clk/rst in emac is acceptable ?

Why not just parsing the DT child nodes looking for resets and clocks
properties? The usual PHY don't have that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] dt-bindings: add device tree binding for Allwinner XR819 SDIO Wi-Fi

[Maxime Ripard]

On Wed, Oct 04, 2017 at 10:02:48AM +, Arend van Spriel wrote:
> On 10/4/2017 11:03 AM, Icenowy Zheng wrote:
> > 
> > 
> > 于 2017年10月4日 GMT+08:00 下午5:02:17, Kalle Valo <kv...@codeaurora.org> 写到:
> > > Icenowy Zheng <icen...@aosc.io> writes:
> > > 
> > > > Allwinner XR819 is a SDIO Wi-Fi chip, which has the functionality to
> > > use
> > > > an out-of-band interrupt pin instead of SDIO in-band interrupt.
> > > > 
> > > > Add the device tree binding of this chip, in order to make it
> > > possible
> > > > to add this interrupt pin to device trees.
> > > > 
> > > > Signed-off-by: Icenowy Zheng <icen...@aosc.io>
> > > > Acked-by: Rob Herring <r...@kernel.org>
> > > > ---
> > > > Changes in v3:
> > > > - Renames the node name.
> > > > - Adds ACK from Rob.
> > > > Changes in v2:
> > > > - Removed status property in example.
> > > > - Added required property reg.
> > > > 
> > > >   .../bindings/net/wireless/allwinner,xr819.txt  | 38
> > > ++
> > > >   1 file changed, 38 insertions(+)
> > > >   create mode 100644
> > > Documentation/devicetree/bindings/net/wireless/allwinner,xr819.txt
> > > 
> > > Like I asked already last time, AFAICS there is no upstream xr819
> > > wireless driver in drivers/net/wireless directory. Do we still accept
> > > bindings like this for out-of-tree drivers?
> > 
> > See esp8089.
> > 
> > There's also no in-tree driver for it.
> 
> The question is whether we should. The above might be a precedent, but it
> may not necessarily be the way to go. The commit message for esp8089 seems
> to hint that there is intent to have an in-tree driver:
> 
> """
> Note that at this point there only is an out of tree driver for this
> hardware, there is no clear timeline / path for merging this. Still
> I believe it would be good to specify the binding for this in tree
> now, so that any future migration to an in tree driver will not cause
> compatiblity issues.
> 
> Cc: Icenowy Zheng <icen...@aosc.xyz>
> Signed-off-by: Hans de Goede <hdego...@redhat.com>
> Signed-off-by: Rob Herring <r...@kernel.org>
> """
> 
> Regardless the bindings are in principle independent of the kernel and just
> describing hardware. I think there have been discussions to move the
> bindings to their own repository, but apparently it was decided otherwise.

Yeah, I guess especially how it could be merged with the cw1200 driver
would be very relevant to that commit log.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 05/11] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY

[Maxime Ripard]

Hi,

On Wed, Sep 27, 2017 at 07:34:08AM +, Corentin Labbe wrote:
> This patch add documentation about the MDIO switch used on sun8i-h3-emac
> for integrated PHY.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>

This should be squashed with patch 1.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 06/11] ARM: dts: sunxi: h3/h5: represent the mdio switch used by sun8i-h3-emac

[Maxime Ripard]

On Wed, Sep 27, 2017 at 07:34:09AM +, Corentin Labbe wrote:
> Since dwmac-sun8i could use either an integrated PHY or an external PHY
> (which could be at same MDIO address), we need to represent this selection
> by a MDIO switch.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sunxi-h3-h5.dtsi | 31 +--
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/sunxi-h3-h5.dtsi 
> b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> index 3b7d953429a6..a8e9b8f378ba 100644
> --- a/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> +++ b/arch/arm/boot/dts/sunxi-h3-h5.dtsi
> @@ -422,14 +422,33 @@
>   #size-cells = <0>;
>   status = "disabled";
>  
> - mdio: mdio {
> + mdio0: mdio {
>   #address-cells = <1>;
>   #size-cells = <0>;
> - int_mii_phy: ethernet-phy@1 {
> - compatible = 
> "ethernet-phy-ieee802.3-c22";
> - reg = <1>;
> - clocks = < CLK_BUS_EPHY>;
> - resets = < RST_BUS_EPHY>;
> + compatible = "snps,dwmac-mdio";
> +
> + mdio-mux {
> + compatible = "mdio-mux";
> + #address-cells = <1>;
> + #size-cells = <0>;

Newline

> + /* Only one MDIO is usable at the time 
> */
> + internal_mdio: mdio@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;

Newline

> + int_mii_phy: ethernet-phy@1 {
> + compatible = 
> "ethernet-phy-ieee802.3-c22";
> + reg = <1>;
> + clocks = < 
> CLK_BUS_EPHY>;
> + resets = < 
> RST_BUS_EPHY>;
> +         phy-is-integrated;
> + };
> + };

Newline

> + mdio: mdio@2 {

This is quite confusing. Why not call the label external_mdio?

Thanks

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2] net: stmmac: dwmac-sun8i: Use reset exclusive

[Maxime Ripard]

On Mon, Sep 18, 2017 at 08:30:43PM +0200, Corentin Labbe wrote:
> The current dwmac_sun8i module cannot be rmmod/modprobe due to that
> the reset controller was not released when removed.
> 
> This patch remove ambiguity, by using of_reset_control_get_exclusive and
> add the missing reset_control_put().
> 
> Note that we cannot use devm_reset_control_get, since the reset is not
> in the device node.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
> Changes since v1:
> - added a note about devm_reset_control_get in commit message

That comment would be better if it was in the code.

> 
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index 57bb6dd7b401..1736d7cb0d96 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -854,6 +854,7 @@ static int sun8i_dwmac_unpower_internal_phy(struct 
> sunxi_priv_data *gmac)
>  
>   clk_disable_unprepare(gmac->ephy_clk);
>   reset_control_assert(gmac->rst_ephy);
> + reset_control_put(gmac->rst_ephy);

Putting it here is weird.

What would happen if power_phy / unpower_phy is called several times?

Can't we just make it symetric and undo in remove what we do in probe?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v5 05/10] dt-bindings: net: dwmac-sun8i: update documentation about integrated PHY

[Maxime Ripard]

 #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "snps,dwmac-mdio";
> + };
> +
> + mdio-mux {
> + compatible = "mdio-mux";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + int_mdio: mdio@1 {
> + reg = <0>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + int_mii_phy: ethernet-phy@1 {
> + reg = <1>;
> + clocks = < CLK_BUS_EPHY>;
> + resets = < RST_BUS_EPHY>;
> + phy-is-integrated
> + };
> + };
> + ext_mdio: mdio@0 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + ext_rgmii_phy: ethernet-phy@1 {
> + reg = <1>;
> + };
> + };
> + };
> +};
> +
> +Example with SoC without integrated PHY
> +
> +emac: ethernet@1c0b000 {
> + compatible = "allwinner,sun8i-a83t-emac";
> + syscon = <>;
> + reg = <0x01c0b000 0x104>;
> + interrupts = ;
> + interrupt-names = "macirq";
> + resets = < RST_BUS_EMAC>;
> + reset-names = "stmmaceth";
> + clocks = < CLK_BUS_EMAC>;
> + clock-names = "stmmaceth";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> +
>   mdio: mdio {
> + compatible = "snps,dwmac-mdio";
>   #address-cells = <1>;
>   #size-cells = <0>;
> - int_mii_phy: ethernet-phy@1 {
> + ext_rgmii_phy: ethernet-phy@1 {
>   reg = <1>;
> - clocks = < CLK_BUS_EPHY>;
> - resets = < RST_BUS_EPHY>;
>   };
>   };
>  };
> -- 
> 2.13.5
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v5 01/10] arm64: dts: allwinner: Restore EMAC changes

[Maxime Ripard]

On Fri, Sep 08, 2017 at 09:11:47AM +0200, Corentin Labbe wrote:
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts 
> b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts
> index 1c2387bd5df6..968908761194 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts
> @@ -50,6 +50,7 @@
>   compatible = "friendlyarm,nanopi-neo2", "allwinner,sun50i-h5";
>  
>   aliases {
> + ethernet0 = 
>   serial0 = 
>   };
>  
> @@ -108,6 +109,22 @@
>   status = "okay";
>  };
>  
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_rgmii_pins>;
> + phy-supply = <_gmac_3v3>;
> + phy-handle = <_rgmii_phy>;
> + phy-mode = "rgmii";
> + status = "okay";
> +};
> +
> + {
> + ext_rgmii_phy: ethernet-phy@7 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + reg = <7>;
> + };
> +};
> +

This won't compile, you don't have that node in the H5 DTSI.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

[PATCH 1/4] dt-bindings: net: Revert sun8i dwmac binding

[Maxime Ripard]

This binding still doesn't please everyone, and we're getting far too
close from the release to allow it to reach a stable version.

Let's remove it until the discussion settles down.

Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
 .../devicetree/bindings/net/dwmac-sun8i.txt| 84 --
 1 file changed, 84 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/dwmac-sun8i.txt

diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt 
b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
deleted file mode 100644
index 725f3b187886..
--- a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
+++ /dev/null
@@ -1,84 +0,0 @@
-* Allwinner sun8i GMAC ethernet controller
-
-This device is a platform glue layer for stmmac.
-Please see stmmac.txt for the other unchanged properties.
-
-Required properties:
-- compatible: should be one of the following string:
-   "allwinner,sun8i-a83t-emac"
-   "allwinner,sun8i-h3-emac"
-   "allwinner,sun8i-v3s-emac"
-   "allwinner,sun50i-a64-emac"
-- reg: address and length of the register for the device.
-- interrupts: interrupt for the device
-- interrupt-names: should be "macirq"
-- clocks: A phandle to the reference clock for this device
-- clock-names: should be "stmmaceth"
-- resets: A phandle to the reset control for this device
-- reset-names: should be "stmmaceth"
-- phy-mode: See ethernet.txt
-- phy-handle: See ethernet.txt
-- #address-cells: shall be 1
-- #size-cells: shall be 0
-- syscon: A phandle to the syscon of the SoC with one of the following
- compatible string:
-  - allwinner,sun8i-h3-system-controller
-  - allwinner,sun8i-v3s-system-controller
-  - allwinner,sun50i-a64-system-controller
-  - allwinner,sun8i-a83t-system-controller
-
-Optional properties:
-- allwinner,tx-delay-ps: TX clock delay chain value in ps. Range value is 
0-700. Default is 0)
-- allwinner,rx-delay-ps: RX clock delay chain value in ps. Range value is 
0-3100. Default is 0)
-Both delay properties need to be a multiple of 100. They control the delay for
-external PHY.
-
-Optional properties for the following compatibles:
-  - "allwinner,sun8i-h3-emac",
-  - "allwinner,sun8i-v3s-emac":
-- allwinner,leds-active-low: EPHY LEDs are active low
-
-Required child node of emac:
-- mdio bus node: should be named mdio
-
-Required properties of the mdio node:
-- #address-cells: shall be 1
-- #size-cells: shall be 0
-
-The device node referenced by "phy" or "phy-handle" should be a child node
-of the mdio node. See phy.txt for the generic PHY bindings.
-
-Required properties of the phy node with the following compatibles:
-  - "allwinner,sun8i-h3-emac",
-  - "allwinner,sun8i-v3s-emac":
-- clocks: a phandle to the reference clock for the EPHY
-- resets: a phandle to the reset control for the EPHY
-
-Example:
-
-emac: ethernet@1c0b000 {
-   compatible = "allwinner,sun8i-h3-emac";
-   syscon = <>;
-   reg = <0x01c0b000 0x104>;
-   interrupts = ;
-   interrupt-names = "macirq";
-   resets = < RST_BUS_EMAC>;
-   reset-names = "stmmaceth";
-   clocks = < CLK_BUS_EMAC>;
-   clock-names = "stmmaceth";
-   #address-cells = <1>;
-   #size-cells = <0>;
-
-   phy-handle = <_mii_phy>;
-   phy-mode = "mii";
-   allwinner,leds-active-low;
-   mdio: mdio {
-   #address-cells = <1>;
-   #size-cells = <0>;
-   int_mii_phy: ethernet-phy@1 {
-   reg = <1>;
-   clocks = < CLK_BUS_EPHY>;
-   resets = < RST_BUS_EPHY>;
-   };
-   };
-};
-- 
2.13.5

[PATCH 0/4] net: stmmac: revert the EMAC bindings

[Maxime Ripard]

Hi,

The bindings of the stmmac glue for the new Allwinner EMAC controller
are still controversial and being discussed, even though they've been
merged in 4.13.

In order not to introduce any binding we do not really want to commit
to in a stable release, especially since that would mean we would have
to support both the right and old bindings, let's revert them.

We will reintroduce them in due time, once the discussion has settled
down.

The first three patches should go through the arm-soc tree, the last
one through the net tree. All of them must be treated as fixes.

Thanks!
Maxime

Maxime Ripard (4):
  dt-bindings: net: Revert sun8i dwmac binding
  arm64: dts: allwinner: Revert EMAC changes
  arm: dts: sunxi: Revert EMAC changes
  net: stmmac: sun8i: Remove the compatibles

 .../devicetree/bindings/net/dwmac-sun8i.txt| 84 --
 arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts  |  9 ---
 arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts| 19 -
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts  |  8 ---
 arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts  |  7 --
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts  |  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-one.dts|  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts|  5 --
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts |  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts   | 22 --
 arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts | 16 -
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 26 ---
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 17 -
 .../boot/dts/allwinner/sun50i-a64-pine64-plus.dts  | 15 
 .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 18 -
 .../dts/allwinner/sun50i-a64-sopine-baseboard.dts  | 17 -
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi  | 20 --
 .../boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts   | 17 -
 .../boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts  | 17 -
 .../dts/allwinner/sun50i-h5-orangepi-prime.dts | 17 -
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c  |  8 ---
 21 files changed, 366 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/dwmac-sun8i.txt

-- 
2.13.5

[PATCH 2/4] arm64: dts: allwinner: Revert EMAC changes

[Maxime Ripard]

Since the discussion is not settled yet for the EMAC, and that the release
in getting really close, let's revert the changes for now, and we'll
reintroduce them later.

Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts   | 17 -
 .../boot/dts/allwinner/sun50i-a64-pine64-plus.dts| 15 ---
 arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts  | 18 --
 .../dts/allwinner/sun50i-a64-sopine-baseboard.dts| 17 -
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi| 20 
 .../boot/dts/allwinner/sun50i-h5-nanopi-neo2.dts | 17 -
 .../boot/dts/allwinner/sun50i-h5-orangepi-pc2.dts| 17 -
 .../boot/dts/allwinner/sun50i-h5-orangepi-prime.dts  | 17 -
 8 files changed, 138 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts 
b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
index 4a8d3f83a36e..d347f52e27f6 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-bananapi-m64.dts
@@ -51,7 +51,6 @@
compatible = "sinovoip,bananapi-m64", "allwinner,sun50i-a64";
 
aliases {
-   ethernet0 = 
serial0 = 
serial1 = 
};
@@ -70,15 +69,6 @@
status = "okay";
 };
 
- {
-   pinctrl-names = "default";
-   pinctrl-0 = <_pins>;
-   phy-mode = "rgmii";
-   phy-handle = <_rgmii_phy>;
-   phy-supply = <_dc1sw>;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
@@ -89,13 +79,6 @@
bias-pull-up;
 };
 
- {
-   ext_rgmii_phy: ethernet-phy@1 {
-   compatible = "ethernet-phy-ieee802.3-c22";
-   reg = <1>;
-   };
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts 
b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
index 24f1aac366d6..f82ccf332c0f 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64-plus.dts
@@ -48,18 +48,3 @@
 
/* TODO: Camera, touchscreen, etc. */
 };
-
- {
-   pinctrl-names = "default";
-   pinctrl-0 = <_pins>;
-   phy-mode = "rgmii";
-   phy-handle = <_rgmii_phy>;
-   status = "okay";
-};
-
- {
-   ext_rgmii_phy: ethernet-phy@1 {
-   compatible = "ethernet-phy-ieee802.3-c22";
-   reg = <1>;
-   };
-};
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts 
b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
index 122b5d8e5438..caf8b6fbe5e3 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-pine64.dts
@@ -51,7 +51,6 @@
compatible = "pine64,pine64", "allwinner,sun50i-a64";
 
aliases {
-   ethernet0 = 
serial0 = 
serial1 = 
serial2 = 
@@ -79,16 +78,6 @@
status = "okay";
 };
 
- {
-   pinctrl-names = "default";
-   pinctrl-0 = <_pins>;
-   phy-mode = "rmii";
-   phy-handle = <_rmii_phy1>;
-   phy-supply = <_dc1sw>;
-   status = "okay";
-
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
@@ -99,13 +88,6 @@
bias-pull-up;
 };
 
- {
-   ext_rmii_phy1: ethernet-phy@1 {
-   compatible = "ethernet-phy-ieee802.3-c22";
-   reg = <1>;
-   };
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins>;
diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts 
b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
index a053a6ac5267..17ccc12b58df 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64-sopine-baseboard.dts
@@ -53,7 +53,6 @@
 "allwinner,sun50i-a64";
 
aliases {
-   ethernet0 = 
serial0 = 
};
 
@@ -77,22 +76,6 @@
status = "okay";
 };
 
- {
-   pinctrl-names = "default";
-   pinctrl-0 = <_pins>;
-   phy-mode = "rgmii";
-   phy-handle = <_rgmii_phy>;
-   phy-supply = <_dc1sw>;
-   status = "okay";
-};
-
- {
-   ext_rgmii_phy: ethernet-phy@1 {
-   compatible = "ethernet-phy-ieee802.3-c22";
-   reg = <1>;
-   };
-};
-
  {
pinctrl-names = "d

[PATCH 3/4] arm: dts: sunxi: Revert EMAC changes

[Maxime Ripard]

Since the discussion is not settled yet for the EMAC, and that the release
in getting really close, let's revert the changes for now, and we'll
reintroduce them later.

Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts |  9 
 arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts   | 19 -
 arch/arm/boot/dts/sun8i-h3-beelink-x2.dts |  8 ---
 arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts |  7 --
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts |  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-one.dts   |  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts   |  5 -
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts|  8 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts  | 22 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-plus2e.dts| 16 --
 arch/arm/boot/dts/sunxi-h3-h5.dtsi| 26 ---
 11 files changed, 136 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts 
b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
index 6713d0f2b3f4..b1502df7b509 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
@@ -56,8 +56,6 @@
 
aliases {
serial0 = 
-   /* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
-   ethernet0 = 
ethernet1 = 
};
 
@@ -104,13 +102,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "mii";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
diff --git a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts 
b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
index d756ff825116..a337af1de322 100644
--- a/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
+++ b/arch/arm/boot/dts/sun8i-h3-bananapi-m2-plus.dts
@@ -52,7 +52,6 @@
compatible = "sinovoip,bpi-m2-plus", "allwinner,sun8i-h3";
 
aliases {
-   ethernet0 = 
serial0 = 
serial1 = 
};
@@ -115,30 +114,12 @@
status = "okay";
 };
 
- {
-   pinctrl-names = "default";
-   pinctrl-0 = <_rgmii_pins>;
-   phy-supply = <_gmac_3v3>;
-   phy-handle = <_rgmii_phy>;
-   phy-mode = "rgmii";
-
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
status = "okay";
 };
 
- {
-   ext_rgmii_phy: ethernet-phy@1 {
-   compatible = "ethernet-phy-ieee802.3-c22";
-   reg = <0>;
-   };
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>, <_cd_pin>;
diff --git a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts 
b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
index 546837ccd8af..5cd3a391bfd9 100644
--- a/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-beelink-x2.dts
@@ -53,7 +53,6 @@
 
aliases {
serial0 = 
-   ethernet0 = 
ethernet1 = 
};
 
@@ -108,13 +107,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "mii";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
diff --git a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts 
b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
index 78f6c24952dd..8d2cc6e9a03f 100644
--- a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
+++ b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
@@ -46,10 +46,3 @@
model = "FriendlyARM NanoPi NEO";
compatible = "friendlyarm,nanopi-neo", "allwinner,sun8i-h3";
 };
-
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "mii";
-   allwinner,leds-active-low;
-   status = "okay";
-};
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts 
b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
index 17cdeae19c6f..8ff71b1bb45b 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
@@ -54,7 +54,6 @@
aliases {
serial0 = 
/* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
-   ethernet0 = 
ethernet1 = 
};
 
@@ -118,13 +117,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "mii";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <

[PATCH 4/4] net: stmmac: sun8i: Remove the compatibles

[Maxime Ripard]

Since the bindings have been controversial, and we follow the DT stable ABI
rule, we shouldn't let a driver with a DT binding that might change slip
through in a stable release.

Remove the compatibles to make sure the driver will not probe and no-one
will start using the binding currently implemented. This commit will
obviously need to be reverted in due time.

Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 8 
 1 file changed, 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
index fffd6d5fc907..39c2122a4f26 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
@@ -979,14 +979,6 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id sun8i_dwmac_match[] = {
-   { .compatible = "allwinner,sun8i-h3-emac",
-   .data = _variant_h3 },
-   { .compatible = "allwinner,sun8i-v3s-emac",
-   .data = _variant_v3s },
-   { .compatible = "allwinner,sun8i-a83t-emac",
-   .data = _variant_a83t },
-   { .compatible = "allwinner,sun50i-a64-emac",
-   .data = _variant_a64 },
{ }
 };
 MODULE_DEVICE_TABLE(of, sun8i_dwmac_match);
-- 
2.13.5

Re: [PATCH] net: stmmac: dwmac-sun8i: Use reset exclusive

[Maxime Ripard]

On Fri, Aug 25, 2017 at 05:17:33PM +0200, Corentin Labbe wrote:
> On Fri, Aug 25, 2017 at 04:48:32PM +0200, Maxime Ripard wrote:
> > On Fri, Aug 25, 2017 at 04:38:05PM +0200, Corentin Labbe wrote:
> > > The current dwmac_sun8i module cannot be rmmod/modprobe due to that
> > > the reset controller was not released when removed.
> > > 
> > > This patch remove ambiguity, by using of_reset_control_get_exclusive and
> > > add the missing reset_control_put().
> > > 
> > > Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
> > > b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> > > index fffd6d5fc907..675a09629d85 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> > > @@ -782,6 +782,7 @@ static int sun8i_dwmac_unpower_internal_phy(struct 
> > > sunxi_priv_data *gmac)
> > >  
> > >   clk_disable_unprepare(gmac->ephy_clk);
> > >   reset_control_assert(gmac->rst_ephy);
> > > + reset_control_put(gmac->rst_ephy);
> > >   return 0;
> > >  }
> > >  
> > > @@ -942,7 +943,7 @@ static int sun8i_dwmac_probe(struct platform_device 
> > > *pdev)
> > >   return -EINVAL;
> > >   }
> > >  
> > > - gmac->rst_ephy = of_reset_control_get(plat_dat->phy_node, NULL);
> > > + gmac->rst_ephy = 
> > > of_reset_control_get_exclusive(plat_dat->phy_node, NULL);
> > 
> > Why not just use devm_reset_control_get?
> > 
> 
> Because there no devm_ functions with of_

devm_reset_control_get uses of_reset_control_get internally.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH] net: stmmac: dwmac-sun8i: Use reset exclusive

[Maxime Ripard]

On Fri, Aug 25, 2017 at 04:38:05PM +0200, Corentin Labbe wrote:
> The current dwmac_sun8i module cannot be rmmod/modprobe due to that
> the reset controller was not released when removed.
> 
> This patch remove ambiguity, by using of_reset_control_get_exclusive and
> add the missing reset_control_put().
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c 
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> index fffd6d5fc907..675a09629d85 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c
> @@ -782,6 +782,7 @@ static int sun8i_dwmac_unpower_internal_phy(struct 
> sunxi_priv_data *gmac)
>  
>   clk_disable_unprepare(gmac->ephy_clk);
>   reset_control_assert(gmac->rst_ephy);
> + reset_control_put(gmac->rst_ephy);
>   return 0;
>  }
>  
> @@ -942,7 +943,7 @@ static int sun8i_dwmac_probe(struct platform_device *pdev)
>   return -EINVAL;
>   }
>  
> - gmac->rst_ephy = of_reset_control_get(plat_dat->phy_node, NULL);
> + gmac->rst_ephy = 
> of_reset_control_get_exclusive(plat_dat->phy_node, NULL);

Why not just use devm_reset_control_get?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

On Wed, Aug 23, 2017 at 09:31:53AM -0700, Florian Fainelli wrote:
> On 08/23/2017 12:49 AM, Maxime Ripard wrote:
> > Hi Florian,
> > 
> > On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> >>>>> So I think what you are saying is either impossible or engineering-wise
> >>>>> a very stupid design, like using an external MAC with a discrete PHY
> >>>>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>>>> with the internal PHY.
> >>>>>
> >>>>> Now can we please decide on something? We're a week and a half from
> >>>>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>>>> nodes (internal-mdio & external-mdio).
> >>>>
> >>>> I really don't see a need for a mdio-mux in the first place, just have
> >>>> one MDIO controller (current state) sub-node which describes the
> >>>> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >>>> node (along with 'phy-is-integrated'). If a different configuration is
> >>>> used, then just put the external PHY as a child node there.
> >>>>
> >>>> If fixed-link is required, the mdio node becomes unused anyway.
> >>>>
> >>>> Works for everyone?
> >>>
> >>> If we put an external PHY with reg=1 as a child of internal MDIO,
> >>> il will be merged with internal PHY node and get
> >>> phy-is-integrated.
> >>
> >> Then have the .dtsi file contain just the mdio node, but no internal or
> >> external PHY and push all the internal and external PHY node definition
> >> (in its entirety) to the per-board DTS file, does not that work?
> > 
> > If possible, I'd really like to have the internal PHY in the
> > DTSI. It's always there in hardware anyway, and duplicating the PHY,
> > with its clock, reset line, and whatever info we might need in the
> > future in each and every board DTS that uses it will be very error
> > prone and we will have the usual bunch of issues that come up with
> > duplication.
> 
> OK, then what if you put the internal PHY in the DTSI, mark it with a
> status = "disabled" property, and have the per-board DTS put a status =
> "okay" property along with a "phy-is-integrated" boolean property? Would
> that work?

Yeah, that would work for me.

> What I really don't think is necessary is:
> 
> - duplicating the "mdio" controller node for external vs. internal PHY,
> because this is not accurate, there is just one MDIO controller, but
> there may be different kinds of MDIO/PHY devices attached

Agreed.

> - having the STMMAC driver MDIO probing code having to deal with a
> "mdio" sub-node or an "internal-mdio" sub-node because this is confusing
> and requiring more driver-level changes that are error prone

I don't really have an opinion on that one, so I'll defer to your
judgment of what's best :)

I guess we have an agreement. Andrew, is that ok for you too?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

Hi Florian,

On Tue, Aug 22, 2017 at 11:35:01AM -0700, Florian Fainelli wrote:
> >>> So I think what you are saying is either impossible or engineering-wise
> >>> a very stupid design, like using an external MAC with a discrete PHY
> >>> connected to the internal MAC's MDIO bus, while using the internal MAC
> >>> with the internal PHY.
> >>>
> >>> Now can we please decide on something? We're a week and a half from
> >>> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> >>> nodes (internal-mdio & external-mdio).
> >>
> >> I really don't see a need for a mdio-mux in the first place, just have
> >> one MDIO controller (current state) sub-node which describes the
> >> built-in STMMAC MDIO controller and declare the internal PHY as a child
> >> node (along with 'phy-is-integrated'). If a different configuration is
> >> used, then just put the external PHY as a child node there.
> >>
> >> If fixed-link is required, the mdio node becomes unused anyway.
> >>
> >> Works for everyone?
> > 
> > If we put an external PHY with reg=1 as a child of internal MDIO,
> > il will be merged with internal PHY node and get
> > phy-is-integrated.
> 
> Then have the .dtsi file contain just the mdio node, but no internal or
> external PHY and push all the internal and external PHY node definition
> (in its entirety) to the per-board DTS file, does not that work?

If possible, I'd really like to have the internal PHY in the
DTSI. It's always there in hardware anyway, and duplicating the PHY,
with its clock, reset line, and whatever info we might need in the
future in each and every board DTS that uses it will be very error
prone and we will have the usual bunch of issues that come up with
duplication.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

On Tue, Aug 22, 2017 at 11:39:22PM +0800, Chen-Yu Tsai wrote:
> Now can we please decide on something? We're a week and a half from
> the 4.13 release. If mdio-mux is wrong, then we could have two mdio
> nodes (internal-mdio & external-mdio).

I can only emphasize this. I'm afraid that if we don't have an
agreement and a patch implementing it with the proper acks by the end
of the week, we'll have to revert all the DT bits and the bindings.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 3/4] net: stmmac: register parent MDIO node for sun8i-h3-emac

[Maxime Ripard]

Hi Andrew,

On Mon, Aug 21, 2017 at 03:20:15PM +0200, Andrew Lunn wrote:
> > With our hardware, and likely Rockchip's as well, the muxed connections
> > include the MDIO and MII connections
> 
> Ah, i did not realise the MII was muxed as well. Then i agree, an MDIO
> mux is wrong.
> 
> However, please try to make the binding not look like an mdio mux. We
> don't want people misunderstanding the binding and thinking it is an
> mdio mux.

Do you have any suggestion on what the binding would look like?

All muxes are mostly always represented the same way afaik, or do you
want to simply introduce a new compatible / property?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 05/11] net: stmmac: dwmac-rk: Add internal phy support

[Maxime Ripard]

On Thu, Jul 27, 2017 at 07:31:52PM +0200, Corentin Labbe wrote:
> On Thu, Jul 27, 2017 at 09:54:01AM -0700, Florian Fainelli wrote:
> > On 07/27/2017 06:48 AM, Andrew Lunn wrote:
> > > On Thu, Jul 27, 2017 at 09:02:16PM +0800, David Wu wrote:
> > >> To make internal phy work, need to configure the phy_clock,
> > >> phy cru_reset and related registers.
> > >>
> > >> Signed-off-by: David Wu <david...@rock-chips.com>
> > >> ---
> > >> changes in v2:
> > >>  - Use the standard "phy-mode" property for internal phy. (Florian)
> > > 
> > > I think we need to discuss this. This PHY appears to be on an MDIO
> > > bus, it uses a standard PHY driver, and it appears to be using an RMII
> > > interface. So it is just an ordinary PHY.
> > 
> > First, the fact that the internal PHY also appears through MDIO is
> > orthogonal to the fact that it is internal or external. Plenty of
> > designs have internal PHYs exposed through MDIO because that is
> > convenient. What matters though is how the data/clock lines are wired
> > internally, which is what "phy-mode" describes.
> > 
> > > 
> > > Internal is supposed to be something which is not ordinary, does not
> > > use one of the standard phy modes, needs something special to make it
> > > work.
> > > 
> > > Florain, it appears to be your suggestion to use internal. What do you
> > > say?
> > 
> > phy-mode = "internal" really means that it is not a standard MII variant
> > to connect the data/clock lines between the Ethernet MAC and the PHY,
> > and this can happen in some designs (although quite unlikely). So from
> > there we could do several things depending on the requirements:
> > 
> > - if you can have your Ethernet MAC driver perform the necessary
> > configuration *after* you have been able to bind the PHY device with its
> > PHY driver, then the PHY driver should have PHY_IS_INTERNAL in its
> > flags, and you can use phy_is_internal() from PHYLIB to tell you that
> > and we could imagine using: phy-mode = "rmii" because that would not too
> > much of a stretch
> > 
> > - if you need knowledge about this PHY connection type prior to binding
> > the PHY device and its driver (that is, before of_phy_connect()) we
> > could add a boolean property e.g: "phy-is-internal" that allows us to
> > know that, or we can have a new phy-mode value, e.g: "internal-rmii"
> > which describes that, either way would probably be fine, but the former
> > scales better
> 
> We have the same problem on Allwinner SoCs for dwmac-sun8i, we need
> to set a syscon for chossing between internal/external PHY.
>
> Having this phy-is-internal would be very helpfull. (adding
> internal-xmii will add too many flags in our case)

In our case, we'll always have a phy node, so we can have a compatible
that will give you the same information.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

[PATCH] arm: sunxi: Revert changes merged through net-next.

[Maxime Ripard]

This reverts commits 2c0cba482e79 ("arm: sun8i: sunxi-h3-h5: Add dt node
for the syscon control module") to 2428fd0fe550 ("arm64: defconfig: Enable
dwmac-sun8i driver on defconfig") and 3432a86e641c ("arm: sun8i:
orangepipc: use internal phy-mode") to 5a79b4f2a5e7 ("arm: sun8i:
orangepi-2: use internal phy-mode") that should be merged
through the arm-soc tree, and end up in merge conflicts and build failures.

Signed-off-by: Maxime Ripard <maxime.rip...@free-electrons.com>
---
 arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts  |  8 -
 arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts  |  7 
 arch/arm/boot/dts/sun8i-h3-orangepi-2.dts  |  8 -
 arch/arm/boot/dts/sun8i-h3-orangepi-one.dts|  8 -
 arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts|  5 ---
 arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts |  8 -
 arch/arm/boot/dts/sunxi-h3-h5.dtsi | 40 -
 arch/arm/configs/multi_v7_defconfig|  1 -
 arch/arm/configs/sunxi_defconfig   |  1 -
 .../boot/dts/allwinner/sun50i-a64-bananapi-m64.dts | 15 
 .../boot/dts/allwinner/sun50i-a64-pine64-plus.dts  | 17 +
 .../arm64/boot/dts/allwinner/sun50i-a64-pine64.dts | 16 -
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi  | 41 --
 arch/arm64/configs/defconfig   |  1 -
 14 files changed, 1 insertion(+), 175 deletions(-)

diff --git a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts 
b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
index 9bd9a0e90034..9e8b082c134f 100644
--- a/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
+++ b/arch/arm/boot/dts/sun8i-h2-plus-orangepi-zero.dts
@@ -57,7 +57,6 @@
aliases {
serial0 = 
/* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
-   ethernet0 = 
ethernet1 = 
};
 
@@ -104,13 +103,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "internal";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
diff --git a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts 
b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
index 5c5ba806e2f1..8d2cc6e9a03f 100644
--- a/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
+++ b/arch/arm/boot/dts/sun8i-h3-nanopi-neo.dts
@@ -46,10 +46,3 @@
model = "FriendlyARM NanoPi NEO";
compatible = "friendlyarm,nanopi-neo", "allwinner,sun8i-h3";
 };
-
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "internal";
-   allwinner,leds-active-low;
-   status = "okay";
-};
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts 
b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
index 77c861deb658..5b6d14555b7c 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
@@ -54,7 +54,6 @@
aliases {
serial0 = 
/* ethernet0 is the H3 emac, defined in sun8i-h3.dtsi */
-   ethernet0 = 
ethernet1 = 
};
 
@@ -109,13 +108,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "internal";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts 
b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts
index 27e7ef4e42f2..5fea430e0eb1 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-one.dts
@@ -52,7 +52,6 @@
compatible = "xunlong,orangepi-one", "allwinner,sun8i-h3";
 
aliases {
-   ethernet0 = 
serial0 = 
};
 
@@ -98,13 +97,6 @@
status = "okay";
 };
 
- {
-   phy-handle = <_mii_phy>;
-   phy-mode = "internal";
-   allwinner,leds-active-low;
-   status = "okay";
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>, <_cd_pin>;
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts 
b/arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts
index a10281b455f5..8b93f5c781a7 100644
--- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts
+++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc-plus.dts
@@ -53,11 +53,6 @@
};
 };
 
- {
-   /* LEDs changed to active high on the plus */
-   /delete-property/ allwinner,leds-active-low;
-};
-
  {
pinctrl-names = "default";
pinctrl-0 = <_pins_a>;
diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts 
b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
index fecebb43506a..f148111c326d 100644
--- a/

Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Maxime Ripard]

On Tue, Jun 27, 2017 at 02:37:48PM +0200, Corentin Labbe wrote:
> On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> > Hi,
> > 
> > On 27/06/17 11:23, Icenowy Zheng wrote:
> > > 
> > > 
> > > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <andre.przyw...@arm.com> 
> > > 写到:
> > >> Hi,
> > >>
> > >> On 27/06/17 10:41, Maxime Ripard wrote:
> > >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> > >>>> Hi,
> > >>>>
> > >>>> (CC:ing some people from that Rockchip dmwac series)
> > >>>>
> > >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> > >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> > >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> > >>>>>> <clabbe.montj...@gmail.com> wrote:
> > >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> > >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> > >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> > >>>>>>>>> allwinner.
> > >>>>>>>>> In fact the only common part is the descriptor management and
> > >> the first
> > >>>>>>>>> register function.
> > >>>>>>>>
> > >>>>>>>> Hi,
> > >>>>>>>>
> > >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> > >> driver
> > >>>>>>>> to the new binding I was wondering about the internal PHY
> > >> detection:
> > >>>>>>>>
> > >>>>>>>>
> > >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> > >> PHY
> > >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> > >> external).
> > >>>>>>>> I think I raised this question before, but isn't it perfectly
> > >> legal for
> > >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> > >> feature
> > >>>>>>>> an internal PHY?
> > >>>>>>>> On the first glance that does not make too much sense, but apart
> > >> from
> > >>>>>>>> not being the correct binding to describe all of the SoCs
> > >> features I see
> > >>>>>>>> two scenarios:
> > >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> > >> because it
> > >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> > >> For
> > >>>>>>>> instance I have heard reports that the internal PHY makes the
> > >> SoC go
> > >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> > >> external
> > >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> > >> avoided.
> > >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> > >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> > >> switch
> > >>>>>>>> IC or some other network circuitry, for instance fibre
> > >> connectors.
> > >>>>>>>>
> > >>>>>>>> So I was wondering if we would need an explicit:
> > >>>>>>>>   allwinner,use-internal-phy;
> > >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> > >>>>>>>> Alternatively we could go with the negative version:
> > >>>>>>>>   allwinner,disable-internal-phy;
> > >>>>>>>>
> > >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> > >> compatible
> > >>>>>>>> string for the *PHY* node and use that?
> > >>>>>>>>
> > >>>>>>>> I just want to avoid that we introduce a binding that causes us
> > >>>>>>>&

Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Maxime Ripard]

On Tue, Jun 27, 2017 at 11:33:56AM +0100, Andre Przywara wrote:
> Hi,
> 
> On 27/06/17 11:23, Icenowy Zheng wrote:
> > 
> > 
> > 于 2017年6月27日 GMT+08:00 下午6:15:58, Andre Przywara <andre.przyw...@arm.com> 
> > 写到:
> >> Hi,
> >>
> >> On 27/06/17 10:41, Maxime Ripard wrote:
> >>> On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> >>>> Hi,
> >>>>
> >>>> (CC:ing some people from that Rockchip dmwac series)
> >>>>
> >>>> On 27/06/17 09:21, Corentin Labbe wrote:
> >>>>> On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >>>>>> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >>>>>> <clabbe.montj...@gmail.com> wrote:
> >>>>>>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>>>>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>>>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>>>>>> allwinner.
> >>>>>>>>> In fact the only common part is the descriptor management and
> >> the first
> >>>>>>>>> register function.
> >>>>>>>>
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> I know I am a bit late with this, but while adapting the U-Boot
> >> driver
> >>>>>>>> to the new binding I was wondering about the internal PHY
> >> detection:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> So here you seem to deduce the usage of the internal PHY by the
> >> PHY
> >>>>>>>> interface specified in the DT (MII = internal, RGMII =
> >> external).
> >>>>>>>> I think I raised this question before, but isn't it perfectly
> >> legal for
> >>>>>>>> a board to use MII with an external PHY even on those SoCs that
> >> feature
> >>>>>>>> an internal PHY?
> >>>>>>>> On the first glance that does not make too much sense, but apart
> >> from
> >>>>>>>> not being the correct binding to describe all of the SoCs
> >> features I see
> >>>>>>>> two scenarios:
> >>>>>>>> 1) A board vendor might choose to not use the internal PHY
> >> because it
> >>>>>>>> has bugs, lacks features (configurability) or has other issues.
> >> For
> >>>>>>>> instance I have heard reports that the internal PHY makes the
> >> SoC go
> >>>>>>>> rather hot, possibly limiting the CPU frequency. By using an
> >> external
> >>>>>>>> MII PHY (which are still cheaper than RGMII PHYs) this can be
> >> avoided.
> >>>>>>>> 2) A PHY does not necessarily need to be directly connected to
> >>>>>>>> magnetics. Indeed quite some boards use (RG)MII to connect to a
> >> switch
> >>>>>>>> IC or some other network circuitry, for instance fibre
> >> connectors.
> >>>>>>>>
> >>>>>>>> So I was wondering if we would need an explicit:
> >>>>>>>>   allwinner,use-internal-phy;
> >>>>>>>> boolean DT property to signal the usage of the internal PHY?
> >>>>>>>> Alternatively we could go with the negative version:
> >>>>>>>>   allwinner,disable-internal-phy;
> >>>>>>>>
> >>>>>>>> Or what about introducing a new "allwinner,internal-mii-phy"
> >> compatible
> >>>>>>>> string for the *PHY* node and use that?
> >>>>>>>>
> >>>>>>>> I just want to avoid that we introduce a binding that causes us
> >>>>>>>> headaches later. I think we can still fix this with a followup
> >> patch
> >>>>>>>> before the driver and its binding hit a release kernel.
> >>>>>>>>
> >>>>>>>> Cheers,
> >>>>>>>> Andre.
> >>>>>>>>
> >>>>>>>
> >>>>>>> I just see some patch, where "phy-mode = internal" is valid.
>

Re: [PATCH v6 05/21] net-next: stmmac: Add dwmac-sun8i

[Maxime Ripard]

On Tue, Jun 27, 2017 at 10:02:45AM +0100, Andre Przywara wrote:
> Hi,
> 
> (CC:ing some people from that Rockchip dmwac series)
> 
> On 27/06/17 09:21, Corentin Labbe wrote:
> > On Tue, Jun 27, 2017 at 04:11:21PM +0800, Chen-Yu Tsai wrote:
> >> On Tue, Jun 27, 2017 at 4:05 PM, Corentin Labbe
> >> <clabbe.montj...@gmail.com> wrote:
> >>> On Mon, Jun 26, 2017 at 01:18:23AM +0100, André Przywara wrote:
> >>>> On 31/05/17 08:18, Corentin Labbe wrote:
> >>>>> The dwmac-sun8i is a heavy hacked version of stmmac hardware by
> >>>>> allwinner.
> >>>>> In fact the only common part is the descriptor management and the first
> >>>>> register function.
> >>>>
> >>>> Hi,
> >>>>
> >>>> I know I am a bit late with this, but while adapting the U-Boot driver
> >>>> to the new binding I was wondering about the internal PHY detection:
> >>>>
> >>>>
> >>>> So here you seem to deduce the usage of the internal PHY by the PHY
> >>>> interface specified in the DT (MII = internal, RGMII = external).
> >>>> I think I raised this question before, but isn't it perfectly legal for
> >>>> a board to use MII with an external PHY even on those SoCs that feature
> >>>> an internal PHY?
> >>>> On the first glance that does not make too much sense, but apart from
> >>>> not being the correct binding to describe all of the SoCs features I see
> >>>> two scenarios:
> >>>> 1) A board vendor might choose to not use the internal PHY because it
> >>>> has bugs, lacks features (configurability) or has other issues. For
> >>>> instance I have heard reports that the internal PHY makes the SoC go
> >>>> rather hot, possibly limiting the CPU frequency. By using an external
> >>>> MII PHY (which are still cheaper than RGMII PHYs) this can be avoided.
> >>>> 2) A PHY does not necessarily need to be directly connected to
> >>>> magnetics. Indeed quite some boards use (RG)MII to connect to a switch
> >>>> IC or some other network circuitry, for instance fibre connectors.
> >>>>
> >>>> So I was wondering if we would need an explicit:
> >>>>   allwinner,use-internal-phy;
> >>>> boolean DT property to signal the usage of the internal PHY?
> >>>> Alternatively we could go with the negative version:
> >>>>   allwinner,disable-internal-phy;
> >>>>
> >>>> Or what about introducing a new "allwinner,internal-mii-phy" compatible
> >>>> string for the *PHY* node and use that?
> >>>>
> >>>> I just want to avoid that we introduce a binding that causes us
> >>>> headaches later. I think we can still fix this with a followup patch
> >>>> before the driver and its binding hit a release kernel.
> >>>>
> >>>> Cheers,
> >>>> Andre.
> >>>>
> >>>
> >>> I just see some patch, where "phy-mode = internal" is valid.
> >>> I will try to find a way to use it
> >>
> >> Can you provide a link?
> > 
> > https://lkml.org/lkml/2017/6/23/479
> > 
> >>
> >> I'm not a fan of using phy-mode for this. There's no guarantee what
> >> mode the internal PHY uses. That's what phy-mode is for.
> 
> I can understand Chen-Yu's concerns, but ...
> 
> > For each soc the internal PHY mode is know and setted in 
> > emac_variant/internal_phy
> > So its not a problem.
> 
> that is true as well, at least for now.
>
> So while I agree that having a separate property to indicate the usage
> of the internal PHY would be nice, I am bit tempted to use this easier
> approach and piggy back on the existing phy-mode property.

We're trying to fix an issue that works for now too.

If we want to consider future weird cases, then we must consider all
of them. And the phy mode changing is definitely not really far
fetched.

I agree with Chen-Yu, and I really feel like the compatible solution
you suggested would cover both your concerns, and ours.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver

[Maxime Ripard]

On Sat, Jun 03, 2017 at 12:24:22AM +0200, Maxime Ripard wrote:
> Hi,
> 
> On Fri, Jun 02, 2017 at 10:22:05AM -0400, David Miller wrote:
> > From: Maxime Ripard <maxime.rip...@free-electrons.com>
> > Date: Fri, 2 Jun 2017 11:13:20 +0200
> > 
> > > On Fri, Jun 02, 2017 at 08:37:52AM +0200, Maxime Ripard wrote:
> > >> On Thu, Jun 01, 2017 at 02:58:19PM -0400, David Miller wrote:
> > >> > From: Corentin Labbe <clabbe.montj...@gmail.com>
> > >> > Date: Wed, 31 May 2017 09:18:31 +0200
> > >> > 
> > >> > > This patch series add the driver for dwmac-sun8i which handle the 
> > >> > > Ethernet MAC
> > >> > > present on Allwinner H3/H5/A83T/A64 SoCs.
> > >> > 
> > >> > Series applied, but wow that's a lot of DT file changes :-(
> > >> 
> > >> The DT patches should not go through your tree, but arm-soc, so I
> > >> guess this is not an issue for you?
> > > 
> > > Ok, so I saw that you actually merged them. Can you revert or drop
> > > that merge for the DT part?
> > > 
> > > This will generate a lot of conflicts with our tree, and I'm not sure
> > > this would be efficient to make you take all the entirely unrelated to
> > > next patches.
> > 
> > Please tell me which specific changes to revert.
> > 
> > Thank you.
> 
> Ideally everything from 2c0cba482e79 ("arm: sun8i: sunxi-h3-h5: Add dt
> node for the syscon control module") to 2428fd0fe550 ("arm64:
> defconfig: Enable dwmac-sun8i driver on defconfig")

Ping?

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver

[Maxime Ripard]

Hi,

On Fri, Jun 02, 2017 at 10:22:05AM -0400, David Miller wrote:
> From: Maxime Ripard <maxime.rip...@free-electrons.com>
> Date: Fri, 2 Jun 2017 11:13:20 +0200
> 
> > On Fri, Jun 02, 2017 at 08:37:52AM +0200, Maxime Ripard wrote:
> >> On Thu, Jun 01, 2017 at 02:58:19PM -0400, David Miller wrote:
> >> > From: Corentin Labbe <clabbe.montj...@gmail.com>
> >> > Date: Wed, 31 May 2017 09:18:31 +0200
> >> > 
> >> > > This patch series add the driver for dwmac-sun8i which handle the 
> >> > > Ethernet MAC
> >> > > present on Allwinner H3/H5/A83T/A64 SoCs.
> >> > 
> >> > Series applied, but wow that's a lot of DT file changes :-(
> >> 
> >> The DT patches should not go through your tree, but arm-soc, so I
> >> guess this is not an issue for you?
> > 
> > Ok, so I saw that you actually merged them. Can you revert or drop
> > that merge for the DT part?
> > 
> > This will generate a lot of conflicts with our tree, and I'm not sure
> > this would be efficient to make you take all the entirely unrelated to
> > next patches.
> 
> Please tell me which specific changes to revert.
> 
> Thank you.

Ideally everything from 2c0cba482e79 ("arm: sun8i: sunxi-h3-h5: Add dt
node for the syscon control module") to 2428fd0fe550 ("arm64:
defconfig: Enable dwmac-sun8i driver on defconfig")

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver

[Maxime Ripard]

On Fri, Jun 02, 2017 at 08:37:52AM +0200, Maxime Ripard wrote:
> On Thu, Jun 01, 2017 at 02:58:19PM -0400, David Miller wrote:
> > From: Corentin Labbe <clabbe.montj...@gmail.com>
> > Date: Wed, 31 May 2017 09:18:31 +0200
> > 
> > > This patch series add the driver for dwmac-sun8i which handle the 
> > > Ethernet MAC
> > > present on Allwinner H3/H5/A83T/A64 SoCs.
> > 
> > Series applied, but wow that's a lot of DT file changes :-(
> 
> The DT patches should not go through your tree, but arm-soc, so I
> guess this is not an issue for you?

Ok, so I saw that you actually merged them. Can you revert or drop
that merge for the DT part?

This will generate a lot of conflicts with our tree, and I'm not sure
this would be efficient to make you take all the entirely unrelated to
next patches.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v6 00/21] net-next: stmmac: add dwmac-sun8i ethernet driver

[Maxime Ripard]

On Thu, Jun 01, 2017 at 02:58:19PM -0400, David Miller wrote:
> From: Corentin Labbe <clabbe.montj...@gmail.com>
> Date: Wed, 31 May 2017 09:18:31 +0200
> 
> > This patch series add the driver for dwmac-sun8i which handle the Ethernet 
> > MAC
> > present on Allwinner H3/H5/A83T/A64 SoCs.
> 
> Series applied, but wow that's a lot of DT file changes :-(

The DT patches should not go through your tree, but arm-soc, so I
guess this is not an issue for you?

Maximee

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 18/18] arm: multi_v7: Enable dwmac-sun8i driver on multi_v7_defconfig

[Maxime Ripard]

On Wed, Apr 12, 2017 at 01:14:00PM +0200, Corentin Labbe wrote:
> Enable the dwmac-sun8i driver in the multi_v7 default configuration
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>

Maybe we should enable that in arm64's defconfig too?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 14/18] arm: allwinner: pine64: Enable dwmac-sun8i

[Maxime Ripard]

On Wed, Apr 12, 2017 at 01:13:56PM +0200, Corentin Labbe wrote:
> The dwmac-sun8i hardware is present on the pine64
> It uses an external PHY via RMII.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>

The architecture in your commit title in arm64, not arm (the next
patch also have this issue).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 13/18] arm64: allwinner: sun50i-a64: add dwmac-sun8i Ethernet driver

[Maxime Ripard]

On Wed, Apr 12, 2017 at 01:13:55PM +0200, Corentin Labbe wrote:
> The dwmac-sun8i is an Ethernet MAC that supports 10/100/1000 Mbit
> connections. It is very similar to the device found in the Allwinner
> H3, but lacks the internal 100 Mbit PHY and its associated control
> bits.
> This adds the necessary bits to the Allwinner A64 SoC .dtsi, but keeps
> it disabled at this level.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 37 
> +++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi 
> b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> index 0b0f4ab..2569827 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
> @@ -287,6 +287,23 @@
>   bias-pull-up;
>   };
>  
> + rmii_pins: rmii_pins {
> + pins = "PD10", "PD11", "PD13", "PD14",
> + "PD17", "PD18", "PD19", "PD20",
> + "PD22", "PD23";

Please align the wrapped lines on the first pin.

> +     function = "emac";
> + drive-strength = <40>;

Do you actually need that for all the boards, or only a few of them?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 08/18] arm: sun8i: orangepi-pc: Enable dwmac-sun8i

[Maxime Ripard]

Hi Corentin,

On Wed, Apr 12, 2017 at 01:13:50PM +0200, Corentin Labbe wrote:
> The dwmac-sun8i hardware is present on the Orange PI PC.
> It uses the internal PHY.
> 
> This patch create the needed emac node.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts 
> b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> index f148111..746c25a 100644
> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-pc.dts
> @@ -53,6 +53,7 @@
>  
>   aliases {
>   serial0 = 
> + ethernet0 = 

Sorry for not spotting this earlier...

This should be ordered by alphabetical order, 

>   };
>  
>   chosen {
> @@ -184,3 +185,10 @@
>   /* USB VBUS is always on */
>   status = "okay";
>  };
> +
> + {
> + phy-handle = <_mii_phy>;
> + phy-mode = "mii";
> + allwinner,leds-active-low;
> + status = "okay";
> +};

And the node here as well.

Almost all your other DT patches also have this issue, please fix it
in all of them.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 20/20] ARM: sunxi: Enable dwmac-sun8i driver on multi_v7_defconfig

[Maxime Ripard]

On Mon, Apr 03, 2017 at 11:14:44AM +0200, Corentin Labbe wrote:
> Enable the dwmac-sun8i driver in the multi_v7 default configuration

Your prefix should be arm: multi_v7:

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 16/20] ARM: dts: sun50i-a64: enable dwmac-sun8i on pine64

[Maxime Ripard]

On Mon, Apr 03, 2017 at 11:14:40AM +0200, Corentin Labbe wrote:
> The dwmac-sun8i hardware is present on the pine64
> It uses an external PHY via RMII.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>

Looks fine, but please use "arm64: allwinner: pine64: " as your title
prefix. It applies to all your other patches (and the arm ones should
be "arm: : :".

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 12/20] ARM: dts: sun8i: Enable dwmac-sun8i on the Orange Pi plus

[Maxime Ripard]

On Mon, Apr 03, 2017 at 11:14:36AM +0200, Corentin Labbe wrote:
> The dwmac-sun8i hardware is present on the Orange PI plus.
> It uses an external PHY rtl8211e via RGMII.
> 
> This patch create the needed regulator, emac and phy nodes.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts | 36 
> 
>  1 file changed, 36 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts 
> b/arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts
> index 8c40ab7..6852006 100644
> --- a/arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts
> +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-plus.dts
> @@ -58,6 +58,18 @@
>   enable-active-high;
>   gpio = < 6 11 GPIO_ACTIVE_HIGH>;
>   };
> +
> + reg_gmac_3v3: gmac-3v3 {
> + compatible = "regulator-fixed";
> + pinctrl-names = "default";
> + pinctrl-0 = <_power_pin_orangepi>;
> + regulator-name = "gmac-3v3";
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> + startup-delay-us = <10>;
> + enable-active-high;
> + gpio = < 3 6 GPIO_ACTIVE_HIGH>;
> + };
>  };
>  
>   {
> @@ -86,8 +98,32 @@
>   pins = "PG11";
>   function = "gpio_out";
>   };
> +
> + gmac_power_pin_orangepi: gmac_power_pin@0 {
> + pins = "PD6";
> + function = "gpio_out";
> + drive-strength = <10>;
> + };

This is not needed, and will even harm the fixing of a bug that will
require to remove all the GPIO nodes. It works fine without it, please
remove it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 04/20] ARM: sun8i: dt: Add DT bindings documentation for Allwinner syscon

[Maxime Ripard]

On Mon, Apr 03, 2017 at 11:14:28AM +0200, Corentin Labbe wrote:
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  .../devicetree/bindings/misc/allwinner,syscon.txt | 19 
> +++
>  1 file changed, 19 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/misc/allwinner,syscon.txt
> 
> diff --git a/Documentation/devicetree/bindings/misc/allwinner,syscon.txt 
> b/Documentation/devicetree/bindings/misc/allwinner,syscon.txt
> new file mode 100644
> index 000..9f5f1f5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/allwinner,syscon.txt
> @@ -0,0 +1,19 @@
> +* Allwinner sun8i system controller
> +
> +This file describes the bindings for the system controller present in
> +Allwinner SoC H3, A83T and A64.
> +The principal function of this syscon is to control EMAC PHY choice and
> +config.
> +
> +Required properties for the system controller:
> +- reg: address and length of the register for the device.
> +- compatible: should be "syscon" and one of the following string:
> + "allwinner,sun8i-h3-system-controller"
> + "allwinner,sun8i-a64-system-controller"
> + "allwinner,sun8i-a83t-system-controller"
> +
> +Example:
> +syscon: syscon@01c0 {
> + compatible = "syscon", "allwinner,sun8i-h3-system-controller";

The syntax is the more specific first, so your compatibles should be
the other way around.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 03/20] dt-bindings: net: Add DT bindings documentation for Allwinner dwmac-sun8i

[Maxime Ripard]

On Mon, Apr 03, 2017 at 11:14:27AM +0200, Corentin Labbe wrote:
> This patch adds documentation for Device-Tree bindings for the
> Allwinner dwmac-sun8i driver.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> Acked-by: Rob Herring <r...@kernel.org>
> ---
>  .../devicetree/bindings/net/dwmac-sun8i.txt| 77 
> ++
>  1 file changed, 77 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt 
> b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> new file mode 100644
> index 000..f01ef17
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> @@ -0,0 +1,77 @@
> +* Allwinner sun8i GMAC ethernet controller
> +
> +This device is a platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +Required properties:
> +- compatible: should be one of the following string:
> + "allwinner,sun8i-a83t-emac"
> + "allwinner,sun8i-h3-emac"
> + "allwinner,sun50i-a64-emac"
> +- reg: address and length of the register for the device.
> +- interrupts: interrupt for the device
> +- interrupt-names: should be "macirq"
> +- clocks: A phandle to the reference clock for this device
> +- clock-names: should be "stmmaceth"
> +- resets: A phandle to the reset control for this device
> +- reset-names: should be "stmmaceth"
> +- phy-mode: See ethernet.txt
> +- phy-handle: See ethernet.txt
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +- syscon: A phandle to the syscon of the SoC with one of the following
> + compatible string:
> +  - allwinner,sun8i-h3-system-controller
> +  - allwinner,sun8i-a64-system-controller
> +  - allwinner,sun8i-a83t-system-controller

I'm not sure you need to document those compatibles here.

> +Optional properties:
> +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. 
> Default is 0)
> +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. 
> Default is 0)
> +Both delay properties are in 0.1ns step.

allwinner,tx-delay-ps and allwinner,rx-delay-ps, with the value in
picoseconds?

Looks good otherwise, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

[Maxime Ripard]

On Fri, Feb 17, 2017 at 02:18:02PM +0100, Corentin Labbe wrote:
> On Thu, Feb 16, 2017 at 08:05:24PM +0100, Maxime Ripard wrote:
> > Hi,
> > 
> 
> [...]
> > > +
> > > +struct emac_variant {
> > > + u32 default_syscon_value;
> > 
> > Why do you need a default value? Can't you read it from the syscon
> > directly?
> > 
> 
> Why not, but you can see the default value as "value for disabled
> state".

i'm not sure what you mean here, sorry.

> My fear is that something (uboot) modify it (keep it activated)
> before driver load.

You could have the same argument there then for the board that require
reading it. What if U-boot modified it to some non-functional state?

Either you trust the value there, and you read it, or you don't, and
then you never read it. But being stuck in between doesn't seem that
great.

> > > +static void sun8i_dwmac_dma_start_tx(void __iomem *ioaddr)
> > > +{
> > > + u32 v;
> > > +
> > > + v = readl(ioaddr + EMAC_TX_CTL0);
> > > + v |= EMAC_TX_TRANSMITTER_EN;
> > > + writel(v, ioaddr + EMAC_TX_CTL0);
> > > +
> > > + v = readl(ioaddr + EMAC_TX_CTL1);
> > > + v |= EMAC_TX_DMA_START;
> > > + v |= EMAC_TX_DMA_EN;
> > > + writel(v, ioaddr + EMAC_TX_CTL1);
> > 
> > This is a bit worrying. There's not a single lock in your driver,
> > while you have a significant number of read / modify / write.
> > 
> > Where is the locking handled?
> 
> All thoses function are handled by the "stmmac_ops framework", all
> other glue drivers does not lock anything.

Most of them seem to use regmap though, that has an internal lock.

> The few functions that need locking already got it on the calling
> stmmac side.

Ok.

> > > +
> > > + if (of_property_read_bool(priv->plat->phy_node,
> > > +   "allwinner,leds-active-low"))
> > > + reg |= H3_EPHY_LED_POL;
> > > + else
> > > + reg &= ~H3_EPHY_LED_POL;
> > > +
> > > + ret = of_mdio_parse_addr(priv->device,
> > > +  priv->plat->phy_node);
> > > + if (ret < 0) {
> > > + dev_err(priv->device, "Could not parse MDIO 
> > > addr\n");
> > > + return ret;
> > > + }
> > > + /* of_mdio_parse_addr returns a valid (0 ~ 31) PHY
> > > +  * address. No need to mask it again.
> > > +  */
> > > + reg |= ret << H3_EPHY_ADDR_SHIFT;
> > > + }
> > > + }
> > > +
> > > + if (!of_property_read_u32(node, "allwinner,tx-delay", )) {
> > 
> > How do you compute it? Can't this be done through auto-training?
> 
> The value is the same as used in vendor BSP kernel.

This is not really usable though. I've had already three boards that
never got any BSP kernel. You need to be able at least to document
some way to compute it (even if it's based on manual, trial and error
process).

> I do not understand what you mean by auto-training.

Being able to automatically detect the optimal settings at boot time.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 04/21] ARM: sun8i: dt: Add DT bindings documentation for Allwinner dwmac-sun8i

[Maxime Ripard]

Hi,

On Thu, Feb 16, 2017 at 01:48:42PM +0100, Corentin Labbe wrote:
> This patch adds documentation for Device-Tree bindings for the
> Allwinner dwmac-sun8i driver.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  .../devicetree/bindings/net/dwmac-sun8i.txt| 86 
> ++
>  1 file changed, 86 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/dwmac-sun8i.txt 
> b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> new file mode 100644
> index 000..ac806c6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/dwmac-sun8i.txt
> @@ -0,0 +1,86 @@
> +* Allwinner sun8i GMAC ethernet controller
> +
> +This device is a platform glue layer for stmmac.
> +Please see stmmac.txt for the other unchanged properties.
> +
> +Required properties:
> +- compatible: should be one of the following string:
> + "allwinner,sun8i-a83t-emac"
> + "allwinner,sun8i-h3-emac"
> + "allwinner,sun50i-a64-emac"
> +- reg: address and length of the register for the device.
> +- interrupts: interrupt for the device
> +- interrupt-names: should be "macirq"
> +- clocks: A phandle to the reference clock for this device
> +- clock-names: should be "stmmaceth"
> +- resets: A phandle to the reset control for this device
> +- reset-names: should be "stmmaceth"
> +- phy-mode: See ethernet.txt
> +- phy-handle: See ethernet.txt
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +- syscon: A phandle to the syscon of the SoC with one of the following
> + compatible string:
> +  - allwinner,sun8i-h3-system-controller
> +  - allwinner,sun8i-a64-system-controller
> +  - allwinner,sun8i-a83t-system-controller
> +
> +Optional properties:
> +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. 
> Default is 0)
> +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. 
> Default is 0)
> +Both delay properties does not have units, there are arbitrary value.
> +The TX/RX clock delay chain settings are board specific and could be found
> +in vendor FEX files.
> +
> +Optional properties for "allwinner,sun8i-h3-emac":
> +- allwinner,leds-active-low: EPHY LEDs are active low
> +
> +Required child node of emac:
> +- mdio bus node: should be named mdio
> +
> +Required properties of the mdio node:
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +
> +The device node referenced by "phy" or "phy-handle" should be a child node
> +of the mdio node. See phy.txt for the generic PHY bindings.
> +
> +Required properties of the phy node with "allwinner,sun8i-h3-emac":
> +- clocks: an extra phandle to the reference clock for the EPHY
> +- resets: an extra phandle to the reset control for the EPHY
> +
> +Required properties for the system controller:
> +- reg: address and length of the register for the device.
> +- compatible: should be "syscon" and one of the following string:
> + "allwinner,sun8i-h3-system-controller"
> + "allwinner,sun8i-a64-system-controller"
> + "allwinner,sun8i-a83t-system-controller"

This should be in a separate binding document.

What does it describe / represent?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 08/21] ARM: dts: sun8i-h3: add dwmac-sun8i rgmii pins

[Maxime Ripard]

On Thu, Feb 16, 2017 at 01:48:46PM +0100, Corentin Labbe wrote:
> This patch add pinctrl node for dwmac-sun8i on H3.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 61d56d2..59ed40e 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -349,6 +349,17 @@
>   function = "i2c2";
>   };
>  
> + emac_rgmii_pins: emac0@0 {
> + allwinner,pins = "PD0", "PD1", "PD2", "PD3",
> + "PD4", "PD5", "PD7",
> + "PD8", "PD9", "PD10",
> + "PD12", "PD13", "PD15",
> + "PD16", "PD17";
> + allwinner,function = "emac";

Please use the generic pin config properties (ie. pins and functions).

> + allwinner,drive = ;

Why do you need to use 40mA?

> + allwinner,pull = ;

This is the default now.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 05/21] net-next: stmmac: Add dwmac-sun8i

[Maxime Ripard]

low_ctrl,
> + .set_umac_addr = sun8i_dwmac_set_umac_addr,
> + .get_umac_addr = sun8i_dwmac_get_umac_addr,
> + .init_phy = sun8i_power_phy,
> + .uninit_phy = sun8i_unpower_phy,
> +};
> +
> +static struct mac_device_info *sun8i_dwmac_setup(void __iomem *ioaddr,
> +  int mcbins,
> +  int perfect_uc_entries,
> +  int *synopsys_id)
> +{
> + struct mac_device_info *mac;
> +
> + mac = kzalloc(sizeof(const struct mac_device_info), GFP_KERNEL);
> + if (!mac)
> + return NULL;

Do you ever free that memory?

> +
> + mac->pcsr = ioaddr;
> + mac->mac = _dwmac_ops;
> + mac->dma = _dwmac_dma_ops;
> +
> + mac->link.port = 0;
> + mac->link.duplex = BIT(0);
> + mac->link.speed = 1;
> + mac->mii.addr = EMAC_MDIO_CMD;
> + mac->mii.data = EMAC_MDIO_DATA;
> + mac->mii.reg_shift = 4;
> + mac->mii.reg_mask = GENMASK(8, 4);
> + mac->mii.addr_shift = 12;
> + mac->mii.addr_mask = GENMASK(16, 12);
> + mac->mii.clk_csr_shift = 20;
> + mac->mii.clk_csr_mask = GENMASK(22, 20);
> + mac->unicast_filter_entries = 8;
> +
> + /* Synopsys Id is not available */
> + *synopsys_id = 0;
> +
> + return mac;
> +}
> +
> +static int sun8i_dwmac_probe(struct platform_device *pdev)
> +{
> + struct plat_stmmacenet_data *plat_dat;
> + struct stmmac_resources stmmac_res;
> + struct sunxi_priv_data *gmac;
> + struct device *dev = >dev;
> + int ret;
> +
> + ret = stmmac_get_platform_resources(pdev, _res);
> + if (ret)
> + return ret;
> +
> + plat_dat = stmmac_probe_config_dt(pdev, _res.mac);
> + if (IS_ERR(plat_dat))
> + return PTR_ERR(plat_dat);
> +
> + gmac = devm_kzalloc(dev, sizeof(*gmac), GFP_KERNEL);
> + if (!gmac)
> + return -ENOMEM;
> +
> + gmac->variant = of_device_get_match_data(>dev);
> + if (!gmac->variant) {
> + dev_err(>dev, "Missing sun8i-emac variant\n");
> + return -EINVAL;
> + }
> +
> + gmac->tx_clk = devm_clk_get(dev, "stmmaceth");
> + if (IS_ERR(gmac->tx_clk)) {
> + dev_err(dev, "could not get tx clock\n");
> + return PTR_ERR(gmac->tx_clk);
> + }
> +
> + /* Optional regulator for PHY */
> + gmac->regulator = devm_regulator_get_optional(dev, "phy");
> + if (IS_ERR(gmac->regulator)) {
> + if (PTR_ERR(gmac->regulator) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
> + dev_info(dev, "no regulator found\n");
> + gmac->regulator = NULL;
> + }
> +
> + gmac->regmap = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +"syscon");
> + if (IS_ERR(gmac->regmap)) {
> + ret = PTR_ERR(gmac->regmap);
> + dev_err(>dev, "unable to map SYSCON:%d\n", ret);
> + return ret;
> +     }
> +
> + plat_dat->interface = of_get_phy_mode(dev->of_node);
> + if (plat_dat->interface == gmac->variant->internal_phy) {
> + dev_info(>dev, "Will use internal PHY\n");
> + gmac->use_internal_phy = true;
> + gmac->ephy_clk = of_clk_get(plat_dat->phy_node, 0);
> + if (IS_ERR(gmac->ephy_clk)) {
> + ret = PTR_ERR(gmac->ephy_clk);
> + dev_err(>dev, "Cannot get EPHY clock err=%d\n",
> + ret);
> + return -EINVAL;
> + }
> +
> + gmac->rst_ephy = of_reset_control_get(plat_dat->phy_node, NULL);
> + if (IS_ERR(gmac->rst_ephy)) {
> + ret = PTR_ERR(gmac->rst_ephy);
> + if (ret == -EPROBE_DEFER)
> + return ret;
> + dev_err(>dev, "No EPHY reset control found %d\n",
> + ret);
> + return -EINVAL;
> + }
> + } else {
> + dev_info(>dev, "Will use external PHY\n");
> + gmac->use_internal_phy = false;
> + }
> +
> + /* platform data specifying hardware features and callbacks.
> +  * hardware features were copied from Allwinner drivers.
> +  */
> + plat_dat->rx_coe = STMMAC_RX_COE_TYPE2;
> + plat_dat->tx_coe = 1;
> + plat_dat->has_sun8i = true;
> + plat_dat->bsp_priv = gmac;
> + plat_dat->init = sun8i_dwmac_init;
> + plat_dat->exit = sun8i_dwmac_exit;
> + plat_dat->setup = sun8i_dwmac_setup;
> +
> + ret = sun8i_dwmac_init(pdev, plat_dat->bsp_priv);
> + if (ret)
> + return ret;
> +
> + ret = stmmac_dvr_probe(>dev, plat_dat, _res);
> + if (ret)
> + sun8i_dwmac_exit(pdev, plat_dat->bsp_priv);
> +
> + return ret;
> +}
> +
> +static const struct of_device_id sun8i_dwmac_match[] = {
> + { .compatible = "allwinner,sun8i-h3-emac",
> + .data = _variant_h3 },
> + { .compatible = "allwinner,sun8i-a83t-emac",
> + .data = _variant_a83t },
> + { .compatible = "allwinner,sun50i-a64-emac",
> + .data = _variant_a64 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_dwmac_match);
> +
> +static struct platform_driver sun8i_dwmac_driver = {
> + .probe  = sun8i_dwmac_probe,
> + .remove = stmmac_pltfr_remove,
> + .driver = {
> + .name   = "sun8i-dwmac",
> + .pm = _pltfr_pm_ops,
> + .of_match_table = sun8i_dwmac_match,
> + },
> +};
> +module_platform_driver(sun8i_dwmac_driver);
> +
> +MODULE_AUTHOR("Corentin Labbe <clabbe.montj...@gmail.com>");
> +MODULE_DESCRIPTION("Allwinner sun8i DWMAC specific glue layer");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c 
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> index 5ff6bc4..11db658 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -450,6 +450,9 @@ static void stmmac_ethtool_gregs(struct net_device *dev,
>   for (i = 0; i < 22; i++)
>   reg_space[i + 55] =
>   readl(priv->ioaddr + (DMA_BUS_MODE + (i * 4)));
> + } else if (priv->plat->has_sun8i) {

Surely we don't want to add a new flag to the common structure for
every new platform supported.

Can't you base that on the compatible instead?

Thanks a lot for your work,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 20/21] ARM: sunxi: Enable dwmac-sun8i driver on sunxi_defconfig

[Maxime Ripard]

Hi,

On Thu, Feb 16, 2017 at 01:48:58PM +0100, Corentin Labbe wrote:
> From: LABBE Corentin <clabbe.montj...@gmail.com>
> 
> Enable the dwmac-sun8i driver in the sunxi default configuration
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/configs/sunxi_defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/configs/sunxi_defconfig 
> b/arch/arm/configs/sunxi_defconfig
> index da92c25..33bde86 100644
> --- a/arch/arm/configs/sunxi_defconfig
> +++ b/arch/arm/configs/sunxi_defconfig
> @@ -40,6 +40,7 @@ CONFIG_ATA=y
>  CONFIG_AHCI_SUNXI=y
>  CONFIG_NETDEVICES=y
>  CONFIG_SUN4I_EMAC=y
> +CONFIG_DWMAC_SUN8I=m

I think I'd prefer to have it compiled statically, just like the other
net drivers, and drivers in general.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH net 7/7] net: ethernet: stmmac: fix of-node and fixed-link-phydev leaks

[Maxime Ripard]

On Wed, Nov 30, 2016 at 03:29:55PM +0100, Johan Hovold wrote:
> Make sure to deregister and free any fixed-link phy registered during
> probe on probe errors and on driver unbind by adding a new glue helper
> function.
> 
> Drop the of-node reference taken in the same path also on late probe
> errors (and not just on driver unbind) by moving the put from
> stmmac_dvr_remove() to the new helper.
> 
> Fixes: 277323814e49 ("stmmac: add fixed-link device-tree support")
> Fixes: 4613b279bee7 ("ethernet: stmicro: stmmac: add missing of_node_put
> after calling of_parse_phandle")
> Signed-off-by: Johan Hovold <jo...@kernel.org>

For the sunxi part,
Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 2/3] net: ethernet: sun4i-emac: Allow to enable netif messages

[Maxime Ripard]

On Mon, Nov 14, 2016 at 06:58:06PM +0100, Michael Weiser wrote:
> sun4i-emac has the ability to print a number of diagnostic messages using
> dev_dbg depending on message level settings implemented using netif_msg_*
> macros. But there's no way to actually enable them.
> 
> Add the ability to switch diagnostic messages on using either a module
> parameter debug or ethtool -s  msglvl .
> 
> Signed-off-by: Michael Weiser <michael.wei...@gmx.de>
> Cc: Maxime Ripard <maxime.rip...@free-electrons.com>
> Cc: netdev@vger.kernel.org

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH 3/3] net: ethernet: sun4i-emac: Read rxhdr in CPU byte-order

[Maxime Ripard]

On Mon, Nov 14, 2016 at 06:58:07PM +0100, Michael Weiser wrote:
> The EMAC EMAC_RX_IO_DATA_REG data register is dual-purpose: On one hand
> it is used to move actual packet data off the wire. This will be in
> wire-format and accepted as such by higher layers such as IP. Therefore
> it is correctly read as-is (i.e. raw) using readsl.
> 
> On the other hand it provides metadata about incoming transfers to the
> driver such as length and checksum validation status. This data is
> little-endian, always and it is interpreted by the driver. Therefore it
> needs to be swapped to CPU endianness to make sense to the driver. This
> is already done for the "receive header" but not rxhdr.
> 
> Read rxhdr using readl in order for sun4i-emac to work correctly when
> running a big-endian kernel.
> 
> Signed-off-by: Michael Weiser <michael.wei...@gmx.de>
> Cc: Maxime Ripard <maxime.rip...@free-electrons.com>
> Cc: netdev@vger.kernel.org

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 08/10] ARM: dts: sun8i: Enable sun8i-emac on the Orange Pi 2

[Maxime Ripard]

On Wed, Oct 12, 2016 at 10:55:59AM +0200, Jean-Francois Moine wrote:
> On Fri,  7 Oct 2016 10:25:55 +0200
> Corentin Labbe <clabbe.montj...@gmail.com> wrote:
> 
> > The sun8i-emac hardware is present on the Orange PI 2.
> > It uses the internal PHY.
> > 
> > This patch create the needed emac node.
> > 
> > Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> > ---
> >  arch/arm/boot/dts/sun8i-h3-orangepi-2.dts | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts 
> > b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
> > index f93f5d1..5608eb4 100644
> > --- a/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
> > +++ b/arch/arm/boot/dts/sun8i-h3-orangepi-2.dts
> > @@ -54,6 +54,7 @@
> >  
> > aliases {
> > serial0 = 
> > +   ethernet0 = 
> 
> As there is no 'of_alias_get_id' in the driver, this alias is
> useless.

Not really, this is used by U-Boot to set the mac address.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 04/10] ARM: dts: sun8i-h3: Add dt node for the syscon control module

[Maxime Ripard]

On Mon, Oct 10, 2016 at 02:50:21PM +0200, Jean-Francois Moine wrote:
> On Mon, 10 Oct 2016 14:31:51 +0200
> Maxime Ripard <maxime.rip...@free-electrons.com> wrote:
> 
> > Hi,
> > 
> > On Fri, Oct 07, 2016 at 10:25:51AM +0200, Corentin Labbe wrote:
> > > This patch add the dt node for the syscon register present on the
> > > Allwinner H3.
> > > 
> > > Only two register are present in this syscon and the only one useful is
> > > the one dedicated to EMAC clock.
> > > 
> > > Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> > > ---
> > >  arch/arm/boot/dts/sun8i-h3.dtsi | 5 +
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi 
> > > b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > index 8a95e36..1101d2f 100644
> > > --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> > > +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> > > @@ -140,6 +140,11 @@
> > >   #size-cells = <1>;
> > >   ranges;
> > >  
> > > + syscon: syscon@01c0 {
> > > + compatible = "syscon";
> > 
> > It would be great to have a more specific compatible here in addition
> > to the syscon, like "allwinner,sun8i-h3-system-controller".
> 
> The System Control area is just like the PRCM area: it would be simpler
> to define the specific registers in the associated drivers.

Until you actually have to share those registers between different
devices, and then you're just screwed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 10/10] ARM: sunxi: Enable sun8i-emac driver on multi_v7_defconfig

[Maxime Ripard]

On Mon, Oct 10, 2016 at 03:09:43PM +0200, Jean-Francois Moine wrote:
> On Mon, 10 Oct 2016 14:35:11 +0200
> LABBE Corentin <clabbe.montj...@gmail.com> wrote:
> 
> > On Mon, Oct 10, 2016 at 02:30:46PM +0200, Maxime Ripard wrote:
> > > On Fri, Oct 07, 2016 at 10:25:57AM +0200, Corentin Labbe wrote:
> > > > Enable the sun8i-emac driver in the multi_v7 default configuration
> > > > 
> > > > Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> > > > ---
> > > >  arch/arm/configs/multi_v7_defconfig | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/arch/arm/configs/multi_v7_defconfig 
> > > > b/arch/arm/configs/multi_v7_defconfig
> > > > index 5845910..f44d633 100644
> > > > --- a/arch/arm/configs/multi_v7_defconfig
> > > > +++ b/arch/arm/configs/multi_v7_defconfig
> > > > @@ -229,6 +229,7 @@ CONFIG_NETDEVICES=y
> > > >  CONFIG_VIRTIO_NET=y
> > > >  CONFIG_HIX5HD2_GMAC=y
> > > >  CONFIG_SUN4I_EMAC=y
> > > > +CONFIG_SUN8I_EMAC=y
> > > 
> > > Any reason to build it statically?
> > 
> > No, just copied the same than CONFIG_SUN4I_EMAC that probably do
> > not need it also.
> 
> All arm configs are done the same way, and, some day, the generic ARM
> V7 kernel will not be loadable in 1Gb RAM...

Yeah, if possible, I'd really like to avoid introducing statically
built drivers to multi_v7.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 04/10] ARM: dts: sun8i-h3: Add dt node for the syscon control module

[Maxime Ripard]

Hi,

On Fri, Oct 07, 2016 at 10:25:51AM +0200, Corentin Labbe wrote:
> This patch add the dt node for the syscon register present on the
> Allwinner H3.
> 
> Only two register are present in this syscon and the only one useful is
> the one dedicated to EMAC clock.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 8a95e36..1101d2f 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -140,6 +140,11 @@
>   #size-cells = <1>;
>   ranges;
>  
> + syscon: syscon@01c0 {
> + compatible = "syscon";

It would be great to have a more specific compatible here in addition
to the syscon, like "allwinner,sun8i-h3-system-controller".

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 03/10] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

[Maxime Ripard]

On Fri, Oct 07, 2016 at 10:25:50AM +0200, Corentin Labbe wrote:
> This patch adds documentation for Device-Tree bindings for the
> Allwinner sun8i-emac driver.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  .../bindings/net/allwinner,sun8i-emac.txt  | 70 
> ++
>  1 file changed, 70 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> new file mode 100644
> index 000..92e4ef3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> @@ -0,0 +1,70 @@
> +* Allwinner sun8i EMAC ethernet controller
> +
> +Required properties:
> +- compatible: should be one of the following string:
> + "allwinner,sun8i-a83t-emac"
> + "allwinner,sun8i-h3-emac"
> + "allwinner,sun50i-a64-emac"
> +- reg: address and length of the register for the device.
> +- syscon: A phandle to the syscon of the SoC
> +- interrupts: interrupt for the device
> +- clocks: A phandle to the reference clock for this device
> +- clock-names: should be "ahb"
> +- resets: A phandle to the reset control for this device
> +- reset-names: should be "ahb"
> +- phy-mode: See ethernet.txt
> +- phy-handle: See ethernet.txt
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +
> +Optional properties:
> +- allwinner,tx-delay: TX clock delay chain value. Range value is 0-0x07. 
> Default is 0)
> +- allwinner,rx-delay: RX clock delay chain value. Range value is 0-0x1F. 
> Default is 0)
> +Both delay properties does not have units, there are arbitrary value.
> +The TX/RX clock delay chain settings are board specific and could be found
> +in vendor FEX files.
> +
> +Optional properties for "allwinner,sun8i-h3-emac":
> +- allwinner,leds-active-low: EPHY LEDs are active low
> +
> +Required child node of emac:
> +- mdio bus node: should be named mdio
> +
> +Required properties of the mdio node:
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +
> +The device node referenced by "phy" or "phy-handle" should be a child node
> +of the mdio node. See phy.txt for the generic PHY bindings.
> +
> +Required properties of the phy node with "allwinner,sun8i-h3-emac":
> +- clocks: an extra phandle to the reference clock for the EPHY
> +- resets: an extra phandle to the reset control for the EPHY
> +
> +Example:
> +
> +emac: ethernet@01c0b000 {
> + compatible = "allwinner,sun8i-h3-emac";
> + syscon = <>;
> + reg = <0x01c0b000 0x104>;
> + interrupts = ;
> + resets = < RST_BUS_EMAC>;
> + reset-names = "ahb";
> + clocks = < CLK_BUS_EMAC>;
> + clock-names = "ahb";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + phy = <_mii_phy>;
> + phy-mode = "mii";
> + allwinner,leds-active-low;
> + mdio: mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + int_mii_phy: ethernet-phy@1 {
> + reg = <1>;
> + clocks = < CLK_BUS_EPHY>;
> + resets = < RST_BUS_EPHY>;

That works for me, let's see how the DT maintainers feel about it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v4 10/10] ARM: sunxi: Enable sun8i-emac driver on multi_v7_defconfig

[Maxime Ripard]

On Fri, Oct 07, 2016 at 10:25:57AM +0200, Corentin Labbe wrote:
> Enable the sun8i-emac driver in the multi_v7 default configuration
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/configs/multi_v7_defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/configs/multi_v7_defconfig 
> b/arch/arm/configs/multi_v7_defconfig
> index 5845910..f44d633 100644
> --- a/arch/arm/configs/multi_v7_defconfig
> +++ b/arch/arm/configs/multi_v7_defconfig
> @@ -229,6 +229,7 @@ CONFIG_NETDEVICES=y
>  CONFIG_VIRTIO_NET=y
>  CONFIG_HIX5HD2_GMAC=y
>  CONFIG_SUN4I_EMAC=y
> +CONFIG_SUN8I_EMAC=y

Any reason to build it statically?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [RFC PATCH 9/9] ethernet: sun8i-emac: add pm_runtime support

[Maxime Ripard]

On Wed, Sep 14, 2016 at 04:03:04PM +0200, LABBE Corentin wrote:
> > > +static int __maybe_unused sun8i_emac_suspend(struct platform_device 
> > > *pdev, pm_message_t state)
> > > +{
> > > + struct net_device *ndev = platform_get_drvdata(pdev);
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > +
> > > + napi_disable(>napi);
> > > +
> > > + if (netif_running(ndev))
> > > + netif_device_detach(ndev);
> > > +
> > > + sun8i_emac_stop_tx(ndev);
> > > + sun8i_emac_stop_rx(ndev);
> > > +
> > > + sun8i_emac_rx_clean(ndev);
> > > + sun8i_emac_tx_clean(ndev);
> > > +
> > > + phy_stop(ndev->phydev);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int __maybe_unused sun8i_emac_resume(struct platform_device *pdev)
> > > +{
> > > + struct net_device *ndev = platform_get_drvdata(pdev);
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > +
> > > + phy_start(ndev->phydev);
> > > +
> > > + sun8i_emac_start_tx(ndev);
> > > + sun8i_emac_start_rx(ndev);
> > > +
> > > + if (netif_running(ndev))
> > > + netif_device_attach(ndev);
> > > +
> > > + netif_start_queue(ndev);
> > > +
> > > + napi_enable(>napi);
> > > +
> > > + return 0;
> > > +}
> > 
> > The main idea behind the runtime PM hooks is that they bring the
> > device to a working state and shuts it down when it's not needed
> > anymore.

Indeed.

> I expect that the first part (all pm_runtime_xxx) of the patch bring that.
> When the interface is not opened:
> cat /sys/devices/platform/soc/1c3.ethernet/power/runtime_status 
> suspended
> 
> > However, they shouldn't be called when the device is still in used, so
> > all the mangling with NAPI, the phy and so on is irrelevant here, but
> > the clocks, resets, for example, are.
> > 
> 
> I do the same as other ethernet driver for suspend/resume.

suspend / resume are used when you put the whole system into suspend,
and bring it back.

runtime_pm is only when the device is not used anymore. It makes sense
when you suspend to do whatever you're doing here. It doesn't make any
when the system is not suspended, but the device is.

> > >  static const struct of_device_id sun8i_emac_of_match_table[] = {
> > >   { .compatible = "allwinner,sun8i-a83t-emac",
> > > .data = _variant_a83t },
> > > @@ -2246,6 +2302,8 @@ static struct platform_driver sun8i_emac_driver = {
> > >   .name   = "sun8i-emac",
> > >   .of_match_table = sun8i_emac_of_match_table,
> > >   },
> > > + .suspend= sun8i_emac_suspend,
> > > + .resume = sun8i_emac_resume,
> > 
> > These are not the runtime PM hooks. How did you test that?
> > 
> 
> Anyway I didnt test suspend/resume so I will remove it until I
> successfully found how to hibernate my board.

So you submit code you never tested? That's usually a recipe for
disaster.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [RFC PATCH 9/9] ethernet: sun8i-emac: add pm_runtime support

[Maxime Ripard]

Hi,

On Fri, Sep 09, 2016 at 02:45:17PM +0200, Corentin Labbe wrote:
> This patch add pm_runtime support to sun8i-emac.
> For the moment, only basic support is added, (the device is marked as
> used when net/open)
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  drivers/net/ethernet/allwinner/sun8i-emac.c | 62 
> -
>  1 file changed, 60 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c 
> b/drivers/net/ethernet/allwinner/sun8i-emac.c
> index 1c4bc80..cce886e 100644
> --- a/drivers/net/ethernet/allwinner/sun8i-emac.c
> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
> @@ -9,7 +9,6 @@
>   * - MAC filtering
>   * - Jumbo frame
>   * - features rx-all (NETIF_F_RXALL_BIT)
> - * - PM runtime
>   */
>  #include 
>  #include 
> @@ -27,6 +26,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -1301,11 +1301,18 @@ static int sun8i_emac_open(struct net_device *ndev)
>   int err;
>   u32 v;
>  
> + err = pm_runtime_get_sync(priv->dev);
> + if (err) {
> + pm_runtime_put_noidle(priv->dev);
> + dev_err(priv->dev, "pm_runtime error: %d\n", err);
> + return err;
> + }
> +
>   err = request_irq(priv->irq, sun8i_emac_dma_interrupt, 0,
> dev_name(priv->dev), ndev);
>   if (err) {
>   dev_err(priv->dev, "Cannot request IRQ: %d\n", err);
> - return err;
> + goto err_runtime;
>   }
>  
>   /* Set interface mode (and configure internal PHY on H3) */
> @@ -1395,6 +1402,8 @@ err_syscon:
>   sun8i_emac_unset_syscon(ndev);
>  err_irq:
>   free_irq(priv->irq, ndev);
> +err_runtime:
> + pm_runtime_put(priv->dev);
>   return err;
>  }
>  
> @@ -1483,6 +1492,8 @@ static int sun8i_emac_stop(struct net_device *ndev)
>   dma_free_coherent(priv->dev, priv->nbdesc_tx * sizeof(struct dma_desc),
> priv->dd_tx, priv->dd_tx_phy);
>  
> + pm_runtime_put(priv->dev);
> +
>   return 0;
>  }
>  
> @@ -2210,6 +2221,8 @@ static int sun8i_emac_probe(struct platform_device 
> *pdev)
>   goto probe_err;
>   }
>  
> + pm_runtime_enable(priv->dev);
> +
>   return 0;
>  
>  probe_err:
> @@ -2221,6 +2234,8 @@ static int sun8i_emac_remove(struct platform_device 
> *pdev)
>  {
>   struct net_device *ndev = platform_get_drvdata(pdev);
>  
> + pm_runtime_disable(>dev);
> +
>   unregister_netdev(ndev);
>   platform_set_drvdata(pdev, NULL);
>   free_netdev(ndev);
> @@ -2228,6 +2243,47 @@ static int sun8i_emac_remove(struct platform_device 
> *pdev)
>   return 0;
>  }
>  
> +static int __maybe_unused sun8i_emac_suspend(struct platform_device *pdev, 
> pm_message_t state)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> +
> + napi_disable(>napi);
> +
> + if (netif_running(ndev))
> + netif_device_detach(ndev);
> +
> + sun8i_emac_stop_tx(ndev);
> + sun8i_emac_stop_rx(ndev);
> +
> + sun8i_emac_rx_clean(ndev);
> + sun8i_emac_tx_clean(ndev);
> +
> + phy_stop(ndev->phydev);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused sun8i_emac_resume(struct platform_device *pdev)
> +{
> + struct net_device *ndev = platform_get_drvdata(pdev);
> + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> +
> + phy_start(ndev->phydev);
> +
> + sun8i_emac_start_tx(ndev);
> + sun8i_emac_start_rx(ndev);
> +
> + if (netif_running(ndev))
> + netif_device_attach(ndev);
> +
> + netif_start_queue(ndev);
> +
> + napi_enable(>napi);
> +
> + return 0;
> +}

The main idea behind the runtime PM hooks is that they bring the
device to a working state and shuts it down when it's not needed
anymore.

However, they shouldn't be called when the device is still in used, so
all the mangling with NAPI, the phy and so on is irrelevant here, but
the clocks, resets, for example, are.

>  static const struct of_device_id sun8i_emac_of_match_table[] = {
>   { .compatible = "allwinner,sun8i-a83t-emac",
> .data = _variant_a83t },
> @@ -2246,6 +2302,8 @@ static struct platform_driver sun8i_emac_driver = {
>   .name   = "sun8i-emac",
>   .of_match_table = sun8i_emac_of_match_table,
>   },
> + .suspend= sun8i_emac_suspend,
> + .resume = sun8i_emac_resume,

These are not the runtime PM hooks. How did you test that?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 8/9] ARM: sunxi: Enable sun8i-emac driver on sunxi_defconfig

[Maxime Ripard]

Hi,

On Fri, Sep 09, 2016 at 02:45:16PM +0200, Corentin Labbe wrote:
> Enable the sun8i-emac driver in the sunxi default configuration
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>

Could you make the same patch for multi_v7 ?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 5/9] ARM: dts: sun8i-h3: add sun8i-emac ethernet driver

[Maxime Ripard]

On Fri, Sep 09, 2016 at 02:45:13PM +0200, Corentin Labbe wrote:
> The sun8i-emac is an ethernet MAC hardware that support 10/100/1000
> speed.
> 
> This patch enable the sun8i-emac on the Allwinner H3 SoC Device-tree.
> The SoC H3 have an internal PHY, so optionals syscon and ephy are set.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 19 +++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index a39da6f..a3ac476 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -50,6 +50,10 @@
>  / {
>   interrupt-parent = <>;
>  
> + aliases {
> + ethernet0 = 
> + };
> +

This needs to be done at the board level.

>   cpus {
>   #address-cells = <1>;
>   #size-cells = <0>;
> @@ -446,6 +450,21 @@
>   status = "disabled";
>   };
>  
> + emac: ethernet@1c3 {
> + compatible = "allwinner,sun8i-h3-emac";
> + syscon = <>;
> + reg = <0x01c3 0x104>;
> + reg-names = "emac";

You don't need reg-names anymore.

> + interrupts = ;
> + resets = < RST_BUS_EMAC>, < RST_BUS_EPHY>;
> + reset-names = "ahb", "ephy";
> +         clocks = < CLK_BUS_EMAC>, < CLK_BUS_EPHY>;
> + clock-names = "ahb", "ephy";

I still believe that having the same node for both the PHY and the MAC
is wrong.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v3 4/9] ARM: dts: sun8i-h3: Add dt node for the syscon control module

[Maxime Ripard]

On Fri, Sep 09, 2016 at 02:45:12PM +0200, Corentin Labbe wrote:
> This patch add the dt node for the syscon register present on the
> Allwinner H3.
> 
> Only two register are present in this syscon and the only one useful is
> the one dedicated to EMAC clock.
> 
> Signed-off-by: Corentin Labbe <clabbe.montj...@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index fdf9fdb..a39da6f 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -140,6 +140,11 @@
>   #size-cells = <1>;
>   ranges;
>  
> + syscon: syscon@01c0 {
> + compatible = "syscon";

Having our compatible would be nice here. syscon doesn't mean anything
by itself.

> + reg = <0x01c0 0x34>;

And the size of our system controller is 0x1000

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Maxime Ripard]

Hi,

On Wed, Aug 24, 2016 at 02:02:21PM +0200, LABBE Corentin wrote:
> > > +/* Set Management Data Clock, must be call after device reset */
> > > +static void sun8i_emac_set_mdc(struct net_device *ndev)
> > > +{
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > + unsigned long rate;
> > > + u32 reg;
> > > +
> > > + rate = clk_get_rate(priv->ahb_clk);
> > > + if (rate > 16000)
> > > + reg = 0x3 << 20; /* AHB / 128 */
> > > + else if (rate > 8000)
> > > + reg = 0x2 << 20; /* AHB / 64 */
> > > + else if (rate > 4000)
> > > + reg = 0x1 << 20; /* AHB / 32 */
> > > + else
> > > + reg = 0x0 << 20; /* AHB / 16 */
> > > + netif_dbg(priv, link, ndev, "MDC auto : %x\n", reg);
> > > + writel(reg, priv->base + SUN8I_EMAC_MDIO_CMD);
> > 
> > You could also expose that as a clock.
> > 
> 
> For which purpose ?
> No ethernet driver expose the MDC as clock and I dont see any interest:
> - I dont think that tuning it give any gain
> - Knowing it's value is of little interest

You don't have to implement anything, you can just register a clk_div
driver, and everything works, and you would use the proper clock APIs
(ie. clk_set_rate, and that's it).

That would be exposed just like any other clock, including in debugfs,
which would remove the need for the debug call.

But this really was just a suggestion.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Maxime Ripard]

On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote:
> >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
> >> > > +{
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 reg = 0;
> >> > > +
> >> > > + if (priv->variant == H3_EMAC)
> >> > > + reg = H3_EPHY_DEFAULT_VALUE;
> >> >
> >> > Why do you need that?
> >> >
> >> For resetting the syscon to the factory default.
> >
> > Yes, but does it matter? Does it have any side effect? Is that
> > register shared with another device?
> >
> > Otherwise, either it won't be used anymore, and you don't care, or you
> > will reload the driver later, and the driver should work whatever
> > state is programmed in there. In both cases, you don't need to reset
> > that value.
> 
> The "default" setting also disables and powers down the internal PHY.
> I think that's a good thing? The naming could be better.

Ah, it might, and that would obviously be the right thing to do. Using
a define for those enable and power down bits would be better though.

> >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
> >> > > +{
> >> > > + struct net_device *ndev = dev_id;
> >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> >> > > + u32 v, u;
> >> > > +
> >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
> >> > > +
> >> > > + /* When this bit is asserted, a frame transmission is completed. */
> >> > > + if (v & BIT(0)) {
> >> > > + priv->estats.tx_int++;
> >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN);
> >> > > + napi_schedule(>napi);
> >> > > + }
> >> > > +
> >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
> >> > > + if (v & BIT(1))
> >> > > + priv->estats.tx_dma_stop++;
> >> > > +
> >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
> >> > > +  * and TX DMA FSM is suspended.
> >> > > + */
> >> > > + if (v & BIT(2))
> >> > > + priv->estats.tx_dma_ua++;
> >> > > +
> >> > > + if (v & BIT(3))
> >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX 
> >> > > TIMEOUT\n");
> >> >
> >> > Why do you enable that interrupt if you can't handle it?
> >>
> >> Some interrupt fire even when not enabled (like 
> >> RX_BUF_UA_INT/TX_BUF_UA_INT)
> >
> > So the bits 9 and 2, respectively, in the interrupt enable register
> > are useless?
> 
> Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just
> the interrupt state showing an event? IIRC some other hardware blocks have 
> this
> behavior, such as the timer.

That's quite easy to implement, you can do a bitwise and on the status
and enable registers.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

[Maxime Ripard]

On Fri, Jul 29, 2016 at 10:15:19AM +0200, LABBE Corentin wrote:
> > > > > +See ethernet.txt in the same directory for generic bindings for 
> > > > > ethernet
> > > > > +controllers.
> > > > > +
> > > > > +The device node referenced by "phy" or "phy-handle" should be a 
> > > > > child node
> > > > > +of this node. See phy.txt for the generic PHY bindings.
> > > > > +
> > > > > +Optional properties:
> > > > > +- phy-supply: phandle to a regulator if the PHY needs one
> > > > > +- phy-io-supply: phandle to a regulator if the PHY needs a another 
> > > > > one for I/O.
> > > > > +  This is sometimes found with RGMII PHYs, which use a 
> > > > > second
> > > > > +  regulator for the lower I/O voltage.
> > > > > +- allwinner,tx-delay: The setting of the TX clock delay chain
> > > > > +- allwinner,rx-delay: The setting of the RX clock delay chain
> > > > 
> > > > In which unit? What is the default value?
> > > 
> > > The unit is unknown to me, but I have added a comment for the
> > > default and acceptable range value.
> > 
> > That's unfortunate. We'll see how the DT maintainers feel about that.
> > 
> 
> I have searched for txdelay in Documentation, and found a few driver
> that give the units (us/ps).
>
> But in that case, the value in ps/us must be found in a table
> indexed by the Xxdelay value.
>
> So the settings seems always a raw number, and for sun8i-emac
> nothing in user manual could help to find what each value is/related
> to.
> 
> So the good value is either found by "try and test" or "copy the
> value found in fex file".

What I meant was that, just like you found out already, most of the
time the properties should be in absolute units, so that it doesn't
depend on some clock rate most likely in that case.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Maxime Ripard]

On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote:
> > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int 
> > > phy_reg,
> > > + u16 data)
> > > +{
> > > + struct net_device *ndev = bus->priv;
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > + u32 reg;
> > > + int err;
> > > +
> > > + err = readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg,
> > > +  !(reg & MDIO_CMD_MII_BUSY), 100, 1);
> > > + if (err) {
> > > + dev_err(priv->dev, "%s timeout %x\n", __func__, reg);
> > > + return err;
> > > + }
> > 
> > Why the poll_timeout variant?
> > 
> Because, in case of bad clock/reset/regulator setting, the value
> expected to come could never be set.

Ah, I missed that it was for a busy bit, my bad. However, you seem to
be using that on several occasions, maybe you could turn that into a
function?

> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev)
> > > +{
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > + u32 reg = 0;
> > > +
> > > + if (priv->variant == H3_EMAC)
> > > + reg = H3_EPHY_DEFAULT_VALUE;
> > 
> > Why do you need that?
> > 
> For resetting the syscon to the factory default.

Yes, but does it matter? Does it have any side effect? Is that
register shared with another device?

Otherwise, either it won't be used anymore, and you don't care, or you
will reload the driver later, and the driver should work whatever
state is programmed in there. In both cases, you don't need to reset
that value.

> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id)
> > > +{
> > > + struct net_device *ndev = dev_id;
> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev);
> > > + u32 v, u;
> > > +
> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA);
> > > +
> > > + /* When this bit is asserted, a frame transmission is completed. */
> > > + if (v & BIT(0)) {
> > > + priv->estats.tx_int++;
> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN);
> > > + napi_schedule(>napi);
> > > + }
> > > +
> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */
> > > + if (v & BIT(1))
> > > + priv->estats.tx_dma_stop++;
> > > +
> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor
> > > +  * and TX DMA FSM is suspended.
> > > + */
> > > + if (v & BIT(2))
> > > + priv->estats.tx_dma_ua++;
> > > +
> > > + if (v & BIT(3))
> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n");
> > 
> > Why do you enable that interrupt if you can't handle it?
>
> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT)

So the bits 9 and 2, respectively, in the interrupt enable register
are useless?

> > And printing in the interrupt handler is a very bad idea.
> 
> There are printed only when DEBUG is set, so not a problem ?

It's always a problem, this adds a very significant latency and will
fill the kernel log buffer at an insane rate, flushing out actual
important messages, for no particular reason.
> > > +
> > > + return IRQ_HANDLED;
> > 
> > The lack of spinlocks in there is quite worrying.
> > 
> 
> The interrupt handler just do nothing harmfull if it race with itself.
> Just stats, enabling NAPI etc..
> Anyway, It miss a comment for that non-locking strategy

The interrupt handler cannot race with itself. The interrupts will be
masked on the local CPU and the interrupt can only be delivered to a
single CPU (so, the one that the handler is currently running from).

> > > +}
> > > +
> > > +static int sun8i_emac_probe(struct platform_device *pdev)
> > > +{
> > > + struct device_node *node = pdev->dev.of_node;
> > > + struct sun8i_emac_priv *priv;
> > > + struct net_device *ndev;
> > > + struct resource *res;
> > > + int ret;
> > > +
> > > + ret = dma_set_mask_and_coherent(>dev, DMA_BIT_MASK(32));
> > > + if (ret) {
> > > + dev_err(>dev, "No suitable DMA available\n");
> > > + return ret;
> > > + }
> > 
> > Isn't that the default?
> > 
> No, it is necessary on arm64 as apritzel requested.

http://lxr.free-electrons.com/source/drivers/of/device.c#L93

It seems to be shared between the two.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

[Maxime Ripard]

On Thu, Jul 28, 2016 at 03:40:31PM +0200, LABBE Corentin wrote:
> On Thu, Jul 21, 2016 at 09:55:19AM +0200, Maxime Ripard wrote:
> > Hi,
> > 
> > On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote:
> > > This patch adds documentation for Device-Tree bindings for the
> > > Allwinner sun8i-emac driver.
> > > 
> > > Signed-off-by: LABBE Corentin <clabbe.montj...@gmail.com>
> > > ---
> > >  .../bindings/net/allwinner,sun8i-emac.txt  | 65 
> > > ++
> > >  1 file changed, 65 insertions(+)
> > >  create mode 100644 
> > > Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > 
> > > diff --git 
> > > a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> > > b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > new file mode 100644
> > > index 000..4bf4e53
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> > > @@ -0,0 +1,65 @@
> > > +* Allwinner sun8i EMAC ethernet controller
> > > +
> > > +Required properties:
> > > +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac",
> > > + or "allwinner,sun50i-a64-emac"
> > > +- reg: address and length of the register sets for the device.
> > > +- reg-names: should be "emac" and "syscon", matching the register sets
> > 
> > Blindly mapping a register of some other device on the SoC doesn't
> > look very reasonable.
> 
> As we discuss after this mail on IRC, this register is dedicated to EMAC.

I don't think we did. It's still right in the middle of some other
hardware block register space. You actually have a syscon driver to do
just that, why not use it?

> > > +See ethernet.txt in the same directory for generic bindings for ethernet
> > > +controllers.
> > > +
> > > +The device node referenced by "phy" or "phy-handle" should be a child 
> > > node
> > > +of this node. See phy.txt for the generic PHY bindings.
> > > +
> > > +Optional properties:
> > > +- phy-supply: phandle to a regulator if the PHY needs one
> > > +- phy-io-supply: phandle to a regulator if the PHY needs a another one 
> > > for I/O.
> > > +  This is sometimes found with RGMII PHYs, which use a second
> > > +  regulator for the lower I/O voltage.
> > > +- allwinner,tx-delay: The setting of the TX clock delay chain
> > > +- allwinner,rx-delay: The setting of the RX clock delay chain
> > 
> > In which unit? What is the default value?
> 
> The unit is unknown to me, but I have added a comment for the
> default and acceptable range value.

That's unfortunate. We'll see how the DT maintainers feel about that.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver

[Maxime Ripard]

On Wed, Jul 20, 2016 at 10:03:16AM +0200, LABBE Corentin wrote:
> This patch add support for sun8i-emac ethernet MAC hardware.
> It could be found in Allwinner H3/A83T/A64 SoCs.
> 
> It supports 10/100/1000 Mbit/s speed with half/full duplex.
> It can use an internal PHY (MII 10/100) or an external PHY
> via RGMII/RMII.
> 
> Signed-off-by: LABBE Corentin 
> ---
>  drivers/net/ethernet/allwinner/Kconfig  |   13 +
>  drivers/net/ethernet/allwinner/Makefile |1 +
>  drivers/net/ethernet/allwinner/sun8i-emac.c | 2129 
> +++
>  3 files changed, 2143 insertions(+)
>  create mode 100644 drivers/net/ethernet/allwinner/sun8i-emac.c
> 
> diff --git a/drivers/net/ethernet/allwinner/Kconfig 
> b/drivers/net/ethernet/allwinner/Kconfig
> index 47da7e7..060569c 100644
> --- a/drivers/net/ethernet/allwinner/Kconfig
> +++ b/drivers/net/ethernet/allwinner/Kconfig
> @@ -33,4 +33,17 @@ config SUN4I_EMAC
>To compile this driver as a module, choose M here.  The module
>will be called sun4i-emac.
>  
> +config SUN8I_EMAC
> + tristate "Allwinner sun8i EMAC support"
> + depends on ARCH_SUNXI || COMPILE_TEST
> + depends on OF
> + select MII
> + select PHYLIB
> +---help---
> +   This driver support the sun8i EMAC ethernet driver present on
> +   H3/A83T/A64 Allwinner SoCs.
> +
> +  To compile this driver as a module, choose M here.  The module
> +  will be called sun8i-emac.
> +
>  endif # NET_VENDOR_ALLWINNER
> diff --git a/drivers/net/ethernet/allwinner/Makefile 
> b/drivers/net/ethernet/allwinner/Makefile
> index 03129f7..8bd1693c 100644
> --- a/drivers/net/ethernet/allwinner/Makefile
> +++ b/drivers/net/ethernet/allwinner/Makefile
> @@ -3,3 +3,4 @@
>  #
>  
>  obj-$(CONFIG_SUN4I_EMAC) += sun4i-emac.o
> +obj-$(CONFIG_SUN8I_EMAC) += sun8i-emac.o
> diff --git a/drivers/net/ethernet/allwinner/sun8i-emac.c 
> b/drivers/net/ethernet/allwinner/sun8i-emac.c
> new file mode 100644
> index 000..fc0c1dd
> --- /dev/null
> +++ b/drivers/net/ethernet/allwinner/sun8i-emac.c
> @@ -0,0 +1,2129 @@
> +/*
> + * sun8i-emac driver
> + *
> + * Copyright (C) 2015-2016 Corentin LABBE 
> + *
> + * This is the driver for Allwinner Ethernet MAC found in H3/A83T/A64 SoC
> + *
> + * TODO:
> + * - MAC filtering
> + * - Jumbo frame
> + * - features rx-all (NETIF_F_RXALL_BIT)
> + */
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define SUN8I_EMAC_BASIC_CTL00x00
> +#define SUN8I_EMAC_BASIC_CTL10x04
> +#define SUN8I_EMAC_INT_STA   0x08
> +#define SUN8I_EMAC_INT_EN0x0C
> +#define SUN8I_EMAC_TX_CTL0   0x10
> +#define SUN8I_EMAC_TX_CTL1   0x14
> +#define SUN8I_EMAC_TX_FLOW_CTL   0x1C
> +#define SUN8I_EMAC_RX_CTL0   0x24
> +#define SUN8I_EMAC_RX_CTL1   0x28
> +#define SUN8I_EMAC_RX_FRM_FLT0x38
> +#define SUN8I_EMAC_MDIO_CMD  0x48
> +#define SUN8I_EMAC_MDIO_DATA 0x4C
> +#define SUN8I_EMAC_TX_DMA_STA0xB0
> +#define SUN8I_EMAC_TX_CUR_DESC   0xB4
> +#define SUN8I_EMAC_TX_CUR_BUF0xB8
> +#define SUN8I_EMAC_RX_DMA_STA0xC0
> +
> +#define MDIO_CMD_MII_BUSYBIT(0)
> +#define MDIO_CMD_MII_WRITE   BIT(1)
> +#define MDIO_CMD_MII_PHY_REG_ADDR_MASK   GENMASK(8, 4)
> +#define MDIO_CMD_MII_PHY_REG_ADDR_SHIFT  4
> +#define MDIO_CMD_MII_PHY_ADDR_MASK   GENMASK(16, 12)
> +#define MDIO_CMD_MII_PHY_ADDR_SHIFT  12
> +
> +#define SUN8I_EMAC_MACADDR_HI0x50
> +#define SUN8I_EMAC_MACADDR_LO0x54
> +
> +#define SUN8I_EMAC_RX_DESC_LIST 0x34
> +#define SUN8I_EMAC_TX_DESC_LIST 0x20
> +
> +#define SUN8I_EMAC_RX_DO_CRC BIT(27)
> +#define SUN8I_EMAC_RX_STRIP_FCS BIT(28)
> +
> +#define SUN8I_COULD_BE_USED_BY_DMA BIT(31)
> +
> +/* Used in RX_CTL1*/
> +#define RX_DMA_ENBIT(30)
> +#define RX_DMA_START BIT(31)
> +/* Used in TX_CTL1*/
> +#define TX_DMA_ENBIT(30)
> +#define TX_DMA_START BIT(31)
> +
> +/* Used in RX_CTL0 */
> +#define RX_RECEIVER_EN   BIT(31)
> +/* Used in TX_CTL0 */
> +#define TX_TRANSMITTER_ENBIT(31)
> +
> +/* Basic CTL0 */
> +#define BCTL0_FD BIT(0)
> +#define BCTL0_SPEED_10   2
> +#define BCTL0_SPEED_100  3
> +#define BCTL0_SPEED_MASK GENMASK(3, 2)
> +#define BCTL0_SPEED_SHIFT2
> +
> +#define FLOW_RX 1
> +#define FLOW_TX 2
> +
> +#define RX_INT  BIT(8)
> +#define TX_INT  BIT(0)
> +
> +/* Bits used in frame RX status */
> +#define DSC_RX_FIRST BIT(9)
> +#define DSC_RX_LAST  BIT(8)
> +
> +/* Bits used in frame TX ctl */
> +#define SUN8I_EMAC_MAGIC_TX_BIT  BIT(24)
> +#define SUN8I_EMAC_TX_DO_CRC (BIT(27) | BIT(28))
> +#define DSC_TX_FIRST BIT(29)
> +#define DSC_TX_LAST  BIT(30)
> +#define SUN8I_EMAC_WANT_INT  

Re: [PATCH v2 3/5] ARM: sun8i: dt: Add DT bindings documentation for Allwinner sun8i-emac

[Maxime Ripard]

Hi,

On Wed, Jul 20, 2016 at 10:03:18AM +0200, LABBE Corentin wrote:
> This patch adds documentation for Device-Tree bindings for the
> Allwinner sun8i-emac driver.
> 
> Signed-off-by: LABBE Corentin <clabbe.montj...@gmail.com>
> ---
>  .../bindings/net/allwinner,sun8i-emac.txt  | 65 
> ++
>  1 file changed, 65 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> 
> diff --git a/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt 
> b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> new file mode 100644
> index 000..4bf4e53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/allwinner,sun8i-emac.txt
> @@ -0,0 +1,65 @@
> +* Allwinner sun8i EMAC ethernet controller
> +
> +Required properties:
> +- compatible: "allwinner,sun8i-a83t-emac", "allwinner,sun8i-h3-emac",
> + or "allwinner,sun50i-a64-emac"
> +- reg: address and length of the register sets for the device.
> +- reg-names: should be "emac" and "syscon", matching the register sets

Blindly mapping a register of some other device on the SoC doesn't
look very reasonable.

> +- interrupts: interrupt for the device
> +- clocks: A phandle to the reference clock for this device
> +- clock-names: should be "ahb"
> +- resets: A phandle to the reset control for this device
> +- reset-names: should be "ahb"
> +- phy-mode: See ethernet.txt
> +- phy or phy-handle: See ethernet.txt
> +- #address-cells: shall be 1
> +- #size-cells: shall be 0
> +
> +"allwinner,sun8i-h3-emac" also requires:
> +- clocks: an extra phandle to the reference clock for the EPHY
> +- clock-names: an extra "ephy" entry matching the clocks property
> +- resets: an extra phandle to the reset control for the EPHY
> +- resets-names: an extra "ephy" entry matching the resets property

Shouldn't that be attached to the phy itself?

> +See ethernet.txt in the same directory for generic bindings for ethernet
> +controllers.
> +
> +The device node referenced by "phy" or "phy-handle" should be a child node
> +of this node. See phy.txt for the generic PHY bindings.
> +
> +Optional properties:
> +- phy-supply: phandle to a regulator if the PHY needs one
> +- phy-io-supply: phandle to a regulator if the PHY needs a another one for 
> I/O.
> +  This is sometimes found with RGMII PHYs, which use a second
> +  regulator for the lower I/O voltage.
> +- allwinner,tx-delay: The setting of the TX clock delay chain
> +- allwinner,rx-delay: The setting of the RX clock delay chain

In which unit? What is the default value?

> +
> +The TX/RX clock delay chain settings are board specific.
> +
> +Optional properties for "allwinner,sun8i-h3-emac":
> +- allwinner,use-internal-phy: Use the H3 SoC's internal E(thernet) PHY

Can't that be derived from the presence of the phy property?

> +- allwinner,leds-active-low: EPHY LEDs are active low

That also seems PHY related. Overall, I feel like we really need a phy
node for the internal phy.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com


signature.asc
Description: PGP signature

Re: [PATCH resend] net: sun4i-emac: Properly free resources on probe failure and remove

[Maxime Ripard]

On Tue, Oct 20, 2015 at 10:42:24AM +0200, Hans de Goede wrote:
> Fix sun4i-emac not releasing the following resources:
> -iomapped memory not released on probe-failure nor on remove
> -clock not getting disabled on probe-failure nor on remove
> -sram not being released on remove
> 
> And while at it also add error checking to the clk_prepare_enable call
> done on probe.
> 
> Signed-off-by: Hans de Goede <hdego...@redhat.com>

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH] net: sun4i-emac: Properly free resources on probe failure and remove

[Maxime Ripard]

On Wed, Sep 23, 2015 at 11:39:16AM +0200, Hans de Goede wrote:
> Fix sun4i-emac not releasing the following resources:
> -iomapped memory not released on probe-failure nor on remove
> -clock not getting disabled on probe-failure nor on remove
> -sram not being released on remove
> 
> And while at it also add error checking to the clk_prepare_enable call
> done on probe.
> 
> Signed-off-by: Hans de Goede <hdego...@redhat.com>

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v8 1/3] can: Allwinner A10/A20 CAN Controller support - Devicetree bindings

[Maxime Ripard]

On Wed, Sep 16, 2015 at 01:21:19PM +0200, Gerhard Bertelsmann wrote:
> Devicetree bindings for Allwinner A10/A20 CAN
> 
> Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v8 1/3] can: Allwinner A10/A20 CAN Controller support - Devicetree bindings

[Maxime Ripard]

On Thu, Sep 17, 2015 at 10:04:56AM +0200, Marc Kleine-Budde wrote:
> On 09/16/2015 01:21 PM, Gerhard Bertelsmann wrote:
> > Devicetree bindings for Allwinner A10/A20 CAN
> > 
> > Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>
> > ---
> > 
> >  .../devicetree/bindings/net/can/sun4i_can.txt  |  38 +
> >  1 files changed, 389 insertions(+)
> > 
> > 
> > diff --git a/Documentation/devicetree/bindings/net/can/sun4i_can.txt 
> > b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> > new file mode 100644
> > index 000..cd0f50c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> > @@ -0,0 +1,38 @@
> > +Allwinner A10/A20 CAN controller Device Tree Bindings
> > +-
> > +
> > +Required properties:
> > +- compatible: "allwinner,sun4i-a10-can"
> > +- reg: physical base address and size of the Allwinner A10/A20 CAN 
> > register map.
> > +- interrupts: interrupt specifier for the sole interrupt.
> > +- clock: phandle and clock specifier.
> > +
> > +
> > +Example
> > +---
> > +
> > +SoC common .dtsi file:
> > +
> > +   can0_pins_a: can0@0 {
> > +   allwinner,pins = "PH20","PH21";
> > +   allwinner,function = "can";
> > +   allwinner,drive = <0>;
> > +   allwinner,pull = <0>;
> > +   };
> > +...
> > +   can0: can@01c2bc00 {
> > +   compatible = "allwinner,sun4i-a10-can";
> > +   reg = <0x01c2bc00 0x400>;
> > +   interrupts = <0 26 4>;
> > +   clocks = <_gates 4>;
> > +   status = "disabled";
> > +   };
> 
> What about adding this snippet to SoC where the CAN core is available?
> Maxime, what's the policy on sinxi?

It would be great, but it can come as a second step.

> If you give me an Ack I'd like to take the series via linux-can-next
> (and to net-next) upstream.

I just did so for this patch, I'll review the driver when I'll have a
bit of time.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v8 0/4] can: Allwinner A10/A20 CAN Controller support - Summary

[Maxime Ripard]

On Wed, Sep 16, 2015 at 01:21:18PM +0200, Gerhard Bertelsmann wrote:
> Hi,
> 
> please find attached the next version of my patch set. I have 
> taken all remarks from Maxime Ripard into the new version
> 
> Please review, test and report bugs if exists.
> 
> The patchset applies to all recent Kernel versions (4.x, next etc.).
> 
> [PATCH v8 1/4] Device Tree Binding Documentation
> [PATCH v8 2/4] Defconfig multi_v7
> [PATCH v8 3/4] Defconfig sunxi
> [PATCH v8 4/4] Kernel Module

Applied 3 and 4.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v8 0/4] can: Allwinner A10/A20 CAN Controller support - Summary

[Maxime Ripard]

On Thu, Sep 17, 2015 at 08:12:31PM +0200, Oliver Hartkopp wrote:
> 
> 
> On 17.09.2015 19:54, Maxime Ripard wrote:
> >On Wed, Sep 16, 2015 at 01:21:18PM +0200, Gerhard Bertelsmann wrote:
> >>Hi,
> >>
> >>please find attached the next version of my patch set. I have
> >>taken all remarks from Maxime Ripard into the new version
> >>
> >>Please review, test and report bugs if exists.
> >>
> >>The patchset applies to all recent Kernel versions (4.x, next etc.).
> >>
> >>[PATCH v8 1/4] Device Tree Binding Documentation
> >>[PATCH v8 2/4] Defconfig multi_v7
> >>[PATCH v8 3/4] Defconfig sunxi
> >>[PATCH v8 4/4] Kernel Module
> >
> >Applied 3 and 4.
> 
> Applied to what tree?
> 
> That's not the friendly way when Marc asks you about the documentation about
> the device tree (patch 1) and you commit the CAN driver and the sunxi
> defconfig (patch 3 & 4) that he mainly reviewed to whatever tree.
> 
> New CAN drivers go via can-next and net-next into mainline.
> 
> So please answer Marcs question and let him queue up the CAN driver via
> can-next himself.

Hmmm, actually, I meant 2 and 3, the two defconfig patches.

The driver and bindings should of course go through Marc's tree.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v8 4/4] can: Allwinner A10/A20 CAN Controller support - Kernel module

[Maxime Ripard]

On Wed, Sep 16, 2015 at 01:21:22PM +0200, Gerhard Bertelsmann wrote:
> Kernel module for Allwinner A10/A20 CAN
> 
> Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>

Acked-by: Maxime Ripard <maxime.rip...@free-electrons.com>

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v7 1/3] can: Allwinner A10/A20 CAN Controller support - Devicetree bindings

[Maxime Ripard]

Hi,

On Tue, Sep 15, 2015 at 01:17:10AM +0200, Gerhard Bertelsmann wrote:
> Devicetree bindings for Allwinner A10/A20 CAN
> 
> Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>
> ---
> 
>  .../devicetree/bindings/net/can/sun4i_can.txt  |  38 +
>  1 file changed, 38 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/sun4i_can.txt 
> b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> new file mode 100644
> index 000..5819043
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> @@ -0,0 +1,38 @@
> +Allwinner A10/A20 CAN controller Device Tree Bindings
> +-
> +
> +Required properties:
> +- compatible: "allwinner,sun4ican"

A bit better, but still not the one I told you to use.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v7 2/3] can: Allwinner A10/A20 CAN Controller support - Defconfigs

[Maxime Ripard]

Hi,

On Tue, Sep 15, 2015 at 01:17:11AM +0200, Gerhard Bertelsmann wrote:
> Defconfigs for Allwinner A10/A20 CAN driver
> 
> Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>
> ---
> 
>  arch/arm/configs/multi_v7_defconfig|   1 +
>  arch/arm/configs/sunxi_defconfig   |   2 +
>  2 files changed, 3 insertions(+)

Sorry if it wasn't clear, I meant one patch for each defconfig.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v7 3/3] can: Allwinner A10/A20 CAN Controller support - Kernel module

[Maxime Ripard]

/* receive interrupt */
> + while (status & SUNXI_STA_RBUF_RDY) {
> + /* RX buffer is not empty */
> + sunxi_can_rx(dev);
> + status = readl(priv->base + SUNXI_REG_STA_ADDR);
> + }
> + }
> + if (isrc &
> + (SUNXI_INT_DATA_OR | SUNXI_INT_ERR_WRN | SUNXI_INT_BUS_ERR |
> +  SUNXI_INT_ERR_PASSIVE | SUNXI_INT_ARB_LOST)) {
> + /* error interrupt */
> + if (sunxi_can_err(dev, isrc, status))
> + netdev_err(dev, "can't allocate buffer - 
> clearing pending interrupts\n");
> + }
> + /* clear interrupts */
> + writel(isrc, priv->base + SUNXI_REG_INT_ADDR);
> + readl(priv->base + SUNXI_REG_INT_ADDR);
> + }
> + if (n >= SUNXI_CAN_MAX_IRQ)
> + netdev_dbg(dev, "%d messages handled in ISR", n);
> +
> + return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static int sunxican_open(struct net_device *dev)
> +{
> + struct sunxican_priv *priv = netdev_priv(dev);
> + int err;
> +
> + /* common open */
> + err = open_candev(dev);
> + if (err)
> + return err;
> +
> + /* register interrupt handler */
> + err = request_irq(dev->irq, sunxi_can_interrupt, IRQF_SHARED,

We don't have any shared interrupt as far as I know, do you really
need this flag?

> +   dev->name, dev);
> + if (err) {
> + netdev_err(dev, "request_irq err: %d\n", err);
> + goto exit_irq;
> + }
> +
> + /* turn on clocking for CAN peripheral block */
> + err = clk_prepare_enable(priv->clk);
> + if (err) {
> + netdev_err(dev, "could not enable CAN peripheral clock\n");
> + goto exit_clock;
> + }
> +
> + err = sunxi_can_start(dev);
> + if (err) {
> + netdev_err(dev, "could not start CAN peripheral\n");
> + goto exit_can_start;
> + }
> +
> + can_led_event(dev, CAN_LED_EVENT_OPEN);
> + netif_start_queue(dev);
> +
> + return 0;
> +
> +exit_can_start:
> + clk_disable_unprepare(priv->clk);
> +exit_clock:
> + free_irq(dev->irq, dev);
> +exit_irq:
> + close_candev(dev);
> + return err;
> +}
> +
> +static int sunxican_close(struct net_device *dev)
> +{
> + struct sunxican_priv *priv = netdev_priv(dev);
> +
> + netif_stop_queue(dev);
> + sunxi_can_stop(dev);
> + clk_disable_unprepare(priv->clk);
> +
> + free_irq(dev->irq, dev);
> + close_candev(dev);
> + can_led_event(dev, CAN_LED_EVENT_STOP);
> +
> + return 0;
> +}
> +
> +static const struct net_device_ops sunxican_netdev_ops = {
> + .ndo_open = sunxican_open,
> + .ndo_stop = sunxican_close,
> + .ndo_start_xmit = sunxican_start_xmit,
> +};
> +
> +static const struct of_device_id sunxican_of_match[] = {
> + {.compatible = "allwinner,sun4ican"},

allwinner,sun4i-a10-can

> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, sunxican_of_match);
> +
> +static int sunxican_remove(struct platform_device *pdev)
> +{
> + struct net_device *dev = platform_get_drvdata(pdev);
> +
> + unregister_netdev(dev);
> + free_candev(dev);
> +
> + return 0;
> +}
> +
> +static int sunxican_probe(struct platform_device *pdev)
> +{
> + struct resource *mem;
> + struct clk *clk;
> + void __iomem *addr;
> + int err, irq;
> + struct net_device *dev;
> + struct sunxican_priv *priv;
> +
> + clk = devm_clk_get(>dev, "apb1_can");

You still don't use the DT for the lookup and use the global
name. This relies on the fact that we use clkdev, which will be
removed soon, causes issue if you have several can controllers in the
device, and makes your clock property useless.

As I said before, just passing NULL as the ID will do the right thing.

> + if (IS_ERR(clk)) {
> + dev_err(>dev, "no clock defined\n");
> + err = -ENODEV;
> + goto exit;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(>dev, "could not get a valid irq\n");
> + err = -ENODEV;
> + goto exit;
> + }
> +
> + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + addr = devm_ioremap_resource(>dev, mem);
> + if (IS_ERR(addr)) {
> + err = -EBUSY;
> + goto exit;
> + }
> +
> + dev = alloc_candev(sizeof(struct sunxican_priv), 1);
> + if (!dev) {
> + dev_err(>dev,
> + "could not allocate memory for CAN device\n");
> + err = -ENOMEM;
> + goto exit;
> + }
> +
> + dev->netdev_ops = _netdev_ops;
> + dev->irq = irq;
> + dev->flags |= IFF_ECHO;
> +
> + priv = netdev_priv(dev);
> + priv->can.clock.freq = clk_get_rate(clk);
> + priv->can.bittiming_const = _bittiming_const;
> + priv->can.do_set_mode = sunxican_set_mode;
> + priv->can.do_get_berr_counter = sunxican_get_berr_counter;
> + priv->can.ctrlmode_supported = CAN_CTRLMODE_BERR_REPORTING |
> +CAN_CTRLMODE_LISTENONLY |
> +CAN_CTRLMODE_LOOPBACK |
> +CAN_CTRLMODE_PRESUME_ACK |
> +CAN_CTRLMODE_3_SAMPLES;
> + priv->base = addr;
> + priv->clk = clk;
> + spin_lock_init(>cmdreg_lock);
> +
> + platform_set_drvdata(pdev, dev);
> + SET_NETDEV_DEV(dev, >dev);
> +
> + err = register_candev(dev);
> + if (err) {
> + dev_err(>dev, "registering %s failed (err=%d)\n",
> + DRV_NAME, err);
> + goto exit_free;
> + }
> + devm_can_led_init(dev);
> +
> + dev_info(>dev, "device registered (base=%p, irq=%d)\n",
> +  priv->base, dev->irq);
> +
> + return 0;
> +
> +exit_free:
> + free_candev(dev);
> +exit:
> + return err;
> +}
> +
> +static struct platform_driver sunxi_can_driver = {
> + .driver = {
> + .name = DRV_NAME,
> + .of_match_table = sunxican_of_match,
> + },
> + .probe = sunxican_probe,
> + .remove = sunxican_remove,
> +};
> +
> +module_platform_driver(sunxi_can_driver);
> +
> +MODULE_AUTHOR("Peter Chen <xingkon...@gmail.com>");
> +MODULE_AUTHOR("Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>");
> +MODULE_LICENSE("Dual BSD/GPL");
> +MODULE_DESCRIPTION(DRV_NAME "CAN driver for Allwinner SoCs (A10/A20)");

Looks good otherwise!

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH v6 1/3] can: Allwinner A10/A20 CAN Controller support - Devicetree bindings

[Maxime Ripard]

Hi,

On Mon, Sep 14, 2015 at 02:58:21PM +0200, Marc Kleine-Budde wrote:
> On 09/14/2015 02:54 PM, Gerhard Bertelsmann wrote:
> > Signed-off-by: Gerhard Bertelsmann <i...@gerhard-bertelsmann.de>
> > ---
> > 
> >  .../devicetree/bindings/net/can/sun4i_can.txt  |  37 +
> >  1 files changed, 37 insertions(+)
> > 
> > 
> > diff --git a/Documentation/devicetree/bindings/net/can/sun4i_can.txt 
> > b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> > new file mode 100644
> > index 000..b572e2b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/can/sun4i_can.txt
> > @@ -0,0 +1,37 @@
> > +Allwinner A10/A20 CAN controller Device Tree Bindings
> > +-
> > +
> > +Required properties:
> > +- compatible: "allwinner,sunxican"
> > +- reg: physical base address and size of the Allwinner A10/A20 CAN 
> > register map.
> > +- interrupts: interrupt specifier for the sole interrupt.
> > +- clock: phandle and clock specifier.
> > +
> > +Example
> > +---
> > +
> > +SoC common .dtsi file:
> > +
> > +   can0_pins_a: can0  0 {
> 
> That's supposed to be a proper "@":
> 
> can0@0
> 
> > +   allwinner,pins = "PH20","PH21";
> > +   allwinner,function = "can";
> > +   allwinner,drive = <0>;
> > +   allwinner,pull = <0>;
> > +   };
> > +
> > +   can0: can  01c2bc00 {
> 
> can0@01c2bc00

Actually, beside '' vs '@', he was right.

The name of the node in the DT should be:

@

Which in our case is can@01c2bc00, like he used.

> 
> > +   compatible = "allwinner,sunxican";

However, the compatible is still not right.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [linux-sunxi] Re: [PATCH] net: sun4i-emac: Claim emac sram

[Maxime Ripard]

On Mon, Aug 24, 2015 at 11:17:43AM +0200, Hans de Goede wrote:
 Hi,
 
 On 24-08-15 09:46, Maxime Ripard wrote:
 Hi Hans,
 
 On Sun, Aug 23, 2015 at 08:31:38PM +0200, Hans de Goede wrote:
 Claim the emac sram ourselves, rather then relying on the bootloader
 having mapped the sram to the emac controller during boot.
 
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
   drivers/net/ethernet/allwinner/sun4i-emac.c | 13 +++--
   1 file changed, 11 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c 
 b/drivers/net/ethernet/allwinner/sun4i-emac.c
 index bab01c84..48ce83e 100644
 --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
 +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
 @@ -28,6 +28,7 @@
   #include linux/of_platform.h
   #include linux/platform_device.h
   #include linux/phy.h
 +#include linux/soc/sunxi/sunxi_sram.h
 
   #include sun4i-emac.h
 
 @@ -857,11 +858,17 @@ static int emac_probe(struct platform_device *pdev)
 
 clk_prepare_enable(db-clk);
 
 +   ret = sunxi_sram_claim(pdev-dev);
 +   if (ret) {
 +   dev_err(pdev-dev, Error couldn't map SRAM to device\n);
 +   goto out;
 
 Shouldn't you disable you clock too?
 
 You're right, but that is a pre-existing problem, iow an unrelated
 issue.
 
 I've put doing a follow-up patch for this on my todo list.

Thanks.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH] net: sun4i-emac: Claim emac sram

[Maxime Ripard]

Hi Hans,

On Sun, Aug 23, 2015 at 08:31:38PM +0200, Hans de Goede wrote:
 Claim the emac sram ourselves, rather then relying on the bootloader
 having mapped the sram to the emac controller during boot.
 
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
  drivers/net/ethernet/allwinner/sun4i-emac.c | 13 +++--
  1 file changed, 11 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/net/ethernet/allwinner/sun4i-emac.c 
 b/drivers/net/ethernet/allwinner/sun4i-emac.c
 index bab01c84..48ce83e 100644
 --- a/drivers/net/ethernet/allwinner/sun4i-emac.c
 +++ b/drivers/net/ethernet/allwinner/sun4i-emac.c
 @@ -28,6 +28,7 @@
  #include linux/of_platform.h
  #include linux/platform_device.h
  #include linux/phy.h
 +#include linux/soc/sunxi/sunxi_sram.h
  
  #include sun4i-emac.h
  
 @@ -857,11 +858,17 @@ static int emac_probe(struct platform_device *pdev)
  
   clk_prepare_enable(db-clk);
  
 + ret = sunxi_sram_claim(pdev-dev);
 + if (ret) {
 + dev_err(pdev-dev, Error couldn't map SRAM to device\n);
 + goto out;

Shouldn't you disable you clock too?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH 6/6] net: mvneta: Statically assign queues to CPUs

[Maxime Ripard]

On Sun, Jul 05, 2015 at 03:00:11PM +0200, Willy Tarreau wrote:
 Hi Thomas,
 
 On Fri, Jul 03, 2015 at 04:46:24PM +0200, Thomas Petazzoni wrote:
  Maxime,
  
  On Fri,  3 Jul 2015 16:25:51 +0200, Maxime Ripard wrote:
  
   +static void mvneta_percpu_enable(void *arg)
   +{
   + struct mvneta_port *pp = arg;
   +
   + enable_percpu_irq(pp-dev-irq, IRQ_TYPE_NONE);
   +}
   +
static int mvneta_open(struct net_device *dev)
{
 struct mvneta_port *pp = netdev_priv(dev);
   @@ -2655,6 +2662,19 @@ static int mvneta_open(struct net_device *dev)
 goto err_cleanup_txqs;
 }

   + /*
   +  * Even though the documentation says that request_percpu_irq
   +  * doesn't enable the interrupts automatically, it actually
   +  * does so on the local CPU.
   +  *
   +  * Make sure it's disabled.
   +  */
   + disable_percpu_irq(pp-dev-irq);
   +
   + /* Enable per-CPU interrupt on the one CPU we care about */
   + smp_call_function_single(rxq_def % num_online_cpus(),
   +  mvneta_percpu_enable, pp, true);
  
  What happens if that CPU goes offline through CPU hotplug?
 
 I just tried : if I start mvneta with rxq_def=1, then my irq runs
 on CPU1. Then I offline CPU1 and the irqs are automatically handled
 by CPU0.  Then I online CPU1 and irqs stay on CPU0.

I'm confused, I would have thought that it wouldn't even work.

I guess it can be easily solved with a cpu_notifier though.

 More or less related, I found that if I enable a queue number larger
 than the CPU count it does work, but then the system complains
 during rmmod :
 
 [  877.146203] [ cut here ]
 [  877.146227] WARNING: CPU: 1 PID: 1731 at fs/proc/generic.c:552 
 remove_proc_entry+0x144/0x15c()
 [  877.146233] remove_proc_entry: removing non-empty directory 'irq/29', 
 leaking at least 'mvneta'
 [  877.146238] Modules linked in: mvneta(-) [last unloaded: mvneta]
 [  877.146254] CPU: 1 PID: 1731 Comm: rmmod Tainted: GW   
 4.1.1-mvebu-6-g3d317ed-dirty #5
 [  877.146260] Hardware name: Marvell Armada 370/XP (Device Tree)
 [  877.146281] [c0017194] (unwind_backtrace) from [c00126d4] 
 (show_stack+0x10/0x14)
 [  877.146293] [c00126d4] (show_stack) from [c04d32e4] 
 (dump_stack+0x74/0x90)
 [  877.146305] [c04d32e4] (dump_stack) from [c0025200] 
 (warn_slowpath_common+0x74/0xb0)
 [  877.146315] [c0025200] (warn_slowpath_common) from [c00252d0] 
 (warn_slowpath_fmt+0x30/0x40)
 [  877.146325] [c00252d0] (warn_slowpath_fmt) from [c0115694] 
 (remove_proc_entry+0x144/0x15c)
 [  877.146336] [c0115694] (remove_proc_entry) from [c0060fd4] 
 (unregister_irq_proc+0x8c/0xb0)
 [  877.146347] [c0060fd4] (unregister_irq_proc) from [c0059f84] 
 (free_desc+0x28/0x58)
 [  877.146356] [c0059f84] (free_desc) from [c005a028] 
 (irq_free_descs+0x44/0x80)
 [  877.146368] [c005a028] (irq_free_descs) from [bf015748] 
 (mvneta_remove+0x3c/0x4c [mvneta])
 [  877.146382] [bf015748] (mvneta_remove [mvneta]) from [c024d6e8] 
 (platform_drv_remove+0x18/0x30)
 [  877.146393] [c024d6e8] (platform_drv_remove) from [c024bd50] 
 (__device_release_driver+0x70/0xe4)
 [  877.146402] [c024bd50] (__device_release_driver) from [c024c5b8] 
 (driver_detach+0xcc/0xd0)
 [  877.146411] [c024c5b8] (driver_detach) from [c024bbe0] 
 (bus_remove_driver+0x4c/0x90)
 [  877.146425] [c024bbe0] (bus_remove_driver) from [c007d3f0] 
 (SyS_delete_module+0x164/0x1b4)
 [  877.146437] [c007d3f0] (SyS_delete_module) from [c000f4c0] 
 (ret_fast_syscall+0x0/0x3c)
 [  877.146443] ---[ end trace 48713a9ae31204b1 ]---
 
 This was on the AX3 (dual-proc) with rxq_def=2.

Hmmm, interesting, I'll look into it, thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

Re: [PATCH 4/6] net: mvneta: Handle per-cpu interrupts

[Maxime Ripard]

Hi Willy,

On Sun, Jul 05, 2015 at 02:37:08PM +0200, Willy Tarreau wrote:
 Hi Maxime,
 
 On Fri, Jul 03, 2015 at 04:25:49PM +0200, Maxime Ripard wrote:
  Now that our interrupt controller is allowing us to use per-CPU interrupts,
  actually use it in the mvneta driver.
  
  This involves obviously reworking the driver to have a CPU-local NAPI
  structure, and report for incoming packet using that structure.
  
  Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
 
 This patch breaks module build of mvneta unless you export request_percpu_irq 
 :
 
 diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
 index ec31697..1440a92 100644
 --- a/kernel/irq/manage.c
 +++ b/kernel/irq/manage.c
 @@ -1799,6 +1799,7 @@ int request_percpu_irq(unsigned int irq, irq_handler_t 
 handler,
  
   return retval;
  }
 +EXPORT_SYMBOL_GPL(request_percpu_irq);

Ah, right. Thanks!

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com


signature.asc
Description: Digital signature

[PATCH 3/6] irqchip: armada-370-xp: Rework per-cpu interrupts handling

[Maxime Ripard]

The MPIC driver currently has a list of interrupts to handle as per-cpu.

Since the timer, fabric and neta interrupts were the only per-cpu
interrupts in the system, we can now remove the switch and just check for
the hardware irq number to determine whether a given interrupt is per-cpu
or not.

Signed-off-by: Maxime Ripard maxime.rip...@free-electrons.com
---
 drivers/irqchip/irq-armada-370-xp.c | 14 --
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c 
b/drivers/irqchip/irq-armada-370-xp.c
index daccc8bdbb42..42c69bd95bf8 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -57,9 +57,6 @@
 
 #define ARMADA_370_XP_MAX_PER_CPU_IRQS (28)
 
-#define ARMADA_370_XP_TIMER0_PER_CPU_IRQ   (5)
-#define ARMADA_370_XP_FABRIC_IRQ   (3)
-
 #define IPI_DOORBELL_START  (0)
 #define IPI_DOORBELL_END(8)
 #define IPI_DOORBELL_MASK   0xFF
@@ -82,13 +79,10 @@ static phys_addr_t msi_doorbell_addr;
 
 static inline bool is_percpu_irq(irq_hw_number_t irq)
 {
-   switch (irq) {
-   case ARMADA_370_XP_TIMER0_PER_CPU_IRQ:
-   case ARMADA_370_XP_FABRIC_IRQ:
+   if (irq = ARMADA_370_XP_MAX_PER_CPU_IRQS)
return true;
-   default:
-   return false;
-   }
+
+   return false;
 }
 
 /*
@@ -552,7 +546,7 @@ static void armada_370_xp_mpic_resume(void)
if (virq == 0)
continue;
 
-   if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
+   if (!is_percpu_irq(irq))
writel(irq, per_cpu_int_base +
   ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
else
-- 
2.4.5

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html