Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
Hi Vinod, On 5/5/2020 3:54 PM, Dilip Kota wrote: On 5/5/2020 1:21 PM, Vinod Koul wrote: On 04-05-20, 17:32, Dilip Kota wrote: On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. Driver uses regmap except here, which seems odd hence explanation required for this. During the driver Probe, the register phandles are stored in regmap datatype variables and PHY core addresses are stored in iomem datatype. Since then, regmap access is performed for the regmap datatype variables and readl/writel access is performed on the iomem datatype variables. And nowhere in the driver iomem datatype address are converted to regmap address and performed regmap access. Driver is not doing any 'regmap_init' on any physical address. Driver is getting the register address phandle from the device tree node and performing the regmap access. ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 1, 0, &ref); [...] cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1, 0, &ref); [...] cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app"); [...] cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core"); The DT parsing logic in the driver is explaining why the PHY driver should do regmap access and to whom should be done. For this reason i am a bit puzzled to what more is needed to explain in the comments and where to add it. Please let me know your view. Gentle Reminder! Could you please update on this. Regards, Dilip Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/5/2020 1:21 PM, Vinod Koul wrote: On 04-05-20, 17:32, Dilip Kota wrote: On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. Driver uses regmap except here, which seems odd hence explanation required for this. During the driver Probe, the register phandles are stored in regmap datatype variables and PHY core addresses are stored in iomem datatype. Since then, regmap access is performed for the regmap datatype variables and readl/writel access is performed on the iomem datatype variables. And nowhere in the driver iomem datatype address are converted to regmap address and performed regmap access. Driver is not doing any 'regmap_init' on any physical address. Driver is getting the register address phandle from the device tree node and performing the regmap access. ret = fwnode_property_get_reference_args(fwnode, "intel,syscfg", NULL, 1, 0, &ref); [...] cbphy->syscfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] ret = fwnode_property_get_reference_args(fwnode, "intel,hsio", NULL, 1, 0, &ref); [...] cbphy->hsiocfg = device_node_to_regmap(to_of_node(ref.fwnode)); [...] cbphy->app_base = devm_platform_ioremap_resource_byname(pdev, "app"); [...] cbphy->cr_base = devm_platform_ioremap_resource_byname(pdev, "core"); The DT parsing logic in the driver is explaining why the PHY driver should do regmap access and to whom should be done. For this reason i am a bit puzzled to what more is needed to explain in the comments and where to add it. Please let me know your view. Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 04-05-20, 17:32, Dilip Kota wrote: > > On 5/4/2020 5:20 PM, Vinod Koul wrote: > > On 04-05-20, 16:26, Dilip Kota wrote: > > > On 5/4/2020 3:29 PM, Vinod Koul wrote: > > > > On 30-04-20, 15:15, Dilip Kota wrote: > > > > > > > > > + u32 mask, u32 val) > > > > > +{ > > > > > + u32 reg_val; > > > > > + > > > > > + reg_val = readl(base + reg); > > > > > + reg_val &= ~mask; > > > > > + reg_val |= FIELD_PREP(mask, val); > > > > > + writel(reg_val, base + reg); > > > > bypassing regmap here... why? > > > It is not regmap address, one of the below two addresses are passed to > > > this > > > function. > > okay, perhaps add a comment somewhere that regmap is not used for this > > base? > I dont see a need of adding a comment, describing don't do regmap here. Driver uses regmap except here, which seems odd hence explanation required for this. > > > > > struct intel_combo_phy { > > > ... > > > void __iomem *app_base; > > > void __iomem *cr_base; > > > ... > > > } > > > > > > > +static int intel_cbphy_calibrate(struct phy *phy) > > > > > +{ > > > > > + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); > > > > > + struct intel_combo_phy *cbphy = iphy->parent; > > > > > + void __iomem *cr_base = cbphy->cr_base; > > > > > + int val, ret, id; > > > > > + > > > > > + if (cbphy->phy_mode != PHY_XPCS_MODE) > > > > > + return 0; > > > > > + > > > > > + id = PHY_ID(iphy); > > > > > + > > > > > + /* trigger auto RX adaptation */ > > > > > + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, > > > > > id), > > > > > +ADAPT_REQ_MSK, 3); > > > > > + /* Wait RX adaptation to finish */ > > > > > + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, > > > > > id), > > > > > + val, val & RX_ADAPT_ACK_BIT, 10, 5000); > > > > > + if (ret) > > > > > + dev_err(cbphy->dev, "RX Adaptation failed!\n"); > > > > you want to continue her and not return error? > > > Next step is stopping the Adaptation, it should be done in both error and > > > success case. > > Again documenting this helps, pls add some comments on this behaviour > Comments are already in place, mentioning Start and Stop of Rx Adaptation. > And Stop is being is done as Start is triggered, so not needed to mention > error and success. Ok -- ~Vinod
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/4/2020 5:20 PM, Vinod Koul wrote: On 04-05-20, 16:26, Dilip Kota wrote: On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. okay, perhaps add a comment somewhere that regmap is not used for this base? I dont see a need of adding a comment, describing don't do regmap here. struct intel_combo_phy { ... void __iomem *app_base; void __iomem *cr_base; ... } +static int intel_cbphy_calibrate(struct phy *phy) +{ + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); + struct intel_combo_phy *cbphy = iphy->parent; + void __iomem *cr_base = cbphy->cr_base; + int val, ret, id; + + if (cbphy->phy_mode != PHY_XPCS_MODE) + return 0; + + id = PHY_ID(iphy); + + /* trigger auto RX adaptation */ + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), + ADAPT_REQ_MSK, 3); + /* Wait RX adaptation to finish */ + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), +val, val & RX_ADAPT_ACK_BIT, 10, 5000); + if (ret) + dev_err(cbphy->dev, "RX Adaptation failed!\n"); you want to continue her and not return error? Next step is stopping the Adaptation, it should be done in both error and success case. Again documenting this helps, pls add some comments on this behaviour Comments are already in place, mentioning Start and Stop of Rx Adaptation. And Stop is being is done as Start is triggered, so not needed to mention error and success. Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 04-05-20, 16:26, Dilip Kota wrote: > > On 5/4/2020 3:29 PM, Vinod Koul wrote: > > On 30-04-20, 15:15, Dilip Kota wrote: > > > > > +enum { > > > + PHY_0, > > > + PHY_1, > > > + PHY_MAX_NUM > > PHY_MAX_NUM = PHY_1? > Driver is using it for no. of PHYs/maximum PHY id. Ok > > > +static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned > > > int reg, > > > + u32 mask, u32 val) > > > +{ > > > + u32 reg_val; > > > + > > > + reg_val = readl(base + reg); > > > + reg_val &= ~mask; > > > + reg_val |= FIELD_PREP(mask, val); > > > + writel(reg_val, base + reg); > > bypassing regmap here... why? > It is not regmap address, one of the below two addresses are passed to this > function. okay, perhaps add a comment somewhere that regmap is not used for this base? > struct intel_combo_phy { > ... > void __iomem *app_base; > void __iomem *cr_base; > ... > } > > > +static int intel_cbphy_calibrate(struct phy *phy) > > > +{ > > > + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); > > > + struct intel_combo_phy *cbphy = iphy->parent; > > > + void __iomem *cr_base = cbphy->cr_base; > > > + int val, ret, id; > > > + > > > + if (cbphy->phy_mode != PHY_XPCS_MODE) > > > + return 0; > > > + > > > + id = PHY_ID(iphy); > > > + > > > + /* trigger auto RX adaptation */ > > > + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), > > > +ADAPT_REQ_MSK, 3); > > > + /* Wait RX adaptation to finish */ > > > + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), > > > + val, val & RX_ADAPT_ACK_BIT, 10, 5000); > > > + if (ret) > > > + dev_err(cbphy->dev, "RX Adaptation failed!\n"); > > you want to continue her and not return error? > > Next step is stopping the Adaptation, it should be done in both error and > success case. Again documenting this helps, pls add some comments on this behaviour -- ~Vinod
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 5/4/2020 3:29 PM, Vinod Koul wrote: On 30-04-20, 15:15, Dilip Kota wrote: +enum { + PHY_0, + PHY_1, + PHY_MAX_NUM PHY_MAX_NUM = PHY_1? Driver is using it for no. of PHYs/maximum PHY id. Code snippets: struct intel_combo_phy { ... struct reset_control *phy_rst; struct reset_control *core_rst; struct intel_cbphy_iphy iphy[PHY_MAX_NUM]; ... } static int intel_cbphy_create(struct intel_combo_phy *cbphy) { struct phy_provider *phy_provider; struct device *dev = cbphy->dev; struct intel_cbphy_iphy *iphy; int i; for (i = 0; i < PHY_MAX_NUM; i++) { ... +static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int reg, + u32 mask, u32 val) +{ + u32 reg_val; + + reg_val = readl(base + reg); + reg_val &= ~mask; + reg_val |= FIELD_PREP(mask, val); + writel(reg_val, base + reg); bypassing regmap here... why? It is not regmap address, one of the below two addresses are passed to this function. struct intel_combo_phy { ... void __iomem *app_base; void __iomem *cr_base; ... } +static int intel_cbphy_calibrate(struct phy *phy) +{ + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); + struct intel_combo_phy *cbphy = iphy->parent; + void __iomem *cr_base = cbphy->cr_base; + int val, ret, id; + + if (cbphy->phy_mode != PHY_XPCS_MODE) + return 0; + + id = PHY_ID(iphy); + + /* trigger auto RX adaptation */ + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), + ADAPT_REQ_MSK, 3); + /* Wait RX adaptation to finish */ + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), +val, val & RX_ADAPT_ACK_BIT, 10, 5000); + if (ret) + dev_err(cbphy->dev, "RX Adaptation failed!\n"); you want to continue her and not return error? Next step is stopping the Adaptation, it should be done in both error and success case. Regards, Dilip
Re: [PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
On 30-04-20, 15:15, Dilip Kota wrote: > +enum { > + PHY_0, > + PHY_1, > + PHY_MAX_NUM PHY_MAX_NUM = PHY_1? > +static inline void combo_phy_w32_off_mask(void __iomem *base, unsigned int > reg, > + u32 mask, u32 val) > +{ > + u32 reg_val; > + > + reg_val = readl(base + reg); > + reg_val &= ~mask; > + reg_val |= FIELD_PREP(mask, val); > + writel(reg_val, base + reg); bypassing regmap here... why? > +static int intel_cbphy_calibrate(struct phy *phy) > +{ > + struct intel_cbphy_iphy *iphy = phy_get_drvdata(phy); > + struct intel_combo_phy *cbphy = iphy->parent; > + void __iomem *cr_base = cbphy->cr_base; > + int val, ret, id; > + > + if (cbphy->phy_mode != PHY_XPCS_MODE) > + return 0; > + > + id = PHY_ID(iphy); > + > + /* trigger auto RX adaptation */ > + combo_phy_w32_off_mask(cr_base, CR_ADDR(PCS_XF_ATE_OVRD_IN_2, id), > +ADAPT_REQ_MSK, 3); > + /* Wait RX adaptation to finish */ > + ret = readl_poll_timeout(cr_base + CR_ADDR(PCS_XF_RX_ADAPT_ACK, id), > + val, val & RX_ADAPT_ACK_BIT, 10, 5000); > + if (ret) > + dev_err(cbphy->dev, "RX Adaptation failed!\n"); you want to continue her and not return error? -- ~Vinod
[PATCH v7 3/3] phy: intel: Add driver support for ComboPhy
ComboPhy subsystem provides PHYs for various controllers like PCIe, SATA and EMAC. Signed-off-by: Dilip Kota --- Changes on v7: Use device_node_to_regmap instead of fwnode_to_regmap Changes on v6: No changes Changes on v5: Add changes as per inputs from Andy and Rob: DT node uses phy-mode values as defined in "include/dt-bindings/phy/phy.h", add changes to handle it. ComboPhy no longer has children nodes, and children node properties(reset) moved to parent node, so do the code changes accordingly. Add _xlate() function to pass the appropriate phy handle. Fix couple of nitpicks. Changes on v4: Address review comments Remove dependency on OF config Update copyright to 2019-2020 Define register macro PAD_DIS_CFG instead of const variable inside function. Improve the error prints, and error returns. Call put_device(dev), for get_dev_from_fwnode() Move platform_set_drvdata() at the end of the probe(). Correct alignment in phy_ops intel_cbphy_ops. Correct commented lines with proper vocabulary and punctuation. Add/remove commas for the required constant arrays and enums. Remove in driver: linux/kernel.h, not required macros: PCIE_PHY_MPLLA_CTRL, PCIE_PHY_MPLLB_CTRL temp variable u32 prop; Change function names: intel_cbphy_iphy_dt_parse() -> intel_cbphy_iphy_fwnode_parse() intel_cbphy_dt_sanity_check() -> intel_cbphy_sanity_check() intel_cbphy_dt_parse() -> intel_cbphy_fwnode_parse() Changes on v3: Remove intel_iphy_names Remove struct phy in struct intel_cbphy_iphy Imporve if conditions logic Use fwnode_to_regmap() Call devm_of_platform_populate() to populate child nodes Fix reset sequence during phy_init Add SoC specific compatible "intel,combophy-lgm" Add description for enums Remove default case in switch {} intel_cbphy_set_mode() as it never happens. Use mutex_lock to synchronise combophy initialization across two phys. Change init_cnt to u32 datatype as it is within mutex lock. Correct error handling of fwnode_property_read_u32_array(fwnode, "intel,phy-mode", ...) drivers/phy/intel/Kconfig | 14 + drivers/phy/intel/Makefile | 1 + drivers/phy/intel/phy-intel-combo.c | 627 3 files changed, 642 insertions(+) create mode 100644 drivers/phy/intel/phy-intel-combo.c diff --git a/drivers/phy/intel/Kconfig b/drivers/phy/intel/Kconfig index 4ea6a8897cd7..3b40eb7b4fb4 100644 --- a/drivers/phy/intel/Kconfig +++ b/drivers/phy/intel/Kconfig @@ -2,6 +2,20 @@ # # Phy drivers for Intel Lightning Mountain(LGM) platform # +config PHY_INTEL_COMBO + bool "Intel ComboPHY driver" + depends on X86 || COMPILE_TEST + depends on OF && HAS_IOMEM + select MFD_SYSCON + select GENERIC_PHY + select REGMAP + help + Enable this to support Intel ComboPhy. + + This driver configures ComboPhy subsystem on Intel gateway + chipsets which provides PHYs for various controllers, EMAC, + SATA and PCIe. + config PHY_INTEL_EMMC tristate "Intel EMMC PHY driver" select GENERIC_PHY diff --git a/drivers/phy/intel/Makefile b/drivers/phy/intel/Makefile index 6b876a75599d..233d530dadde 100644 --- a/drivers/phy/intel/Makefile +++ b/drivers/phy/intel/Makefile @@ -1,2 +1,3 @@ # SPDX-License-Identifier: GPL-2.0 +obj-$(CONFIG_PHY_INTEL_COMBO) += phy-intel-combo.o obj-$(CONFIG_PHY_INTEL_EMMC)+= phy-intel-emmc.o diff --git a/drivers/phy/intel/phy-intel-combo.c b/drivers/phy/intel/phy-intel-combo.c new file mode 100644 index ..04ad595e21e4 --- /dev/null +++ b/drivers/phy/intel/phy-intel-combo.c @@ -0,0 +1,627 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Intel Combo-PHY driver + * + * Copyright (C) 2019-2020 Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +#define PCIE_PHY_GEN_CTRL 0x00 +#define PCIE_PHY_CLK_PAD BIT(17) + +#define PAD_DIS_CFG0x174 + +#define PCS_XF_ATE_OVRD_IN_2 0x3008 +#define ADAPT_REQ_MSK GENMASK(5, 4) + +#define PCS_XF_RX_ADAPT_ACK0x3010 +#define RX_ADAPT_ACK_BIT BIT(0) + +#define CR_ADDR(addr, lane)(((addr) + (lane) * 0x100) << 2) +#define REG_COMBO_MODE(x) ((x) * 0x200) +#define REG_CLK_DISABLE(x) ((x) * 0x200 + 0x124) + +#define COMBO_PHY_ID(x)((x)->parent->id) +#define PHY_ID(x) ((x)->id) + +#define CLK_100MHZ 1 +#define CLK_156_25MHZ 15625 + +static const unsigned long intel_iphy_clk_rates[] = { + CLK_100MHZ, CLK_156_25MHZ, CLK_100MHZ, +}; + +enum { + PHY_0, + PHY_1, + PHY_MAX_NUM +}; + +/* + * Clock Register bit fields to enable clocks + * for ComboPhy according to the mode. + */ +enum intel_phy_mode { + PHY_PCIE_MODE = 0, + PHY_XPCS_MODE, + PHY_SATA_MODE,