Messages in this thread | | | Date | Thu, 25 Apr 2024 11:28:17 -0400 | Subject | Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type | From | Sean Anderson <> |
| |
On 4/24/24 02:38, Michal Simek wrote: > > > On 4/23/24 17:02, Sean Anderson wrote: >> On 4/23/24 02:25, Michal Simek wrote: >>> >>> >>> On 4/22/24 20:58, Sean Anderson wrote: >>>> The phy "type" is just the combination of protocol and instance, and is >>>> never used apart from that. Store the instance directly, instead of >>>> converting to a type first. No functional change intended. >>>> >>>> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >>>> --- >>>> >>>> drivers/phy/xilinx/phy-zynqmp.c | 107 +++++++------------------------- >>>> 1 file changed, 24 insertions(+), 83 deletions(-) >>>> >>>> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c >>>> index f72c5257d712..b507ed4c3053 100644 >>>> --- a/drivers/phy/xilinx/phy-zynqmp.c >>>> +++ b/drivers/phy/xilinx/phy-zynqmp.c >>>> @@ -146,22 +146,6 @@ >>>> /* Total number of controllers */ >>>> #define CONTROLLERS_PER_LANE 5 >>>> -/* Protocol Type parameters */ >>>> -#define XPSGTR_TYPE_USB0 0 /* USB controller 0 */ >>>> -#define XPSGTR_TYPE_USB1 1 /* USB controller 1 */ >>>> -#define XPSGTR_TYPE_SATA_0 2 /* SATA controller lane 0 */ >>>> -#define XPSGTR_TYPE_SATA_1 3 /* SATA controller lane 1 */ >>>> -#define XPSGTR_TYPE_PCIE_0 4 /* PCIe controller lane 0 */ >>>> -#define XPSGTR_TYPE_PCIE_1 5 /* PCIe controller lane 1 */ >>>> -#define XPSGTR_TYPE_PCIE_2 6 /* PCIe controller lane 2 */ >>>> -#define XPSGTR_TYPE_PCIE_3 7 /* PCIe controller lane 3 */ >>>> -#define XPSGTR_TYPE_DP_0 8 /* Display Port controller lane 0 */ >>>> -#define XPSGTR_TYPE_DP_1 9 /* Display Port controller lane 1 */ >>>> -#define XPSGTR_TYPE_SGMII0 10 /* Ethernet SGMII controller 0 */ >>>> -#define XPSGTR_TYPE_SGMII1 11 /* Ethernet SGMII controller 1 */ >>>> -#define XPSGTR_TYPE_SGMII2 12 /* Ethernet SGMII controller 2 */ >>>> -#define XPSGTR_TYPE_SGMII3 13 /* Ethernet SGMII controller 3 */ >>>> - >>>> /* Timeout values */ >>>> #define TIMEOUT_US 1000 >>>> @@ -184,7 +168,8 @@ struct xpsgtr_ssc { >>>> /** >>>> * struct xpsgtr_phy - representation of a lane >>>> * @phy: pointer to the kernel PHY device >>>> - * @type: controller which uses this lane >>>> + * @instance: instance of the protocol type (such as the lane within a >>>> + * protocol, or the USB/Ethernet controller) >>>> * @lane: lane number >>>> * @protocol: protocol in which the lane operates >>>> * @skip_phy_init: skip phy_init() if true >>>> @@ -193,7 +178,7 @@ struct xpsgtr_ssc { >>>> */ >>>> struct xpsgtr_phy { >>>> struct phy *phy; >>>> - u8 type; >>>> + u8 instance; >>>> u8 lane; >>>> u8 protocol; >>>> bool skip_phy_init; >>>> @@ -330,8 +315,8 @@ static int xpsgtr_wait_pll_lock(struct phy *phy) >>>> if (ret == -ETIMEDOUT) >>>> dev_err(gtr_dev->dev, >>>> - "lane %u (type %u, protocol %u): PLL lock timeout\n", >>>> - gtr_phy->lane, gtr_phy->type, gtr_phy->protocol); >>>> + "lane %u (protocol %u, instance %u): PLL lock timeout\n", >>>> + gtr_phy->lane, gtr_phy->protocol, gtr_phy->instance); >>>> return ret; >>>> } >>>> @@ -643,8 +628,7 @@ static int xpsgtr_phy_power_on(struct phy *phy) >>>> * cumulating waits for both lanes. The user is expected to initialize >>>> * lane 0 last. >>>> */ >>>> - if (gtr_phy->protocol != ICM_PROTOCOL_DP || >>>> - gtr_phy->type == XPSGTR_TYPE_DP_0) >>>> + if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance) >>>> ret = xpsgtr_wait_pll_lock(phy); >>>> return ret; >>>> @@ -674,73 +658,33 @@ static const struct phy_ops xpsgtr_phyops = { >>>> * OF Xlate Support >>>> */ >>>> -/* Set the lane type and protocol based on the PHY type and instance number. */ >>>> +/* Set the lane protocol and instance based on the PHY type and instance number. */ >>>> static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type, >>>> unsigned int phy_instance) >>>> { >>>> unsigned int num_phy_types; >>>> - const int *phy_types; >>>> switch (phy_type) { >>>> - case PHY_TYPE_SATA: { >>>> - static const int types[] = { >>>> - XPSGTR_TYPE_SATA_0, >>>> - XPSGTR_TYPE_SATA_1, >>>> - }; >>>> - >>>> - phy_types = types; >>>> - num_phy_types = ARRAY_SIZE(types); >>>> + case PHY_TYPE_SATA: >>>> + num_phy_types = 2; >>>> gtr_phy->protocol = ICM_PROTOCOL_SATA; >>>> break; >>>> - } >>>> - case PHY_TYPE_USB3: { >>>> - static const int types[] = { >>>> - XPSGTR_TYPE_USB0, >>>> - XPSGTR_TYPE_USB1, >>>> - }; >>>> - >>>> - phy_types = types; >>>> - num_phy_types = ARRAY_SIZE(types); >>>> + case PHY_TYPE_USB3: >>>> + num_phy_types = 2; >>>> gtr_phy->protocol = ICM_PROTOCOL_USB; >>>> break; >>>> - } >>>> - case PHY_TYPE_DP: { >>>> - static const int types[] = { >>>> - XPSGTR_TYPE_DP_0, >>>> - XPSGTR_TYPE_DP_1, >>>> - }; >>>> - >>>> - phy_types = types; >>>> - num_phy_types = ARRAY_SIZE(types); >>>> + case PHY_TYPE_DP: >>>> + num_phy_types = 2; >>>> gtr_phy->protocol = ICM_PROTOCOL_DP; >>>> break; >>>> - } >>>> - case PHY_TYPE_PCIE: { >>>> - static const int types[] = { >>>> - XPSGTR_TYPE_PCIE_0, >>>> - XPSGTR_TYPE_PCIE_1, >>>> - XPSGTR_TYPE_PCIE_2, >>>> - XPSGTR_TYPE_PCIE_3, >>>> - }; >>>> - >>>> - phy_types = types; >>>> - num_phy_types = ARRAY_SIZE(types); >>>> + case PHY_TYPE_PCIE: >>>> + num_phy_types = 4; >>>> gtr_phy->protocol = ICM_PROTOCOL_PCIE; >>>> break; >>>> - } >>>> - case PHY_TYPE_SGMII: { >>>> - static const int types[] = { >>>> - XPSGTR_TYPE_SGMII0, >>>> - XPSGTR_TYPE_SGMII1, >>>> - XPSGTR_TYPE_SGMII2, >>>> - XPSGTR_TYPE_SGMII3, >>>> - }; >>>> - >>>> - phy_types = types; >>>> - num_phy_types = ARRAY_SIZE(types); >>>> + case PHY_TYPE_SGMII: >>>> + num_phy_types = 4; >>>> gtr_phy->protocol = ICM_PROTOCOL_SGMII; >>>> break; >>>> - } >>>> default: >>>> return -EINVAL; >>>> } >>>> @@ -748,7 +692,7 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type, >>>> if (phy_instance >= num_phy_types) >>>> return -EINVAL; >>>> - gtr_phy->type = phy_types[phy_instance]; >>>> + gtr_phy->instance = phy_instance; >>>> return 0; >>>> } >>>> @@ -756,14 +700,11 @@ static int xpsgtr_set_lane_type(struct xpsgtr_phy *gtr_phy, u8 phy_type, >>>> * Valid combinations of controllers and lanes (Interconnect Matrix). >>>> */ >>>> static const unsigned int icm_matrix[NUM_LANES][CONTROLLERS_PER_LANE] = { >>>> - { XPSGTR_TYPE_PCIE_0, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0, >>>> - XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII0 }, >>>> - { XPSGTR_TYPE_PCIE_1, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB0, >>>> - XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII1 }, >>>> - { XPSGTR_TYPE_PCIE_2, XPSGTR_TYPE_SATA_0, XPSGTR_TYPE_USB0, >>>> - XPSGTR_TYPE_DP_1, XPSGTR_TYPE_SGMII2 }, >>>> - { XPSGTR_TYPE_PCIE_3, XPSGTR_TYPE_SATA_1, XPSGTR_TYPE_USB1, >>>> - XPSGTR_TYPE_DP_0, XPSGTR_TYPE_SGMII3 } >>>> + /* PCIe, SATA, USB, DP, SGMII */ >>>> + { 0, 0, 0, 1, 0 }, >>>> + { 1, 1, 0, 0, 1 }, >>>> + { 2, 0, 0, 1, 2 }, >>>> + { 3, 1, 1, 0, 3 }, >>> >>> >>> Do you think that this is more understandable than was before? >> >> Yes. This better matches the documentation. And now it is easier to >> programmatically access the index. Before we might have >> >> protocol = ICM_PROTOCOL_USB >> type = XPSGTR_TYPE_USB0 >> >> and the instance is there in the type name, but we can't access it. Now >> we have >> >> instance = 0 >> >> and it is easy to determine which instance we have. > > Numbers itself means in PCIE, SATA and DP number of lanes used but > there is only single controller. In USB and SGMII case it is one lane > per controller which is different information pointing to controller > ID.
Yeah, the instance is really just to validate the devicetree binding. It's not used in the driver except to determine the lane to wait for PLL lock on in a multi-lane configuration.
> I didn't look at this driver for a while but at least for this array I > would prefer to extend comment above to describe position of > controllers and connected lanes and explain what that numbers really > means.
OK, I will expand the comment for v2.
--Sean
| |