| Message ID | 20240214-spmi-multi-master-support-v3-3-0bae0ef04faf@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | spmi: pmic-arb: Add support for multiple buses | expand |
On 14.02.2024 22:13, Abel Vesa wrote: > Rather than setting up the core, obsrv and chnls in probe by using > version specific conditionals, add a dedicated "get_core_resources" > version specific op and move the acquiring in there. > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > 1 file changed, 78 insertions(+), 33 deletions(-) > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > index 23939c0d225f..489556467a4c 100644 > --- a/drivers/spmi/spmi-pmic-arb.c > +++ b/drivers/spmi/spmi-pmic-arb.c > @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > */ > struct pmic_arb_ver_ops { > const char *ver_str; > + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > return 0; > } > > +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > + void __iomem *core) > +{ > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > + > + pmic_arb->wr_base = core; > + pmic_arb->rd_base = core; > + > + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > + > + return 0; > +} > + > static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > { > u32 *mapping_table; > @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > return apid; > } > > +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > +{ > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > + struct device *dev = &pdev->dev; > + struct resource *res; > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, It's no longer indented to deep, no need to keep such aggressive wrapping > + "obsrvr"); > + pmic_arb->rd_base = devm_ioremap(dev, res->start, > + resource_size(res)); > + if (IS_ERR(pmic_arb->rd_base)) > + return PTR_ERR(pmic_arb->rd_base); > + > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > + "chnls"); > + pmic_arb->wr_base = devm_ioremap(dev, res->start, > + resource_size(res)); > + if (IS_ERR(pmic_arb->wr_base)) > + return PTR_ERR(pmic_arb->wr_base); Could probably make it "devm_platform_get_and_ioremap_resource " Konrad
On 24-02-14 22:18:33, Konrad Dybcio wrote: > On 14.02.2024 22:13, Abel Vesa wrote: > > Rather than setting up the core, obsrv and chnls in probe by using > > version specific conditionals, add a dedicated "get_core_resources" > > version specific op and move the acquiring in there. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > > 1 file changed, 78 insertions(+), 33 deletions(-) > > > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > > index 23939c0d225f..489556467a4c 100644 > > --- a/drivers/spmi/spmi-pmic-arb.c > > +++ b/drivers/spmi/spmi-pmic-arb.c > > @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > > */ > > struct pmic_arb_ver_ops { > > const char *ver_str; > > + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > > int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > > int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > > @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > > return 0; > > } > > > > +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > > + void __iomem *core) > > +{ > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > + > > + pmic_arb->wr_base = core; > > + pmic_arb->rd_base = core; > > + > > + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > > + > > + return 0; > > +} > > + > > static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > > { > > u32 *mapping_table; > > @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > > return apid; > > } > > > > +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > > +{ > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > + struct device *dev = &pdev->dev; > > + struct resource *res; > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > It's no longer indented to deep, no need to keep such aggressive wrapping > The pmic_arb_get_obsrvr_chnls_v2 is used by both: pmic_arb_get_core_resources_v2 pmic_arb_get_core_resources_v7 > > + "obsrvr"); > > + pmic_arb->rd_base = devm_ioremap(dev, res->start, > > + resource_size(res)); > > + if (IS_ERR(pmic_arb->rd_base)) > > + return PTR_ERR(pmic_arb->rd_base); > > + > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > + "chnls"); > > + pmic_arb->wr_base = devm_ioremap(dev, res->start, > > + resource_size(res)); > > + if (IS_ERR(pmic_arb->wr_base)) > > + return PTR_ERR(pmic_arb->wr_base); > > Could probably make it "devm_platform_get_and_ioremap_resource " The reason this needs to stay as is is because of reason explained by the following comment found in probe: /* * Please don't replace this with devm_platform_ioremap_resource() or * devm_ioremap_resource(). These both result in a call to * devm_request_mem_region() which prevents multiple mappings of this * register address range. SoCs with PMIC arbiter v7 may define two * arbiter devices, for the two physical SPMI interfaces, which share * some register address ranges (i.e. "core", "obsrvr", and "chnls"). * Ensure that both devices probe successfully by calling devm_ioremap() * which does not result in a devm_request_mem_region() call. */ Even though, AFAICT, there is no platform that adds a second node for the second bus, currently, in mainline, we should probably allow the "legacy" approach to still work. > > Konrad
On 14.02.2024 22:36, Abel Vesa wrote: > On 24-02-14 22:18:33, Konrad Dybcio wrote: >> On 14.02.2024 22:13, Abel Vesa wrote: >>> Rather than setting up the core, obsrv and chnls in probe by using >>> version specific conditionals, add a dedicated "get_core_resources" >>> version specific op and move the acquiring in there. >>> >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> >>> --- >>> drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- >>> 1 file changed, 78 insertions(+), 33 deletions(-) >>> >>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c >>> index 23939c0d225f..489556467a4c 100644 >>> --- a/drivers/spmi/spmi-pmic-arb.c >>> +++ b/drivers/spmi/spmi-pmic-arb.c >>> @@ -203,6 +203,7 @@ struct spmi_pmic_arb { >>> */ >>> struct pmic_arb_ver_ops { >>> const char *ver_str; >>> + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); >>> int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); >>> int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); >>> /* spmi commands (read_cmd, write_cmd, cmd) functionality */ >>> @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) >>> return 0; >>> } >>> >>> +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, >>> + void __iomem *core) >>> +{ >>> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); >>> + >>> + pmic_arb->wr_base = core; >>> + pmic_arb->rd_base = core; >>> + >>> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; >>> + >>> + return 0; >>> +} >>> + >>> static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) >>> { >>> u32 *mapping_table; >>> @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) >>> return apid; >>> } >>> >>> +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) >>> +{ >>> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); >>> + struct device *dev = &pdev->dev; >>> + struct resource *res; >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >> >> It's no longer indented to deep, no need to keep such aggressive wrapping >> > > The pmic_arb_get_obsrvr_chnls_v2 is used by both: > pmic_arb_get_core_resources_v2 > pmic_arb_get_core_resources_v7 I meant line wrapping > >>> + "obsrvr"); >>> + pmic_arb->rd_base = devm_ioremap(dev, res->start, >>> + resource_size(res)); >>> + if (IS_ERR(pmic_arb->rd_base)) >>> + return PTR_ERR(pmic_arb->rd_base); >>> + >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, >>> + "chnls"); >>> + pmic_arb->wr_base = devm_ioremap(dev, res->start, >>> + resource_size(res)); >>> + if (IS_ERR(pmic_arb->wr_base)) >>> + return PTR_ERR(pmic_arb->wr_base); >> >> Could probably make it "devm_platform_get_and_ioremap_resource " > > The reason this needs to stay as is is because of reason explained by > the following comment found in probe: > > /* > * Please don't replace this with devm_platform_ioremap_resource() or > * devm_ioremap_resource(). These both result in a call to > * devm_request_mem_region() which prevents multiple mappings of this > * register address range. SoCs with PMIC arbiter v7 may define two > * arbiter devices, for the two physical SPMI interfaces, which share > * some register address ranges (i.e. "core", "obsrvr", and "chnls"). > * Ensure that both devices probe successfully by calling devm_ioremap() > * which does not result in a devm_request_mem_region() call. > */ > > Even though, AFAICT, there is no platform that adds a second node for > the second bus, currently, in mainline, we should probably allow the > "legacy" approach to still work. OK right, let's keep it. Konrad
On 24-02-14 22:44:55, Konrad Dybcio wrote: > On 14.02.2024 22:36, Abel Vesa wrote: > > On 24-02-14 22:18:33, Konrad Dybcio wrote: > >> On 14.02.2024 22:13, Abel Vesa wrote: > >>> Rather than setting up the core, obsrv and chnls in probe by using > >>> version specific conditionals, add a dedicated "get_core_resources" > >>> version specific op and move the acquiring in there. > >>> > >>> Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > >>> --- > >>> drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > >>> 1 file changed, 78 insertions(+), 33 deletions(-) > >>> > >>> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > >>> index 23939c0d225f..489556467a4c 100644 > >>> --- a/drivers/spmi/spmi-pmic-arb.c > >>> +++ b/drivers/spmi/spmi-pmic-arb.c > >>> @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > >>> */ > >>> struct pmic_arb_ver_ops { > >>> const char *ver_str; > >>> + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > >>> int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > >>> int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > >>> /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > >>> @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > >>> return 0; > >>> } > >>> > >>> +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > >>> + void __iomem *core) > >>> +{ > >>> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > >>> + > >>> + pmic_arb->wr_base = core; > >>> + pmic_arb->rd_base = core; > >>> + > >>> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > >>> { > >>> u32 *mapping_table; > >>> @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > >>> return apid; > >>> } > >>> > >>> +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > >>> +{ > >>> + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > >>> + struct device *dev = &pdev->dev; > >>> + struct resource *res; > >>> + > >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > >> > >> It's no longer indented to deep, no need to keep such aggressive wrapping > >> > > > > The pmic_arb_get_obsrvr_chnls_v2 is used by both: > > pmic_arb_get_core_resources_v2 > > pmic_arb_get_core_resources_v7 > > I meant line wrapping Oh, ok. Will do. > > > > >>> + "obsrvr"); > >>> + pmic_arb->rd_base = devm_ioremap(dev, res->start, > >>> + resource_size(res)); > >>> + if (IS_ERR(pmic_arb->rd_base)) > >>> + return PTR_ERR(pmic_arb->rd_base); > >>> + > >>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > >>> + "chnls"); > >>> + pmic_arb->wr_base = devm_ioremap(dev, res->start, > >>> + resource_size(res)); > >>> + if (IS_ERR(pmic_arb->wr_base)) > >>> + return PTR_ERR(pmic_arb->wr_base); > >> > >> Could probably make it "devm_platform_get_and_ioremap_resource " > > > > The reason this needs to stay as is is because of reason explained by > > the following comment found in probe: > > > > /* > > * Please don't replace this with devm_platform_ioremap_resource() or > > * devm_ioremap_resource(). These both result in a call to > > * devm_request_mem_region() which prevents multiple mappings of this > > * register address range. SoCs with PMIC arbiter v7 may define two > > * arbiter devices, for the two physical SPMI interfaces, which share > > * some register address ranges (i.e. "core", "obsrvr", and "chnls"). > > * Ensure that both devices probe successfully by calling devm_ioremap() > > * which does not result in a devm_request_mem_region() call. > > */ > > > > Even though, AFAICT, there is no platform that adds a second node for > > the second bus, currently, in mainline, we should probably allow the > > "legacy" approach to still work. > > OK right, let's keep it. > > Konrad
On Wed, 14 Feb 2024 at 23:36, Abel Vesa <abel.vesa@linaro.org> wrote: > > On 24-02-14 22:18:33, Konrad Dybcio wrote: > > On 14.02.2024 22:13, Abel Vesa wrote: > > > Rather than setting up the core, obsrv and chnls in probe by using > > > version specific conditionals, add a dedicated "get_core_resources" > > > version specific op and move the acquiring in there. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > --- > > > drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > > > 1 file changed, 78 insertions(+), 33 deletions(-) > > > > > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > > > index 23939c0d225f..489556467a4c 100644 > > > --- a/drivers/spmi/spmi-pmic-arb.c > > > +++ b/drivers/spmi/spmi-pmic-arb.c > > > @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > > > */ > > > struct pmic_arb_ver_ops { > > > const char *ver_str; > > > + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > > > int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > > > int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > > > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > > > @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > > > return 0; > > > } > > > > > > +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > > > + void __iomem *core) > > > +{ > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > + > > > + pmic_arb->wr_base = core; > > > + pmic_arb->rd_base = core; > > > + > > > + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > > > + > > > + return 0; > > > +} > > > + > > > static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > > > { > > > u32 *mapping_table; > > > @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > > > return apid; > > > } > > > > > > +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > > > +{ > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > + struct device *dev = &pdev->dev; > > > + struct resource *res; > > > + > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > It's no longer indented to deep, no need to keep such aggressive wrapping > > > > The pmic_arb_get_obsrvr_chnls_v2 is used by both: > pmic_arb_get_core_resources_v2 > pmic_arb_get_core_resources_v7 > > > > + "obsrvr"); > > > + pmic_arb->rd_base = devm_ioremap(dev, res->start, > > > + resource_size(res)); > > > + if (IS_ERR(pmic_arb->rd_base)) > > > + return PTR_ERR(pmic_arb->rd_base); > > > + > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > + "chnls"); > > > + pmic_arb->wr_base = devm_ioremap(dev, res->start, > > > + resource_size(res)); > > > + if (IS_ERR(pmic_arb->wr_base)) > > > + return PTR_ERR(pmic_arb->wr_base); > > > > Could probably make it "devm_platform_get_and_ioremap_resource " > > The reason this needs to stay as is is because of reason explained by > the following comment found in probe: > > /* > * Please don't replace this with devm_platform_ioremap_resource() or > * devm_ioremap_resource(). These both result in a call to > * devm_request_mem_region() which prevents multiple mappings of this > * register address range. SoCs with PMIC arbiter v7 may define two > * arbiter devices, for the two physical SPMI interfaces, which share > * some register address ranges (i.e. "core", "obsrvr", and "chnls"). > * Ensure that both devices probe successfully by calling devm_ioremap() > * which does not result in a devm_request_mem_region() call. > */ > > Even though, AFAICT, there is no platform that adds a second node for > the second bus, currently, in mainline, we should probably allow the > "legacy" approach to still work. If there were no DT files which used two SPMI devices, I think we should drop this comment and use existing helpers. We must keep compatibility with the existing DTs, not with the _possible_ device trees. > > > > > Konrad
On 24-02-15 11:30:23, Dmitry Baryshkov wrote: > On Wed, 14 Feb 2024 at 23:36, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > On 24-02-14 22:18:33, Konrad Dybcio wrote: > > > On 14.02.2024 22:13, Abel Vesa wrote: > > > > Rather than setting up the core, obsrv and chnls in probe by using > > > > version specific conditionals, add a dedicated "get_core_resources" > > > > version specific op and move the acquiring in there. > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > --- > > > > drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > > > > 1 file changed, 78 insertions(+), 33 deletions(-) > > > > > > > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > > > > index 23939c0d225f..489556467a4c 100644 > > > > --- a/drivers/spmi/spmi-pmic-arb.c > > > > +++ b/drivers/spmi/spmi-pmic-arb.c > > > > @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > > > > */ > > > > struct pmic_arb_ver_ops { > > > > const char *ver_str; > > > > + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > > > > int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > > > > int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > > > > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > > > > @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > > > > return 0; > > > > } > > > > > > > > +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > > > > + void __iomem *core) > > > > +{ > > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > > + > > > > + pmic_arb->wr_base = core; > > > > + pmic_arb->rd_base = core; > > > > + > > > > + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > > > > + > > > > + return 0; > > > > +} > > > > + > > > > static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > > > > { > > > > u32 *mapping_table; > > > > @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > > > > return apid; > > > > } > > > > > > > > +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > > > > +{ > > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > > + struct device *dev = &pdev->dev; > > > > + struct resource *res; > > > > + > > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > > > It's no longer indented to deep, no need to keep such aggressive wrapping > > > > > > > The pmic_arb_get_obsrvr_chnls_v2 is used by both: > > pmic_arb_get_core_resources_v2 > > pmic_arb_get_core_resources_v7 > > > > > > + "obsrvr"); > > > > + pmic_arb->rd_base = devm_ioremap(dev, res->start, > > > > + resource_size(res)); > > > > + if (IS_ERR(pmic_arb->rd_base)) > > > > + return PTR_ERR(pmic_arb->rd_base); > > > > + > > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > + "chnls"); > > > > + pmic_arb->wr_base = devm_ioremap(dev, res->start, > > > > + resource_size(res)); > > > > + if (IS_ERR(pmic_arb->wr_base)) > > > > + return PTR_ERR(pmic_arb->wr_base); > > > > > > Could probably make it "devm_platform_get_and_ioremap_resource " > > > > The reason this needs to stay as is is because of reason explained by > > the following comment found in probe: > > > > /* > > * Please don't replace this with devm_platform_ioremap_resource() or > > * devm_ioremap_resource(). These both result in a call to > > * devm_request_mem_region() which prevents multiple mappings of this > > * register address range. SoCs with PMIC arbiter v7 may define two > > * arbiter devices, for the two physical SPMI interfaces, which share > > * some register address ranges (i.e. "core", "obsrvr", and "chnls"). > > * Ensure that both devices probe successfully by calling devm_ioremap() > > * which does not result in a devm_request_mem_region() call. > > */ > > > > Even though, AFAICT, there is no platform that adds a second node for > > the second bus, currently, in mainline, we should probably allow the > > "legacy" approach to still work. > > If there were no DT files which used two SPMI devices, I think we > should drop this comment and use existing helpers. We must keep > compatibility with the existing DTs, not with the _possible_ device > trees. Sure. Should I drop the qcom,bus-id from the driver as well? It is optional after all. > > > > > > > > > Konrad > > > > -- > With best wishes > Dmitry
On Thu, 15 Feb 2024 at 15:32, Abel Vesa <abel.vesa@linaro.org> wrote: > > On 24-02-15 11:30:23, Dmitry Baryshkov wrote: > > On Wed, 14 Feb 2024 at 23:36, Abel Vesa <abel.vesa@linaro.org> wrote: > > > > > > On 24-02-14 22:18:33, Konrad Dybcio wrote: > > > > On 14.02.2024 22:13, Abel Vesa wrote: > > > > > Rather than setting up the core, obsrv and chnls in probe by using > > > > > version specific conditionals, add a dedicated "get_core_resources" > > > > > version specific op and move the acquiring in there. > > > > > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > > > --- > > > > > drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- > > > > > 1 file changed, 78 insertions(+), 33 deletions(-) > > > > > > > > > > diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c > > > > > index 23939c0d225f..489556467a4c 100644 > > > > > --- a/drivers/spmi/spmi-pmic-arb.c > > > > > +++ b/drivers/spmi/spmi-pmic-arb.c > > > > > @@ -203,6 +203,7 @@ struct spmi_pmic_arb { > > > > > */ > > > > > struct pmic_arb_ver_ops { > > > > > const char *ver_str; > > > > > + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); > > > > > int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); > > > > > int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); > > > > > /* spmi commands (read_cmd, write_cmd, cmd) functionality */ > > > > > @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) > > > > > return 0; > > > > > } > > > > > > > > > > +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, > > > > > + void __iomem *core) > > > > > +{ > > > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > > > + > > > > > + pmic_arb->wr_base = core; > > > > > + pmic_arb->rd_base = core; > > > > > + > > > > > + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) > > > > > { > > > > > u32 *mapping_table; > > > > > @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) > > > > > return apid; > > > > > } > > > > > > > > > > +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) > > > > > +{ > > > > > + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); > > > > > + struct device *dev = &pdev->dev; > > > > > + struct resource *res; > > > > > + > > > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > > > > > It's no longer indented to deep, no need to keep such aggressive wrapping > > > > > > > > > > The pmic_arb_get_obsrvr_chnls_v2 is used by both: > > > pmic_arb_get_core_resources_v2 > > > pmic_arb_get_core_resources_v7 > > > > > > > > + "obsrvr"); > > > > > + pmic_arb->rd_base = devm_ioremap(dev, res->start, > > > > > + resource_size(res)); > > > > > + if (IS_ERR(pmic_arb->rd_base)) > > > > > + return PTR_ERR(pmic_arb->rd_base); > > > > > + > > > > > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, > > > > > + "chnls"); > > > > > + pmic_arb->wr_base = devm_ioremap(dev, res->start, > > > > > + resource_size(res)); > > > > > + if (IS_ERR(pmic_arb->wr_base)) > > > > > + return PTR_ERR(pmic_arb->wr_base); > > > > > > > > Could probably make it "devm_platform_get_and_ioremap_resource " > > > > > > The reason this needs to stay as is is because of reason explained by > > > the following comment found in probe: > > > > > > /* > > > * Please don't replace this with devm_platform_ioremap_resource() or > > > * devm_ioremap_resource(). These both result in a call to > > > * devm_request_mem_region() which prevents multiple mappings of this > > > * register address range. SoCs with PMIC arbiter v7 may define two > > > * arbiter devices, for the two physical SPMI interfaces, which share > > > * some register address ranges (i.e. "core", "obsrvr", and "chnls"). > > > * Ensure that both devices probe successfully by calling devm_ioremap() > > > * which does not result in a devm_request_mem_region() call. > > > */ > > > > > > Even though, AFAICT, there is no platform that adds a second node for > > > the second bus, currently, in mainline, we should probably allow the > > > "legacy" approach to still work. > > > > If there were no DT files which used two SPMI devices, I think we > > should drop this comment and use existing helpers. We must keep > > compatibility with the existing DTs, not with the _possible_ device > > trees. > > Sure. > > Should I drop the qcom,bus-id from the driver as well? It is optional > after all. I think so. Let's drop it completely. And for the new sub-devices you perfectly know the bus ID.
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index 23939c0d225f..489556467a4c 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -203,6 +203,7 @@ struct spmi_pmic_arb { */ struct pmic_arb_ver_ops { const char *ver_str; + int (*get_core_resources)(struct platform_device *pdev, void __iomem *core); int (*init_apid)(struct spmi_pmic_arb *pmic_arb, int index); int (*ppid_to_apid)(struct spmi_pmic_arb *pmic_arb, u16 ppid); /* spmi commands (read_cmd, write_cmd, cmd) functionality */ @@ -956,6 +957,19 @@ static int pmic_arb_init_apid_min_max(struct spmi_pmic_arb *pmic_arb) return 0; } +static int pmic_arb_get_core_resources_v1(struct platform_device *pdev, + void __iomem *core) +{ + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); + + pmic_arb->wr_base = core; + pmic_arb->rd_base = core; + + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; + + return 0; +} + static int pmic_arb_init_apid_v1(struct spmi_pmic_arb *pmic_arb, int index) { u32 *mapping_table; @@ -1063,6 +1077,41 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) return apid; } +static int pmic_arb_get_obsrvr_chnls_v2(struct platform_device *pdev) +{ + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); + struct device *dev = &pdev->dev; + struct resource *res; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "obsrvr"); + pmic_arb->rd_base = devm_ioremap(dev, res->start, + resource_size(res)); + if (IS_ERR(pmic_arb->rd_base)) + return PTR_ERR(pmic_arb->rd_base); + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, + "chnls"); + pmic_arb->wr_base = devm_ioremap(dev, res->start, + resource_size(res)); + if (IS_ERR(pmic_arb->wr_base)) + return PTR_ERR(pmic_arb->wr_base); + + return 0; +} + +static int pmic_arb_get_core_resources_v2(struct platform_device *pdev, + void __iomem *core) +{ + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); + + pmic_arb->core = core; + + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; + + return pmic_arb_get_obsrvr_chnls_v2(pdev); +} + static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) { u16 apid_valid; @@ -1246,6 +1295,18 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr, return offset; } +static int pmic_arb_get_core_resources_v7(struct platform_device *pdev, + void __iomem *core) +{ + struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev); + + pmic_arb->core = core; + + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7; + + return pmic_arb_get_obsrvr_chnls_v2(pdev); +} + /* * Only v7 supports 2 bus buses. Each bus will get a different apid count, * read from different registers. @@ -1469,6 +1530,7 @@ pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n) static const struct pmic_arb_ver_ops pmic_arb_v1 = { .ver_str = "v1", + .get_core_resources = pmic_arb_get_core_resources_v1, .init_apid = pmic_arb_init_apid_v1, .ppid_to_apid = pmic_arb_ppid_to_apid_v1, .non_data_cmd = pmic_arb_non_data_cmd_v1, @@ -1484,6 +1546,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = { static const struct pmic_arb_ver_ops pmic_arb_v2 = { .ver_str = "v2", + .get_core_resources = pmic_arb_get_core_resources_v2, .init_apid = pmic_arb_init_apid_v1, .ppid_to_apid = pmic_arb_ppid_to_apid_v2, .non_data_cmd = pmic_arb_non_data_cmd_v2, @@ -1499,6 +1562,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = { static const struct pmic_arb_ver_ops pmic_arb_v3 = { .ver_str = "v3", + .get_core_resources = pmic_arb_get_core_resources_v2, .init_apid = pmic_arb_init_apid_v1, .ppid_to_apid = pmic_arb_ppid_to_apid_v2, .non_data_cmd = pmic_arb_non_data_cmd_v2, @@ -1514,6 +1578,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = { static const struct pmic_arb_ver_ops pmic_arb_v5 = { .ver_str = "v5", + .get_core_resources = pmic_arb_get_core_resources_v2, .init_apid = pmic_arb_init_apid_v5, .ppid_to_apid = pmic_arb_ppid_to_apid_v5, .non_data_cmd = pmic_arb_non_data_cmd_v2, @@ -1529,6 +1594,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = { static const struct pmic_arb_ver_ops pmic_arb_v7 = { .ver_str = "v7", + .get_core_resources = pmic_arb_get_core_resources_v7, .init_apid = pmic_arb_init_apid_v7, .ppid_to_apid = pmic_arb_ppid_to_apid_v5, .non_data_cmd = pmic_arb_non_data_cmd_v2, @@ -1584,44 +1650,23 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) hw_ver = readl_relaxed(core + PMIC_ARB_VERSION); - if (hw_ver < PMIC_ARB_VERSION_V2_MIN) { + if (hw_ver < PMIC_ARB_VERSION_V2_MIN) pmic_arb->ver_ops = &pmic_arb_v1; - pmic_arb->wr_base = core; - pmic_arb->rd_base = core; - } else { - pmic_arb->core = core; - - if (hw_ver < PMIC_ARB_VERSION_V3_MIN) - pmic_arb->ver_ops = &pmic_arb_v2; - else if (hw_ver < PMIC_ARB_VERSION_V5_MIN) - pmic_arb->ver_ops = &pmic_arb_v3; - else if (hw_ver < PMIC_ARB_VERSION_V7_MIN) - pmic_arb->ver_ops = &pmic_arb_v5; - else - pmic_arb->ver_ops = &pmic_arb_v7; - - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, - "obsrvr"); - pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start, - resource_size(res)); - if (IS_ERR(pmic_arb->rd_base)) - return PTR_ERR(pmic_arb->rd_base); - - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, - "chnls"); - pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start, - resource_size(res)); - if (IS_ERR(pmic_arb->wr_base)) - return PTR_ERR(pmic_arb->wr_base); - } + else if (hw_ver < PMIC_ARB_VERSION_V3_MIN) + pmic_arb->ver_ops = &pmic_arb_v2; + else if (hw_ver < PMIC_ARB_VERSION_V5_MIN) + pmic_arb->ver_ops = &pmic_arb_v3; + else if (hw_ver < PMIC_ARB_VERSION_V7_MIN) + pmic_arb->ver_ops = &pmic_arb_v5; + else + pmic_arb->ver_ops = &pmic_arb_v7; dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n", pmic_arb->ver_ops->ver_str, hw_ver); - if (hw_ver < PMIC_ARB_VERSION_V7_MIN) - pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; - else - pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7; + err = pmic_arb->ver_ops->get_core_resources(pdev, core); + if (err) + return err; err = pmic_arb->ver_ops->init_apid(pmic_arb, 0); if (err)
Rather than setting up the core, obsrv and chnls in probe by using version specific conditionals, add a dedicated "get_core_resources" version specific op and move the acquiring in there. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/spmi/spmi-pmic-arb.c | 111 ++++++++++++++++++++++++++++++------------- 1 file changed, 78 insertions(+), 33 deletions(-)