| Message ID | 1408381705-3623-3-git-send-email-abrestic@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > The Tegra xHCI controller's firmware communicates requests to the host > processor through a mailbox interface. While there is only a single > communication channel, messages sent by the controller can be divided > into two groups: those intended for the PHY driver and those intended > for the host-controller driver. This mailbox driver exposes the two > channels and routes incoming messages to the appropriate channel based > on the command encoded in the message. > diff --git a/drivers/mailbox/tegra-xusb-mailbox.c b/drivers/mailbox/tegra-xusb-mailbox.c > +#define XUSB_CFG_ARU_MBOX_CMD 0xe4 > +#define MBOX_FALC_INT_EN BIT(27) > +#define MBOX_PME_INT_EN BIT(28) > +#define MBOX_SMI_INT_EN BIT(29) > +#define MBOX_XHCI_INT_EN BIT(30) > +#define MBOX_INT_EN BIT(31) Those field names don't match the documentation in the TRM; they're called DEST_xxx rather than xxx_INT_EN. I'm not sure what that disconnect means (i.e. whether it's just a different naming choice, or there's some practical disconnect that will cause issues.) > +static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox, u32 cmd) > +{ > + switch (cmd) { > + case MBOX_CMD_INC_FALC_CLOCK: > + case MBOX_CMD_DEC_FALC_CLOCK: > + case MBOX_CMD_INC_SSPI_CLOCK: > + case MBOX_CMD_DEC_SSPI_CLOCK: > + case MBOX_CMD_SET_BW: > + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST]; > + case MBOX_CMD_SAVE_DFE_CTLE_CTX: > + case MBOX_CMD_START_HSIC_IDLE: > + case MBOX_CMD_STOP_HSIC_IDLE: > + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY]; > + default: > + return NULL; > + } > +} This makes me think that the CHAN_HOST/CHAN_PHY values are purely a facet of the Linux driver's message de-multiplexing, rather than anything to do with the HW. I'm not even sure if it's appropriate for the low-level mailbox driver to know about the semantics of the message, rather than simply sending them on to the client driver? Perhaps when drivers register(?) for callbacks(?) for messages, they should state which types of messages they want to listen to? > +static irqreturn_t tegra_xusb_mbox_irq(int irq, void *p) > + /* Clear mbox interrupts */ > + reg = mbox_readl(mbox, XUSB_CFG_ARU_SMI_INTR); > + if (reg & MBOX_SMI_INTR_FW_HANG) > + dev_err(mbox->mbox.dev, "Controller firmware hang\n"); > + mbox_writel(mbox, reg, XUSB_CFG_ARU_SMI_INTR); > + /* > + * Set the mailbox back to idle. The recipient of the message is > + * responsible for sending an ACK/NAK, if necessary. > + */ > + reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD); > + reg &= ~MBOX_SMI_INT_EN; > + mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD); Does the protocol not allow the remote firmware to send another message until the host has ack'd/nak'd the message; the code above turns off the IRQ that indicated to the host that a message was sent to it... > +static int tegra_xusb_mbox_probe(struct platform_device *pdev) > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENODEV; Should devm_request_mem_region() be called here to claim the region? > + mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > + resource_size(res)); > + if (!mbox->regs) > + return -ENOMEM; Is _nocache required? I don't see other drivers using it. I assume there's nothing special about the mbox registers. > + mbox->irq = platform_get_irq(pdev, 0); > + if (mbox->irq < 0) > + return mbox->irq; > + ret = devm_request_irq(&pdev->dev, mbox->irq, tegra_xusb_mbox_irq, 0, > + dev_name(&pdev->dev), mbox); Is it possible for an IRQ to occur after tegra_xusb_mbox_remove() has returned, but before the cleanup for the devm IRQ allocation occurs? If that happens, will the code handle it gracefully, or crash? > +MODULE_ALIAS("platform:tegra-xusb-mailbox"); I don't think that's required; it should auto-load based on the of_device_id/MODULE_DEVICE_TABLE(of,...) table. > diff --git a/include/soc/tegra/xusb.h b/include/soc/tegra/xusb.h > +#define TEGRA_XUSB_MBOX_NUM_CHANS 2 /* host + phy */ I'd rather see that definition in the same place as the TEGRA_XUSB_MBOX_CHAN_* values it refers to. Otherwise, it'd be quite easy to add values without updating this constant.
On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: [...] > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > >+ if (!res) > >+ return -ENODEV; > > Should devm_request_mem_region() be called here to claim the region? > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > >+ resource_size(res)); > >+ if (!mbox->regs) > >+ return -ENOMEM; > > Is _nocache required? I don't see other drivers using it. I assume there's > nothing special about the mbox registers. Most drivers should be using devm_ioremap_resource() which will use the _nocache variant of devm_ioremap() when appropriate. Usually the region will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be remapped uncached. Thierry
On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > [...] > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > >+ if (!res) > > >+ return -ENODEV; > > > > Should devm_request_mem_region() be called here to claim the region? > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > >+ resource_size(res)); > > >+ if (!mbox->regs) > > >+ return -ENOMEM; > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > nothing special about the mbox registers. > > Most drivers should be using devm_ioremap_resource() which will use the > _nocache variant of devm_ioremap() when appropriate. Usually the region > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > remapped uncached. > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't ever call ioremap_nocache(). devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is rather silly, since it doesn't call ioremap_cache() in that case. Arnd
On Tue, Aug 26, 2014 at 09:43:50AM +0200, Arnd Bergmann wrote: > On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > > [...] > > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > >+ if (!res) > > > >+ return -ENODEV; > > > > > > Should devm_request_mem_region() be called here to claim the region? > > > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > > >+ resource_size(res)); > > > >+ if (!mbox->regs) > > > >+ return -ENOMEM; > > > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > nothing special about the mbox registers. > > > > Most drivers should be using devm_ioremap_resource() which will use the > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > remapped uncached. > > > > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't > ever call ioremap_nocache(). Perhaps we should remove ioremap_nocache() in that case. Or ioremap(), really, and keep only those variants that do what they claim to do. > devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is > rather silly, since it doesn't call ioremap_cache() in that case. Then that should be fixed. Thierry
On Tuesday 26 August 2014 09:50:25 Thierry Reding wrote: > On Tue, Aug 26, 2014 at 09:43:50AM +0200, Arnd Bergmann wrote: > > On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > > > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > > > [...] > > > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > >+ if (!res) > > > > >+ return -ENODEV; > > > > > > > > Should devm_request_mem_region() be called here to claim the region? > > > > > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > > > >+ resource_size(res)); > > > > >+ if (!mbox->regs) > > > > >+ return -ENOMEM; > > > > > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > > nothing special about the mbox registers. > > > > > > Most drivers should be using devm_ioremap_resource() which will use the > > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > > remapped uncached. > > > > > > > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't > > ever call ioremap_nocache(). > > Perhaps we should remove ioremap_nocache() in that case. Or ioremap(), > really, and keep only those variants that do what they claim to do. That would be good, but there are many instances of either one: arnd@wuerfel:/git/arm-soc$ git grep -w ioremap | wc 2156 13402 183732 arnd@wuerfel:/git/arm-soc$ git grep -w ioremap_nocache | wc 485 2529 42955 FWIW, I just looked through all architectures and found three on which ioremap and ioremap_nocache are not the same, and ioremap defaults to cacheable: - OpenRISC so far only supports running in a simulator, so this is likely to be a bug that will get hit on actual hardware with MMIO. Jonas should probably look into this. - mn10300 has no MMU and doesn't really use ioremap, but it should still be fixed for PCI drivers using it on the one board that supports PCI. - cris seems to have been broken forever. > > devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is > > rather silly, since it doesn't call ioremap_cache() in that case. > > Then that should be fixed. Yes. I'd suggest we just ignore that flag and always call ioremap here. When I checked this before, IORESOURCE_CACHEABLE only ever gets set for PCI ROM BARs, which we don't map into the kernel. Arnd
From: Thierry Reding ... > > Is _nocache required? I don't see other drivers using it. I assume there's > > nothing special about the mbox registers. > > Most drivers should be using devm_ioremap_resource() which will use the > _nocache variant of devm_ioremap() when appropriate. Usually the region > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > remapped uncached. A related question: Is there any way for a driver to force that part of a PCIe BAR be mapped through the data cache even when the BAR isn't actually marked cacheable? Some hardware has address regions (which might not be an entire BAR) that are actually memory and mapping through the data cache will generate longer PCIe transfers [1]. Clearly the driver will have to be very careful about cache flushes and invalidates to make this work. [1] PCIe is high throughput and high latency, single word reads can be much slower that PCI, much nearer x86 ISA bus speeds. David
On Tue, Aug 26, 2014 at 10:09:25AM +0200, Arnd Bergmann wrote: > On Tuesday 26 August 2014 09:50:25 Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 09:43:50AM +0200, Arnd Bergmann wrote: > > > On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > > > > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > > > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > > > > [...] > > > > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > > > > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > >+ if (!res) > > > > > >+ return -ENODEV; > > > > > > > > > > Should devm_request_mem_region() be called here to claim the region? > > > > > > > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > > > > >+ resource_size(res)); > > > > > >+ if (!mbox->regs) > > > > > >+ return -ENOMEM; > > > > > > > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > > > nothing special about the mbox registers. > > > > > > > > Most drivers should be using devm_ioremap_resource() which will use the > > > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > > > remapped uncached. > > > > > > > > > > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't > > > ever call ioremap_nocache(). > > > > Perhaps we should remove ioremap_nocache() in that case. Or ioremap(), > > really, and keep only those variants that do what they claim to do. > > That would be good, but there are many instances of either one: > > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap | wc > 2156 13402 183732 > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap_nocache | wc > 485 2529 42955 Ugh... nothing that I currently have time for. Perhaps this is a good one for the Janitors? I'm not sure if the kernelnewbies.org TODO list is still frequented since many pages seem to be very old. Is there some other place where I could add this? > > > devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is > > > rather silly, since it doesn't call ioremap_cache() in that case. > > > > Then that should be fixed. > > Yes. I'd suggest we just ignore that flag and always call ioremap here. > > When I checked this before, IORESOURCE_CACHEABLE only ever gets set for > PCI ROM BARs, which we don't map into the kernel. There's still a few users of ioremap_cache() around and they are potential candidates for a conversion to devm_ioremap_resource(), so I think it'd still make sense to keep the check. Thierry
On Tuesday 26 August 2014 11:08:11 Thierry Reding wrote: > On Tue, Aug 26, 2014 at 10:09:25AM +0200, Arnd Bergmann wrote: > > On Tuesday 26 August 2014 09:50:25 Thierry Reding wrote: > > > On Tue, Aug 26, 2014 at 09:43:50AM +0200, Arnd Bergmann wrote: > > > > On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > > > > > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > > > > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > > > > > [...] > > > > > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > > > > > > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > >+ if (!res) > > > > > > >+ return -ENODEV; > > > > > > > > > > > > Should devm_request_mem_region() be called here to claim the region? > > > > > > > > > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > > > > > >+ resource_size(res)); > > > > > > >+ if (!mbox->regs) > > > > > > >+ return -ENOMEM; > > > > > > > > > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > > > > nothing special about the mbox registers. > > > > > > > > > > Most drivers should be using devm_ioremap_resource() which will use the > > > > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > > > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > > > > remapped uncached. > > > > > > > > > > > > > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't > > > > ever call ioremap_nocache(). > > > > > > Perhaps we should remove ioremap_nocache() in that case. Or ioremap(), > > > really, and keep only those variants that do what they claim to do. > > > > That would be good, but there are many instances of either one: > > > > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap | wc > > 2156 13402 183732 > > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap_nocache | wc > > 485 2529 42955 > > Ugh... nothing that I currently have time for. Perhaps this is a good > one for the Janitors? I'm not sure if the kernelnewbies.org TODO list is > still frequented since many pages seem to be very old. Is there some > other place where I could add this? I'm not sure if it's really worth it. One thing we might do is just remove all definitions of ioremap_nocache and add a wrapper to include/linux/io.h, to make it more obvious what is going on. > > > > devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is > > > > rather silly, since it doesn't call ioremap_cache() in that case. > > > > > > Then that should be fixed. > > > > Yes. I'd suggest we just ignore that flag and always call ioremap here. > > > > When I checked this before, IORESOURCE_CACHEABLE only ever gets set for > > PCI ROM BARs, which we don't map into the kernel. > > There's still a few users of ioremap_cache() around and they are > potential candidates for a conversion to devm_ioremap_resource(), so I > think it'd still make sense to keep the check. Possibly. Note that these are all in architecture-specific code, as evidenced by the fact that we have multiple names for this function: ioremap_cache: arm, arm64, x86, ia64, sh ioremap_cached: metag, unicore32 ioremap_cachable: mips All other architectures have none of the above. An alternative approach would be to kill off IORESOURCE_CACHEABLE and introduce a devm_ioremap_resource_cache() helper when the first driver wants it. Arnd
On Tuesday 26 August 2014 08:54:53 David Laight wrote: > From: Thierry Reding > ... > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > nothing special about the mbox registers. > > > > Most drivers should be using devm_ioremap_resource() which will use the > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > remapped uncached. > > A related question: > Is there any way for a driver to force that part of a PCIe BAR be mapped > through the data cache even when the BAR isn't actually marked cacheable? No. BARs are not actually marked cacheable anyway, except for the ROM BAR, which we tend to not use. Some architectures don't even allow any caching of PCI memory ranges, so we have no architecture independent API for that. It's possible that ioremap_cache() works on x86 and/or ARM if you call it manually on the physical address (rather than using a resource API). > Some hardware has address regions (which might not be an entire BAR) > that are actually memory and mapping through the data cache will > generate longer PCIe transfers [1]. > Clearly the driver will have to be very careful about cache flushes > and invalidates to make this work. Some framebuffer drivers use writethrough mappings, but those again are only available on few architectures. vesafb uses ioremap_cache, but this works because the memory is in system RAM and not on PCI. Arnd
On Tue, Aug 26, 2014 at 11:54:43AM +0200, Arnd Bergmann wrote: > On Tuesday 26 August 2014 11:08:11 Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 10:09:25AM +0200, Arnd Bergmann wrote: > > > On Tuesday 26 August 2014 09:50:25 Thierry Reding wrote: > > > > On Tue, Aug 26, 2014 at 09:43:50AM +0200, Arnd Bergmann wrote: > > > > > On Tuesday 26 August 2014 08:57:31 Thierry Reding wrote: > > > > > > On Mon, Aug 25, 2014 at 01:01:52PM -0600, Stephen Warren wrote: > > > > > > > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: > > > > > > [...] > > > > > > > >+static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > > > > > > > > > > > > > >+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > > > > > > >+ if (!res) > > > > > > > >+ return -ENODEV; > > > > > > > > > > > > > > Should devm_request_mem_region() be called here to claim the region? > > > > > > > > > > > > > > >+ mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, > > > > > > > >+ resource_size(res)); > > > > > > > >+ if (!mbox->regs) > > > > > > > >+ return -ENOMEM; > > > > > > > > > > > > > > Is _nocache required? I don't see other drivers using it. I assume there's > > > > > > > nothing special about the mbox registers. > > > > > > > > > > > > Most drivers should be using devm_ioremap_resource() which will use the > > > > > > _nocache variant of devm_ioremap() when appropriate. Usually the region > > > > > > will not be marked cacheable (IORESOURCE_CACHEABLE) and therefore be > > > > > > remapped uncached. > > > > > > > > > > > > > > > > Note that ioremap() and ioremap_nocache() are the same. We really shouldn't > > > > > ever call ioremap_nocache(). > > > > > > > > Perhaps we should remove ioremap_nocache() in that case. Or ioremap(), > > > > really, and keep only those variants that do what they claim to do. > > > > > > That would be good, but there are many instances of either one: > > > > > > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap | wc > > > 2156 13402 183732 > > > arnd@wuerfel:/git/arm-soc$ git grep -w ioremap_nocache | wc > > > 485 2529 42955 > > > > Ugh... nothing that I currently have time for. Perhaps this is a good > > one for the Janitors? I'm not sure if the kernelnewbies.org TODO list is > > still frequented since many pages seem to be very old. Is there some > > other place where I could add this? > > I'm not sure if it's really worth it. One thing we might do is just > remove all definitions of ioremap_nocache and add a wrapper to > include/linux/io.h, to make it more obvious what is going on. Yes, I suppose that would work too. I still think there's an advantage in being explicit and avoid aliases like this. Perhaps a __deprecated annotation would help with that? > > > > > devm_ioremap_resource() and pci_iomap() checking for IORESOURCE_CACHEABLE is > > > > > rather silly, since it doesn't call ioremap_cache() in that case. > > > > > > > > Then that should be fixed. > > > > > > Yes. I'd suggest we just ignore that flag and always call ioremap here. > > > > > > When I checked this before, IORESOURCE_CACHEABLE only ever gets set for > > > PCI ROM BARs, which we don't map into the kernel. > > > > There's still a few users of ioremap_cache() around and they are > > potential candidates for a conversion to devm_ioremap_resource(), so I > > think it'd still make sense to keep the check. > > Possibly. Note that these are all in architecture-specific code, as > evidenced by the fact that we have multiple names for this function: > > ioremap_cache: arm, arm64, x86, ia64, sh > ioremap_cached: metag, unicore32 > ioremap_cachable: mips > > All other architectures have none of the above. > > An alternative approach would be to kill off IORESOURCE_CACHEABLE > and introduce a devm_ioremap_resource_cache() helper when the first > driver wants it. Looking briefly at the involved headers and structure there seems to be quite a bit of potential for cleanup. Thierry
On Tuesday 26 August 2014 12:20:13 Thierry Reding wrote: > On Tue, Aug 26, 2014 at 11:54:43AM +0200, Arnd Bergmann wrote: > > > I'm not sure if it's really worth it. One thing we might do is just > > remove all definitions of ioremap_nocache and add a wrapper to > > include/linux/io.h, to make it more obvious what is going on. > > Yes, I suppose that would work too. I still think there's an advantage > in being explicit and avoid aliases like this. Perhaps a __deprecated > annotation would help with that? I fear adding __deprecated would be too controversial, because that would add hundreds of new warnings to code that is not actually wrong. Arnd
On Tue, Aug 26, 2014 at 01:35:34PM +0200, Arnd Bergmann wrote: > On Tuesday 26 August 2014 12:20:13 Thierry Reding wrote: > > On Tue, Aug 26, 2014 at 11:54:43AM +0200, Arnd Bergmann wrote: > > > > I'm not sure if it's really worth it. One thing we might do is just > > > remove all definitions of ioremap_nocache and add a wrapper to > > > include/linux/io.h, to make it more obvious what is going on. > > > > Yes, I suppose that would work too. I still think there's an advantage > > in being explicit and avoid aliases like this. Perhaps a __deprecated > > annotation would help with that? > > I fear adding __deprecated would be too controversial, because that > would add hundreds of new warnings to code that is not actually wrong. Right. __deprecated is enabled via Kconfig, though, so people could turn that off if they don't want to see the warnings. I don't mind very much either way. Thierry
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >> >> The Tegra xHCI controller's firmware communicates requests to the host >> processor through a mailbox interface. While there is only a single >> communication channel, messages sent by the controller can be divided >> into two groups: those intended for the PHY driver and those intended >> for the host-controller driver. This mailbox driver exposes the two >> channels and routes incoming messages to the appropriate channel based >> on the command encoded in the message. > > >> diff --git a/drivers/mailbox/tegra-xusb-mailbox.c >> b/drivers/mailbox/tegra-xusb-mailbox.c > > >> +#define XUSB_CFG_ARU_MBOX_CMD 0xe4 >> +#define MBOX_FALC_INT_EN BIT(27) >> +#define MBOX_PME_INT_EN BIT(28) >> +#define MBOX_SMI_INT_EN BIT(29) >> +#define MBOX_XHCI_INT_EN BIT(30) >> +#define MBOX_INT_EN BIT(31) > > > Those field names don't match the documentation in the TRM; they're called > DEST_xxx rather than xxx_INT_EN. I'm not sure what that disconnect means > (i.e. whether it's just a different naming choice, or there's some practical > disconnect that will cause issues.) Hmm... interestingly *_INT_EN is the convention the downstream kernels used. DEST_* is definitely more accurate as I'm pretty sure these bits select the destination for the interrupt. >> +static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox, >> u32 cmd) >> +{ >> + switch (cmd) { >> + case MBOX_CMD_INC_FALC_CLOCK: >> + case MBOX_CMD_DEC_FALC_CLOCK: >> + case MBOX_CMD_INC_SSPI_CLOCK: >> + case MBOX_CMD_DEC_SSPI_CLOCK: >> + case MBOX_CMD_SET_BW: >> + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST]; >> + case MBOX_CMD_SAVE_DFE_CTLE_CTX: >> + case MBOX_CMD_START_HSIC_IDLE: >> + case MBOX_CMD_STOP_HSIC_IDLE: >> + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY]; >> + default: >> + return NULL; >> + } >> +} > > > This makes me think that the CHAN_HOST/CHAN_PHY values are purely a facet of > the Linux driver's message de-multiplexing, rather than anything to do with > the HW. Yup, they are... > I'm not even sure if it's appropriate for the low-level mailbox driver to > know about the semantics of the message, rather than simply sending them on > to the client driver? Perhaps when drivers register(?) for callbacks(?) for > messages, they should state which types of messages they want to listen to? So there's not really a way for the client driver to tell the mailbox driver which types of messages it wants to listen to on a particular channel with the mailbox framework - it simply provides a way for clients to bind with channels. I think there are a couple of options here, either: a) have a channel per message (as you mentioned in the previous patch), which allows the client to only register for messages (channels) it wants to handle, or b) extend the mailbox framework to allow shared channels so that both clients can receive messages on the single channel and handle messages appropriately. The disadvantage of (a) is that the commands are firmware defined and could theoretically change between releases of the firmware, though I'm not sure how common that is in practice. So that leaves (b) - Jassi, what do you think about having shared (non-exclusive) channels? >> +static irqreturn_t tegra_xusb_mbox_irq(int irq, void *p) > > >> + /* Clear mbox interrupts */ >> >> + reg = mbox_readl(mbox, XUSB_CFG_ARU_SMI_INTR); >> + if (reg & MBOX_SMI_INTR_FW_HANG) >> + dev_err(mbox->mbox.dev, "Controller firmware hang\n"); >> + mbox_writel(mbox, reg, XUSB_CFG_ARU_SMI_INTR); > > >> + /* >> >> + * Set the mailbox back to idle. The recipient of the message is >> + * responsible for sending an ACK/NAK, if necessary. >> + */ >> + reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD); >> + reg &= ~MBOX_SMI_INT_EN; >> + mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD); > > > Does the protocol not allow the remote firmware to send another message > until the host has ack'd/nak'd the message; the code above turns off the IRQ > that indicated to the host that a message was sent to it... While the firmware generally will not send another message until the previous one is ACK'd/NAK'd (with the exception of the SET_BW command), the above does not prevent it from doing so. I believe the controller sets up the DEST_* bits properly before sending another message. >> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) > > >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> >> + if (!res) >> + return -ENODEV; > > > Should devm_request_mem_region() be called here to claim the region? No, the xHCI host driver also needs to map these registers, so they cannot be mapped exclusively here. >> + mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, >> + resource_size(res)); >> + if (!mbox->regs) >> + return -ENOMEM; > > > Is _nocache required? I don't see other drivers using it. I assume there's > nothing special about the mbox registers. I'll drop the _nocache. >> + mbox->irq = platform_get_irq(pdev, 0); >> + if (mbox->irq < 0) >> + return mbox->irq; >> + ret = devm_request_irq(&pdev->dev, mbox->irq, tegra_xusb_mbox_irq, >> 0, >> + dev_name(&pdev->dev), mbox); > > > Is it possible for an IRQ to occur after tegra_xusb_mbox_remove() has > returned, but before the cleanup for the devm IRQ allocation occurs? If that > happens, will the code handle it gracefully, or crash? It looks like mbox_chan_received_data() will crash if the channel is unbound, so yes, this needs to be fixed.
On 08/27/2014 11:38 AM, Andrew Bresticker wrote: > On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >>> >>> The Tegra xHCI controller's firmware communicates requests to the host >>> processor through a mailbox interface. While there is only a single >>> communication channel, messages sent by the controller can be divided >>> into two groups: those intended for the PHY driver and those intended >>> for the host-controller driver. This mailbox driver exposes the two >>> channels and routes incoming messages to the appropriate channel based >>> on the command encoded in the message. >> >> >>> diff --git a/drivers/mailbox/tegra-xusb-mailbox.c >>> b/drivers/mailbox/tegra-xusb-mailbox.c >>> +static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox, >>> u32 cmd) >>> +{ >>> + switch (cmd) { >>> + case MBOX_CMD_INC_FALC_CLOCK: >>> + case MBOX_CMD_DEC_FALC_CLOCK: >>> + case MBOX_CMD_INC_SSPI_CLOCK: >>> + case MBOX_CMD_DEC_SSPI_CLOCK: >>> + case MBOX_CMD_SET_BW: >>> + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST]; >>> + case MBOX_CMD_SAVE_DFE_CTLE_CTX: >>> + case MBOX_CMD_START_HSIC_IDLE: >>> + case MBOX_CMD_STOP_HSIC_IDLE: >>> + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY]; >>> + default: >>> + return NULL; >>> + } >>> +} >> >> >> This makes me think that the CHAN_HOST/CHAN_PHY values are purely a facet of >> the Linux driver's message de-multiplexing, rather than anything to do with >> the HW. > > Yup, they are... > >> I'm not even sure if it's appropriate for the low-level mailbox driver to >> know about the semantics of the message, rather than simply sending them on >> to the client driver? Perhaps when drivers register(?) for callbacks(?) for >> messages, they should state which types of messages they want to listen to? > > So there's not really a way for the client driver to tell the mailbox > driver which types of messages it wants to listen to on a particular > channel with the mailbox framework - it simply provides a way for > clients to bind with channels. I think there are a couple of options > here, either: a) have a channel per message (as you mentioned in the > previous patch), which allows the client to only register for messages > (channels) it wants to handle, or b) extend the mailbox framework to > allow shared channels so that both clients can receive messages on the > single channel and handle messages appropriately. The disadvantage > of (a) is that the commands are firmware defined and could > theoretically change between releases of the firmware, though I'm not > sure how common that is in practice. So that leaves (b) - Jassi, what > do you think about having shared (non-exclusive) channels? Another alternative might be for each client driver to hard-code a unique dummy channel ID so that each client still gets a separate exclusive channel, but then have the mbox driver broadcast each message to each of those channels. I'm not sure that would be any better though; adding (b) as an explicit option to the mbox subsystem would almost certainly be cleaner. >>> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) >> >> >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> >>> + if (!res) >>> + return -ENODEV; >> >> >> Should devm_request_mem_region() be called here to claim the region? > > No, the xHCI host driver also needs to map these registers, so they > cannot be mapped exclusively here. That's unfortunate. Having multiple drivers with overlapping register regions is not a good idea. Can we instead have a top-level driver map all the IO regions, then instantiate the various different sub-components internally, and divide up the address space. Probably via MFD or similar. That would prevent multiple drivers from touching the same register region.
On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/27/2014 11:38 AM, Andrew Bresticker wrote: >> >> On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >>>> >>>> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) >>> >>> >>> >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> >>>> + if (!res) >>>> + return -ENODEV; >>> >>> >>> >>> Should devm_request_mem_region() be called here to claim the region? >> >> >> No, the xHCI host driver also needs to map these registers, so they >> cannot be mapped exclusively here. > > > That's unfortunate. Having multiple drivers with overlapping register > regions is not a good idea. Can we instead have a top-level driver map all > the IO regions, then instantiate the various different sub-components > internally, and divide up the address space. Probably via MFD or similar. > That would prevent multiple drivers from touching the same register region. Perhaps I'm misunderstanding, but I don't see how MFD would prevent us from having to map this register space in two different locations - the XUSB FPCI address space cannot be divided cleanly between host and mailbox registers. Or are you saying that there should be a separate device driver that exposes an API for accessing this register space, like the Tegra fuse or PMC drivers?
On 08/27/2014 12:13 PM, Andrew Bresticker wrote: > On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/27/2014 11:38 AM, Andrew Bresticker wrote: >>> >>> On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> >>> wrote: >>>> >>>> On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >>>>> >>>>> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) >>>> >>>> >>>> >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> >>>>> + if (!res) >>>>> + return -ENODEV; >>>> >>>> >>>> >>>> Should devm_request_mem_region() be called here to claim the region? >>> >>> >>> No, the xHCI host driver also needs to map these registers, so they >>> cannot be mapped exclusively here. >> >> >> That's unfortunate. Having multiple drivers with overlapping register >> regions is not a good idea. Can we instead have a top-level driver map all >> the IO regions, then instantiate the various different sub-components >> internally, and divide up the address space. Probably via MFD or similar. >> That would prevent multiple drivers from touching the same register region. > > Perhaps I'm misunderstanding, but I don't see how MFD would prevent us > from having to map this register space in two different locations - > the XUSB FPCI address space cannot be divided cleanly between host and > mailbox registers. Or are you saying that there should be a separate > device driver that exposes an API for accessing this register space, > like the Tegra fuse or PMC drivers? With MFD, there's typically a top-level driver for the HW module (or register space) that gets instantiated by the DT node. This driver then instantiates all the different sub-drivers that use that register space, and provides APIs for the sub-drivers to access the registers (either custom APIs or more recently by passing a regmap object down to the sub-drivers). This top-level driver is the only driver that maps the space, and can manage sharing the space between the various sub-drivers. That said, I haven't noticed many MFD drivers for MMIO devices. I certainly have seen multiple different drivers just re-mapping shared registers for themselves. It is simpler and does work. However, people usually mutter about it when it happens, since it's not clear which drivers are using what from the IO mapping registry. Using MFD or similar would allow the sharing to work in a clean fashion.
On 27 August 2014 23:08, Andrew Bresticker <abrestic@chromium.org> wrote: > On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> I'm not even sure if it's appropriate for the low-level mailbox driver to >> know about the semantics of the message, rather than simply sending them on >> to the client driver? Perhaps when drivers register(?) for callbacks(?) for >> messages, they should state which types of messages they want to listen to? > > So there's not really a way for the client driver to tell the mailbox > driver which types of messages it wants to listen to on a particular > channel with the mailbox framework - it simply provides a way for > clients to bind with channels. I think there are a couple of options > here, either: a) have a channel per message (as you mentioned in the > previous patch), which allows the client to only register for messages > (channels) it wants to handle, or b) extend the mailbox framework to > allow shared channels so that both clients can receive messages on the > single channel and handle messages appropriately. The disadvantage > of (a) is that the commands are firmware defined and could > theoretically change between releases of the firmware, though I'm not > sure how common that is in practice. So that leaves (b) - Jassi, what > do you think about having shared (non-exclusive) channels? > n++ ... 'mailbox' is one such device that imbibes properties of local controller hardware and the remote firmware. Change in remote f/w's behavior might require the controller driver to change besides our client driver. A typical example is format of payloads (if involved).... a client and mailbox controller driver have direct understanding of the packet format ... which is likely to change with remote f/w. So if the original concern "why are we doing s/w protocol stuff in controller driver?" won't go away by providing for shared channels (which would have its own tradeoffs). BTW, on DMAEngine we are moving towards virtual channels backed by limited physical ones ... your setup looks quite similar. So your demuxer doesn't hurt my eyes at all. -jassi
On Wed, Aug 27, 2014 at 11:19 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 08/27/2014 12:13 PM, Andrew Bresticker wrote: >> >> On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren <swarren@wwwdotorg.org> >> wrote: >>> >>> On 08/27/2014 11:38 AM, Andrew Bresticker wrote: >>>> >>>> >>>> On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> >>>> wrote: >>>>> >>>>> >>>>> On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >>>>>> >>>>>> >>>>>> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) >>>>> >>>>> >>>>> >>>>> >>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> >>>>>> + if (!res) >>>>>> + return -ENODEV; >>>>> >>>>> >>>>> >>>>> >>>>> Should devm_request_mem_region() be called here to claim the region? >>>> >>>> >>>> >>>> No, the xHCI host driver also needs to map these registers, so they >>>> cannot be mapped exclusively here. >>> >>> >>> >>> That's unfortunate. Having multiple drivers with overlapping register >>> regions is not a good idea. Can we instead have a top-level driver map >>> all >>> the IO regions, then instantiate the various different sub-components >>> internally, and divide up the address space. Probably via MFD or similar. >>> That would prevent multiple drivers from touching the same register >>> region. >> >> >> Perhaps I'm misunderstanding, but I don't see how MFD would prevent us >> from having to map this register space in two different locations - >> the XUSB FPCI address space cannot be divided cleanly between host and >> mailbox registers. Or are you saying that there should be a separate >> device driver that exposes an API for accessing this register space, >> like the Tegra fuse or PMC drivers? > > > With MFD, there's typically a top-level driver for the HW module (or > register space) that gets instantiated by the DT node. This driver then > instantiates all the different sub-drivers that use that register space, and > provides APIs for the sub-drivers to access the registers (either custom > APIs or more recently by passing a regmap object down to the sub-drivers). > > This top-level driver is the only driver that maps the space, and can manage > sharing the space between the various sub-drivers. So if I'm understanding correctly, we end up with something like this: usb@70090000 { compatible = "nvidia,tegra124-xusb"; reg = <0x0 0x70090000 0x0 0x8000>, // xHCI host registers <0x0 0x70098000 0x0 0x1000>, // FPCI registers <0x0 0x70099000 0x0 0x1000>; // IPFS registers interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>, // host interrupt <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH"; // mailbox interrupt host-controller { compatible = "nvidia,tegra124-xhci"; ... }; mailbox { compatible = "nvidia,tegra124-xusb-mailbox"; ... }; }; To be honest though, this seems like overkill to share a couple of registers when no other resources are shared between the two.
On 08/27/2014 03:56 PM, Andrew Bresticker wrote: > On Wed, Aug 27, 2014 at 11:19 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 08/27/2014 12:13 PM, Andrew Bresticker wrote: >>> >>> On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren <swarren@wwwdotorg.org> >>> wrote: >>>> >>>> On 08/27/2014 11:38 AM, Andrew Bresticker wrote: >>>>> >>>>> >>>>> On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren <swarren@wwwdotorg.org> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 08/18/2014 11:08 AM, Andrew Bresticker wrote: >>>>>>> >>>>>>> >>>>>>> +static int tegra_xusb_mbox_probe(struct platform_device *pdev) >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>>> >>>>>>> + if (!res) >>>>>>> + return -ENODEV; >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Should devm_request_mem_region() be called here to claim the region? >>>>> >>>>> >>>>> >>>>> No, the xHCI host driver also needs to map these registers, so they >>>>> cannot be mapped exclusively here. >>>> >>>> >>>> >>>> That's unfortunate. Having multiple drivers with overlapping register >>>> regions is not a good idea. Can we instead have a top-level driver map >>>> all >>>> the IO regions, then instantiate the various different sub-components >>>> internally, and divide up the address space. Probably via MFD or similar. >>>> That would prevent multiple drivers from touching the same register >>>> region. >>> >>> >>> Perhaps I'm misunderstanding, but I don't see how MFD would prevent us >>> from having to map this register space in two different locations - >>> the XUSB FPCI address space cannot be divided cleanly between host and >>> mailbox registers. Or are you saying that there should be a separate >>> device driver that exposes an API for accessing this register space, >>> like the Tegra fuse or PMC drivers? >> >> >> With MFD, there's typically a top-level driver for the HW module (or >> register space) that gets instantiated by the DT node. This driver then >> instantiates all the different sub-drivers that use that register space, and >> provides APIs for the sub-drivers to access the registers (either custom >> APIs or more recently by passing a regmap object down to the sub-drivers). >> >> This top-level driver is the only driver that maps the space, and can manage >> sharing the space between the various sub-drivers. > > So if I'm understanding correctly, we end up with something like this: > > usb@70090000 { > compatible = "nvidia,tegra124-xusb"; > reg = <0x0 0x70090000 0x0 0x8000>, // xHCI host registers > <0x0 0x70098000 0x0 0x1000>, // FPCI registers > <0x0 0x70099000 0x0 0x1000>; // IPFS registers > interrupts = <GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>, // host interrupt > <GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH"; // mailbox interrupt > > host-controller { > compatible = "nvidia,tegra124-xhci"; > ... > }; > > mailbox { > compatible = "nvidia,tegra124-xusb-mailbox"; > ... > }; > }; > > To be honest though, this seems like overkill to share a couple of > registers when no other resources are shared between the two. Something like that, yes. Given that the "xusb" driver knows that its HW module contains both an XHCI and XUSB mailbox chunk, those might not need to appear inside the main XUSB module, but could be hard-coded into the driver. They might serve as convenient containers for sub-device-specific properties though. Other alternatives might be: a) If the registers that are shared between drivers are distinct, then divide the reg values into non-overlapping lists. We have taken this approach for the MC/SMMU drivers on Tegra, although it's a horrible mess and Thierry is actively thinking about reverting that and doing it through MFD or something MFD-like. b) Allow the same IO region to be claimed by multiple devices, perhaps with a new API so that it doesn't accidentally happen when not desired. c) Ignore the issue and deal with the fact that not all driver usage shows up in /proc/iomem. This is what we have for the Tegra USB2 and USB2 PHY drivers, and this is (I think) what your current patch does. To be honest, none of the options are that good; some end up with the correct result but are a pain to implement, but others are nice and simple yet /proc/iomem isn't complete. Given that, I'm personally not going to try and mandate one option or the other, so the current patch is fine. Food for thought though:-)
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index 9fd9c67..97369c2 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -33,4 +33,7 @@ config OMAP_MBOX_KFIFO_SIZE Specify the default size of mailbox's kfifo buffers (bytes). This can also be changed at runtime (via the mbox_kfifo_size module parameter). + +config TEGRA_XUSB_MBOX + def_bool y if ARCH_TEGRA endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 94ed7ce..7f0af9c 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -5,3 +5,5 @@ obj-$(CONFIG_MAILBOX) += mailbox.o obj-$(CONFIG_PL320_MBOX) += pl320-ipc.o obj-$(CONFIG_OMAP2PLUS_MBOX) += omap-mailbox.o + +obj-$(CONFIG_TEGRA_XUSB_MBOX) += tegra-xusb-mailbox.o diff --git a/drivers/mailbox/tegra-xusb-mailbox.c b/drivers/mailbox/tegra-xusb-mailbox.c new file mode 100644 index 0000000..917d48e7 --- /dev/null +++ b/drivers/mailbox/tegra-xusb-mailbox.c @@ -0,0 +1,279 @@ +/* + * NVIDIA Tegra XUSB mailbox driver + * + * Copyright (C) 2014 NVIDIA Corporation + * Copyright (C) 2014 Google, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + */ + +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/module.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/slab.h> + +#include <soc/tegra/xusb.h> + +#include <dt-bindings/mailbox/tegra-xusb-mailbox.h> + +#define XUSB_CFG_ARU_MBOX_CMD 0xe4 +#define MBOX_FALC_INT_EN BIT(27) +#define MBOX_PME_INT_EN BIT(28) +#define MBOX_SMI_INT_EN BIT(29) +#define MBOX_XHCI_INT_EN BIT(30) +#define MBOX_INT_EN BIT(31) +#define XUSB_CFG_ARU_MBOX_DATA_IN 0xe8 +#define CMD_DATA_SHIFT 0 +#define CMD_DATA_MASK 0xffffff +#define CMD_TYPE_SHIFT 24 +#define CMD_TYPE_MASK 0xff +#define XUSB_CFG_ARU_MBOX_DATA_OUT 0xec +#define XUSB_CFG_ARU_MBOX_OWNER 0xf0 +#define MBOX_OWNER_NONE 0 +#define MBOX_OWNER_FW 1 +#define MBOX_OWNER_SW 2 +#define XUSB_CFG_ARU_SMI_INTR 0x428 +#define MBOX_SMI_INTR_FW_HANG BIT(1) +#define MBOX_SMI_INTR_EN BIT(3) + +#define XUSB_MBOX_IDLE_TIMEOUT 20 +#define XUSB_MBOX_ACQUIRE_TIMEOUT 10 + +struct tegra_xusb_mbox { + struct mbox_controller mbox; + int irq; + void __iomem *regs; + spinlock_t lock; +}; + +static inline u32 mbox_readl(struct tegra_xusb_mbox *mbox, unsigned long offset) +{ + return readl(mbox->regs + offset); +} + +static inline void mbox_writel(struct tegra_xusb_mbox *mbox, u32 val, + unsigned long offset) +{ + writel(val, mbox->regs + offset); +} + +static inline u32 mbox_pack_msg(struct tegra_xusb_mbox_msg *msg) +{ + u32 val; + + val = (msg->cmd & CMD_TYPE_MASK) << CMD_TYPE_SHIFT; + val |= (msg->data & CMD_DATA_MASK) << CMD_DATA_SHIFT; + + return val; +} + +static inline void mbox_unpack_msg(u32 val, struct tegra_xusb_mbox_msg *msg) +{ + msg->cmd = (val >> CMD_TYPE_SHIFT) & CMD_TYPE_MASK; + msg->data = (val >> CMD_DATA_SHIFT) & CMD_DATA_MASK; +} + +static struct mbox_chan *mbox_cmd_to_chan(struct tegra_xusb_mbox *mbox, u32 cmd) +{ + switch (cmd) { + case MBOX_CMD_INC_FALC_CLOCK: + case MBOX_CMD_DEC_FALC_CLOCK: + case MBOX_CMD_INC_SSPI_CLOCK: + case MBOX_CMD_DEC_SSPI_CLOCK: + case MBOX_CMD_SET_BW: + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_HOST]; + case MBOX_CMD_SAVE_DFE_CTLE_CTX: + case MBOX_CMD_START_HSIC_IDLE: + case MBOX_CMD_STOP_HSIC_IDLE: + return &mbox->mbox.chans[TEGRA_XUSB_MBOX_CHAN_PHY]; + default: + return NULL; + } +} + +static int tegra_xusb_mbox_send_data(struct mbox_chan *chan, void *data) +{ + struct tegra_xusb_mbox *mbox = dev_get_drvdata(chan->mbox->dev); + struct tegra_xusb_mbox_msg *msg = data; + unsigned long flags; + u32 reg, owner; + + dev_dbg(mbox->mbox.dev, "TX message 0x%x:0x%x\n", msg->cmd, msg->data); + + /* ACK/NAK must be sent with the controller as the mailbox owner */ + if (msg->cmd == MBOX_CMD_ACK || msg->cmd == MBOX_CMD_NAK) + owner = MBOX_OWNER_FW; + else + owner = MBOX_OWNER_SW; + + spin_lock_irqsave(&mbox->lock, flags); + + /* Acquire mailbox */ + if (mbox_readl(mbox, XUSB_CFG_ARU_MBOX_OWNER) != MBOX_OWNER_NONE) { + dev_err(mbox->mbox.dev, "Mailbox not idle\n"); + goto busy; + } + mbox_writel(mbox, owner, XUSB_CFG_ARU_MBOX_OWNER); + if (mbox_readl(mbox, XUSB_CFG_ARU_MBOX_OWNER) != owner) { + dev_err(mbox->mbox.dev, "Failed to acquire mailbox"); + goto busy; + } + + mbox_writel(mbox, mbox_pack_msg(msg), XUSB_CFG_ARU_MBOX_DATA_IN); + reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD); + reg |= MBOX_INT_EN | MBOX_FALC_INT_EN; + mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD); + + spin_unlock_irqrestore(&mbox->lock, flags); + + return 0; +busy: + spin_unlock_irqrestore(&mbox->lock, flags); + return -EBUSY; +} + +static int tegra_xusb_mbox_startup(struct mbox_chan *chan) +{ + return 0; +} + +static void tegra_xusb_mbox_shutdown(struct mbox_chan *chan) +{ +} + +static bool tegra_xusb_mbox_last_tx_done(struct mbox_chan *chan) +{ + /* + * Transmissions are assumed to be completed as soon as they are + * written to the mailbox. + */ + return true; +} + +static struct mbox_chan_ops tegra_xusb_mbox_chan_ops = { + .send_data = tegra_xusb_mbox_send_data, + .startup = tegra_xusb_mbox_startup, + .shutdown = tegra_xusb_mbox_shutdown, + .last_tx_done = tegra_xusb_mbox_last_tx_done, +}; + +static irqreturn_t tegra_xusb_mbox_irq(int irq, void *p) +{ + struct tegra_xusb_mbox *mbox = (struct tegra_xusb_mbox *)p; + struct mbox_chan *chan; + struct tegra_xusb_mbox_msg msg; + u32 reg; + + spin_lock(&mbox->lock); + + /* Clear mbox interrupts */ + reg = mbox_readl(mbox, XUSB_CFG_ARU_SMI_INTR); + if (reg & MBOX_SMI_INTR_FW_HANG) + dev_err(mbox->mbox.dev, "Controller firmware hang\n"); + mbox_writel(mbox, reg, XUSB_CFG_ARU_SMI_INTR); + + reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_DATA_OUT); + mbox_unpack_msg(reg, &msg); + + /* + * Set the mailbox back to idle. The recipient of the message is + * responsible for sending an ACK/NAK, if necessary. + */ + reg = mbox_readl(mbox, XUSB_CFG_ARU_MBOX_CMD); + reg &= ~MBOX_SMI_INT_EN; + mbox_writel(mbox, reg, XUSB_CFG_ARU_MBOX_CMD); + mbox_writel(mbox, MBOX_OWNER_NONE, XUSB_CFG_ARU_MBOX_OWNER); + + spin_unlock(&mbox->lock); + + dev_dbg(mbox->mbox.dev, "RX message 0x%x:0x%x\n", msg.cmd, msg.data); + chan = mbox_cmd_to_chan(mbox, msg.cmd); + if (chan) + mbox_chan_received_data(chan, &msg); + else + dev_err(mbox->mbox.dev, "Unexpected message: 0x%x:0x%x\n", + msg.cmd, msg.data); + + return IRQ_HANDLED; +} + +static struct of_device_id tegra_xusb_mbox_of_match[] = { + { .compatible = "nvidia,tegra124-xusb-mbox" }, + { }, +}; +MODULE_DEVICE_TABLE(of, tegra_xusb_mbox_of_match); + +static int tegra_xusb_mbox_probe(struct platform_device *pdev) +{ + struct tegra_xusb_mbox *mbox; + struct resource *res; + int ret; + + mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL); + if (!mbox) + return -ENOMEM; + platform_set_drvdata(pdev, mbox); + spin_lock_init(&mbox->lock); + + mbox->mbox.dev = &pdev->dev; + mbox->mbox.chans = devm_kcalloc(&pdev->dev, TEGRA_XUSB_MBOX_NUM_CHANS, + sizeof(*mbox->mbox.chans), GFP_KERNEL); + if (!mbox->mbox.chans) + return -ENOMEM; + mbox->mbox.num_chans = TEGRA_XUSB_MBOX_NUM_CHANS; + mbox->mbox.ops = &tegra_xusb_mbox_chan_ops; + mbox->mbox.txdone_poll = true; + mbox->mbox.txpoll_period = 0; /* no need to actually poll */ + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENODEV; + mbox->regs = devm_ioremap_nocache(&pdev->dev, res->start, + resource_size(res)); + if (!mbox->regs) + return -ENOMEM; + + mbox->irq = platform_get_irq(pdev, 0); + if (mbox->irq < 0) + return mbox->irq; + ret = devm_request_irq(&pdev->dev, mbox->irq, tegra_xusb_mbox_irq, 0, + dev_name(&pdev->dev), mbox); + if (ret < 0) + return ret; + + ret = mbox_controller_register(&mbox->mbox); + if (ret < 0) + dev_err(&pdev->dev, "failed to register mailbox: %d\n", ret); + + return ret; +} + +static int tegra_xusb_mbox_remove(struct platform_device *pdev) +{ + struct tegra_xusb_mbox *mbox = platform_get_drvdata(pdev); + + mbox_controller_unregister(&mbox->mbox); + + return 0; +} + +static struct platform_driver tegra_xusb_mbox_driver = { + .probe = tegra_xusb_mbox_probe, + .remove = tegra_xusb_mbox_remove, + .driver = { + .name = "tegra-xusb-mbox", + .of_match_table = of_match_ptr(tegra_xusb_mbox_of_match), + }, +}; +module_platform_driver(tegra_xusb_mbox_driver); + +MODULE_AUTHOR("Andrew Bresticker <abrestic@chromium.org>"); +MODULE_DESCRIPTION("NVIDIA Tegra XUSB mailbox driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("platform:tegra-xusb-mailbox"); diff --git a/include/soc/tegra/xusb.h b/include/soc/tegra/xusb.h new file mode 100644 index 0000000..8efef8c --- /dev/null +++ b/include/soc/tegra/xusb.h @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2014 NVIDIA Corporation + * Copyright (C) 2014 Google, Inc. + * + * This program is free software; you can redistribute it and/or modify it + * under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + */ + +#ifndef __SOC_TEGRA_XUSB_H__ +#define __SOC_TEGRA_XUSB_H__ + +#define TEGRA_XUSB_MBOX_NUM_CHANS 2 /* host + phy */ + +/* Command requests from the firmware */ +enum tegra_xusb_mbox_cmd { + MBOX_CMD_MSG_ENABLED = 1, + MBOX_CMD_INC_FALC_CLOCK, + MBOX_CMD_DEC_FALC_CLOCK, + MBOX_CMD_INC_SSPI_CLOCK, + MBOX_CMD_DEC_SSPI_CLOCK, + MBOX_CMD_SET_BW, /* no ACK/NAK required */ + MBOX_CMD_SET_SS_PWR_GATING, + MBOX_CMD_SET_SS_PWR_UNGATING, + MBOX_CMD_SAVE_DFE_CTLE_CTX, + MBOX_CMD_AIRPLANE_MODE_ENABLED, /* unused */ + MBOX_CMD_AIRPLANE_MODE_DISABLED, /* unused */ + MBOX_CMD_START_HSIC_IDLE, + MBOX_CMD_STOP_HSIC_IDLE, + MBOX_CMD_DBC_WAKE_STACK, /* unused */ + MBOX_CMD_HSIC_PRETEND_CONNECT, + + MBOX_CMD_MAX, + + /* Response message to above commands */ + MBOX_CMD_ACK = 128, + MBOX_CMD_NAK +}; + +struct tegra_xusb_mbox_msg { + u32 cmd; + u32 data; +}; + +#endif /* __SOC_TEGRA_XUSB_H__ */
The Tegra xHCI controller's firmware communicates requests to the host processor through a mailbox interface. While there is only a single communication channel, messages sent by the controller can be divided into two groups: those intended for the PHY driver and those intended for the host-controller driver. This mailbox driver exposes the two channels and routes incoming messages to the appropriate channel based on the command encoded in the message. Signed-off-by: Andrew Bresticker <abrestic@chromium.org> --- Changes from v1: - Converted to common mailbox framework. - Removed useless polling sequences in TX path. - Moved xusb include from linux/ to soc/tegra/ --- drivers/mailbox/Kconfig | 3 + drivers/mailbox/Makefile | 2 + drivers/mailbox/tegra-xusb-mailbox.c | 279 +++++++++++++++++++++++++++++++++++ include/soc/tegra/xusb.h | 45 ++++++ 4 files changed, 329 insertions(+) create mode 100644 drivers/mailbox/tegra-xusb-mailbox.c create mode 100644 include/soc/tegra/xusb.h