From: Grygorii Strashko <grygorii.strashko@ti.com> To: Andrew Lunn <andrew@lunn.ch> Cc: "David S. Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>, Tony Lindgren <tony@atomide.com>, Rob Herring <robh+dt@kernel.org>, Kishon Vijay Abraham I <kishon@ti.com>, Sekhar Nori <nsekhar@ti.com>, <linux-kernel@vger.kernel.org>, <linux-omap@vger.kernel.org>, <devicetree@vger.kernel.org> Subject: Re: [RFC PATCH 03/11] phy: ti: introduce phy-gmii-sel driver Date: Tue, 9 Oct 2018 15:22:15 -0500 [thread overview] Message-ID: <cf3aba04-fad9-ad1d-1b51-1b6dbbd4ba5d@ti.com> (raw) In-Reply-To: <20181009003935.GA23588@lunn.ch> Hi Andrew, On 10/08/2018 07:39 PM, Andrew Lunn wrote: > On Mon, Oct 08, 2018 at 06:49:41PM -0500, Grygorii Strashko wrote: >> +static int phy_gmii_sel_mode(struct phy *phy, phy_interface_t intf_mode) >> +{ >> + struct phy_gmii_sel_phy_priv *if_phy = phy_get_drvdata(phy); >> + const struct phy_gmii_sel_soc_data *soc_data = if_phy->priv->soc_data; >> + struct device *dev = if_phy->priv->dev; >> + struct regmap_field *regfield; >> + int ret, rgmii_id = 0; >> + u32 mode = 0; >> + >> + if_phy->phy_if_mode = intf_mode; >> + >> + switch (if_phy->phy_if_mode) { >> + case PHY_INTERFACE_MODE_RMII: >> + mode = AM33XX_GMII_SEL_MODE_RMII; >> + break; >> + >> + case PHY_INTERFACE_MODE_RGMII: >> + mode = AM33XX_GMII_SEL_MODE_RGMII; >> + break; >> + >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + mode = AM33XX_GMII_SEL_MODE_RGMII; >> + rgmii_id = 1; >> + break; > > Hi Grygorii > > It looks like the MAC can do AM33XX_GMII_SEL_MODE_RGMII and > AM33XX_GMII_SEL_MODE_RGMII_ID. I don't think it can do > AM33XX_GMII_SEL_MODE_RGMII_RXID or AM33XX_GMII_SEL_MODE_RGMII_TXID? Sry, but would prefer not to thought this logic as part of this series as i moved it here unchanged rom cpsw-phy-sel.c (except adding possibility to update only supported field) and any changes here would require separate review (including all existing TI DT boards) and testing. I > would prefer it return -EINVAL when asked to do something it cannot > do. Just to clarify rgmii_id = 1 means *disable* CPSW Internal Delay Mode. > >> + >> + default: >> + dev_warn(dev, >> + "port%u: unsupported mode: \"%s\". Defaulting to MII.\n", >> + if_phy->id, phy_modes(rgmii_id)); >> + /* fall through */ > > Returning -EINVAL would be better. Otherwise the DT might never get > fixed. ok > >> + case PHY_INTERFACE_MODE_MII: >> + mode = AM33XX_GMII_SEL_MODE_MII; >> + break; >> + }; >> + >> + dev_dbg(dev, "%s id:%u mode:%u rgmii_id:%d rmii_clk_ext:%d\n", >> + __func__, if_phy->id, mode, rgmii_id, >> + if_phy->rmii_clock_external); >> + >> + regfield = if_phy->fields[PHY_GMII_SEL_PORT_MODE]; >> + ret = regmap_field_write(regfield, mode); >> + >> + if (soc_data->features & BIT(PHY_GMII_SEL_RGMII_ID_MODE) && >> + if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE]) { >> + regfield = if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE]; >> + ret |= regmap_field_write(regfield, rgmii_id); >> + } >> + >> + if (soc_data->features & BIT(PHY_GMII_SEL_RMII_IO_CLK_EN) && >> + if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN]) { >> + regfield = if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN]; >> + ret |= regmap_field_write(regfield, >> + if_phy->rmii_clock_external); >> + } >> + >> + if (ret) { >> + dev_err(dev, "port%u: set mode fail %d", if_phy->id, ret); >> + return -EIO; >> + } > > I would prefer each write had its own error check. The fact you don't > return ret means you know ret could be -EINVAL|-EOIO, making > -EMORECOFFEE. :) right, sry. -- regards, -grygorii
WARNING: multiple messages have this Message-ID (diff)
From: Grygorii Strashko <grygorii.strashko@ti.com> To: Andrew Lunn <andrew@lunn.ch> Cc: "David S. Miller" <davem@davemloft.net>, netdev@vger.kernel.org, Tony Lindgren <tony@atomide.com>, Rob Herring <robh+dt@kernel.org>, Kishon Vijay Abraham I <kishon@ti.com>, Sekhar Nori <nsekhar@ti.com>, linux-kernel@vger.kernel.org, linux-omap@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [RFC PATCH 03/11] phy: ti: introduce phy-gmii-sel driver Date: Tue, 9 Oct 2018 15:22:15 -0500 [thread overview] Message-ID: <cf3aba04-fad9-ad1d-1b51-1b6dbbd4ba5d@ti.com> (raw) In-Reply-To: <20181009003935.GA23588@lunn.ch> Hi Andrew, On 10/08/2018 07:39 PM, Andrew Lunn wrote: > On Mon, Oct 08, 2018 at 06:49:41PM -0500, Grygorii Strashko wrote: >> +static int phy_gmii_sel_mode(struct phy *phy, phy_interface_t intf_mode) >> +{ >> + struct phy_gmii_sel_phy_priv *if_phy = phy_get_drvdata(phy); >> + const struct phy_gmii_sel_soc_data *soc_data = if_phy->priv->soc_data; >> + struct device *dev = if_phy->priv->dev; >> + struct regmap_field *regfield; >> + int ret, rgmii_id = 0; >> + u32 mode = 0; >> + >> + if_phy->phy_if_mode = intf_mode; >> + >> + switch (if_phy->phy_if_mode) { >> + case PHY_INTERFACE_MODE_RMII: >> + mode = AM33XX_GMII_SEL_MODE_RMII; >> + break; >> + >> + case PHY_INTERFACE_MODE_RGMII: >> + mode = AM33XX_GMII_SEL_MODE_RGMII; >> + break; >> + >> + case PHY_INTERFACE_MODE_RGMII_ID: >> + case PHY_INTERFACE_MODE_RGMII_RXID: >> + case PHY_INTERFACE_MODE_RGMII_TXID: >> + mode = AM33XX_GMII_SEL_MODE_RGMII; >> + rgmii_id = 1; >> + break; > > Hi Grygorii > > It looks like the MAC can do AM33XX_GMII_SEL_MODE_RGMII and > AM33XX_GMII_SEL_MODE_RGMII_ID. I don't think it can do > AM33XX_GMII_SEL_MODE_RGMII_RXID or AM33XX_GMII_SEL_MODE_RGMII_TXID? Sry, but would prefer not to thought this logic as part of this series as i moved it here unchanged rom cpsw-phy-sel.c (except adding possibility to update only supported field) and any changes here would require separate review (including all existing TI DT boards) and testing. I > would prefer it return -EINVAL when asked to do something it cannot > do. Just to clarify rgmii_id = 1 means *disable* CPSW Internal Delay Mode. > >> + >> + default: >> + dev_warn(dev, >> + "port%u: unsupported mode: \"%s\". Defaulting to MII.\n", >> + if_phy->id, phy_modes(rgmii_id)); >> + /* fall through */ > > Returning -EINVAL would be better. Otherwise the DT might never get > fixed. ok > >> + case PHY_INTERFACE_MODE_MII: >> + mode = AM33XX_GMII_SEL_MODE_MII; >> + break; >> + }; >> + >> + dev_dbg(dev, "%s id:%u mode:%u rgmii_id:%d rmii_clk_ext:%d\n", >> + __func__, if_phy->id, mode, rgmii_id, >> + if_phy->rmii_clock_external); >> + >> + regfield = if_phy->fields[PHY_GMII_SEL_PORT_MODE]; >> + ret = regmap_field_write(regfield, mode); >> + >> + if (soc_data->features & BIT(PHY_GMII_SEL_RGMII_ID_MODE) && >> + if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE]) { >> + regfield = if_phy->fields[PHY_GMII_SEL_RGMII_ID_MODE]; >> + ret |= regmap_field_write(regfield, rgmii_id); >> + } >> + >> + if (soc_data->features & BIT(PHY_GMII_SEL_RMII_IO_CLK_EN) && >> + if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN]) { >> + regfield = if_phy->fields[PHY_GMII_SEL_RMII_IO_CLK_EN]; >> + ret |= regmap_field_write(regfield, >> + if_phy->rmii_clock_external); >> + } >> + >> + if (ret) { >> + dev_err(dev, "port%u: set mode fail %d", if_phy->id, ret); >> + return -EIO; >> + } > > I would prefer each write had its own error check. The fact you don't > return ret means you know ret could be -EINVAL|-EOIO, making > -EMORECOFFEE. :) right, sry. -- regards, -grygorii
next prev parent reply other threads:[~2018-10-09 20:22 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-10-08 23:49 [RFC PATCH 00/11] net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 01/11] phy: core add phy_set_netif_mode() api Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 5:22 ` Kishon Vijay Abraham I 2018-10-09 5:22 ` Kishon Vijay Abraham I 2018-10-09 22:43 ` Grygorii Strashko 2018-10-09 22:43 ` Grygorii Strashko 2018-10-25 10:05 ` Kishon Vijay Abraham I 2018-10-25 10:05 ` Kishon Vijay Abraham I 2018-10-08 23:49 ` [RFC PATCH 02/11] dt-bindings: phy: add cpsw port interface mode selection phy bindings Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 14:40 ` Tony Lindgren 2018-10-09 20:10 ` Grygorii Strashko 2018-10-09 20:10 ` Grygorii Strashko 2018-10-09 20:30 ` Tony Lindgren 2018-10-09 22:04 ` Grygorii Strashko 2018-10-09 22:04 ` Grygorii Strashko 2018-10-09 22:07 ` Tony Lindgren 2018-10-17 15:39 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 03/11] phy: ti: introduce phy-gmii-sel driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 0:39 ` Andrew Lunn 2018-10-09 20:22 ` Grygorii Strashko [this message] 2018-10-09 20:22 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 04/11] dt-bindings: net: ti: cpsw: switch to use phy-gmii-sel phy Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-17 15:41 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 05/11] net: ethernet: ti: cpsw: add support for port interface mode selection phy Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 0:50 ` Andrew Lunn 2018-10-09 20:28 ` Grygorii Strashko 2018-10-09 20:28 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 06/11] ARM: dts: dra7: switch to use phy-gmii-sel Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 07/11] ARM: dts: dm814x: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 08/11] ARM: dts: am4372: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 09/11] ARM: dts: am335x: " Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-08 23:49 ` [RFC PATCH 10/11] dt-bindings: net: ti: deprecate cpsw-phy-sel bindings Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-17 15:41 ` Rob Herring 2018-10-17 15:41 ` Rob Herring 2018-10-08 23:49 ` [RFC PATCH 11/11] net: ethernet: ti: cpsw: deprecate cpsw-phy-sel driver Grygorii Strashko 2018-10-08 23:49 ` Grygorii Strashko 2018-10-09 14:36 ` [RFC PATCH 00/11] net: ethernet: ti: cpsw: replace cpsw-phy-sel with phy driver Tony Lindgren 2018-10-09 21:12 ` Grygorii Strashko 2018-10-09 21:12 ` Grygorii Strashko
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=cf3aba04-fad9-ad1d-1b51-1b6dbbd4ba5d@ti.com \ --to=grygorii.strashko@ti.com \ --cc=andrew@lunn.ch \ --cc=davem@davemloft.net \ --cc=devicetree@vger.kernel.org \ --cc=kishon@ti.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=netdev@vger.kernel.org \ --cc=nsekhar@ti.com \ --cc=robh+dt@kernel.org \ --cc=tony@atomide.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.