From: Ziyang Huang <hzyitc@outlook.com> To: Andrew Lunn <andrew@lunn.ch> Cc: mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, richardcochran@gmail.com, p.zabel@pengutronix.de, matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver Date: Mon, 22 Jan 2024 23:37:29 +0800 [thread overview] Message-ID: <TYZPR01MB5556D035D9A13962844BB553C9752@TYZPR01MB5556.apcprd01.prod.exchangelabs.com> (raw) In-Reply-To: <2c6c0d72-5d4e-4ec4-beb6-d30852108a67@lunn.ch> 在 2024/1/22 0:19, Andrew Lunn 写道: > On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: >> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > > You need to say something in the commit message. One obvious thing is > to justify not using the at803x driver, since I want add more descriptions here. But I have no idea what to write. This is a mininal driver for a special phy. Here is the thing, at first, I was tring to add these into at803x driver, then I found that the IPQ5018 internel phy is a special device. The initialization sequence is initing GCC clock and reset control, then registering clocks providers, which is very different from other devices. What's more, I remembered it *wrongly* and thought it need to be accessed through MMIO. After checking the vendor code again, this doesn't exist. So it seem like that it's a good idea to move it back to at803x driver. > >> +#define IPQ5018_PHY_ID 0x004dd0c0 > > is in the Atheros OUI range. > >> +static int ipq5018_probe(struct phy_device *phydev) >> +{ >> + struct ipq5018_phy *priv; >> + struct device *dev = &phydev->mdio.dev; >> + char name[64]; >> + int ret; > > I guess you are new to mainline network. Please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > Section 1.6.4. Sorry for missing it, will read it later. > >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate priv\n"); > > Please read the documentation of dev_err_probe() and this fix the > obvious problem with this. I had read it and I had known this helper function is to resolve the duplicate code for EPROBE_DEFER. But it also say "Note that it is deemed acceptable to use this function for error prints during probe even if the @err is known to never be -EPROBE_DEFER. The benefit compared to a normal dev_err() is the standardized format of the error code and the fact that the error code is returned." And I can find the same code in other driver, so I thought it is a standard. Or should I just return -ENOMEM? Please let me known. > >> + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); >> + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_rx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), >> + "failed to register rx clock\n"); >> + >> + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); >> + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_tx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), >> + "failed to register tx clock\n"); >> + >> + priv->clk_data = devm_kzalloc(dev, >> + struct_size(priv->clk_data, hws, 2), >> + GFP_KERNEL); >> + if (!priv->clk_data) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate clk_data\n"); >> + >> + priv->clk_data->num = 2; >> + priv->clk_data->hws[0] = priv->clk_rx; >> + priv->clk_data->hws[1] = priv->clk_tx; >> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, >> + priv->clk_data); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "fail to register clock provider\n"); > > This needs an explanation. Why register two fixed clocks, which you > never use? Why not put these two clocks in DT? Without documentions, here is my guess: This is required by GCC controller. GCC driver require TX and RX clocks from GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered in GCC controller. Will add a figure describing the clock tree in UNIPHY driver. The frequency of these clocks is depends on the mode. For GEPHY, it only supports SGMII ( Or something similar, this is a internal bus ) and the clock is fixed at 1.25G. But for UNIPHY, is supports more mode like SGMII+ which used 3.125G. > >> + >> + return 0; >> +} >> + >> +static int ipq5018_soft_reset(struct phy_device *phydev) >> +{ >> + int ret; >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, 0); >> + if (ret < 0) >> + return ret; >> + >> + msleep(50); >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} > > This needs an explanation. It is also somewhat like > qca808x_link_change_notify(). Is it really sufficient to only do this > reset FIFO during a soft reset, or is it needed when ever the link > changes? I'm not sure here, this is what u-boot does. But I guess, we can reset GCC_GEPHY_* serial reset_controls. > > You also appear to be missing device tree bindings. Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in next patches. > > Andrew > > --- > pw-bot: cr >
WARNING: multiple messages have this Message-ID (diff)
From: Ziyang Huang <hzyitc@outlook.com> To: Andrew Lunn <andrew@lunn.ch> Cc: mcoquelin.stm32@gmail.com, alexandre.torgue@foss.st.com, richardcochran@gmail.com, p.zabel@pengutronix.de, matthias.bgg@gmail.com, angelogioacchino.delregno@collabora.com, linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org, linux-mediatek@lists.infradead.org Subject: Re: [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver Date: Mon, 22 Jan 2024 23:37:29 +0800 [thread overview] Message-ID: <TYZPR01MB5556D035D9A13962844BB553C9752@TYZPR01MB5556.apcprd01.prod.exchangelabs.com> (raw) In-Reply-To: <2c6c0d72-5d4e-4ec4-beb6-d30852108a67@lunn.ch> 在 2024/1/22 0:19, Andrew Lunn 写道: > On Sun, Jan 21, 2024 at 08:42:30PM +0800, Ziyang Huang wrote: >> Signed-off-by: Ziyang Huang <hzyitc@outlook.com> > > You need to say something in the commit message. One obvious thing is > to justify not using the at803x driver, since I want add more descriptions here. But I have no idea what to write. This is a mininal driver for a special phy. Here is the thing, at first, I was tring to add these into at803x driver, then I found that the IPQ5018 internel phy is a special device. The initialization sequence is initing GCC clock and reset control, then registering clocks providers, which is very different from other devices. What's more, I remembered it *wrongly* and thought it need to be accessed through MMIO. After checking the vendor code again, this doesn't exist. So it seem like that it's a good idea to move it back to at803x driver. > >> +#define IPQ5018_PHY_ID 0x004dd0c0 > > is in the Atheros OUI range. > >> +static int ipq5018_probe(struct phy_device *phydev) >> +{ >> + struct ipq5018_phy *priv; >> + struct device *dev = &phydev->mdio.dev; >> + char name[64]; >> + int ret; > > I guess you are new to mainline network. Please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html > > Section 1.6.4. Sorry for missing it, will read it later. > >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate priv\n"); > > Please read the documentation of dev_err_probe() and this fix the > obvious problem with this. I had read it and I had known this helper function is to resolve the duplicate code for EPROBE_DEFER. But it also say "Note that it is deemed acceptable to use this function for error prints during probe even if the @err is known to never be -EPROBE_DEFER. The benefit compared to a normal dev_err() is the standardized format of the error code and the fact that the error code is returned." And I can find the same code in other driver, so I thought it is a standard. Or should I just return -ENOMEM? Please let me known. > >> + snprintf(name, sizeof(name), "%s#rx", dev_name(dev)); >> + priv->clk_rx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_rx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_rx), >> + "failed to register rx clock\n"); >> + >> + snprintf(name, sizeof(name), "%s#tx", dev_name(dev)); >> + priv->clk_tx = clk_hw_register_fixed_rate(dev, name, NULL, 0, >> + TX_RX_CLK_RATE); >> + if (IS_ERR_OR_NULL(priv->clk_tx)) >> + return dev_err_probe(dev, PTR_ERR(priv->clk_tx), >> + "failed to register tx clock\n"); >> + >> + priv->clk_data = devm_kzalloc(dev, >> + struct_size(priv->clk_data, hws, 2), >> + GFP_KERNEL); >> + if (!priv->clk_data) >> + return dev_err_probe(dev, -ENOMEM, >> + "failed to allocate clk_data\n"); >> + >> + priv->clk_data->num = 2; >> + priv->clk_data->hws[0] = priv->clk_rx; >> + priv->clk_data->hws[1] = priv->clk_tx; >> + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get, >> + priv->clk_data); >> + if (ret) >> + return dev_err_probe(dev, ret, >> + "fail to register clock provider\n"); > > This needs an explanation. Why register two fixed clocks, which you > never use? Why not put these two clocks in DT? Without documentions, here is my guess: This is required by GCC controller. GCC driver require TX and RX clocks from GEPHY/UNIPHY. Then throught some sel or div cells, output clocks to GEPHY/UNIPHY and MAC. So I need to register them to make them can be refered in GCC controller. Will add a figure describing the clock tree in UNIPHY driver. The frequency of these clocks is depends on the mode. For GEPHY, it only supports SGMII ( Or something similar, this is a internal bus ) and the clock is fixed at 1.25G. But for UNIPHY, is supports more mode like SGMII+ which used 3.125G. > >> + >> + return 0; >> +} >> + >> +static int ipq5018_soft_reset(struct phy_device *phydev) >> +{ >> + int ret; >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, 0); >> + if (ret < 0) >> + return ret; >> + >> + msleep(50); >> + >> + ret = phy_modify(phydev, IPQ5018_PHY_FIFO_CONTROL, >> + IPQ5018_PHY_FIFO_RESET, IPQ5018_PHY_FIFO_RESET); >> + if (ret < 0) >> + return ret; >> + >> + return 0; >> +} > > This needs an explanation. It is also somewhat like > qca808x_link_change_notify(). Is it really sufficient to only do this > reset FIFO during a soft reset, or is it needed when ever the link > changes? I'm not sure here, this is what u-boot does. But I guess, we can reset GCC_GEPHY_* serial reset_controls. > > You also appear to be missing device tree bindings. Sorry for forgeting to add a WiP tag. Will add dt-bindings documentions in next patches. > > Andrew > > --- > pw-bot: cr > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2024-01-22 15:38 UTC|newest] Thread overview: 70+ messages / expand[flat|nested] mbox.gz Atom feed top 2024-01-21 12:40 [PATCH 0/8] ipq5018: enable ethernet support Ziyang Huang 2024-01-21 12:40 ` Ziyang Huang 2024-01-21 12:42 ` [PATCH 1/8] net: phy: Introduce Qualcomm IPQ5018 internal PHY driver Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-21 16:19 ` Andrew Lunn 2024-01-21 16:19 ` Andrew Lunn 2024-01-22 15:37 ` Ziyang Huang [this message] 2024-01-22 15:37 ` Ziyang Huang 2024-01-22 17:18 ` Andrew Lunn 2024-01-22 17:18 ` Andrew Lunn 2024-01-23 15:38 ` Ziyang Huang 2024-01-23 15:38 ` Ziyang Huang 2024-01-23 23:15 ` Andrew Lunn 2024-01-23 23:15 ` Andrew Lunn 2024-01-21 12:42 ` [PATCH 2/8] phy: Introduce Qualcomm ethernet uniphy driver Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-23 15:58 ` Ziyang Huang 2024-01-23 15:58 ` Ziyang Huang 2024-01-23 23:25 ` Andrew Lunn 2024-01-23 23:25 ` Andrew Lunn 2024-01-21 12:42 ` [PATCH 3/8] net: stmmac: Introduce Qualcomm IPQ50xx DWMAC driver Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-24 5:54 ` kernel test robot 2024-01-24 5:54 ` kernel test robot 2024-01-24 9:40 ` kernel test robot 2024-01-24 9:40 ` kernel test robot 2024-01-21 12:42 ` [PATCH 4/8] clk: qcom: gcc-ipq5018: correct gcc_gmac0_sys_clk reg Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-21 16:28 ` Andrew Lunn 2024-01-21 16:28 ` Andrew Lunn 2024-01-22 15:39 ` Ziyang Huang 2024-01-22 15:39 ` Ziyang Huang 2024-01-21 12:42 ` [PATCH 5/8] clk: qcom: support for duplicate freq in RCG2 freq table Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-21 16:57 ` Andrew Lunn 2024-01-21 16:57 ` Andrew Lunn 2024-01-22 16:35 ` Ziyang Huang 2024-01-22 16:35 ` Ziyang Huang 2024-01-22 17:34 ` Andrew Lunn 2024-01-22 17:34 ` Andrew Lunn 2024-01-23 15:43 ` Ziyang Huang 2024-01-23 15:43 ` Ziyang Huang 2024-01-22 7:55 ` Krzysztof Kozlowski 2024-01-22 7:55 ` Krzysztof Kozlowski 2024-01-22 14:48 ` Ziyang Huang 2024-01-22 14:48 ` Ziyang Huang 2024-01-21 12:42 ` [PATCH 6/8] net: mdio: ipq4019: support reset control Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-21 16:35 ` Andrew Lunn 2024-01-21 16:35 ` Andrew Lunn 2024-01-22 15:52 ` Ziyang Huang 2024-01-22 15:52 ` Ziyang Huang 2024-01-21 12:42 ` [PATCH 7/8] arm64: dts: qcom: ipq5018: enable ethernet support Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-21 16:45 ` Andrew Lunn 2024-01-21 16:45 ` Andrew Lunn 2024-01-22 15:52 ` Ziyang Huang 2024-01-22 15:52 ` Ziyang Huang 2024-01-22 17:27 ` Andrew Lunn 2024-01-22 17:27 ` Andrew Lunn 2024-01-21 12:42 ` [PATCH 8/8] arm64: dts: qcom: ipq5018-rdp432-c2: " Ziyang Huang 2024-01-21 12:42 ` Ziyang Huang 2024-01-22 7:54 ` Krzysztof Kozlowski 2024-01-22 7:54 ` Krzysztof Kozlowski 2024-01-24 0:53 ` kernel test robot 2024-01-24 0:53 ` kernel test robot 2024-01-21 15:51 ` [PATCH 0/8] ipq5018: " Andrew Lunn 2024-01-21 15:51 ` Andrew Lunn 2024-01-22 14:45 ` Ziyang Huang 2024-01-22 14:45 ` Ziyang Huang
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=TYZPR01MB5556D035D9A13962844BB553C9752@TYZPR01MB5556.apcprd01.prod.exchangelabs.com \ --to=hzyitc@outlook.com \ --cc=alexandre.torgue@foss.st.com \ --cc=andrew@lunn.ch \ --cc=angelogioacchino.delregno@collabora.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-stm32@st-md-mailman.stormreply.com \ --cc=matthias.bgg@gmail.com \ --cc=mcoquelin.stm32@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=p.zabel@pengutronix.de \ --cc=richardcochran@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.