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@xxxxxx>
>> ---
>> 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.
