diff mbox series

[10/13] mailbox: pcc: Avoid accessing PCCT table in pcc_send_data and pcc_mbox_irq

Message ID 20210708180851.2311192-11-sudeep.holla@arm.com (mailing list archive)
State Not Applicable, archived
Headers show
Series mailbox: pcc: Add support for PCCT extended PCC subspaces | expand

Commit Message

Sudeep Holla July 8, 2021, 6:08 p.m. UTC 
Now that the con_priv is availvale solely for PCC mailbox controller
driver, let us use the same to save the channel specific information
in it so that we can it whenever required instead of parsing the PCCT
table entries every time in both pcc_send_data and pcc_mbox_irq.

We can now use the newly introduces PCC register bundle to simplify both
saving of channel specific information and accessing them without repeated
checks for the subspace type.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/mailbox/pcc.c | 116 +++++++++++-------------------------------
 1 file changed, 30 insertions(+), 86 deletions(-)

Comments

Cristian Marussi July 14, 2021, 6:37 p.m. UTC | #1 
On Thu, Jul 08, 2021 at 07:08:48PM +0100, Sudeep Holla wrote:
> Now that the con_priv is availvale solely for PCC mailbox controller
nit: s/availvale/available

> driver, let us use the same to save the channel specific information
> in it so that we can it whenever required instead of parsing the PCCT
> table entries every time in both pcc_send_data and pcc_mbox_irq.
> 
> We can now use the newly introduces PCC register bundle to simplify both
> saving of channel specific information and accessing them without repeated
> checks for the subspace type.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

>  drivers/mailbox/pcc.c | 116 +++++++++++-------------------------------
>  1 file changed, 30 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
> index 7d48e5c1ac52..237dba9cb445 100644
> --- a/drivers/mailbox/pcc.c
> +++ b/drivers/mailbox/pcc.c
> @@ -86,15 +86,15 @@ struct pcc_chan_reg {
>   * struct pcc_chan_info - PCC channel specific information
>   *
>   * @chan: PCC channel information with Shared Memory Region info
> - * @db_vaddr: cached virtual address for doorbell register
> - * @plat_irq_ack_vaddr: cached virtual address for platform interrupt
> - *	acknowledge register
> + * @db: PCC register bundle for the doorbell register
> + * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
> + *	register
>   * @db_irq: doorbell interrupt
>   */
>  struct pcc_chan_info {
>  	struct pcc_mbox_chan chan;
> -	void __iomem *db_vaddr;
> -	void __iomem *plat_irq_ack_vaddr;
> +	struct pcc_chan_reg db;
> +	struct pcc_chan_reg plat_irq_ack;
>  	int db_irq;
>  };
>  
> @@ -242,40 +242,15 @@ static int pcc_map_interrupt(u32 interrupt, u32 flags)
>   */
>  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;
> -	u64 doorbell_ack_val;
> -	int ret;
>  
> -	pcct_ss = chan->con_priv;
> +	pchan = chan->con_priv;
>  
>  	mbox_chan_received_data(chan, NULL);
>  
> -	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
> -		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(pchan->plat_irq_ack_vaddr,
> -				    &doorbell_ack_val, doorbell_ack->bit_width);
> -		if (ret)
> -			return IRQ_NONE;
> -
> -		ret = write_register(pchan->plat_irq_ack_vaddr,
> -				     (doorbell_ack_val & doorbell_ack_preserve)
> -					| doorbell_ack_write,
> -				     doorbell_ack->bit_width);
> -		if (ret)
> -			return IRQ_NONE;
> -	}
> +	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
> +		return IRQ_NONE;
>  
>  	return IRQ_HANDLED;
>  }
> @@ -376,42 +351,9 @@ EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
>   */
>  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;
> -	u32 id = chan - pcc_mbox_channels;
> -	int ret = 0;
> -
> -	if (id >= pcc_mbox_ctrl.num_chans) {
> -		pr_debug("pcc_send_data: Invalid mbox_chan passed\n");
> -		return -ENOENT;
> -	}
> +	struct pcc_chan_info *pchan = chan->con_priv;
>  
> -	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 (pchan->db_vaddr) {
> -		ret = read_register(pchan->db_vaddr, &doorbell_val,
> -				    doorbell->bit_width);
> -		if (ret)
> -			return ret;
> -		ret = write_register(pchan->db_vaddr,
> -				     (doorbell_val & doorbell_preserve)
> -				      | doorbell_write, doorbell->bit_width);
> -	} else {
> -		ret = acpi_read(&doorbell_val, doorbell);
> -		if (ret)
> -			return ret;
> -		ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
> -			doorbell);
> -	}
> -	return ret;
> +	return pcc_chan_reg_read_modify_write(&pchan->db);
>  }
>  
>  static const struct mbox_chan_ops pcc_chan_ops = {
> @@ -479,6 +421,7 @@ pcc_chan_reg_init(struct pcc_chan_reg *reg, struct acpi_generic_address *gas,
>  static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>  				  struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
>  	struct acpi_pcct_hw_reduced *pcct_ss;
>  
>  	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
> @@ -497,16 +440,14 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>  	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
>  		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
>  
> -		pchan->plat_irq_ack_vaddr =
> -			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
> -					pcct2_ss->platform_ack_register.bit_width / 8);
> -		if (!pchan->plat_irq_ack_vaddr) {
> -			pr_err("Failed to ioremap PCC ACK register\n");
> -			return -ENOMEM;
> -		}
> +		ret = pcc_chan_reg_init(&pchan->plat_irq_ack,
> +					&pcct2_ss->platform_ack_register,
> +					pcct2_ss->ack_preserve_mask,
> +					pcct2_ss->ack_write_mask, 0,
> +					"PLAT IRQ ACK");
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -516,19 +457,20 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>   * @pcct_entry: Pointer to the ACPI subtable header.
>   *
>   */
> -static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> -				      struct acpi_subtable_header *pcct_entry)
> +static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> +				     struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
> +
>  	struct acpi_pcct_subspace *pcct_ss;
> -	struct acpi_generic_address *db_reg;
>  
>  	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
>  
> -	/* 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)
> -		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
> -						  db_reg->bit_width / 8);
> +	ret = pcc_chan_reg_init(&pchan->db,
> +				&pcct_ss->doorbell_register,
> +				pcct_ss->preserve_mask,
> +				pcct_ss->write_mask, 0,	"Doorbell");
> +	return ret;
>  }
>  
>  /**
> @@ -617,8 +559,8 @@ static int __init acpi_pcc_probe(void)
>  
>  	for (i = 0; i < count; i++) {
>  		struct pcc_chan_info *pchan = chan_info + i;
> -		pcc_mbox_channels[i].con_priv = pcct_entry;
>  
> +		pcc_mbox_channels[i].con_priv = pchan;
>  		pchan->chan.mchan = &pcc_mbox_channels[i];
>  
>  		if (pcc_mbox_ctrl.txdone_irq) {
> @@ -626,7 +568,9 @@ static int __init acpi_pcc_probe(void)
>  			if (rc < 0)
>  				goto err;
>  		}
> -		pcc_parse_subspace_db_reg(pchan, pcct_entry);
> +		rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
> +		if (rc < 0)
> +			goto err;
>  
>  		pcc_parse_subspace_shmem(pchan, pcct_entry);
>  
> -- 
> 2.25.1
>
Cristian Marussi July 15, 2021, 1:38 p.m. UTC | #2 
On Thu, Jul 08, 2021 at 07:08:48PM +0100, Sudeep Holla wrote:
> Now that the con_priv is availvale solely for PCC mailbox controller
> driver, let us use the same to save the channel specific information
> in it so that we can it whenever required instead of parsing the PCCT
> table entries every time in both pcc_send_data and pcc_mbox_irq.
> 
> We can now use the newly introduces PCC register bundle to simplify both
> saving of channel specific information and accessing them without repeated
> checks for the subspace type.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---o
[snip]
>

Sorry missed this small doxygen break along the way early:

>  /**
> @@ -516,19 +457,20 @@ static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
>   * @pcct_entry: Pointer to the ACPI subtable header.
>   *
>   */
> -static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> -				      struct acpi_subtable_header *pcct_entry)
> +static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
> +				     struct acpi_subtable_header *pcct_entry)
>  {
> +	int ret = 0;
> +


drivers/mailbox/pcc.c:485: warning: No description found for return value of 'pcc_parse_subspace_db_reg'

Thanks,
Cristian
diff mbox series

Patch

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c
index 7d48e5c1ac52..237dba9cb445 100644
--- a/drivers/mailbox/pcc.c
+++ b/drivers/mailbox/pcc.c
@@ -86,15 +86,15 @@  struct pcc_chan_reg {
  * struct pcc_chan_info - PCC channel specific information
  *
  * @chan: PCC channel information with Shared Memory Region info
- * @db_vaddr: cached virtual address for doorbell register
- * @plat_irq_ack_vaddr: cached virtual address for platform interrupt
- *	acknowledge register
+ * @db: PCC register bundle for the doorbell register
+ * @plat_irq_ack: PCC register bundle for the platform interrupt acknowledge
+ *	register
  * @db_irq: doorbell interrupt
  */
 struct pcc_chan_info {
 	struct pcc_mbox_chan chan;
-	void __iomem *db_vaddr;
-	void __iomem *plat_irq_ack_vaddr;
+	struct pcc_chan_reg db;
+	struct pcc_chan_reg plat_irq_ack;
 	int db_irq;
 };
 
@@ -242,40 +242,15 @@  static int pcc_map_interrupt(u32 interrupt, u32 flags)
  */
 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;
-	u64 doorbell_ack_val;
-	int ret;
 
-	pcct_ss = chan->con_priv;
+	pchan = chan->con_priv;
 
 	mbox_chan_received_data(chan, NULL);
 
-	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
-		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(pchan->plat_irq_ack_vaddr,
-				    &doorbell_ack_val, doorbell_ack->bit_width);
-		if (ret)
-			return IRQ_NONE;
-
-		ret = write_register(pchan->plat_irq_ack_vaddr,
-				     (doorbell_ack_val & doorbell_ack_preserve)
-					| doorbell_ack_write,
-				     doorbell_ack->bit_width);
-		if (ret)
-			return IRQ_NONE;
-	}
+	if (pcc_chan_reg_read_modify_write(&pchan->plat_irq_ack))
+		return IRQ_NONE;
 
 	return IRQ_HANDLED;
 }
@@ -376,42 +351,9 @@  EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);
  */
 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;
-	u32 id = chan - pcc_mbox_channels;
-	int ret = 0;
-
-	if (id >= pcc_mbox_ctrl.num_chans) {
-		pr_debug("pcc_send_data: Invalid mbox_chan passed\n");
-		return -ENOENT;
-	}
+	struct pcc_chan_info *pchan = chan->con_priv;
 
-	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 (pchan->db_vaddr) {
-		ret = read_register(pchan->db_vaddr, &doorbell_val,
-				    doorbell->bit_width);
-		if (ret)
-			return ret;
-		ret = write_register(pchan->db_vaddr,
-				     (doorbell_val & doorbell_preserve)
-				      | doorbell_write, doorbell->bit_width);
-	} else {
-		ret = acpi_read(&doorbell_val, doorbell);
-		if (ret)
-			return ret;
-		ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
-			doorbell);
-	}
-	return ret;
+	return pcc_chan_reg_read_modify_write(&pchan->db);
 }
 
 static const struct mbox_chan_ops pcc_chan_ops = {
@@ -479,6 +421,7 @@  pcc_chan_reg_init(struct pcc_chan_reg *reg, struct acpi_generic_address *gas,
 static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 				  struct acpi_subtable_header *pcct_entry)
 {
+	int ret = 0;
 	struct acpi_pcct_hw_reduced *pcct_ss;
 
 	if (pcct_entry->type < ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE ||
@@ -497,16 +440,14 @@  static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
 	if (pcct_ss->header.type == ACPI_PCCT_TYPE_HW_REDUCED_SUBSPACE_TYPE2) {
 		struct acpi_pcct_hw_reduced_type2 *pcct2_ss = (void *)pcct_ss;
 
-		pchan->plat_irq_ack_vaddr =
-			acpi_os_ioremap(pcct2_ss->platform_ack_register.address,
-					pcct2_ss->platform_ack_register.bit_width / 8);
-		if (!pchan->plat_irq_ack_vaddr) {
-			pr_err("Failed to ioremap PCC ACK register\n");
-			return -ENOMEM;
-		}
+		ret = pcc_chan_reg_init(&pchan->plat_irq_ack,
+					&pcct2_ss->platform_ack_register,
+					pcct2_ss->ack_preserve_mask,
+					pcct2_ss->ack_write_mask, 0,
+					"PLAT IRQ ACK");
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -516,19 +457,20 @@  static int pcc_parse_subspace_irq(struct pcc_chan_info *pchan,
  * @pcct_entry: Pointer to the ACPI subtable header.
  *
  */
-static void pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
-				      struct acpi_subtable_header *pcct_entry)
+static int pcc_parse_subspace_db_reg(struct pcc_chan_info *pchan,
+				     struct acpi_subtable_header *pcct_entry)
 {
+	int ret = 0;
+
 	struct acpi_pcct_subspace *pcct_ss;
-	struct acpi_generic_address *db_reg;
 
 	pcct_ss = (struct acpi_pcct_subspace *)pcct_entry;
 
-	/* 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)
-		pchan->db_vaddr = acpi_os_ioremap(db_reg->address,
-						  db_reg->bit_width / 8);
+	ret = pcc_chan_reg_init(&pchan->db,
+				&pcct_ss->doorbell_register,
+				pcct_ss->preserve_mask,
+				pcct_ss->write_mask, 0,	"Doorbell");
+	return ret;
 }
 
 /**
@@ -617,8 +559,8 @@  static int __init acpi_pcc_probe(void)
 
 	for (i = 0; i < count; i++) {
 		struct pcc_chan_info *pchan = chan_info + i;
-		pcc_mbox_channels[i].con_priv = pcct_entry;
 
+		pcc_mbox_channels[i].con_priv = pchan;
 		pchan->chan.mchan = &pcc_mbox_channels[i];
 
 		if (pcc_mbox_ctrl.txdone_irq) {
@@ -626,7 +568,9 @@  static int __init acpi_pcc_probe(void)
 			if (rc < 0)
 				goto err;
 		}
-		pcc_parse_subspace_db_reg(pchan, pcct_entry);
+		rc = pcc_parse_subspace_db_reg(pchan, pcct_entry);
+		if (rc < 0)
+			goto err;
 
 		pcc_parse_subspace_shmem(pchan, pcct_entry);