Hi Ashwin,
On 11/11/14 13:18, Ashwin Chaugule wrote:
On 11 November 2014 05:30, Sudeep Holla sudeep.holla@arm.com wrote:
On 07/11/14 14:52, Ashwin Chaugule wrote:
+config PCC
bool "Platform Communication Channel Driver"depends on ACPII am bit confused here, though I prefer PCC to depend on ACPI, I have seen discussion in this thread about using PCC without ACPI, how will that work ?
Been discussed early on. DT users, if at all, will need to come up with their own DT bindings to make this work.
Thanks understood, I had not followed this thread earlier closely, so asked to get clarified.
+static struct mbox_chan pcc_mbox_chan[MAX_PCC_SUBSPACES];
Really, do you want to allocate 256 structures of mbox_chan even on systems with just 1 - 2 channels(typically that would be the case) ?
Spec says, max of 256. Easier to support it this way.
I tend to disagree, you can allocate dynamically. And use exactly same logic you use in get_subspace_id to populate con_priv later. When you parse PCC Subspace, you can just validate header and not record them in pcc_mbox_chan. You can get the exact count in acpi_table_parse_entries and then do the required allocation.
+/**
- pcc_tx_done - Callback from Mailbox controller code to
check if PCC message transmission completed.
- @chan: Pointer to Mailbox channel on which previous
transmission occurred.
- Return: TRUE if succeeded.
- */
+static bool pcc_tx_done(struct mbox_chan *chan) +{
struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;struct acpi_pcct_shared_memory *generic_comm_base =(struct acpi_pcct_shared_memory *) pcct_ss->base_address;IIUC, the PCCT table has the physical base Address of the shared memory range in the PCC subspace structures. Can you use that directly here ? Or are you mapping that memory elsewhere and updating the mapped address to the table ?
In the client side. Its really owned by the client. pcc_tx_done can never be called on a channel which doesn't have a mapped base address.
Yes but I don't like the way client has to read pcct_ss, read the physical address, map it and replace it in the table before it's used here. That could be issue especially for async notification when the client is not ready. If all the shared memory access happens in one place where it get's mapped, we can avoid such issues.
u16 cmd_delay = pcct_ss->min_turnaround_time;
I missed this last time, you should not use this here. As per the ACPI spec, it is minimum amount of time that OSPM must wait after the completion of a command before issuing the next command. So you need this at the end of the function. IIUC you need to use nominal latency instead here.
unsigned int retries = 0;/* Try a few times while waiting for platform to consume */while (!(readw_relaxed(&generic_comm_base->status)This will explode if the pcct_ss->base_address is not mapped.
It is. Client side. Called only _after_ client is done with its thing.
Yes for now, once you add interrupt handling that may not be true as I mentioned earlier.
& PCC_CMD_COMPLETE)) {if (retries++ < 5)udelay(cmd_delay);else {/** If the remote is dead, this will cause the Mbox* controller to timeout after mbox client.tx_tout* msecs.*/pr_err("PCC platform did not respond.\n");return false;}}return true;+}
In general you can isolate all the access to the shared memory in the protocol layer. In that case you might have to use mbox_client_txdone instead of this pcc_tx_done.
I dont see the problem in keeping this way though.
For the reasons mentioned above, I prefer isolating the access to shared memory region from the control part(this driver).
+/**
- pcc_send_data - Called from Mailbox Controller code to finally
transmit data over channel.
- @chan: Pointer to Mailbox channel over which to send data.
- @data: Actual data to be written over channel.
- Return: Err if something failed else 0 for success.
- */
+static int pcc_send_data(struct mbox_chan *chan, void *data) +{
struct acpi_pcct_hw_reduced *pcct_ss = chan->con_priv;struct acpi_pcct_shared_memory *generic_comm_base =(struct acpi_pcct_shared_memory *) pcct_ss->base_address;struct acpi_generic_address doorbell;u64 doorbell_preserve;u64 doorbell_val;u64 doorbell_write;u16 cmd = *(u16 *) data;u16 ss_idx = -1;ss_idx = get_subspace_id(chan);if (ss_idx < 0) {pr_err("Invalid Subspace ID from PCC client\n");return -EINVAL;}doorbell = pcct_ss->doorbell_register;doorbell_preserve = pcct_ss->preserve_mask;doorbell_write = pcct_ss->write_mask;/* Write to the shared comm region. */writew(cmd, &generic_comm_base->command);/* Write Subspace MAGIC value so platform can identifydestination. */
writel((PCCS_SS_SIG_MAGIC | ss_idx),&generic_comm_base->signature);
/* Flip CMD COMPLETE bit */writew(0, &generic_comm_base->status);IMO it's not clean to modify only first 3 elements namely: Signature, Command and Status while the Communication Space is updated elsewhere. It's better to have all of them in one place if possible as mentioned above.
The access sequence for first 3 elements is the same for each channel. Thats all the controller is expected to do on behalf of the client. So this function reduces code duplication in all client sites.
You can have a helper or a macro to avoid duplication.
/* Sync notification from OSPM to Platform. */acpi_read(&doorbell_val, &doorbell);acpi_write((doorbell_val & doorbell_preserve) | doorbell_write,&doorbell);Only the above 2 must be part of the controller driver, the remaining as I mentioned can be move to the protocol/client layer, thoughts ?
I'd really prefer for it to be separate. Once client is done writing, it calls this and lets the controller driver take over the rest. Its not racy.
I think otherwise, so would like to see others' opinion on this :)
count = acpi_table_parse_entries(ACPI_SIG_PCCT,sizeof(struct acpi_table_pcct),s/struct acpi_table_pcct/*pcct_tbl/
In general, the interrupt part of the PCCT SS is not considered in this patch. It's better to see/discuss how that can be fit into the mailbox framework, though it could be follow up patch.
The IRQ part of the spec seems to be under discussion (single irq per subspace / common IRQ across all) and as you may be aware we're working on trying it out on Juno. That'll guide the design. What I have here is good enough to start off with and has been tested. I dont think we should have a problem using the mailbox API for asyn tx though, but I'd really^n prefer if we get something out there first.
That's the different and still under discussion. But you need to support Type 1 subspace as it stands in ACPI v5.1
Regards, Sudeep