Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8ef8b2f8-a1c1-b277-22b1-6e951e8dfc72@kernel.org>
Date:   Mon, 29 Aug 2022 15:43:56 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
Subject: Re: [PATCH v3 2/2] phy: ti: gmii-sel: Add support for CPSW5G GMII SEL
 in J7200
Content-Language: en-US
To:     Siddharth Vadapalli <s-vadapalli@ti.com>
Cc:     robh+dt@kernel.org, lee.jones@linaro.org,
        krzysztof.kozlowski@linaro.org, krzysztof.kozlowski+dt@linaro.org,
        kishon@ti.com, vkoul@kernel.org, dan.carpenter@oracle.com,
        grygorii.strashko@ti.com, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-phy@lists.infradead.org
References: <20220822065631.27933-1-s-vadapalli@ti.com>
 <20220822065631.27933-3-s-vadapalli@ti.com>
 <655c39a3-cf5f-6dd7-3c1a-61ebeae915bd@kernel.org>
 <7dd27795-11d0-3147-ed86-959ff3e89df4@ti.com>
From:   Roger Quadros <rogerq@kernel.org>
In-Reply-To: <7dd27795-11d0-3147-ed86-959ff3e89df4@ti.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Siddharth,

On 29/08/2022 07:53, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 25/08/22 13:11, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 22/08/2022 09:56, Siddharth Vadapalli wrote:
>>> Each of the CPSW5G ports in J7200 support additional modes like QSGMII.
>>> Add a new compatible for J7200 to support the additional modes.
>>>
>>> In TI's J7200, each of the CPSW5G ethernet interfaces can act as a
>>> QSGMII or QSGMII-SUB port. The QSGMII interface is responsible for
>>> performing auto-negotiation between the MAC and the PHY while the rest of
>>> the interfaces are designated as QSGMII-SUB interfaces, indicating that
>>> they will not be taking part in the auto-negotiation process.
>>>
>>> To indicate the interface which will serve as the main QSGMII interface,
>>> add a property "ti,qsgmii-main-ports", whose value indicates the
>>> port number of the interface which shall serve as the main QSGMII
>>> interface. The rest of the interfaces are then assigned QSGMII-SUB mode by
>>> default.
>>
>> Can you please describe here why you are using "ti,qsgmii-main-ports" instead
>> of "ti,qsgmii-main-port" as there can be only one main port per PHY instance?
> 
> Thank you for reviewing the patch. I am using "ports" instead of "port"
> because I plan to add support for CPSW9G on TI's J721e device in the
> future patches. CPSW9G (8 external ports) supports up to two QSGMII main
> ports. For CPSW9G, by specifying the two main ports in the device tree,
> it is possible to configure the CTRLMMR_ENETx_CTRL register for each of
> the 8 ports, with the two QSGMII main ports being configured as main
> ports in the CTRLMMR_ENETx_CTRL register and the rest of them being
> configured as sub ports. Since I will be using the same property
> "ti,qsgmii-main-ports" for CPSW9G as well, the property will be an array
> of 2 values for CPSW9G. Therefore, I am using "ports" instead of "port".
> Please let me know if this is fine.
> 

OK. Please mention this in commit message.

>>
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  drivers/phy/ti/phy-gmii-sel.c | 40 ++++++++++++++++++++++++++++++++---
>>>  1 file changed, 37 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>>> index d0ab69750c6b..270083606b14 100644
>>> --- a/drivers/phy/ti/phy-gmii-sel.c
>>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>>> @@ -22,6 +22,12 @@
>>>  #define AM33XX_GMII_SEL_MODE_RMII	1
>>>  #define AM33XX_GMII_SEL_MODE_RGMII	2
>>>  
>>> +/* J72xx SoC specific definitions for the CONTROL port */
>>> +#define J72XX_GMII_SEL_MODE_QSGMII	4
>>> +#define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
>>> +
>>> +#define PHY_GMII_PORT(n)	BIT((n) - 1)
>>> +
>>>  enum {
>>>  	PHY_GMII_SEL_PORT_MODE = 0,
>>>  	PHY_GMII_SEL_RGMII_ID_MODE,
>>> @@ -43,6 +49,7 @@ struct phy_gmii_sel_soc_data {
>>>  	u32 features;
>>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>>  	bool use_of_data;
>>> +	u64 extra_modes;
>>>  };
>>>  
>>>  struct phy_gmii_sel_priv {
>>> @@ -53,6 +60,7 @@ struct phy_gmii_sel_priv {
>>>  	struct phy_gmii_sel_phy_priv *if_phys;
>>>  	u32 num_ports;
>>>  	u32 reg_offset;
>>> +	u32 qsgmii_main_ports;
>>>  };
>>>  
>>>  static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>> @@ -88,10 +96,17 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>  		gmii_sel_mode = AM33XX_GMII_SEL_MODE_MII;
>>>  		break;
>>>  
>>> +	case PHY_INTERFACE_MODE_QSGMII:
>>> +		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_QSGMII)))
>>> +			goto unsupported;
>>> +		if (if_phy->priv->qsgmii_main_ports & BIT(if_phy->id - 1))
>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII;
>>> +		else
>>> +			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
>>> +		break;
>>> +
>>>  	default:
>>> -		dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>> -			 if_phy->id, phy_modes(submode));
>>> -		return -EINVAL;
>>> +		goto unsupported;
>>>  	}
>>>  
>>>  	if_phy->phy_if_mode = submode;
>>> @@ -123,6 +138,11 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
>>>  	}
>>>  
>>>  	return 0;
>>> +
>>> +unsupported:
>>> +	dev_warn(dev, "port%u: unsupported mode: \"%s\"\n",
>>> +		 if_phy->id, phy_modes(submode));
>>> +	return -EINVAL;
>>>  }
>>>  
>>>  static const
>>> @@ -188,6 +208,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_soc_am654 = {
>>>  	.regfields = phy_gmii_sel_fields_am654,
>>>  };
>>>  
>>> +static const
>>> +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
>>> +	.use_of_data = true,
>>> +	.regfields = phy_gmii_sel_fields_am654,
>>> +	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
>>> +};
>>> +
>>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>  	{
>>>  		.compatible	= "ti,am3352-phy-gmii-sel",
>>> @@ -209,6 +236,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = {
>>>  		.compatible	= "ti,am654-phy-gmii-sel",
>>>  		.data		= &phy_gmii_sel_soc_am654,
>>>  	},
>>> +	{
>>> +		.compatible	= "ti,j7200-cpsw5g-phy-gmii-sel",
>>> +		.data		= &phy_gmii_sel_cpsw5g_soc_j7200,
>>> +	},
>>>  	{}
>>>  };
>>>  MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table);
>>> @@ -350,6 +381,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  	struct device_node *node = dev->of_node;
>>>  	const struct of_device_id *of_id;
>>>  	struct phy_gmii_sel_priv *priv;
>>> +	u32 main_ports = 1;
>>>  	int ret;
>>>  
>>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>> @@ -363,6 +395,8 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  	priv->dev = &pdev->dev;
>>>  	priv->soc_data = of_id->data;
>>>  	priv->num_ports = priv->soc_data->num_ports;
>>> +	of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports, 1);
>>
>> of_property_read_u32_array can return error and you are not doing any sanity checks.
>> This should fail if "ti,qsgmii-main-ports" is not present or out of bounds if port mode is QSGMII.
> 
> In the scenario that the user doesn't want to use QSGMII mode, the
> property will not be mentioned in the device tree. In the phy-gmii-sel
> driver, the call to of_property_read_u32_array() will return an error
> since the optional ti,qsgmii-main-ports property doesn't exist in the
> device tree. However, the main_ports variable has already been
> initialized to 1 and in case of Non-QSGMII modes, the main_ports
> variable will not even be used. If the mode is QSGMII and the user
> forgets to mention the ti,qsgmii-main-ports property in the device tree,
> then the default value of 1 is used.
> 
> Since the of_property_read_u32_array() function doesn't overwrite
> main_ports variable if the ti,qsgmii-main-ports property is not present
> in the device tree, the value of main_ports will continue to remain 1
> even in the case where of_property_read_u32_array() errors out.
> 
> In the other scenario where the user mentions a value that is smaller or
> greater than the allowed value for "ti,qsgmii-main-ports" property, I
> have added bindings to ensure that make dtbs_check fails. This will
> enforce the bounds on the property.

This I'm not sure about. dtbs_check is not always run at build time and we cannot
depend on that.

> 
> For the above reasons, I think that the return value of the call to
> of_property_read_u32_array() can be safely ignored, and the value of
> main_ports doesn't need to be validated within the driver as it is being
> enforced in the bindings.
> 
>>
>> How is this going to scale if you are going to have multiple main ports?
>> Let's say you increase it to 2 in the future. won't this break with -EOVERFLOW for older
>> DTs where you had only 1 u32.
> 
> For multiple main ports (like CPSW9G for example), I had planned to add
> an IF-ELSE condition to check the compatible (I plan to add a new
> compatible for J721e which uses CPSW9G) and then call
> of_property_read_u32_array() with either 1 or 2 values to be read based
> on the compatible.

In that case please use of_property_read_u32 for the case where you know only there is only 1 value.

> 
> Please let me know if you have a suggestion for a better way to
> implement this.
> 
> Regards,
> Siddharth.

cheers,
-roger