Re: [PATCH v3] mailbox: arm_mhu: add support for mhuv2

[Jassi Brar]

On Tue, May 29, 2018 at 2:12 AM, Samarth Parikh <samarth.parikh@xxxxxxx> 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!