| Message ID | 20220909091433.3715981-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | PCI: qcom: Support using the same PHY for both RC and EP | expand |
On Fri, Sep 09, 2022 at 12:14:24PM +0300, Dmitry Baryshkov wrote: > Programming of QMP PCIe PHYs slightly differs between RC and EP modes. > > Currently both qcom and qcom-ep PCIe controllers setup the PHY in the > default mode, making it impossible to select at runtime whether the PHY > should be running in RC or in EP modes. Usually this is not an issue, > since for most devices only the RC mode is used. Some devices (SDX55) > currently support only the EP mode without supporting the RC mode (at > this moment). > > Nevertheless some of the Qualcomm platforms (e.g. the aforementioned > SDX55) would still benefit from being able to switch between RC and EP > depending on the driver being used. While it is possible to use > different compat strings for the PHY depending on the mode, it seems > like an incorrect approach, since the PHY doesn't differ between > usecases. It's the PCIe controller, who should decide how to configure > the PHY. > > This patch series implements the ability to select between RC and EP > modes, by allowing the PCIe QMP PHY driver to switch between > programming tables. > > Unlike previous iterations, this series brings in the dependecy from > PCI parts onto the first patch. Merging of PHY and PCI parts should be > coordinated by the maintainers (e.g. by putting the first patch into the > immutable branch). > > Changes since v2: > - Added PHY_SUBMODE_PCIE_RC/EP defines (Vinod), > - Changed `primary' table name to `main', added extra comments > describing that `secondary' are the additional tables, not required in > most of the cases (following the suggestion by Johan to rename > `primary' table), This wasn't really what I suggested. "main" is in itself is no more understandable than "primary". Please take another look at: https://lore.kernel.org/all/Yw2+aVbqBfMSUcWq@hovoldconsulting.com/ Johan
On 14-09-22, 08:48, Johan Hovold wrote: > On Fri, Sep 09, 2022 at 12:14:24PM +0300, Dmitry Baryshkov wrote: > > Changes since v2: > > - Added PHY_SUBMODE_PCIE_RC/EP defines (Vinod), > > - Changed `primary' table name to `main', added extra comments > > describing that `secondary' are the additional tables, not required in > > most of the cases (following the suggestion by Johan to rename > > `primary' table), > > This wasn't really what I suggested. "main" is in itself is no more > understandable than "primary". > > Please take another look at: > > https://lore.kernel.org/all/Yw2+aVbqBfMSUcWq@hovoldconsulting.com/ Am not sure example quoted there was very initutive: "as the tables can be referred to as cfg.tbls2.serdes instead of cfg.secondary.serdes_tbl;" I would agree with Johan that primary and secondary are too long, but that tbls2 is not very intuitive either... Maybe shorten to pri/sec... any better idea?
On 09-09-22, 12:14, Dmitry Baryshkov wrote: > Define two submodes to be used for the PCIe PHYs, where required. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > include/linux/phy/phy.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h > index b1413757fcc3..bd60c1a72988 100644 > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -45,6 +45,15 @@ enum phy_mode { > PHY_MODE_DP > }; > > +/* > + * Submodes for the PHY_MODE_PCIE, allowing the host to select between RC (Root > + * Complex) and EP (End Point) PHY modes. > + */ > +enum { > + PHY_SUBMODE_PCIE_RC, > + PHY_SUBMODE_PCIE_EP, > +}; This can be dropped see include/linux/phy/pcie.h > + > enum phy_media { > PHY_MEDIA_DEFAULT, > PHY_MEDIA_SR, > -- > 2.35.1
On Sat, Sep 24, 2022 at 11:18:12AM +0530, Vinod Koul wrote: > On 14-09-22, 08:48, Johan Hovold wrote: > > On Fri, Sep 09, 2022 at 12:14:24PM +0300, Dmitry Baryshkov wrote: > > > > Changes since v2: > > > - Added PHY_SUBMODE_PCIE_RC/EP defines (Vinod), > > > - Changed `primary' table name to `main', added extra comments > > > describing that `secondary' are the additional tables, not required in > > > most of the cases (following the suggestion by Johan to rename > > > `primary' table), > > > > This wasn't really what I suggested. "main" is in itself is no more > > understandable than "primary". > > > > Please take another look at: > > > > https://lore.kernel.org/all/Yw2+aVbqBfMSUcWq@hovoldconsulting.com/ > > Am not sure example quoted there was very initutive: > "as the tables can be referred to as > > cfg.tbls2.serdes > > instead of > > cfg.secondary.serdes_tbl;" The main point was that "secondary" doesn't say anything about what the variable is used for (unlike for example "secondary_tbls") and that keeping a "_tbl" suffix on every member in a structure that holds only tables is redundant. > I would agree with Johan that primary and secondary are too long, but > that tbls2 is not very intuitive either... That could be cfg.tbls_extra.serdes; too or whatever. The key point was to have a descriptive name of the tables-structure variable and dropping "_tbl" from the individual members. Johan
Programming of QMP PCIe PHYs slightly differs between RC and EP modes. Currently both qcom and qcom-ep PCIe controllers setup the PHY in the default mode, making it impossible to select at runtime whether the PHY should be running in RC or in EP modes. Usually this is not an issue, since for most devices only the RC mode is used. Some devices (SDX55) currently support only the EP mode without supporting the RC mode (at this moment). Nevertheless some of the Qualcomm platforms (e.g. the aforementioned SDX55) would still benefit from being able to switch between RC and EP depending on the driver being used. While it is possible to use different compat strings for the PHY depending on the mode, it seems like an incorrect approach, since the PHY doesn't differ between usecases. It's the PCIe controller, who should decide how to configure the PHY. This patch series implements the ability to select between RC and EP modes, by allowing the PCIe QMP PHY driver to switch between programming tables. Unlike previous iterations, this series brings in the dependecy from PCI parts onto the first patch. Merging of PHY and PCI parts should be coordinated by the maintainers (e.g. by putting the first patch into the immutable branch). Changes since v2: - Added PHY_SUBMODE_PCIE_RC/EP defines (Vinod), - Changed `primary' table name to `main', added extra comments describing that `secondary' are the additional tables, not required in most of the cases (following the suggestion by Johan to rename `primary' table), - Changed secondary tables into the pointers to stop wasting extra memory (Vinod), - Split several functions for programming the PHY using these tables. Changes since v1: - Split the if(table) removal to the separate patch - Expanded commit messages and comments to provide additional details - Fixed build error on pcie-qcom.c - Added support for EP mode on sm8450 to demonstrate the usage of this patchset Changes since RFC: - Fixed the compilation of PCIe EP driver, - Changed pri/sec names to primary and secondary, - Added comments regarding usage of secondary_rc/_ep fields. Dmitry Baryshkov (9): phy: define submodes for PCIe PHYs phy: qcom-qmp-pcie: drop if (table) conditions phy: qcom-qmp-pcie: split register tables into main and secondary parts phy: qcom-qmp-pcie: split PHY programming to separate functions phy: qcom-qmp-pcie: turn secondary programming table into a pointer phy: qcom-qmp-pcie: support separate tables for EP mode phy: qcom-qmp-pcie: Support SM8450 PCIe1 PHY in EP mode PCI: qcom: Setup PHY to work in RC mode PCI: qcom-ep: Setup PHY to work in EP mode drivers/pci/controller/dwc/pcie-qcom-ep.c | 4 + drivers/pci/controller/dwc/pcie-qcom.c | 4 + drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 312 ++++++++++++------ .../qualcomm/phy-qcom-qmp-pcs-pcie-v5_20.h | 1 + include/linux/phy/phy.h | 9 + 5 files changed, 229 insertions(+), 101 deletions(-)