From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from kirsty.vergenet.net ([202.4.237.240]:37306 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750828AbdA0SWS (ORCPT ); Fri, 27 Jan 2017 13:22:18 -0500 Date: Fri, 27 Jan 2017 19:21:14 +0100 From: Simon Horman To: Sergei Shtylyov Cc: David Miller , Magnus Damm , netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org, Kazuya Mizuguchi Subject: Re: [PATCH net-next 1/2] ravb: Add tx and rx clock internal delays mode of APSR Message-ID: <20170127182113.GB13402@verge.net.au> References: <1485515090-7570-1-git-send-email-horms+renesas@verge.net.au> <1485515090-7570-2-git-send-email-horms+renesas@verge.net.au> <813af981-9a1b-8e45-1fed-788047ab26b5@cogentembedded.com> <20170127164921.GB7336@verge.net.au> <20170127180701.GA13402@verge.net.au> <77bad70c-49f5-e70c-1fd6-929a394f9cbe@cogentembedded.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <77bad70c-49f5-e70c-1fd6-929a394f9cbe@cogentembedded.com> Sender: linux-renesas-soc-owner@vger.kernel.org List-ID: On Fri, Jan 27, 2017 at 09:11:35PM +0300, Sergei Shtylyov wrote: > On 01/27/2017 09:07 PM, Simon Horman wrote: > > >>>>>From: Kazuya Mizuguchi > >>>>> > >>>>>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 > >>>>>Signed-off-by: Simon Horman > >>>>> > >>>>>--- > >>>>>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 > >>>>[...] > >>>>>@@ -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. :-) > >> > >>>>>+ APSR_CMSW = 0x00000010, > >>> > >>>I think the above can also be dropped as it is unused. > >> > >> No need. > > > >Ok, lets leave it and MEMS. > > > >>>>>+ 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. > > > >>[...] > >>>>>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.