Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR

On Fri, Jan 27, 2017 at 09:11:35PM +0300, Sergei Shtylyov wrote:

On 01/27/2017 09:07 PM, Simon Horman wrote:

quoted

quoted

quoted

quoted

quoted

From: Kazuya Mizuguchi <redacted>

This patch enables tx and rx clock internal delay modes (TDM and RDM).

This is to address a failure in the case of 1Gbps communication using the
by salvator-x board with the KSZ9031RNX phy. This has been reported to
occur with both the r8a7795 (H3) and r8a7796 (M3-W) SoCs.

With this change APSR internal delay modes are enabled for
"rgmii-id", "rgmii-rxid" and "rgmii-txid" phy modes as follows:

phy mode   | ASPR delay mode
-----------+----------------
rgmii-id   | TDM and RDM
rgmii-rxid | RDM
rgmii-txid | TDM

Signed-off-by: Kazuya Mizuguchi <redacted>
Signed-off-by: Simon Horman <redacted>

---
v1 [Simon Horman]
- Combined patches
- Reworded changelog

v0 [Kazuya Mizuguchi]
---
drivers/net/ethernet/renesas/ravb.h      | 10 ++++++++++
drivers/net/ethernet/renesas/ravb_main.c | 29 +++++++++++++++++++++++++++++
2 files changed, 39 insertions(+)

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index f1109661a533..d7c91d48cc48 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h

[...]

quoted

@@ -248,6 +249,15 @@ enum ESR_BIT {

ESR_EIL		= 0x00001000,
};

+/* APSR */
+enum APSR_BIT {
+	APSR_MEMS		= 0x00000002,

 Not documented in the revision 0.5 of the gen3 manual...

It is in version 0.53 of the documentation but I think it can be dropped
from this patch as its unused.

  No need to drop, let it be. :-)

quoted

quoted

quoted

+	APSR_CMSW		= 0x00000010,

I think the above can also be dropped as it is unused.

  No need.

Ok, lets leave it and MEMS.

quoted

quoted

quoted

quoted

+	APSR_DM			= 0x00006000,

 ... and neither this field. Are these documented in the latter revs?

This one is not. I can ask Mizuguchi-san to confirm it exists (with himself).

  The whole patch depends on it! If it's not documented, I'd like a comment
added, like I did for the other undocumented things in this driver...

Sure.

quoted

[...]

quoted

quoted

quoted

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 89ac1e3f6175..9fb4c04c5885 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c

@@ -1904,6 +1904,29 @@ static void ravb_set_config_mode(struct net_device *ndev)

}
}

+static void ravb_set_delay_mode(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (priv->chip_id != RCAR_GEN2) {
+		switch (priv->phy_interface) {
+		case PHY_INTERFACE_MODE_RGMII_ID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM |
+				   APSR_DM_TDM);
+			break;
+		case PHY_INTERFACE_MODE_RGMII_RXID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_RDM);
+			break;
+		case PHY_INTERFACE_MODE_RGMII_TXID:
+			ravb_modify(ndev, APSR, APSR_DM, APSR_DM_TDM);
+			break;
+		default:
+			ravb_modify(ndev, APSR, APSR_DM, 0);
+			break;
+		}

 How about doing ravb_modify() only once?

Something like this?

	int set = 0;

	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_RXID)
	    set |= APSR_DM_RDM;

	if (priv->phy_interface == PHY_INTERFACE_MODE_RGMII_ID ||
	    priv->phy_interface == PHY_INTERFACE_MODE_RGMII_TXID)
	    set |= APSR_DM_TDM;

	ravb_modify(ndev, APSR, APSR_DM, set);

I don't really see any advantage to it but I can change things around
however you like.

  I didn't mean to replace *switch*, actually -- just to move ravb_modify()
out of it.

Ok, I will make it so.

   Your variant is probably even better! :-)
   I'm not sure how good gcc is at merging the similar code paths,
simplifying its work seems worth it to me. The less repetitive code, the
better.

Ok, thanks. I'll go with my version.