[PATCH net v2 1/2] ARM: dts: imx: name the interrupts for the fec ethernet driver

[PATCH net v2 1/2] ARM: dts: imx: name the interrupts for the fec ethernet driver

[Troy Kisky]

imx7s/imx7d has the ptp interrupt newly added as well.

For imx7, "int0" is the interrupt for queue 0 and ENET_MII
"int1" is for queue 1
"int2" is for queue 2

For imx6sx, "int0" handles all 3 queues and ENET_MII

And of course, the "ptp" interrupt is for the PTP_CLOCK_PPS interrupts
This will help document what each interrupt does.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---
v2: replaced empty names with "int0","int1", or "int2"

reordered imx7 interrupts so that "int0", for queue 0, is first.
---
 arch/arm/boot/dts/imx6qdl.dtsi | 1 +
 arch/arm/boot/dts/imx6sx.dtsi  | 2 ++
 arch/arm/boot/dts/imx6ul.dtsi  | 2 ++
 arch/arm/boot/dts/imx7d.dtsi   | 6 ++++--
 arch/arm/boot/dts/imx7s.dtsi   | 6 ++++--
 5 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 8884b4a3cafb..b78f7e7a0869 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1017,6 +1017,7 @@
 			fec: ethernet@02188000 {
 				compatible = "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
+				interrupt-names = "int0","ptp";
 				interrupts-extended =
 					<&intc 0 118 IRQ_TYPE_LEVEL_HIGH>,
 					<&intc 0 119 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/arch/arm/boot/dts/imx6sx.dtsi b/arch/arm/boot/dts/imx6sx.dtsi
index 6c7eb54be9e2..ed148775f991 100644
--- a/arch/arm/boot/dts/imx6sx.dtsi
+++ b/arch/arm/boot/dts/imx6sx.dtsi
@@ -861,6 +861,7 @@
 			fec1: ethernet@02188000 {
 				compatible = "fsl,imx6sx-fec", "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
+				interrupt-names = "int0","ptp";
 				interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_ENET>,
@@ -970,6 +971,7 @@
 			fec2: ethernet@021b4000 {
 				compatible = "fsl,imx6sx-fec", "fsl,imx6q-fec";
 				reg = <0x021b4000 0x4000>;
+				interrupt-names = "int0","ptp";
 				interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6SX_CLK_ENET>,
diff --git a/arch/arm/boot/dts/imx6ul.dtsi b/arch/arm/boot/dts/imx6ul.dtsi
index f11a241a340d..b624b5fc2d99 100644
--- a/arch/arm/boot/dts/imx6ul.dtsi
+++ b/arch/arm/boot/dts/imx6ul.dtsi
@@ -476,6 +476,7 @@
 			fec2: ethernet@020b4000 {
 				compatible = "fsl,imx6ul-fec", "fsl,imx6q-fec";
 				reg = <0x020b4000 0x4000>;
+				interrupt-names = "int0","ptp";
 				interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6UL_CLK_ENET>,
@@ -775,6 +776,7 @@
 			fec1: ethernet@02188000 {
 				compatible = "fsl,imx6ul-fec", "fsl,imx6q-fec";
 				reg = <0x02188000 0x4000>;
+				interrupt-names = "int0","ptp";
 				interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
 					     <GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX6UL_CLK_ENET>,
diff --git a/arch/arm/boot/dts/imx7d.dtsi b/arch/arm/boot/dts/imx7d.dtsi
index f46814a7ea44..312d24ff106e 100644
--- a/arch/arm/boot/dts/imx7d.dtsi
+++ b/arch/arm/boot/dts/imx7d.dtsi
@@ -114,9 +114,11 @@
 	fec2: ethernet@30bf0000 {
 		compatible = "fsl,imx7d-fec", "fsl,imx6sx-fec";
 		reg = <0x30bf0000 0x10000>;
-		interrupts = <GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
+		interrupt-names = "int0","int1","int2","ptp";
+		interrupts = <GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>,
+			<GIC_SPI 100 IRQ_TYPE_LEVEL_HIGH>,
 			<GIC_SPI 101 IRQ_TYPE_LEVEL_HIGH>,
-			<GIC_SPI 102 IRQ_TYPE_LEVEL_HIGH>;
+			<GIC_SPI 103 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&clks IMX7D_ENET_AXI_ROOT_CLK>,
 			<&clks IMX7D_ENET_AXI_ROOT_CLK>,
 			<&clks IMX7D_ENET2_TIME_ROOT_CLK>,
diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 82ad26e766eb..b00a31a50771 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -1007,9 +1007,11 @@
 			fec1: ethernet@30be0000 {
 				compatible = "fsl,imx7d-fec", "fsl,imx6sx-fec";
 				reg = <0x30be0000 0x10000>;
-				interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
+				interrupt-names = "int0","int1","int2","ptp";
+				interrupts = <GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>,
+					<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>,
 					<GIC_SPI 119 IRQ_TYPE_LEVEL_HIGH>,
-					<GIC_SPI 120 IRQ_TYPE_LEVEL_HIGH>;
+					<GIC_SPI 121 IRQ_TYPE_LEVEL_HIGH>;
 				clocks = <&clks IMX7D_ENET_AXI_ROOT_CLK>,
 					<&clks IMX7D_ENET_AXI_ROOT_CLK>,
 					<&clks IMX7D_ENET1_TIME_ROOT_CLK>,
-- 
2.11.0

[PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Troy Kisky]

This is better for code locality and should slightly
speed up normal interrupts.

This also allows PPS clock output to start working for
i.mx7. This is because i.mx7 was already using the limit
of 3 interrupts, and needed another.

Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>

---

v2: made this change independent of any devicetree change
so that old dtbs continue to work.

Continue to register ptp clock if interrupt is not found.
---
 drivers/net/ethernet/freescale/fec.h      |  3 +-
 drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
 drivers/net/ethernet/freescale/fec_ptp.c  | 82 ++++++++++++++++++-------------
 3 files changed, 65 insertions(+), 45 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
index ede1876a9a19..be56ac1f1ac4 100644
--- a/drivers/net/ethernet/freescale/fec.h
+++ b/drivers/net/ethernet/freescale/fec.h
@@ -582,12 +582,11 @@ struct fec_enet_private {
 	u64 ethtool_stats[0];
 };
 
-void fec_ptp_init(struct platform_device *pdev);
+void fec_ptp_init(struct platform_device *pdev, int irq_index);
 void fec_ptp_stop(struct platform_device *pdev);
 void fec_ptp_start_cyclecounter(struct net_device *ndev);
 int fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);
 int fec_ptp_get(struct net_device *ndev, struct ifreq *ifr);
-uint fec_ptp_check_pps_event(struct fec_enet_private *fep);
 
 /****************************************************************************/
 #endif /* FEC_H */
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 3dc2d771a222..21afabbc560f 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 		complete(&fep->mdio_done);
 	}
-
-	if (fep->ptp_clock)
-		if (fec_ptp_check_pps_event(fep))
-			ret = IRQ_HANDLED;
 	return ret;
 }
 
@@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node, *phy_node;
 	int num_tx_qs;
 	int num_rx_qs;
+	char irq_name[8];
+	int irq_cnt;
 
 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
 
@@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
 	if (ret)
 		goto failed_reset;
 
+	irq_cnt = platform_irq_count(pdev);
+	if (irq_cnt > FEC_IRQ_NUM)
+		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
+	else if (irq_cnt == 2)
+		irq_cnt = 1;	/* last for ptp */
+	else if (irq_cnt <= 0)
+		irq_cnt = 1;	/* Let the for loop fail */
+
 	if (fep->bufdesc_ex)
-		fec_ptp_init(pdev);
+		fec_ptp_init(pdev, irq_cnt);
 
 	ret = fec_enet_init(ndev);
 	if (ret)
 		goto failed_init;
 
-	for (i = 0; i < FEC_IRQ_NUM; i++) {
-		irq = platform_get_irq(pdev, i);
+	for (i = 0; i < irq_cnt; i++) {
+		sprintf(irq_name, "int%d", i);
+		irq = platform_get_irq_byname(pdev, irq_name);
+		if (irq < 0)
+			irq = platform_get_irq(pdev, i);
 		if (irq < 0) {
-			if (i)
-				break;
 			ret = irq;
 			goto failed_irq;
 		}
diff --git a/drivers/net/ethernet/freescale/fec_ptp.c b/drivers/net/ethernet/freescale/fec_ptp.c
index 6ebad3fac81d..3abeee0d16dd 100644
--- a/drivers/net/ethernet/freescale/fec_ptp.c
+++ b/drivers/net/ethernet/freescale/fec_ptp.c
@@ -549,6 +549,37 @@ static void fec_time_keep(struct work_struct *work)
 	schedule_delayed_work(&fep->time_keep, HZ);
 }
 
+/* This function checks the pps event and reloads the timer compare counter. */
+static irqreturn_t fec_ptp_interrupt(int irq, void *dev_id)
+{
+	struct net_device *ndev = dev_id;
+	struct fec_enet_private *fep = netdev_priv(ndev);
+	u32 val;
+	u8 channel = fep->pps_channel;
+	struct ptp_clock_event event;
+
+	val = readl(fep->hwp + FEC_TCSR(channel));
+	if (val & FEC_T_TF_MASK) {
+		/* Write the next next compare(not the next according the spec)
+		 * value to the register
+		 */
+		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
+		do {
+			writel(val, fep->hwp + FEC_TCSR(channel));
+		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
+
+		/* Update the counter; */
+		fep->next_counter = (fep->next_counter + fep->reload_period) &
+				fep->cc.mask;
+
+		event.type = PTP_CLOCK_PPS;
+		ptp_clock_event(fep->ptp_clock, &event);
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
 /**
  * fec_ptp_init
  * @ndev: The FEC network adapter
@@ -558,10 +589,12 @@ static void fec_time_keep(struct work_struct *work)
  * cyclecounter init routine and exits.
  */
 
-void fec_ptp_init(struct platform_device *pdev)
+void fec_ptp_init(struct platform_device *pdev, int irq_index)
 {
 	struct net_device *ndev = platform_get_drvdata(pdev);
 	struct fec_enet_private *fep = netdev_priv(ndev);
+	int irq;
+	int ret;
 
 	fep->ptp_caps.owner = THIS_MODULE;
 	snprintf(fep->ptp_caps.name, 16, "fec ptp");
@@ -587,6 +620,20 @@ void fec_ptp_init(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
 
+	irq = platform_get_irq_byname(pdev, "ptp");
+	if (irq < 0)
+		irq = platform_get_irq(pdev, irq_index);
+	/* Failure to get an irq is not fatal,
+	 * only the PTP_CLOCK_PPS clock events should stop
+	 */
+	if (irq >= 0) {
+		ret = devm_request_irq(&pdev->dev, irq, fec_ptp_interrupt,
+				       0, pdev->name, ndev);
+		if (ret < 0)
+			dev_warn(&pdev->dev, "request for ptp irq failed(%d)\n",
+				 ret);
+	}
+
 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
 	if (IS_ERR(fep->ptp_clock)) {
 		fep->ptp_clock = NULL;
@@ -605,36 +652,3 @@ void fec_ptp_stop(struct platform_device *pdev)
 	if (fep->ptp_clock)
 		ptp_clock_unregister(fep->ptp_clock);
 }
-
-/**
- * fec_ptp_check_pps_event
- * @fep: the fec_enet_private structure handle
- *
- * This function check the pps event and reload the timer compare counter.
- */
-uint fec_ptp_check_pps_event(struct fec_enet_private *fep)
-{
-	u32 val;
-	u8 channel = fep->pps_channel;
-	struct ptp_clock_event event;
-
-	val = readl(fep->hwp + FEC_TCSR(channel));
-	if (val & FEC_T_TF_MASK) {
-		/* Write the next next compare(not the next according the spec)
-		 * value to the register
-		 */
-		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
-		do {
-			writel(val, fep->hwp + FEC_TCSR(channel));
-		} while (readl(fep->hwp + FEC_TCSR(channel)) & FEC_T_TF_MASK);
-
-		/* Update the counter; */
-		fep->next_counter = (fep->next_counter + fep->reload_period) & fep->cc.mask;
-
-		event.type = PTP_CLOCK_PPS;
-		ptp_clock_event(fep->ptp_clock, &event);
-		return 1;
-	}
-
-	return 0;
-}
-- 
2.11.0

Re: [PATCH net v2 1/2] ARM: dts: imx: name the interrupts for the fec ethernet driver

[Andrew Lunn]

On Fri, Oct 13, 2017 at 07:09:39PM -0700, Troy Kisky wrote:
> imx7s/imx7d has the ptp interrupt newly added as well.

Hi Troy
 
> For imx7, "int0" is the interrupt for queue 0 and ENET_MII
> "int1" is for queue 1
> "int2" is for queue 2

Thanks for adding this explanation. Please also add it to

Documentation/devicetree/bindings/net/fsl-fec.txt 

>  			fec: ethernet@02188000 {
>  				compatible = "fsl,imx6q-fec";
>  				reg = <0x02188000 0x4000>;
> +				interrupt-names = "int0","ptp";

It is normal to have a space after the ,

   Andrew

RE: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Andy Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Saturday, October 14, 2017 10:10 AM
>This is better for code locality and should slightly speed up normal interrupts.
>
>This also allows PPS clock output to start working for i.mx7. This is because
>i.mx7 was already using the limit of 3 interrupts, and needed another.
>
>Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>
>---
>
>v2: made this change independent of any devicetree change so that old dtbs
>continue to work.
>
>Continue to register ptp clock if interrupt is not found.
>---
> drivers/net/ethernet/freescale/fec.h      |  3 +-
> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>drivers/net/ethernet/freescale/fec_ptp.c  | 82 ++++++++++++++++++--------
>-----
> 3 files changed, 65 insertions(+), 45 deletions(-)
>
>diff --git a/drivers/net/ethernet/freescale/fec.h
>b/drivers/net/ethernet/freescale/fec.h
>index ede1876a9a19..be56ac1f1ac4 100644
>--- a/drivers/net/ethernet/freescale/fec.h
>+++ b/drivers/net/ethernet/freescale/fec.h
>@@ -582,12 +582,11 @@ struct fec_enet_private {
> 	u64 ethtool_stats[0];
> };
>
>-void fec_ptp_init(struct platform_device *pdev);
>+void fec_ptp_init(struct platform_device *pdev, int irq_index);
> void fec_ptp_stop(struct platform_device *pdev);  void
>fec_ptp_start_cyclecounter(struct net_device *ndev);  int fec_ptp_set(struct
>net_device *ndev, struct ifreq *ifr);  int fec_ptp_get(struct net_device *ndev,
>struct ifreq *ifr); -uint fec_ptp_check_pps_event(struct fec_enet_private
>*fep);
>
>
>/**********************************************************
>******************/
> #endif /* FEC_H */
>diff --git a/drivers/net/ethernet/freescale/fec_main.c
>b/drivers/net/ethernet/freescale/fec_main.c
>index 3dc2d771a222..21afabbc560f 100644
>--- a/drivers/net/ethernet/freescale/fec_main.c
>+++ b/drivers/net/ethernet/freescale/fec_main.c
>@@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
> 		ret = IRQ_HANDLED;
> 		complete(&fep->mdio_done);
> 	}
>-
>-	if (fep->ptp_clock)
>-		if (fec_ptp_check_pps_event(fep))
>-			ret = IRQ_HANDLED;
> 	return ret;
> }
>
>@@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
> 	struct device_node *np = pdev->dev.of_node, *phy_node;
> 	int num_tx_qs;
> 	int num_rx_qs;
>+	char irq_name[8];
>+	int irq_cnt;
>
> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>
>@@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
> 	if (ret)
> 		goto failed_reset;
>
>+	irq_cnt = platform_irq_count(pdev);
>+	if (irq_cnt > FEC_IRQ_NUM)
>+		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>+	else if (irq_cnt == 2)
>+		irq_cnt = 1;	/* last for ptp */
>+	else if (irq_cnt <= 0)
>+		irq_cnt = 1;	/* Let the for loop fail */

Don't do like this. Don't suppose pps interrupt is the last one.
And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec interrupt function.

I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like your v1 logic check.

>+
> 	if (fep->bufdesc_ex)
>-		fec_ptp_init(pdev);
>+		fec_ptp_init(pdev, irq_cnt);
>
> 	ret = fec_enet_init(ndev);
> 	if (ret)
> 		goto failed_init;
>
>-	for (i = 0; i < FEC_IRQ_NUM; i++) {
>-		irq = platform_get_irq(pdev, i);
>+	for (i = 0; i < irq_cnt; i++) {
>+		sprintf(irq_name, "int%d", i);
>+		irq = platform_get_irq_byname(pdev, irq_name);
>+		if (irq < 0)
>+			irq = platform_get_irq(pdev, i);
> 		if (irq < 0) {
>-			if (i)
>-				break;
> 			ret = irq;
> 			goto failed_irq;
> 		}
>diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>b/drivers/net/ethernet/freescale/fec_ptp.c
>index 6ebad3fac81d..3abeee0d16dd 100644
>--- a/drivers/net/ethernet/freescale/fec_ptp.c
>+++ b/drivers/net/ethernet/freescale/fec_ptp.c
>@@ -549,6 +549,37 @@ static void fec_time_keep(struct work_struct *work)
> 	schedule_delayed_work(&fep->time_keep, HZ);  }
>
>+/* This function checks the pps event and reloads the timer compare
>+counter. */ static irqreturn_t fec_ptp_interrupt(int irq, void *dev_id)
>+{
>+	struct net_device *ndev = dev_id;
>+	struct fec_enet_private *fep = netdev_priv(ndev);
>+	u32 val;
>+	u8 channel = fep->pps_channel;
>+	struct ptp_clock_event event;
>+
>+	val = readl(fep->hwp + FEC_TCSR(channel));
>+	if (val & FEC_T_TF_MASK) {
>+		/* Write the next next compare(not the next according the
>spec)
>+		 * value to the register
>+		 */
>+		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>+		do {
>+			writel(val, fep->hwp + FEC_TCSR(channel));
>+		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>FEC_T_TF_MASK);
>+
>+		/* Update the counter; */
>+		fep->next_counter = (fep->next_counter + fep-
>>reload_period) &
>+				fep->cc.mask;
>+
>+		event.type = PTP_CLOCK_PPS;
>+		ptp_clock_event(fep->ptp_clock, &event);
>+		return IRQ_HANDLED;
>+	}
>+
>+	return IRQ_NONE;
>+}
>+
> /**
>  * fec_ptp_init
>  * @ndev: The FEC network adapter
>@@ -558,10 +589,12 @@ static void fec_time_keep(struct work_struct *work)
>  * cyclecounter init routine and exits.
>  */
>
>-void fec_ptp_init(struct platform_device *pdev)
>+void fec_ptp_init(struct platform_device *pdev, int irq_index)
> {
> 	struct net_device *ndev = platform_get_drvdata(pdev);
> 	struct fec_enet_private *fep = netdev_priv(ndev);
>+	int irq;
>+	int ret;
>
> 	fep->ptp_caps.owner = THIS_MODULE;
> 	snprintf(fep->ptp_caps.name, 16, "fec ptp"); @@ -587,6 +620,20 @@
>void fec_ptp_init(struct platform_device *pdev)
>
> 	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
>
>+	irq = platform_get_irq_byname(pdev, "ptp");
>+	if (irq < 0)
>+		irq = platform_get_irq(pdev, irq_index);
>+	/* Failure to get an irq is not fatal,
>+	 * only the PTP_CLOCK_PPS clock events should stop
>+	 */
>+	if (irq >= 0) {
>+		ret = devm_request_irq(&pdev->dev, irq, fec_ptp_interrupt,
>+				       0, pdev->name, ndev);
>+		if (ret < 0)
>+			dev_warn(&pdev->dev, "request for ptp irq
>failed(%d)\n",
>+				 ret);
>+	}
>+
> 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
> 	if (IS_ERR(fep->ptp_clock)) {
> 		fep->ptp_clock = NULL;
>@@ -605,36 +652,3 @@ void fec_ptp_stop(struct platform_device *pdev)
> 	if (fep->ptp_clock)
> 		ptp_clock_unregister(fep->ptp_clock);
> }
>-
>-/**
>- * fec_ptp_check_pps_event
>- * @fep: the fec_enet_private structure handle
>- *
>- * This function check the pps event and reload the timer compare counter.
>- */
>-uint fec_ptp_check_pps_event(struct fec_enet_private *fep) -{
>-	u32 val;
>-	u8 channel = fep->pps_channel;
>-	struct ptp_clock_event event;
>-
>-	val = readl(fep->hwp + FEC_TCSR(channel));
>-	if (val & FEC_T_TF_MASK) {
>-		/* Write the next next compare(not the next according the
>spec)
>-		 * value to the register
>-		 */
>-		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>-		do {
>-			writel(val, fep->hwp + FEC_TCSR(channel));
>-		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>FEC_T_TF_MASK);
>-
>-		/* Update the counter; */
>-		fep->next_counter = (fep->next_counter + fep-
>>reload_period) & fep->cc.mask;
>-
>-		event.type = PTP_CLOCK_PPS;
>-		ptp_clock_event(fep->ptp_clock, &event);
>-		return 1;
>-	}
>-
>-	return 0;
>-}
>--
>2.11.0

Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Troy Kisky]

On 10/15/2017 8:41 PM, Andy Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Saturday, October 14, 2017 10:10 AM
>> This is better for code locality and should slightly speed up normal interrupts.
>>
>> This also allows PPS clock output to start working for i.mx7. This is because
>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>
>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>
>> ---
>>
>> v2: made this change independent of any devicetree change so that old dtbs
>> continue to work.
>>
>> Continue to register ptp clock if interrupt is not found.
>> ---
>> drivers/net/ethernet/freescale/fec.h      |  3 +-
>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>> drivers/net/ethernet/freescale/fec_ptp.c  | 82 ++++++++++++++++++--------
>> -----
>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec.h
>> b/drivers/net/ethernet/freescale/fec.h
>> index ede1876a9a19..be56ac1f1ac4 100644
>> --- a/drivers/net/ethernet/freescale/fec.h
>> +++ b/drivers/net/ethernet/freescale/fec.h
>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>> 	u64 ethtool_stats[0];
>> };
>>
>> -void fec_ptp_init(struct platform_device *pdev);
>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>> void fec_ptp_stop(struct platform_device *pdev);  void
>> fec_ptp_start_cyclecounter(struct net_device *ndev);  int fec_ptp_set(struct
>> net_device *ndev, struct ifreq *ifr);  int fec_ptp_get(struct net_device *ndev,
>> struct ifreq *ifr); -uint fec_ptp_check_pps_event(struct fec_enet_private
>> *fep);
>>
>>
>> /**********************************************************
>> ******************/
>> #endif /* FEC_H */
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> b/drivers/net/ethernet/freescale/fec_main.c
>> index 3dc2d771a222..21afabbc560f 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>> 		ret = IRQ_HANDLED;
>> 		complete(&fep->mdio_done);
>> 	}
>> -
>> -	if (fep->ptp_clock)
>> -		if (fec_ptp_check_pps_event(fep))
>> -			ret = IRQ_HANDLED;
>> 	return ret;
>> }
>>
>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>> 	struct device_node *np = pdev->dev.of_node, *phy_node;
>> 	int num_tx_qs;
>> 	int num_rx_qs;
>> +	char irq_name[8];
>> +	int irq_cnt;
>>
>> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>
>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>> 	if (ret)
>> 		goto failed_reset;
>>
>> +	irq_cnt = platform_irq_count(pdev);
>> +	if (irq_cnt > FEC_IRQ_NUM)
>> +		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>> +	else if (irq_cnt == 2)
>> +		irq_cnt = 1;	/* last for ptp */
>> +	else if (irq_cnt <= 0)
>> +		irq_cnt = 1;	/* Let the for loop fail */
> 
> Don't do like this. Don't suppose pps interrupt is the last one.


I don't. If the pps interrupt is named, the named interrupt will be used. If it is NOT
named, the last interrupt is used, if 2 interrupts, or >3 interrupt are provided.
Otherwise, no pps interrupt is assumed.
Fortunately this seems to be true currently.


> And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec interrupt function.

How ?  fec_ptp_init will not be called as bufdesc_ex is 0.


Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps interrupt.


> 
> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like your v1 logic check.
> 
>> +
>> 	if (fep->bufdesc_ex)
>> -		fec_ptp_init(pdev);
>> +		fec_ptp_init(pdev, irq_cnt);
>>
>> 	ret = fec_enet_init(ndev);
>> 	if (ret)
>> 		goto failed_init;
>>
>> -	for (i = 0; i < FEC_IRQ_NUM; i++) {
>> -		irq = platform_get_irq(pdev, i);
>> +	for (i = 0; i < irq_cnt; i++) {
>> +		sprintf(irq_name, "int%d", i);
>> +		irq = platform_get_irq_byname(pdev, irq_name);
>> +		if (irq < 0)
>> +			irq = platform_get_irq(pdev, i);
>> 		if (irq < 0) {
>> -			if (i)
>> -				break;
>> 			ret = irq;
>> 			goto failed_irq;
>> 		}
>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>> b/drivers/net/ethernet/freescale/fec_ptp.c
>> index 6ebad3fac81d..3abeee0d16dd 100644
>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>> @@ -549,6 +549,37 @@ static void fec_time_keep(struct work_struct *work)
>> 	schedule_delayed_work(&fep->time_keep, HZ);  }
>>
>> +/* This function checks the pps event and reloads the timer compare
>> +counter. */ static irqreturn_t fec_ptp_interrupt(int irq, void *dev_id)
>> +{
>> +	struct net_device *ndev = dev_id;
>> +	struct fec_enet_private *fep = netdev_priv(ndev);
>> +	u32 val;
>> +	u8 channel = fep->pps_channel;
>> +	struct ptp_clock_event event;
>> +
>> +	val = readl(fep->hwp + FEC_TCSR(channel));
>> +	if (val & FEC_T_TF_MASK) {
>> +		/* Write the next next compare(not the next according the
>> spec)
>> +		 * value to the register
>> +		 */
>> +		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>> +		do {
>> +			writel(val, fep->hwp + FEC_TCSR(channel));
>> +		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>> FEC_T_TF_MASK);
>> +
>> +		/* Update the counter; */
>> +		fep->next_counter = (fep->next_counter + fep-
>>> reload_period) &
>> +				fep->cc.mask;
>> +
>> +		event.type = PTP_CLOCK_PPS;
>> +		ptp_clock_event(fep->ptp_clock, &event);
>> +		return IRQ_HANDLED;
>> +	}
>> +
>> +	return IRQ_NONE;
>> +}
>> +
>> /**
>>  * fec_ptp_init
>>  * @ndev: The FEC network adapter
>> @@ -558,10 +589,12 @@ static void fec_time_keep(struct work_struct *work)
>>  * cyclecounter init routine and exits.
>>  */
>>
>> -void fec_ptp_init(struct platform_device *pdev)
>> +void fec_ptp_init(struct platform_device *pdev, int irq_index)
>> {
>> 	struct net_device *ndev = platform_get_drvdata(pdev);
>> 	struct fec_enet_private *fep = netdev_priv(ndev);
>> +	int irq;
>> +	int ret;
>>
>> 	fep->ptp_caps.owner = THIS_MODULE;
>> 	snprintf(fep->ptp_caps.name, 16, "fec ptp"); @@ -587,6 +620,20 @@
>> void fec_ptp_init(struct platform_device *pdev)
>>
>> 	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
>>
>> +	irq = platform_get_irq_byname(pdev, "ptp");
>> +	if (irq < 0)
>> +		irq = platform_get_irq(pdev, irq_index);
>> +	/* Failure to get an irq is not fatal,
>> +	 * only the PTP_CLOCK_PPS clock events should stop
>> +	 */
>> +	if (irq >= 0) {
>> +		ret = devm_request_irq(&pdev->dev, irq, fec_ptp_interrupt,
>> +				       0, pdev->name, ndev);
>> +		if (ret < 0)
>> +			dev_warn(&pdev->dev, "request for ptp irq
>> failed(%d)\n",
>> +				 ret);
>> +	}
>> +
>> 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>> 	if (IS_ERR(fep->ptp_clock)) {
>> 		fep->ptp_clock = NULL;
>> @@ -605,36 +652,3 @@ void fec_ptp_stop(struct platform_device *pdev)
>> 	if (fep->ptp_clock)
>> 		ptp_clock_unregister(fep->ptp_clock);
>> }
>> -
>> -/**
>> - * fec_ptp_check_pps_event
>> - * @fep: the fec_enet_private structure handle
>> - *
>> - * This function check the pps event and reload the timer compare counter.
>> - */
>> -uint fec_ptp_check_pps_event(struct fec_enet_private *fep) -{
>> -	u32 val;
>> -	u8 channel = fep->pps_channel;
>> -	struct ptp_clock_event event;
>> -
>> -	val = readl(fep->hwp + FEC_TCSR(channel));
>> -	if (val & FEC_T_TF_MASK) {
>> -		/* Write the next next compare(not the next according the
>> spec)
>> -		 * value to the register
>> -		 */
>> -		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>> -		do {
>> -			writel(val, fep->hwp + FEC_TCSR(channel));
>> -		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>> FEC_T_TF_MASK);
>> -
>> -		/* Update the counter; */
>> -		fep->next_counter = (fep->next_counter + fep-
>>> reload_period) & fep->cc.mask;
>> -
>> -		event.type = PTP_CLOCK_PPS;
>> -		ptp_clock_event(fep->ptp_clock, &event);
>> -		return 1;
>> -	}
>> -
>> -	return 0;
>> -}
>> --
>> 2.11.0
> 
> 

RE: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Andy Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>> This is better for code locality and should slightly speed up normal
>interrupts.
>>>
>>> This also allows PPS clock output to start working for i.mx7. This is
>>> because
>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>
>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>
>>> ---
>>>
>>> v2: made this change independent of any devicetree change so that old
>>> dtbs continue to work.
>>>
>>> Continue to register ptp clock if interrupt is not found.
>>> ---
>>> drivers/net/ethernet/freescale/fec.h      |  3 +-
>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>> drivers/net/ethernet/freescale/fec_ptp.c  | 82
>>> ++++++++++++++++++--------
>>> -----
>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>> b/drivers/net/ethernet/freescale/fec.h
>>> index ede1876a9a19..be56ac1f1ac4 100644
>>> --- a/drivers/net/ethernet/freescale/fec.h
>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>> 	u64 ethtool_stats[0];
>>> };
>>>
>>> -void fec_ptp_init(struct platform_device *pdev);
>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>> void fec_ptp_stop(struct platform_device *pdev);  void
>>> fec_ptp_start_cyclecounter(struct net_device *ndev);  int
>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>
>>>
>>>
>/**********************************************************
>>> ******************/
>>> #endif /* FEC_H */
>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>> b/drivers/net/ethernet/freescale/fec_main.c
>>> index 3dc2d771a222..21afabbc560f 100644
>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>> 		ret = IRQ_HANDLED;
>>> 		complete(&fep->mdio_done);
>>> 	}
>>> -
>>> -	if (fep->ptp_clock)
>>> -		if (fec_ptp_check_pps_event(fep))
>>> -			ret = IRQ_HANDLED;
>>> 	return ret;
>>> }
>>>
>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>> 	struct device_node *np = pdev->dev.of_node, *phy_node;
>>> 	int num_tx_qs;
>>> 	int num_rx_qs;
>>> +	char irq_name[8];
>>> +	int irq_cnt;
>>>
>>> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>
>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>> 	if (ret)
>>> 		goto failed_reset;
>>>
>>> +	irq_cnt = platform_irq_count(pdev);
>>> +	if (irq_cnt > FEC_IRQ_NUM)
>>> +		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>>> +	else if (irq_cnt == 2)
>>> +		irq_cnt = 1;	/* last for ptp */
>>> +	else if (irq_cnt <= 0)
>>> +		irq_cnt = 1;	/* Let the for loop fail */
>>
>> Don't do like this. Don't suppose pps interrupt is the last one.
>
>
>I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>provided.
>Otherwise, no pps interrupt is assumed.
>Fortunately this seems to be true currently.
>
If pps interrupt is not named, then it limit the last one is pps.
We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
Although your current implementation can work with current chips, but it is not really good solution.

>
>> And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec interrupt
>function.
>
>How ?  fec_ptp_init will not be called as bufdesc_ex is 0.
>
Imx28 also support enhanced buffer descriptor,  if define the ptp clock in dts then bufdesc_ex also can be 1.

I still suggest to use v1 logic check pps interrupt that need to check irq name.

>
>Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>interrupt.
>
>
>>
>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>your v1 logic check.
>>
>>> +
>>> 	if (fep->bufdesc_ex)
>>> -		fec_ptp_init(pdev);
>>> +		fec_ptp_init(pdev, irq_cnt);
>>>
>>> 	ret = fec_enet_init(ndev);
>>> 	if (ret)
>>> 		goto failed_init;
>>>
>>> -	for (i = 0; i < FEC_IRQ_NUM; i++) {
>>> -		irq = platform_get_irq(pdev, i);
>>> +	for (i = 0; i < irq_cnt; i++) {
>>> +		sprintf(irq_name, "int%d", i);
>>> +		irq = platform_get_irq_byname(pdev, irq_name);
>>> +		if (irq < 0)
>>> +			irq = platform_get_irq(pdev, i);
>>> 		if (irq < 0) {
>>> -			if (i)
>>> -				break;
>>> 			ret = irq;
>>> 			goto failed_irq;
>>> 		}
>>> diff --git a/drivers/net/ethernet/freescale/fec_ptp.c
>>> b/drivers/net/ethernet/freescale/fec_ptp.c
>>> index 6ebad3fac81d..3abeee0d16dd 100644
>>> --- a/drivers/net/ethernet/freescale/fec_ptp.c
>>> +++ b/drivers/net/ethernet/freescale/fec_ptp.c
>>> @@ -549,6 +549,37 @@ static void fec_time_keep(struct work_struct
>*work)
>>> 	schedule_delayed_work(&fep->time_keep, HZ);  }
>>>
>>> +/* This function checks the pps event and reloads the timer compare
>>> +counter. */ static irqreturn_t fec_ptp_interrupt(int irq, void
>>> +*dev_id) {
>>> +	struct net_device *ndev = dev_id;
>>> +	struct fec_enet_private *fep = netdev_priv(ndev);
>>> +	u32 val;
>>> +	u8 channel = fep->pps_channel;
>>> +	struct ptp_clock_event event;
>>> +
>>> +	val = readl(fep->hwp + FEC_TCSR(channel));
>>> +	if (val & FEC_T_TF_MASK) {
>>> +		/* Write the next next compare(not the next according the
>>> spec)
>>> +		 * value to the register
>>> +		 */
>>> +		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>>> +		do {
>>> +			writel(val, fep->hwp + FEC_TCSR(channel));
>>> +		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>>> FEC_T_TF_MASK);
>>> +
>>> +		/* Update the counter; */
>>> +		fep->next_counter = (fep->next_counter + fep-
>>>> reload_period) &
>>> +				fep->cc.mask;
>>> +
>>> +		event.type = PTP_CLOCK_PPS;
>>> +		ptp_clock_event(fep->ptp_clock, &event);
>>> +		return IRQ_HANDLED;
>>> +	}
>>> +
>>> +	return IRQ_NONE;
>>> +}
>>> +
>>> /**
>>>  * fec_ptp_init
>>>  * @ndev: The FEC network adapter
>>> @@ -558,10 +589,12 @@ static void fec_time_keep(struct work_struct
>>> *work)
>>>  * cyclecounter init routine and exits.
>>>  */
>>>
>>> -void fec_ptp_init(struct platform_device *pdev)
>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index)
>>> {
>>> 	struct net_device *ndev = platform_get_drvdata(pdev);
>>> 	struct fec_enet_private *fep = netdev_priv(ndev);
>>> +	int irq;
>>> +	int ret;
>>>
>>> 	fep->ptp_caps.owner = THIS_MODULE;
>>> 	snprintf(fep->ptp_caps.name, 16, "fec ptp"); @@ -587,6 +620,20 @@
>>> void fec_ptp_init(struct platform_device *pdev)
>>>
>>> 	INIT_DELAYED_WORK(&fep->time_keep, fec_time_keep);
>>>
>>> +	irq = platform_get_irq_byname(pdev, "ptp");
>>> +	if (irq < 0)
>>> +		irq = platform_get_irq(pdev, irq_index);
>>> +	/* Failure to get an irq is not fatal,
>>> +	 * only the PTP_CLOCK_PPS clock events should stop
>>> +	 */
>>> +	if (irq >= 0) {
>>> +		ret = devm_request_irq(&pdev->dev, irq, fec_ptp_interrupt,
>>> +				       0, pdev->name, ndev);
>>> +		if (ret < 0)
>>> +			dev_warn(&pdev->dev, "request for ptp irq
>>> failed(%d)\n",
>>> +				 ret);
>>> +	}
>>> +
>>> 	fep->ptp_clock = ptp_clock_register(&fep->ptp_caps, &pdev->dev);
>>> 	if (IS_ERR(fep->ptp_clock)) {
>>> 		fep->ptp_clock = NULL;
>>> @@ -605,36 +652,3 @@ void fec_ptp_stop(struct platform_device *pdev)
>>> 	if (fep->ptp_clock)
>>> 		ptp_clock_unregister(fep->ptp_clock);
>>> }
>>> -
>>> -/**
>>> - * fec_ptp_check_pps_event
>>> - * @fep: the fec_enet_private structure handle
>>> - *
>>> - * This function check the pps event and reload the timer compare
>counter.
>>> - */
>>> -uint fec_ptp_check_pps_event(struct fec_enet_private *fep) -{
>>> -	u32 val;
>>> -	u8 channel = fep->pps_channel;
>>> -	struct ptp_clock_event event;
>>> -
>>> -	val = readl(fep->hwp + FEC_TCSR(channel));
>>> -	if (val & FEC_T_TF_MASK) {
>>> -		/* Write the next next compare(not the next according the
>>> spec)
>>> -		 * value to the register
>>> -		 */
>>> -		writel(fep->next_counter, fep->hwp + FEC_TCCR(channel));
>>> -		do {
>>> -			writel(val, fep->hwp + FEC_TCSR(channel));
>>> -		} while (readl(fep->hwp + FEC_TCSR(channel)) &
>>> FEC_T_TF_MASK);
>>> -
>>> -		/* Update the counter; */
>>> -		fep->next_counter = (fep->next_counter + fep-
>>>> reload_period) & fep->cc.mask;
>>> -
>>> -		event.type = PTP_CLOCK_PPS;
>>> -		ptp_clock_event(fep->ptp_clock, &event);
>>> -		return 1;
>>> -	}
>>> -
>>> -	return 0;
>>> -}
>>> --
>>> 2.11.0
>>
>>

Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Troy Kisky]

On 10/17/2017 7:30 PM, Andy Duan wrote:
> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>>> This is better for code locality and should slightly speed up normal
>> interrupts.
>>>>
>>>> This also allows PPS clock output to start working for i.mx7. This is
>>>> because
>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>
>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>
>>>> ---
>>>>
>>>> v2: made this change independent of any devicetree change so that old
>>>> dtbs continue to work.
>>>>
>>>> Continue to register ptp clock if interrupt is not found.
>>>> ---
>>>> drivers/net/ethernet/freescale/fec.h      |  3 +-
>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>> drivers/net/ethernet/freescale/fec_ptp.c  | 82
>>>> ++++++++++++++++++--------
>>>> -----
>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>> b/drivers/net/ethernet/freescale/fec.h
>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>> 	u64 ethtool_stats[0];
>>>> };
>>>>
>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>> void fec_ptp_stop(struct platform_device *pdev);  void
>>>> fec_ptp_start_cyclecounter(struct net_device *ndev);  int
>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>
>>>>
>>>>
>> /**********************************************************
>>>> ******************/
>>>> #endif /* FEC_H */
>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>> index 3dc2d771a222..21afabbc560f 100644
>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>> 		ret = IRQ_HANDLED;
>>>> 		complete(&fep->mdio_done);
>>>> 	}
>>>> -
>>>> -	if (fep->ptp_clock)
>>>> -		if (fec_ptp_check_pps_event(fep))
>>>> -			ret = IRQ_HANDLED;
>>>> 	return ret;
>>>> }
>>>>
>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>> 	struct device_node *np = pdev->dev.of_node, *phy_node;
>>>> 	int num_tx_qs;
>>>> 	int num_rx_qs;
>>>> +	char irq_name[8];
>>>> +	int irq_cnt;
>>>>
>>>> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>
>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>> 	if (ret)
>>>> 		goto failed_reset;
>>>>
>>>> +	irq_cnt = platform_irq_count(pdev);
>>>> +	if (irq_cnt > FEC_IRQ_NUM)
>>>> +		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>>>> +	else if (irq_cnt == 2)
>>>> +		irq_cnt = 1;	/* last for ptp */
>>>> +	else if (irq_cnt <= 0)
>>>> +		irq_cnt = 1;	/* Let the for loop fail */
>>>
>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>
>>
>> I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>> NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>> provided.
>> Otherwise, no pps interrupt is assumed.
>> Fortunately this seems to be true currently.
>>
> If pps interrupt is not named, then it limit the last one is pps.
> We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
> Although your current implementation can work with current chips, but it is not really good solution.
> 
>>
>>> And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec interrupt
>> function.
>>
>> How ?  fec_ptp_init will not be called as bufdesc_ex is 0.
>>
> Imx28 also support enhanced buffer descriptor,  if define the ptp clock in dts then bufdesc_ex also can be 1.


Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the relevant code snippets

	}, {
		.name = "imx25-fec",
		.driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_MIB_CLEAR,
	}, {
		.name = "imx27-fec",
		.driver_data = FEC_QUIRK_MIB_CLEAR,
	}, {
		.name = "imx28-fec",
		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
				FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC,
	}, {

....
	fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
	fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
	if (IS_ERR(fep->clk_ptp)) {
		fep->clk_ptp = NULL;
		fep->bufdesc_ex = false;
	}
______________

You could make your way work though, if you remove the pps clock from imx50.dtsi, imx51.dtsi, and
imx53.dtsi.

Then you would not need the FEC_QUIRK_HAS_BUFDESC_EX quirk.
This is all a little off topic though.


> 
> I still suggest to use v1 logic check pps interrupt that need to check irq name.


I could add that back. But it seems redundant when also looking up by name "int0", "int1", "int2".

I would think that we could recommend in the binding documentation that either all are
named, or none are. And I don't think we need to make work mistakes like

interrupt-names = "irq0", "ptp", "int1", "int2"

but
interrupt-names = "int0", "ptp", "int1", "int2"
should currently work.


Let me know if you want me to make work
interrupt-names = "", "ptp", "", ""

or if you just don't think this patch is necessary. I might have some more
reasons to try and persuade you.



> 
>>
>> Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>> interrupt.
>>
>>
>>>
>>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>> your v1 logic check.
>>>
>>>> +
>>>> 	if (fep->bufdesc_ex)
>>>> -		fec_ptp_init(pdev);
>>>> +		fec_ptp_init(pdev, irq_cnt);
>>>>
>>>> 	ret = fec_enet_init(ndev);
>>>> 	if (ret)
>>>> 		goto failed_init;
>>>>
>>>> -	for (i = 0; i < FEC_IRQ_NUM; i++) {
>>>> -		irq = platform_get_irq(pdev, i);
>>>> +	for (i = 0; i < irq_cnt; i++) {
>>>> +		sprintf(irq_name, "int%d", i);
>>>> +		irq = platform_get_irq_byname(pdev, irq_name);
>>>> +		if (irq < 0)
>>>> +			irq = platform_get_irq(pdev, i);
>>>> 		if (irq < 0) {
>>>> -			if (i)
>>>> -				break;
>>>> 			ret = irq;
>>>> 			goto failed_irq;
>>>> 		}


Thanks for the review

Troy

Re: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Troy Kisky]

On 10/18/2017 11:10 AM, Troy Kisky wrote:
> On 10/17/2017 7:30 PM, Andy Duan wrote:
>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday, October 18, 2017 5:34 AM
>>>>> This is better for code locality and should slightly speed up normal
>>> interrupts.
>>>>>
>>>>> This also allows PPS clock output to start working for i.mx7. This is
>>>>> because
>>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>>
>>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>>
>>>>> ---
>>>>>
>>>>> v2: made this change independent of any devicetree change so that old
>>>>> dtbs continue to work.
>>>>>
>>>>> Continue to register ptp clock if interrupt is not found.
>>>>> ---
>>>>> drivers/net/ethernet/freescale/fec.h      |  3 +-
>>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>>> drivers/net/ethernet/freescale/fec_ptp.c  | 82
>>>>> ++++++++++++++++++--------
>>>>> -----
>>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>>> b/drivers/net/ethernet/freescale/fec.h
>>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>>> 	u64 ethtool_stats[0];
>>>>> };
>>>>>
>>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>>> void fec_ptp_stop(struct platform_device *pdev);  void
>>>>> fec_ptp_start_cyclecounter(struct net_device *ndev);  int
>>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
>>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>>
>>>>>
>>>>>
>>> /**********************************************************
>>>>> ******************/
>>>>> #endif /* FEC_H */
>>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>>> index 3dc2d771a222..21afabbc560f 100644
>>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>>> 		ret = IRQ_HANDLED;
>>>>> 		complete(&fep->mdio_done);
>>>>> 	}
>>>>> -
>>>>> -	if (fep->ptp_clock)
>>>>> -		if (fec_ptp_check_pps_event(fep))
>>>>> -			ret = IRQ_HANDLED;
>>>>> 	return ret;
>>>>> }
>>>>>
>>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>>> 	struct device_node *np = pdev->dev.of_node, *phy_node;
>>>>> 	int num_tx_qs;
>>>>> 	int num_rx_qs;
>>>>> +	char irq_name[8];
>>>>> +	int irq_cnt;
>>>>>
>>>>> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>>
>>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>>> 	if (ret)
>>>>> 		goto failed_reset;
>>>>>
>>>>> +	irq_cnt = platform_irq_count(pdev);
>>>>> +	if (irq_cnt > FEC_IRQ_NUM)
>>>>> +		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>>>>> +	else if (irq_cnt == 2)
>>>>> +		irq_cnt = 1;	/* last for ptp */
>>>>> +	else if (irq_cnt <= 0)
>>>>> +		irq_cnt = 1;	/* Let the for loop fail */
>>>>
>>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>>
>>>
>>> I don't. If the pps interrupt is named, the named interrupt will be used. If it is
>>> NOT named, the last interrupt is used, if 2 interrupts, or >3 interrupt are
>>> provided.
>>> Otherwise, no pps interrupt is assumed.
>>> Fortunately this seems to be true currently.
>>>
>> If pps interrupt is not named, then it limit the last one is pps.
>> We cannot get the pps interrupt based on current chip interrupt define, we never know the future chip how to define interrupt.
>> Although your current implementation can work with current chips, but it is not really good solution.
>>
>>>
>>>> And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec interrupt
>>> function.
>>>
>>> How ?  fec_ptp_init will not be called as bufdesc_ex is 0.
>>>
>> Imx28 also support enhanced buffer descriptor,  if define the ptp clock in dts then bufdesc_ex also can be 1.
> 
> 
> Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the relevant code snippets
> 
> 	}, {
> 		.name = "imx25-fec",
> 		.driver_data = FEC_QUIRK_USE_GASKET | FEC_QUIRK_MIB_CLEAR,
> 	}, {
> 		.name = "imx27-fec",
> 		.driver_data = FEC_QUIRK_MIB_CLEAR,
> 	}, {
> 		.name = "imx28-fec",
> 		.driver_data = FEC_QUIRK_ENET_MAC | FEC_QUIRK_SWAP_FRAME |
> 				FEC_QUIRK_SINGLE_MDIO | FEC_QUIRK_HAS_RACC,
> 	}, {
> 
> ....
> 	fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
> 	fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
> 	if (IS_ERR(fep->clk_ptp)) {
> 		fep->clk_ptp = NULL;
> 		fep->bufdesc_ex = false;
> 	}
> ______________
> 
> You could make your way work though, if you remove the pps clock from imx50.dtsi, imx51.dtsi, and
> imx53.dtsi.


Whoops I meant "ptp" clock.

That brings up a question though.

interrupt-names = "int0", "int1", "int2", "pps"

may be more accurate. Is there any desire for me to use "pps" instead ?



> 
> Then you would not need the FEC_QUIRK_HAS_BUFDESC_EX quirk.
> This is all a little off topic though.
> 
> 
>>
>> I still suggest to use v1 logic check pps interrupt that need to check irq name.
> 
> 
> I could add that back. But it seems redundant when also looking up by name "int0", "int1", "int2".
> 
> I would think that we could recommend in the binding documentation that either all are
> named, or none are. And I don't think we need to make work mistakes like
> 
> interrupt-names = "irq0", "ptp", "int1", "int2"
> 
> but
> interrupt-names = "int0", "ptp", "int1", "int2"
> should currently work.
> 
> 
> Let me know if you want me to make work
> interrupt-names = "", "ptp", "", ""
> 
> or if you just don't think this patch is necessary. I might have some more
> reasons to try and persuade you.
> 
> 
> 
>>
>>>
>>> Also, if only 1 interrupt is provided, it is assumed there is no unnamed pps
>>> interrupt.
>>>
>>>
>>>>
>>>> I suggest to use .platform_get_irq_byname() to get pps(ptp) interrupt like
>>> your v1 logic check.
>>>>
>>>>> +
>>>>> 	if (fep->bufdesc_ex)
>>>>> -		fec_ptp_init(pdev);
>>>>> +		fec_ptp_init(pdev, irq_cnt);
>>>>>
>>>>> 	ret = fec_enet_init(ndev);
>>>>> 	if (ret)
>>>>> 		goto failed_init;
>>>>>
>>>>> -	for (i = 0; i < FEC_IRQ_NUM; i++) {
>>>>> -		irq = platform_get_irq(pdev, i);
>>>>> +	for (i = 0; i < irq_cnt; i++) {
>>>>> +		sprintf(irq_name, "int%d", i);
>>>>> +		irq = platform_get_irq_byname(pdev, irq_name);
>>>>> +		if (irq < 0)
>>>>> +			irq = platform_get_irq(pdev, i);
>>>>> 		if (irq < 0) {
>>>>> -			if (i)
>>>>> -				break;
>>>>> 			ret = irq;
>>>>> 			goto failed_irq;
>>>>> 		}
> 
> 
> Thanks for the review
> 
> Troy
> 

RE: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt routine

[Andy Duan]

From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Thursday, October 19, 2017 2:30 AM
>On 10/18/2017 11:10 AM, Troy Kisky wrote:
>> On 10/17/2017 7:30 PM, Andy Duan wrote:
>>> From: Troy Kisky <troy.kisky@boundarydevices.com> Sent: Wednesday,
>>> October 18, 2017 5:34 AM
>>>>>> This is better for code locality and should slightly speed up
>>>>>> normal
>>>> interrupts.
>>>>>>
>>>>>> This also allows PPS clock output to start working for i.mx7. This
>>>>>> is because
>>>>>> i.mx7 was already using the limit of 3 interrupts, and needed another.
>>>>>>
>>>>>> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> v2: made this change independent of any devicetree change so that
>>>>>> old dtbs continue to work.
>>>>>>
>>>>>> Continue to register ptp clock if interrupt is not found.
>>>>>> ---
>>>>>> drivers/net/ethernet/freescale/fec.h      |  3 +-
>>>>>> drivers/net/ethernet/freescale/fec_main.c | 25 ++++++----
>>>>>> drivers/net/ethernet/freescale/fec_ptp.c  | 82
>>>>>> ++++++++++++++++++--------
>>>>>> -----
>>>>>> 3 files changed, 65 insertions(+), 45 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/ethernet/freescale/fec.h
>>>>>> b/drivers/net/ethernet/freescale/fec.h
>>>>>> index ede1876a9a19..be56ac1f1ac4 100644
>>>>>> --- a/drivers/net/ethernet/freescale/fec.h
>>>>>> +++ b/drivers/net/ethernet/freescale/fec.h
>>>>>> @@ -582,12 +582,11 @@ struct fec_enet_private {
>>>>>> 	u64 ethtool_stats[0];
>>>>>> };
>>>>>>
>>>>>> -void fec_ptp_init(struct platform_device *pdev);
>>>>>> +void fec_ptp_init(struct platform_device *pdev, int irq_index);
>>>>>> void fec_ptp_stop(struct platform_device *pdev);  void
>>>>>> fec_ptp_start_cyclecounter(struct net_device *ndev);  int
>>>>>> fec_ptp_set(struct net_device *ndev, struct ifreq *ifr);  int
>>>>>> fec_ptp_get(struct net_device *ndev, struct ifreq *ifr); -uint
>>>>>> fec_ptp_check_pps_event(struct fec_enet_private *fep);
>>>>>>
>>>>>>
>>>>>>
>>>>
>/**********************************************************
>>>>>> ******************/
>>>>>> #endif /* FEC_H */
>>>>>> diff --git a/drivers/net/ethernet/freescale/fec_main.c
>>>>>> b/drivers/net/ethernet/freescale/fec_main.c
>>>>>> index 3dc2d771a222..21afabbc560f 100644
>>>>>> --- a/drivers/net/ethernet/freescale/fec_main.c
>>>>>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>>>>>> @@ -1602,10 +1602,6 @@ fec_enet_interrupt(int irq, void *dev_id)
>>>>>> 		ret = IRQ_HANDLED;
>>>>>> 		complete(&fep->mdio_done);
>>>>>> 	}
>>>>>> -
>>>>>> -	if (fep->ptp_clock)
>>>>>> -		if (fec_ptp_check_pps_event(fep))
>>>>>> -			ret = IRQ_HANDLED;
>>>>>> 	return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -3325,6 +3321,8 @@ fec_probe(struct platform_device *pdev)
>>>>>> 	struct device_node *np = pdev->dev.of_node, *phy_node;
>>>>>> 	int num_tx_qs;
>>>>>> 	int num_rx_qs;
>>>>>> +	char irq_name[8];
>>>>>> +	int irq_cnt;
>>>>>>
>>>>>> 	fec_enet_get_queue_num(pdev, &num_tx_qs, &num_rx_qs);
>>>>>>
>>>>>> @@ -3465,18 +3463,27 @@ fec_probe(struct platform_device *pdev)
>>>>>> 	if (ret)
>>>>>> 		goto failed_reset;
>>>>>>
>>>>>> +	irq_cnt = platform_irq_count(pdev);
>>>>>> +	if (irq_cnt > FEC_IRQ_NUM)
>>>>>> +		irq_cnt = FEC_IRQ_NUM;	/* last for ptp */
>>>>>> +	else if (irq_cnt == 2)
>>>>>> +		irq_cnt = 1;	/* last for ptp */
>>>>>> +	else if (irq_cnt <= 0)
>>>>>> +		irq_cnt = 1;	/* Let the for loop fail */
>>>>>
>>>>> Don't do like this. Don't suppose pps interrupt is the last one.
>>>>
>>>>
>>>> I don't. If the pps interrupt is named, the named interrupt will be
>>>> used. If it is NOT named, the last interrupt is used, if 2
>>>> interrupts, or >3 interrupt are provided.
>>>> Otherwise, no pps interrupt is assumed.
>>>> Fortunately this seems to be true currently.
>>>>
>>> If pps interrupt is not named, then it limit the last one is pps.
>>> We cannot get the pps interrupt based on current chip interrupt define,
>we never know the future chip how to define interrupt.
>>> Although your current implementation can work with current chips, but it is
>not really good solution.
>>>
>>>>
>>>>> And if irq_cnt is 1 like imx28/imx5x,  the patch will break fec
>>>>> interrupt
>>>> function.
>>>>
>>>> How ?  fec_ptp_init will not be called as bufdesc_ex is 0.
>>>>
>>> Imx28 also support enhanced buffer descriptor,  if define the ptp clock in
>dts then bufdesc_ex also can be 1.
>>
>>
>> Only if FEC_QUIRK_HAS_BUFDESC_EX is set, which it is not. Here's the
>> relevant code snippets
>>
>> 	}, {
>> 		.name = "imx25-fec",
>> 		.driver_data = FEC_QUIRK_USE_GASKET |
>FEC_QUIRK_MIB_CLEAR,
>> 	}, {
>> 		.name = "imx27-fec",
>> 		.driver_data = FEC_QUIRK_MIB_CLEAR,
>> 	}, {
>> 		.name = "imx28-fec",
>> 		.driver_data = FEC_QUIRK_ENET_MAC |
>FEC_QUIRK_SWAP_FRAME |
>> 				FEC_QUIRK_SINGLE_MDIO |
>FEC_QUIRK_HAS_RACC,
>> 	}, {
>>
>> ....
>> 	fep->bufdesc_ex = fep->quirks & FEC_QUIRK_HAS_BUFDESC_EX;
>> 	fep->clk_ptp = devm_clk_get(&pdev->dev, "ptp");
>> 	if (IS_ERR(fep->clk_ptp)) {
>> 		fep->clk_ptp = NULL;
>> 		fep->bufdesc_ex = false;
>> 	}
>> ______________
>>
>> You could make your way work though, if you remove the pps clock from
>> imx50.dtsi, imx51.dtsi, and imx53.dtsi.
>
>
>Whoops I meant "ptp" clock.
>
>That brings up a question though.
>
>interrupt-names = "int0", "int1", "int2", "pps"
>
>may be more accurate. Is there any desire for me to use "pps" instead ?
>
I agree this named method.
Set the property "interrupt-names" is optional properties that don't break other platforms.