Message-ID: <20220510185701.xa6m7pu7zd34ybgd@bang-olufsen.dk>
Date:   Tue, 10 May 2022 18:57:01 +0000
From:   Alvin Šipraga <ALSI@...g-olufsen.dk>
To:     Hauke Mehrtens <hauke@...ke-m.de>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "vivien.didelot@...il.com" <vivien.didelot@...il.com>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH 4/4] net: dsa: realtek: rtl8365mb: Add SGMII and HSGMII
 support

On Mon, May 09, 2022 at 12:48:48AM +0200, Hauke Mehrtens wrote:
> This adds support for SGMII and HSGMII on RTL8367S switches.
> HSGMII is configured using the 2500BASEX mode.
> This is baed on the rtl8367c driver found in OpenWrt.
> 
> For (H)SGMII mode we have to load a firmware into some memory which gets
> executed on the integrated 8051. The firmware binary was added as a
> array into the driver with a GPL license notice on top.
> 
> This was tested on RTL8367S (ver=0x00a0, opt=0x0001).
> 
> Signed-off-by: Hauke Mehrtens <hauke@...ke-m.de>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 356 +++++++++++++++++++++++++++-
>  1 file changed, 345 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index f9b690251155..5504a34fffeb 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2021 Alvin Šipraga <alsi@...g-olufsen.dk>
>   * Copyright (C) 2021 Michael Rasmussen <mir@...g-olufsen.dk>
> + * Copyright (C) 2022 Hauke Mehrtens <hauke@...ke-m.de>
>   *
>   * The RTL8365MB-VC is a 4+1 port 10/100/1000M switch controller. It includes 4
>   * integrated PHYs for the user facing ports, and an extension interface which
> @@ -98,6 +99,7 @@
>  #include <linux/of_irq.h>
>  #include <linux/regmap.h>
>  #include <linux/if_bridge.h>
> +#include <linux/firmware.h>
>  
>  #include "realtek.h"
>  
> @@ -135,6 +137,7 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  
>  /* Chip reset register */
>  #define RTL8365MB_CHIP_RESET_REG	0x1322
> +#define RTL8365MB_CHIP_RESET_DW8051	BIT(4)

Please stick with the convention and use 0x0010.

>  #define RTL8365MB_CHIP_RESET_SW_MASK	0x0002
>  #define RTL8365MB_CHIP_RESET_HW_MASK	0x0001
>  
> @@ -278,6 +281,29 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_DUPLEX_MASK		0x0004
>  #define   RTL8365MB_DIGITAL_INTERFACE_FORCE_SPEED_MASK		0x0003
>  
> +#define RTL8365MB_SDS_MISC			0x1d11
> +#define  RTL8365MB_CFG_SGMII_RXFC		0x4000

Please stick to the existing naming conventions of the Linux driver rather than
the Realtek (OpenWrt) one. The convention is roughly something like this:

                  reg name   _REG because it's a register 
                  vvvvvvvvvv vvv
#define RTL8365MB_COOL_STUFF_REG			0xC001 < all-caps hex
#define   RTL8365MB_COOL_STUFF_RINGTONE_SELECT_MASK	0x8000
        ^^          ^^^^^^^^^^ ^^^^^^^^^^^^^^^ ^^^^
  2 space indent    reg name   field name      _MASK because it's a mask

You are of course at liberty to use a different register name to the Realtek
driver if you think it fits better.

> +#define  RTL8365MB_CFG_SGMII_TXFC		0x2000
> +#define  RTL8365MB_INB_ARB			0x1000
> +#define  RTL8365MB_CFG_MAC8_SEL_HSGMII		0x0800
> +#define  RTL8365MB_CFG_SGMII_FDUP		0x0400
> +#define  RTL8365MB_CFG_SGMII_LINK		0x0200
> +#define  RTL8365MB_CFG_SGMII_SPD		0x0180
> +#define  RTL8365MB_CFG_MAC8_SEL_SGMII		0x0040
> +#define  RTL8365MB_CFG_INB_SEL			0x0038
> +#define  RTL8365MB_CFG_SDS_MODE_18C		0x0007
> +
> +#define RTL8365MB_SDS_INDACS_CMD		0x6600
> +#define RTL8365MB_SDS_INDACS_ADR		0x6601
> +#define RTL8365MB_SDS_INDACS_DATA		0x6602
> +
> +#define RTL8365MB_MISC_CFG0			0x130c
> +#define  RTL8365MB_MISC_CFG0_DW8051_EN		BIT(5)
> +
> +#define RTL8365MB_DW8051_RDY			0x1336
> +#define  RTL8365MB_DW8051_RDY_IROM_MSB		BIT(2)
> +#define  RTL8365MB_DW8051_RDY_ACS_IROM_EN	BIT(1)

Similar comments apply here, registers with _REG, masks with _MASK, no BIT(),
more indentation, etc.

Also, if you know a little bit about what the register(s) do, a comment is
always appreciated. For example I had a look at this register map from Realtek:

#define    RTL8367C_REG_DW8051_RDY    0x1336
#define    RTL8367C_VIAROM_WRITE_EN_OFFSET    9
#define    RTL8367C_VIAROM_WRITE_EN_MASK    0x200
#define    RTL8367C_SPIF_CK2_OFFSET    8
#define    RTL8367C_SPIF_CK2_MASK    0x100
#define    RTL8367C_RRCP_MDOE_OFFSET    7
#define    RTL8367C_RRCP_MDOE_MASK    0x80
#define    RTL8367C_DW8051_RATE_OFFSET    4
#define    RTL8367C_DW8051_RATE_MASK    0x70
#define    RTL8367C_IROM_MSB_OFFSET    2
#define    RTL8367C_IROM_MSB_MASK    0xC
#define    RTL8367C_ACS_IROM_ENABLE_OFFSET    1
#define    RTL8367C_ACS_IROM_ENABLE_MASK    0x2
#define    RTL8367C_DW8051_READY_OFFSET    0
#define    RTL8367C_DW8051_READY_MASK    0x1

Here it would appear that the register was originally called DW8051_READY
because it contained a single READY bit at offset 0. Then Realtek added some
other stuff completely unrelated to readiness. So you might want to call it
something more generic like RTL8365MB_DW8051_REG. And a comment explaining what
an 8051 is doing here, etc.

> +
>  /* CPU port mask register - controls which ports are treated as CPU ports */
>  #define RTL8365MB_CPU_PORT_MASK_REG	0x1219
>  #define   RTL8365MB_CPU_PORT_MASK_MASK	0x07FF
> @@ -296,6 +322,8 @@ static const int rtl8365mb_extint_port_map[]  = { -1, -1, -1, -1, -1, -1, 1, 2,
>  #define RTL8365MB_CFG0_MAX_LEN_REG	0x088C
>  #define   RTL8365MB_CFG0_MAX_LEN_MASK	0x3FFF
>  
> +#define RTL8365MB_BYPASS_LINE_RATE 0x03f7

Please document the type of value this can hold by specifying the relevant
masks. Also you could point out that it is only used for TMII mode (Turbo MII?).

> +
>  /* Port learning limit registers */
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_BASE		0x0A20
>  #define RTL8365MB_LUT_PORT_LEARN_LIMIT_REG(_physport) \
> @@ -493,6 +521,39 @@ static const struct rtl8365mb_jam_tbl_entry rtl8365mb_init_jam_common[] = {
>  	{ 0x1D32, 0x0002 },
>  };
>  
> +struct rtl8365mb_sds_init {
> +	unsigned int data;
> +	unsigned int addr;
> +};

I would swap the order here to be consistent with struct
rtl8365mb_jam_tbl_entry. Also make it u16 since the values are truly 16 bit.

Actually, this is just a jam table entry, just for a different register map (via
serdes indirect access). Maybe you can just follow the same model as the regular
jam tables and add a const pointer to one of your arrays below based on the
option in the rtl8365mb_detect function?

> +
> +static const struct rtl8365mb_sds_init redData[] = {

What does redData stand for? Also we don't use camelCase in the driver.

> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x21A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataSB[] = {
> +	{0x04D7, 0x0480}, {0xF994, 0x0481}, {0x31A2, 0x0482}, {0x6960, 0x0483},
> +	{0x9728, 0x0484}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData1_5_6[] = {
> +	{0x82F1, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redData8_9[] = {
> +	{0x82F1, 0x0500}, {0xF995, 0x0501}, {0x31A2, 0x0502}, {0x796C, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
> +static const struct rtl8365mb_sds_init redDataHB[] = {

How about SB/HB?

> +	{0x82F0, 0x0500}, {0xF195, 0x0501}, {0x31A2, 0x0502}, {0x7960, 0x0503},
> +	{0x9728, 0x0504}, {0x9D85, 0x0423}, {0xD810, 0x0424}, {0x0F80, 0x0001},
> +	{0x83F2, 0x002E}
> +};
> +
>  enum rtl8365mb_stp_state {
>  	RTL8365MB_STP_STATE_DISABLED = 0,
>  	RTL8365MB_STP_STATE_BLOCKING = 1,
> @@ -801,6 +862,232 @@ rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  	return DSA_TAG_PROTO_RTL8_4;
>  }
>  
> +static int rtl8365mb_sds_indacs_writee(struct realtek_priv *priv, unsigned int addr,
> +				      unsigned int data)

Likewise I would make these unsigned ints into u16's, and why not just
serdes_read/write? That it is indirect access (indacs) is not really relevant.

> +{
> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);

Sorry to drone on about naming but since we use the abbreviation addr here, the
register name ought to also be ADDR and not ADR.

> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);

Avoid magic values by populating the relevant macro fields:

#define    RTL8367C_REG_SDS_INDACS_CMD    0x6600
#define    RTL8367C_SDS_CMD_BUSY_OFFSET    8
#define    RTL8367C_SDS_CMD_BUSY_MASK    0x100
#define    RTL8367C_SDS_CMD_OFFSET    7
#define    RTL8367C_SDS_CMD_MASK    0x80
#define    RTL8367C_SDS_RWOP_OFFSET    6
#define    RTL8367C_SDS_RWOP_MASK    0x40
#define    RTL8367C_SDS_INDEX_OFFSET    0
#define    RTL8367C_SDS_INDEX_MASK    0x3F

So 0x00C0 would correspond to RWOP=1 and CMD=1 I think. This is similar to the
indirect access for the PHY registers. From rtl8365mb_phy_ocp_write():

	/* Execute write operation */
	val = FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_MASK,
			 RTL8365MB_INDIRECT_ACCESS_CTRL_CMD_VALUE) |
	      FIELD_PREP(RTL8365MB_INDIRECT_ACCESS_CTRL_RW_MASK,
			 RTL8365MB_INDIRECT_ACCESS_CTRL_RW_WRITE);
	ret = regmap_write(priv->map_nolock, RTL8365MB_INDIRECT_ACCESS_CTRL_REG,
			   val);

So maybe you can take inspiration from there.


> +}
> +
> +static int rtl8365mb_sds_indacs_read(struct realtek_priv *priv, unsigned int addr,
> +				     unsigned int *data)
> +{

By the way, we had issues with indirect access of the PHY registers colliding
with ordinary register access on SMP systems and returning nonsense data. Could
you please acquire the priv->map_lock directly in these serdes read/write
functions and use the priv->map_nolock regmap instead? See the phy access
functions for how it works or check the recent git history.

> +	int ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_ADR, addr);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_SDS_INDACS_CMD, 0x00C0);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->map, RTL8365MB_SDS_INDACS_DATA, *data);

Shouldn't this be a read? Did you test the relevant code that relies on serdes
read?

> +}
> +
> +static int rtl8365mb_ext_init_sgmii_fw(struct realtek_priv *priv)
> +{
> +	struct device *dev = priv->dev;

This variable is not really needed

> +	const struct firmware *fw;
> +	int ret;
> +	int i;
> +
> +	ret = request_firmware(&fw, "rtl_switch/rtl8367s-sgmii.bin", dev);

Since the firmware is generic to the whole family, I would name this
rtl8365mb-sgmii.bin, since we have tacitly agreed to refer to the family by
rtl8365mb. Other ideas welcome but this looks a bit odd.

Also, how about not hardcoding the FW name here but putting it in a macro?

> +	if (ret) {
> +		dev_err(dev, "failed to load firmware rtl_switch/rtl8367s-sgmii.bin, error: %i\n",
> +			ret);

AFAIK request_firmware will print errors for you, so you can just return ret; here.

> +		return ret;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +				 RTL8365MB_MISC_CFG0_DW8051_EN,
> +				 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 1));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));

Can you do this and the last write in one go? Is this timing sensitive somehow?

> +	if (ret)
> +		goto release_fw;
> +
> +	for (i = 0; i < fw->size; i++) {

Perhaps you ought to do a size check on the firmware file?

> +		ret = regmap_write(priv->map, 0xE000 + i, fw->data[i]);

Avoid magic values please :)

> +		if (ret)
> +			goto release_fw;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_IROM_MSB,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_IROM_MSB, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_DW8051_RDY,
> +				 RTL8365MB_DW8051_RDY_ACS_IROM_EN,
> +				 FIELD_PREP(RTL8365MB_DW8051_RDY_ACS_IROM_EN, 0));
> +	if (ret)
> +		goto release_fw;
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CHIP_RESET_REG,
> +				 RTL8365MB_CHIP_RESET_DW8051,
> +				 FIELD_PREP(RTL8365MB_CHIP_RESET_DW8051, 0));

It would be cool to document here in this function that you are actually loading
firmware into the 8051.

> +
> +release_fw:
> +	release_firmware(fw);
> +	return ret;

Newline before the return.

> +}
> +
> +static int rtl8365mb_ext_init_sgmii(struct realtek_priv *priv, int port, phy_interface_t interface)
> +{
> +	struct rtl8365mb *mb;
> +	int interface_mode;
> +	int sds_mode;
> +	const struct rtl8365mb_sds_init *sds_init;
> +	size_t sds_init_len;

Consider putting this in the mb struct like the jam table.

> +	int ext_int;
> +	int ret;
> +	int i;
> +	int val;
> +	int mask;

Also remember to use reverse christmas tree order.

> +
> +	mb = priv->chip_data;
> +
> +	if (mb->chip_id != RTL8365MB_CHIP_ID_8365MB_VC)
> +		return -EINVAL;

Why this check?

> +
> +	ext_int = rtl8365mb_extint_port_map[port];
> +	if (ext_int != 1)
> +		return -EINVAL;

Also this needs some explanation

> +
> +	if (interface == PHY_INTERFACE_MODE_SGMII) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_SGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_SGMII;
> +
> +		if (mb->chip_option == 0) {
> +			sds_init = redData;
> +			sds_init_len = ARRAY_SIZE(redData);
> +		} else {
> +			sds_init = redDataSB;
> +			sds_init_len = ARRAY_SIZE(redDataSB);
> +		}
> +	} else if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		sds_mode = FIELD_PREP(RTL8365MB_CFG_MAC8_SEL_HSGMII, 1);
> +		interface_mode = RTL8365MB_EXT_PORT_MODE_HSGMII;
> +
> +		if (mb->chip_option == 0) {
> +			switch (mb->chip_ver & 0x00F0) {

Rather than magic masks like 0x00F0, why not just extend the _MASKs under the
CHIP_VER_REG macro? Here is what the vendor driver says the subfields are:

#define    RTL8367C_REG_CHIP_VER    0x1301
#define    RTL8367C_VERID_OFFSET    12
#define    RTL8367C_VERID_MASK    0xF000
#define    RTL8367C_MCID_OFFSET    8
#define    RTL8367C_MCID_MASK    0xF00
#define    RTL8367C_MODEL_ID_OFFSET    4
#define    RTL8367C_MODEL_ID_MASK    0xF0
#define    RTL8367C_AFE_VERSION_OFFSET    0
#define    RTL8367C_AFE_VERSION_MASK    0x1

So here you are testing the model and picking a specific serdes init.

(Sadly Realtek said they can't provide me with a mapping of chip ID/ver value
<-> chip name, but perhaps one day we can make better sense of this...)

> +			case 0x0010:
> +			case 0x0050:
> +			case 0x0060:
> +				sds_init = redData1_5_6;
> +				sds_init_len = ARRAY_SIZE(redData1_5_6);
> +				break;
> +			case 0x0080:
> +			case 0x0090:
> +				sds_init = redData8_9;
> +				sds_init_len = ARRAY_SIZE(redData8_9);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		} else {
> +			sds_init = redDataHB;
> +			sds_init_len = ARRAY_SIZE(redDataHB);
> +		}
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < sds_init_len; i++) {
> +		ret = rtl8365mb_sds_indacs_write(priv, sds_init[i].addr, sds_init[i].data);

Try to stick to 80 columns unless it looks really ugly

> +		if (ret)
> +			return ret;
> +	}
> +
> +	mask = RTL8365MB_CFG_MAC8_SEL_SGMII | RTL8365MB_CFG_MAC8_SEL_HSGMII;
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_SDS_MISC,
> +				 mask,
> +				 sds_mode);
> +	if (ret)
> +		return ret;

Can the interface be changed at runtime with DSA? (I don't know.) If so then I
think you need to do some cleaning up in the case where (H)SGMII was configured,
and then e.g. RGMII is later configured. e.g. then this SDS_MISC bits SEL_SGMII
or SEL_HSGMII should be zeroed out.

> +
> +	mask = RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_MASK(ext_int);
> +	val = interface_mode << RTL8365MB_DIGITAL_INTERFACE_SELECT_MODE_OFFSET(ext_int);
> +	ret = regmap_update_bits(priv->map,
> +				 RTL8365MB_DIGITAL_INTERFACE_SELECT_REG(ext_int),
> +				 mask,
> +				 val);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->map, RTL8365MB_BYPASS_LINE_RATE, 0x0);
> +	if (ret)
> +		return ret;
> +
> +	/* Serdes not reset */
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0003, 0x7106);

If this is random Realtek voodoo you do not understand, please say so rather
than copying the cryptic comment from the driver. That way we know what we don't
know.

> +	if (ret)
> +		return ret;
> +
> +	return rtl8365mb_ext_init_sgmii_fw(priv);

We don't really end functions like this in the driver unless they are (nearly)
one-liners, but rather:

	ret = rtl8365mb_ext_init_sgmii_fw(priv);
	if(ret)
		return ret;

	return 0;

Not a big deal though.

> +}
> +
> +static int rtl8365mb_ext_sgmii_nway(struct realtek_priv *priv, bool state)
> +{
> +	u32 running;
> +	u32 regValue;

Reverse christmas tree & camelCase again. s/regValue/val/ per convention.

> +	int ret;
> +
> +	ret = regmap_read(priv->map, RTL8365MB_MISC_CFG0, &running);

I think this line is a good indication that the register macro is poorly named.

> +	if (running & RTL8365MB_MISC_CFG0_DW8051_EN) {
> +		ret = regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,
> +					 RTL8365MB_MISC_CFG0_DW8051_EN,
> +					 FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 0));
> +		if (ret)
> +			return ret;
> +	}

Is there any harm in just setting the bit to 0 unconditionally? And can we even
enter this function with the 8051 enabled?

> +
> +	ret = rtl8365mb_sds_indacs_read(priv, 0x0002, &regValue);
> +	if (ret)
> +		return ret;
> +
> +	if (state)
> +		regValue |= 0x0200;
> +	else
> +		regValue &= ~0x0200;
> +	regValue |= 0x0100;

Any idea what serdes register 0x0002 and its value means?

> +
> +	ret = rtl8365mb_sds_indacs_write(priv, 0x0002, regValue);
> +	if (ret)
> +		return ret;
> +	return regmap_update_bits(priv->map, RTL8365MB_MISC_CFG0,

Again please don't end functions like this but rather with a return 0;. Also you
are missing a newline.

> +				  RTL8365MB_MISC_CFG0_DW8051_EN,
> +				  FIELD_PREP(RTL8365MB_MISC_CFG0_DW8051_EN, 1));
> +}
> +
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  				      phy_interface_t interface)
>  {
> @@ -886,6 +1173,7 @@ static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  }
>  
>  static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
> +					  phy_interface_t interface,
>  					  bool link, int speed, int duplex,
>  					  bool tx_pause, bool rx_pause)
>  {
> @@ -911,7 +1199,7 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_rx_pause = rx_pause ? 1 : 0;
>  		r_tx_pause = tx_pause ? 1 : 0;
>  
> -		if (speed == SPEED_1000) {
> +		if (speed == SPEED_1000 || speed == SPEED_2500) {
>  			r_speed = RTL8365MB_PORT_SPEED_1000M;

This looks odd, maybe a comment is in order?

>  		} else if (speed == SPEED_100) {
>  			r_speed = RTL8365MB_PORT_SPEED_100M;
> @@ -941,6 +1229,25 @@ static int rtl8365mb_ext_config_forcemode(struct realtek_priv *priv, int port,
>  		r_duplex = 0;
>  	}
>  
> +	if (interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		val = FIELD_PREP(RTL8365MB_CFG_SGMII_FDUP, r_duplex) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_SPD, r_speed) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_LINK, r_link) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_TXFC, r_tx_pause) |
> +		      FIELD_PREP(RTL8365MB_CFG_SGMII_RXFC, r_rx_pause);
> +		ret = regmap_update_bits(priv->map,
> +					 RTL8365MB_SDS_MISC,
> +					 RTL8365MB_CFG_SGMII_FDUP |
> +					 RTL8365MB_CFG_SGMII_SPD |
> +					 RTL8365MB_CFG_SGMII_LINK |
> +					 RTL8365MB_CFG_SGMII_TXFC |
> +					 RTL8365MB_CFG_SGMII_RXFC,
> +					 val);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	val = FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_EN_MASK, 1) |
>  	      FIELD_PREP(RTL8365MB_DIGITAL_INTERFACE_FORCE_TXPAUSE_MASK,
>  			 r_tx_pause) |
> @@ -972,10 +1279,15 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  	     interface == PHY_INTERFACE_MODE_GMII))
>  		/* Internal PHY */
>  		return true;
> -	else if ((ext_int >= 1) &&
> -		 phy_interface_mode_is_rgmii(interface))
> +	else if ((ext_int == 1) &&

I'm again curious why you limit these changes to EXT port 1.

> +		 (phy_interface_mode_is_rgmii(interface) ||
> +		  interface == PHY_INTERFACE_MODE_SGMII ||
> +		  interface == PHY_INTERFACE_MODE_2500BASEX))
>  		/* Extension MAC */
>  		return true;
> +	else if ((ext_int >= 2) &&
> +		 phy_interface_mode_is_rgmii(interface))
> +		return true;
>  
>  	return false;
>  }
> @@ -983,14 +1295,25 @@ static bool rtl8365mb_phy_mode_supported(struct dsa_switch *ds, int port,
>  static void rtl8365mb_phylink_get_caps(struct dsa_switch *ds, int port,
>  				       struct phylink_config *config)
>  {
> -	if (dsa_is_user_port(ds, port))
> +	int ext_int = rtl8365mb_extint_port_map[port];
> +
> +	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> +				   MAC_10 | MAC_100 | MAC_1000FD;
> +
> +	if (dsa_is_user_port(ds, port)) {
>  		__set_bit(PHY_INTERFACE_MODE_INTERNAL,
>  			  config->supported_interfaces);
> -	else if (dsa_is_cpu_port(ds, port))
> +	} else if (dsa_is_cpu_port(ds, port)) {
> +		if (ext_int == 1) {
> +			__set_bit(PHY_INTERFACE_MODE_SGMII,
> +				  config->supported_interfaces);
> +			__set_bit(PHY_INTERFACE_MODE_2500BASEX,
> +				  config->supported_interfaces);
> +			config->mac_capabilities |= MAC_2500FD;
> +		}

I am not satisfied with this check because not all switches in this family
support (H)SGMII. Also testing the ext_int ID is not particularly elucidating to
the reader.

>  		phy_interface_set_rgmii(config->supported_interfaces);

Hmm, I guess this is actually not true for your RTL8367S? Isn't that
(H)SGMII-only on its first extension port? I guess this was a bug already,
though.

> +	}
>  
> -	config->mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE |
> -				   MAC_10 | MAC_100 | MAC_1000FD;
>  }
>  
>  static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
> @@ -1020,6 +1343,10 @@ static void rtl8365mb_phylink_mac_config(struct dsa_switch *ds, int port,
>  				"failed to configure RGMII mode on port %d: %d\n",
>  				port, ret);
>  		return;
> +	} else if (state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		   state->interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		rtl8365mb_ext_init_sgmii(priv, port, state->interface);
> +		rtl8365mb_ext_sgmii_nway(priv, false);
>  	}
>  
>  	/* TODO: Implement MII and RMII modes, which the RTL8365MB-VC also
> @@ -1040,8 +1367,11 @@ static void rtl8365mb_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	cancel_delayed_work_sync(&p->mib_work);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, false, 0, 0,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     false, 0, 0,
>  						     false, false);
>  		if (ret)
>  			dev_err(priv->dev,
> @@ -1068,8 +1398,11 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  	p = &mb->ports[port];
>  	schedule_delayed_work(&p->mib_work, 0);
>  
> -	if (phy_interface_mode_is_rgmii(interface)) {
> -		ret = rtl8365mb_ext_config_forcemode(priv, port, true, speed,
> +	if (phy_interface_mode_is_rgmii(interface) ||
> +	    interface == PHY_INTERFACE_MODE_SGMII ||
> +	    interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		ret = rtl8365mb_ext_config_forcemode(priv, port, interface,
> +						     true, speed,
>  						     duplex, tx_pause,
>  						     rx_pause);
>  		if (ret)
> @@ -2156,6 +2489,7 @@ const struct realtek_variant rtl8365mb_variant = {
>  };
>  EXPORT_SYMBOL_GPL(rtl8365mb_variant);
>  
> +MODULE_FIRMWARE("rtl_switch/rtl8367s-sgmii.bin");

Do you happen to know the kernel workflow for merging patches which depend on
firmware? Should the firmware land before or after this lands in net-next and/or
mainline?

Kind regards,
Alvin

>  MODULE_AUTHOR("Alvin Šipraga <alsi@...g-olufsen.dk>");
>  MODULE_DESCRIPTION("Driver for RTL8365MB-VC ethernet switch");
>  MODULE_LICENSE("GPL");
> -- 
> 2.30.2
>

Powered by blists - more mailing lists