| Message ID | 20240529-rockchip-pcie-ep-v1-v4-9-3dc00fe21a78@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI: dw-rockchip: Add endpoint mode support | expand |
On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote: > This refactors the driver to prepare for EP mode. > Add of-match data to the existing compatible, and explicitly define it as > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up > commit in a much less intrusive way, which makes the follup-up commit much > easier to review. > > No functional change intended. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Few nitpicks below. With those addressed, Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-dw-rockchip.c | 84 +++++++++++++++++++-------- > 1 file changed, 60 insertions(+), 24 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > index 1380e3a5284b..e133511692af 100644 > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c > @@ -49,15 +49,20 @@ > #define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0) > > struct rockchip_pcie { > - struct dw_pcie pci; > - void __iomem *apb_base; > - struct phy *phy; > - struct clk_bulk_data *clks; > - unsigned int clk_cnt; > - struct reset_control *rst; > - struct gpio_desc *rst_gpio; > - struct regulator *vpcie3v3; > - struct irq_domain *irq_domain; > + struct dw_pcie pci; > + void __iomem *apb_base; > + struct phy *phy; > + struct clk_bulk_data *clks; > + unsigned int clk_cnt; > + struct reset_control *rst; > + struct gpio_desc *rst_gpio; > + struct regulator *vpcie3v3; > + struct irq_domain *irq_domain; > + const struct rockchip_pcie_of_data *data; I prefer to avoid aligning the struct members just for this reason. Once you add a member with a bigger name, then you need to align others also. Please just use space. > +}; > + > +struct rockchip_pcie_of_data { > + enum dw_pcie_device_mode mode; > }; > > static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg) > @@ -195,7 +200,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > struct dw_pcie *pci = to_dw_pcie_from_pp(pp); > struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); > struct device *dev = rockchip->pci.dev; > - u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > int irq, ret; > > irq = of_irq_get_byname(dev->of_node, "legacy"); > @@ -209,12 +213,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) > irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler, > rockchip); > > - /* LTSSM enable control mode */ > - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > - > - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > - PCIE_CLIENT_GENERAL_CONTROL); > - > return 0; > } > > @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = { > .start_link = rockchip_pcie_start_link, > }; > > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip) > +{ > + struct dw_pcie_rp *pp; > + u32 val; > + > + /* LTSSM enable control mode */ > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > + > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > + PCIE_CLIENT_GENERAL_CONTROL); > + > + pp = &rockchip->pci.pp; > + pp->ops = &rockchip_pcie_host_ops; > + > + return dw_pcie_host_init(pp); > +} > + > static int rockchip_pcie_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct rockchip_pcie *rockchip; > - struct dw_pcie_rp *pp; > + const struct rockchip_pcie_of_data *data; > int ret; > > + data = of_device_get_match_data(dev); > + if (!data) > + return -EINVAL; -ENODATA? > + > rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); > if (!rockchip) > return -ENOMEM; > @@ -309,9 +329,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > > rockchip->pci.dev = dev; > rockchip->pci.ops = &dw_pcie_ops; > - > - pp = &rockchip->pci.pp; > - pp->ops = &rockchip_pcie_host_ops; > + rockchip->data = data; > > ret = rockchip_pcie_resource_get(pdev, rockchip); > if (ret) > @@ -347,10 +365,21 @@ static int rockchip_pcie_probe(struct platform_device *pdev) > if (ret) > goto deinit_phy; > > - ret = dw_pcie_host_init(pp); > - if (!ret) > - return 0; Thanks a lot for getting rid of this ugly piece of code! - Mani
On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote: > On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote: > > This refactors the driver to prepare for EP mode. > > Add of-match data to the existing compatible, and explicitly define it as > > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up > > commit in a much less intrusive way, which makes the follup-up commit much > > easier to review. > > > > No functional change intended. > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > Few nitpicks below. With those addressed, > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- (snip) > > @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = { > > .start_link = rockchip_pcie_start_link, > > }; > > > > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip) > > +{ > > + struct dw_pcie_rp *pp; > > + u32 val; > > + > > + /* LTSSM enable control mode */ > > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > > + > > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > > + PCIE_CLIENT_GENERAL_CONTROL); > > + > > + pp = &rockchip->pci.pp; > > + pp->ops = &rockchip_pcie_host_ops; > > + > > + return dw_pcie_host_init(pp); > > +} > > + > > static int rockchip_pcie_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct rockchip_pcie *rockchip; > > - struct dw_pcie_rp *pp; > > + const struct rockchip_pcie_of_data *data; > > int ret; > > > > + data = of_device_get_match_data(dev); > > + if (!data) > > + return -EINVAL; > > -ENODATA? -EINVAL seems to be most common: $ git grep -A 5 of_device_get_match_data drivers/pci/ Kind regards, Niklas
On Wed, Jun 05, 2024 at 07:57:12PM +0200, Niklas Cassel wrote: > On Wed, Jun 05, 2024 at 01:36:40PM +0530, Manivannan Sadhasivam wrote: > > On Wed, May 29, 2024 at 10:29:03AM +0200, Niklas Cassel wrote: > > > This refactors the driver to prepare for EP mode. > > > Add of-match data to the existing compatible, and explicitly define it as > > > DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up > > > commit in a much less intrusive way, which makes the follup-up commit much > > > easier to review. > > > > > > No functional change intended. > > > > > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > > > > Few nitpicks below. With those addressed, > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > --- > > (snip) > > > > @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = { > > > .start_link = rockchip_pcie_start_link, > > > }; > > > > > > +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip) > > > +{ > > > + struct dw_pcie_rp *pp; > > > + u32 val; > > > + > > > + /* LTSSM enable control mode */ > > > + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); > > > + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); > > > + > > > + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, > > > + PCIE_CLIENT_GENERAL_CONTROL); > > > + > > > + pp = &rockchip->pci.pp; > > > + pp->ops = &rockchip_pcie_host_ops; > > > + > > > + return dw_pcie_host_init(pp); > > > +} > > > + > > > static int rockchip_pcie_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > struct rockchip_pcie *rockchip; > > > - struct dw_pcie_rp *pp; > > > + const struct rockchip_pcie_of_data *data; > > > int ret; > > > > > > + data = of_device_get_match_data(dev); > > > + if (!data) > > > + return -EINVAL; > > > > -ENODATA? > > -EINVAL seems to be most common: > $ git grep -A 5 of_device_get_match_data drivers/pci/ > Yeah, but we abused -EINVAL a lot ;) Nowadays, I prefer to use more apt error codes. - Mani
diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c index 1380e3a5284b..e133511692af 100644 --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c @@ -49,15 +49,20 @@ #define PCIE_LTSSM_STATUS_MASK GENMASK(5, 0) struct rockchip_pcie { - struct dw_pcie pci; - void __iomem *apb_base; - struct phy *phy; - struct clk_bulk_data *clks; - unsigned int clk_cnt; - struct reset_control *rst; - struct gpio_desc *rst_gpio; - struct regulator *vpcie3v3; - struct irq_domain *irq_domain; + struct dw_pcie pci; + void __iomem *apb_base; + struct phy *phy; + struct clk_bulk_data *clks; + unsigned int clk_cnt; + struct reset_control *rst; + struct gpio_desc *rst_gpio; + struct regulator *vpcie3v3; + struct irq_domain *irq_domain; + const struct rockchip_pcie_of_data *data; +}; + +struct rockchip_pcie_of_data { + enum dw_pcie_device_mode mode; }; static int rockchip_pcie_readl_apb(struct rockchip_pcie *rockchip, u32 reg) @@ -195,7 +200,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) struct dw_pcie *pci = to_dw_pcie_from_pp(pp); struct rockchip_pcie *rockchip = to_rockchip_pcie(pci); struct device *dev = rockchip->pci.dev; - u32 val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); int irq, ret; irq = of_irq_get_byname(dev->of_node, "legacy"); @@ -209,12 +213,6 @@ static int rockchip_pcie_host_init(struct dw_pcie_rp *pp) irq_set_chained_handler_and_data(irq, rockchip_pcie_intx_handler, rockchip); - /* LTSSM enable control mode */ - rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); - - rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, - PCIE_CLIENT_GENERAL_CONTROL); - return 0; } @@ -294,13 +292,35 @@ static const struct dw_pcie_ops dw_pcie_ops = { .start_link = rockchip_pcie_start_link, }; +static int rockchip_pcie_configure_rc(struct rockchip_pcie *rockchip) +{ + struct dw_pcie_rp *pp; + u32 val; + + /* LTSSM enable control mode */ + val = HIWORD_UPDATE_BIT(PCIE_LTSSM_ENABLE_ENHANCE); + rockchip_pcie_writel_apb(rockchip, val, PCIE_CLIENT_HOT_RESET_CTRL); + + rockchip_pcie_writel_apb(rockchip, PCIE_CLIENT_RC_MODE, + PCIE_CLIENT_GENERAL_CONTROL); + + pp = &rockchip->pci.pp; + pp->ops = &rockchip_pcie_host_ops; + + return dw_pcie_host_init(pp); +} + static int rockchip_pcie_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct rockchip_pcie *rockchip; - struct dw_pcie_rp *pp; + const struct rockchip_pcie_of_data *data; int ret; + data = of_device_get_match_data(dev); + if (!data) + return -EINVAL; + rockchip = devm_kzalloc(dev, sizeof(*rockchip), GFP_KERNEL); if (!rockchip) return -ENOMEM; @@ -309,9 +329,7 @@ static int rockchip_pcie_probe(struct platform_device *pdev) rockchip->pci.dev = dev; rockchip->pci.ops = &dw_pcie_ops; - - pp = &rockchip->pci.pp; - pp->ops = &rockchip_pcie_host_ops; + rockchip->data = data; ret = rockchip_pcie_resource_get(pdev, rockchip); if (ret) @@ -347,10 +365,21 @@ static int rockchip_pcie_probe(struct platform_device *pdev) if (ret) goto deinit_phy; - ret = dw_pcie_host_init(pp); - if (!ret) - return 0; + switch (data->mode) { + case DW_PCIE_RC_TYPE: + ret = rockchip_pcie_configure_rc(rockchip); + if (ret) + goto deinit_clk; + break; + default: + dev_err(dev, "INVALID device type %d\n", data->mode); + ret = -EINVAL; + goto deinit_clk; + } + + return 0; +deinit_clk: clk_bulk_disable_unprepare(rockchip->clk_cnt, rockchip->clks); deinit_phy: rockchip_pcie_phy_deinit(rockchip); @@ -361,8 +390,15 @@ static int rockchip_pcie_probe(struct platform_device *pdev) return ret; } +static const struct rockchip_pcie_of_data rockchip_pcie_rc_of_data_rk3568 = { + .mode = DW_PCIE_RC_TYPE, +}; + static const struct of_device_id rockchip_pcie_of_match[] = { - { .compatible = "rockchip,rk3568-pcie", }, + { + .compatible = "rockchip,rk3568-pcie", + .data = &rockchip_pcie_rc_of_data_rk3568, + }, {}, };
This refactors the driver to prepare for EP mode. Add of-match data to the existing compatible, and explicitly define it as DW_PCIE_RC_TYPE. This way, we will be able to add EP mode in a follow-up commit in a much less intrusive way, which makes the follup-up commit much easier to review. No functional change intended. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/pci/controller/dwc/pcie-dw-rockchip.c | 84 +++++++++++++++++++-------- 1 file changed, 60 insertions(+), 24 deletions(-)