[net-next,00/11] Add EMAC3 support for sa8540p-ride

This is a forward port / upstream refactor of code delivered
downstream by Qualcomm over at [0] to enable the DWMAC5 based
implementation called EMAC3 on the sa8540p-ride dev board.

From what I can tell with the board schematic in hand,
as well as the code delivered, the main changes needed are:

    1. A new address space layout for /dwmac5/EMAC3 MTL/DMA regs
    2. A new programming sequence required for the EMAC3 base platforms

This series makes those adaptations as well as other housekeeping items
such as converting dt-bindings to yaml, adding clock descriptions, etc.

[0] https://git.codelinaro.org/clo/la/kernel/ark-5.14/-/commit/510235ad02d7f0df478146fb00d7a4ba74821b17

Thanks,
Andrew

Bhupesh Sharma (3):
  dt-bindings: net: snps,dwmac: Update interrupt-names
  dt-bindings: net: snps,dwmac: Add Qualcomm Ethernet ETHQOS compatibles
  dt-bindings: net: qcom,ethqos: Convert bindings to yaml

Brian Masney (1):
  net: stmmac: Add EMAC3 variant of dwmac4

 .../devicetree/bindings/net/qcom,ethqos.txt   |  66 ----
 .../devicetree/bindings/net/qcom,ethqos.yaml  | 113 ++++++
 .../devicetree/bindings/net/snps,dwmac.yaml   |  11 +-
 MAINTAINERS                                   |   2 +-
 arch/arm64/boot/dts/qcom/sa8540p-ride.dts     | 181 ++++++++++
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi        |  53 +++
 drivers/clk/qcom/gcc-sc8280xp.c               |  18 +
 .../stmicro/stmmac/dwmac-qcom-ethqos.c        | 161 ++++++---
 drivers/net/ethernet/stmicro/stmmac/dwmac4.h  |  32 +-
 .../net/ethernet/stmicro/stmmac/dwmac4_core.c | 190 ++++++++--
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.c  | 336 ++++++++++++++----
 .../net/ethernet/stmicro/stmmac/dwmac4_dma.h  |  38 ++
 .../net/ethernet/stmicro/stmmac/dwmac4_lib.c  | 144 ++++++--
 drivers/net/ethernet/stmicro/stmmac/hwif.c    |  29 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |   2 +
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |   6 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  17 +-
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c |   9 +-
 .../net/ethernet/stmicro/stmmac/stmmac_ptp.c  |   4 +-
 include/dt-bindings/clock/qcom,gcc-sc8280xp.h |   2 +
 include/linux/stmmac.h                        |   1 +
 21 files changed, 1164 insertions(+), 251 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.txt
 create mode 100644 Documentation/devicetree/bindings/net/qcom,ethqos.yaml

On Mon, 13 Mar 2023 11:56:17 -0500 Andrew Halaney wrote:
> EMAC3 is a Qualcomm variant of dwmac4 that functions the same, but has a
> different address space layout for MTL and DMA registers. This makes the
> patch a bit more complicated than we would like so let's explain why the
> current approach was used.

Please drop all the static inlines in C sources, you're wrapping 
a single function call, the compiler will do the right thing.

Please no more than 6 function arguments.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index 21aaa2730ac8..9e3d8e1202bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -281,9 +281,8 @@ static int stmmac_mdio_read_c22(struct mii_bus *bus, int phyaddr, int phyreg)
>  	value |= (phyreg << priv->hw->mii.reg_shift) & priv->hw->mii.reg_mask;
>  	value |= (priv->clk_csr << priv->hw->mii.clk_csr_shift)
>  		& priv->hw->mii.clk_csr_mask;
> -	if (priv->plat->has_gmac4) {
> +	if (priv->plat->has_gmac4 || priv->plat->has_emac3)
>  		value |= MII_GMAC4_READ;
> -	}

Removing the {} is correct in terms of the coding style, but it should
be done as part of a separate patch.

	Andrew

On Mon, Mar 13, 2023 at 05:39:04PM -0700, Jakub Kicinski wrote:
> On Mon, 13 Mar 2023 11:56:17 -0500 Andrew Halaney wrote:
> > EMAC3 is a Qualcomm variant of dwmac4 that functions the same, but has a
> > different address space layout for MTL and DMA registers. This makes the
> > patch a bit more complicated than we would like so let's explain why the
> > current approach was used.
> 
> Please drop all the static inlines in C sources, you're wrapping 
> a single function call, the compiler will do the right thing.
> 
> Please no more than 6 function arguments.
> 

Thanks for the feedback! With respect to <= 6 function arguments, if I
counted right the only violation is this:

static void do_config_cbs(struct mac_device_info *hw, u32 send_slope,
			  u32 idle_slope, u32 high_credit, u32 low_credit,
			  u32 queue, u32 etsx_ctrl_base_addr,
			  u32 send_slp_credx_base_addr,
			  u32 high_credx_base_addr, u32 low_credx_base_addr,
			  void (*set_mtl_tx_queue_weight)(struct mac_device_info *hw,
							  u32 weight, u32 queue))
(...)
static void emac3_config_cbs(struct mac_device_info *hw, u32 send_slope,
				    u32 idle_slope, u32 high_credit,
				    u32 low_credit, u32 queue)

I agree, that's quite gnarly to read. the emac3_config_cbs is the
callback, so it's already at 6 arguments, so there's nothing I can
trim there. I could create some struct for readability, populate that,
then call the do_config_cbs() func with it from emac3_config_cbs.
Is that the sort of thing you want to see?

Thanks,
Andrew

On Thu, 16 Mar 2023 13:36:09 -0500 Andrew Halaney wrote:
> static void emac3_config_cbs(struct mac_device_info *hw, u32 send_slope,
> 				    u32 idle_slope, u32 high_credit,
> 				    u32 low_credit, u32 queue)
> 
> I agree, that's quite gnarly to read. the emac3_config_cbs is the
> callback, so it's already at 6 arguments, so there's nothing I can
> trim there. I could create some struct for readability, populate that,
> then call the do_config_cbs() func with it from emac3_config_cbs.
> Is that the sort of thing you want to see?

Yes, a structure is much better, because it can be initialized member
by member,

struct bla my_bla = { .this = 1, .that = 2, .and = 3, another = 4, };

That's much easier to read. A poor man's version of Python's keyword
arguments, if you will.

On Thu, Mar 16, 2023 at 11:52:34AM -0700, Jakub Kicinski wrote:
> On Thu, 16 Mar 2023 13:36:09 -0500 Andrew Halaney wrote:
> > static void emac3_config_cbs(struct mac_device_info *hw, u32 send_slope,
> > 				    u32 idle_slope, u32 high_credit,
> > 				    u32 low_credit, u32 queue)
> > 
> > I agree, that's quite gnarly to read. the emac3_config_cbs is the
> > callback, so it's already at 6 arguments, so there's nothing I can
> > trim there. I could create some struct for readability, populate that,
> > then call the do_config_cbs() func with it from emac3_config_cbs.
> > Is that the sort of thing you want to see?
> 
> Yes, a structure is much better, because it can be initialized member
> by member,
> 
> struct bla my_bla = { .this = 1, .that = 2, .and = 3, another = 4, };
> 
> That's much easier to read. A poor man's version of Python's keyword
> arguments, if you will.

What I would say is be careful with that - make sure "struct bla" is
specific to the interface being called and not generic.

I had that mistake with struct phylink_state... and there is an
endless stream of people who don't seem to bother reading the
documentation, who blindly access whatever members of that they
damn well please because it suits them, even when either they
shouldn't be writing to them, or when phylink doesn't guarantee
their contents, they read them.

As a result, I'm now of the opinion that using a struct to pass
arguments is in principle a bad idea.

There's other reasons why it's a bad idea. Many ABIs are capable of
passing arguments to functions via processor registers. As soon as
one uses a struct, they typically end up being written to memory.
Not only does that potentially cause cache line churn, it also
means that there could be more slow memory accesses that have to be
made at some point, potentially making other accesses slow.

So, all in all, I'm really not a fan of the struct approach for
all the reasons above.

On Thu, 16 Mar 2023 23:01:13 +0000 Russell King (Oracle) wrote:
> What I would say is be careful with that - make sure "struct bla" is
> specific to the interface being called and not generic.
> 
> I had that mistake with struct phylink_state... and there is an
> endless stream of people who don't seem to bother reading the
> documentation, who blindly access whatever members of that they
> damn well please because it suits them, even when either they
> shouldn't be writing to them, or when phylink doesn't guarantee
> their contents, they read them.

Right, gotta take it case by case. I really like structs for
const capabilities of the driver / device, which need to be
communicated to the core.

> As a result, I'm now of the opinion that using a struct to pass
> arguments is in principle a bad idea.
> 
> There's other reasons why it's a bad idea. Many ABIs are capable of
> passing arguments to functions via processor registers. As soon as
> one uses a struct, they typically end up being written to memory.
> Not only does that potentially cause cache line churn, it also
> means that there could be more slow memory accesses that have to be
> made at some point, potentially making other accesses slow.
> 
> So, all in all, I'm really not a fan of the struct approach for
> all the reasons above.

Also true, one has to be careful on the fast paths. There are cases
where similar set of arguments is passed multiple functions down.
Making the code hard to follow and extend. But you're right, structs
will be slower for the most part.

For stmmac I figured it can only help. The driver is touched my very
many people, it has layers and confusions...