[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d7782a5e-2f67-4f62-a594-0f52144a368f@lunn.ch>
Date: Fri, 1 Nov 2024 14:21:24 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Lei Wei <quic_leiwei@...cinc.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>,
Russell King <linux@...linux.org.uk>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
quic_kkumarcs@...cinc.com, quic_suruchia@...cinc.com,
quic_pavir@...cinc.com, quic_linchen@...cinc.com,
quic_luoj@...cinc.com, srinivas.kandagatla@...aro.org,
bartosz.golaszewski@...aro.org, vsmuthu@....qualcomm.com,
john@...ozen.org
Subject: Re: [PATCH net-next 3/5] net: pcs: qcom-ipq: Add PCS create and
phylink operations for IPQ9574
> +static int ipq_pcs_config_mode(struct ipq_pcs *qpcs,
> + phy_interface_t interface)
> +{
> + unsigned int val;
> + int ret;
> +
> + /* Configure PCS interface mode */
> + switch (interface) {
> + case PHY_INTERFACE_MODE_SGMII:
> + /* Select Qualcomm SGMII AN mode */
> + ret = regmap_update_bits(qpcs->regmap, PCS_MODE_CTRL,
> + PCS_MODE_SEL_MASK | PCS_MODE_AN_MODE,
> + PCS_MODE_SGMII);
How does Qualcomm SGMII AN mode differ from Cisco SGMII AN mode?
> +static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
> + int index,
> + unsigned int neg_mode,
> + phy_interface_t interface)
> +{
> + int ret;
> +
> + /* Access to PCS registers such as PCS_MODE_CTRL which are
> + * common to all MIIs, is lock protected and configured
> + * only once. This is required only for interface modes
> + * such as QSGMII.
> + */
> + if (interface == PHY_INTERFACE_MODE_QSGMII)
> + mutex_lock(&qpcs->config_lock);
Is there a lot of contention on this lock? Why not take it for every
interface mode? It would make the code simpler.
> +struct phylink_pcs *ipq_pcs_create(struct device_node *np)
> +{
> + struct platform_device *pdev;
> + struct ipq_pcs_mii *qpcs_mii;
> + struct device_node *pcs_np;
> + struct ipq_pcs *qpcs;
> + int i, ret;
> + u32 index;
> +
> + if (!of_device_is_available(np))
> + return ERR_PTR(-ENODEV);
> +
> + if (of_property_read_u32(np, "reg", &index))
> + return ERR_PTR(-EINVAL);
> +
> + if (index >= PCS_MAX_MII_NRS)
> + return ERR_PTR(-EINVAL);
> +
> + pcs_np = of_get_parent(np);
> + if (!pcs_np)
> + return ERR_PTR(-ENODEV);
> +
> + if (!of_device_is_available(pcs_np)) {
> + of_node_put(pcs_np);
> + return ERR_PTR(-ENODEV);
> + }
How have you got this far if the parent is not available?
> + for (i = 0; i < PCS_MII_CLK_MAX; i++) {
> + qpcs_mii->clk[i] = of_clk_get_by_name(np, pcs_mii_clk_name[i]);
> + if (IS_ERR(qpcs_mii->clk[i])) {
> + dev_err(qpcs->dev,
> + "Failed to get MII %d interface clock %s\n",
> + index, pcs_mii_clk_name[i]);
> + goto err_clk_get;
> + }
> +
> + ret = clk_prepare_enable(qpcs_mii->clk[i]);
> + if (ret) {
> + dev_err(qpcs->dev,
> + "Failed to enable MII %d interface clock %s\n",
> + index, pcs_mii_clk_name[i]);
> + goto err_clk_en;
> + }
> + }
Maybe devm_clk_bulk_get() etc will help you here? I've never actually
used them, so i don't know for sure.
Andrew
Powered by blists - more mailing lists