Hi Parthiban,
> Register MAC-PHY interrupt and handle reset complete interrupt. Reset
> complete bit is set when the MAC-PHY reset complete 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.
I'm using the MicroE module with LAN8651 device connected to nucleo
STM32G4 microcontroller.
Maybe not directly related to Linux, but I would like to ask for some
clarification.
>
> Signed-off-by: Parthiban Veerasooran
> <[email protected]> ---
> drivers/net/ethernet/oa_tc6.c | 141
> ++++++++++++++++++++++++++++++++-- include/linux/oa_tc6.h |
> 16 +++- 2 files changed, 150 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ethernet/oa_tc6.c
> b/drivers/net/ethernet/oa_tc6.c index 613cf034430a..0019f70345b6
> 100644 --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c
> @@ -6,6 +6,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/interrupt.h>
> #include <linux/oa_tc6.h>
>
> static int oa_tc6_spi_transfer(struct spi_device *spi, u8 *ptx, u8
> *prx, @@ -160,10 +161,16 @@ int oa_tc6_perform_ctrl(struct oa_tc6
> *tc6, u32 addr, u32 val[], u8 len, if (ret)
> goto err_ctrl;
>
> - /* Check echoed/received control reply */
> - ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len, wnr,
> ctrl_prot);
> - if (ret)
> - goto err_ctrl;
> + /* In case of reset write, the echoed control command
> doesn't have any
> + * valid data. So no need to check for error.
> + */
> + if (addr != OA_TC6_RESET) {
> + /* Check echoed/received control reply */
> + ret = oa_tc6_check_control(tc6, tx_buf, rx_buf, len,
> wnr,
> + ctrl_prot);
> + if (ret)
> + goto err_ctrl;
> + }
>
> if (!wnr) {
> /* Copy read data from the rx data in case of ctrl
> read */ @@ -186,6 +193,88 @@ int oa_tc6_perform_ctrl(struct oa_tc6
> *tc6, u32 addr, u32 val[], u8 len, return ret;
> }
>
> +static int oa_tc6_handler(void *data)
> +{
> + struct oa_tc6 *tc6 = data;
> + u32 regval;
> + int ret;
> +
> + while (likely(!kthread_should_stop())) {
> + wait_event_interruptible(tc6->tc6_wq, tc6->int_flag
> ||
> + kthread_should_stop());
> + if (tc6->int_flag) {
> + tc6->int_flag = false;
> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_STS0,
> ®val, 1,
> + false, false);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "Failed to
> read STS0\n");
> + continue;
> + }
> + /* Check for reset complete interrupt status
> */
> + if (regval & RESETC) {
Just maybe mine small remark. IMHO the reset shall not pollute the IRQ
hander. The RESETC is just set on the initialization phase and only
then shall be served. Please correct me if I'm wrong, but it will not
be handled during "normal" operation.
> + regval = RESETC;
> + /* SPI host should write RESETC bit
> with one to
> + * clear the reset interrupt status.
> + */
> + ret = oa_tc6_perform_ctrl(tc6,
> OA_TC6_STS0,
> + ®val,
> 1, true,
> + false);
Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)?
The documentation states it clearly that one also needs to set SYNC bit
(BIT(15)) in the OA_CONFIG0 register (which would have the 0x8006 value).
Mine problem is that even after writing 0x40 to OA_STATUS0 and 0x8006
to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via 10K resistor).
(I'm able to read those registers and those show expected values)
> + if (ret) {
> + dev_err(&tc6->spi->dev,
> + "Failed to write
> STS0\n");
> + continue;
> + }
> + complete(&tc6->rst_complete);
> + }
> + }
> + }
> + return 0;
> +}
> +
> +static irqreturn_t macphy_irq(int irq, void *dev_id)
> +{
> + struct oa_tc6 *tc6 = dev_id;
> +
> + /* Wake tc6 task to perform interrupt action */
> + tc6->int_flag = true;
> + wake_up_interruptible(&tc6->tc6_wq);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int oa_tc6_sw_reset(struct oa_tc6 *tc6)
> +{
> + long timeleft;
> + 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 = SW_RESET;
> + reinit_completion(&tc6->rst_complete);
> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1,
> true, false);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "RESET register write
> failed\n");
> + return ret;
> + }
> +
> + ret = oa_tc6_perform_ctrl(tc6, OA_TC6_RESET, ®val, 1,
> true, true);
> + if (ret) {
> + dev_err(&tc6->spi->dev, "RESET register write
> failed\n");
> + return ret;
> + }
> + timeleft =
Was it on purpose to not use the RST_N pin to perform GPIO based reset?
When I generate reset pulse (and keep it for low for > 5us) the IRQ_N
gets high. After some time it gets low (as expected). But then it
doesn't get high any more.
> wait_for_completion_interruptible_timeout(&tc6->rst_complete,
> +
> msecs_to_jiffies(1));
Please also clarify - does the LAN8651 require up to 1ms "settle down"
(after reset) time before it gets operational again?
> + if (timeleft <= 0) {
> + dev_err(&tc6->spi->dev, "MAC-PHY reset failed\n");
> + return -ENODEV;
> + }
> +
> + return 0;
> +}
> +
> int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 val[],
> u8 len) {
> return oa_tc6_perform_ctrl(tc6, addr, val, len, true,
> tc6->ctrl_prot); @@ -201,6 +290,7 @@
> EXPORT_SYMBOL_GPL(oa_tc6_read_register); struct oa_tc6
> *oa_tc6_init(struct spi_device *spi) {
> struct oa_tc6 *tc6;
> + int ret;
>
> if (!spi)
> return NULL;
> @@ -211,12 +301,51 @@ struct oa_tc6 *oa_tc6_init(struct spi_device
> *spi)
> tc6->spi = spi;
>
> + /* Used for triggering the OA TC6 task */
> + init_waitqueue_head(&tc6->tc6_wq);
> +
> + init_completion(&tc6->rst_complete);
> +
> + /* This task performs the SPI transfer */
> + tc6->tc6_task = kthread_run(oa_tc6_handler, tc6, "OA TC6
> Task");
> + if (IS_ERR(tc6->tc6_task))
> + goto err_tc6_task;
> +
> + /* Set the highest priority to the tc6 task as it is time
> critical */
> + sched_set_fifo(tc6->tc6_task);
> +
> + /* Register MAC-PHY interrupt service routine */
> + ret = devm_request_irq(&spi->dev, spi->irq, macphy_irq, 0,
> "macphy int",
> + tc6);
> + if ((ret != -ENOTCONN) && ret < 0) {
> + dev_err(&spi->dev, "Error attaching macphy irq
> %d\n", ret);
> + goto err_macphy_irq;
> + }
> +
> + /* Perform MAC-PHY software reset */
> + if (oa_tc6_sw_reset(tc6))
> + goto err_macphy_reset;
> +
> return tc6;
> +
> +err_macphy_reset:
> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6);
> +err_macphy_irq:
> + kthread_stop(tc6->tc6_task);
> +err_tc6_task:
> + kfree(tc6);
> + return NULL;
> }
> EXPORT_SYMBOL_GPL(oa_tc6_init);
>
> -void oa_tc6_deinit(struct oa_tc6 *tc6)
> +int oa_tc6_deinit(struct oa_tc6 *tc6)
> {
> - kfree(tc6);
> + int ret;
> +
> + devm_free_irq(&tc6->spi->dev, tc6->spi->irq, tc6);
> + ret = kthread_stop(tc6->tc6_task);
> + if (!ret)
> + kfree(tc6);
> + return ret;
> }
> EXPORT_SYMBOL_GPL(oa_tc6_deinit);
> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
> index 5e0a58ab1dcd..315f061c2dfe 100644
> --- a/include/linux/oa_tc6.h
> +++ b/include/linux/oa_tc6.h
> @@ -17,15 +17,29 @@
> #define CTRL_HDR_LEN GENMASK(7, 1) /* Length */
> #define CTRL_HDR_P BIT(0) /* Parity Bit */
>
> +/* Open Alliance TC6 Standard Control and Status Registers */
> +#define OA_TC6_RESET 0x0003 /* Reset Control
> and Status Register */ +#define OA_TC6_STS0 0x0008
> /* Status Register #0 */ +
> +/* RESET register field */
> +#define SW_RESET BIT(0) /* Software Reset */
> +
> +/* STATUS0 register field */
> +#define RESETC BIT(6) /* Reset
> Complete */ +
> #define TC6_HDR_SIZE 4 /* Ctrl command header
> size as per OA */ #define TC6_FTR_SIZE 4 /*
> Ctrl command footer size ss per OA */
> struct oa_tc6 {
> struct spi_device *spi;
> bool ctrl_prot;
> + struct task_struct *tc6_task;
> + wait_queue_head_t tc6_wq;
> + bool int_flag;
> + struct completion rst_complete;
> };
>
> struct oa_tc6 *oa_tc6_init(struct spi_device *spi);
> -void oa_tc6_deinit(struct oa_tc6 *tc6);
> +int oa_tc6_deinit(struct oa_tc6 *tc6);
> int oa_tc6_write_register(struct oa_tc6 *tc6, u32 addr, u32 value[],
> u8 len); int oa_tc6_read_register(struct oa_tc6 *tc6, u32 addr, u32
> value[], u8 len);
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]
> Just maybe mine small remark. IMHO the reset shall not pollute the IRQ
> hander. The RESETC is just set on the initialization phase and only
> then shall be served. Please correct me if I'm wrong, but it will not
> be handled during "normal" operation.
This is something i also wondered. Maybe if the firmware in the
MAC-PHY crashes, burns, and a watchdog reset it, could it assert
RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt
handler would be useful, but otherwise ignore it. Probe can then poll
during its reset.
> > + regval = RESETC;
> > + /* SPI host should write RESETC bit
> > with one to
> > + * clear the reset interrupt status.
> > + */
> > + ret = oa_tc6_perform_ctrl(tc6,
> > OA_TC6_STS0,
> > + ®val,
> > 1, true,
> > + false);
>
> Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)?
>
> The documentation states it clearly that one also needs to set SYNC bit
> (BIT(15)) in the OA_CONFIG0 register (which would have the 0x8006 value).
>
> Mine problem is that even after writing 0x40 to OA_STATUS0 and 0x8006
> to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via 10K resistor).
>
> (I'm able to read those registers and those show expected values)
What does STATUS0 and STATUS1 contain? That might be a dumb question,
i've not read the details for interrupt handling yet, but maybe there
is another interrupt pending? Or the interrupt mask needs writing?
> Was it on purpose to not use the RST_N pin to perform GPIO based reset?
>
> When I generate reset pulse (and keep it for low for > 5us) the IRQ_N
> gets high. After some time it gets low (as expected). But then it
> doesn't get high any more.
Does the standard say RST_N is mandatory to be controlled by software?
I could imagine RST_N is tied to the board global reset when the power
supply is stable. Software reset is then used at probe time.
So this could be a board design decision. I can see this code getting
extended in the future, an optional gpiod passed to the core for it to
use.
> > msecs_to_jiffies(1));
>
> Please also clarify - does the LAN8651 require up to 1ms "settle down"
> (after reset) time before it gets operational again?
If this is not part of the standard, it really should be in the MAC
driver, or configurable, since different devices might need different
delays. But ideally, if the status bit says it is good to go, i would
really expect it to be good to go. So this probably should be a
LAN8651 quirk.
Andrew
Hi Andrew,
> > Just maybe mine small remark. IMHO the reset shall not pollute the
> > IRQ hander. The RESETC is just set on the initialization phase and
> > only then shall be served. Please correct me if I'm wrong, but it
> > will not be handled during "normal" operation.
>
> This is something i also wondered. Maybe if the firmware in the
> MAC-PHY crashes, burns, and a watchdog reset it, could it assert
> RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt
> handler would be useful, but otherwise ignore it. Probe can then poll
> during its reset.
>
> > > + regval = RESETC;
> > > + /* SPI host should write RESETC
> > > bit with one to
> > > + * clear the reset interrupt
> > > status.
> > > + */
> > > + ret = oa_tc6_perform_ctrl(tc6,
> > > OA_TC6_STS0,
> > > +
> > > ®val, 1, true,
> > > +
> > > false);
> >
> > Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)?
> >
> > The documentation states it clearly that one also needs to set SYNC
> > bit (BIT(15)) in the OA_CONFIG0 register (which would have the
> > 0x8006 value).
> >
> > Mine problem is that even after writing 0x40 to OA_STATUS0 and
> > 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via
> > 10K resistor).
> >
> > (I'm able to read those registers and those show expected values)
>
> What does STATUS0 and STATUS1 contain?
STATUS0 => 0x40, which is expected.
Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C
After reading the same register - I do receive 0x00 (it has been
cleared).
Then I write 0x8006 to OA_CONFIG0.
(Those two steps are regarded as "configuration" of LAN865x device in
the documentation)
In this patch set -> the OA_COFIG0 also has the 0x6 added to indicate
the SPI transfer chunk.
Dump of OA registers:
{0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0}
Status 0 (0x8) -> 0x0
Status 1 (0x9) -> 0x0
> That might be a dumb question,
> i've not read the details for interrupt handling yet, but maybe there
> is another interrupt pending? Or the interrupt mask needs writing?
All the interrupts on MASK{01} are masked.
Changing it to:
sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM |
OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM
doesn't fix this problem.
>
> > Was it on purpose to not use the RST_N pin to perform GPIO based
> > reset?
> >
> > When I generate reset pulse (and keep it for low for > 5us) the
> > IRQ_N gets high. After some time it gets low (as expected). But
> > then it doesn't get high any more.
>
> Does the standard say RST_N is mandatory to be controlled by software?
> I could imagine RST_N is tied to the board global reset when the power
> supply is stable.
It can be GPIO controlled. However, it is not required. I've tied it to
3V3 and also left NC, but no change.
> Software reset is then used at probe time.
I've reconfigured the board to use only SW based reset (i.e. set bit0
in OA_RESET - 0x3).
>
> So this could be a board design decision. I can see this code getting
> extended in the future, an optional gpiod passed to the core for it to
> use.
I can omit the RST_N control. I'd just expect the IRQ_N to be high
after reset.
>
> > > msecs_to_jiffies(1));
> >
> > Please also clarify - does the LAN8651 require up to 1ms "settle
> > down" (after reset) time before it gets operational again?
>
> If this is not part of the standard, it really should be in the MAC
> driver, or configurable, since different devices might need different
> delays. But ideally, if the status bit says it is good to go, i would
> really expect it to be good to go. So this probably should be a
> LAN8651 quirk.
The documentation is silent about the "settle down time". The only
requirements is for RST_N assertion > 5us. However, when the IRQ_N goes
low, and the interrupt is served - it happens that I cannot read ID
from the chip via SPI.
>
> Andrew
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]
Hi Lukasz,
Sorry for the delayed response. Regarding your issue, we just noticed
that you have also filed an issue in our oa tc6 lib github page. Our oa
tc6 lib for controllers developer Thorsten will get back to you on this.
You can get the solution from there.
https://github.com/MicrochipTech/oa-tc6-lib/issues/14
Best Regards,
Parthiban V
On 13/09/23 6:56 pm, Lukasz Majewski wrote:
> Hi Andrew,
>
>>> Just maybe mine small remark. IMHO the reset shall not pollute the
>>> IRQ hander. The RESETC is just set on the initialization phase and
>>> only then shall be served. Please correct me if I'm wrong, but it
>>> will not be handled during "normal" operation.
>>
>> This is something i also wondered. Maybe if the firmware in the
>> MAC-PHY crashes, burns, and a watchdog reset it, could it assert
>> RESETC? I think maybe a WARN_ON_ONCE() for RESETC in the interrupt
>> handler would be useful, but otherwise ignore it. Probe can then poll
>> during its reset.
>>
>>>> + regval = RESETC;
>>>> + /* SPI host should write RESETC
>>>> bit with one to
>>>> + * clear the reset interrupt
>>>> status.
>>>> + */
>>>> + ret = oa_tc6_perform_ctrl(tc6,
>>>> OA_TC6_STS0,
>>>> +
>>>> ®val, 1, true,
>>>> +
>>>> false);
>>>
>>> Is this enough to have the IRQ_N deasserted (i.e. pulled HIGH)?
>>>
>>> The documentation states it clearly that one also needs to set SYNC
>>> bit (BIT(15)) in the OA_CONFIG0 register (which would have the
>>> 0x8006 value).
>>>
>>> Mine problem is that even after writing 0x40 to OA_STATUS0 and
>>> 0x8006 to OA_CONFIG0 the IRQ_N is still LOW (it is pulled up via
>>> 10K resistor).
>>>
>>> (I'm able to read those registers and those show expected values)
>>
>> What does STATUS0 and STATUS1 contain?
>
> STATUS0 => 0x40, which is expected.
>
> Then I do write 0x40 to STATUS0 -> bit6 (RESETC) is R/W1C
>
> After reading the same register - I do receive 0x00 (it has been
> cleared).
>
> Then I write 0x8006 to OA_CONFIG0.
>
> (Those two steps are regarded as "configuration" of LAN865x device in
> the documentation)
>
> In this patch set -> the OA_COFIG0 also has the 0x6 added to indicate
> the SPI transfer chunk.
>
> Dump of OA registers:
> {0x11, 0x7c1b3, 0x5e5, 0x0, 0x8006, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0,
> 0x3000, 0x1fbf, 0x3ffe0003, 0x0, 0x0}
>
> Status 0 (0x8) -> 0x0
> Status 1 (0x9) -> 0x0
>
>> That might be a dumb question,
>> i've not read the details for interrupt handling yet, but maybe there
>> is another interrupt pending? Or the interrupt mask needs writing?
>
> All the interrupts on MASK{01} are masked.
>
> Changing it to:
> sts &= ~(OA_IMASK0_TXPEM | OA_IMASK0_TXBOEM | OA_IMASK0_TXBUEM |
> OA_IMASK0_RXBOEM | OA_IMASK0_LOFEM | OA_IMASK0_HDREM
>
> doesn't fix this problem.
>
>>
>>> Was it on purpose to not use the RST_N pin to perform GPIO based
>>> reset?
>>>
>>> When I generate reset pulse (and keep it for low for > 5us) the
>>> IRQ_N gets high. After some time it gets low (as expected). But
>>> then it doesn't get high any more.
>>
>> Does the standard say RST_N is mandatory to be controlled by software?
>> I could imagine RST_N is tied to the board global reset when the power
>> supply is stable.
>
> It can be GPIO controlled. However, it is not required. I've tied it to
> 3V3 and also left NC, but no change.
>
>> Software reset is then used at probe time.
>
> I've reconfigured the board to use only SW based reset (i.e. set bit0
> in OA_RESET - 0x3).
>
>>
>> So this could be a board design decision. I can see this code getting
>> extended in the future, an optional gpiod passed to the core for it to
>> use.
>
> I can omit the RST_N control. I'd just expect the IRQ_N to be high
> after reset.
>
>>
>>>> msecs_to_jiffies(1));
>>>
>>> Please also clarify - does the LAN8651 require up to 1ms "settle
>>> down" (after reset) time before it gets operational again?
>>
>> If this is not part of the standard, it really should be in the MAC
>> driver, or configurable, since different devices might need different
>> delays. But ideally, if the status bit says it is good to go, i would
>> really expect it to be good to go. So this probably should be a
>> LAN8651 quirk.
>
> The documentation is silent about the "settle down time". The only
> requirements is for RST_N assertion > 5us. However, when the IRQ_N goes
> low, and the interrupt is served - it happens that I cannot read ID
> from the chip via SPI.
>
>>
>> Andrew
>
> Best regards,
>
> Lukasz Majewski
>
> --
>
> DENX Software Engineering GmbH, Managing Director: Erika Unter
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: [email protected]