[RFC PATCH net-next 0/2] Add Half Duplex support for ICSSG Driver

[RFC PATCH net-next 0/2] Add Half Duplex support for ICSSG Driver

[MD Danish Anwar]

This series adds support for half duplex operation for ICSSG driver.

In order to support half-duplex operation at 10M and 100M link speeds, the
PHY collision detection signal (COL) should be routed to ICSSG
GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
property, "ti,half-duplex-capable" is introduced for this purpose in the
first patch of the series. If board has PHY COL pin conencted to
PRGx_PRU1_GPIO10, this DT property can be added to eth node of ICSSG, MII
port to support half duplex operation at that port.

Second patch of the series configures driver to support half-duplex
operation if the DT property "ti,half-duplex-capable" is enabled.

This series depends on [1] which is posted as RFC.

[1] https://lore.kernel.org/all/20230830110847.1219515-1-danishanwar@ti.com/

Thanks and Regards,
Md Danish Anwar

MD Danish Anwar (2):
  dt-bindings: net: Add documentation for Half duplex support.
  net: ti: icssg-prueth: Add support for half duplex operation

 .../bindings/net/ti,icssg-prueth.yaml           |  7 +++++++
 drivers/net/ethernet/ti/icssg/icssg_config.c    | 14 ++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c    | 17 +++++++++++++++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h    |  2 ++
 4 files changed, 38 insertions(+), 2 deletions(-)

-- 
2.34.1

[RFC PATCH net-next 1/2] dt-bindings: net: Add documentation for Half duplex support.

[MD Danish Anwar]

In order to support half-duplex operation at 10M and 100M link speeds, the
PHY collision detection signal (COL) should be routed to ICSSG
GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
property, "ti,half-duplex-capable" is introduced for this purpose. If
board has PHY COL pin conencted to PRGx_PRU1_GPIO10, this DT property can
be added to eth node of ICSSG, MII port to support half duplex operation at
that port.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
index 13371159515a..59da9aeaee7e 100644
--- a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
+++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
@@ -107,6 +107,13 @@ properties:
               phandle to system controller node and register offset
               to ICSSG control register for RGMII transmit delay
 
+          ti,half-duplex-capable:
+            type: boolean
+            description:
+              Enable half duplex operation on ICSSG MII port. This requires
+              PHY output pin (COL) to be routed to ICSSG GPIO pin
+              (PRGx_PRU0/1_GPIO10) as input.
+
         required:
           - reg
     anyOf:
-- 
2.34.1

[RFC PATCH net-next 2/2] net: ti: icssg-prueth: Add support for half duplex operation

[MD Danish Anwar]

This patch adds support for half duplex operation at 10M and 100M link
speeds for AM654x/AM64x devices.
- Driver configures rand_seed, a random number, in DMEM HD_RAND_SEED_OFFSET
field, which will be used by firmware for Back off time calculation.
- Driver informs FW about half duplex link operation in DMEM
PORT_LINK_SPEED_OFFSET field by setting bit 7 for 10/100M HD.

Hence, the half duplex operation depends on board design the
"ti,half-duplex-capable" property has to be enabled for ICSS-G ports if HW
is capable to perform half duplex.

Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/net/ethernet/ti/icssg/icssg_config.c | 14 ++++++++++++++
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 17 +++++++++++++++--
 drivers/net/ethernet/ti/icssg/icssg_prueth.h |  2 ++
 3 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_config.c b/drivers/net/ethernet/ti/icssg/icssg_config.c
index 03968dbc2d62..b676d200ee90 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_config.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_config.c
@@ -540,6 +540,17 @@ int emac_set_port_state(struct prueth_emac *emac,
 	return ret;
 }
 
+void icssg_config_half_duplex(struct prueth_emac *emac)
+{
+	u32 val;
+
+	if (!emac->half_duplex)
+		return;
+
+	val = get_random_u32();
+	writel(val, emac->dram.va + HD_RAND_SEED_OFFSET);
+}
+
 void icssg_config_set_speed(struct prueth_emac *emac)
 {
 	u8 fw_speed;
@@ -560,6 +571,9 @@ void icssg_config_set_speed(struct prueth_emac *emac)
 		return;
 	}
 
+	if (emac->duplex == DUPLEX_HALF)
+		fw_speed |= FW_LINK_SPEED_HD;
+
 	writeb(fw_speed, emac->dram.va + PORT_LINK_SPEED_OFFSET);
 }
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index c2d1d0c7deb0..00e1e286e351 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -1048,6 +1048,8 @@ static void emac_adjust_link(struct net_device *ndev)
 		 * values
 		 */
 		if (emac->link) {
+			if (emac->duplex == DUPLEX_HALF)
+				icssg_config_half_duplex(emac);
 			/* Set the RGMII cfg for gig en and full duplex */
 			icssg_update_rgmii_cfg(prueth->miig_rt, emac);
 
@@ -1166,9 +1168,13 @@ static int emac_phy_connect(struct prueth_emac *emac)
 		return -ENODEV;
 	}
 
+	if (!emac->half_duplex) {
+		dev_dbg(prueth->dev, "half duplex mode is not supported\n");
+		phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+		phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+	}
+
 	/* remove unsupported modes */
-	phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
-	phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
 	phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
 	phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_Pause_BIT);
 	phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_Asym_Pause_BIT);
@@ -2454,6 +2460,10 @@ static int prueth_probe(struct platform_device *pdev)
 				      eth0_node->name);
 			goto exit_iep;
 		}
+
+		if (of_find_property(eth0_node, "ti,half-duplex-capable", NULL))
+			prueth->emac[PRUETH_MAC0]->half_duplex = 1;
+
 		prueth->emac[PRUETH_MAC0]->iep = prueth->iep0;
 	}
 
@@ -2465,6 +2475,9 @@ static int prueth_probe(struct platform_device *pdev)
 			goto netdev_exit;
 		}
 
+		if (of_find_property(eth1_node, "ti,half-duplex-capable", NULL))
+			prueth->emac[PRUETH_MAC1]->half_duplex = 1;
+
 		prueth->emac[PRUETH_MAC1]->iep = prueth->iep0;
 	}
 
diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.h b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
index 43b67213d8c7..6854ba4253c6 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.h
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.h
@@ -155,6 +155,7 @@ struct prueth_emac {
 	struct icss_iep *iep;
 	unsigned int rx_ts_enabled : 1;
 	unsigned int tx_ts_enabled : 1;
+	unsigned int half_duplex : 1;
 
 	/* DMA related */
 	struct prueth_tx_chn tx_chns[PRUETH_MAX_TX_QUEUES];
@@ -313,6 +314,7 @@ int icssg_config(struct prueth *prueth, struct prueth_emac *emac,
 int emac_set_port_state(struct prueth_emac *emac,
 			enum icssg_port_state_cmd state);
 void icssg_config_set_speed(struct prueth_emac *emac);
+void icssg_config_half_duplex(struct prueth_emac *emac);
 
 /* Buffer queue helpers */
 int icssg_queue_pop(struct prueth *prueth, u8 queue);
-- 
2.34.1

Re: [RFC PATCH net-next 1/2] dt-bindings: net: Add documentation for Half duplex support.

[Rob Herring]

On Wed, Aug 30, 2023 at 05:01:33PM +0530, MD Danish Anwar wrote:
> In order to support half-duplex operation at 10M and 100M link speeds, the
> PHY collision detection signal (COL) should be routed to ICSSG
> GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
> and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
> property, "ti,half-duplex-capable" is introduced for this purpose. If
> board has PHY COL pin conencted to PRGx_PRU1_GPIO10, this DT property can
> be added to eth node of ICSSG, MII port to support half duplex operation at
> that port.

I take it the GPIO here is not visble to the OS and that's why it's not 
described in DT?
 
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> index 13371159515a..59da9aeaee7e 100644
> --- a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
> @@ -107,6 +107,13 @@ properties:
>                phandle to system controller node and register offset
>                to ICSSG control register for RGMII transmit delay
>  
> +          ti,half-duplex-capable:

capable or...

> +            type: boolean
> +            description:
> +              Enable half duplex operation on ICSSG MII port. This requires

enable the mode?

Maybe too late if it's already been assumed not supported, but shouldn't 
supporting half duplex be the default? I guess half duplex isn't too 
common any more.

Rob

Re: [RFC PATCH net-next 1/2] dt-bindings: net: Add documentation for Half duplex support.

[Md Danish Anwar]

Hi Rob,

On 31/08/23 11:46 pm, Rob Herring wrote:
> On Wed, Aug 30, 2023 at 05:01:33PM +0530, MD Danish Anwar wrote:
>> In order to support half-duplex operation at 10M and 100M link speeds, the
>> PHY collision detection signal (COL) should be routed to ICSSG
>> GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
>> and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
>> property, "ti,half-duplex-capable" is introduced for this purpose. If
>> board has PHY COL pin conencted to PRGx_PRU1_GPIO10, this DT property can
>> be added to eth node of ICSSG, MII port to support half duplex operation at
>> that port.
> 
> I take it the GPIO here is not visble to the OS and that's why it's not 
> described in DT?
>  

Yes the GPIO here is not visible in the OS and we need to indicate whether the
PHY COL signal is routed to PRGx_PRU0/1_GPI10 pin or not by setting the
property "ti,half-duplex-capable" as true.

>>
>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> index 13371159515a..59da9aeaee7e 100644
>> --- a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>> @@ -107,6 +107,13 @@ properties:
>>                phandle to system controller node and register offset
>>                to ICSSG control register for RGMII transmit delay
>>  
>> +          ti,half-duplex-capable:
> 
> capable or...
> 
>> +            type: boolean
>> +            description:
>> +              Enable half duplex operation on ICSSG MII port. This requires
> 
> enable the mode?
> 

I think capable is good here. The property "ti,half-duplex-capable" indicates
that the board is capable of half duplex operation. This doesn't necessarily
means we have to enable the half duplex mode. The user can modify the duplex
settings from ethtool and enable / disable is controlled by the user. This
property basically let's the driver know that it can support half duplex
operations and when user enables half duplex mode through ethtool, the driver
can do the necessary configurations.

When this property is false, half duplex is not supported. If user still wants
to change the duplex mode, it will get an error saying half duplex is not
supported.

So the property "ti,half-duplex-capable" let's the driver know whether half
duplex is supported or not. Enable / disable is controlled by user through ethtool.

> Maybe too late if it's already been assumed not supported, but shouldn't 
> supporting half duplex be the default? I guess half duplex isn't too 
> common any more.
> 

Unfortunately ICSSG doesn't support half duplex by default. Routing the PHY COL
signal is necessary.

> Rob

-- 
Thanks and Regards,
Danish.

Re: [RFC PATCH net-next 2/2] net: ti: icssg-prueth: Add support for half duplex operation

[Andrew Lunn]

On Wed, Aug 30, 2023 at 05:01:34PM +0530, MD Danish Anwar wrote:
> This patch adds support for half duplex operation at 10M and 100M link
> speeds for AM654x/AM64x devices.
> - Driver configures rand_seed, a random number, in DMEM HD_RAND_SEED_OFFSET
> field, which will be used by firmware for Back off time calculation.
> - Driver informs FW about half duplex link operation in DMEM
> PORT_LINK_SPEED_OFFSET field by setting bit 7 for 10/100M HD.
> 
> Hence, the half duplex operation depends on board design the
> "ti,half-duplex-capable" property has to be enabled for ICSS-G ports if HW
> is capable to perform half duplex.
> 
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [RFC PATCH net-next 1/2] dt-bindings: net: Add documentation for Half duplex support.

[Roger Quadros]

On 01/09/2023 08:21, Md Danish Anwar wrote:
> Hi Rob,
> 
> On 31/08/23 11:46 pm, Rob Herring wrote:
>> On Wed, Aug 30, 2023 at 05:01:33PM +0530, MD Danish Anwar wrote:
>>> In order to support half-duplex operation at 10M and 100M link speeds, the
>>> PHY collision detection signal (COL) should be routed to ICSSG
>>> GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
>>> and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
>>> property, "ti,half-duplex-capable" is introduced for this purpose. If
>>> board has PHY COL pin conencted to PRGx_PRU1_GPIO10, this DT property can
>>> be added to eth node of ICSSG, MII port to support half duplex operation at
>>> that port.
>>
>> I take it the GPIO here is not visble to the OS and that's why it's not 
>> described in DT?
>>  
> 
> Yes the GPIO here is not visible in the OS and we need to indicate whether the
> PHY COL signal is routed to PRGx_PRU0/1_GPI10 pin or not by setting the
> property "ti,half-duplex-capable" as true.
> 
>>>
>>> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
>>> ---
>>>  Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml | 7 +++++++
>>>  1 file changed, 7 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>>> index 13371159515a..59da9aeaee7e 100644
>>> --- a/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>>> +++ b/Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml
>>> @@ -107,6 +107,13 @@ properties:
>>>                phandle to system controller node and register offset
>>>                to ICSSG control register for RGMII transmit delay
>>>  
>>> +          ti,half-duplex-capable:
>>
>> capable or...
>>
>>> +            type: boolean
>>> +            description:
>>> +              Enable half duplex operation on ICSSG MII port. This requires
>>
>> enable the mode?
>>
> 
> I think capable is good here. The property "ti,half-duplex-capable" indicates
> that the board is capable of half duplex operation. This doesn't necessarily
> means we have to enable the half duplex mode. The user can modify the duplex
> settings from ethtool and enable / disable is controlled by the user. This
> property basically let's the driver know that it can support half duplex
> operations and when user enables half duplex mode through ethtool, the driver
> can do the necessary configurations.
> 
> When this property is false, half duplex is not supported. If user still wants
> to change the duplex mode, it will get an error saying half duplex is not
> supported.
> 
> So the property "ti,half-duplex-capable" let's the driver know whether half
> duplex is supported or not. Enable / disable is controlled by user through ethtool.
> 
>> Maybe too late if it's already been assumed not supported, but shouldn't 
>> supporting half duplex be the default? I guess half duplex isn't too 
>> common any more.
>>
> 
> Unfortunately ICSSG doesn't support half duplex by default. Routing the PHY COL
> signal is necessary.

But the half-duplex advertising is always enabled by default. Whether it gets
used or not will depend on negotiation with link partner.

That's why you had to explicitly disable them in your next patch with

+	if (!emac->half_duplex) {
+		dev_dbg(prueth->dev, "half duplex mode is not supported\n");
+		phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
+		phy_remove_link_mode(ndev->phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
+	}

-- 
cheers,
-roger

Re: [RFC PATCH net-next 0/2] Add Half Duplex support for ICSSG Driver

[Roger Quadros]

On 30/08/2023 14:31, MD Danish Anwar wrote:
> This series adds support for half duplex operation for ICSSG driver.
> 
> In order to support half-duplex operation at 10M and 100M link speeds, the
> PHY collision detection signal (COL) should be routed to ICSSG
> GPIO pin (PRGx_PRU0/1_GPI10) so that firmware can detect collision signal
> and apply the CSMA/CD algorithm applicable for half duplex operation. A DT
> property, "ti,half-duplex-capable" is introduced for this purpose in the
> first patch of the series. If board has PHY COL pin conencted to
> PRGx_PRU1_GPIO10, this DT property can be added to eth node of ICSSG, MII
> port to support half duplex operation at that port.
> 
> Second patch of the series configures driver to support half-duplex
> operation if the DT property "ti,half-duplex-capable" is enabled.
> 
> This series depends on [1] which is posted as RFC.
> 
> [1] https://lore.kernel.org/all/20230830110847.1219515-1-danishanwar@ti.com/
> 
> Thanks and Regards,
> Md Danish Anwar
> 
> MD Danish Anwar (2):
>   dt-bindings: net: Add documentation for Half duplex support.
>   net: ti: icssg-prueth: Add support for half duplex operation
> 
>  .../bindings/net/ti,icssg-prueth.yaml           |  7 +++++++
>  drivers/net/ethernet/ti/icssg/icssg_config.c    | 14 ++++++++++++++
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c    | 17 +++++++++++++++--
>  drivers/net/ethernet/ti/icssg/icssg_prueth.h    |  2 ++
>  4 files changed, 38 insertions(+), 2 deletions(-)
> 

For this series:

Reviewed-by: Roger Quadros <rogerq@kernel.org>