Hi Prashanth and Ashwin,

On Sat, Jan 23, 2016 at 1:07 AM, Prashanth Prakash pprakash@codeaurora.org wrote:

From: Ashwin Chaugule ashwin.chaugule@linaro.org

Previously the send_pcc_cmd() code checked if the PCC operation had completed before returning from the function. This check was performed regardless of the PCC op type (i.e. Read/Write). Knowing the type of cmd can be used to optimize the check and avoid needless waiting. e.g. with Write ops, the actual Writing is done before calling send_pcc_cmd(). And the subsequent Writes will check if the channel is free at the entry of send_pcc_cmd() anyway.

However, for Read cmds, we need to wait for the cmd completion bit to be flipped, since the actual Read ops follow after returning from the send_pcc_cmd(). So, only do the looping check at the end for Read ops.

Also, instead of using udelay() calls, use ktime as a means to check for deadlines. The current deadline

udelay() is still there. Well, proposed approach in this patch is better than current code but I will be much more happy to see approach without delay()s under spin_lock if such approach exists.

in which the Remote should flip the cmd completion bit is defined as N * Nominal latency. Where N is arbitrary and large enough to work on slow emulators and Nominal latency comes from the ACPI table (PCCT). This helps in working around the CONFIG_HZ effects on udelay() and also avoids needing different ACPI tables for Silicon and Emulation platforms.

What's the logic behind choosing N? Is it defined as 500 currently? Can it be some kind of configurable module or boot parameter? What if nominal latency given by firmware will be 100 mS?

Frankly speaking, I personally expect that slow emulator is going to fail timing-sensitive activities and that's fine. Your requirement to use same tables for emulator and hardware is understandable too.

Signed-off-by: Ashwin Chaugule ashwin.chaugule@linaro.org Signed-off-by: Prashanth Prakash pprakash@codeaurora.org

---

drivers/acpi/cppc_acpi.c | 102 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 28 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c index 6730f96..36c3e4d 100644 --- a/drivers/acpi/cppc_acpi.c +++ b/drivers/acpi/cppc_acpi.c @@ -39,6 +39,7 @@

#include <linux/cpufreq.h> #include <linux/delay.h> +#include <linux/ktime.h>

#include <acpi/cppc_acpi.h> /* @@ -63,25 +64,56 @@ static struct mbox_chan *pcc_channel; static void __iomem *pcc_comm_addr; static u64 comm_base_addr; static int pcc_subspace_idx = -1; -static u16 pcc_cmd_delay; static bool pcc_channel_acquired; +static ktime_t deadline;

/*

• Arbitrary Retries in case the remote processor is slow to respond

• • to PCC commands.

• • to PCC commands. Keeping it high enough to cover emulators where
• • the processors run painfully slow.
  */

#define NUM_RETRIES 500

+static int check_pcc_chan(void) +{

•   int result = -EIO;
  

•   struct acpi_pcct_shared_memory *generic_comm_base =
  

•           (struct acpi_pcct_shared_memory *) pcc_comm_addr;
  

•   ktime_t next_deadline = ktime_add(ktime_get(), deadline);
  

•   int retries = 0;
  

•   /* Retry in case the remote processor was too slow to catch up. */
  

•   while (!ktime_after(ktime_get(), next_deadline)) {
  

•           if (readw_relaxed(&generic_comm_base->status) & PCC_CMD_COMPLETE) {
  

•                   result = 0;
  

•                   break;
  

•           }
  

•           /*
  

•            * Reducing the bus traffic in case this loop takes longer than
  

•            * a few retries.
  

•            */
  

•           udelay(3);
  

•           retries++;
  

Looks like retries variable is not used properly. You just increment it and that's all. Could you please check its usage or remove it?

•   }
  

•   return result;
  

+}

static int send_pcc_cmd(u16 cmd) {

•   int retries, result = -EIO;
  

•   struct acpi_pcct_hw_reduced *pcct_ss = pcc_channel->con_priv;
  

•   int ret = -EIO;
    struct acpi_pcct_shared_memory *generic_comm_base =
            (struct acpi_pcct_shared_memory *) pcc_comm_addr;
  

•   u32 cmd_latency = pcct_ss->latency;
  
  

•   /* Min time OS should wait before sending next command. */
  

•   udelay(pcc_cmd_delay);
  

•   /*
  

•    * For CMD_WRITE we know for a fact the caller should have checked
  

•    * the channel before writing to PCC space
  

•    */
  

•   if (cmd == CMD_READ) {
  

•           ret = check_pcc_chan();
  

•           if (ret)
  

•                   return ret;
  

•   }
  

For my eyes first pcc command to be called during booting is send_pcc_cmd(CMD_READ) from cppc_get_perf_caps(). That means we go to the loop inside check_pcc_chan() and to break that loop and successfully continue software expects bit PCC_CMD_COMPLETE to be set. I checked ACPI specs and didn't find any words about reset value of status register here. If it's there could you please point me to it and place comment here? Or you shouldn't rely on reset value of status reg (let's imagine that we rebooted kernel without informing firmware). I actually hit this bug when I started to test your patches.

[snip]

Anyway, I tested your patches and ready to give reviewed-and-tested-by after resolving current issues (and after re-testing). So, could you please keep me in c/c in next version?

Thanks, Alexey.

On 25 January 2016 at 12:34, Timur Tabi timur@codeaurora.org wrote:

On Fri, Jan 22, 2016 at 7:07 PM, Prashanth Prakash pprakash@codeaurora.org wrote:

...

pcc_send_data can be invoked during the execution of performance critical code as in cppc_cpufreq driver. With acpi_* APIs, the doorbell register accessed in pcc_send_data if present in system memory will be searched (in cached virt to phys addr mapping), mapped, read/written and then unmapped. These operations take significant amount of time.

This patch maps the performance critical doorbell register during init and then reads/writes to it directly using the mapped virtual address. This patch + similar changes to CPPC acpi driver reduce the time per freq. transition from around 200us to about 20us for cppc cpufreq driver

Signed-off-by: Prashanth Prakash pprakash@codeaurora.org Acked-by: Ashwin Chaugule ashwin.chaugule@linaro.org

---

drivers/mailbox/pcc.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 104 insertions(+), 7 deletions(-)

diff --git a/drivers/mailbox/pcc.c b/drivers/mailbox/pcc.c index 45d85ae..050c206 100644 --- a/drivers/mailbox/pcc.c +++ b/drivers/mailbox/pcc.c @@ -63,6 +63,7 @@ #include <linux/platform_device.h> #include <linux/mailbox_controller.h> #include <linux/mailbox_client.h> +#include <linux/io-64-nonatomic-lo-hi.h>

#include "mailbox.h"

@@ -70,6 +71,9 @@

static struct mbox_chan *pcc_mbox_channels;

+/*Array of cached virtual address for doorbell registers*/ +static void **pcc_doorbell_vaddr;

I think this needs to be

static void __iomem **pcc_doorbell_vaddr.

You really should use the 'sparse' tool on your patches to make sure that you have correct __iomem usage. Pointers that are passed to readl() and writel() should be __iomem pointers, and they should be allocated with ioremap(). However, I noticed that you're not doing that:

...

static struct mbox_controller pcc_mbox_ctrl = {}; /**

• get_pcc_channel - Given a PCC subspace idx, get

@@ -166,6 +170,66 @@ void pcc_mbox_free_channel(struct mbox_chan *chan) } EXPORT_SYMBOL_GPL(pcc_mbox_free_channel);

+/*

• • PCC can be used with perf critical drivers such as CPPC
• • So it makes sense to locally cache the virtual address and
• • use it to read/write to PCC registers such as doorbell register
• • The below read_register and write_registers are used to read and
• • write from perf critical registers such as PCC doorbell register
• */

+static int read_register(void *vaddr, u64 *val, unsigned int bit_width)

Once again, this should be "void __iomem *vaddr".

...

+{

•   int ret_val = 0;
  

•   switch (bit_width) {
  

•   case 8:
  

•           *val = readb(vaddr);
  

Remember, functions like readb(), writeb(), readl(), writel(), etc, should only be used on pointers allocated via ioremap(), however ...

...

•           break;
  

•   case 16:
  

•           *val = readw(vaddr);
  

•           break;
  

•   case 32:
  

•           *val = readl(vaddr);
  

•           break;
  

•   case 64:
  

•           *val = readq(vaddr);
  

•           break;
  

•   default:
  

•           pr_debug("Error: Cannot read register of %u bit width",
  

•                   bit_width);
  

•           ret_val = -EFAULT;
  

•           break;
  

•   }
  

•   return ret_val;
  

+}

+static int write_register(void *vaddr, u64 val, unsigned int bit_width) +{

•   int ret_val = 0;
  

•   switch (bit_width) {
  

•   case 8:
  

•           writeb(val, vaddr);
  

•           break;
  

•   case 16:
  

•           writew(val, vaddr);
  

•           break;
  

•   case 32:
  

•           writel(val, vaddr);
  

•           break;
  

•   case 64:
  

•           writeq(val, vaddr);
  

•           break;
  

•   default:
  

•           pr_debug("Error: Cannot write register of %u bit width",
  

•                   bit_width);
  

•           ret_val = -EFAULT;
  

•           break;
  

•   }
  

•   return ret_val;
  

+}

/**

• pcc_send_data - Called from Mailbox Controller code. Used

•         here only to ring the channel doorbell. The PCC client
  

@@ -181,21 +245,39 @@ 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 acpi_generic_address *doorbell;
    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;
  

•   }
  
  

•   doorbell = pcct_ss->doorbell_register;
  

•   doorbell = &pcct_ss->doorbell_register;
    doorbell_preserve = pcct_ss->preserve_mask;
    doorbell_write = pcct_ss->write_mask;
  
    /* Sync notification from OS to Platform. */
  

•   acpi_read(&doorbell_val, &doorbell);
  

•   acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
  

•                   &doorbell);
  

•   return 0;
  

•   if (pcc_doorbell_vaddr[id]) {
  

•           ret = read_register(pcc_doorbell_vaddr[id], &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);
  

•   } else {
  

•           ret = acpi_read(&doorbell_val, doorbell);
  

•           if (ret)
  

•                   return ret;
  

•           ret = acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,
  

•                   doorbell);
  

•   }
  

•   return ret;
  

}

static const struct mbox_chan_ops pcc_chan_ops = { @@ -271,14 +353,29 @@ static int __init acpi_pcc_probe(void) return -ENOMEM; }

•   pcc_doorbell_vaddr = kcalloc(count, sizeof(void *), GFP_KERNEL);
  

... this is wrong. You're using readl() and writel() on this pointer, but it's not an iomem pointer, it's normal kernel memory. You need to delete your usage of readX, writeX on this pointer.

The acpi_os_ioremap() below should take care of that.

...

•   if (!pcc_doorbell_vaddr) {
  

•           kfree(pcc_mbox_channels);
  

•           return -ENOMEM;
  

•   }
  

•   /* Point to the first PCC subspace entry */
    pcct_entry = (struct acpi_subtable_header *) (
            (unsigned long) pcct_tbl + sizeof(struct acpi_table_pcct));
  
    for (i = 0; i < count; i++) {
  

•           struct acpi_generic_address *db_reg;
  

•           struct acpi_pcct_hw_reduced *pcct_ss;
            pcc_mbox_channels[i].con_priv = pcct_entry;
            pcct_entry = (struct acpi_subtable_header *)
                    ((unsigned long) pcct_entry + pcct_entry->length);
  

•           /* If doorbell is in system memory cache the virt address */
  

•           pcct_ss = (struct acpi_pcct_hw_reduced *)pcct_entry;
  

•           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);
    }
  
    pcc_mbox_ctrl.num_chans = count;
  

Ashwin