On 05/30, Kiran Gunda wrote:
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 412481d..b755c24 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code {
>
> struct apid_data {
> u16 ppid;
> - u8 owner;
> + u8 write_owner;
> + u8 irq_owner;
How about irq_ee and write_ee instead?
> };
>
> /**
> static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
> @@ -705,11 +724,18 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
> (intspec[1] << 8), &apid);
> if (rc < 0) {
> dev_err(&pa->spmic->dev,
> - "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
> + "failed to xlate sid = 0x%x, periph = 0x%x, irq = %u rc = %d\n",
What is this change? Also, use %#x instead of 0x%x and the "
should line up with the ( on the previous line.
> intspec[0], intspec[1], intspec[2], rc);
> return rc;
> }
>
> + if (pa->apid_data[apid].irq_owner != pa->ee) {
> + dev_err(&pa->spmic->dev, "failed to xlate sid = 0x%x, periph = 0x%x, irq = %u: ee=%u but owner=%u\n",
> + intspec[0], intspec[1], intspec[2], pa->ee,
> + pa->apid_data[apid].irq_owner);
> + return -ENODEV;
> + }
> +
> /* Keep track of {max,min}_apid for bounding search during interrupt */
> if (apid > pa->max_apid)
> pa->max_apid = apid;
> @@ -814,9 +841,11 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
> for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
> regval = readl_relaxed(pa->cnfg +
> SPMI_OWNERSHIP_TABLE_REG(apid));
> - pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> + pa->apid_data[apid].irq_owner
> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> + pa->apid_data[apid].write_owner = pa->apid_data[apid].irq_owner;
Please use a local variable pointer *apid.
>
> - offset = PMIC_ARB_REG_CHNL(apid);
> + offset = pa->ver_ops->channel_map_offset(apid);
> if (offset >= pa->core_size)
> break;
>
> @@ -854,27 +883,110 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
> return 0;
> }
>
> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pa)
> +{
> + u32 regval, offset;
> + u16 apid, prev_apid, ppid;
> + bool valid, is_irq_owner;
> +
> + /*
> + * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to PPID.
> + * ppid_to_apid is an in-memory invert of that table. In order to allow
> + * multiple EE's to write to a single PPID in arbiter version 5, there
Drop the apostrophe ^
> + * is more than one APID mapped to each PPID. The owner field for each
> + * of these mappings specifies the EE which is allowed to write to the
> + * APID. The owner of the last (highest) APID for a given PPID will
> + * receive interrupts from the PPID.
> + */
> + for (apid = 0; apid < pa->max_periph; apid++) {
> + offset = pa->ver_ops->channel_map_offset(apid);
> + if (offset >= pa->core_size)
> + break;
> +
> + regval = readl_relaxed(pa->core + offset);
> + if (!regval)
> + continue;
> + ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> + is_irq_owner = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
> +
> + regval = readl_relaxed(pa->cnfg +
> + SPMI_OWNERSHIP_TABLE_REG(apid));
> + pa->apid_data[apid].write_owner
> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
Please use a pointer like *apid and *prev where *apid is
incremented during the for-loop. That way we have shorter lines
of code:
apid->write_owner = SPMI_OWNERSHIP_TABLE_REG(regval);
and
if (valid && is_irq_owner && prev->write_owner == pa->ee) {
obviously the existing apid will need to be renamed to i, but
that's ok because it's a counter.
> +
> + pa->apid_data[apid].irq_owner = is_irq_owner ?
> + pa->apid_data[apid].write_owner : INVALID_EE;
> +
> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
> + prev_apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
> +
> + if (valid && is_irq_owner &&
> + pa->apid_data[prev_apid].write_owner == pa->ee) {
> + /*
> + * Duplicate PPID mapping after the one for this EE;
> + * override the irq owner
> + */
> + pa->apid_data[prev_apid].irq_owner
> + = pa->apid_data[apid].irq_owner;
> + } else if (!valid || is_irq_owner) {
> + /* First PPID mapping or duplicate for another EE */
> + pa->ppid_to_apid[ppid] = apid | PMIC_ARB_CHAN_VALID;
> + }
> +
> + pa->apid_data[apid].ppid = ppid;
> + pa->last_apid = apid;
> + }
> +
> + /* Dump the mapping table for debug purposes. */
> + dev_dbg(&pa->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
> + for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
> + apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
> +
> + if (valid)
> + dev_dbg(&pa->spmic->dev, "0x%03X %3u %2u %2u\n",
Same %#x story here.
> + ppid, apid, pa->apid_data[apid].write_owner,
> + pa->apid_data[apid].irq_owner);
> + }
> +
> + return 0;
> +}
> +
>
> @@ -887,6 +999,27 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
> return 0;
> }
>
> +/*
> + * v5 offset per ee and per apid for observer channels and per apid for
> + * read/write channels.
> + */
> +static int
> +pmic_arb_offset_v5(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
> + enum pmic_arb_channel ch_type, u32 *offset)
> +{
> + u16 apid;
> + int rc;
> +
> + rc = pmic_arb_ppid_to_apid_v5(pa, sid, addr, &apid);
> + if (rc < 0)
> + return rc;
> +
> + *offset = (ch_type == PMIC_ARB_CHANNEL_OBS)
> + ? 0x10000 * pa->ee + 0x80 * apid
> + : 0x10000 * apid;
Please use a switch statement to handle the enum here. That way
we can get compiler checking to make sure all enumerations are
handled.
> + return 0;
> +}
> +
> 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);
> @@ -1033,11 +1213,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
>
> if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
> pa->ver_ops = &pmic_arb_v2;
> - else
> + else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
> pa->ver_ops = &pmic_arb_v3;
> + else
> + pa->ver_ops = &pmic_arb_v5;
>
> - /* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
> - pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
> + /* the apid to ppid table starts at PMIC_ARB_REG_CHNL0 */
> + pa->max_periph
> + = (pa->core_size - pa->ver_ops->channel_map_offset(0)) / 4;
This is really ugly. Please grow a local variable so we can keep
things on one line.
>
> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "obsrvr");
> @@ -1074,6 +1257,14 @@ static int spmi_pmic_arb_probe(struct platform_device *pdev)
> err = PTR_ERR(pa->intr);
> goto err_put_ctrl;
> }
> + pa->acc_status = pa->intr;
> +
> + /*
> + * PMIC arbiter v5 groups the IRQ control registers in the same hardware
> + * module as the read/write channels.
> + */
> + if (hw_ver >= PMIC_ARB_VERSION_V5_MIN)
> + pa->intr = pa->wr_base;
There's some weird things going on here. How about we make the
version ops return an __iomem pointer to the address instead of
an offset? That way we don't need to care what pa->intr is or add
pa->acc_status? That could be a patch early in the series that
adjusts the ops to return an iomem pointer, and perhaps also
update the 'n' variable to be 16 instead of 8 bits. We can also
express errors through iomem pointers with IS_ERR() checks, so it
nicely removes the need to retrieve offsets through references
sometimes too.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
On 2017-06-01 11:38, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> diff --git a/drivers/spmi/spmi-pmic-arb.c
>> b/drivers/spmi/spmi-pmic-arb.c
>> index 412481d..b755c24 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code {
>>
>> struct apid_data {
>> u16 ppid;
>> - u8 owner;
>> + u8 write_owner;
>> + u8 irq_owner;
>
> How about irq_ee and write_ee instead?
>
Sure. will change it in the subsequent patch.
>> };
>>
>> /**
>> static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
>> @@ -705,11 +724,18 @@ static int
>> qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>> (intspec[1] << 8), &apid);
>> if (rc < 0) {
>> dev_err(&pa->spmic->dev,
>> - "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
>> + "failed to xlate sid = 0x%x, periph = 0x%x, irq = %u rc = %d\n",
>
> What is this change? Also, use %#x instead of 0x%x and the "
> should line up with the ( on the previous line.
>
Sure. will change it in the subsequent patch.
>> intspec[0], intspec[1], intspec[2], rc);
>> return rc;
>> }
>>
>> + if (pa->apid_data[apid].irq_owner != pa->ee) {
>> + dev_err(&pa->spmic->dev, "failed to xlate sid = 0x%x, periph =
>> 0x%x, irq = %u: ee=%u but owner=%u\n",
>> + intspec[0], intspec[1], intspec[2], pa->ee,
>> + pa->apid_data[apid].irq_owner);
>> + return -ENODEV;
>> + }
>> +
>> /* Keep track of {max,min}_apid for bounding search during interrupt
>> */
>> if (apid > pa->max_apid)
>> pa->max_apid = apid;
>> @@ -814,9 +841,11 @@ static u16 pmic_arb_find_apid(struct
>> spmi_pmic_arb *pa, u16 ppid)
>> for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
>> regval = readl_relaxed(pa->cnfg +
>> SPMI_OWNERSHIP_TABLE_REG(apid));
>> - pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>> + pa->apid_data[apid].irq_owner
>> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>> + pa->apid_data[apid].write_owner = pa->apid_data[apid].irq_owner;
>
> Please use a local variable pointer *apid.
>
Sure. will change it in the subsequent patch.
>>
>> - offset = PMIC_ARB_REG_CHNL(apid);
>> + offset = pa->ver_ops->channel_map_offset(apid);
>> if (offset >= pa->core_size)
>> break;
>>
>> @@ -854,27 +883,110 @@ static u16 pmic_arb_find_apid(struct
>> spmi_pmic_arb *pa, u16 ppid)
>> return 0;
>> }
>>
>> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pa)
>> +{
>> + u32 regval, offset;
>> + u16 apid, prev_apid, ppid;
>> + bool valid, is_irq_owner;
>> +
>> + /*
>> + * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to
>> PPID.
>> + * ppid_to_apid is an in-memory invert of that table. In order to
>> allow
>> + * multiple EE's to write to a single PPID in arbiter version 5,
>> there
>
> Drop the apostrophe ^
>
Ok. will change it in the subsequent patch.
>> + * is more than one APID mapped to each PPID. The owner field for
>> each
>> + * of these mappings specifies the EE which is allowed to write to
>> the
>> + * APID. The owner of the last (highest) APID for a given PPID will
>> + * receive interrupts from the PPID.
>> + */
>> + for (apid = 0; apid < pa->max_periph; apid++) {
>> + offset = pa->ver_ops->channel_map_offset(apid);
>> + if (offset >= pa->core_size)
>> + break;
>> +
>> + regval = readl_relaxed(pa->core + offset);
>> + if (!regval)
>> + continue;
>> + ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
>> + is_irq_owner = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
>> +
>> + regval = readl_relaxed(pa->cnfg +
>> + SPMI_OWNERSHIP_TABLE_REG(apid));
>> + pa->apid_data[apid].write_owner
>> + = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
>
> Please use a pointer like *apid and *prev where *apid is
> incremented during the for-loop. That way we have shorter lines
> of code:
>
> apid->write_owner = SPMI_OWNERSHIP_TABLE_REG(regval);
>
> and
>
> if (valid && is_irq_owner && prev->write_owner == pa->ee) {
>
> obviously the existing apid will need to be renamed to i, but
> that's ok because it's a counter.
>
Sure. will do it in the subsequent patch..
>> +
>> + pa->apid_data[apid].irq_owner = is_irq_owner ?
>> + pa->apid_data[apid].write_owner : INVALID_EE;
>> +
>> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
>> + prev_apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
>> +
>> + if (valid && is_irq_owner &&
>> + pa->apid_data[prev_apid].write_owner == pa->ee) {
>> + /*
>> + * Duplicate PPID mapping after the one for this EE;
>> + * override the irq owner
>> + */
>> + pa->apid_data[prev_apid].irq_owner
>> + = pa->apid_data[apid].irq_owner;
>> + } else if (!valid || is_irq_owner) {
>> + /* First PPID mapping or duplicate for another EE */
>> + pa->ppid_to_apid[ppid] = apid | PMIC_ARB_CHAN_VALID;
>> + }
>> +
>> + pa->apid_data[apid].ppid = ppid;
>> + pa->last_apid = apid;
>> + }
>> +
>> + /* Dump the mapping table for debug purposes. */
>> + dev_dbg(&pa->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
>> + for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
>> + valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
>> + apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
>> +
>> + if (valid)
>> + dev_dbg(&pa->spmic->dev, "0x%03X %3u %2u %2u\n",
>
> Same %#x story here.
>
Sure. will change it in the subsequent patch.
>> + ppid, apid, pa->apid_data[apid].write_owner,
>> + pa->apid_data[apid].irq_owner);
>> + }
>> +
>> + return 0;
>> +}
>> +
>>
>> @@ -887,6 +999,27 @@ static u16 pmic_arb_find_apid(struct
>> spmi_pmic_arb *pa, u16 ppid)
>> return 0;
>> }
>>
>> +/*
>> + * v5 offset per ee and per apid for observer channels and per apid
>> for
>> + * read/write channels.
>> + */
>> +static int
>> +pmic_arb_offset_v5(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
>> + enum pmic_arb_channel ch_type, u32 *offset)
>> +{
>> + u16 apid;
>> + int rc;
>> +
>> + rc = pmic_arb_ppid_to_apid_v5(pa, sid, addr, &apid);
>> + if (rc < 0)
>> + return rc;
>> +
>> + *offset = (ch_type == PMIC_ARB_CHANNEL_OBS)
>> + ? 0x10000 * pa->ee + 0x80 * apid
>> + : 0x10000 * apid;
>
> Please use a switch statement to handle the enum here. That way
> we can get compiler checking to make sure all enumerations are
> handled.
>
Sure. will change it in the subsequent patch.
>> + return 0;
>> +}
>> +
>> 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);
>> @@ -1033,11 +1213,14 @@ static int spmi_pmic_arb_probe(struct
>> platform_device *pdev)
>>
>> if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
>> pa->ver_ops = &pmic_arb_v2;
>> - else
>> + else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
>> pa->ver_ops = &pmic_arb_v3;
>> + else
>> + pa->ver_ops = &pmic_arb_v5;
>>
>> - /* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
>> - pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
>> + /* the apid to ppid table starts at PMIC_ARB_REG_CHNL0 */
>> + pa->max_periph
>> + = (pa->core_size - pa->ver_ops->channel_map_offset(0)) / 4;
>
> This is really ugly. Please grow a local variable so we can keep
> things on one line.
>
Ok. will change it in the subsequent patch.
>>
>> res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "obsrvr");
>> @@ -1074,6 +1257,14 @@ static int spmi_pmic_arb_probe(struct
>> platform_device *pdev)
>> err = PTR_ERR(pa->intr);
>> goto err_put_ctrl;
>> }
>> + pa->acc_status = pa->intr;
>> +
>> + /*
>> + * PMIC arbiter v5 groups the IRQ control registers in the same
>> hardware
>> + * module as the read/write channels.
>> + */
>> + if (hw_ver >= PMIC_ARB_VERSION_V5_MIN)
>> + pa->intr = pa->wr_base;
>
> There's some weird things going on here. How about we make the
> version ops return an __iomem pointer to the address instead of
> an offset? That way we don't need to care what pa->intr is or add
> pa->acc_status? That could be a patch early in the series that
> adjusts the ops to return an iomem pointer, and perhaps also
> update the 'n' variable to be 16 instead of 8 bits. We can also
> express errors through iomem pointers with IS_ERR() checks, so it
> nicely removes the need to retrieve offsets through references
> sometimes too.
Yes. This looks good. Will change it in the subsequent patch series.