Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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 =
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On 08/27/2014 11:38 AM, Andrew Bresticker wrote:
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/27/2014 11:38 AM, Andrew Bresticker wrote: On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On 08/27/2014 12:13 PM, Andrew Bresticker wrote: On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org wrote: On 08/27/2014 11:38 AM, Andrew Bresticker wrote: On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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. -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On 27 August 2014 23:08, Andrew Bresticker abres...@chromium.org wrote: On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On Wed, Aug 27, 2014 at 11:19 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/27/2014 12:13 PM, Andrew Bresticker wrote:
On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/27/2014 11:38 AM, Andrew Bresticker wrote:
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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@7009 {
compatible = nvidia,tegra124-xusb;
reg = 0x0 0x7009 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.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
On 08/27/2014 03:56 PM, Andrew Bresticker wrote:
On Wed, Aug 27, 2014 at 11:19 AM, Stephen Warren swar...@wwwdotorg.org wrote:
On 08/27/2014 12:13 PM, Andrew Bresticker wrote:
On Wed, Aug 27, 2014 at 10:50 AM, Stephen Warren swar...@wwwdotorg.org
wrote:
On 08/27/2014 11:38 AM, Andrew Bresticker wrote:
On Mon, Aug 25, 2014 at 12:01 PM, Stephen Warren swar...@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@7009 {
compatible = nvidia,tegra124-xusb;
reg = 0x0 0x7009 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:-)
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 pgp50o3bXWYAi.pgp Description: PGP signature
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 pgpGQVHpun82y.pgp Description: PGP signature
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 4852529 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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 4852529 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 pgptatR6DBSFl.pgp Description: PGP signature
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 4852529 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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 4852529 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 pgp2gIqEyM2cr.pgp Description: PGP signature
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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 pgp0IgrLtc5cC.pgp Description: PGP signature
Re: [PATCH v2 2/9] mailbox: Add NVIDIA Tegra XUSB mailbox driver
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.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
