| Message ID | 20240415081135.3814373-5-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Manivannan Sadhasivam |
| Headers | show |
| Series | PCI: rcar-gen4: Add R-Car V4H support | expand |
On Mon, Apr 15, 2024 at 05:11:32PM +0900, Yoshihiro Shimoda wrote: > In other to support future SoCs such as r8a779g0 and r8a779h0 that > require different initialization settings, let's introduce SoC > specific driver data with the initial member being the device mode. > No functional change. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> One nitpick below. With that addressed, Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-rcar-gen4.c | 30 ++++++++++++++------- > 1 file changed, 21 insertions(+), 9 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > index 0be760ed420b..3da0a844e1b6 100644 > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > @@ -48,11 +48,15 @@ > #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET 0x1000 > #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET 0x800 > > +struct rcar_gen4_pcie_drvdata { > + enum dw_pcie_device_mode mode; > +}; > + > struct rcar_gen4_pcie { > struct dw_pcie dw; > void __iomem *base; > struct platform_device *pdev; > - enum dw_pcie_device_mode mode; > + const struct rcar_gen4_pcie_drvdata *drvdata; > }; > #define to_rcar_gen4_pcie(_dw) container_of(_dw, struct rcar_gen4_pcie, dw) > > @@ -137,7 +141,7 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw) > * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained. > * So, this needs remaining times for up to PCIe Gen4 if RC mode. > */ > - if (changes && rcar->mode == DW_PCIE_RC_TYPE) > + if (changes && rcar->drvdata->mode == DW_PCIE_RC_TYPE) > changes--; > > for (i = 0; i < changes; i++) { > @@ -172,9 +176,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar) > reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc); > > val = readl(rcar->base + PCIEMSR0); > - if (rcar->mode == DW_PCIE_RC_TYPE) { > + if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) { > val |= DEVICE_TYPE_RC; > - } else if (rcar->mode == DW_PCIE_EP_TYPE) { > + } else if (rcar->drvdata->mode == DW_PCIE_EP_TYPE) { > val |= DEVICE_TYPE_EP; > } else { > ret = -EINVAL; > @@ -437,9 +441,9 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > /* Common */ > static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar) > { > - rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev); > + rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev); Even though rcar->drvdata won't be NULL, the lack of NULL check will cause folks to send fixup patch later. So please add a NULL check here itself. - Mani > > - switch (rcar->mode) { > + switch (rcar->drvdata->mode) { > case DW_PCIE_RC_TYPE: > return rcar_gen4_add_dw_pcie_rp(rcar); > case DW_PCIE_EP_TYPE: > @@ -480,7 +484,7 @@ static int rcar_gen4_pcie_probe(struct platform_device *pdev) > > static void rcar_gen4_remove_dw_pcie(struct rcar_gen4_pcie *rcar) > { > - switch (rcar->mode) { > + switch (rcar->drvdata->mode) { > case DW_PCIE_RC_TYPE: > rcar_gen4_remove_dw_pcie_rp(rcar); > break; > @@ -500,14 +504,22 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev) > rcar_gen4_pcie_unprepare(rcar); > } > > +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = { > + .mode = DW_PCIE_RC_TYPE, > +}; > + > +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = { > + .mode = DW_PCIE_EP_TYPE, > +}; > + > static const struct of_device_id rcar_gen4_pcie_of_match[] = { > { > .compatible = "renesas,rcar-gen4-pcie", > - .data = (void *)DW_PCIE_RC_TYPE, > + .data = &drvdata_rcar_gen4_pcie, > }, > { > .compatible = "renesas,rcar-gen4-pcie-ep", > - .data = (void *)DW_PCIE_EP_TYPE, > + .data = &drvdata_rcar_gen4_pcie_ep, > }, > {}, > }; > -- > 2.25.1 > >
Hi Mani, On Sat, May 11, 2024 at 9:37 AM Manivannan Sadhasivam <mani@kernel.org> wrote: > On Mon, Apr 15, 2024 at 05:11:32PM +0900, Yoshihiro Shimoda wrote: > > In other to support future SoCs such as r8a779g0 and r8a779h0 that > > require different initialization settings, let's introduce SoC > > specific driver data with the initial member being the device mode. > > No functional change. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > One nitpick below. With that addressed, > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > > @@ -437,9 +441,9 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > > /* Common */ > > static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar) > > { > > - rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev); > > + rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev); > > Even though rcar->drvdata won't be NULL, the lack of NULL check will cause > folks to send fixup patch later. So please add a NULL check here itself. I tend to disagree: this can never return NULL. Less than half of the callers of of_device_get_match_data() check for a NULL pointer, and many of them do so because they are used both with and without DT. Gr{oetje,eeting}s, Geert
On Mon, May 13, 2024 at 11:20:56AM +0200, Geert Uytterhoeven wrote: > Hi Mani, > > On Sat, May 11, 2024 at 9:37 AM Manivannan Sadhasivam <mani@kernel.org> wrote: > > On Mon, Apr 15, 2024 at 05:11:32PM +0900, Yoshihiro Shimoda wrote: > > > In other to support future SoCs such as r8a779g0 and r8a779h0 that > > > require different initialization settings, let's introduce SoC > > > specific driver data with the initial member being the device mode. > > > No functional change. > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > One nitpick below. With that addressed, > > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c > > > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c > > > > @@ -437,9 +441,9 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar) > > > /* Common */ > > > static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar) > > > { > > > - rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev); > > > + rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev); > > > > Even though rcar->drvdata won't be NULL, the lack of NULL check will cause > > folks to send fixup patch later. So please add a NULL check here itself. > > I tend to disagree: this can never return NULL. > Less than half of the callers of of_device_get_match_data() check for > a NULL pointer, and many of them do so because they are used both > with and without DT. > As I said, there is no way it can be NULL. But folks tend to send the 'fix' patches as the automated tools report these kind of non-existent issues. And I agree that we can decide not to accept those patches, but I just wanted to avoid that noise. As a maintainer, I try to avoid seeing those kind of patches if there is a way we can avoid it. But no strong preference here. - Mani
diff --git a/drivers/pci/controller/dwc/pcie-rcar-gen4.c b/drivers/pci/controller/dwc/pcie-rcar-gen4.c index 0be760ed420b..3da0a844e1b6 100644 --- a/drivers/pci/controller/dwc/pcie-rcar-gen4.c +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c @@ -48,11 +48,15 @@ #define RCAR_GEN4_PCIE_EP_FUNC_DBI_OFFSET 0x1000 #define RCAR_GEN4_PCIE_EP_FUNC_DBI2_OFFSET 0x800 +struct rcar_gen4_pcie_drvdata { + enum dw_pcie_device_mode mode; +}; + struct rcar_gen4_pcie { struct dw_pcie dw; void __iomem *base; struct platform_device *pdev; - enum dw_pcie_device_mode mode; + const struct rcar_gen4_pcie_drvdata *drvdata; }; #define to_rcar_gen4_pcie(_dw) container_of(_dw, struct rcar_gen4_pcie, dw) @@ -137,7 +141,7 @@ static int rcar_gen4_pcie_start_link(struct dw_pcie *dw) * Since dw_pcie_setup_rc() sets it once, PCIe Gen2 will be trained. * So, this needs remaining times for up to PCIe Gen4 if RC mode. */ - if (changes && rcar->mode == DW_PCIE_RC_TYPE) + if (changes && rcar->drvdata->mode == DW_PCIE_RC_TYPE) changes--; for (i = 0; i < changes; i++) { @@ -172,9 +176,9 @@ static int rcar_gen4_pcie_common_init(struct rcar_gen4_pcie *rcar) reset_control_assert(dw->core_rsts[DW_PCIE_PWR_RST].rstc); val = readl(rcar->base + PCIEMSR0); - if (rcar->mode == DW_PCIE_RC_TYPE) { + if (rcar->drvdata->mode == DW_PCIE_RC_TYPE) { val |= DEVICE_TYPE_RC; - } else if (rcar->mode == DW_PCIE_EP_TYPE) { + } else if (rcar->drvdata->mode == DW_PCIE_EP_TYPE) { val |= DEVICE_TYPE_EP; } else { ret = -EINVAL; @@ -437,9 +441,9 @@ static void rcar_gen4_remove_dw_pcie_ep(struct rcar_gen4_pcie *rcar) /* Common */ static int rcar_gen4_add_dw_pcie(struct rcar_gen4_pcie *rcar) { - rcar->mode = (uintptr_t)of_device_get_match_data(&rcar->pdev->dev); + rcar->drvdata = of_device_get_match_data(&rcar->pdev->dev); - switch (rcar->mode) { + switch (rcar->drvdata->mode) { case DW_PCIE_RC_TYPE: return rcar_gen4_add_dw_pcie_rp(rcar); case DW_PCIE_EP_TYPE: @@ -480,7 +484,7 @@ static int rcar_gen4_pcie_probe(struct platform_device *pdev) static void rcar_gen4_remove_dw_pcie(struct rcar_gen4_pcie *rcar) { - switch (rcar->mode) { + switch (rcar->drvdata->mode) { case DW_PCIE_RC_TYPE: rcar_gen4_remove_dw_pcie_rp(rcar); break; @@ -500,14 +504,22 @@ static void rcar_gen4_pcie_remove(struct platform_device *pdev) rcar_gen4_pcie_unprepare(rcar); } +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie = { + .mode = DW_PCIE_RC_TYPE, +}; + +static struct rcar_gen4_pcie_drvdata drvdata_rcar_gen4_pcie_ep = { + .mode = DW_PCIE_EP_TYPE, +}; + static const struct of_device_id rcar_gen4_pcie_of_match[] = { { .compatible = "renesas,rcar-gen4-pcie", - .data = (void *)DW_PCIE_RC_TYPE, + .data = &drvdata_rcar_gen4_pcie, }, { .compatible = "renesas,rcar-gen4-pcie-ep", - .data = (void *)DW_PCIE_EP_TYPE, + .data = &drvdata_rcar_gen4_pcie_ep, }, {}, };
In other to support future SoCs such as r8a779g0 and r8a779h0 that require different initialization settings, let's introduce SoC specific driver data with the initial member being the device mode. No functional change. Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- drivers/pci/controller/dwc/pcie-rcar-gen4.c | 30 ++++++++++++++------- 1 file changed, 21 insertions(+), 9 deletions(-)