diff mbox series

[net-next,v2,3/9] net: ethernet: oa_tc6: implement OA TC6 configuration function

Message ID 20231023154649.45931-4-Parthiban.Veerasooran@microchip.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series Add support for OPEN Alliance 10BASE-T1x MACPHY Serial Interface | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1364 this patch: 1365
netdev/cc_maintainers warning 1 maintainers not CCed: parthiban.veerasooran@microchip.com
netdev/build_clang fail Errors and warnings before: 1389 this patch: 1390
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1389 this patch: 1390
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc fail Errors and warnings before: 1 this patch: 2
netdev/source_inline success Was 0 now: 0

Commit Message

Parthiban Veerasooran Oct. 23, 2023, 3:46 p.m. UTC
Read STDCAP register for the MAC-PHY capability and check against the
configuration parameters such as chunk payload, tx cut through and rx cut
through to configure the MAC-PHY. It also configures the control command
protected/unprotected mode.

In cut through mode configuration the MAC-PHY doesn't buffer the incoming
data. In tx case, it passes the data to the network if the configured cps
of data available. In rx case, it passes the data to the SPI host if the
configured cps of data available from the network. Also disables all the
errors mask in the IMASK0 register to check for the errors.

Signed-off-by: Parthiban Veerasooran <Parthiban.Veerasooran@microchip.com>
---
 drivers/net/ethernet/oa_tc6.c | 86 ++++++++++++++++++++++++++++++++++-
 include/linux/oa_tc6.h        | 24 +++++++++-
 2 files changed, 107 insertions(+), 3 deletions(-)

Comments

Andrew Lunn Oct. 23, 2023, 10:58 p.m. UTC | #1
> +	/* Read and configure the IMASK0 register for unmasking the interrupts */
> +	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, false, false);
> +	if (ret)
> +		return ret;

Can you use oa_tc6_read_register() here? I guess the question is, what
does tc6->protect default to until it is set later in this function?
So long as it defaults to false, i guess you can use the register
read/write functions, which are a lot more readable than this generic
oa_tc6_perform_ctrl().

> +
> +	regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
> +
> +	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, true, false);
> +	if (ret)
> +		return ret;
> +
> +	/* Read STDCAP register to get the MAC-PHY standard capabilities */
> +	ret = oa_tc6_perform_ctrl(tc6, STDCAP, &regval, 1, false, false);
> +	if (ret)
> +		return ret;
> +
> +	mincps = FIELD_GET(MINCPS, regval);
> +	ctc = (regval & CTC) ? true : false;
> +
> +	regval = 0;
> +	oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
> +	if (oa_node) {
> +		/* Read OA parameters from DT */
> +		if (of_property_present(oa_node, "oa-cps")) {
> +			ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);

If of_property_read_u32() does not find the property, it is documented
to not touch tc6->cps. So you can set tc6->cps to the default 64,
before the big if, and skip the of_property_present(). You can then
probably remove the else at the end as well.

> +			if (ret < 0)
> +				return ret;
> +			/* Return error if the configured cps is less than the
> +			 * minimum cps supported by the MAC-PHY.
> +			 */
> +			if (tc6->cps < mincps)
> +				return -ENODEV;

A dev_err() would be nice here to indicate why.

> +		} else {
> +			tc6->cps = 64;
> +		}
> +		if (of_property_present(oa_node, "oa-txcte")) {
> +			/* Return error if the tx cut through mode is configured
> +			 * but it is not supported by MAC-PHY.
> +			 */
> +			if (ctc)
> +				regval |= TXCTE;
> +			else
> +				return -ENODEV;

and a dev_err() here as well.

> +		}
> +		if (of_property_present(oa_node, "oa-rxcte")) {
> +			/* Return error if the rx cut through mode is configured
> +			 * but it is not supported by MAC-PHY.
> +			 */
> +			if (ctc)
> +				regval |= RXCTE;
> +			else
> +				return -ENODEV;
> +		}

and another dev_err(). Without these prints, you probably need to
modify the code to figure out why the probe failed.

> +		if (of_property_present(oa_node, "oa-prote")) {
> +			regval |= PROTE;
> +			tc6->prote = true;
> +		}
> +	} else {
> +		tc6->cps = 64;
> +	}
> +
> +	regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
> +
> +	return oa_tc6_perform_ctrl(tc6, CONFIG0, &regval, 1, true, false);
> +}
> +
>  static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
>  {
>  	u32 regval;
> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
>   * Returns pointer reference to the oa_tc6 structure if all the memory
>   * allocation success otherwise NULL.
>   */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)

Was there a reason to have prote initially, and then remove it here?

    Andrew
Parthiban Veerasooran Oct. 25, 2023, 12:02 p.m. UTC | #2
Hi Andrew,

On 24/10/23 4:28 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> +     /* Read and configure the IMASK0 register for unmasking the interrupts */
>> +     ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, false, false);
>> +     if (ret)
>> +             return ret;
> 
> Can you use oa_tc6_read_register() here? I guess the question is, what
> does tc6->protect default to until it is set later in this function?
> So long as it defaults to false, i guess you can use the register
> read/write functions, which are a lot more readable than this generic
> oa_tc6_perform_ctrl().
Yes, I will do that. Also for next two calls as well.
> 
>> +
>> +     regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
>> +
>> +     ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, true, false);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Read STDCAP register to get the MAC-PHY standard capabilities */
>> +     ret = oa_tc6_perform_ctrl(tc6, STDCAP, &regval, 1, false, false);
>> +     if (ret)
>> +             return ret;
>> +
>> +     mincps = FIELD_GET(MINCPS, regval);
>> +     ctc = (regval & CTC) ? true : false;
>> +
>> +     regval = 0;
>> +     oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
>> +     if (oa_node) {
>> +             /* Read OA parameters from DT */
>> +             if (of_property_present(oa_node, "oa-cps")) {
>> +                     ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
> 
> If of_property_read_u32() does not find the property, it is documented
> to not touch tc6->cps. So you can set tc6->cps to the default 64,
> before the big if, and skip the of_property_present(). You can then
> probably remove the else at the end as well.
Ah ok, will do that.
> 
>> +                     if (ret < 0)
>> +                             return ret;
>> +                     /* Return error if the configured cps is less than the
>> +                      * minimum cps supported by the MAC-PHY.
>> +                      */
>> +                     if (tc6->cps < mincps)
>> +                             return -ENODEV;
> 
> A dev_err() would be nice here to indicate why.
Ok sure.
> 
>> +             } else {
>> +                     tc6->cps = 64;
>> +             }
>> +             if (of_property_present(oa_node, "oa-txcte")) {
>> +                     /* Return error if the tx cut through mode is configured
>> +                      * but it is not supported by MAC-PHY.
>> +                      */
>> +                     if (ctc)
>> +                             regval |= TXCTE;
>> +                     else
>> +                             return -ENODEV;
> 
> and a dev_err() here as well.
Ok sure.
> 
>> +             }
>> +             if (of_property_present(oa_node, "oa-rxcte")) {
>> +                     /* Return error if the rx cut through mode is configured
>> +                      * but it is not supported by MAC-PHY.
>> +                      */
>> +                     if (ctc)
>> +                             regval |= RXCTE;
>> +                     else
>> +                             return -ENODEV;
>> +             }
> 
> and another dev_err(). Without these prints, you probably need to
> modify the code to figure out why the probe failed.
Yes I understand. Will do that in the next revision.
> 
>> +             if (of_property_present(oa_node, "oa-prote")) {
>> +                     regval |= PROTE;
>> +                     tc6->prote = true;
>> +             }
>> +     } else {
>> +             tc6->cps = 64;
>> +     }
>> +
>> +     regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
>> +
>> +     return oa_tc6_perform_ctrl(tc6, CONFIG0, &regval, 1, true, false);
>> +}
>> +
>>   static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
>>   {
>>        u32 regval;
>> @@ -310,7 +387,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
>>    * Returns pointer reference to the oa_tc6 structure if all the memory
>>    * allocation success otherwise NULL.
>>    */
>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> 
> Was there a reason to have prote initially, and then remove it here?
The reason is, control communication uses "protect". But in the first 
patch there was no dt used. Later in this patch, dt used for all the 
configuration parameters and this also part of that. That's why removed 
and moved this to dt configuration.

What's your opinion? shall I keep as it is like this? or remove the 
protect in the first two patches and introduce in this patch?

Best Regards,
Parthiban V
> 
>      Andrew
Andrew Lunn Oct. 26, 2023, 8:06 p.m. UTC | #3
> >> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
> >> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
> > 
> > Was there a reason to have prote initially, and then remove it here?
> The reason is, control communication uses "protect". But in the first 
> patch there was no dt used. Later in this patch, dt used for all the 
> configuration parameters and this also part of that. That's why removed 
> and moved this to dt configuration.
> 
> What's your opinion? shall I keep as it is like this? or remove the 
> protect in the first two patches and introduce in this patch?

It will actually depend on what goes into the DT binding. If using
protections costs very little, i would just hard code it on. Maybe you
can run some iperf tests and see if it makes a measurable difference.

How fast an SPI bus are you using on your development board? If you
have a 50Mbps SPI bus, it does not even matter, since the media
bandwidth is just 10Mbps.

    Andrew
Parthiban Veerasooran Oct. 27, 2023, 7:10 a.m. UTC | #4
Hi Andrew,

On 27/10/23 1:36 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
>>>> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
>>>
>>> Was there a reason to have prote initially, and then remove it here?
>> The reason is, control communication uses "protect". But in the first
>> patch there was no dt used. Later in this patch, dt used for all the
>> configuration parameters and this also part of that. That's why removed
>> and moved this to dt configuration.
>>
>> What's your opinion? shall I keep as it is like this? or remove the
>> protect in the first two patches and introduce in this patch?
> 
> It will actually depend on what goes into the DT binding. If using
> protections costs very little, i would just hard code it on. Maybe you
> can run some iperf tests and see if it makes a measurable difference.
> 
> How fast an SPI bus are you using on your development board? If you
> have a 50Mbps SPI bus, it does not even matter, since the media
> bandwidth is just 10Mbps.
Actually protection is only used for control communication to read/write 
registers. It is not used in data communication where ethernet frame 
transfer performed. So it doesn't hurt data traffic. But of course in 
between data communication I may perform some control transfer for 
register read/write but they are not big and will not affect the speed. 
In my development board I use 15MHz speed SPI bus.

As this is given as a configurable parameter in the OPEN Alliance 
specification, I have implemented it in the DT binding for user 
input/choice.

Best Regards,
Parthiban V
> 
>      Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index e4457569135f..9a98d59f286d 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -8,6 +8,7 @@ 
 #include <linux/bitfield.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
+#include <linux/of.h>
 #include <linux/oa_tc6.h>
 
 /* Opaque structure for MACPHY drivers */
@@ -16,6 +17,7 @@  struct oa_tc6 {
 	u8 *ctrl_tx_buf;
 	u8 *ctrl_rx_buf;
 	bool prote;
+	u32 cps;
 };
 
 static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8 *prx, u16 len)
@@ -198,6 +200,81 @@  static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
 	return ret;
 }
 
+static int oa_tc6_configure(struct oa_tc6 *tc6)
+{
+	struct spi_device *spi = tc6->spi;
+	struct device_node *oa_node;
+	u32 regval;
+	u8 mincps;
+	bool ctc;
+	int ret;
+
+	/* Read and configure the IMASK0 register for unmasking the interrupts */
+	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, false, false);
+	if (ret)
+		return ret;
+
+	regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM);
+
+	ret = oa_tc6_perform_ctrl(tc6, IMASK0, &regval, 1, true, false);
+	if (ret)
+		return ret;
+
+	/* Read STDCAP register to get the MAC-PHY standard capabilities */
+	ret = oa_tc6_perform_ctrl(tc6, STDCAP, &regval, 1, false, false);
+	if (ret)
+		return ret;
+
+	mincps = FIELD_GET(MINCPS, regval);
+	ctc = (regval & CTC) ? true : false;
+
+	regval = 0;
+	oa_node = of_get_child_by_name(spi->dev.of_node, "oa-tc6");
+	if (oa_node) {
+		/* Read OA parameters from DT */
+		if (of_property_present(oa_node, "oa-cps")) {
+			ret = of_property_read_u32(oa_node, "oa-cps", &tc6->cps);
+			if (ret < 0)
+				return ret;
+			/* Return error if the configured cps is less than the
+			 * minimum cps supported by the MAC-PHY.
+			 */
+			if (tc6->cps < mincps)
+				return -ENODEV;
+		} else {
+			tc6->cps = 64;
+		}
+		if (of_property_present(oa_node, "oa-txcte")) {
+			/* Return error if the tx cut through mode is configured
+			 * but it is not supported by MAC-PHY.
+			 */
+			if (ctc)
+				regval |= TXCTE;
+			else
+				return -ENODEV;
+		}
+		if (of_property_present(oa_node, "oa-rxcte")) {
+			/* Return error if the rx cut through mode is configured
+			 * but it is not supported by MAC-PHY.
+			 */
+			if (ctc)
+				regval |= RXCTE;
+			else
+				return -ENODEV;
+		}
+		if (of_property_present(oa_node, "oa-prote")) {
+			regval |= PROTE;
+			tc6->prote = true;
+		}
+	} else {
+		tc6->cps = 64;
+	}
+
+	regval |= FIELD_PREP(CPS, ilog2(tc6->cps) / ilog2(2)) | SYNC;
+
+	return oa_tc6_perform_ctrl(tc6, CONFIG0, &regval, 1, true, false);
+}
+
 static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
 {
 	u32 regval;
@@ -310,7 +387,7 @@  EXPORT_SYMBOL_GPL(oa_tc6_read_registers);
  * Returns pointer reference to the oa_tc6 structure if all the memory
  * allocation success otherwise NULL.
  */
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi)
 {
 	struct oa_tc6 *tc6;
 
@@ -327,7 +404,6 @@  struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
 		return NULL;
 
 	tc6->spi = spi;
-	tc6->prote = prote;
 
 	/* Perform MAC-PHY software reset */
 	if (oa_tc6_sw_reset(tc6)) {
@@ -335,6 +411,12 @@  struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
 		return NULL;
 	}
 
+	/* Perform OA parameters and MAC-PHY configuration */
+	if (oa_tc6_configure(tc6)) {
+		dev_err(&spi->dev, "OA and MAC-PHY configuration failed\n");
+		return NULL;
+	}
+
 	return tc6;
 }
 EXPORT_SYMBOL_GPL(oa_tc6_init);
diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
index 8a838499da97..378636fd9ca8 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -22,15 +22,37 @@ 
 #define TC6_CTRL_BUF_SIZE	1032	/* Max ctrl buffer size for 128 regs */
 
 /* Open Alliance TC6 Standard Control and Status Registers */
+/* Standard Capabilities Register */
+#define STDCAP			0x0002
+#define CTC			BIT(7)	/* Cut-Through Capability */
+#define MINCPS			GENMASK(2, 0)	/* Minimum supported cps */
+
 /* Reset Control and Status Register */
 #define RESET			0x0003
 #define SWRESET			BIT(0)	/* Software Reset */
 
+/* Configuration Register #0 */
+#define CONFIG0			0x0004
+#define SYNC			BIT(15)	/* Configuration Synchronization */
+#define TXCTE			BIT(9)	/* Tx cut-through enable */
+#define RXCTE			BIT(8)	/* Rx cut-through enable */
+#define PROTE			BIT(5)	/* Ctrl read/write Protection Enable */
+#define CPS			GENMASK(2, 0)	/* Chunk Payload Size */
+
 /* Status Register #0 */
 #define STATUS0			0x0008
 #define RESETC			BIT(6)	/* Reset Complete */
 
-struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote);
+/* Interrupt Mask Register #0 */
+#define IMASK0			0x000C
+#define HDREM			BIT(5)	/* Header Error Mask */
+#define LOFEM			BIT(4)	/* Loss of Framing Error Mask */
+#define RXBOEM			BIT(3)	/* Rx Buffer Overflow Error Mask */
+#define TXBUEM			BIT(2)	/* Tx Buffer Underflow Error Mask */
+#define TXBOEM			BIT(1)	/* Tx Buffer Overflow Error Mask */
+#define TXPEM			BIT(0)	/* Tx Protocol Error Mask */
+
+struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
 int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val);
 int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32 *val);
 int oa_tc6_write_registers(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len);