Date:   Thu, 19 Oct 2017 06:25:39 +0000
From:   Andy Duan <fugang.duan@....com>
To:     Troy Kisky <troy.kisky@...ndarydevices.com>,
        "shawn.guo@...aro.org" <shawn.guo@...aro.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>
CC:     Fabio Estevam <fabio.estevam@....com>,
        "lznuaa@...il.com" <lznuaa@...il.com>,
        "andrew@...n.ch" <andrew@...n.ch>
Subject: RE: [PATCH net v2 2/2] net: fec: Let fec_ptp have its own interrupt
 routine

From: Troy Kisky <troy.kisky@...ndarydevices.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@...ndarydevices.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@...ndarydevices.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.

Powered by blists - more mailing lists