On Tue, May 29, 2018 at 2:12 AM, Samarth Parikh <samarth.par...@arm.com> wrote:
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> index 4971f03..1633535 100644
> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt
>
>From the documentation you shared, MHUv2 is quite a different
controller. Please create separate bindings document.
> +
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/amba/bus.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/of_device.h>
> +#include <linux/of_address.h>
> +
Can we trim the list ?
> +#define MHU_V2_REG_STAT_OFS 0x0
> +#define MHU_V2_REG_CLR_OFS 0x8
> +#define MHU_V2_REG_SET_OFS 0xC
> +#define MHU_V2_REG_MSG_NO_CAP_OFS 0xF80
> +#define MHU_V2_REG_ACC_REQ_OFS 0xF88
> +#define MHU_V2_REG_ACC_RDY_OFS 0xF8C
> +
> +#define MHU_V2_LP_OFFSET 0x20
> +#define MHU_V2_HP_OFFSET 0x0
> +#define MHU_V2_CHANS 2
> +
The documentation says there can be upto 124 channels, actual value
reflected in MHU_V2_REG_MSG_NO_CAP_OFS.
And there is no priority of any channel really.
> +#define mbox_to_arm_mhuv2(c) container_of(c, struct arm_mhuv2, mbox)
> +
> +struct mhuv2_link {
> + unsigned int irq;
> + void __iomem *tx_reg;
> + void __iomem *rx_reg;
> +};
> +
TX and RX are independent channels, and may be different in number. So
maybe have only 'reg' and a 'direction' flag. Accordingly, update the
bindings.
> +
> +static int mhuv2_probe(struct amba_device *adev, const struct amba_id *id)
> +{
> + int i, err;
> + struct arm_mhuv2 *mhuv2;
> + struct device *dev = &adev->dev;
> + void __iomem *rx_base, *tx_base;
> + const struct device_node *np = dev->of_node;
> + unsigned int pchans;
> + int mhuv2_reg[MHU_V2_CHANS] = {MHU_V2_LP_OFFSET, MHU_V2_HP_OFFSET};
> +
> + /* Allocate memory for device */
> + mhuv2 = devm_kzalloc(dev, sizeof(*mhuv2), GFP_KERNEL);
> + if (!mhuv2)
> + return -ENOMEM;
> +
> + rx_base = of_iomap((struct device_node *)np, 0);
> + if (!rx_base) {
> + dev_err(dev, "failed to map rx registers\n");
> + return -ENOMEM;
> + }
> +
> + tx_base = of_iomap((struct device_node *)np, 1);
> + if (!tx_base) {
> + dev_err(dev, "failed to map tx registers\n");
> + iounmap(rx_base);
> + return -ENOMEM;
> + }
> +
We don't have to fix the order of TX and RX offsets.
The PID[2] is 0x4 for TX and 0xb for RX. Which is more reliable and flexible.
> + pchans = readl_relaxed(tx_base + MHU_V2_REG_MSG_NO_CAP_OFS);
>
There should be also
rxpchans = readl_relaxed(rx_base + MHU_V2_REG_MSG_NO_CAP_OFS);
because the spec has a separate MHU_V2_REG_MSG_NO_CAP_OFS for tx and rx regions.
Cheers!
