Re: [PATCH v2] phy: cadence-torrent: add support for three or more links using 2 protocols

[Roger Quadros <rogerq@kernel.org>]

Hi Siddhath,

On 12/07/2024 13:38, Siddharth Vadapalli wrote:
> Roger, Swapnil,
> 
> On Thu, Jul 11, 2024 at 11:04:57AM +0300, Roger Quadros wrote:
>> Siddharth,
>>
>> On 11/07/2024 10:53, Siddharth Vadapalli wrote:
> 
> [...]
> 
>>> I suppose that PCIe is the only such protocol since it can have
>>> different speeds despite a single protocol (Gen1, Gen2, Gen3...), unlike
>>> other protocols which have a fixed speed and therefore the PLL
>>> associated with them doesn't have to be reconfigured as the rate will
>>> never change. Please let me know if there are other protocols (maybe DP?)
>>> which also require such special handling.
>>
>> The constraint is PLL frequency and not protocols as such right?
>> e.g. If there are 4 protocols that all use same PLL frequency then we should
>> be able to support it?
>>
>> How about updating the patch to limit on number of PLL frequencies rather than
>> number of protocols? This should deal with PCIe multi-link case as well.
> 
> I suppose that an indirect way of determining whether a configuration
> can be supported or not is by checking if an entry exists in the "tables"
> (link_cmn_vals_tbl). That should be accurate since it reflects what the
> driver supports.
> 
> I will update this patch accordingly so that Swapnil's inputs regarding
> PCIe Multi-link are also addressed.
> 
> I am describing the logic for the updated patch below. Please share your
> feedback.
> 
> 1. All single-link configurations (1 sub-node) can have only one
> protocol and will be handled via the "phy_ops" callbacks namely:
> .init, .power_on, ...
> No change will be made to this existing implementation.
> 
> 2. All multi-link configurations (2 or more sub-nodes) have to be
> configured via cdns_torrent_phy_configure_multilink().
> 
> CASE-1 (2 Links/Sub-nodes):
> Check if there is an entry in "link_cmn_vals_entries" for the requested
> configuration and configure accordingly. This should handle the PCIe
> Multi-link configuration as well as other similar configurations which
> have a single protocol but cannot be treated as two single link
> configurations performed successively for each link.
> 
> CASE-2 (3 or more Links/Sub-nodes):
> 
> The links shall be grouped together by the protocol. Since we eventually
> have to look for an entry in "link_cmn_vals_entries", it is safe to impose
> the constraint that there shouldn't be more than 2 Protocols as the table
> is of the form:
> (phy_type1, phy_type2)
> i.e. Protocol 1, Protocol 2.
> It is guaranteed to be the case that Protocol1 != Protocol2 due to the
> following reason:
> If Protocol 1 == Protocol 2, it could have been represented in the
> device-tree using either:
> a) single link (sub-node)
> b) double link (sub-node) -> Special cases like PCIe Multi-link
> 
> So assuming the above, we can enforce the constraint that there should
> be only 2 Protocols when 3 or more Links are present in the device-tree.
> This also handles the cases of
> PCIe Multi-Link + USB, PCIe Multi-Link + Q/SGMII
> which Swapnil has pointed to at [1], since PCIe Multi-Link is now a new
> protocol in itself (PHY_TYPE_PCIE_ML) and shall be represented in that
> manner in the device-tree when it is expected to be combined with a second
> protocol.
> 
> After grouping the links by protocol, we can check for the entry in
> "link_cmn_vals_entries" and proceed to configure it identical to the
> 2 Link case.
> 
> [1] https://github.com/t-c-collab/linux/commits/ti-linux-6.1.y-torrent-multi-pcie-sgmii-v1

This proposal looks good to me. Thanks!

-- 
cheers,
-roger