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 |
> + /* Read and configure the IMASK0 register for unmasking the interrupts */ > + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 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, ®val, 1, true, false); > + if (ret) > + return ret; > + > + /* Read STDCAP register to get the MAC-PHY standard capabilities */ > + ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 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, ®val, 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
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, ®val, 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, ®val, 1, true, false); >> + if (ret) >> + return ret; >> + >> + /* Read STDCAP register to get the MAC-PHY standard capabilities */ >> + ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 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, ®val, 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
> >> -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
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 --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, ®val, 1, false, false); + if (ret) + return ret; + + regval &= ~(TXPEM & TXBOEM & TXBUEM & RXBOEM & LOFEM & HDREM); + + ret = oa_tc6_perform_ctrl(tc6, IMASK0, ®val, 1, true, false); + if (ret) + return ret; + + /* Read STDCAP register to get the MAC-PHY standard capabilities */ + ret = oa_tc6_perform_ctrl(tc6, STDCAP, ®val, 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, ®val, 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);
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(-)