On Mon, Sep 02, 2019 at 11:54:45AM +0800, JC Kuo wrote: > Tegra194 XUSB host controller has rearranged mailbox registers. This > commit makes mailbox registers address a part of "soc" data so that > xhci-tegra driver can be used for Tegra194. > > Signed-off-by: JC Kuo <jc...@nvidia.com> > --- > drivers/usb/host/xhci-tegra.c | 51 ++++++++++++++++++++++++++--------- > 1 file changed, 39 insertions(+), 12 deletions(-)
I'd perhaps reformulate the subject a little to be something like: xhci: tegra: Parameterize mailbox register addresses Two other minor comments below. > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index dafc65911fc0..b92a03bbbd2c 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -146,6 +146,13 @@ struct tegra_xusb_phy_type { > unsigned int num; > }; > > +struct tega_xusb_mbox_regs { > + u32 cmd; > + u32 data_in; > + u32 data_out; > + u32 owner; > +}; Perhaps make these unsigned int (or unsigned long). Making these explicitly sized variables suggests (at least to me) that they are register values, whereas they really are offsets. So I prefer to use "unsized" types to distinguish between register offsets and values. > + > struct tegra_xusb_soc { > const char *firmware; > const char * const *supply_names; > @@ -160,6 +167,8 @@ struct tegra_xusb_soc { > } usb2, ulpi, hsic, usb3; > } ports; > > + struct tega_xusb_mbox_regs mbox; > + > bool scale_ss_clock; > bool has_ipfs; > }; > @@ -395,15 +404,15 @@ static int tegra_xusb_mbox_send(struct tegra_xusb > *tegra, > * ACK/NAK messages. > */ > if (!(msg->cmd == MBOX_CMD_ACK || msg->cmd == MBOX_CMD_NAK)) { > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER); > + value = fpci_readl(tegra, tegra->soc->mbox.owner); > if (value != MBOX_OWNER_NONE) { > dev_err(tegra->dev, "mailbox is busy\n"); > return -EBUSY; > } > > - fpci_writel(tegra, MBOX_OWNER_SW, XUSB_CFG_ARU_MBOX_OWNER); > + fpci_writel(tegra, MBOX_OWNER_SW, tegra->soc->mbox.owner); > > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER); > + value = fpci_readl(tegra, tegra->soc->mbox.owner); > if (value != MBOX_OWNER_SW) { > dev_err(tegra->dev, "failed to acquire mailbox\n"); > return -EBUSY; > @@ -413,17 +422,17 @@ static int tegra_xusb_mbox_send(struct tegra_xusb > *tegra, > } > > value = tegra_xusb_mbox_pack(msg); > - fpci_writel(tegra, value, XUSB_CFG_ARU_MBOX_DATA_IN); > + fpci_writel(tegra, value, tegra->soc->mbox.data_in); > > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_CMD); > + value = fpci_readl(tegra, tegra->soc->mbox.cmd); > value |= MBOX_INT_EN | MBOX_DEST_FALC; > - fpci_writel(tegra, value, XUSB_CFG_ARU_MBOX_CMD); > + fpci_writel(tegra, value, tegra->soc->mbox.cmd); > > if (wait_for_idle) { > unsigned long timeout = jiffies + msecs_to_jiffies(250); > > while (time_before(jiffies, timeout)) { > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER); > + value = fpci_readl(tegra, tegra->soc->mbox.owner); > if (value == MBOX_OWNER_NONE) > break; > > @@ -431,7 +440,7 @@ static int tegra_xusb_mbox_send(struct tegra_xusb *tegra, > } > > if (time_after(jiffies, timeout)) > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_OWNER); > + value = fpci_readl(tegra, tegra->soc->mbox.owner); > > if (value != MBOX_OWNER_NONE) > return -ETIMEDOUT; > @@ -598,16 +607,16 @@ static irqreturn_t tegra_xusb_mbox_thread(int irq, void > *data) > > mutex_lock(&tegra->lock); > > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_DATA_OUT); > + value = fpci_readl(tegra, tegra->soc->mbox.data_out); > tegra_xusb_mbox_unpack(&msg, value); > > - value = fpci_readl(tegra, XUSB_CFG_ARU_MBOX_CMD); > + value = fpci_readl(tegra, tegra->soc->mbox.cmd); > value &= ~MBOX_DEST_SMI; > - fpci_writel(tegra, value, XUSB_CFG_ARU_MBOX_CMD); > + fpci_writel(tegra, value, tegra->soc->mbox.cmd); > > /* clear mailbox owner if no ACK/NAK is required */ > if (!tegra_xusb_mbox_cmd_requires_ack(msg.cmd)) > - fpci_writel(tegra, MBOX_OWNER_NONE, XUSB_CFG_ARU_MBOX_OWNER); > + fpci_writel(tegra, MBOX_OWNER_NONE, tegra->soc->mbox.owner); > > tegra_xusb_mbox_handle(tegra, &msg); > > @@ -1365,6 +1374,12 @@ static const struct tegra_xusb_soc tegra124_soc = { > }, > .scale_ss_clock = true, > .has_ipfs = true, > + .mbox = { > + .cmd = XUSB_CFG_ARU_MBOX_CMD, > + .data_in = XUSB_CFG_ARU_MBOX_DATA_IN, > + .data_out = XUSB_CFG_ARU_MBOX_DATA_OUT, > + .owner = XUSB_CFG_ARU_MBOX_OWNER, > + }, > }; > MODULE_FIRMWARE("nvidia/tegra124/xusb.bin"); > > @@ -1397,6 +1412,12 @@ static const struct tegra_xusb_soc tegra210_soc = { > }, > .scale_ss_clock = false, > .has_ipfs = true, > + .mbox = { > + .cmd = XUSB_CFG_ARU_MBOX_CMD, > + .data_in = XUSB_CFG_ARU_MBOX_DATA_IN, > + .data_out = XUSB_CFG_ARU_MBOX_DATA_OUT, > + .owner = XUSB_CFG_ARU_MBOX_OWNER, > + }, > }; > MODULE_FIRMWARE("nvidia/tegra210/xusb.bin"); > > @@ -1422,6 +1443,12 @@ static const struct tegra_xusb_soc tegra186_soc = { > }, > .scale_ss_clock = false, > .has_ipfs = false, > + .mbox = { > + .cmd = XUSB_CFG_ARU_MBOX_CMD, > + .data_in = XUSB_CFG_ARU_MBOX_DATA_IN, > + .data_out = XUSB_CFG_ARU_MBOX_DATA_OUT, > + .owner = XUSB_CFG_ARU_MBOX_OWNER, > + }, You're already giving these registers names via the new parameters, so I don't think we need the symbolic names. This usually just leads to weird things like: #define XUSB_CFG_ARU_MBOX_CMD_T194 ... #define XUSB_CFG_ARU_MBOX_DATA_IN_T194 ... #define XUSB_CFG_ARU_MBOX_DATA_OUT_T194 ... #define XUSB_CFG_ARU_MBOX_OWNER_T194 ... .mbox = { .cmd = XUSB_CFG_ARU_MBOX_CMD_T194, .data_in = XUSB_CFG_ARU_MBOX_DATA_IN_T194, .data_out = XUSB_CFG_ARU_MBOX_DATA_OUT_T194, .owner = XUSB_CFG_ARU_MBOX_OWNER_T194, }, Just remove the symbolic names and use the literal address in the structure definition. Thierry
signature.asc
Description: PGP signature