Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
> Is it ok if we defer the solution for this drivers/patchset? Yes, not a problem if phy-mode means phy-mode. Andrew
Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
On Tue, 2019-08-06 at 17:39 +0200, Andrew Lunn wrote: > [External] > > On Tue, Aug 06, 2019 at 06:47:08AM +, Ardelean, Alexandru wrote: > > On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote: > > > [External] > > > > > > On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote: > > > > Sometimes, the connection between a MAC and PHY is done via a > > > > mode/interface converter. An example is a GMII-to-RGMII converter, which > > > > would mean that the MAC operates in GMII mode while the PHY operates in > > > > RGMII. In this case there is a discrepancy between what the MAC expects > > > > & > > > > what the PHY expects and both need to be configured in their respective > > > > modes. > > > > > > > > Sometimes, this converter is specified via a board/system configuration > > > > (in > > > > the device-tree for example). But, other times it can be left > > > > unspecified. > > > > The use of these converters is common in boards that have FPGA on them. > > > > > > > > This patch also adds support for a `adi,phy-mode-internal` property that > > > > can be used in these (implicit convert) cases. The internal PHY mode > > > > will > > > > be used to specify the correct register settings for the PHY. > > > > > > > > `fwnode_handle` is used, since this property may be specified via ACPI > > > > as > > > > well in other setups, but testing has been done in DT context. > > > > > > Looking at the patch, you seems to assume phy-mode is what the MAC is > > > using? That seems rather odd, given the name. It seems like a better > > > solution would be to add a mac-mode, which the MAC uses to configure > > > its side of the link. The MAC driver would then implement this > > > property. > > > > > > > actually, that's a pretty good idea; > > i guess i was narrow-minded when writing the driver, and got stuck on phy > > specifics, and forgot about the MAC-side; > > [ i also catch these design elements when reviewing, but i also seem to > > miss them when writing stuff sometimes ] > > > > Hi Ardelean > > We should also consider the media converter itself. It is passive, or > does it need a driver. You seems to be considering GMII-to-RGMII. But > what about RGMII to SGMII? or RGMII to 1000Base-KX etc? Ideally we > want a generic solution and we need to think about all the parts in > the system. In our case the GMII-to-RGMII converter is passive and does not need a driver. It's an HDL/FPGA block. There may be other converters that do need a driver. To be honest, the multitude of possible configurations [given that it's FPGA] can be... a lot. In one of our cases, specifying the MAC mode to be different than PHY mode [which assumes that there is an implicit passive media converter in-between] works. I admit that a generic solution would be nice. Is it ok if we defer the solution for this drivers/patchset? If you propose something, I can take a look as part of a different/new discussion. No guarrantees about how soon it would be implemented. Thanks Alex > > Andrew
Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
On Tue, Aug 06, 2019 at 06:47:08AM +, Ardelean, Alexandru wrote: > On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote: > > [External] > > > > On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote: > > > Sometimes, the connection between a MAC and PHY is done via a > > > mode/interface converter. An example is a GMII-to-RGMII converter, which > > > would mean that the MAC operates in GMII mode while the PHY operates in > > > RGMII. In this case there is a discrepancy between what the MAC expects & > > > what the PHY expects and both need to be configured in their respective > > > modes. > > > > > > Sometimes, this converter is specified via a board/system configuration > > > (in > > > the device-tree for example). But, other times it can be left unspecified. > > > The use of these converters is common in boards that have FPGA on them. > > > > > > This patch also adds support for a `adi,phy-mode-internal` property that > > > can be used in these (implicit convert) cases. The internal PHY mode will > > > be used to specify the correct register settings for the PHY. > > > > > > `fwnode_handle` is used, since this property may be specified via ACPI as > > > well in other setups, but testing has been done in DT context. > > > > Looking at the patch, you seems to assume phy-mode is what the MAC is > > using? That seems rather odd, given the name. It seems like a better > > solution would be to add a mac-mode, which the MAC uses to configure > > its side of the link. The MAC driver would then implement this > > property. > > > > actually, that's a pretty good idea; > i guess i was narrow-minded when writing the driver, and got stuck on phy > specifics, and forgot about the MAC-side; > [ i also catch these design elements when reviewing, but i also seem to miss > them when writing stuff sometimes ] > Hi Ardelean We should also consider the media converter itself. It is passive, or does it need a driver. You seems to be considering GMII-to-RGMII. But what about RGMII to SGMII? or RGMII to 1000Base-KX etc? Ideally we want a generic solution and we need to think about all the parts in the system. Andrew
Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
On Mon, 2019-08-05 at 16:51 +0200, Andrew Lunn wrote: > [External] > > On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote: > > Sometimes, the connection between a MAC and PHY is done via a > > mode/interface converter. An example is a GMII-to-RGMII converter, which > > would mean that the MAC operates in GMII mode while the PHY operates in > > RGMII. In this case there is a discrepancy between what the MAC expects & > > what the PHY expects and both need to be configured in their respective > > modes. > > > > Sometimes, this converter is specified via a board/system configuration (in > > the device-tree for example). But, other times it can be left unspecified. > > The use of these converters is common in boards that have FPGA on them. > > > > This patch also adds support for a `adi,phy-mode-internal` property that > > can be used in these (implicit convert) cases. The internal PHY mode will > > be used to specify the correct register settings for the PHY. > > > > `fwnode_handle` is used, since this property may be specified via ACPI as > > well in other setups, but testing has been done in DT context. > > Looking at the patch, you seems to assume phy-mode is what the MAC is > using? That seems rather odd, given the name. It seems like a better > solution would be to add a mac-mode, which the MAC uses to configure > its side of the link. The MAC driver would then implement this > property. > actually, that's a pretty good idea; i guess i was narrow-minded when writing the driver, and got stuck on phy specifics, and forgot about the MAC-side; [ i also catch these design elements when reviewing, but i also seem to miss them when writing stuff sometimes ] thanks > I don't see a need for this. phy-mode indicates what the PHY should > use. End of story. > > Andrew
Re: [PATCH 06/16] net: phy: adin: support PHY mode converters
On Mon, Aug 05, 2019 at 07:54:43PM +0300, Alexandru Ardelean wrote: > Sometimes, the connection between a MAC and PHY is done via a > mode/interface converter. An example is a GMII-to-RGMII converter, which > would mean that the MAC operates in GMII mode while the PHY operates in > RGMII. In this case there is a discrepancy between what the MAC expects & > what the PHY expects and both need to be configured in their respective > modes. > > Sometimes, this converter is specified via a board/system configuration (in > the device-tree for example). But, other times it can be left unspecified. > The use of these converters is common in boards that have FPGA on them. > > This patch also adds support for a `adi,phy-mode-internal` property that > can be used in these (implicit convert) cases. The internal PHY mode will > be used to specify the correct register settings for the PHY. > > `fwnode_handle` is used, since this property may be specified via ACPI as > well in other setups, but testing has been done in DT context. Looking at the patch, you seems to assume phy-mode is what the MAC is using? That seems rather odd, given the name. It seems like a better solution would be to add a mac-mode, which the MAC uses to configure its side of the link. The MAC driver would then implement this property. I don't see a need for this. phy-mode indicates what the PHY should use. End of story. Andrew
[PATCH 06/16] net: phy: adin: support PHY mode converters
Sometimes, the connection between a MAC and PHY is done via a
mode/interface converter. An example is a GMII-to-RGMII converter, which
would mean that the MAC operates in GMII mode while the PHY operates in
RGMII. In this case there is a discrepancy between what the MAC expects &
what the PHY expects and both need to be configured in their respective
modes.
Sometimes, this converter is specified via a board/system configuration (in
the device-tree for example). But, other times it can be left unspecified.
The use of these converters is common in boards that have FPGA on them.
This patch also adds support for a `adi,phy-mode-internal` property that
can be used in these (implicit convert) cases. The internal PHY mode will
be used to specify the correct register settings for the PHY.
`fwnode_handle` is used, since this property may be specified via ACPI as
well in other setups, but testing has been done in DT context.
Signed-off-by: Alexandru Ardelean
---
drivers/net/phy/adin.c | 39 ---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/drivers/net/phy/adin.c b/drivers/net/phy/adin.c
index dbdb8f60741c..e3d2ff8cc09c 100644
--- a/drivers/net/phy/adin.c
+++ b/drivers/net/phy/adin.c
@@ -10,6 +10,7 @@
#include
#include
#include
+#include
#define PHY_ID_ADIN12000x0283bc20
#define PHY_ID_ADIN13000x0283bc30
@@ -41,6 +42,31 @@
#define ADIN1300_GE_RMII_CFG_REG 0xff24
#define ADIN1300_GE_RMII_EN BIT(0)
+static int adin_get_phy_internal_mode(struct phy_device *phydev)
+{
+ struct device *dev = >mdio.dev;
+ const char *pm;
+ int i;
+
+ if (device_property_read_string(dev, "adi,phy-mode-internal", ))
+ return phydev->interface;
+
+ /**
+* Getting here assumes that there is converter in-between the actual
+* PHY, for example a GMII-to-RGMII converter. In this case the MAC
+* talks GMII and PHY talks RGMII, so the PHY needs to be set in RGMII
+* while the MAC can work in GMII mode.
+*/
+
+ for (i = 0; i < PHY_INTERFACE_MODE_MAX; i++)
+ if (!strcasecmp(pm, phy_modes(i)))
+ return i;
+
+ dev_err(dev, "Invalid value for 'phy-mode-internal': '%s'\n", pm);
+
+ return -EINVAL;
+}
+
static int adin_config_rgmii_mode(struct phy_device *phydev,
phy_interface_t intf)
{
@@ -105,7 +131,9 @@ static int adin_config_init(struct phy_device *phydev)
if (rc < 0)
return rc;
- interface = phydev->interface;
+ interface = adin_get_phy_internal_mode(phydev);
+ if (interface < 0)
+ return interface;
rc = adin_config_rgmii_mode(phydev, interface);
if (rc < 0)
@@ -115,8 +143,13 @@ static int adin_config_init(struct phy_device *phydev)
if (rc < 0)
return rc;
- dev_info(>mdio.dev, "PHY is using mode '%s'\n",
-phy_modes(phydev->interface));
+ if (phydev->interface == interface)
+ dev_info(>mdio.dev, "PHY is using mode '%s'\n",
+phy_modes(phydev->interface));
+ else
+ dev_info(>mdio.dev,
+"PHY is using mode '%s', MAC is using mode '%s'\n",
+phy_modes(interface), phy_modes(phydev->interface));
return 0;
}
--
2.20.1
