Message-ID: <20230913104458.1d4cdd51@wsk>
Date:   Wed, 13 Sep 2023 10:44:58 +0200
From:   Lukasz Majewski <lukma@...x.de>
To:     Parthiban Veerasooran <Parthiban.Veerasooran@...rochip.com>
Cc:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <robh+dt@...nel.org>,
        <krzysztof.kozlowski+dt@...aro.org>, <conor+dt@...nel.org>,
        <corbet@....net>, <steen.hegelund@...rochip.com>,
        <rdunlap@...radead.org>, <horms@...nel.org>,
        <casper.casan@...il.com>, <andrew@...n.ch>,
        <netdev@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <horatiu.vultur@...rochip.com>, <Woojung.Huh@...rochip.com>,
        <Nicolas.Ferre@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
        <Thorsten.Kummermehr@...rochip.com>
Subject: Re: [RFC PATCH net-next 2/6] net: ethernet: add mac-phy interrupt
 support with reset complete handling

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
> <Parthiban.Veerasooran@...rochip.com> ---
>  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,
> &regval, 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,
> +							  &regval,
> 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, &regval, 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, &regval, 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: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists