Reset complete bit is set when the MAC-PHY reset completes and ready for
configuration. When it is set, it will generate a non-maskable interrupt
to alert the SPI host. Additionally reset complete bit in the STS0
register has to be written by one upon reset complete to clear the
interrupt.
Signed-off-by: Parthiban Veerasooran <[email protected]>
---
drivers/net/ethernet/oa_tc6.c | 64 ++++++++++++++++++++++++++++++++---
include/linux/oa_tc6.h | 9 +++++
2 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
index acedc327b05e..e4457569135f 100644
--- a/drivers/net/ethernet/oa_tc6.c
+++ b/drivers/net/ethernet/oa_tc6.c
@@ -6,6 +6,8 @@
*/
#include <linux/bitfield.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
#include <linux/oa_tc6.h>
/* Opaque structure for MACPHY drivers */
@@ -169,10 +171,15 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
if (ret)
return ret;
- /* Check echoed/received control reply for errors */
- ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, prote);
- if (ret)
- return ret;
+ /* In case of reset write, the echoed control command doesn't have any
+ * valid data. So no need to check for errors.
+ */
+ if (addr != RESET) {
+ /* Check echoed/received control reply for errors */
+ ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr, prote);
+ if (ret)
+ return ret;
+ }
if (!wnr) {
/* Copy read data from the rx data in case of ctrl read */
@@ -191,6 +198,49 @@ static int oa_tc6_perform_ctrl(struct oa_tc6 *tc6, u32 addr, u32 val[], u8 len,
return ret;
}
+static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
+{
+ u32 regval;
+ int ret;
+
+ /* Perform software reset with both protected and unprotected control
+ * commands because the driver doesn't know the current status of the
+ * MAC-PHY.
+ */
+ regval = SWRESET;
+ ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true);
+ if (ret)
+ return ret;
+
+ ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false);
+ if (ret)
+ return ret;
+
+ /* The chip completes a reset in 3us, we might get here earlier than
+ * that, as an added margin we'll conditionally sleep 5us.
+ */
+ udelay(5);
+
+ ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
+ if (ret)
+ return ret;
+
+ /* Check for reset complete interrupt status */
+ if (regval & RESETC) {
+ regval = RESETC;
+ /* SPI host should write RESETC bit with one to
+ * clear the reset interrupt status.
+ */
+ ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, true, false);
+ if (ret)
+ return ret;
+ } else {
+ return -ENODEV;
+ }
+
+ return 0;
+}
+
/**
* oa_tc6_write_register - function for writing a MACPHY register.
* @tc6: oa_tc6 struct.
@@ -279,6 +329,12 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote)
tc6->spi = spi;
tc6->prote = prote;
+ /* Perform MAC-PHY software reset */
+ if (oa_tc6_sw_reset(tc6)) {
+ dev_err(&spi->dev, "MAC-PHY software reset 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 6284828bda42..8a838499da97 100644
--- a/include/linux/oa_tc6.h
+++ b/include/linux/oa_tc6.h
@@ -21,6 +21,15 @@
#define TC6_FTR_SIZE 4 /* Ctrl command footer size ss per OA */
#define TC6_CTRL_BUF_SIZE 1032 /* Max ctrl buffer size for 128 regs */
+/* Open Alliance TC6 Standard Control and Status Registers */
+/* Reset Control and Status Register */
+#define RESET 0x0003
+#define SWRESET BIT(0) /* Software Reset */
+
+/* Status Register #0 */
+#define STATUS0 0x0008
+#define RESETC BIT(6) /* Reset Complete */
+
struct oa_tc6 *oa_tc6_init(struct spi_device *spi, bool prote);
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);
--
2.34.1
> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true);
> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false);
Just looking at this, it is not clear what these true/false mean. Maybe add some #defines
#define TC6_READ true
#define TC6_WRITE false
#define TC6_PROTECTED true
#define TC6_UNPROTECTED false
> + if (ret)
> + return ret;
> +
> + /* The chip completes a reset in 3us, we might get here earlier than
> + * that, as an added margin we'll conditionally sleep 5us.
> + */
> + udelay(5);
> +
> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
> + if (ret)
> + return ret;
> +
> + /* Check for reset complete interrupt status */
> + if (regval & RESETC) {
> + regval = RESETC;
People don't always agree, but i found STATUS0_RESETC easier to see
you have the correct bit for the register you just read.
Andrew
Hi Andrew,
On 24/10/23 4:13 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true);
>> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false);
>
> Just looking at this, it is not clear what these true/false mean. Maybe add some #defines
>
> #define TC6_READ true
> #define TC6_WRITE false
> #define TC6_PROTECTED true
> #define TC6_UNPROTECTED false
Sure will do this.
>
>> + if (ret)
>> + return ret;
>> +
>> + /* The chip completes a reset in 3us, we might get here earlier than
>> + * that, as an added margin we'll conditionally sleep 5us.
>> + */
>> + udelay(5);
>> +
>> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + /* Check for reset complete interrupt status */
>> + if (regval & RESETC) {
>> + regval = RESETC;
>
> People don't always agree, but i found STATUS0_RESETC easier to see
> you have the correct bit for the register you just read.
Do you want me to define STATUS0_RESETC instead of RESETC or is my
understanding wrong?
Best Regards,
Parthiban V
>
> Andrew
Hi Andrew,
On 24/10/23 4:13 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, true);
>> + ret = oa_tc6_perform_ctrl(tc6, RESET, ®val, 1, true, false);
>
> Just looking at this, it is not clear what these true/false mean. Maybe add some #defines
>
> #define TC6_READ true
> #define TC6_WRITE false
> #define TC6_PROTECTED true
> #define TC6_UNPROTECTED false
Sure will do this.
>
>> + if (ret)
>> + return ret;
>> +
>> + /* The chip completes a reset in 3us, we might get here earlier than
>> + * that, as an added margin we'll conditionally sleep 5us.
>> + */
>> + udelay(5);
>> +
>> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
>> + if (ret)
>> + return ret;
>> +
>> + /* Check for reset complete interrupt status */
>> + if (regval & RESETC) {
>> + regval = RESETC;
>
> People don't always agree, but i found STATUS0_RESETC easier to see
> you have the correct bit for the register you just read.
Do you want me to define STATUS0_RESETC instead of RESETC or is my
understanding is wrong?
Best Regards,
Parthiban V
>
> Andrew
> >> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + /* Check for reset complete interrupt status */
> >> + if (regval & RESETC) {
> >> + regval = RESETC;
> >
> > People don't always agree, but i found STATUS0_RESETC easier to see
> > you have the correct bit for the register you just read.
> Do you want me to define STATUS0_RESETC instead of RESETC or is my
> understanding wrong?
Correct, STATUS0_RESETC. It avoids silly typos like:
ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
if (ret)
return ret;
/* Check for reset complete interrupt status */
if (regval & RESET) {
regval = RESETC;
where RESET is a valid register name, but not a bit. Or say:
ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
if (ret)
return ret;
/* Check for reset complete interrupt status */
if (regval & SWRESET) {
regval = STATUS0_;
where SWRESET is a valid bit, but not for STATUS0.
I've made silly mistakes like this, and learnt that good naming helps
to avoid it.
Andrew
Hi Andrew,
On 27/10/23 1:31 am, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
>>>> + ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + /* Check for reset complete interrupt status */
>>>> + if (regval & RESETC) {
>>>> + regval = RESETC;
>>>
>>> People don't always agree, but i found STATUS0_RESETC easier to see
>>> you have the correct bit for the register you just read.
>> Do you want me to define STATUS0_RESETC instead of RESETC or is my
>> understanding wrong?
>
> Correct, STATUS0_RESETC. It avoids silly typos like:
>
> ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
> if (ret)
> return ret;
>
> /* Check for reset complete interrupt status */
> if (regval & RESET) {
> regval = RESETC;
>
> where RESET is a valid register name, but not a bit. Or say:
>
> ret = oa_tc6_perform_ctrl(tc6, STATUS0, ®val, 1, false, false);
> if (ret)
> return ret;
>
> /* Check for reset complete interrupt status */
> if (regval & SWRESET) {
> regval = STATUS0_;
>
> where SWRESET is a valid bit, but not for STATUS0.
>
> I've made silly mistakes like this, and learnt that good naming helps
> to avoid it.
Ok, thanks for the detailed explanation.
Best Regards,
Parthiban V
>
> Andrew