Hi Andrew,

From: Andrew Lunn, Sent: Tuesday, September 27, 2022 4:18 AM

On Mon, Sep 26, 2022 at 08:12:14AM +0000, Yoshihiro Shimoda wrote:

quoted

Hi Andrew,

quoted

From: Andrew Lunn, Sent: Friday, September 23, 2022 10:12 PM

quoted

+/* Forwarding engine block (MFWD) */
+static void rswitch_fwd_init(struct rswitch_private *priv)
+{
+	int i;
+
+	for (i = 0; i < RSWITCH_NUM_HW; i++) {
+		iowrite32(FWPC0_DEFAULT, priv->addr + FWPC0(i));
+		iowrite32(0, priv->addr + FWPBFC(i));
+	}

What is RSWITCH_NUM_HW?

I think the name is unclear...
Anyway, this hardware has 3 ethernet ports and 2 CPU ports.
So that the RSWITCH_NUM_HW is 5. Perhaps, RSWITCH_NUM_ALL_PORTS
is better name.

How do the CPU ports differ to the other ports? When you mention CPU
ports, it makes me wonder if this should be a DSA driver?

I compared a DSA diagram [1] and this Ethernet Switch and then
this switch differs than the DSA diagram:
- This switch has a feature which accesses DRAM directly like an "ethernet controller".
  I called this feature as "cpu port", but it might be incorrect.
- This switch has doesn't have any "control path". Instead of that, this switch
  is controled by registers via APB (internal bus) directly.

So, IIUC, this should not be a DSA driver.

[1] https://bootlin.com/blog/tag/dsa/

Is there a public data sheet for this device?

Unfortunately, we have no public data sheet for this device.
But, I tried to figure this switch diagram about control/data paths as below:

Control path:
+--- R-Car S4-8 SoC -------------------------+
|                                            |
| CPU ---(APB bus)---+--- Ethernet Switch ---|---(MDIO)--------------+
|                    |                       |                       |
|                    +--- Ethernet SERDES    |              External Ethernet PHY --- RJ45
|                                            |
+--------------------------------------------+
Notes: The switch and SERDES have 3 ports of MDIO and SGMII.

Data Path:
+--- R-Car S4-8 SoC ---------------------------------------------------------+
|                                                                            |
| CPU ---(AXI bus)---+--- DRAM        +--------+                             |
|                    +---(cpu port)---|        |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                    |                | Switch |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                    +---(cpu port)---|        |---(ether port)--- SERDES ---|---(SGMII)--- PHY --- RJ45
|                                     +--------|                             |
+----------------------------------------------------------------------------+

The current driver only supports one of MDIO, cpu port and ethernet port, and it acts as an ethernet device.

quoted

Perhaps, since the current driver supports 1 ethernet port and 1 CPU port only,
I should modify this driver for the current condition strictly.

I would suggest you support all three user ports. For an initial
driver you don't need to support any sort of acceleration. You don't
need any hardware bridging etc. That can be added later. Just three
separated ports.

Thank you for your suggestion. However, supporting three user ports
is required to modify an external ethernet PHY driver.
(For now, a boot loader initialized the PHY, but one port only.)

The PHY is 88E2110 on my environment, so Linux has a driver in
drivers/net/phy/marvell10g.c. However, I guess this is related to
configuration of the PHY chip on the board, it needs to change
the host 7interface mode, but the driver doesn't support it for now.

So, I'd like to support three user ports later if possible...

quoted

quoted

quoted

+
+	for (i = 0; i < RSWITCH_NUM_ETHA; i++) {

RSWITCH_NUM_ETHA appears to be the number of ports?

Yes, this is number of ethernet ports.

In the DSA world we call these user ports.

I got it.
However, when I read the dsa document of Linux kernel,
it seems to call DSA ports. Perhaps, I could misunderstand the document though...
https://docs.kernel.org/networking/dsa/dsa.html

quoted

quoted

quoted

+	kfree(c->skb);
+	c->skb = NULL;

When i see code like this, i wonder why an API call like
dev_kfree_skb() is not being used. I would suggest reaming this to
something other than skb, which has a very well understood meaning.

Perhaps, c->skbs is better name than just c->skb.

Yes, that is O.K.

I got it. Thanks!

Best regards,
Yoshihiro Shimoda

     Andrew