mbox series

[RFC,v3,0/4] spmi: pmic-arb: Add support for multiple buses

Message ID 20240214-spmi-multi-master-support-v3-0-0bae0ef04faf@linaro.org
Headers
Series spmi: pmic-arb: Add support for multiple buses |

Message

Abel Vesa Feb. 14, 2024, 9:13 p.m. UTC 
  This RFC prepares for and adds support for 2 buses, which is supported
in HW starting with version 7. Until now, none of the currently
supported platforms in upstream have used the second bus. The X1E80100
platform, on the other hand, needs the second bus for the USB2.0 to work
as there are 3 SMB2360 PMICs which provide eUSB2 repeaters and they are
all found on the second bus.

Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
---
Changes in v3:
- Split the change into 3 separate patches. First 2 patches are moving
  apid init and core resources into version specific ops. Third one is
  adding the support for 2 buses and dedicated compatible.
- Added separate bindings patch
- Link to v2: https://lore.kernel.org/r/20240213-spmi-multi-master-support-v2-1-b3b102326906@linaro.org

Changes in v2:
- Reworked it so that it registers a spmi controller for each bus
  rather than relying on the generic framework to pass on the bus
  (master) id.
- Link to v1: https://lore.kernel.org/r/20240207-spmi-multi-master-support-v1-0-ce57f301c7fd@linaro.org

---
Abel Vesa (4):
      dt-bindings: spmi: Add PMIC ARB v7 schema
      spmi: pmic-arb: Make the APID init a version operation
      spmi: pmic-arb: Make core resources acquiring a version operation
      spmi: pmic-arb: Add multi bus support

 .../bindings/spmi/qcom,spmi-pmic-arb-v7.yaml       | 119 +++
 drivers/spmi/spmi-pmic-arb.c                       | 956 +++++++++++++--------
 2 files changed, 712 insertions(+), 363 deletions(-)
---
base-commit: 2c3b09aac00d7835023bbc4473ee06696be64fa8
change-id: 20240207-spmi-multi-master-support-832a704b779b

Best regards,

Comments

Abel Vesa Feb. 14, 2024, 9:36 p.m. UTC | #1 
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