From: "David Rivshin (Allworx)" <drivshin.allworx@gmail.com> To: Markus Brunner <systemprogrammierung.brunner@gmail.com>, netdev@vger.kernel.org Cc: linux-arm-kernel@lists.infradead.org, linux-omap@vger.kernel.org, David Miller <davem@davemloft.net>, Mugunthan V N <mugunthanvnm@ti.com>, Pascal Speck <kernel@iktek.de>, Daniel Trautmann <dtrautmann@ibhsoftec-sps.de> Subject: Re: [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY Date: Mon, 14 Dec 2015 13:04:46 -0500 [thread overview] Message-ID: <20151214130446.1e14fbb8.drivshin.allworx@gmail.com> (raw) In-Reply-To: <6416396.ZOturxhSar@localhost> On Sat, 12 Dec 2015 16:44:19 +0100 Markus Brunner <systemprogrammierung.brunner@gmail.com> wrote: > On Wednesday 09 December 2015 22:31:15 David Rivshin wrote: > ... > > This patch was originally developed in parallel with 1f71e8c96fc6 to > > accomplish the same goal. When I replaced this patch with > > 1f71e8c96fc6, I found that it did not work on my hardware (which > > uses RGMII), so I am submitting this patch now as a bugfix. > > Your patch works fine on my board, which uses MII and dual_emac with > a fixed_phy and a real one. Thanks for checking. The only dual_emac board I have available is the EVMSK, which has two real PHYs. I'm not sure of the usual etiquette (and Google was unhelpful), should I add a Tested-by on the next version? > > Besides fixing the bug mentioned in the commit log, there are a few > > other differences to note: > > * If both "phy_id" and a "fixed-link" subnode are present this > > patch will use the "phy_id" property. This should preserve current > > behavior with existing devicetrees that might incorrectly have > > both. This motivates the biggest difference in code organization > > from 1f71e8c96fc6. > > * Some error messages have been tweaked to reflect the slightly > > changed meanings of the checks. > > I wanted to keep changes small and didn't spend too much thinking > about already broken devicetrees. Since my patch is quite new, I I'm honestly not sure it's an important consideration myself. Most patches I've seen in this area for this or other drivers do not take such behavior into account (e.g. the phy-handle parsing that went in to cpsw in 4.3). I would generally feel more comfortable with such a behavior tweak (minor as it is) before 4.4 is released, to avoid ping-ponging the behavior. But given how far along the cycle is, I'm not sure about the chances of that. > don't see any problems with subtle changes like that. However you > should update the documentation as well. Your patch already updated .../bindings/net/cpsw.txt, which this patch left alone. Are you referring to some other documentation, or do you think I should update the binding documentation to state that phy_id takes precedence over fixed-link? I figured that such devicetrees were still officially malformed, so I thought the existing text was appropriate. > > * of_node_get() is called on slave_node before passing the result > > to of_phy_find_device(). This increments the reference count, which > > I believe is correct for this situation, but I'm not certain of > > that. > I think you are right. At least most other drivers calling > of_phy_register_fixed_link(), call of_node_get afterwards. I can't > remember where I got my "inspiration" for the patch. I made it almost > a year ago and now 3 other guys tinker with the same feature? What a > coincidence ;-) Perhaps not a complete coincidence. I first wrote this patch a few months ago against 4.1. While I had always intended on submitting when time allowed, testing your patch gave me an extra push. [...] > > --- a/drivers/net/ethernet/ti/cpsw.c > > +++ b/drivers/net/ethernet/ti/cpsw.c > > @@ -26,6 +26,7 @@ > > #include <linux/netdevice.h> > > #include <linux/net_tstamp.h> > > #include <linux/phy.h> > > +#include <linux/phy_fixed.h> > > The prototypes for of_*_fixed_link are in linux/of_mdio.h, which is > already included. You are correct; I forget why I had originally included that. I will remove it for v2.
WARNING: multiple messages have this Message-ID (diff)
From: drivshin.allworx@gmail.com (David Rivshin (Allworx)) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY Date: Mon, 14 Dec 2015 13:04:46 -0500 [thread overview] Message-ID: <20151214130446.1e14fbb8.drivshin.allworx@gmail.com> (raw) In-Reply-To: <6416396.ZOturxhSar@localhost> On Sat, 12 Dec 2015 16:44:19 +0100 Markus Brunner <systemprogrammierung.brunner@gmail.com> wrote: > On Wednesday 09 December 2015 22:31:15 David Rivshin wrote: > ... > > This patch was originally developed in parallel with 1f71e8c96fc6 to > > accomplish the same goal. When I replaced this patch with > > 1f71e8c96fc6, I found that it did not work on my hardware (which > > uses RGMII), so I am submitting this patch now as a bugfix. > > Your patch works fine on my board, which uses MII and dual_emac with > a fixed_phy and a real one. Thanks for checking. The only dual_emac board I have available is the EVMSK, which has two real PHYs. I'm not sure of the usual etiquette (and Google was unhelpful), should I add a Tested-by on the next version? > > Besides fixing the bug mentioned in the commit log, there are a few > > other differences to note: > > * If both "phy_id" and a "fixed-link" subnode are present this > > patch will use the "phy_id" property. This should preserve current > > behavior with existing devicetrees that might incorrectly have > > both. This motivates the biggest difference in code organization > > from 1f71e8c96fc6. > > * Some error messages have been tweaked to reflect the slightly > > changed meanings of the checks. > > I wanted to keep changes small and didn't spend too much thinking > about already broken devicetrees. Since my patch is quite new, I I'm honestly not sure it's an important consideration myself. Most patches I've seen in this area for this or other drivers do not take such behavior into account (e.g. the phy-handle parsing that went in to cpsw in 4.3). I would generally feel more comfortable with such a behavior tweak (minor as it is) before 4.4 is released, to avoid ping-ponging the behavior. But given how far along the cycle is, I'm not sure about the chances of that. > don't see any problems with subtle changes like that. However you > should update the documentation as well. Your patch already updated .../bindings/net/cpsw.txt, which this patch left alone. Are you referring to some other documentation, or do you think I should update the binding documentation to state that phy_id takes precedence over fixed-link? I figured that such devicetrees were still officially malformed, so I thought the existing text was appropriate. > > * of_node_get() is called on slave_node before passing the result > > to of_phy_find_device(). This increments the reference count, which > > I believe is correct for this situation, but I'm not certain of > > that. > I think you are right. At least most other drivers calling > of_phy_register_fixed_link(), call of_node_get afterwards. I can't > remember where I got my "inspiration" for the patch. I made it almost > a year ago and now 3 other guys tinker with the same feature? What a > coincidence ;-) Perhaps not a complete coincidence. I first wrote this patch a few months ago against 4.1. While I had always intended on submitting when time allowed, testing your patch gave me an extra push. [...] > > --- a/drivers/net/ethernet/ti/cpsw.c > > +++ b/drivers/net/ethernet/ti/cpsw.c > > @@ -26,6 +26,7 @@ > > #include <linux/netdevice.h> > > #include <linux/net_tstamp.h> > > #include <linux/phy.h> > > +#include <linux/phy_fixed.h> > > The prototypes for of_*_fixed_link are in linux/of_mdio.h, which is > already included. You are correct; I forget why I had originally included that. I will remove it for v2.
next prev parent reply other threads:[~2015-12-14 18:04 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-12-10 3:31 [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY David Rivshin (Allworx) 2015-12-10 3:31 ` David Rivshin (Allworx) 2015-12-12 15:44 ` Markus Brunner 2015-12-12 15:44 ` Markus Brunner 2015-12-14 18:04 ` David Rivshin (Allworx) [this message] 2015-12-14 18:04 ` David Rivshin (Allworx) 2015-12-16 6:39 ` Markus Brunner 2015-12-16 6:39 ` Markus Brunner 2015-12-17 4:02 ` [PATCH v2 0/3] drivers: net: cpsw: Fix bugs in fixed-link PHY DT parsing David Rivshin (Allworx) 2015-12-17 4:02 ` David Rivshin (Allworx) 2015-12-04 15:55 ` [PATCH] ethernet:ti:cpsw: fix phy identification with multiple slaves on fixed-phy Pascal Speck (Iktek) 2015-12-17 4:02 ` [PATCH v2 1/3] " David Rivshin (Allworx) 2015-12-17 4:02 ` David Rivshin (Allworx) 2015-12-17 4:02 ` [PATCH v2 2/3] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY David Rivshin (Allworx) 2015-12-17 4:02 ` David Rivshin (Allworx) 2015-12-17 4:02 ` [PATCH v2 3/3] drivers: net: cpsw: increment reference count on fixed-link PHY node David Rivshin (Allworx) 2015-12-17 4:02 ` David Rivshin (Allworx) 2015-12-17 20:45 ` [PATCH v2 0/3] drivers: net: cpsw: Fix bugs in fixed-link PHY DT parsing David Miller 2015-12-17 20:45 ` David Miller 2015-12-18 10:20 ` Daniel Trautmann 2015-12-18 10:20 ` Daniel Trautmann 2015-12-18 22:06 ` David Rivshin (Allworx) 2015-12-18 22:06 ` David Rivshin (Allworx) 2015-12-18 22:06 ` David Rivshin (Allworx) 2015-12-18 19:46 ` David Miller 2015-12-18 19:46 ` David Miller 2015-12-17 5:04 ` [PATCH] drivers: net: cpsw: fix RMII/RGMII mode when used with fixed-link PHY David Rivshin (Allworx) 2015-12-17 5:04 ` David Rivshin (Allworx)
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20151214130446.1e14fbb8.drivshin.allworx@gmail.com \ --to=drivshin.allworx@gmail.com \ --cc=davem@davemloft.net \ --cc=dtrautmann@ibhsoftec-sps.de \ --cc=kernel@iktek.de \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-omap@vger.kernel.org \ --cc=mugunthanvnm@ti.com \ --cc=netdev@vger.kernel.org \ --cc=systemprogrammierung.brunner@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.