diff mbox series

[2/6] net: bcmgenet: refactor phy mode configuration

Message ID 20200201074625.8698-3-jeremy.linton@arm.com
State Deferred
Delegated to: David Miller
Headers show
Series Add ACPI bindings to the genet | expand

Commit Message

Jeremy Linton Feb. 1, 2020, 7:46 a.m. UTC 
The DT phy mode is similar to what we want for ACPI
lets factor it out of the of path, and change the
of_ call to device_. Further if the phy-mode property
cannot be found instead of failing the driver load lets
just default it to RGMII.

Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
---
 drivers/net/ethernet/broadcom/genet/bcmmii.c | 39 +++++++++++---------
 1 file changed, 22 insertions(+), 17 deletions(-)

Comments

Florian Fainelli Feb. 1, 2020, 4:24 p.m. UTC | #1 
On 1/31/2020 11:46 PM, Jeremy Linton wrote:
> The DT phy mode is similar to what we want for ACPI
> lets factor it out of the of path, and change the
> of_ call to device_. Further if the phy-mode property
> cannot be found instead of failing the driver load lets
> just default it to RGMII.

Humm no please do not provide a fallback, if we cannot find a valid
'phy-mode' property we error out. This controller can be used with a
variety of configurations (internal EPHY/GPHY, MoCA, external
MII/Reverse MII/RGMII) and from a support perspective it is much easier
for us if the driver errors out if one of those essential properties are
omitted.

Other than that, this looks OK.
Jeremy Linton Feb. 1, 2020, 7:10 p.m. UTC | #2 
Hi,

Thanks for taking a look at this again!

On 2/1/20 10:24 AM, Florian Fainelli wrote:
> 
> 
> On 1/31/2020 11:46 PM, Jeremy Linton wrote:
>> The DT phy mode is similar to what we want for ACPI
>> lets factor it out of the of path, and change the
>> of_ call to device_. Further if the phy-mode property
>> cannot be found instead of failing the driver load lets
>> just default it to RGMII.
> 
> Humm no please do not provide a fallback, if we cannot find a valid
> 'phy-mode' property we error out. This controller can be used with a
> variety of configurations (internal EPHY/GPHY, MoCA, external
> MII/Reverse MII/RGMII) and from a support perspective it is much easier
> for us if the driver errors out if one of those essential properties are
> omitted.
> 
> Other than that, this looks OK.
> 

Sure, I went around in circles about this one because it cluttered the 
code path up a bit.
Andrew Lunn Feb. 3, 2020, 1:17 a.m. UTC | #3 
On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:
> 
> 
> On 1/31/2020 11:46 PM, Jeremy Linton wrote:
> > The DT phy mode is similar to what we want for ACPI
> > lets factor it out of the of path, and change the
> > of_ call to device_. Further if the phy-mode property
> > cannot be found instead of failing the driver load lets
> > just default it to RGMII.
> 
> Humm no please do not provide a fallback, if we cannot find a valid
> 'phy-mode' property we error out. This controller can be used with a
> variety of configurations (internal EPHY/GPHY, MoCA, external
> MII/Reverse MII/RGMII) and from a support perspective it is much easier
> for us if the driver errors out if one of those essential properties are
> omitted.

Hi Florian

Does any of the silicon variants have two or more MACs sharing one
MDIO bus? I'm thinking about the next patch in the series.

     Thanks
	Andrew
Florian Fainelli Feb. 3, 2020, 3:24 a.m. UTC | #4 
On 2/2/2020 5:17 PM, Andrew Lunn wrote:
> On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:
>>
>>
>> On 1/31/2020 11:46 PM, Jeremy Linton wrote:
>>> The DT phy mode is similar to what we want for ACPI
>>> lets factor it out of the of path, and change the
>>> of_ call to device_. Further if the phy-mode property
>>> cannot be found instead of failing the driver load lets
>>> just default it to RGMII.
>>
>> Humm no please do not provide a fallback, if we cannot find a valid
>> 'phy-mode' property we error out. This controller can be used with a
>> variety of configurations (internal EPHY/GPHY, MoCA, external
>> MII/Reverse MII/RGMII) and from a support perspective it is much easier
>> for us if the driver errors out if one of those essential properties are
>> omitted.
> 
> Hi Florian
> 
> Does any of the silicon variants have two or more MACs sharing one
> MDIO bus? I'm thinking about the next patch in the series.

Have not come across a customer doing that, but the hardware
definitively permits it, and so does the top-level chip pinmuxing.
Jeremy Linton Feb. 3, 2020, 6:46 p.m. UTC | #5 
Hi,

On 2/2/20 9:24 PM, Florian Fainelli wrote:
> 
> 
> On 2/2/2020 5:17 PM, Andrew Lunn wrote:
>> On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:
>>>
>>>
>>> On 1/31/2020 11:46 PM, Jeremy Linton wrote:
>>>> The DT phy mode is similar to what we want for ACPI
>>>> lets factor it out of the of path, and change the
>>>> of_ call to device_. Further if the phy-mode property
>>>> cannot be found instead of failing the driver load lets
>>>> just default it to RGMII.
>>>
>>> Humm no please do not provide a fallback, if we cannot find a valid
>>> 'phy-mode' property we error out. This controller can be used with a
>>> variety of configurations (internal EPHY/GPHY, MoCA, external
>>> MII/Reverse MII/RGMII) and from a support perspective it is much easier
>>> for us if the driver errors out if one of those essential properties are
>>> omitted.
>>
>> Hi Florian
>>
>> Does any of the silicon variants have two or more MACs sharing one
>> MDIO bus? I'm thinking about the next patch in the series.
> 
> Have not come across a customer doing that, but the hardware
> definitively permits it, and so does the top-level chip pinmuxing.
> 

Does the genet driver?

I might be missing something in the driver, but it looks like the whole 
thing is 1:1:1:1 with platform dev:mdio:phy:netdev at the moment. Given 
the way bcmgenet_mii_register is throwing a bcmgenet MII bus for every 
device _probe().
Florian Fainelli Feb. 3, 2020, 6:55 p.m. UTC | #6 
On 2/3/20 10:46 AM, Jeremy Linton wrote:
> Hi,
> 
> On 2/2/20 9:24 PM, Florian Fainelli wrote:
>>
>>
>> On 2/2/2020 5:17 PM, Andrew Lunn wrote:
>>> On Sat, Feb 01, 2020 at 08:24:14AM -0800, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 1/31/2020 11:46 PM, Jeremy Linton wrote:
>>>>> The DT phy mode is similar to what we want for ACPI
>>>>> lets factor it out of the of path, and change the
>>>>> of_ call to device_. Further if the phy-mode property
>>>>> cannot be found instead of failing the driver load lets
>>>>> just default it to RGMII.
>>>>
>>>> Humm no please do not provide a fallback, if we cannot find a valid
>>>> 'phy-mode' property we error out. This controller can be used with a
>>>> variety of configurations (internal EPHY/GPHY, MoCA, external
>>>> MII/Reverse MII/RGMII) and from a support perspective it is much easier
>>>> for us if the driver errors out if one of those essential properties
>>>> are
>>>> omitted.
>>>
>>> Hi Florian
>>>
>>> Does any of the silicon variants have two or more MACs sharing one
>>> MDIO bus? I'm thinking about the next patch in the series.
>>
>> Have not come across a customer doing that, but the hardware
>> definitively permits it, and so does the top-level chip pinmuxing.
>>
> 
> Does the genet driver?
> 
> I might be missing something in the driver, but it looks like the whole
> thing is 1:1:1:1 with platform dev:mdio:phy:netdev at the moment. Given
> the way bcmgenet_mii_register is throwing a bcmgenet MII bus for every
> device _probe().

Andrew's question was hypothetical, and I answered it the best way I
could. I did not say this was a *currently* supported combination from a
software perspective or that it would work today, but the hardware would
allow it. This question probably belongs to patch #3 which is about
using (or not phy_find_first).
kernel test robot Feb. 5, 2020, 9:05 p.m. UTC | #7 
Hi Jeremy,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.5]
[also build test WARNING on next-20200205]
[cannot apply to net/master net-next/master linus/master ipvs/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Jeremy-Linton/Add-ACPI-bindings-to-the-genet/20200203-101928
base:    d5226fa6dbae0569ee43ecfc08bdcd6770fc4755

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

smatch warnings:
drivers/net/ethernet/broadcom/genet/bcmmii.c:485 bcmgenet_phy_interface_init() warn: unsigned 'priv->phy_interface' is never less than zero.

vim +485 drivers/net/ethernet/broadcom/genet/bcmmii.c

   479	
   480	static int bcmgenet_phy_interface_init(struct bcmgenet_priv *priv)
   481	{
   482		struct device *kdev = &priv->pdev->dev;
   483	
   484		priv->phy_interface = device_get_phy_mode(kdev);
 > 485		if (priv->phy_interface < 0) {
   486			dev_dbg(kdev, "invalid PHY mode property\n");
   487			priv->phy_interface = PHY_INTERFACE_MODE_RGMII;
   488		}
   489	
   490		/* We need to specifically look up whether this PHY interface is internal
   491		 * or not *before* we even try to probe the PHY driver over MDIO as we
   492		 * may have shut down the internal PHY for power saving purposes.
   493		 */
   494		if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
   495			priv->internal_phy = true;
   496	
   497		return 0;
   498	}
   499	

---
0-DAY kernel test infrastructure                 Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
diff mbox series

Patch

diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index 6392a2530183..2049f8218589 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -477,12 +477,30 @@  static int bcmgenet_mii_register(struct bcmgenet_priv *priv)
 	return ret;
 }
 
+static int bcmgenet_phy_interface_init(struct bcmgenet_priv *priv)
+{
+	struct device *kdev = &priv->pdev->dev;
+
+	priv->phy_interface = device_get_phy_mode(kdev);
+	if (priv->phy_interface < 0) {
+		dev_dbg(kdev, "invalid PHY mode property\n");
+		priv->phy_interface = PHY_INTERFACE_MODE_RGMII;
+	}
+
+	/* We need to specifically look up whether this PHY interface is internal
+	 * or not *before* we even try to probe the PHY driver over MDIO as we
+	 * may have shut down the internal PHY for power saving purposes.
+	 */
+	if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
+		priv->internal_phy = true;
+
+	return 0;
+}
+
 static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 {
 	struct device_node *dn = priv->pdev->dev.of_node;
-	struct device *kdev = &priv->pdev->dev;
 	struct phy_device *phydev;
-	phy_interface_t phy_mode;
 	int ret;
 
 	/* Fetch the PHY phandle */
@@ -500,23 +518,10 @@  static int bcmgenet_mii_of_init(struct bcmgenet_priv *priv)
 	}
 
 	/* Get the link mode */
-	ret = of_get_phy_mode(dn, &phy_mode);
-	if (ret) {
-		dev_err(kdev, "invalid PHY mode property\n");
-		return ret;
-	}
-
-	priv->phy_interface = phy_mode;
-
-	/* We need to specifically look up whether this PHY interface is internal
-	 * or not *before* we even try to probe the PHY driver over MDIO as we
-	 * may have shut down the internal PHY for power saving purposes.
-	 */
-	if (priv->phy_interface == PHY_INTERFACE_MODE_INTERNAL)
-		priv->internal_phy = true;
+	bcmgenet_phy_interface_init(priv);
 
 	/* Make sure we initialize MoCA PHYs with a link down */
-	if (phy_mode == PHY_INTERFACE_MODE_MOCA) {
+	if (priv->phy_interface == PHY_INTERFACE_MODE_MOCA) {
 		phydev = of_phy_find_device(dn);
 		if (phydev) {
 			phydev->link = 0;