diff mbox series

[v3,2/3] phy: ti: gmii-sel: Update methods for fetching and using qsgmii main port

Message ID 20221026074532.109220-3-s-vadapalli@ti.com
State New
Headers
Series Add support to PHY GMII SEL for J721e CPSW9G QSGMII |

Commit Message

Siddharth Vadapalli Oct. 26, 2022, 7:45 a.m. UTC 
  The number of QSGMII main ports are specific to the device. TI's J7200 for
which the QSGMII main port property is fetched from the device-tree has
only one QSGMII main port. However, devices like TI's J721e support up to
two QSGMII main ports. Thus, the existing methods for fetching and using
the QSGMII main port are not scalable.

Update the existing methods for handling the QSGMII main ports and its
associated requirements to make it scalable for future devices.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

Comments

Roger Quadros Oct. 28, 2022, 10:23 a.m. UTC | #1 
Hi Siddharth,

On 26/10/2022 10:45, Siddharth Vadapalli wrote:
> The number of QSGMII main ports are specific to the device. TI's J7200 for
> which the QSGMII main port property is fetched from the device-tree has
> only one QSGMII main port. However, devices like TI's J721e support up to
> two QSGMII main ports. Thus, the existing methods for fetching and using
> the QSGMII main port are not scalable.
> 
> Update the existing methods for handling the QSGMII main ports and its
> associated requirements to make it scalable for future devices.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>  1 file changed, 22 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
> index 0bcfd6d96b4d..c8f30d2e1f46 100644
> --- a/drivers/phy/ti/phy-gmii-sel.c
> +++ b/drivers/phy/ti/phy-gmii-sel.c
> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>  	bool use_of_data;
>  	u64 extra_modes;
> +	u32 num_qsgmii_main_ports;
>  };
>  
>  struct phy_gmii_sel_priv {
> @@ -213,6 +214,8 @@ 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),
> +	.num_ports = 4,
> +	.num_qsgmii_main_ports = 1,
>  };
>  
>  static const struct of_device_id phy_gmii_sel_id_table[] = {
> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> +	const struct phy_gmii_sel_soc_data *soc_data;
>  	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;
> +	u32 i;
>  
>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>  	if (!of_id)
> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>  
>  	priv->dev = &pdev->dev;
>  	priv->soc_data = of_id->data;
> +	soc_data = priv->soc_data;
>  	priv->num_ports = priv->soc_data->num_ports;
> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
> +	priv->qsgmii_main_ports = 0;
> +
>  	/*
> -	 * Ensure that main_ports is within bounds. If the property
> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
> -	 * is out of bounds, default to 1.
> +	 * Based on the compatible, try to read the appropriate number of
> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
> +	 * the device-tree node.
>  	 */
> -	if (main_ports < 1 || main_ports > 4)
> -		main_ports = 1;
> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
> +		/*
> +		 * Ensure that main_ports is within bounds.
> +		 */
> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
> +			dev_err(dev, "Invalid qsgmii main port provided\n");

nit: This message is a bit misleading if the property does not exist in DT.

How about just "Invalid ti,qsgmii-main-ports"

> +			return -EINVAL;
> +		}
> +		priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports);
> +	}
>  
>  	priv->regmap = syscon_node_to_regmap(node->parent);
>  	if (IS_ERR(priv->regmap)) {

Reviewed-by: Roger Quadros <rogerq@kernel.org>

cheers,
-roger
Siddharth Vadapalli Oct. 28, 2022, 10:32 a.m. UTC | #2 
Hello Roger,

On 28/10/22 15:53, Roger Quadros wrote:
> Hi Siddharth,
> 
> On 26/10/2022 10:45, Siddharth Vadapalli wrote:
>> The number of QSGMII main ports are specific to the device. TI's J7200 for
>> which the QSGMII main port property is fetched from the device-tree has
>> only one QSGMII main port. However, devices like TI's J721e support up to
>> two QSGMII main ports. Thus, the existing methods for fetching and using
>> the QSGMII main port are not scalable.
>>
>> Update the existing methods for handling the QSGMII main ports and its
>> associated requirements to make it scalable for future devices.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>> index 0bcfd6d96b4d..c8f30d2e1f46 100644
>> --- a/drivers/phy/ti/phy-gmii-sel.c
>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>  	bool use_of_data;
>>  	u64 extra_modes;
>> +	u32 num_qsgmii_main_ports;
>>  };
>>  
>>  struct phy_gmii_sel_priv {
>> @@ -213,6 +214,8 @@ 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),
>> +	.num_ports = 4,
>> +	.num_qsgmii_main_ports = 1,
>>  };
>>  
>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  {
>>  	struct device *dev = &pdev->dev;
>> +	const struct phy_gmii_sel_soc_data *soc_data;
>>  	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;
>> +	u32 i;
>>  
>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>  	if (!of_id)
>> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>  
>>  	priv->dev = &pdev->dev;
>>  	priv->soc_data = of_id->data;
>> +	soc_data = priv->soc_data;
>>  	priv->num_ports = priv->soc_data->num_ports;
>> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
>> +	priv->qsgmii_main_ports = 0;
>> +
>>  	/*
>> -	 * Ensure that main_ports is within bounds. If the property
>> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
>> -	 * is out of bounds, default to 1.
>> +	 * Based on the compatible, try to read the appropriate number of
>> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
>> +	 * the device-tree node.
>>  	 */
>> -	if (main_ports < 1 || main_ports > 4)
>> -		main_ports = 1;
>> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
>> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
>> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
>> +		/*
>> +		 * Ensure that main_ports is within bounds.
>> +		 */
>> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
>> +			dev_err(dev, "Invalid qsgmii main port provided\n");
> 
> nit: This message is a bit misleading if the property does not exist in DT.
> 
> How about just "Invalid ti,qsgmii-main-ports"

Thank you for reviewing the patch. The variable "main_ports" has been
initialized to 1 at the top. Thus, the only way the error condition is
entered is if "ti,qsgmii-main-ports" is mentioned in the device-tree
with an invalid value. If "ti,qsgmii-main-ports" is not mentioned in the
device-tree, then "main_ports" continues being 1, since the function
"of_property_read_u32_index()" does not modify "main_ports" if
"ti,qsgmii-main-ports" is not present in the device-tree. Thus, in this
case, the error condition isn't reached.

Regards,
Siddharth.
Roger Quadros Oct. 28, 2022, 10:42 a.m. UTC | #3 
On 28/10/2022 13:32, Siddharth Vadapalli wrote:
> Hello Roger,
> 
> On 28/10/22 15:53, Roger Quadros wrote:
>> Hi Siddharth,
>>
>> On 26/10/2022 10:45, Siddharth Vadapalli wrote:
>>> The number of QSGMII main ports are specific to the device. TI's J7200 for
>>> which the QSGMII main port property is fetched from the device-tree has
>>> only one QSGMII main port. However, devices like TI's J721e support up to
>>> two QSGMII main ports. Thus, the existing methods for fetching and using
>>> the QSGMII main port are not scalable.
>>>
>>> Update the existing methods for handling the QSGMII main ports and its
>>> associated requirements to make it scalable for future devices.
>>>
>>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>>> ---
>>>  drivers/phy/ti/phy-gmii-sel.c | 29 ++++++++++++++++++++++-------
>>>  1 file changed, 22 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
>>> index 0bcfd6d96b4d..c8f30d2e1f46 100644
>>> --- a/drivers/phy/ti/phy-gmii-sel.c
>>> +++ b/drivers/phy/ti/phy-gmii-sel.c
>>> @@ -50,6 +50,7 @@ struct phy_gmii_sel_soc_data {
>>>  	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
>>>  	bool use_of_data;
>>>  	u64 extra_modes;
>>> +	u32 num_qsgmii_main_ports;
>>>  };
>>>  
>>>  struct phy_gmii_sel_priv {
>>> @@ -213,6 +214,8 @@ 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),
>>> +	.num_ports = 4,
>>> +	.num_qsgmii_main_ports = 1,
>>>  };
>>>  
>>>  static const struct of_device_id phy_gmii_sel_id_table[] = {
>>> @@ -378,11 +381,13 @@ static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
>>>  static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device *dev = &pdev->dev;
>>> +	const struct phy_gmii_sel_soc_data *soc_data;
>>>  	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;
>>> +	u32 i;
>>>  
>>>  	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
>>>  	if (!of_id)
>>> @@ -394,16 +399,26 @@ static int phy_gmii_sel_probe(struct platform_device *pdev)
>>>  
>>>  	priv->dev = &pdev->dev;
>>>  	priv->soc_data = of_id->data;
>>> +	soc_data = priv->soc_data;
>>>  	priv->num_ports = priv->soc_data->num_ports;
>>> -	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
>>> +	priv->qsgmii_main_ports = 0;
>>> +
>>>  	/*
>>> -	 * Ensure that main_ports is within bounds. If the property
>>> -	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
>>> -	 * is out of bounds, default to 1.
>>> +	 * Based on the compatible, try to read the appropriate number of
>>> +	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
>>> +	 * the device-tree node.
>>>  	 */
>>> -	if (main_ports < 1 || main_ports > 4)
>>> -		main_ports = 1;
>>> -	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
>>> +	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
>>> +		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
>>> +		/*
>>> +		 * Ensure that main_ports is within bounds.
>>> +		 */
>>> +		if (main_ports < 1 || main_ports > soc_data->num_ports) {
>>> +			dev_err(dev, "Invalid qsgmii main port provided\n");
>>
>> nit: This message is a bit misleading if the property does not exist in DT.
>>
>> How about just "Invalid ti,qsgmii-main-ports"
> 
> Thank you for reviewing the patch. The variable "main_ports" has been
> initialized to 1 at the top. Thus, the only way the error condition is
> entered is if "ti,qsgmii-main-ports" is mentioned in the device-tree
> with an invalid value. If "ti,qsgmii-main-ports" is not mentioned in the
> device-tree, then "main_ports" continues being 1, since the function
> "of_property_read_u32_index()" does not modify "main_ports" if
> "ti,qsgmii-main-ports" is not present in the device-tree. Thus, in this
> case, the error condition isn't reached.

You are right. No need to change the message in that case.

cheers,
-roger
diff mbox series

Patch

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index 0bcfd6d96b4d..c8f30d2e1f46 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -50,6 +50,7 @@  struct phy_gmii_sel_soc_data {
 	const struct reg_field (*regfields)[PHY_GMII_SEL_LAST];
 	bool use_of_data;
 	u64 extra_modes;
+	u32 num_qsgmii_main_ports;
 };
 
 struct phy_gmii_sel_priv {
@@ -213,6 +214,8 @@  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),
+	.num_ports = 4,
+	.num_qsgmii_main_ports = 1,
 };
 
 static const struct of_device_id phy_gmii_sel_id_table[] = {
@@ -378,11 +381,13 @@  static int phy_gmii_sel_init_ports(struct phy_gmii_sel_priv *priv)
 static int phy_gmii_sel_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	const struct phy_gmii_sel_soc_data *soc_data;
 	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;
+	u32 i;
 
 	of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node);
 	if (!of_id)
@@ -394,16 +399,26 @@  static int phy_gmii_sel_probe(struct platform_device *pdev)
 
 	priv->dev = &pdev->dev;
 	priv->soc_data = of_id->data;
+	soc_data = priv->soc_data;
 	priv->num_ports = priv->soc_data->num_ports;
-	of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports);
+	priv->qsgmii_main_ports = 0;
+
 	/*
-	 * Ensure that main_ports is within bounds. If the property
-	 * ti,qsgmii-main-ports is not mentioned, or the value mentioned
-	 * is out of bounds, default to 1.
+	 * Based on the compatible, try to read the appropriate number of
+	 * QSGMII main ports from the "ti,qsgmii-main-ports" property from
+	 * the device-tree node.
 	 */
-	if (main_ports < 1 || main_ports > 4)
-		main_ports = 1;
-	priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports);
+	for (i = 0; i < soc_data->num_qsgmii_main_ports; i++) {
+		of_property_read_u32_index(node, "ti,qsgmii-main-ports", i, &main_ports);
+		/*
+		 * Ensure that main_ports is within bounds.
+		 */
+		if (main_ports < 1 || main_ports > soc_data->num_ports) {
+			dev_err(dev, "Invalid qsgmii main port provided\n");
+			return -EINVAL;
+		}
+		priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports);
+	}
 
 	priv->regmap = syscon_node_to_regmap(node->parent);
 	if (IS_ERR(priv->regmap)) {