| Message ID | 20240422185803.3575319-2-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | phy: zynqmp: A PCIe fix and debugfs support | expand |
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? Thanks, Michal
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. --Sean
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. 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. Thanks, Michal
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
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 }, }; /* Translate OF phandle and args to PHY instance. */ @@ -818,7 +759,7 @@ static struct phy *xpsgtr_xlate(struct device *dev, * is allowed to operate on the lane. */ for (i = 0; i < CONTROLLERS_PER_LANE; i++) { - if (icm_matrix[phy_lane][i] == gtr_phy->type) + if (icm_matrix[phy_lane][i] == gtr_phy->instance) return gtr_phy->phy; }
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(-)