On Wednesday 03 September 2014 18:19:19 Ashwin Chaugule wrote:
> ACPI 5.0+ spec defines a generic mode of communication
> between the OS and a platform such as the BMC. This medium
> (PCC) is typically used by CPPC (ACPI CPU Performance management),
> RAS (ACPI reliability protocol) and MPST (ACPI Memory power
> states).
Ok, the interaction with the mailbox subsystem looks much nicer now!
Some comments on the actual driver:
> +struct mbox_chan *pcc_mbox_request_channel(struct mbox_client *cl,
> + int index)
> +{
> + struct device *dev = pcc_mbox_ctrl.dev;
> + struct mbox_chan *chan;
> + unsigned long flags;
> +
> + /*
> + * Each PCC Subspace is a Mailbox Channel.
> + * The PCC Clients get their PCC Subspace ID
> + * from their own tables and pass it here.
> + * This returns a pointer to the PCC subspace
> + * for the Client to operate on.
> + */
> + chan = &pcc_mbox_chan[index];
> +
> + if (!chan || chan->cl || !try_module_get(dev->driver->owner)) {
> + dev_err(dev, "%s: PCC mailbox not free\n", __func__);
> + return ERR_PTR(-EBUSY);
> + }
The try_module_get is now pointless, since dev->driver->owner is
the current module.
> +static int get_subspace_id(struct mbox_chan *chan)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < pcc_mbox_ctrl.num_chans; i++) {
> + if (chan == &pcc_mbox_chan[i])
> + return i;
> + }
> +
> + return -ENOENT;
> +}
Isn't this the same as this?
int id = chan - pcc_mbox_chan;
if (id < 0 || id > pcc_mbox_ctrl.num_chans
return -ENOENT;
return id;
> +/* Channel lock is already held by mbox controller code. */
> +static int pcc_send_data(struct mbox_chan *chan, void *data)
> +{
> + struct acpi_pcct_subspace *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 = 0;
> + u16 ss_idx = -1;
> + int ret = 0;
> +
> + /* Get PCC CMD */
> + ret = kstrtou16((char*)data, 0, &cmd);
What is this supposed to do? Why would a client pass data as a string?
Just make it a u16 pointer and force the client to do the conversion
so you have a proper format.
Mailbox drivers should generally have a fixed-format mailbox data pointer.
> + /* Write to the shared comm region. */
> + iowrite16(cmd, &generic_comm_base->command);
> +
> + /* Write Subspace MAGIC value so platform can identify destination. */
> + iowrite32((PCCS_SS_SIG_MAGIC | ss_idx), &generic_comm_base->signature);
> +
> + /* Flip CMD COMPLETE bit */
> + iowrite16(0, &generic_comm_base->status);
using writel_relaxed/writew_relaxed should be more efficient here.
> + * Create a virtual device to back the PCCT, so
> + * that the generic Mailbox framework can do its
> + * ref counting.
> + */
> +struct platform_device pcc_pdev = {
> + .name = "PCCT",
> +};
Don't declare platform devices statically in C code, just
use platform_create_bundle() here to create the device dynamically
and bind it to the driver. That will also simplify the driver
initalization.
> diff --git a/include/linux/mailbox_controller.h b/include/linux/mailbox_controller.h
> index 9ee195b..956cc06 100644
> --- a/include/linux/mailbox_controller.h
> +++ b/include/linux/mailbox_controller.h
> @@ -15,6 +15,10 @@
>
> struct mbox_chan;
>
> +#define TXDONE_BY_IRQ BIT(0) /* controller has remote RTR irq */
> +#define TXDONE_BY_POLL BIT(1) /* controller can read status of last TX */
> +#define TXDONE_BY_ACK BIT(2) /* S/W ACK recevied by Client ticks the TX */
> +
> /**
> * struct mbox_chan_ops - methods to control mailbox channels
> * @send_data: The API asks the MBOX controller driver, in atomic
>
I don't think we want these in a global header file. Better put them in
a local header file in drivers/mailbox/.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
