diff mbox series

[v4,09/13] PCI: dw-rockchip: Refactor the driver to prepare for EP mode

Message ID 20240529-rockchip-pcie-ep-v1-v4-9-3dc00fe21a78@kernel.org (mailing list archive)
State New
Headers show
Series PCI: dw-rockchip: Add endpoint mode support | expand

Commit Message

Niklas Cassel May 29, 2024, 8:29 a.m. UTC 
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(-)

Comments

Manivannan Sadhasivam June 5, 2024, 8:06 a.m. UTC | #1 
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
Niklas Cassel June 5, 2024, 5:57 p.m. UTC | #2 
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
Manivannan Sadhasivam June 6, 2024, 6:27 a.m. UTC | #3 
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 mbox series

Patch

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,
+	},
 	{},
 };