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.
+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.
+/**
- 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.
u16 cmd_delay = pcct_ss->min_turnaround_time;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.
& 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.
+/**
- 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.
/* 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.
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.
Thanks, Ashwin