Hi Swapnil,
On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
>
>
>> -----Original Message-----
>> From: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>> Sent: Wednesday, September 2, 2020 5:47 PM
>> To: Swapnil Kashinath Jakhade <sjakh...@cadence.com>
>> Cc: vk...@kernel.org; kis...@ti.com; linux-kernel@vger.kernel.org;
>> max...@cerno.tech; Milind Parab <mpa...@cadence.com>; Yuti Suresh
>> Amonkar <yamon...@cadence.com>; nsek...@ti.com;
>> tomi.valkei...@ti.com; jsa...@ti.com; prane...@ti.com
>> Subject: Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set
>> PHY attributes
>>
>> EXTERNAL MAIL
>>
>>
>> Hi Swapnil,
>>
>> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
>> wrote:
>>> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
>>>> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
>>>>> Use generic PHY framework function phy_set_attrs() to set number
>>>>> of lanes and maximum link rate supported by PHY.
>>>>>
>>>>> Signed-off-by: Swapnil Jakhade <sjakh...@cadence.com>
>>>>> Acked-by: Kishon Vijay Abraham I <kis...@ti.com>
>>>>> ---
>>>>> drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
>>>>> 1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> index 7116127358ee..eca71467c4a8 100644
>>>>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>> struct cdns_torrent_phy *cdns_phy;
>>>>> struct device *dev = &pdev->dev;
>>>>> struct phy_provider *phy_provider;
>>>>> + struct phy_attrs torrent_attr;
>>>>> const struct of_device_id *match;
>>>>> struct cdns_torrent_data *data;
>>>>> struct device_node *child;
>>>>> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>> cdns_phy->phys[node].num_lanes,
>>>>> cdns_phy->max_bit_rate / 1000,
>>>>> cdns_phy->max_bit_rate % 1000);
>>>>> +
>>>>> + torrent_attr.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>>> + torrent_attr.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>>> + torrent_attr.mode = PHY_MODE_DP;
>>>>> +
>>>>> + phy_set_attrs(gphy, &torrent_attr);
>>>>
>>>> Why is this better than accessing the attributes manually as follows ?
>>>>
>>>> gphy->attrs.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>> gphy->attrs.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>> gphy->attrs.mode = PHY_MODE_DP;
>>>>
>>>> This is called in cdns_torrent_phy_probe(), before the PHY provider
>>>> is registered, so nothing can access the PHY yet. What race
>>>> condition are you trying to protect against with usage of phy_set_attrs() ?
>>>
>>> I agree that for Cadence DP bridge driver and Torrent PHY driver use
>>> case, it would not matter even if we set the attributes in Torrent PHY
>>> driver in a way you suggested above.
>>> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in
>>> future could maybe used by other drivers replacing existing individual
>>> functions for attributes bus_width and mode which are
>>> phy_set_bus_width/phy_get_bus_width and
>> phy_set_mode/phy_get_mode. So
>>> this usage in Torrent PHY driver is an example implementation of the API.
>>>
>>> [1]
>>> https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;!!EH
>>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
>> SYIEEibYdpBMJTll
>>> Yrd-00$
>>
>> This doesn't seem a very good API to me :-S It will require callers to always
>> call phy_get_attrs() first, modify the attributes they want to set, and then
>> call
>> phy_set_attrs(). Not only will be copy the whole phy_attrs structure
>> needlessly, it will also not be an atomic operation as someone else could
>> modify attributes between the get and set calls.
>> The lack of atomicity may not be an issue in practice if there's a single
>> user of
>> the PHY at all times, but in that case no mutex is needed.
What if the consumer tries to set an attribute at the middle of a
phy_power_on() operation? That is still a valid operation and phy core layer
should try to prevent it no?
>>
>> I think this series tries to fix a problem that doesn't exist.
>
> Thanks Laurent for your comments.
>
> Hi Kishon,
>
> Could you please suggest what would be the better approach regarding this PHY
> attributes series. Should we add individual get/set functions for new
> attribute
> max_link_rate just like mode and bus_width, or should we use phy_get_attrs()
> and phy_set_attrs() functions removing mutex. Your suggestions would really
> help.
I think Laurent's point is not having an API at all for configuring attributes
and access them manually?
Thanks
Kishon
>
> Thanks & regards,
> Swapnil
>
>>
>>>>> } else {
>>>>> dev_err(dev, "Driver supports only
>> PHY_TYPE_DP\n");
>>>>> ret = -ENOTSUPP;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
