| Message ID | 20211201072718.3969011-1-vkoul@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | spmi: pmic-arb: Add support for PMIC v7 | expand |
On 12/2/21 3:06 PM, Stephen Boyd wrote: > Quoting Vinod Koul (2021-11-30 23:27:18) >> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) >> pmic_arb = spmi_controller_get_drvdata(ctrl); >> pmic_arb->spmic = ctrl; >> >> + /* >> + * Don't use devm_ioremap_resource() as the resources are shared in >> + * PMIC v7 onwards, so causing failure when mapping >> + */ >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); >> - core = devm_ioremap_resource(&ctrl->dev, res); >> + core = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); > > What does this mean? We have two nodes in DT that have the same reg > properties? How does that work? PMIC Arbiter v7 has two SPMI bus master interfaces. These are used to communicate with two sets of PMICs. The SPMI interfaces operate independently; however, they share some register address ranges (e.g. one common one is used for APID->PPID mapping). The most straightforward way to handle this is to treat them as two independent top-level DT devices. In this case the "cnfg" address is used in the DT node name as that is unique between the two instances. Here are the DT nodes used downstream on a target with PMIC Arbiter v7: spmi0_bus: qcom,spmi@c42d000 { compatible = "qcom,spmi-pmic-arb"; reg = <0xc42d000 0x4000>, <0xc400000 0x3000>, <0xc500000 0x400000>, <0xc440000 0x80000>, <0xc4c0000 0x10000>; reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "periph_irq"; interrupt-controller; #interrupt-cells = <4>; #address-cells = <2>; #size-cells = <0>; cell-index = <0>; qcom,channel = <0>; qcom,ee = <0>; qcom,bus-id = <0>; }; spmi1_bus: qcom,spmi@c432000 { compatible = "qcom,spmi-pmic-arb"; reg = <0xc432000 0x4000>, <0xc400000 0x3000>, <0xc500000 0x400000>, <0xc440000 0x80000>, <0xc4d0000 0x10000>; reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>; interrupt-names = "periph_irq"; interrupt-controller; #interrupt-cells = <4>; #address-cells = <2>; #size-cells = <0>; cell-index = <0>; qcom,channel = <0>; qcom,ee = <0>; qcom,bus-id = <1>; }; Note the inclusion of a new DT property: "qcom,bus-id". This was defined in a DT binding patch that isn't present in Vinod's submission. Here is its definition: - qcom,bus-id : Specifies which SPMI bus instance to use. This property is only applicable for PMIC arbiter version 7 and beyond. Support values: 0 = primary bus, 1 = secondary bus Assumed to be 0 if unspecified. Take care, David
Quoting David Collins (2021-12-02 15:51:18) > On 12/2/21 3:06 PM, Stephen Boyd wrote: > > Quoting Vinod Koul (2021-11-30 23:27:18) > >> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) > >> pmic_arb = spmi_controller_get_drvdata(ctrl); > >> pmic_arb->spmic = ctrl; > >> > >> + /* > >> + * Don't use devm_ioremap_resource() as the resources are shared in > >> + * PMIC v7 onwards, so causing failure when mapping > >> + */ > >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); > >> - core = devm_ioremap_resource(&ctrl->dev, res); > >> + core = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); > > > > What does this mean? We have two nodes in DT that have the same reg > > properties? How does that work? > > PMIC Arbiter v7 has two SPMI bus master interfaces. These are used to > communicate with two sets of PMICs. The SPMI interfaces operate > independently; however, they share some register address ranges (e.g. > one common one is used for APID->PPID mapping). It looks like the two SPMI busses share the mapping table which is OK because it's read-only but then the 'chnls' and 'obsrvr' regions are also shared. There must be some global lock in place to make sure the two controllers aren't writing to the same register in there? Or are we saved because the mapping table splits the regions between the two busses? > The most > straightforward way to handle this is to treat them as two independent > top-level DT devices. > > In this case the "cnfg" address is used in the DT node name as that is > unique between the two instances. Nice trick. The reg properties aren't supposed to change order though so we shouldn't do that. > > Here are the DT nodes used downstream on a target with PMIC Arbiter v7: > > spmi0_bus: qcom,spmi@c42d000 { > compatible = "qcom,spmi-pmic-arb"; > reg = <0xc42d000 0x4000>, > <0xc400000 0x3000>, > <0xc500000 0x400000>, > <0xc440000 0x80000>, > <0xc4c0000 0x10000>; > reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; > interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "periph_irq"; > interrupt-controller; > #interrupt-cells = <4>; > #address-cells = <2>; > #size-cells = <0>; > cell-index = <0>; > qcom,channel = <0>; > qcom,ee = <0>; > qcom,bus-id = <0>; > }; > > spmi1_bus: qcom,spmi@c432000 { > compatible = "qcom,spmi-pmic-arb"; > reg = <0xc432000 0x4000>, > <0xc400000 0x3000>, > <0xc500000 0x400000>, > <0xc440000 0x80000>, > <0xc4d0000 0x10000>; > reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; > interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "periph_irq"; > interrupt-controller; > #interrupt-cells = <4>; > #address-cells = <2>; > #size-cells = <0>; > cell-index = <0>; > qcom,channel = <0>; > qcom,ee = <0>; > qcom,bus-id = <1>; > }; It feels like we should make a parent node that holds the core, chnls, obsrvr reg properties with a new compatible string and then have two child nodes for each bus. Then the various PMICs can hang off those two different bus nodes. Of course, it needs DT review to make sure nothing else goes wrong. > > Note the inclusion of a new DT property: "qcom,bus-id". This was > defined in a DT binding patch that isn't present in Vinod's submission. > Here is its definition: > > - qcom,bus-id : Specifies which SPMI bus instance to use. This property > is only applicable for PMIC arbiter version 7 and > beyond. > Support values: 0 = primary bus, 1 = secondary bus > Assumed to be 0 if unspecified. > How is it used in the driver? Does something care what is primary and what is secondary?
On 12/2/21 7:13 PM, Stephen Boyd wrote: > Quoting David Collins (2021-12-02 15:51:18) >> On 12/2/21 3:06 PM, Stephen Boyd wrote: >>> Quoting Vinod Koul (2021-11-30 23:27:18) >>>> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) >>>> pmic_arb = spmi_controller_get_drvdata(ctrl); >>>> pmic_arb->spmic = ctrl; >>>> >>>> + /* >>>> + * Don't use devm_ioremap_resource() as the resources are shared in >>>> + * PMIC v7 onwards, so causing failure when mapping >>>> + */ >>>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); >>>> - core = devm_ioremap_resource(&ctrl->dev, res); >>>> + core = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); >>> >>> What does this mean? We have two nodes in DT that have the same reg >>> properties? How does that work? >> >> PMIC Arbiter v7 has two SPMI bus master interfaces. These are used to >> communicate with two sets of PMICs. The SPMI interfaces operate >> independently; however, they share some register address ranges (e.g. >> one common one is used for APID->PPID mapping). > > It looks like the two SPMI busses share the mapping table which is OK > because it's read-only but then the 'chnls' and 'obsrvr' regions are > also shared. There must be some global lock in place to make sure the > two controllers aren't writing to the same register in there? Or are we > saved because the mapping table splits the regions between the two > busses? No global lock is present or needed. The 'chnls' and 'obsvr' region registers are indexed by APID. Both SPMI buses use the same APID numbering space. However, that numbering space is segmented, e.g. APID 0-767 corresponds to SPMI0 bus and APID 768-1023 corresponds to SPMI1 bus. Therefore, there are no circumstances where both buses could be writing to the same set of registers. The base and size of these segments is read from registers based upon the "qcom,bus-id" property values: >>>> + if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) { >>>> + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7; >>>> + >>>> + of_property_read_u32(pdev->dev.of_node, "qcom,bus-id", &pmic_arb->bus_instance); >>>> + if (pmic_arb->bus_instance > 1) { >>>> + err = -EINVAL; >>>> + dev_err(&ctrl->dev, "invalid bus instance: %d\n", pmic_arb->bus_instance); >>>> + goto err_put_ctrl; >>>> + } >>>> + >>>> + if (pmic_arb->bus_instance == 0) { >>>> + pmic_arb->base_apid = 0; >>>> + pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES) & >>>> + PMIC_ARB_FEATURES_PERIPH_MASK; >>>> + } else { >>>> + pmic_arb->base_apid = readl_relaxed(core + PMIC_ARB_FEATURES) & >>>> + PMIC_ARB_FEATURES_PERIPH_MASK; >>>> + pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES1) & >>>> + PMIC_ARB_FEATURES_PERIPH_MASK; >>>> + } >>>> + >>>> + } Note that the specific register(s) that need to be read differ based on the bus instance (0 or 1). The 'base_apid' and 'apid_count' elements are then used in various callback functions when indexing into regions with a common numbering space. >> The most >> straightforward way to handle this is to treat them as two independent >> top-level DT devices. >> >> In this case the "cnfg" address is used in the DT node name as that is >> unique between the two instances. > > Nice trick. The reg properties aren't supposed to change order though so > we shouldn't do that. Why does the ordering of "reg" entries matter? "reg-names" defines what each of the entries corresponds to (independent of the ordering). Is there a policy/requirement specified somewhere about the ordering of "reg" entries when "reg-names" is a required property? >> Here are the DT nodes used downstream on a target with PMIC Arbiter v7: >> >> spmi0_bus: qcom,spmi@c42d000 { >> compatible = "qcom,spmi-pmic-arb"; >> reg = <0xc42d000 0x4000>, >> <0xc400000 0x3000>, >> <0xc500000 0x400000>, >> <0xc440000 0x80000>, >> <0xc4c0000 0x10000>; >> reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; >> interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>; >> interrupt-names = "periph_irq"; >> interrupt-controller; >> #interrupt-cells = <4>; >> #address-cells = <2>; >> #size-cells = <0>; >> cell-index = <0>; >> qcom,channel = <0>; >> qcom,ee = <0>; >> qcom,bus-id = <0>; >> }; >> >> spmi1_bus: qcom,spmi@c432000 { >> compatible = "qcom,spmi-pmic-arb"; >> reg = <0xc432000 0x4000>, >> <0xc400000 0x3000>, >> <0xc500000 0x400000>, >> <0xc440000 0x80000>, >> <0xc4d0000 0x10000>; >> reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; >> interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>; >> interrupt-names = "periph_irq"; >> interrupt-controller; >> #interrupt-cells = <4>; >> #address-cells = <2>; >> #size-cells = <0>; >> cell-index = <0>; >> qcom,channel = <0>; >> qcom,ee = <0>; >> qcom,bus-id = <1>; >> }; > > It feels like we should make a parent node that holds the core, chnls, > obsrvr reg properties with a new compatible string and then have two > child nodes for each bus. Then the various PMICs can hang off those two > different bus nodes. Of course, it needs DT review to make sure nothing > else goes wrong. We considered this alternative DT layout when implementing PMIC arbiter v7 support downstream. The benefit is allowing common register ranges to be specified once instead of in both SPMI bus nodes. However, this approach has several downsides. It results in substantially more complex device tree binding documentation that requires a different layout for SPMI buses for PMIC arbiter v7 (and above) vs early versions even though support can be provided with only a minimal modification (i.e. "qcom,bus-id"). Complexity is also increased inside of the spmi-pmic-arb driver. There, special data structures and logic would be needed to handle the shared vs independent register addresses and data. The SPMI framework currently uses a one-to-one mapping between SPMI buses and struct device pointers. This would not work if the new top-level node represents the true struct device and the per-bus subnodes are not true devices. Also, platform_get_resource_byname() (used in the spmi-pmic-arb probe function) needs a struct platform_device pointer to read address ranges using "reg" + "reg-names". That wouldn't work for the subnodes. I suppose that "compatible" could be specified for the top-level node and the per bus subnodes so that all three get probed as struct devices. However, that makes things even more complicated and convoluted in the driver (and DT). I would prefer to stay with the approach of the two bus instances being specified independently in DT. Note that the clk-imx8qxp-lpcg driver has a similar situation where multiple drivers need to map addresses in the same region. Commit [1] documents the requirement. The details of the problem where devm_platform_ioremap_resource() cannot be used in place of devm_ioremap() were discussed in this thread [2]. >> Note the inclusion of a new DT property: "qcom,bus-id". This was >> defined in a DT binding patch that isn't present in Vinod's submission. >> Here is its definition: >> >> - qcom,bus-id : Specifies which SPMI bus instance to use. This property >> is only applicable for PMIC arbiter version 7 and >> beyond. >> Support values: 0 = primary bus, 1 = secondary bus >> Assumed to be 0 if unspecified. >> > > How is it used in the driver? Does something care what is primary and > what is secondary? See above for the code snippet where "qcom,bus-id" is read. Thanks, David [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=249fce6f3ab0dbd98caa9bc8ea4b50fa709e786a [2]: https://lkml.org/lkml/2019/12/4/260
On 02-12-21, 15:51, David Collins wrote: > On 12/2/21 3:06 PM, Stephen Boyd wrote: > > Quoting Vinod Koul (2021-11-30 23:27:18) > >> @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) > >> pmic_arb = spmi_controller_get_drvdata(ctrl); > >> pmic_arb->spmic = ctrl; > >> > >> + /* > >> + * Don't use devm_ioremap_resource() as the resources are shared in > >> + * PMIC v7 onwards, so causing failure when mapping > >> + */ > >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); > >> - core = devm_ioremap_resource(&ctrl->dev, res); > >> + core = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); > > > > What does this mean? We have two nodes in DT that have the same reg > > properties? How does that work? > > PMIC Arbiter v7 has two SPMI bus master interfaces. These are used to > communicate with two sets of PMICs. The SPMI interfaces operate > independently; however, they share some register address ranges (e.g. > one common one is used for APID->PPID mapping). The most > straightforward way to handle this is to treat them as two independent > top-level DT devices. > > In this case the "cnfg" address is used in the DT node name as that is > unique between the two instances. > > Here are the DT nodes used downstream on a target with PMIC Arbiter v7: > > spmi0_bus: qcom,spmi@c42d000 { > compatible = "qcom,spmi-pmic-arb"; > reg = <0xc42d000 0x4000>, > <0xc400000 0x3000>, > <0xc500000 0x400000>, > <0xc440000 0x80000>, > <0xc4c0000 0x10000>; > reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; > interrupts-extended = <&pdc 1 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "periph_irq"; > interrupt-controller; > #interrupt-cells = <4>; > #address-cells = <2>; > #size-cells = <0>; > cell-index = <0>; > qcom,channel = <0>; > qcom,ee = <0>; > qcom,bus-id = <0>; > }; > > spmi1_bus: qcom,spmi@c432000 { > compatible = "qcom,spmi-pmic-arb"; > reg = <0xc432000 0x4000>, > <0xc400000 0x3000>, > <0xc500000 0x400000>, > <0xc440000 0x80000>, > <0xc4d0000 0x10000>; > reg-names = "cnfg", "core", "chnls", "obsrvr", "intr"; > interrupts-extended = <&pdc 3 IRQ_TYPE_LEVEL_HIGH>; > interrupt-names = "periph_irq"; > interrupt-controller; > #interrupt-cells = <4>; > #address-cells = <2>; > #size-cells = <0>; > cell-index = <0>; > qcom,channel = <0>; > qcom,ee = <0>; > qcom,bus-id = <1>; > }; > > Note the inclusion of a new DT property: "qcom,bus-id". This was > defined in a DT binding patch that isn't present in Vinod's submission. > Here is its definition: > > - qcom,bus-id : Specifies which SPMI bus instance to use. This property > is only applicable for PMIC arbiter version 7 and > beyond. > Support values: 0 = primary bus, 1 = secondary bus > Assumed to be 0 if unspecified. I havent picked that part yet. This was not needed for base stuff to work yet. Feel free to send that as additional change
On 03-12-21, 16:45, David Collins wrote: > On 12/2/21 7:13 PM, Stephen Boyd wrote: > > Quoting David Collins (2021-12-02 15:51:18) > >> On 12/2/21 3:06 PM, Stephen Boyd wrote: > >>> Quoting Vinod Koul (2021-11-30 23:27:18) > > It feels like we should make a parent node that holds the core, chnls, > > obsrvr reg properties with a new compatible string and then have two > > child nodes for each bus. Then the various PMICs can hang off those two > > different bus nodes. Of course, it needs DT review to make sure nothing > > else goes wrong. > > We considered this alternative DT layout when implementing PMIC arbiter > v7 support downstream. The benefit is allowing common register ranges > to be specified once instead of in both SPMI bus nodes. However, this > approach has several downsides. > > It results in substantially more complex device tree binding > documentation that requires a different layout for SPMI buses for PMIC > arbiter v7 (and above) vs early versions even though support can be > provided with only a minimal modification (i.e. "qcom,bus-id"). > Complexity is also increased inside of the spmi-pmic-arb driver. There, > special data structures and logic would be needed to handle the shared > vs independent register addresses and data. > > The SPMI framework currently uses a one-to-one mapping between SPMI > buses and struct device pointers. This would not work if the new > top-level node represents the true struct device and the per-bus > subnodes are not true devices. Also, platform_get_resource_byname() > (used in the spmi-pmic-arb probe function) needs a struct > platform_device pointer to read address ranges using "reg" + > "reg-names". That wouldn't work for the subnodes. > > I suppose that "compatible" could be specified for the top-level node > and the per bus subnodes so that all three get probed as struct devices. > However, that makes things even more complicated and convoluted in the > driver (and DT). > > I would prefer to stay with the approach of the two bus instances being > specified independently in DT. > > Note that the clk-imx8qxp-lpcg driver has a similar situation where > multiple drivers need to map addresses in the same region. Commit [1] > documents the requirement. The details of the problem where > devm_platform_ioremap_resource() cannot be used in place of > devm_ioremap() were discussed in this thread [2]. Steve, David, Is this the way we are recommending for this to be move forward with?
diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c index bbbd311eda03..28418a10ee5c 100644 --- a/drivers/spmi/spmi-pmic-arb.c +++ b/drivers/spmi/spmi-pmic-arb.c @@ -22,8 +22,14 @@ #define PMIC_ARB_VERSION_V2_MIN 0x20010000 #define PMIC_ARB_VERSION_V3_MIN 0x30000000 #define PMIC_ARB_VERSION_V5_MIN 0x50000000 +#define PMIC_ARB_VERSION_V7_MIN 0x70000000 #define PMIC_ARB_INT_EN 0x0004 +#define PMIC_ARB_FEATURES 0x0004 +#define PMIC_ARB_FEATURES_PERIPH_MASK GENMASK(10, 0) + +#define PMIC_ARB_FEATURES1 0x008 + /* PMIC Arbiter channel registers offsets */ #define PMIC_ARB_CMD 0x00 #define PMIC_ARB_CONFIG 0x04 @@ -48,7 +54,6 @@ #define INVALID_EE 0xFF /* Ownership Table */ -#define SPMI_OWNERSHIP_TABLE_REG(N) (0x0700 + (4 * (N))) #define SPMI_OWNERSHIP_PERIPH2OWNER(X) ((X) & 0x7) /* Channel Status fields */ @@ -91,6 +96,7 @@ enum pmic_arb_channel { /* Maximum number of support PMIC peripherals */ #define PMIC_ARB_MAX_PERIPHS 512 +#define PMIC_ARB_MAX_PERIPHS_V7 1024 #define PMIC_ARB_TIMEOUT_US 100 #define PMIC_ARB_MAX_TRANS_BYTES (8) @@ -104,12 +110,12 @@ enum pmic_arb_channel { ((((slave_id) & 0xF) << 28) | \ (((periph_id) & 0xFF) << 20) | \ (((irq_id) & 0x7) << 16) | \ - (((apid) & 0x1FF) << 0)) + (((apid) & 0x3FF) << 0)) #define hwirq_to_sid(hwirq) (((hwirq) >> 28) & 0xF) #define hwirq_to_per(hwirq) (((hwirq) >> 20) & 0xFF) #define hwirq_to_irq(hwirq) (((hwirq) >> 16) & 0x7) -#define hwirq_to_apid(hwirq) (((hwirq) >> 0) & 0x1FF) +#define hwirq_to_apid(hwirq) (((hwirq) >> 0) & 0x3FF) struct pmic_arb_ver_ops; @@ -149,8 +155,11 @@ struct spmi_pmic_arb { u8 channel; int irq; u8 ee; + u32 bus_instance; u16 min_apid; u16 max_apid; + u16 base_apid; + int apid_count; u32 *mapping_table; DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS); struct irq_domain *domain; @@ -158,7 +167,8 @@ struct spmi_pmic_arb { const struct pmic_arb_ver_ops *ver_ops; u16 *ppid_to_apid; u16 last_apid; - struct apid_data apid_data[PMIC_ARB_MAX_PERIPHS]; + struct apid_data *apid_data; + int max_periphs; }; /** @@ -196,6 +206,7 @@ struct pmic_arb_ver_ops { void __iomem *(*irq_status)(struct spmi_pmic_arb *pmic_arb, u16 n); void __iomem *(*irq_clear)(struct spmi_pmic_arb *pmic_arb, u16 n); u32 (*apid_map_offset)(u16 n); + void __iomem *(*apid_owner)(struct spmi_pmic_arb *pmic_arb, u16 n); }; static inline void pmic_arb_base_write(struct spmi_pmic_arb *pmic_arb, @@ -530,6 +541,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc) struct irq_chip *chip = irq_desc_get_chip(desc); int first = pmic_arb->min_apid >> 5; int last = pmic_arb->max_apid >> 5; + int acc_offsets = pmic_arb->base_apid >> 5; u8 ee = pmic_arb->ee; u32 status, enable; int i, id, apid; @@ -538,7 +550,7 @@ static void pmic_arb_chained_irq(struct irq_desc *desc) for (i = first; i <= last; ++i) { status = readl_relaxed( - ver_ops->owner_acc_status(pmic_arb, ee, i)); + ver_ops->owner_acc_status(pmic_arb, ee, i - acc_offsets)); while (status) { id = ffs(status) - 1; status &= ~BIT(id); @@ -839,8 +851,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pmic_arb, u16 ppid) if (offset >= pmic_arb->core_size) break; - regval = readl_relaxed(pmic_arb->cnfg + - SPMI_OWNERSHIP_TABLE_REG(apid)); + regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, apid)); apidd->irq_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval); apidd->write_ee = apidd->irq_ee; @@ -876,9 +887,9 @@ static int pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pmic_arb, u16 ppid) static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb) { - struct apid_data *apidd = pmic_arb->apid_data; + struct apid_data *apidd; struct apid_data *prev_apidd; - u16 i, apid, ppid; + u16 i, apid, ppid, apid_max; bool valid, is_irq_ee; u32 regval, offset; @@ -889,7 +900,9 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb) * allowed to write to the APID. The owner of the last (highest) APID * for a given PPID will receive interrupts from the PPID. */ - for (i = 0; ; i++, apidd++) { + apidd = &pmic_arb->apid_data[pmic_arb->base_apid]; + apid_max = pmic_arb->base_apid + pmic_arb->apid_count; + for (i = pmic_arb->base_apid; i < apid_max; i++, apidd++) { offset = pmic_arb->ver_ops->apid_map_offset(i); if (offset >= pmic_arb->core_size) break; @@ -900,8 +913,7 @@ static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb) ppid = (regval >> 8) & PMIC_ARB_PPID_MASK; is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval); - regval = readl_relaxed(pmic_arb->cnfg + - SPMI_OWNERSHIP_TABLE_REG(i)); + regval = readl_relaxed(pmic_arb->ver_ops->apid_owner(pmic_arb, i)); apidd->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval); apidd->irq_ee = is_irq_ee ? apidd->write_ee : INVALID_EE; @@ -995,6 +1007,36 @@ static int pmic_arb_offset_v5(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr, return offset; } +static int pmic_arb_offset_v7(struct spmi_pmic_arb *pmic_arb, u8 sid, u16 addr, + enum pmic_arb_channel ch_type) +{ + u16 apid; + int rc; + u32 offset = 0; + u16 ppid = (sid << 8) | (addr >> 8); + + rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid); + if (rc < 0) + return rc; + + apid = rc; + switch (ch_type) { + case PMIC_ARB_CHANNEL_OBS: + offset = 0x8000 * pmic_arb->ee + 0x20 * apid; + break; + case PMIC_ARB_CHANNEL_RW: + if (pmic_arb->apid_data[apid].write_ee != pmic_arb->ee) { + dev_err(&pmic_arb->spmic->dev, "disallow spmi write to sid=%u, add: %x\n", + sid, addr); + return -EPERM; + } + offset = 0x10000 * apid; + break; + } + + return offset; +} + static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc) { return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7); @@ -1029,6 +1071,12 @@ pmic_arb_owner_acc_status_v5(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n) return pmic_arb->intr + 0x10000 * m + 0x4 * n; } +static void __iomem * +pmic_arb_owner_acc_status_v7(struct spmi_pmic_arb *pmic_arb, u8 m, u16 n) +{ + return pmic_arb->intr + 0x1000 * m + 0x4 * n; +} + static void __iomem * pmic_arb_acc_enable_v1(struct spmi_pmic_arb *pmic_arb, u16 n) { @@ -1047,6 +1095,12 @@ pmic_arb_acc_enable_v5(struct spmi_pmic_arb *pmic_arb, u16 n) return pmic_arb->wr_base + 0x100 + 0x10000 * n; } +static void __iomem * +pmic_arb_acc_enable_v7(struct spmi_pmic_arb *pmic_arb, u16 n) +{ + return pmic_arb->wr_base + 0x100 + 0x1000 * n; +} + static void __iomem * pmic_arb_irq_status_v1(struct spmi_pmic_arb *pmic_arb, u16 n) { @@ -1065,6 +1119,12 @@ pmic_arb_irq_status_v5(struct spmi_pmic_arb *pmic_arb, u16 n) return pmic_arb->wr_base + 0x104 + 0x10000 * n; } +static void __iomem * +pmic_arb_irq_status_v7(struct spmi_pmic_arb *pmic_arb, u16 n) +{ + return pmic_arb->wr_base + 0x104 + 0x1000 * n; +} + static void __iomem * pmic_arb_irq_clear_v1(struct spmi_pmic_arb *pmic_arb, u16 n) { @@ -1079,6 +1139,12 @@ pmic_arb_irq_clear_v2(struct spmi_pmic_arb *pmic_arb, u16 n) static void __iomem * pmic_arb_irq_clear_v5(struct spmi_pmic_arb *pmic_arb, u16 n) +{ + return pmic_arb->wr_base + 0x108 + 0x1000 * n; +} + +static void __iomem * +pmic_arb_irq_clear_v7(struct spmi_pmic_arb *pmic_arb, u16 n) { return pmic_arb->wr_base + 0x108 + 0x10000 * n; } @@ -1093,6 +1159,23 @@ static u32 pmic_arb_apid_map_offset_v5(u16 n) return 0x900 + 0x4 * n; } +static u32 pmic_arb_apid_map_offset_v7(u16 n) +{ + return 0x2000 + 0x4 * n; +} + +static void __iomem * +pmic_arb_apid_owner_v2(struct spmi_pmic_arb *pmic_arb, u16 n) +{ + return pmic_arb->cnfg + 0x700 + 0x4 * n; +} + +static void __iomem * +pmic_arb_apid_owner_v7(struct spmi_pmic_arb *pmic_arb, u16 n) +{ + return pmic_arb->cnfg + 0x4 * (n - pmic_arb->base_apid); +} + static const struct pmic_arb_ver_ops pmic_arb_v1 = { .ver_str = "v1", .ppid_to_apid = pmic_arb_ppid_to_apid_v1, @@ -1104,6 +1187,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v1 = { .irq_status = pmic_arb_irq_status_v1, .irq_clear = pmic_arb_irq_clear_v1, .apid_map_offset = pmic_arb_apid_map_offset_v2, + .apid_owner = pmic_arb_apid_owner_v2, }; static const struct pmic_arb_ver_ops pmic_arb_v2 = { @@ -1117,6 +1201,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v2 = { .irq_status = pmic_arb_irq_status_v2, .irq_clear = pmic_arb_irq_clear_v2, .apid_map_offset = pmic_arb_apid_map_offset_v2, + .apid_owner = pmic_arb_apid_owner_v2, }; static const struct pmic_arb_ver_ops pmic_arb_v3 = { @@ -1130,6 +1215,7 @@ static const struct pmic_arb_ver_ops pmic_arb_v3 = { .irq_status = pmic_arb_irq_status_v2, .irq_clear = pmic_arb_irq_clear_v2, .apid_map_offset = pmic_arb_apid_map_offset_v2, + .apid_owner = pmic_arb_apid_owner_v2, }; static const struct pmic_arb_ver_ops pmic_arb_v5 = { @@ -1143,6 +1229,21 @@ static const struct pmic_arb_ver_ops pmic_arb_v5 = { .irq_status = pmic_arb_irq_status_v5, .irq_clear = pmic_arb_irq_clear_v5, .apid_map_offset = pmic_arb_apid_map_offset_v5, + .apid_owner = pmic_arb_apid_owner_v2, +}; + +static const struct pmic_arb_ver_ops pmic_arb_v7 = { + .ver_str = "v7", + .ppid_to_apid = pmic_arb_ppid_to_apid_v5, + .non_data_cmd = pmic_arb_non_data_cmd_v2, + .offset = pmic_arb_offset_v7, + .fmt_cmd = pmic_arb_fmt_cmd_v2, + .owner_acc_status = pmic_arb_owner_acc_status_v7, + .acc_enable = pmic_arb_acc_enable_v7, + .irq_status = pmic_arb_irq_status_v7, + .irq_clear = pmic_arb_irq_clear_v7, + .apid_map_offset = pmic_arb_apid_map_offset_v7, + .apid_owner = pmic_arb_apid_owner_v7, }; static const struct irq_domain_ops pmic_arb_irq_domain_ops = { @@ -1169,8 +1270,12 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) pmic_arb = spmi_controller_get_drvdata(ctrl); pmic_arb->spmic = ctrl; + /* + * Don't use devm_ioremap_resource() as the resources are shared in + * PMIC v7 onwards, so causing failure when mapping + */ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "core"); - core = devm_ioremap_resource(&ctrl->dev, res); + core = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); if (IS_ERR(core)) { err = PTR_ERR(core); goto err_put_ctrl; @@ -1199,12 +1304,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) pmic_arb->ver_ops = &pmic_arb_v2; else if (hw_ver < PMIC_ARB_VERSION_V5_MIN) pmic_arb->ver_ops = &pmic_arb_v3; - else + 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_resource(&ctrl->dev, res); + pmic_arb->rd_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); if (IS_ERR(pmic_arb->rd_base)) { err = PTR_ERR(pmic_arb->rd_base); goto err_put_ctrl; @@ -1212,25 +1319,68 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "chnls"); - pmic_arb->wr_base = devm_ioremap_resource(&ctrl->dev, res); + pmic_arb->wr_base = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); if (IS_ERR(pmic_arb->wr_base)) { err = PTR_ERR(pmic_arb->wr_base); goto err_put_ctrl; } } + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS; + + if (hw_ver >= PMIC_ARB_VERSION_V7_MIN) { + pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V7; + + of_property_read_u32(pdev->dev.of_node, "qcom,bus-id", &pmic_arb->bus_instance); + if (pmic_arb->bus_instance > 1) { + err = -EINVAL; + dev_err(&ctrl->dev, "invalid bus instance: %d\n", pmic_arb->bus_instance); + goto err_put_ctrl; + } + + if (pmic_arb->bus_instance == 0) { + pmic_arb->base_apid = 0; + pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES) & + PMIC_ARB_FEATURES_PERIPH_MASK; + } else { + pmic_arb->base_apid = readl_relaxed(core + PMIC_ARB_FEATURES) & + PMIC_ARB_FEATURES_PERIPH_MASK; + pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES1) & + PMIC_ARB_FEATURES_PERIPH_MASK; + } + + } else if (hw_ver >= PMIC_ARB_VERSION_V5_MIN) { + pmic_arb->base_apid = 0; + pmic_arb->apid_count = readl_relaxed(core + PMIC_ARB_FEATURES) & + PMIC_ARB_FEATURES_PERIPH_MASK; + } + + if (pmic_arb->base_apid + pmic_arb->apid_count > pmic_arb->max_periphs) { + err = -EINVAL; + dev_err(&ctrl->dev, "Unsupported APID count: %d\n", + pmic_arb->base_apid + pmic_arb->apid_count); + goto err_put_ctrl; + } + + pmic_arb->apid_data = devm_kcalloc(&ctrl->dev, pmic_arb->max_periphs, + sizeof(*pmic_arb->apid_data), GFP_KERNEL); + if (!pmic_arb->apid_data) { + err = -ENOMEM; + goto err_put_ctrl; + } + dev_info(&ctrl->dev, "PMIC arbiter version %s (0x%x)\n", pmic_arb->ver_ops->ver_str, hw_ver); res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr"); - pmic_arb->intr = devm_ioremap_resource(&ctrl->dev, res); + pmic_arb->intr = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); if (IS_ERR(pmic_arb->intr)) { err = PTR_ERR(pmic_arb->intr); goto err_put_ctrl; } res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "cnfg"); - pmic_arb->cnfg = devm_ioremap_resource(&ctrl->dev, res); + pmic_arb->cnfg = devm_ioremap(&ctrl->dev, res->start, resource_size(res)); if (IS_ERR(pmic_arb->cnfg)) { err = PTR_ERR(pmic_arb->cnfg); goto err_put_ctrl; @@ -1281,7 +1431,7 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev) /* Initialize max_apid/min_apid to the opposite bounds, during * the irq domain translation, we are sure to update these */ pmic_arb->max_apid = 0; - pmic_arb->min_apid = PMIC_ARB_MAX_PERIPHS - 1; + pmic_arb->min_apid = pmic_arb->max_periphs - 1; platform_set_drvdata(pdev, ctrl); raw_spin_lock_init(&pmic_arb->lock);