| Message ID | 20210708180851.2311192-4-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mailbox: pcc: Add support for PCCT extended PCC subspaces | expand |
On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote: > Currently all the PCC channel specific information are stored/maintained > in global individual arrays for each of those information. It is not > scalable and not clean if we have to stash more channel specific > information. Couple of reasons to stash more information are to extend > the support to Type 3/4 PCCT subspace and also to avoid accessing the > PCCT table entries themselves each time we need the information. > > This patch moves all those PCC channel specific information into a > separate structure pcc_chan_info. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- Hi Sudeep, > drivers/mailbox/pcc.c | 106 +++++++++++++++++++++--------------------- > 1 file changed, 53 insertions(+), 53 deletions(-) > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > index 23391e224a68..c5f481a615b0 100644 > --- a/drivers/mailbox/pcc.c > +++ b/drivers/mailbox/pcc.c > @@ -64,12 +64,20 @@ > > static struct mbox_chan *pcc_mbox_channels; > > -/* Array of cached virtual address for doorbell registers */ > -static void __iomem **pcc_doorbell_vaddr; > -/* Array of cached virtual address for doorbell ack registers */ > -static void __iomem **pcc_doorbell_ack_vaddr; > -/* Array of doorbell interrupts */ > -static int *pcc_doorbell_irq; > +/** > + * struct pcc_chan_info - PCC channel specific information > + * > + * @db_vaddr: cached virtual address for doorbell register > + * @db_ack_vaddr: cached virtual address for doorbell ack register > + * @db_irq: doorbell interrupt > + */ > +struct pcc_chan_info { > + void __iomem *db_vaddr; > + void __iomem *db_ack_vaddr; > + int db_irq; > +}; Given that this db_irq represents the optional completion interrupt that is used platform-->OSPM to signal command completions and/or notifications/ delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform Interrupt" and also referred in this driver as such somewherelse, is it not misleading to call it then here "doorbell interrupt" since the "doorbell" in this context is usually the interrupt that goes the other way around OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell registers ? (that are indeed called Doorbell throughout this driver down below ...but I understand this was the nomenclature used also before in this driver) Thanks, Cristian
On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote: > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote: > > Currently all the PCC channel specific information are stored/maintained > > in global individual arrays for each of those information. It is not > > scalable and not clean if we have to stash more channel specific > > information. Couple of reasons to stash more information are to extend > > the support to Type 3/4 PCCT subspace and also to avoid accessing the > > PCCT table entries themselves each time we need the information. > > > > This patch moves all those PCC channel specific information into a > > separate structure pcc_chan_info. > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > --- > > Hi Sudeep, > > > drivers/mailbox/pcc.c | 106 +++++++++++++++++++++--------------------- > > 1 file changed, 53 insertions(+), 53 deletions(-) > > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > > index 23391e224a68..c5f481a615b0 100644 > > --- a/drivers/mailbox/pcc.c > > +++ b/drivers/mailbox/pcc.c > > @@ -64,12 +64,20 @@ > > > > static struct mbox_chan *pcc_mbox_channels; > > > > -/* Array of cached virtual address for doorbell registers */ > > -static void __iomem **pcc_doorbell_vaddr; > > -/* Array of cached virtual address for doorbell ack registers */ > > -static void __iomem **pcc_doorbell_ack_vaddr; > > -/* Array of doorbell interrupts */ > > -static int *pcc_doorbell_irq; > > +/** > > + * struct pcc_chan_info - PCC channel specific information > > + * > > + * @db_vaddr: cached virtual address for doorbell register > > + * @db_ack_vaddr: cached virtual address for doorbell ack register > > + * @db_irq: doorbell interrupt > > + */ > > +struct pcc_chan_info { > > + void __iomem *db_vaddr; > > + void __iomem *db_ack_vaddr; > > + int db_irq; > > +}; > > Given that this db_irq represents the optional completion interrupt that is > used platform-->OSPM to signal command completions and/or notifications/ > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform > Interrupt" and also referred in this driver as such somewherelse, is it not > misleading to call it then here "doorbell interrupt" since the "doorbell" in > this context is usually the interrupt that goes the other way around > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell > registers ? (that are indeed called Doorbell throughout this driver down below > ...but I understand this was the nomenclature used also before in this driver) > Exactly, I share your thoughts and I completely agree. I didn't want to change it in this patch as that would mix 2 different change and makes it hard to review. I assume you might have already seen the 8/13 which renames this before we add more such registers in later patches. -- Regards, Sudeep
On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote: > On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote: > > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote: > > > Currently all the PCC channel specific information are stored/maintained > > > in global individual arrays for each of those information. It is not > > > scalable and not clean if we have to stash more channel specific > > > information. Couple of reasons to stash more information are to extend > > > the support to Type 3/4 PCCT subspace and also to avoid accessing the > > > PCCT table entries themselves each time we need the information. > > > > > > This patch moves all those PCC channel specific information into a > > > separate structure pcc_chan_info. > > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > --- > > > > Hi Sudeep, > > > > > drivers/mailbox/pcc.c | 106 +++++++++++++++++++++--------------------- > > > 1 file changed, 53 insertions(+), 53 deletions(-) > > > > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > > > index 23391e224a68..c5f481a615b0 100644 > > > --- a/drivers/mailbox/pcc.c > > > +++ b/drivers/mailbox/pcc.c > > > @@ -64,12 +64,20 @@ > > > > > > static struct mbox_chan *pcc_mbox_channels; > > > > > > -/* Array of cached virtual address for doorbell registers */ > > > -static void __iomem **pcc_doorbell_vaddr; > > > -/* Array of cached virtual address for doorbell ack registers */ > > > -static void __iomem **pcc_doorbell_ack_vaddr; > > > -/* Array of doorbell interrupts */ > > > -static int *pcc_doorbell_irq; > > > +/** > > > + * struct pcc_chan_info - PCC channel specific information > > > + * > > > + * @db_vaddr: cached virtual address for doorbell register > > > + * @db_ack_vaddr: cached virtual address for doorbell ack register > > > + * @db_irq: doorbell interrupt > > > + */ > > > +struct pcc_chan_info { > > > + void __iomem *db_vaddr; > > > + void __iomem *db_ack_vaddr; > > > + int db_irq; > > > +}; > > > > Given that this db_irq represents the optional completion interrupt that is > > used platform-->OSPM to signal command completions and/or notifications/ > > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform > > Interrupt" and also referred in this driver as such somewherelse, is it not > > misleading to call it then here "doorbell interrupt" since the "doorbell" in > > this context is usually the interrupt that goes the other way around > > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell > > registers ? (that are indeed called Doorbell throughout this driver down below > > ...but I understand this was the nomenclature used also before in this driver) > > > > Exactly, I share your thoughts and I completely agree. I didn't want to change > it in this patch as that would mix 2 different change and makes it hard to > review. I assume you might have already seen the 8/13 which renames this > before we add more such registers in later patches. > Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are consolidated and renamed. So that confused me even more :D Thanks, Cristian > -- > Regards, > Sudeep
On Thu, Jul 15, 2021 at 01:50:48PM +0100, Cristian Marussi wrote: > On Thu, Jul 15, 2021 at 12:27:10PM +0100, Sudeep Holla wrote: > > On Wed, Jul 14, 2021 at 05:54:34PM +0100, Cristian Marussi wrote: > > > On Thu, Jul 08, 2021 at 07:08:41PM +0100, Sudeep Holla wrote: > > > > Currently all the PCC channel specific information are stored/maintained > > > > in global individual arrays for each of those information. It is not > > > > scalable and not clean if we have to stash more channel specific > > > > information. Couple of reasons to stash more information are to extend > > > > the support to Type 3/4 PCCT subspace and also to avoid accessing the > > > > PCCT table entries themselves each time we need the information. > > > > > > > > This patch moves all those PCC channel specific information into a > > > > separate structure pcc_chan_info. > > > > > > > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > > > > --- > > > > > > Hi Sudeep, > > > > > > > drivers/mailbox/pcc.c | 106 +++++++++++++++++++++--------------------- > > > > 1 file changed, 53 insertions(+), 53 deletions(-) > > > > > > > > diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c > > > > index 23391e224a68..c5f481a615b0 100644 > > > > --- a/drivers/mailbox/pcc.c > > > > +++ b/drivers/mailbox/pcc.c > > > > @@ -64,12 +64,20 @@ > > > > > > > > static struct mbox_chan *pcc_mbox_channels; > > > > > > > > -/* Array of cached virtual address for doorbell registers */ > > > > -static void __iomem **pcc_doorbell_vaddr; > > > > -/* Array of cached virtual address for doorbell ack registers */ > > > > -static void __iomem **pcc_doorbell_ack_vaddr; > > > > -/* Array of doorbell interrupts */ > > > > -static int *pcc_doorbell_irq; > > > > +/** > > > > + * struct pcc_chan_info - PCC channel specific information > > > > + * > > > > + * @db_vaddr: cached virtual address for doorbell register > > > > + * @db_ack_vaddr: cached virtual address for doorbell ack register > > > > + * @db_irq: doorbell interrupt > > > > + */ > > > > +struct pcc_chan_info { > > > > + void __iomem *db_vaddr; > > > > + void __iomem *db_ack_vaddr; > > > > + int db_irq; > > > > +}; > > > > > > Given that this db_irq represents the optional completion interrupt that is > > > used platform-->OSPM to signal command completions and/or notifications/ > > > delayed_responses and it is mentioned indeed in ACPI 6.4 as "Platform > > > Interrupt" and also referred in this driver as such somewherelse, is it not > > > misleading to call it then here "doorbell interrupt" since the "doorbell" in > > > this context is usually the interrupt that goes the other way around > > > OSPM-->Platform and is indeed handled by a different set of dedicated Doorbell > > > registers ? (that are indeed called Doorbell throughout this driver down below > > > ...but I understand this was the nomenclature used also before in this driver) > > > > > > > Exactly, I share your thoughts and I completely agree. I didn't want to change > > it in this patch as that would mix 2 different change and makes it hard to > > review. I assume you might have already seen the 8/13 which renames this > > before we add more such registers in later patches. > > > > Yes indeed, but db_irq is not renamed even later when db_vaddr/db_ack_addr are > consolidated and renamed. So that confused me even more :D > Yikes! That was not intentional. Since I re-ordered some of the changes after making them originally, I relied on regex to get it right and ease the rebasing which I know I failed terribly. I will fix that too. -- Regards, Sudeep
diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 23391e224a68..c5f481a615b0 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -64,12 +64,20 @@ static struct mbox_chan *pcc_mbox_channels; -/* Array of cached virtual address for doorbell registers */ -static void __iomem **pcc_doorbell_vaddr; -/* Array of cached virtual address for doorbell ack registers */ -static void __iomem **pcc_doorbell_ack_vaddr; -/* Array of doorbell interrupts */ -static int *pcc_doorbell_irq; +/** + * struct pcc_chan_info - PCC channel specific information + * + * @db_vaddr: cached virtual address for doorbell register + * @db_ack_vaddr: cached virtual address for doorbell ack register + * @db_irq: doorbell interrupt + */ +struct pcc_chan_info { + void __iomem *db_vaddr; + void __iomem *db_ack_vaddr; + int db_irq; +}; + +static struct pcc_chan_info *chan_info; static struct mbox_controller pcc_mbox_ctrl = {}; /** @@ -183,6 +191,7 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) { struct acpi_generic_address *doorbell_ack; struct acpi_pcct_hw_reduced *pcct_ss; + struct pcc_chan_info *pchan; struct mbox_chan *chan = p; u64 doorbell_ack_preserve; u64 doorbell_ack_write; @@ -197,17 +206,17 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) struct acpi_pcct_hw_reduced_type2 *pcct2_ss = chan->con_priv; u32 id = chan - pcc_mbox_channels; + pchan = chan_info + id; doorbell_ack = &pcct2_ss->platform_ack_register; doorbell_ack_preserve = pcct2_ss->ack_preserve_mask; doorbell_ack_write = pcct2_ss->ack_write_mask; - ret = read_register(pcc_doorbell_ack_vaddr[id], - &doorbell_ack_val, - doorbell_ack->bit_width); + ret = read_register(pchan->db_ack_vaddr, + &doorbell_ack_val, doorbell_ack->bit_width); if (ret) return IRQ_NONE; - ret = write_register(pcc_doorbell_ack_vaddr[id], + ret = write_register(pchan->db_ack_vaddr, (doorbell_ack_val & doorbell_ack_preserve) | doorbell_ack_write, doorbell_ack->bit_width); @@ -232,8 +241,9 @@ static irqreturn_t pcc_mbox_irq(int irq, void *p) * ERR_PTR. */ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, - int subspace_id) + int subspace_id) { + struct pcc_chan_info *pchan = chan_info + subspace_id; struct device *dev = pcc_mbox_ctrl.dev; struct mbox_chan *chan; unsigned long flags; @@ -264,14 +274,14 @@ struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl, spin_unlock_irqrestore(&chan->lock, flags); - if (pcc_doorbell_irq[subspace_id] > 0) { + if (pchan->db_irq > 0) { int rc; - rc = devm_request_irq(dev, pcc_doorbell_irq[subspace_id], - pcc_mbox_irq, 0, MBOX_IRQ_NAME, chan); + rc = devm_request_irq(dev, pchan->db_irq, pcc_mbox_irq, 0, + MBOX_IRQ_NAME, chan); if (unlikely(rc)) { dev_err(dev, "failed to register PCC interrupt %d\n", - pcc_doorbell_irq[subspace_id]); + pchan->db_irq); pcc_mbox_free_channel(chan); chan = ERR_PTR(rc); } @@ -290,6 +300,7 @@ EXPORT_SYMBOL_GPL(pcc_mbox_request_channel); void pcc_mbox_free_channel(struct mbox_chan *chan) { u32 id = chan - pcc_mbox_channels; + struct pcc_chan_info *pchan; unsigned long flags; if (!chan || !chan->cl) @@ -300,8 +311,9 @@ void pcc_mbox_free_channel(struct mbox_chan *chan) return; } - if (pcc_doorbell_irq[id] > 0) - devm_free_irq(chan->mbox->dev, pcc_doorbell_irq[id], chan); + pchan = chan_info + id; + if (pchan->db_irq > 0) + devm_free_irq(chan->mbox->dev, pchan->db_irq, chan); spin_lock_irqsave(&chan->lock, flags); chan->cl = NULL; @@ -329,6 +341,7 @@ static int pcc_send_data(struct mbox_chan *chan, void *data) { struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv; struct acpi_generic_address *doorbell; + struct pcc_chan_info *pchan; u64 doorbell_preserve; u64 doorbell_val; u64 doorbell_write; @@ -340,19 +353,20 @@ static int pcc_send_data(struct mbox_chan *chan, void *data) return -ENOENT; } + pchan = chan_info + id; doorbell = &pcct_ss->doorbell_register; doorbell_preserve = pcct_ss->preserve_mask; doorbell_write = pcct_ss->write_mask; /* Sync notification from OS to Platform. */ - if (pcc_doorbell_vaddr[id]) { - ret = read_register(pcc_doorbell_vaddr[id], &doorbell_val, - doorbell->bit_width); + if (pchan->db_vaddr) { + ret = read_register(pchan->db_vaddr, &doorbell_val, + doorbell->bit_width); if (ret) return ret; - ret = write_register(pcc_doorbell_vaddr[id], - (doorbell_val & doorbell_preserve) | doorbell_write, - doorbell->bit_width); + ret = write_register(pchan->db_vaddr, + (doorbell_val & doorbell_preserve) + | doorbell_write, doorbell->bit_width); } else { ret = acpi_read(&doorbell_val, doorbell); if (ret) @@ -398,12 +412,13 @@ static int parse_pcc_subspace(union acpi_subtable_headers *header, * * This gets called for each entry in the PCC table. */ -static int pcc_parse_subspace_irq(int id, - struct acpi_pcct_hw_reduced *pcct_ss) +static int pcc_parse_subspace_irq(int id, struct acpi_pcct_hw_reduced *pcct_ss) { - pcc_doorbell_irq[id] = pcc_map_interrupt(pcct_ss->platform_interrupt, - (u32)pcct_ss->flags); - if (pcc_doorbell_irq[id] <= 0) { + struct pcc_chan_info *pchan = chan_info + id; + + pchan->db_irq = pcc_map_interrupt(pcct_ss->platform_interrupt, + (u32)pcct_ss->flags); + if (pchan->db_irq <= 0) { pr_err("PCC GSI %d not registered\n", pcct_ss->platform_interrupt); return -EINVAL; @@ -413,10 +428,10 @@ static int pcc_parse_subspace_irq(int id, == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) { struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss; - pcc_doorbell_ack_vaddr[id] = acpi_os_ioremap( - pcct2_ss->platform_ack_register.address, - pcct2_ss->platform_ack_register.bit_width / 8); - if (!pcc_doorbell_ack_vaddr[id]) { + pchan->db_ack_vaddr = + acpi_os_ioremap(pcct2_ss->platform_ack_register.address, + pcct2_ss->platform_ack_register.bit_width / 8); + if (!pchan->db_ack_vaddr) { pr_err("Failed to ioremap PCC ACK register\n"); return -ENOMEM; } @@ -474,24 +489,12 @@ static int __init acpi_pcc_probe(void) goto err_put_pcct; } - pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL); - if (!pcc_doorbell_vaddr) { + chan_info = kcalloc(count, sizeof(*chan_info), GFP_KERNEL); + if (!chan_info) { rc = -ENOMEM; goto err_free_mbox; } - pcc_doorbell_ack_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL); - if (!pcc_doorbell_ack_vaddr) { - rc = -ENOMEM; - goto err_free_db_vaddr; - } - - pcc_doorbell_irq = kcalloc(count, sizeof(int), GFP_KERNEL); - if (!pcc_doorbell_irq) { - rc = -ENOMEM; - goto err_free_db_ack_vaddr; - } - /* Point to the first PCC subspace entry */ pcct_entry = (struct acpi_subtable_header *) ( (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct)); @@ -501,6 +504,7 @@ static int __init acpi_pcc_probe(void) pcc_mbox_ctrl.txdone_irq = true; for (i = 0; i < count; i++) { + struct pcc_chan_info *pchan = chan_info + i; struct acpi_generic_address *db_reg; struct acpi_pcct_subspace *pcct_ss; pcc_mbox_channels[i].con_priv = pcct_entry; @@ -522,8 +526,8 @@ static int __init acpi_pcc_probe(void) /* If doorbell is in system memory cache the virt address */ db_reg = &pcct_ss->doorbell_register; if (db_reg->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) - pcc_doorbell_vaddr[i] = acpi_os_ioremap(db_reg->address, - db_reg->bit_width/8); + pchan->db_vaddr = acpi_os_ioremap(db_reg->address, + db_reg->bit_width / 8); pcct_entry = (struct acpi_subtable_header *) ((unsigned long) pcct_entry + pcct_entry->length); } @@ -535,11 +539,7 @@ static int __init acpi_pcc_probe(void) return 0; err: - kfree(pcc_doorbell_irq); -err_free_db_ack_vaddr: - kfree(pcc_doorbell_ack_vaddr); -err_free_db_vaddr: - kfree(pcc_doorbell_vaddr); + kfree(chan_info); err_free_mbox: kfree(pcc_mbox_channels); err_put_pcct:
Currently all the PCC channel specific information are stored/maintained in global individual arrays for each of those information. It is not scalable and not clean if we have to stash more channel specific information. Couple of reasons to stash more information are to extend the support to Type 3/4 PCCT subspace and also to avoid accessing the PCCT table entries themselves each time we need the information. This patch moves all those PCC channel specific information into a separate structure pcc_chan_info. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/mailbox/pcc.c | 106 +++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 53 deletions(-) -- 2.25.1