* [PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support
@ 2024-04-22 18:58 Sean Anderson
2024-04-22 18:58 ` [PATCH 1/3] phy: zynqmp: Store instance instead of type Sean Anderson
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Sean Anderson @ 2024-04-22 18:58 UTC (permalink / raw)
To: Laurent Pinchart, linux-phy
Cc: Vinod Koul, linux-kernel, Michal Simek, linux-arm-kernel,
Kishon Vijay Abraham I, Sean Anderson
This has a few small patches cleaning up the driver, adding a fix for
PCIe, and adding some useful debugfs info.
Sean Anderson (3):
phy: zynqmp: Store instance instead of type
phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes
phy: zynqmp: Add debugfs support
drivers/phy/xilinx/phy-zynqmp.c | 169 ++++++++++++++++----------------
1 file changed, 83 insertions(+), 86 deletions(-)
--
2.35.1.1320.gc452695387.dirty
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] phy: zynqmp: Store instance instead of type 2024-04-22 18:58 [PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support Sean Anderson @ 2024-04-22 18:58 ` Sean Anderson 2024-04-23 6:25 ` Michal Simek 2024-04-22 18:58 ` [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes Sean Anderson 2024-04-22 18:58 ` [PATCH 3/3] phy: zynqmp: Add debugfs support Sean Anderson 2 siblings, 1 reply; 12+ messages in thread From: Sean Anderson @ 2024-04-22 18:58 UTC (permalink / raw) To: Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, Michal Simek, linux-arm-kernel, Kishon Vijay Abraham I, Sean Anderson 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 }, }; /* 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; } -- 2.35.1.1320.gc452695387.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type 2024-04-22 18:58 ` [PATCH 1/3] phy: zynqmp: Store instance instead of type Sean Anderson @ 2024-04-23 6:25 ` Michal Simek 2024-04-23 15:02 ` Sean Anderson 0 siblings, 1 reply; 12+ messages in thread From: Michal Simek @ 2024-04-23 6:25 UTC (permalink / raw) To: Sean Anderson, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type 2024-04-23 6:25 ` Michal Simek @ 2024-04-23 15:02 ` Sean Anderson 2024-04-24 6:38 ` Michal Simek 0 siblings, 1 reply; 12+ messages in thread From: Sean Anderson @ 2024-04-23 15:02 UTC (permalink / raw) To: Michal Simek, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type 2024-04-23 15:02 ` Sean Anderson @ 2024-04-24 6:38 ` Michal Simek 2024-04-25 15:28 ` Sean Anderson 0 siblings, 1 reply; 12+ messages in thread From: Michal Simek @ 2024-04-24 6:38 UTC (permalink / raw) To: Sean Anderson, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] phy: zynqmp: Store instance instead of type 2024-04-24 6:38 ` Michal Simek @ 2024-04-25 15:28 ` Sean Anderson 0 siblings, 0 replies; 12+ messages in thread From: Sean Anderson @ 2024-04-25 15:28 UTC (permalink / raw) To: Michal Simek, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes 2024-04-22 18:58 [PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support Sean Anderson 2024-04-22 18:58 ` [PATCH 1/3] phy: zynqmp: Store instance instead of type Sean Anderson @ 2024-04-22 18:58 ` Sean Anderson 2024-04-23 6:25 ` Michal Simek 2024-04-22 18:58 ` [PATCH 3/3] phy: zynqmp: Add debugfs support Sean Anderson 2 siblings, 1 reply; 12+ messages in thread From: Sean Anderson @ 2024-04-22 18:58 UTC (permalink / raw) To: Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, Michal Simek, linux-arm-kernel, Kishon Vijay Abraham I, Sean Anderson Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1]. Don't wait for them. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- Maybe it's actually that only the final lane configured locks? I haven't tested this out. drivers/phy/xilinx/phy-zynqmp.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c index b507ed4c3053..08c88dcd7799 100644 --- a/drivers/phy/xilinx/phy-zynqmp.c +++ b/drivers/phy/xilinx/phy-zynqmp.c @@ -624,11 +624,13 @@ static int xpsgtr_phy_power_on(struct phy *phy) if (!xpsgtr_phy_init_required(gtr_phy)) return ret; /* - * Wait for the PLL to lock. For DP, only wait on DP0 to avoid - * cumulating waits for both lanes. The user is expected to initialize - * lane 0 last. + * Wait for the PLL to lock. For DP and PCIe, only wait on instance 0 + * to avoid cumulative waits for both lanes. The user is expected to + * initialize lane 0 last. */ - if (gtr_phy->protocol != ICM_PROTOCOL_DP || !gtr_phy->instance) + if ((gtr_phy->protocol != ICM_PROTOCOL_DP && + gtr_phy->protocol != ICM_PROTOCOL_PCIE) || + !gtr_phy->instance) ret = xpsgtr_wait_pll_lock(phy); return ret; -- 2.35.1.1320.gc452695387.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes 2024-04-22 18:58 ` [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes Sean Anderson @ 2024-04-23 6:25 ` Michal Simek 2024-04-23 15:03 ` Sean Anderson 0 siblings, 1 reply; 12+ messages in thread From: Michal Simek @ 2024-04-23 6:25 UTC (permalink / raw) To: Sean Anderson, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I On 4/22/24 20:58, Sean Anderson wrote: > Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1]. What is this [1] for? M _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes 2024-04-23 6:25 ` Michal Simek @ 2024-04-23 15:03 ` Sean Anderson 0 siblings, 0 replies; 12+ messages in thread From: Sean Anderson @ 2024-04-23 15:03 UTC (permalink / raw) To: Michal Simek, Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, linux-arm-kernel, Kishon Vijay Abraham I On 4/23/24 02:25, Michal Simek wrote: > > > On 4/22/24 20:58, Sean Anderson wrote: >> Similarly to DisplayPort, nonzero PCIe lanes never achieve PLL lock [1]. > > What is this [1] for? I was originally going to have the comment below the fold in the commit message as a footnote. I forgot to remove this after editing. --Sean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] phy: zynqmp: Add debugfs support 2024-04-22 18:58 [PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support Sean Anderson 2024-04-22 18:58 ` [PATCH 1/3] phy: zynqmp: Store instance instead of type Sean Anderson 2024-04-22 18:58 ` [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes Sean Anderson @ 2024-04-22 18:58 ` Sean Anderson 2024-05-04 11:58 ` Vinod Koul 2 siblings, 1 reply; 12+ messages in thread From: Sean Anderson @ 2024-04-22 18:58 UTC (permalink / raw) To: Laurent Pinchart, linux-phy Cc: Vinod Koul, linux-kernel, Michal Simek, linux-arm-kernel, Kishon Vijay Abraham I, Sean Anderson Add support for printing some basic status information to debugfs. This is helpful when debugging phy consumers to make sure they are configuring the phy appropriately. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- drivers/phy/xilinx/phy-zynqmp.c | 54 +++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c index 08c88dcd7799..e2e86943e9f3 100644 --- a/drivers/phy/xilinx/phy-zynqmp.c +++ b/drivers/phy/xilinx/phy-zynqmp.c @@ -13,6 +13,7 @@ */ #include <linux/clk.h> +#include <linux/debugfs.h> #include <linux/delay.h> #include <linux/io.h> #include <linux/kernel.h> @@ -122,6 +123,15 @@ #define ICM_PROTOCOL_DP 0x4 #define ICM_PROTOCOL_SGMII 0x5 +static const char *const xpsgtr_icm_str[] = { + [ICM_PROTOCOL_PD] = "powered down", + [ICM_PROTOCOL_PCIE] = "PCIe", + [ICM_PROTOCOL_SATA] = "SATA", + [ICM_PROTOCOL_USB] = "USB", + [ICM_PROTOCOL_DP] = "DisplayPort", + [ICM_PROTOCOL_SGMII] = "SGMII", +}; + /* Test Mode common reset control parameters */ #define TM_CMN_RST 0x10018 #define TM_CMN_RST_EN 0x1 @@ -768,6 +778,48 @@ static struct phy *xpsgtr_xlate(struct device *dev, return ERR_PTR(-EINVAL); } +/* + * DebugFS + */ + +static int xpsgtr_status_read(struct seq_file *seq, void *data) +{ + struct xpsgtr_phy *gtr_phy = seq->private; + struct clk *clk; + u32 pll_status; + + mutex_lock(>r_phy->phy->mutex); + pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1); + clk = gtr_phy->dev->clk[gtr_phy->refclk]; + + seq_printf(seq, "Lane: %u\n", gtr_phy->lane); + seq_printf(seq, "Protocol: %s\n", + xpsgtr_icm_str[gtr_phy->protocol]); + seq_printf(seq, "Instance: %u\n", gtr_phy->instance); + seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk); + seq_printf(seq, "Reference rate: %lu\n", clk_get_rate(clk)); + seq_printf(seq, "PLL locked: %s\n", + pll_status & PLL_STATUS_LOCKED ? "yes" : "no"); + + mutex_unlock(>r_phy->phy->mutex); + return 0; +} + +static int xpsgtr_status_open(struct inode *inode, struct file *f) +{ + struct xpsgtr_phy *gtr_phy = inode->i_private; + + return single_open(f, xpsgtr_status_read, gtr_phy); +} + +static const struct file_operations xpsgtr_status_ops = { + .owner = THIS_MODULE, + .open = xpsgtr_status_open, + .release = single_release, + .read = seq_read, + .llseek = seq_lseek +}; + /* * Power Management */ @@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev) gtr_phy->phy = phy; phy_set_drvdata(phy, gtr_phy); + debugfs_create_file("status", 0444, phy->debugfs, gtr_phy, + &xpsgtr_status_ops); } /* Register the PHY provider. */ -- 2.35.1.1320.gc452695387.dirty _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] phy: zynqmp: Add debugfs support 2024-04-22 18:58 ` [PATCH 3/3] phy: zynqmp: Add debugfs support Sean Anderson @ 2024-05-04 11:58 ` Vinod Koul 2024-05-06 14:51 ` Sean Anderson 0 siblings, 1 reply; 12+ messages in thread From: Vinod Koul @ 2024-05-04 11:58 UTC (permalink / raw) To: Sean Anderson Cc: Laurent Pinchart, linux-phy, linux-kernel, Michal Simek, linux-arm-kernel, Kishon Vijay Abraham I On 22-04-24, 14:58, Sean Anderson wrote: > Add support for printing some basic status information to debugfs. This > is helpful when debugging phy consumers to make sure they are configuring > the phy appropriately. > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > drivers/phy/xilinx/phy-zynqmp.c | 54 +++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c > index 08c88dcd7799..e2e86943e9f3 100644 > --- a/drivers/phy/xilinx/phy-zynqmp.c > +++ b/drivers/phy/xilinx/phy-zynqmp.c > @@ -13,6 +13,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/debugfs.h> > #include <linux/delay.h> > #include <linux/io.h> > #include <linux/kernel.h> > @@ -122,6 +123,15 @@ > #define ICM_PROTOCOL_DP 0x4 > #define ICM_PROTOCOL_SGMII 0x5 > > +static const char *const xpsgtr_icm_str[] = { > + [ICM_PROTOCOL_PD] = "powered down", > + [ICM_PROTOCOL_PCIE] = "PCIe", > + [ICM_PROTOCOL_SATA] = "SATA", > + [ICM_PROTOCOL_USB] = "USB", > + [ICM_PROTOCOL_DP] = "DisplayPort", > + [ICM_PROTOCOL_SGMII] = "SGMII", > +}; > + > /* Test Mode common reset control parameters */ > #define TM_CMN_RST 0x10018 > #define TM_CMN_RST_EN 0x1 > @@ -768,6 +778,48 @@ static struct phy *xpsgtr_xlate(struct device *dev, > return ERR_PTR(-EINVAL); > } > > +/* > + * DebugFS > + */ > + > +static int xpsgtr_status_read(struct seq_file *seq, void *data) > +{ > + struct xpsgtr_phy *gtr_phy = seq->private; > + struct clk *clk; > + u32 pll_status; > + > + mutex_lock(>r_phy->phy->mutex); > + pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1); > + clk = gtr_phy->dev->clk[gtr_phy->refclk]; > + > + seq_printf(seq, "Lane: %u\n", gtr_phy->lane); > + seq_printf(seq, "Protocol: %s\n", > + xpsgtr_icm_str[gtr_phy->protocol]); > + seq_printf(seq, "Instance: %u\n", gtr_phy->instance); > + seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk); > + seq_printf(seq, "Reference rate: %lu\n", clk_get_rate(clk)); > + seq_printf(seq, "PLL locked: %s\n", > + pll_status & PLL_STATUS_LOCKED ? "yes" : "no"); > + > + mutex_unlock(>r_phy->phy->mutex); > + return 0; > +} > + > +static int xpsgtr_status_open(struct inode *inode, struct file *f) > +{ > + struct xpsgtr_phy *gtr_phy = inode->i_private; > + > + return single_open(f, xpsgtr_status_read, gtr_phy); > +} > + > +static const struct file_operations xpsgtr_status_ops = { > + .owner = THIS_MODULE, > + .open = xpsgtr_status_open, > + .release = single_release, > + .read = seq_read, > + .llseek = seq_lseek > +}; There are debugfs simple helpers which should help you avoid all this open coding and just have the read call > + > /* > * Power Management > */ > @@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev) > > gtr_phy->phy = phy; > phy_set_drvdata(phy, gtr_phy); > + debugfs_create_file("status", 0444, phy->debugfs, gtr_phy, > + &xpsgtr_status_ops); generic status does not make sense, how about device-name-status -- ~Vinod _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] phy: zynqmp: Add debugfs support 2024-05-04 11:58 ` Vinod Koul @ 2024-05-06 14:51 ` Sean Anderson 0 siblings, 0 replies; 12+ messages in thread From: Sean Anderson @ 2024-05-06 14:51 UTC (permalink / raw) To: Vinod Koul Cc: Laurent Pinchart, linux-phy, linux-kernel, Michal Simek, linux-arm-kernel, Kishon Vijay Abraham I On 5/4/24 07:58, Vinod Koul wrote: > On 22-04-24, 14:58, Sean Anderson wrote: >> Add support for printing some basic status information to debugfs. This >> is helpful when debugging phy consumers to make sure they are configuring >> the phy appropriately. >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> --- >> >> drivers/phy/xilinx/phy-zynqmp.c | 54 +++++++++++++++++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> >> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c >> index 08c88dcd7799..e2e86943e9f3 100644 >> --- a/drivers/phy/xilinx/phy-zynqmp.c >> +++ b/drivers/phy/xilinx/phy-zynqmp.c >> @@ -13,6 +13,7 @@ >> */ >> >> #include <linux/clk.h> >> +#include <linux/debugfs.h> >> #include <linux/delay.h> >> #include <linux/io.h> >> #include <linux/kernel.h> >> @@ -122,6 +123,15 @@ >> #define ICM_PROTOCOL_DP 0x4 >> #define ICM_PROTOCOL_SGMII 0x5 >> >> +static const char *const xpsgtr_icm_str[] = { >> + [ICM_PROTOCOL_PD] = "powered down", >> + [ICM_PROTOCOL_PCIE] = "PCIe", >> + [ICM_PROTOCOL_SATA] = "SATA", >> + [ICM_PROTOCOL_USB] = "USB", >> + [ICM_PROTOCOL_DP] = "DisplayPort", >> + [ICM_PROTOCOL_SGMII] = "SGMII", >> +}; >> + >> /* Test Mode common reset control parameters */ >> #define TM_CMN_RST 0x10018 >> #define TM_CMN_RST_EN 0x1 >> @@ -768,6 +778,48 @@ static struct phy *xpsgtr_xlate(struct device *dev, >> return ERR_PTR(-EINVAL); >> } >> >> +/* >> + * DebugFS >> + */ >> + >> +static int xpsgtr_status_read(struct seq_file *seq, void *data) >> +{ >> + struct xpsgtr_phy *gtr_phy = seq->private; >> + struct clk *clk; >> + u32 pll_status; >> + >> + mutex_lock(>r_phy->phy->mutex); >> + pll_status = xpsgtr_read_phy(gtr_phy, L0_PLL_STATUS_READ_1); >> + clk = gtr_phy->dev->clk[gtr_phy->refclk]; >> + >> + seq_printf(seq, "Lane: %u\n", gtr_phy->lane); >> + seq_printf(seq, "Protocol: %s\n", >> + xpsgtr_icm_str[gtr_phy->protocol]); >> + seq_printf(seq, "Instance: %u\n", gtr_phy->instance); >> + seq_printf(seq, "Reference clock: %u (%pC)\n", gtr_phy->refclk, clk); >> + seq_printf(seq, "Reference rate: %lu\n", clk_get_rate(clk)); >> + seq_printf(seq, "PLL locked: %s\n", >> + pll_status & PLL_STATUS_LOCKED ? "yes" : "no"); >> + >> + mutex_unlock(>r_phy->phy->mutex); >> + return 0; >> +} >> + >> +static int xpsgtr_status_open(struct inode *inode, struct file *f) >> +{ >> + struct xpsgtr_phy *gtr_phy = inode->i_private; >> + >> + return single_open(f, xpsgtr_status_read, gtr_phy); >> +} >> + >> +static const struct file_operations xpsgtr_status_ops = { >> + .owner = THIS_MODULE, >> + .open = xpsgtr_status_open, >> + .release = single_release, >> + .read = seq_read, >> + .llseek = seq_lseek >> +}; > > There are debugfs simple helpers which should help you avoid all this > open coding and just have the read call I assume you mean debugfs_create_devm_seqfile. >> + >> /* >> * Power Management >> */ >> @@ -917,6 +969,8 @@ static int xpsgtr_probe(struct platform_device *pdev) >> >> gtr_phy->phy = phy; >> phy_set_drvdata(phy, gtr_phy); >> + debugfs_create_file("status", 0444, phy->debugfs, gtr_phy, >> + &xpsgtr_status_ops); > > generic status does not make sense, how about device-name-status This file lives in the phy's debugfs directory already. The full path is /sys/kernel/debug/phy/phy-fd400000.phy.0/status so there is no need to add the phy name yet again. --Sean _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-05-06 14:52 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-22 18:58 [PATCH 0/3] phy: zynqmp: A PCIe fix and debugfs support Sean Anderson 2024-04-22 18:58 ` [PATCH 1/3] phy: zynqmp: Store instance instead of type Sean Anderson 2024-04-23 6:25 ` Michal Simek 2024-04-23 15:02 ` Sean Anderson 2024-04-24 6:38 ` Michal Simek 2024-04-25 15:28 ` Sean Anderson 2024-04-22 18:58 ` [PATCH 2/3] phy: zynqmp: Don't wait for PLL lock on nonzero PCIe lanes Sean Anderson 2024-04-23 6:25 ` Michal Simek 2024-04-23 15:03 ` Sean Anderson 2024-04-22 18:58 ` [PATCH 3/3] phy: zynqmp: Add debugfs support Sean Anderson 2024-05-04 11:58 ` Vinod Koul 2024-05-06 14:51 ` Sean Anderson
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).