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