| Message ID | 27d28fabbf761e7a38bc6c8371234bf6a6462473.1719668763.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Airoha EN7581 PCIe support | expand |
On Sat, Jun 29, 2024 at 03:51:54PM +0200, Lorenzo Bianconi wrote: > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > PCIe controller driver. > +/* PCIe reset line delay in ms */ > +#define PCIE_RESET_TIME_MS 100 Is this something required by the PCIe base spec, or is it specific to EN7581? Either way it would be nice to have a citation to the spec (revision and section number). If it's generic to PCIe, it should be in drivers/pci/pci.h so other drivers can use the same thing.
> On Sat, Jun 29, 2024 at 03:51:54PM +0200, Lorenzo Bianconi wrote: > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > > PCIe controller driver. > > > +/* PCIe reset line delay in ms */ > > +#define PCIE_RESET_TIME_MS 100 > > Is this something required by the PCIe base spec, or is it specific to > EN7581? Either way it would be nice to have a citation to the spec > (revision and section number). If it's generic to PCIe, it should be > in drivers/pci/pci.h so other drivers can use the same thing. It is just the time needed by the EN7581 reset controller to complete the operation, it is not something PCIe generic (it is something just related to EN7581 SoC). Do you think we should move it in EN7581 reset controller codebase? Regards, Lorenzo
On Sat, 2024-06-29 at 15:51 +0200, Lorenzo Bianconi wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > PCIe controller driver. > > Reviewed-by: AngeloGioacchino Del Regno < > angelogioacchino.delregno@collabora.com> > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/pci/controller/Kconfig | 2 +- > drivers/pci/controller/pcie-mediatek-gen3.c | 108 > +++++++++++++++++++- > 2 files changed, 108 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/Kconfig > b/drivers/pci/controller/Kconfig > index e534c02ee34f..3bd6c9430010 100644 > --- a/drivers/pci/controller/Kconfig > +++ b/drivers/pci/controller/Kconfig > @@ -196,7 +196,7 @@ config PCIE_MEDIATEK > > config PCIE_MEDIATEK_GEN3 > tristate "MediaTek Gen3 PCIe controller" > - depends on ARCH_MEDIATEK || COMPILE_TEST > + depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST > depends on PCI_MSI > help > Adds support for PCIe Gen3 MAC controller for MediaTek SoCs. > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c > b/drivers/pci/controller/pcie-mediatek-gen3.c > index 438a5222d986..f3f76d1bfd4c 100644 > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > @@ -7,6 +7,7 @@ > */ > > #include <linux/clk.h> > +#include <linux/clk-provider.h> > #include <linux/delay.h> > #include <linux/iopoll.h> > #include <linux/irq.h> > @@ -15,6 +16,8 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/msi.h> > +#include <linux/of_device.h> > +#include <linux/of_pci.h> > #include <linux/pci.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > @@ -29,6 +32,12 @@ > #define PCI_CLASS(class) (class << 8) > #define PCIE_RC_MODE BIT(0) > > +#define PCIE_EQ_PRESET_01_REG 0x100 > +#define PCIE_VAL_LN0_DOWNSTREAM GENMASK(6, 0) > +#define PCIE_VAL_LN0_UPSTREAM GENMASK(14, 8) > +#define PCIE_VAL_LN1_DOWNSTREAM GENMASK(22, 16) > +#define PCIE_VAL_LN1_UPSTREAM GENMASK(30, 24) > + > #define PCIE_CFGNUM_REG 0x140 > #define PCIE_CFG_DEVFN(devfn) ((devfn) & GENMASK(7, > 0)) > #define PCIE_CFG_BUS(bus) (((bus) << 8) & GENMASK(15, 8)) > @@ -68,6 +77,14 @@ > #define PCIE_MSI_SET_ENABLE_REG 0x190 > #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, > 0) > > +#define PCIE_PIPE4_PIE8_REG 0x338 > +#define PCIE_K_FINETUNE_MAX GENMASK(5, 0) > +#define PCIE_K_FINETUNE_ERR GENMASK(7, 6) > +#define PCIE_K_PRESET_TO_USE GENMASK(18, 8) > +#define PCIE_K_PHYPARAM_QUERY BIT(19) > +#define PCIE_K_QUERY_TIMEOUT BIT(20) > +#define PCIE_K_PRESET_TO_USE_16G GENMASK(31, 21) > + > #define PCIE_MSI_SET_BASE_REG 0xc00 > #define PCIE_MSI_SET_OFFSET 0x10 > #define PCIE_MSI_SET_STATUS_OFFSET 0x04 > @@ -100,7 +117,13 @@ > #define PCIE_ATR_TLP_TYPE_MEM PCIE_ATR_TLP_TYPE(0) > #define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2) > > -#define MAX_NUM_PHY_RESETS 1 > +#define MAX_NUM_PHY_RESETS 3 > + > +/* EN7581 */ > +/* PCIe-PHY initialization delay in ms */ > +#define PHY_INIT_TIME_MS 30 Since we have already moved the PHY related settings to the PHY driver, can we also move this init time to the PHY driver? Thanks. > +/* PCIe reset line delay in ms */ > +#define PCIE_RESET_TIME_MS 100 > > struct mtk_gen3_pcie; >
On Tue, Jul 02, 2024 at 11:12:21AM +0200, Lorenzo Bianconi wrote: > > On Sat, Jun 29, 2024 at 03:51:54PM +0200, Lorenzo Bianconi wrote: > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > > > PCIe controller driver. > > > > > +/* PCIe reset line delay in ms */ > > > +#define PCIE_RESET_TIME_MS 100 > > > > Is this something required by the PCIe base spec, or is it specific to > > EN7581? Either way it would be nice to have a citation to the spec > > (revision and section number). If it's generic to PCIe, it should be > > in drivers/pci/pci.h so other drivers can use the same thing. > > It is just the time needed by the EN7581 reset controller to > complete the operation, it is not something PCIe generic (it is > something just related to EN7581 SoC). Do you think we should move > it in EN7581 reset controller codebase? I have no opinion about moving it. But it sounds like maybe it should have a less generic name so it doesn't look like a generic PCIe thing. And also a spec citation would be helpful for future maintenance.
> On Tue, Jul 02, 2024 at 11:12:21AM +0200, Lorenzo Bianconi wrote: > > > On Sat, Jun 29, 2024 at 03:51:54PM +0200, Lorenzo Bianconi wrote: > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > > > > PCIe controller driver. > > > > > > > +/* PCIe reset line delay in ms */ > > > > +#define PCIE_RESET_TIME_MS 100 > > > > > > Is this something required by the PCIe base spec, or is it specific to > > > EN7581? Either way it would be nice to have a citation to the spec > > > (revision and section number). If it's generic to PCIe, it should be > > > in drivers/pci/pci.h so other drivers can use the same thing. > > > > It is just the time needed by the EN7581 reset controller to > > complete the operation, it is not something PCIe generic (it is > > something just related to EN7581 SoC). Do you think we should move > > it in EN7581 reset controller codebase? > > I have no opinion about moving it. But it sounds like maybe it should > have a less generic name so it doesn't look like a generic PCIe thing. > And also a spec citation would be helpful for future maintenance. ack, naming is always hard :) I do not have any specific spec for it. It is in the vendor sdk and the Airoha folks confirmed just this time is needed to complete PCIe reset. Regards, Lorenzo
> On Sat, 2024-06-29 at 15:51 +0200, Lorenzo Bianconi wrote: > > > > External email : Please do not click links or open attachments until > > you have verified the sender or the content. > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > > PCIe controller driver. > > > > Reviewed-by: AngeloGioacchino Del Regno < > > angelogioacchino.delregno@collabora.com> > > Tested-by: Zhengping Zhang <zhengping.zhang@airoha.com> > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > drivers/pci/controller/Kconfig | 2 +- > > drivers/pci/controller/pcie-mediatek-gen3.c | 108 > > +++++++++++++++++++- > > 2 files changed, 108 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/Kconfig > > b/drivers/pci/controller/Kconfig > > index e534c02ee34f..3bd6c9430010 100644 > > --- a/drivers/pci/controller/Kconfig > > +++ b/drivers/pci/controller/Kconfig > > @@ -196,7 +196,7 @@ config PCIE_MEDIATEK > > > > config PCIE_MEDIATEK_GEN3 > > tristate "MediaTek Gen3 PCIe controller" > > - depends on ARCH_MEDIATEK || COMPILE_TEST > > + depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST > > depends on PCI_MSI > > help > > Adds support for PCIe Gen3 MAC controller for MediaTek SoCs. > > diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c > > b/drivers/pci/controller/pcie-mediatek-gen3.c > > index 438a5222d986..f3f76d1bfd4c 100644 > > --- a/drivers/pci/controller/pcie-mediatek-gen3.c > > +++ b/drivers/pci/controller/pcie-mediatek-gen3.c > > @@ -7,6 +7,7 @@ > > */ > > > > #include <linux/clk.h> > > +#include <linux/clk-provider.h> > > #include <linux/delay.h> > > #include <linux/iopoll.h> > > #include <linux/irq.h> > > @@ -15,6 +16,8 @@ > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/msi.h> > > +#include <linux/of_device.h> > > +#include <linux/of_pci.h> > > #include <linux/pci.h> > > #include <linux/phy/phy.h> > > #include <linux/platform_device.h> > > @@ -29,6 +32,12 @@ > > #define PCI_CLASS(class) (class << 8) > > #define PCIE_RC_MODE BIT(0) > > > > +#define PCIE_EQ_PRESET_01_REG 0x100 > > +#define PCIE_VAL_LN0_DOWNSTREAM GENMASK(6, 0) > > +#define PCIE_VAL_LN0_UPSTREAM GENMASK(14, 8) > > +#define PCIE_VAL_LN1_DOWNSTREAM GENMASK(22, 16) > > +#define PCIE_VAL_LN1_UPSTREAM GENMASK(30, 24) > > + > > #define PCIE_CFGNUM_REG 0x140 > > #define PCIE_CFG_DEVFN(devfn) ((devfn) & GENMASK(7, > > 0)) > > #define PCIE_CFG_BUS(bus) (((bus) << 8) & GENMASK(15, 8)) > > @@ -68,6 +77,14 @@ > > #define PCIE_MSI_SET_ENABLE_REG 0x190 > > #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, > > 0) > > > > +#define PCIE_PIPE4_PIE8_REG 0x338 > > +#define PCIE_K_FINETUNE_MAX GENMASK(5, 0) > > +#define PCIE_K_FINETUNE_ERR GENMASK(7, 6) > > +#define PCIE_K_PRESET_TO_USE GENMASK(18, 8) > > +#define PCIE_K_PHYPARAM_QUERY BIT(19) > > +#define PCIE_K_QUERY_TIMEOUT BIT(20) > > +#define PCIE_K_PRESET_TO_USE_16G GENMASK(31, 21) > > + > > #define PCIE_MSI_SET_BASE_REG 0xc00 > > #define PCIE_MSI_SET_OFFSET 0x10 > > #define PCIE_MSI_SET_STATUS_OFFSET 0x04 > > @@ -100,7 +117,13 @@ > > #define PCIE_ATR_TLP_TYPE_MEM PCIE_ATR_TLP_TYPE(0) > > #define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2) > > > > -#define MAX_NUM_PHY_RESETS 1 > > +#define MAX_NUM_PHY_RESETS 3 > > + > > +/* EN7581 */ > > +/* PCIe-PHY initialization delay in ms */ > > +#define PHY_INIT_TIME_MS 30 > > Since we have already moved the PHY related settings to the PHY driver, > can we also move this init time to the PHY driver? > > Thanks. ack, I will do in the next revision. Regards, Lorenzo > > > +/* PCIe reset line delay in ms */ > > +#define PCIE_RESET_TIME_MS 100 > > > > struct mtk_gen3_pcie; > >
On Wed, Jul 03, 2024 at 05:26:56PM +0200, Lorenzo Bianconi wrote: > > On Tue, Jul 02, 2024 at 11:12:21AM +0200, Lorenzo Bianconi wrote: > > > > On Sat, Jun 29, 2024 at 03:51:54PM +0200, Lorenzo Bianconi wrote: > > > > > Introduce support for Airoha EN7581 PCIe controller to mediatek-gen3 > > > > > PCIe controller driver. > > > > > > > > > +/* PCIe reset line delay in ms */ > > > > > +#define PCIE_RESET_TIME_MS 100 > > > > > > > > Is this something required by the PCIe base spec, or is it specific to > > > > EN7581? Either way it would be nice to have a citation to the spec > > > > (revision and section number). If it's generic to PCIe, it should be > > > > in drivers/pci/pci.h so other drivers can use the same thing. > > > > > > It is just the time needed by the EN7581 reset controller to > > > complete the operation, it is not something PCIe generic (it is > > > something just related to EN7581 SoC). Do you think we should move > > > it in EN7581 reset controller codebase? > > > > I have no opinion about moving it. But it sounds like maybe it should > > have a less generic name so it doesn't look like a generic PCIe thing. > > And also a spec citation would be helpful for future maintenance. > > ack, naming is always hard :) > I do not have any specific spec for it. It is in the vendor sdk and the Airoha > folks confirmed just this time is needed to complete PCIe reset. Even a note that this came from an SDK from a specific vendor might be useful since it shows that it wasn't picked out of thin air and it gives a hint of who to ask about it.
diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig index e534c02ee34f..3bd6c9430010 100644 --- a/drivers/pci/controller/Kconfig +++ b/drivers/pci/controller/Kconfig @@ -196,7 +196,7 @@ config PCIE_MEDIATEK config PCIE_MEDIATEK_GEN3 tristate "MediaTek Gen3 PCIe controller" - depends on ARCH_MEDIATEK || COMPILE_TEST + depends on ARCH_AIROHA || ARCH_MEDIATEK || COMPILE_TEST depends on PCI_MSI help Adds support for PCIe Gen3 MAC controller for MediaTek SoCs. diff --git a/drivers/pci/controller/pcie-mediatek-gen3.c b/drivers/pci/controller/pcie-mediatek-gen3.c index 438a5222d986..f3f76d1bfd4c 100644 --- a/drivers/pci/controller/pcie-mediatek-gen3.c +++ b/drivers/pci/controller/pcie-mediatek-gen3.c @@ -7,6 +7,7 @@ */ #include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/delay.h> #include <linux/iopoll.h> #include <linux/irq.h> @@ -15,6 +16,8 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/msi.h> +#include <linux/of_device.h> +#include <linux/of_pci.h> #include <linux/pci.h> #include <linux/phy/phy.h> #include <linux/platform_device.h> @@ -29,6 +32,12 @@ #define PCI_CLASS(class) (class << 8) #define PCIE_RC_MODE BIT(0) +#define PCIE_EQ_PRESET_01_REG 0x100 +#define PCIE_VAL_LN0_DOWNSTREAM GENMASK(6, 0) +#define PCIE_VAL_LN0_UPSTREAM GENMASK(14, 8) +#define PCIE_VAL_LN1_DOWNSTREAM GENMASK(22, 16) +#define PCIE_VAL_LN1_UPSTREAM GENMASK(30, 24) + #define PCIE_CFGNUM_REG 0x140 #define PCIE_CFG_DEVFN(devfn) ((devfn) & GENMASK(7, 0)) #define PCIE_CFG_BUS(bus) (((bus) << 8) & GENMASK(15, 8)) @@ -68,6 +77,14 @@ #define PCIE_MSI_SET_ENABLE_REG 0x190 #define PCIE_MSI_SET_ENABLE GENMASK(PCIE_MSI_SET_NUM - 1, 0) +#define PCIE_PIPE4_PIE8_REG 0x338 +#define PCIE_K_FINETUNE_MAX GENMASK(5, 0) +#define PCIE_K_FINETUNE_ERR GENMASK(7, 6) +#define PCIE_K_PRESET_TO_USE GENMASK(18, 8) +#define PCIE_K_PHYPARAM_QUERY BIT(19) +#define PCIE_K_QUERY_TIMEOUT BIT(20) +#define PCIE_K_PRESET_TO_USE_16G GENMASK(31, 21) + #define PCIE_MSI_SET_BASE_REG 0xc00 #define PCIE_MSI_SET_OFFSET 0x10 #define PCIE_MSI_SET_STATUS_OFFSET 0x04 @@ -100,7 +117,13 @@ #define PCIE_ATR_TLP_TYPE_MEM PCIE_ATR_TLP_TYPE(0) #define PCIE_ATR_TLP_TYPE_IO PCIE_ATR_TLP_TYPE(2) -#define MAX_NUM_PHY_RESETS 1 +#define MAX_NUM_PHY_RESETS 3 + +/* EN7581 */ +/* PCIe-PHY initialization delay in ms */ +#define PHY_INIT_TIME_MS 30 +/* PCIe reset line delay in ms */ +#define PCIE_RESET_TIME_MS 100 struct mtk_gen3_pcie; @@ -847,6 +870,78 @@ static int mtk_pcie_parse_port(struct mtk_gen3_pcie *pcie) return 0; } +static int mtk_pcie_en7581_power_up(struct mtk_gen3_pcie *pcie) +{ + struct device *dev = pcie->dev; + int err; + u32 val; + + /* Wait for bulk assert completion in mtk_pcie_setup */ + mdelay(PCIE_RESET_TIME_MS); + + err = phy_init(pcie->phy); + if (err) { + dev_err(dev, "failed to initialize PHY\n"); + return err; + } + mdelay(PHY_INIT_TIME_MS); + + err = phy_power_on(pcie->phy); + if (err) { + dev_err(dev, "failed to power on PHY\n"); + goto err_phy_on; + } + + err = reset_control_bulk_deassert(pcie->soc->phy_resets.num_resets, pcie->phy_resets); + if (err) { + dev_err(dev, "failed to deassert PHYs\n"); + goto err_phy_deassert; + } + mdelay(PCIE_RESET_TIME_MS); + + pm_runtime_enable(dev); + pm_runtime_get_sync(dev); + + err = clk_bulk_prepare(pcie->num_clks, pcie->clks); + if (err) { + dev_err(dev, "failed to prepare clock\n"); + goto err_clk_prepare; + } + + val = FIELD_PREP(PCIE_VAL_LN0_DOWNSTREAM, 0x47) | + FIELD_PREP(PCIE_VAL_LN1_DOWNSTREAM, 0x47) | + FIELD_PREP(PCIE_VAL_LN0_UPSTREAM, 0x41) | + FIELD_PREP(PCIE_VAL_LN1_UPSTREAM, 0x41); + writel_relaxed(val, pcie->base + PCIE_EQ_PRESET_01_REG); + + val = PCIE_K_PHYPARAM_QUERY | PCIE_K_QUERY_TIMEOUT | + FIELD_PREP(PCIE_K_PRESET_TO_USE_16G, 0x80) | + FIELD_PREP(PCIE_K_PRESET_TO_USE, 0x2) | + FIELD_PREP(PCIE_K_FINETUNE_MAX, 0xf); + writel_relaxed(val, pcie->base + PCIE_PIPE4_PIE8_REG); + + err = clk_bulk_enable(pcie->num_clks, pcie->clks); + if (err) { + dev_err(dev, "failed to prepare clock\n"); + goto err_clk_enable; + } + + return 0; + +err_clk_enable: + clk_bulk_unprepare(pcie->num_clks, pcie->clks); +err_clk_prepare: + pm_runtime_put_sync(dev); + pm_runtime_disable(dev); + reset_control_bulk_assert(pcie->soc->phy_resets.num_resets, pcie->phy_resets); +err_phy_deassert: + phy_power_off(pcie->phy); +err_phy_on: + phy_exit(pcie->phy); + + return err; +} + static int mtk_pcie_power_up(struct mtk_gen3_pcie *pcie) { struct device *dev = pcie->dev; @@ -1113,7 +1208,18 @@ static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_mt8192 = { }, }; +static const struct mtk_gen3_pcie_pdata mtk_pcie_soc_en7581 = { + .power_up = mtk_pcie_en7581_power_up, + .phy_resets = { + .id[0] = "phy-lane0", + .id[1] = "phy-lane1", + .id[2] = "phy-lane2", + .num_resets = 3, + }, +}; + static const struct of_device_id mtk_pcie_of_match[] = { + { .compatible = "airoha,en7581-pcie", .data = &mtk_pcie_soc_en7581 }, { .compatible = "mediatek,mt8192-pcie", .data = &mtk_pcie_soc_mt8192 }, {}, };