[PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120

Radu Pirea (NXP OSS) posted 14 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120
Posted by Radu Pirea (NXP OSS) 2 years, 7 months ago
On TJA1120, the external trigger timestamp now has a VALID bit. This
changes the logic and we can't use the TJA1103 procedure.

For TJA1103, we can always read a valid timestamp from the registers,
compare the new timestamp with the old timestamp and, if they are not the
same, an event occurred. This logic cannot be applied for TJA1120 because
the timestamp is 0 if the VALID bit is not set.

Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/phy/nxp-c45-tja11xx.c | 36 +++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/nxp-c45-tja11xx.c b/drivers/net/phy/nxp-c45-tja11xx.c
index 4b40be45c955..0ed96d696bad 100644
--- a/drivers/net/phy/nxp-c45-tja11xx.c
+++ b/drivers/net/phy/nxp-c45-tja11xx.c
@@ -34,6 +34,8 @@
 #define TJA1120_GLOBAL_INFRA_IRQ_STATUS	0x2C0C
 #define TJA1120_DEV_BOOT_DONE		BIT(1)
 
+#define TJA1120_VEND1_PTP_TRIG_DATA_S	0x1070
+
 #define TJA1120_EGRESS_TS_DATA_S	0x9060
 #define TJA1120_EGRESS_TS_END		0x9067
 #define TJA1120_TS_VALID		BIT(0)
@@ -268,6 +270,7 @@ struct nxp_c45_phy_data {
 	void (*counters_enable)(struct phy_device *phydev);
 	bool (*get_egressts)(struct nxp_c45_phy *priv,
 			     struct nxp_c45_hwts *hwts);
+	bool (*get_extts)(struct nxp_c45_phy *priv, struct timespec64 *extts);
 	void (*ptp_init)(struct phy_device *phydev);
 	void (*ptp_enable)(struct phy_device *phydev, bool enable);
 	void (*nmi_handler)(struct phy_device *phydev,
@@ -504,7 +507,7 @@ static bool nxp_c45_match_ts(struct ptp_header *header,
 	       header->domain_number  == hwts->domain_number;
 }
 
-static void nxp_c45_get_extts(struct nxp_c45_phy *priv,
+static bool nxp_c45_get_extts(struct nxp_c45_phy *priv,
 			      struct timespec64 *extts)
 {
 	const struct nxp_c45_regmap *regmap = nxp_c45_get_regmap(priv->phydev);
@@ -519,6 +522,23 @@ static void nxp_c45_get_extts(struct nxp_c45_phy *priv,
 				      regmap->vend1_ext_trg_data_3) << 16;
 	phy_write_mmd(priv->phydev, MDIO_MMD_VEND1,
 		      regmap->vend1_ext_trg_ctrl, RING_DONE);
+
+	return true;
+}
+
+static bool tja1120_get_extts(struct nxp_c45_phy *priv,
+			      struct timespec64 *extts)
+{
+	bool valid;
+	u16 reg;
+
+	reg = phy_read_mmd(priv->phydev, MDIO_MMD_VEND1,
+			   TJA1120_VEND1_PTP_TRIG_DATA_S);
+	valid = !!(reg & TJA1120_TS_VALID);
+	if (valid)
+		return nxp_c45_get_extts(priv, extts);
+
+	return valid;
 }
 
 static void nxp_c45_read_egress_ts(struct nxp_c45_phy *priv,
@@ -628,12 +648,12 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
 	bool reschedule = false;
 	struct timespec64 ts;
 	struct sk_buff *skb;
-	bool txts_valid;
+	bool ts_valid;
 	u32 ts_raw;
 
 	while (!skb_queue_empty_lockless(&priv->tx_queue) && poll_txts) {
-		txts_valid = data->get_egressts(priv, &hwts);
-		if (unlikely(!txts_valid)) {
+		ts_valid = data->get_egressts(priv, &hwts);
+		if (unlikely(!ts_valid)) {
 			/* Still more skbs in the queue */
 			reschedule = true;
 			break;
@@ -654,9 +674,9 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
 		netif_rx(skb);
 	}
 
-	if (priv->extts) {
-		nxp_c45_get_extts(priv, &ts);
-		if (timespec64_compare(&ts, &priv->extts_ts) != 0) {
+	if (priv->extts && data->get_extts) {
+		ts_valid = data->get_extts(priv, &ts);
+		if (ts_valid && timespec64_compare(&ts, &priv->extts_ts) != 0) {
 			priv->extts_ts = ts;
 			event.index = priv->extts_index;
 			event.type = PTP_CLOCK_EXTTS;
@@ -1702,6 +1722,7 @@ static const struct nxp_c45_phy_data tja1103_phy_data = {
 	.ack_ptp_irq = false,
 	.counters_enable = tja1103_counters_enable,
 	.get_egressts = nxp_c45_get_hwtxts,
+	.get_extts = nxp_c45_get_extts,
 	.ptp_init = tja1103_ptp_init,
 	.ptp_enable = tja1103_ptp_enable,
 	.nmi_handler = tja1103_nmi_handler,
@@ -1816,6 +1837,7 @@ static const struct nxp_c45_phy_data tja1120_phy_data = {
 	.ack_ptp_irq = true,
 	.counters_enable = tja1120_counters_enable,
 	.get_egressts = tja1120_get_hwtxts,
+	.get_extts = tja1120_get_extts,
 	.ptp_init = tja1120_ptp_init,
 	.ptp_enable = tja1120_ptp_enable,
 	.nmi_handler = tja1120_nmi_handler,
-- 
2.34.1
Re: [PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120
Posted by Horatiu Vultur 2 years, 7 months ago
The 06/16/2023 16:53, Radu Pirea (NXP OSS) wrote:

Hi Radu,

> 
>  static void nxp_c45_read_egress_ts(struct nxp_c45_phy *priv,
> @@ -628,12 +648,12 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
>         bool reschedule = false;
>         struct timespec64 ts;
>         struct sk_buff *skb;
> -       bool txts_valid;
> +       bool ts_valid;
>         u32 ts_raw;
> 
>         while (!skb_queue_empty_lockless(&priv->tx_queue) && poll_txts) {
> -               txts_valid = data->get_egressts(priv, &hwts);
> -               if (unlikely(!txts_valid)) {
> +               ts_valid = data->get_egressts(priv, &hwts);
> +               if (unlikely(!ts_valid)) {
>                         /* Still more skbs in the queue */
>                         reschedule = true;
>                         break;
> @@ -654,9 +674,9 @@ static long nxp_c45_do_aux_work(struct ptp_clock_info *ptp)
>                 netif_rx(skb);
>         }
> 
> -       if (priv->extts) {
> -               nxp_c45_get_extts(priv, &ts);
> -               if (timespec64_compare(&ts, &priv->extts_ts) != 0) {
> +       if (priv->extts && data->get_extts) {

The data->get_extts can't be null. So I don't think you need this check.

> +               ts_valid = data->get_extts(priv, &ts);
> +               if (ts_valid && timespec64_compare(&ts, &priv->extts_ts) != 0) {
>                         priv->extts_ts = ts;
>                         event.index = priv->extts_index;
>                         event.type = PTP_CLOCK_EXTTS;
> @@ -1702,6 +1722,7 @@ static const struct nxp_c45_phy_data tja1103_phy_data = {
>         .ack_ptp_irq = false,
>         .counters_enable = tja1103_counters_enable,
>         .get_egressts = nxp_c45_get_hwtxts,
> +       .get_extts = nxp_c45_get_extts,
>         .ptp_init = tja1103_ptp_init,
>         .ptp_enable = tja1103_ptp_enable,
>         .nmi_handler = tja1103_nmi_handler,
> @@ -1816,6 +1837,7 @@ static const struct nxp_c45_phy_data tja1120_phy_data = {
>         .ack_ptp_irq = true,
>         .counters_enable = tja1120_counters_enable,
>         .get_egressts = tja1120_get_hwtxts,
> +       .get_extts = tja1120_get_extts,
>         .ptp_init = tja1120_ptp_init,
>         .ptp_enable = tja1120_ptp_enable,
>         .nmi_handler = tja1120_nmi_handler,
> --
> 2.34.1
> 
> 

-- 
/Horatiu
Re: [PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120
Posted by Radu Pirea (OSS) 2 years, 7 months ago
On 19.06.2023 11:49, Horatiu Vultur wrote:
> The data->get_extts can't be null. So I don't think you need this check.

I kinda agree with this because _I wrote the driver and I know what it 
does_, but on the other hand don't want to fight with any static analyzer.

> 
> --
> /Horatiu

-- 
Radu P.
Re: [PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120
Posted by Horatiu Vultur 2 years, 7 months ago
The 06/19/2023 13:07, Radu Pirea (OSS) wrote:
> 
> On 19.06.2023 11:49, Horatiu Vultur wrote:
> > The data->get_extts can't be null. So I don't think you need this check.
> 
> I kinda agree with this because _I wrote the driver and I know what it
> does_, but on the other hand don't want to fight with any static analyzer.

Yes, but then wouldn't be an issue with the static analyzer tools that
can't detect this kind of code?

> 
> > 
> > --
> > /Horatiu
> 
> --
> Radu P.

-- 
/Horatiu
Re: [PATCH net-next v1 12/14] net: phy: nxp-c45-tja11xx: read ext trig ts TJA1120
Posted by Radu Pirea (OSS) 2 years, 7 months ago

On 19.06.2023 13:48, Horatiu Vultur wrote:
> Caution: This is an external email. Please take care when clicking links or opening attachments. When in doubt, report the message using the 'Report this email' button
> 
> 
> The 06/19/2023 13:07, Radu Pirea (OSS) wrote:
>>
>> On 19.06.2023 11:49, Horatiu Vultur wrote:
>>> The data->get_extts can't be null. So I don't think you need this check.
>>
>> I kinda agree with this because _I wrote the driver and I know what it
>> does_, but on the other hand don't want to fight with any static analyzer.
> 
> Yes, but then wouldn't be an issue with the static analyzer tools that
> can't detect this kind of code?

You are right. I will remove the checks. They are useless in the end. A 
check of private data will be introduced only if a future PHY needs it.

Thank you.

> 
>>
>>>
>>> --
>>> /Horatiu
>>
>> --
>> Radu P.
> 
> --
> /Horatiu

-- 
Radu P.