| Message ID | 1437134647-28298-4-git-send-email-lee.jones@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Tue, 21 Jul 2015, Jassi Brar wrote: > On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <lee.jones@linaro.org> wrote: > > ST's platforms currently support a maximum of 5 Mailboxes, one for > > each of the supported co-processors situated on the platform. Each > > Mailbox is divided up into 4 instances which consist of 32 channels. > > Messages are passed between the application and co-processors using > > shared memory areas. It is the Client's responsibility to manage > > these areas. > > > Thanks. It's a lot better than the old driver. However a few nits as usual :) Never a problem. :) > > + > > +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */ > > > Above you say 5 instances. Another u32 doesn't cost much. 4 instances, 5 mailboxes. > > +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */ > > + > This assumption is reasonable. > > > + > > +static void sti_mbox_enable_channel(struct mbox_chan *chan) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct sti_mbox_device *mdev = chan_info->mdev; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + unsigned int instance = chan_info->instance; > > + unsigned int channel = chan_info->channel; > > + unsigned long flags; > > + void __iomem *base; > > + > > + base = mdev->base + (instance * sizeof(u32)); > > + > Maybe have something simpler like MBOX_BASE(instance)? Or some inline > function to avoid this 5-lines ritual? I think some of the functions also make use of the intermediary pointers, but I'll look into it. > > + spin_lock_irqsave(&sti_mbox_chan_lock, flags); > > + mdev->enabled[instance] |= BIT(channel); > > + writel_relaxed(BIT(channel), base + pdata->ena_set); > > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags); > > > You don't need locking for SET/CLR type registers which are meant for > when they could be accessed by processors that can not share a lock. > So maybe drop the lock here and elsewhere. Okay. > However, you need some mechanism to check if you succeeded 'owning' > the channel by reading back what you write to own the channel (not > sure which is that register here). Usually we need that action and > verification when we assign a channel to some user. I don't think there is a technical reason why it wouldn't succeed. We don't normally read back every register change me make. Why is this IP different? > > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct sti_mbox_device *mdev = chan_info->mdev; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + unsigned int instance = chan_info->instance; > > + unsigned int channel = chan_info->channel; > > + void __iomem *base; > > + > > + if (!sti_mbox_tx_is_ready(chan)) > > + return -EBUSY; > This is the first thing I look out for in every new driver :) this > check is unnecessary. In what way? What if the channel is disabled or there is an IRQ already pending? > > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct mbox_controller *mbox = chan_info->mdev->mbox; > > + int i; > > + > > + for (i = 0; i < mbox->num_chans; i++) > > + if (chan == &mbox->chans[i]) > > + break; > > + > > + if (mbox->num_chans == i) { > > + dev_warn(mbox->dev, "Request to free non-existent channel\n"); > > + return; > > + } > > + > > + sti_mbox_disable_channel(chan); > > + sti_mbox_clear_irq(chan); > > + > > + /* Reset channel */ > > + memset(chan, 0, sizeof(*chan)); > > + chan->mbox = mbox; > > + chan->txdone_method = TXDONE_BY_POLL; > > > No please. mbox_chan is owned by the API. At most you could clear con_priv. I will look for the API call to reset the channel then. > > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox, > > + const struct of_phandle_args *spec) > > +{ > > + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev); > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + struct sti_channel *chan_info; > > + struct mbox_chan *chan = NULL; > > + unsigned int instance = spec->args[0]; > > + unsigned int channel = spec->args[1]; > > + unsigned int direction = spec->args[2]; > > + int i; > > + > > + /* Bounds checking */ > > + if (instance >= pdata->num_inst || channel >= pdata->num_chan) { > > + dev_err(mbox->dev, > > + "Invalid channel requested instance: %d channel: %d\n", > > + instance, channel); > > + return NULL; > > + } > > + > > + for (i = 0; i < mbox->num_chans; i++) { > > + chan_info = mbox->chans[i].con_priv; > > + > > + /* Is requested channel free? */ > > + if (direction != MBOX_LOOPBACK && > > + chan_info && > > + mbox->dev == chan_info->mdev->dev && > > + instance == chan_info->instance && > > + channel == chan_info->channel) { > > + dev_err(mbox->dev, "Channel in use\n"); > > + return NULL; > > + } > > + > > + /* Find the first free slot */ > > + if (!chan && !chan_info) > > + chan = &mbox->chans[i]; > shouldn't it break out of loop here? Yes, I guess it should. Good spot. > > + } > > + > Doesn't mbox->chans[i].con_priv need some locking here? I can add some. > > +static const struct sti_mbox_pdata mbox_stih407_pdata = { > > + .num_inst = 4, > > + .num_chan = 32, > > + .irq_val = 0x04, > > + .irq_set = 0x24, > > + .irq_clr = 0x44, > > + .ena_val = 0x64, > > + .ena_set = 0x84, > > + .ena_clr = 0xa4, > > > Register offsets are parameters of the controller And this is a controller driver? Not sure I get the point. > and also these look ugly. Please make these #define's Sure. > > +static int __init sti_mbox_init(void) > > +{ > > + return platform_driver_register(&sti_mbox_driver); > > +} > > + > > +static void __exit sti_mbox_exit(void) > > +{ > > + platform_driver_unregister(&sti_mbox_driver); > > +} > > + > > +postcore_initcall(sti_mbox_init); > > > This seems fragile. Shouldn't the users defer probe if they don't get a channel? I'm not sure why we have to be early. I will investigate.
On Tue, 21 Jul 2015, Jassi Brar wrote: > On Tue, Jul 21, 2015 at 8:36 PM, Lee Jones <lee.jones@linaro.org> wrote: > > On Tue, 21 Jul 2015, Jassi Brar wrote: > > > >> However, you need some mechanism to check if you succeeded 'owning' > >> the channel by reading back what you write to own the channel (not > >> sure which is that register here). Usually we need that action and > >> verification when we assign a channel to some user. > > > > I don't think there is a technical reason why it wouldn't succeed. We > > don't normally read back every register change me make. Why is this > > IP different? > > > Not the IP, but register access. SET and CLR type registers work on > individual bits because the processors don't share a lock that > protects register read->modify->write. > > Usually there is also some flag that is set to some unique value to > claim ownership of the resource, and if two processors try to claim > simultaneously we need to check if the flag reads back the value we > set to assert claim. There should be some such flag/register but as I > said I don't know if/which. Is there? The only registers we have available are; read_irq_value, set_irq_value, clear_irq_value, read_enable_value, set_enable_value and clear_enable_values. This controller doesn't claim ownership of anything. It essentially just turns IRQs on and off. > >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data) > >> > +{ > >> > + struct sti_channel *chan_info = chan->con_priv; > >> > + struct sti_mbox_device *mdev = chan_info->mdev; > >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > >> > + unsigned int instance = chan_info->instance; > >> > + unsigned int channel = chan_info->channel; > >> > + void __iomem *base; > >> > + > >> > + if (!sti_mbox_tx_is_ready(chan)) > >> > + return -EBUSY; > >> This is the first thing I look out for in every new driver :) this > >> check is unnecessary. > > > > In what way? What if the channel is disabled or there is an IRQ > > already pending? > > > API calls send_data() only if last_tx_done() returned true. I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to fire, because I have triggered them. I'd really rather keep this harmless check in. > >> > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan) > >> > +{ > >> > + struct sti_channel *chan_info = chan->con_priv; > >> > + struct mbox_controller *mbox = chan_info->mdev->mbox; > >> > + int i; > >> > + > >> > + for (i = 0; i < mbox->num_chans; i++) > >> > + if (chan == &mbox->chans[i]) > >> > + break; > >> > + > >> > + if (mbox->num_chans == i) { > >> > + dev_warn(mbox->dev, "Request to free non-existent channel\n"); > >> > + return; > >> > + } > >> > + > >> > + sti_mbox_disable_channel(chan); > >> > + sti_mbox_clear_irq(chan); > >> > + > >> > + /* Reset channel */ > >> > + memset(chan, 0, sizeof(*chan)); > >> > + chan->mbox = mbox; > >> > + chan->txdone_method = TXDONE_BY_POLL; > >> > > >> No please. mbox_chan is owned by the API. At most you could clear con_priv. > > > > I will look for the API call to reset the channel then. > > > API internally does any needed reset. Okay, thanks for the clarification, I will remove these lines. > >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = { > >> > + .num_inst = 4, > >> > + .num_chan = 32, > >> > + .irq_val = 0x04, > >> > + .irq_set = 0x24, > >> > + .irq_clr = 0x44, > >> > + .ena_val = 0x64, > >> > + .ena_set = 0x84, > >> > + .ena_clr = 0xa4, > >> > > >> Register offsets are parameters of the controller > > > > And this is a controller driver? Not sure I get the point. > > > Platform_data usually carries board/platform specific parameters. > Register offset "properties" are as fixed as the behavior of the > controller, so they should stay inside the code, not come via > platform_data. That's not the case for this controller. There is nothing preventing these values from changing on a new board revisions. After all, this isn't really a Controller IP per-say. AFAIK, all current platforms do use this mapping, so I can change it with a view to changing it back if a different set of offsets appear in subsequent incarnations. But again, I think it's pretty harmless.
On Tue, 21 Jul 2015, Jassi Brar wrote: > On Tue, Jul 21, 2015 at 9:22 PM, Lee Jones <lee.jones@linaro.org> wrote: > > On Tue, 21 Jul 2015, Jassi Brar wrote: > > > >> > > >> >> > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data) > >> >> > +{ > >> >> > + struct sti_channel *chan_info = chan->con_priv; > >> >> > + struct sti_mbox_device *mdev = chan_info->mdev; > >> >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > >> >> > + unsigned int instance = chan_info->instance; > >> >> > + unsigned int channel = chan_info->channel; > >> >> > + void __iomem *base; > >> >> > + > >> >> > + if (!sti_mbox_tx_is_ready(chan)) > >> >> > + return -EBUSY; > >> >> This is the first thing I look out for in every new driver :) this > >> >> check is unnecessary. > >> > > >> > In what way? What if the channel is disabled or there is an IRQ > >> > already pending? > >> > > >> API calls send_data() only if last_tx_done() returned true. > > > > I know for a fact that the 'catchers' in sti_mbox_tx_is_ready() to > > fire, because I have triggered them. I'd really rather keep this > > harmless check in. > > > If you put some printk in send_data() and last_tx_done() you'll see > what I mean :) > > >> >> > +static const struct sti_mbox_pdata mbox_stih407_pdata = { > >> >> > + .num_inst = 4, > >> >> > + .num_chan = 32, > >> >> > + .irq_val = 0x04, > >> >> > + .irq_set = 0x24, > >> >> > + .irq_clr = 0x44, > >> >> > + .ena_val = 0x64, > >> >> > + .ena_set = 0x84, > >> >> > + .ena_clr = 0xa4, > >> >> > > >> >> Register offsets are parameters of the controller > >> > > >> > And this is a controller driver? Not sure I get the point. > >> > > >> Platform_data usually carries board/platform specific parameters. > >> Register offset "properties" are as fixed as the behavior of the > >> controller, so they should stay inside the code, not come via > >> platform_data. > > > > That's not the case for this controller. There is nothing preventing > > these values from changing on a new board revisions. > > > Hmm ... interesting! Can't see how enable/disable channel and irq > set/clear could be effected by writing to random, but agreed upon, > location between two processors. There ought to be some controller > listening there? Now I am more interested in knowing this IP :) High level ---------- MB0 MB1 MB2 MB3 MB4 +---------+---------+---------+---------+---------+ INST0 | | | | | | +---------+---------+---------+---------+---------+ INST1 | | | | | | +---------+---------+---------+---------+---------+ INST2 | | | | | | +---------+---------+---------+---------+---------+ INST3 | | | | | | +---------+---------+---------+---------+---------+ Low level [each box above looks like this) ------------------------------------------ 1 32 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ IRQ_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ IRQ_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ IRQ_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ENB_VAL | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ENB_SET | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ ENB_CLR | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | | +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ That's it. That's the entirety of the "IP".
Let's carry on the conversation here, rather than submitting my fixed up patch. Once agreed, I will re-send. > On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <lee.jones@linaro.org> wrote: > > ST's platforms currently support a maximum of 5 Mailboxes, one for > > each of the supported co-processors situated on the platform. Each > > Mailbox is divided up into 4 instances which consist of 32 channels. > > Messages are passed between the application and co-processors using > > shared memory areas. It is the Client's responsibility to manage > > these areas. > > > Thanks. It's a lot better than the old driver. However a few nits as usual :) > > > + > > +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */ > > > Above you say 5 instances. Another u32 doesn't cost much. This should stay the same. There are 4 instances. > > +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */ > > + > This assumption is reasonable. > > > + > > +static void sti_mbox_enable_channel(struct mbox_chan *chan) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct sti_mbox_device *mdev = chan_info->mdev; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + unsigned int instance = chan_info->instance; > > + unsigned int channel = chan_info->channel; > > + unsigned long flags; > > + void __iomem *base; > > + > > + base = mdev->base + (instance * sizeof(u32)); > > + > Maybe have something simpler like MBOX_BASE(instance)? Or some inline > function to avoid this 5-lines ritual? I've checked and we can't do this, as the we need most (all?) of the intermediary variables too. No ritual just to get the final variable for instance. > > + spin_lock_irqsave(&sti_mbox_chan_lock, flags); > > + mdev->enabled[instance] |= BIT(channel); > > + writel_relaxed(BIT(channel), base + pdata->ena_set); > > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags); > > > You don't need locking for SET/CLR type registers which are meant for > when they could be accessed by processors that can not share a lock. > So maybe drop the lock here and elsewhere. From what I can gather, I think we need this locking. What happens if we get scheduled between setting the enabled bit in our cache and actually setting the ena_set bit? We would be out of sync. > However, you need some mechanism to check if you succeeded 'owning' > the channel by reading back what you write to own the channel (not > sure which is that register here). Usually we need that action and > verification when we assign a channel to some user. I don't think we need to do that with this driver. All of the allocation is controlled from within this code base. The channels are pre-allocated and written into the co-proc's Firmware. > > +static int sti_mbox_send_data(struct mbox_chan *chan, void *data) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct sti_mbox_device *mdev = chan_info->mdev; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + unsigned int instance = chan_info->instance; > > + unsigned int channel = chan_info->channel; > > + void __iomem *base; > > + > > + if (!sti_mbox_tx_is_ready(chan)) > > + return -EBUSY; > This is the first thing I look out for in every new driver :) this > check is unnecessary. I see what you mean now. I will remove this check. > > +static void sti_mbox_shutdown_chan(struct mbox_chan *chan) > > +{ > > + struct sti_channel *chan_info = chan->con_priv; > > + struct mbox_controller *mbox = chan_info->mdev->mbox; > > + int i; > > + > > + for (i = 0; i < mbox->num_chans; i++) > > + if (chan == &mbox->chans[i]) > > + break; > > + > > + if (mbox->num_chans == i) { > > + dev_warn(mbox->dev, "Request to free non-existent channel\n"); > > + return; > > + } > > + > > + sti_mbox_disable_channel(chan); > > + sti_mbox_clear_irq(chan); > > + > > + /* Reset channel */ > > + memset(chan, 0, sizeof(*chan)); > > + chan->mbox = mbox; > > + chan->txdone_method = TXDONE_BY_POLL; > > > No please. mbox_chan is owned by the API. At most you could clear con_priv. Removed. > > +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox, > > + const struct of_phandle_args *spec) > > +{ > > + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev); > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + struct sti_channel *chan_info; > > + struct mbox_chan *chan = NULL; > > + unsigned int instance = spec->args[0]; > > + unsigned int channel = spec->args[1]; > > + unsigned int direction = spec->args[2]; > > + int i; > > + > > + /* Bounds checking */ > > + if (instance >= pdata->num_inst || channel >= pdata->num_chan) { > > + dev_err(mbox->dev, > > + "Invalid channel requested instance: %d channel: %d\n", > > + instance, channel); > > + return NULL; > > + } > > + > > + for (i = 0; i < mbox->num_chans; i++) { > > + chan_info = mbox->chans[i].con_priv; > > + > > + /* Is requested channel free? */ > > + if (direction != MBOX_LOOPBACK && > > + chan_info && > > + mbox->dev == chan_info->mdev->dev && > > + instance == chan_info->instance && > > + channel == chan_info->channel) { > > + dev_err(mbox->dev, "Channel in use\n"); > > + return NULL; > > + } > > + > > + /* Find the first free slot */ > > + if (!chan && !chan_info) > > + chan = &mbox->chans[i]; > shouldn't it break out of loop here? I checked that and we don't. We need to check all of the channels do wee if we have double allocated. If we have, NULL is returned from above. I have; however, updated the comment above for clarity. > > + } > > + > Doesn't mbox->chans[i].con_priv need some locking here? Not that I can see. Would you mind explaining further please? > > +static const struct sti_mbox_pdata mbox_stih407_pdata = { > > + .num_inst = 4, > > + .num_chan = 32, > > + .irq_val = 0x04, > > + .irq_set = 0x24, > > + .irq_clr = 0x44, > > + .ena_val = 0x64, > > + .ena_set = 0x84, > > + .ena_clr = 0xa4, > > > Register offsets are parameters of the controller, and also these look > ugly. Please make these #define's These are now #defined. > > +static int __init sti_mbox_init(void) > > +{ > > + return platform_driver_register(&sti_mbox_driver); > > +} > > + > > +static void __exit sti_mbox_exit(void) > > +{ > > + platform_driver_unregister(&sti_mbox_driver); > > +} > > + > > +postcore_initcall(sti_mbox_init); > > > This seems fragile. Shouldn't the users defer probe if they don't get a channel? This has been converted to module_platform_driver()
On Thu, 23 Jul 2015, Jassi Brar wrote: > On Thu, Jul 23, 2015 at 1:59 PM, Lee Jones <lee.jones@linaro.org> wrote: > >> On Fri, Jul 17, 2015 at 5:34 PM, Lee Jones <lee.jones@linaro.org> wrote: > > >> > +static void sti_mbox_enable_channel(struct mbox_chan *chan) > >> > +{ > >> > + struct sti_channel *chan_info = chan->con_priv; > >> > + struct sti_mbox_device *mdev = chan_info->mdev; > >> > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > >> > + unsigned int instance = chan_info->instance; > >> > + unsigned int channel = chan_info->channel; > >> > + unsigned long flags; > >> > + void __iomem *base; > >> > + > >> > + base = mdev->base + (instance * sizeof(u32)); > >> > + > >> Maybe have something simpler like MBOX_BASE(instance)? Or some inline > >> function to avoid this 5-lines ritual? > > > > I've checked and we can't do this, as the we need most (all?) of the > > intermediary variables too. No ritual just to get the final variable > > for instance. > > > OK. How about ? > #define MBOX_BASE(m, n) ((m)->base + (n) * 4) > void __iomem *base = MBOX_BASE(mdev, instance); Oh, those 5 lines. I thought you meant: struct sti_channel *chan_info = chan->con_priv; struct sti_mbox_device *mdev = chan_info->mdev; unsigned int instance = chan_info->instance; base = mdev->base + (instance * sizeof(u32)); ... which is why I said that the intermediary variables are required. Well, I 'can' do that, but it seems to be unnecessarily obfuscating what's going on and doesn't actually save any lines. It's not a point that I consider arguing over though, so if you want me to do it, I will. You have the final say here. > >> > + spin_lock_irqsave(&sti_mbox_chan_lock, flags); > >> > + mdev->enabled[instance] |= BIT(channel); > >> > + writel_relaxed(BIT(channel), base + pdata->ena_set); > >> > + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags); > >> > > >> You don't need locking for SET/CLR type registers which are meant for > >> when they could be accessed by processors that can not share a lock. > >> So maybe drop the lock here and elsewhere. > > > > From what I can gather, I think we need this locking. What happens if > > we get scheduled between setting the enabled bit in our cache and > > actually setting the ena_set bit? We would be out of sync. > > > IIU what you mean... can't that still happen because of the _relaxed()? Not sure what you mean. The _relaxed variant merely omit some IO barriers. > Anyways my point was about set/clr type regs. And you may look at if > channel really needs disabling while interrupts are handled? I think > it shouldn't because either it is going to be a to-fro communication > or random broadcasts without any guarantee of reception. But of course > you would know better your platform. I'd certainly feel more comfortable if we kept this part of the semantics, as it's how the platform experts at ST originally wrote the driver, and they know a lot more about the protocols used than I. > BTW sti_mbox_channel_is_enabled() also needs to have locking around > enabled[] if you want to keep it. Done. > And maybe embed sti_mbox_chan_lock inside sti_mbox_device. Not sure this is required. I can find >600 instances of others using spinlocks as static globals. > >> Doesn't mbox->chans[i].con_priv need some locking here? > > > > Not that I can see. Would you mind explaining further please? > > > Not anymore, after the clarification that we don't need a 'break' statement. Great, thanks.
On Thu, 23 Jul 2015, Alexey Klimov wrote: > On Fri, Jul 17, 2015 at 3:04 PM, Lee Jones <lee.jones@linaro.org> wrote: > > ST's platforms currently support a maximum of 5 Mailboxes, one for > > each of the supported co-processors situated on the platform. Each > > Mailbox is divided up into 4 instances which consist of 32 channels. > > Messages are passed between the application and co-processors using > > shared memory areas. It is the Client's responsibility to manage > > these areas. > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org> > > --- > > drivers/mailbox/Kconfig | 7 + > > drivers/mailbox/Makefile | 2 + > > drivers/mailbox/mailbox-sti.c | 562 ++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 571 insertions(+) > > create mode 100644 drivers/mailbox/mailbox-sti.c > > [..] > > > +static irqreturn_t sti_mbox_thread_handler(int irq, void *data) > > +{ > > + struct sti_mbox_device *mdev = data; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + struct mbox_chan *chan; > > + unsigned int instance; > > + > > + for (instance = 0; instance < pdata->num_inst; instance++) { > > +keep_looking: > > + chan = sti_mbox_irq_to_channel(mdev, instance); > > + if (!chan) > > + continue; > > + > > + mbox_chan_received_data(chan, NULL); > > + sti_mbox_clear_irq(chan); > > + sti_mbox_enable_channel(chan); > > + goto keep_looking; > > + } > > + > > + return IRQ_HANDLED; > > +} > > + > > +static irqreturn_t sti_mbox_irq_handler(int irq, void *data) > > +{ > > + struct sti_mbox_device *mdev = data; > > + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); > > + struct sti_channel *chan_info; > > + struct mbox_chan *chan; > > + unsigned int instance; > > + int ret = IRQ_NONE; > > + > > + for (instance = 0; instance < pdata->num_inst; instance++) { > > + chan = sti_mbox_irq_to_channel(mdev, instance); > > + if (!chan) > > + continue; > > + chan_info = chan->con_priv; > > + > > + if (!sti_mbox_channel_is_enabled(chan)) { > > + dev_warn(mdev->dev, > > + "Unexpected IRQ: %s\n" > > + " instance: %d: channel: %d [enabled: %x]\n", > > + mdev->name, chan_info->instance, > > + chan_info->channel, mdev->enabled[instance]); > > + ret = IRQ_HANDLED; > > + continue; > > + } > > + > > + sti_mbox_disable_channel(chan); > > + ret = IRQ_WAKE_THREAD; > > + } > > + > > + if (ret == IRQ_NONE) > > + dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n"); > > + > > + return ret; > > +} > > With such usage of ret variable can it happen that handling of last > but one channel/instance will set ret to IRQ_WAKE_THREAD and at the > same time handling of last channel/instance will set ret to > IRQ_HANDLED during iteration loop and finally generic subsystem will > not wake up thread handler because it will receive IRQ_HANDLED? > Just checking. Yes, I guess that it theoretically possible. Now fixed. Thanks for the spot.
diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig index e269f08..2cc4c39 100644 --- a/drivers/mailbox/Kconfig +++ b/drivers/mailbox/Kconfig @@ -70,4 +70,11 @@ config BCM2835_MBOX the services of the Videocore. Say Y here if you want to use the BCM2835 Mailbox. +config STI_MBOX + tristate "STI Mailbox framework support" + depends on ARCH_STI && OF + help + Mailbox implementation for STMicroelectonics family chips with + hardware for interprocessor communication. + endif diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile index 8e6d822..7cb4766 100644 --- a/drivers/mailbox/Makefile +++ b/drivers/mailbox/Makefile @@ -13,3 +13,5 @@ obj-$(CONFIG_PCC) += pcc.o obj-$(CONFIG_ALTERA_MBOX) += mailbox-altera.o obj-$(CONFIG_BCM2835_MBOX) += bcm2835-mailbox.o + +obj-$(CONFIG_STI_MBOX) += mailbox-sti.o diff --git a/drivers/mailbox/mailbox-sti.c b/drivers/mailbox/mailbox-sti.c new file mode 100644 index 0000000..0c82f7b --- /dev/null +++ b/drivers/mailbox/mailbox-sti.c @@ -0,0 +1,562 @@ +/* + * STi Mailbox + * + * Copyright (C) 2015 ST Microelectronics + * + * Author: Lee Jones <lee.jones@linaro.org> for ST Microelectronics + * + * Based on the original driver written by; + * Alexandre Torgue, Olivier Lebreton and Loic Pallardy + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/mailbox_controller.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/of_device.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <dt-bindings/mailbox/mailbox.h> + +#include "mailbox.h" + +#define STI_MBOX_INST_MAX 4 /* RAM saving: Max supported instances */ +#define STI_MBOX_CHAN_MAX 20 /* RAM saving: Max supported channels */ + +static DEFINE_SPINLOCK(sti_mbox_chan_lock); + +/** + * STi Mailbox device data + * + * An IP Mailbox is currently composed of 4 instances + * Each instance is currently composed of 32 channels + * This means that we have 128 channels per Mailbox + * A channel an be used for TX or RX + * + * @dev: Device to which it is attached + * @mbox: Representation of a communication channel controller + * @base: Base address of the register mapping region + * @name: Name of the mailbox + * @enabled: Local copy of enabled channels + */ +struct sti_mbox_device { + struct device *dev; + struct mbox_controller *mbox; + void __iomem *base; + const char *name; + u32 enabled[STI_MBOX_INST_MAX]; +}; + +/** + * STi Mailbox platform specfic configuration + * + * @num_inst: Maximum number of instances in one HW Mailbox + * @num_chan: Maximum number of channel per instance + * @irq_val: Register offset to read interrupt status + * @irq_set: Register offset to generate a Tx channel interrupt + * @irq_clr: Register offset to clear pending Rx interrupts + * @ena_val: Register offset to read enable status + * @ena_set: Register offset to enable a channel + * @ena_clr: Register offset to disable a channel + */ +struct sti_mbox_pdata { + unsigned int num_inst; + unsigned int num_chan; + unsigned int irq_val; + unsigned int irq_set; + unsigned int irq_clr; + unsigned int ena_val; + unsigned int ena_set; + unsigned int ena_clr; +}; + +/** + * STi Mailbox allocated channel information + * + * @mdev: Pointer to parent Mailbox device + * @instance: Instance number channel resides in + * @channel: Channel number pertaining to this container + * @direction: Direction which data will travel in through the channel (Tx/Rx) + */ +struct sti_channel { + struct sti_mbox_device *mdev; + unsigned int instance; + unsigned int channel; + unsigned int direction; +}; + +static inline bool sti_mbox_channel_is_enabled(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + + return mdev->enabled[instance] & BIT(channel); +} + +static inline +struct mbox_chan *sti_mbox_to_channel(struct mbox_controller *mbox, + unsigned int instance, + unsigned int channel) +{ + struct sti_channel *chan_info; + int i; + + for (i = 0; i < mbox->num_chans; i++) { + chan_info = mbox->chans[i].con_priv; + if (chan_info && + chan_info->instance == instance && + chan_info->channel == channel) + return &mbox->chans[i]; + } + + dev_err(mbox->dev, + "Channel not registered: instance: %d channel: %d\n", + instance, channel); + + return NULL; +} + +static void sti_mbox_enable_channel(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + unsigned long flags; + void __iomem *base; + + base = mdev->base + (instance * sizeof(u32)); + + spin_lock_irqsave(&sti_mbox_chan_lock, flags); + mdev->enabled[instance] |= BIT(channel); + writel_relaxed(BIT(channel), base + pdata->ena_set); + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags); +} + +static void sti_mbox_disable_channel(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + unsigned long flags; + void __iomem *base; + + base = mdev->base + (instance * sizeof(u32)); + + spin_lock_irqsave(&sti_mbox_chan_lock, flags); + mdev->enabled[instance] &= ~BIT(channel); + writel_relaxed(BIT(channel), base + pdata->ena_clr); + spin_unlock_irqrestore(&sti_mbox_chan_lock, flags); +} + +static void sti_mbox_clear_irq(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + void __iomem *base; + + base = mdev->base + (instance * sizeof(u32)); + + writel_relaxed(BIT(channel), base + pdata->irq_clr); +} + +static struct mbox_chan *sti_mbox_irq_to_channel(struct sti_mbox_device *mdev, + unsigned int instance) +{ + struct mbox_controller *mbox = mdev->mbox; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + struct mbox_chan *chan = NULL; + unsigned int channel; + unsigned long bits; + void __iomem *base; + + base = mdev->base + (instance * sizeof(u32)); + + bits = readl_relaxed(base + pdata->irq_val); + if (!bits) + /* No IRQs fired in specified instance */ + return NULL; + + /* An IRQ has fired, find the associated channel */ + for (channel = 0; bits; channel++) { + if (!test_and_clear_bit(channel, &bits)) + continue; + + chan = sti_mbox_to_channel(mbox, instance, channel); + if (chan) { + dev_dbg(mbox->dev, + "IRQ fired on instance: %d channel: %d\n", + instance, channel); + break; + } + } + + return chan; +} + +static irqreturn_t sti_mbox_thread_handler(int irq, void *data) +{ + struct sti_mbox_device *mdev = data; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + struct mbox_chan *chan; + unsigned int instance; + + for (instance = 0; instance < pdata->num_inst; instance++) { +keep_looking: + chan = sti_mbox_irq_to_channel(mdev, instance); + if (!chan) + continue; + + mbox_chan_received_data(chan, NULL); + sti_mbox_clear_irq(chan); + sti_mbox_enable_channel(chan); + goto keep_looking; + } + + return IRQ_HANDLED; +} + +static irqreturn_t sti_mbox_irq_handler(int irq, void *data) +{ + struct sti_mbox_device *mdev = data; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + struct sti_channel *chan_info; + struct mbox_chan *chan; + unsigned int instance; + int ret = IRQ_NONE; + + for (instance = 0; instance < pdata->num_inst; instance++) { + chan = sti_mbox_irq_to_channel(mdev, instance); + if (!chan) + continue; + chan_info = chan->con_priv; + + if (!sti_mbox_channel_is_enabled(chan)) { + dev_warn(mdev->dev, + "Unexpected IRQ: %s\n" + " instance: %d: channel: %d [enabled: %x]\n", + mdev->name, chan_info->instance, + chan_info->channel, mdev->enabled[instance]); + ret = IRQ_HANDLED; + continue; + } + + sti_mbox_disable_channel(chan); + ret = IRQ_WAKE_THREAD; + } + + if (ret == IRQ_NONE) + dev_err(mdev->dev, "Spurious IRQ - was a channel requested?\n"); + + return ret; +} + +static bool sti_mbox_tx_is_ready(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + void __iomem *base; + + base = mdev->base + (instance * sizeof(u32)); + + if (!(chan_info->direction & MBOX_TX)) + return false; + + if (!(readl_relaxed(base + pdata->ena_val) & BIT(channel))) { + dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d disabled\n", + mdev->name, instance, channel); + return false; + } + + if (readl_relaxed(base + pdata->irq_val) & BIT(channel)) { + dev_dbg(mdev->dev, "Mbox: %s: inst: %d, chan: %d not ready\n", + mdev->name, instance, channel); + return false; + } + + return true; +} + +static int sti_mbox_send_data(struct mbox_chan *chan, void *data) +{ + struct sti_channel *chan_info = chan->con_priv; + struct sti_mbox_device *mdev = chan_info->mdev; + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + unsigned int instance = chan_info->instance; + unsigned int channel = chan_info->channel; + void __iomem *base; + + if (!sti_mbox_tx_is_ready(chan)) + return -EBUSY; + + base = mdev->base + (instance * sizeof(u32)); + + /* Send event to co-processor */ + writel_relaxed(BIT(channel), base + pdata->irq_set); + + dev_dbg(mdev->dev, + "Sent via Mailbox %s: instance: %d channel: %d\n", + mdev->name, instance, channel); + + return 0; +} + +static int sti_mbox_startup_chan(struct mbox_chan *chan) +{ + sti_mbox_clear_irq(chan); + sti_mbox_enable_channel(chan); + + return 0; +} + +static void sti_mbox_shutdown_chan(struct mbox_chan *chan) +{ + struct sti_channel *chan_info = chan->con_priv; + struct mbox_controller *mbox = chan_info->mdev->mbox; + int i; + + for (i = 0; i < mbox->num_chans; i++) + if (chan == &mbox->chans[i]) + break; + + if (mbox->num_chans == i) { + dev_warn(mbox->dev, "Request to free non-existent channel\n"); + return; + } + + sti_mbox_disable_channel(chan); + sti_mbox_clear_irq(chan); + + /* Reset channel */ + memset(chan, 0, sizeof(*chan)); + chan->mbox = mbox; + chan->txdone_method = TXDONE_BY_POLL; + spin_lock_init(&chan->lock); +} + +static struct mbox_chan *sti_mbox_xlate(struct mbox_controller *mbox, + const struct of_phandle_args *spec) +{ + struct sti_mbox_device *mdev = dev_get_drvdata(mbox->dev); + struct sti_mbox_pdata *pdata = dev_get_platdata(mdev->dev); + struct sti_channel *chan_info; + struct mbox_chan *chan = NULL; + unsigned int instance = spec->args[0]; + unsigned int channel = spec->args[1]; + unsigned int direction = spec->args[2]; + int i; + + /* Bounds checking */ + if (instance >= pdata->num_inst || channel >= pdata->num_chan) { + dev_err(mbox->dev, + "Invalid channel requested instance: %d channel: %d\n", + instance, channel); + return NULL; + } + + for (i = 0; i < mbox->num_chans; i++) { + chan_info = mbox->chans[i].con_priv; + + /* Is requested channel free? */ + if (direction != MBOX_LOOPBACK && + chan_info && + mbox->dev == chan_info->mdev->dev && + instance == chan_info->instance && + channel == chan_info->channel) { + dev_err(mbox->dev, "Channel in use\n"); + return NULL; + } + + /* Find the first free slot */ + if (!chan && !chan_info) + chan = &mbox->chans[i]; + } + + if (!chan) { + dev_err(mbox->dev, "No free channels left\n"); + return NULL; + } + + chan_info = devm_kzalloc(mbox->dev, sizeof(*chan_info), GFP_KERNEL); + if (!chan_info) + return NULL; + + chan_info->mdev = mdev; + chan_info->instance = instance; + chan_info->channel = channel; + chan_info->direction = direction; + + chan->con_priv = chan_info; + + dev_info(mbox->dev, + "Mbox: %s: Created channel:\n" + " instance: %d channel: %d direction: %s\n", + mdev->name, instance, channel, + direction == MBOX_LOOPBACK ? "Loopback" : + direction == MBOX_TX ? "Tx" : "Rx"); + + return chan; +} + +static struct mbox_chan_ops sti_mbox_ops = { + .startup = sti_mbox_startup_chan, + .shutdown = sti_mbox_shutdown_chan, + .send_data = sti_mbox_send_data, + .last_tx_done = sti_mbox_tx_is_ready, +}; + +static const struct sti_mbox_pdata mbox_stih407_pdata = { + .num_inst = 4, + .num_chan = 32, + .irq_val = 0x04, + .irq_set = 0x24, + .irq_clr = 0x44, + .ena_val = 0x64, + .ena_set = 0x84, + .ena_clr = 0xa4, +}; + +static const struct of_device_id sti_mailbox_match[] = { + { + .compatible = "st,stih407-mailbox", + .data = (void *)&mbox_stih407_pdata + }, + { } +}; + +static int sti_mbox_probe(struct platform_device *pdev) +{ + const struct of_device_id *match; + struct mbox_controller *mbox; + struct sti_mbox_device *mdev; + struct device_node *np = pdev->dev.of_node; + struct mbox_chan *chans; + struct resource *res; + int irq; + int ret; + + match = of_match_device(sti_mailbox_match, &pdev->dev); + if (!match) { + dev_err(&pdev->dev, "No configuration found\n"); + return -ENODEV; + } + pdev->dev.platform_data = (struct sti_mbox_pdata *) match->data; + + mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL); + if (!mdev) + return -ENOMEM; + + platform_set_drvdata(pdev, mdev); + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + mdev->base = devm_ioremap_resource(&pdev->dev, res); + if (!mdev->base) + return -ENOMEM; + + ret = of_property_read_string(np, "mbox-name", &mdev->name); + if (ret) + mdev->name = np->full_name; + + mbox = devm_kzalloc(&pdev->dev, sizeof(*mbox), GFP_KERNEL); + if (!mbox) + return -ENOMEM; + + chans = devm_kzalloc(&pdev->dev, + sizeof(*chans) * STI_MBOX_CHAN_MAX, GFP_KERNEL); + if (!chans) + return -ENOMEM; + + mdev->dev = &pdev->dev; + mdev->mbox = mbox; + + /* STi Mailbox does not have a Tx-Done or Tx-Ready IRQ */ + mbox->txdone_irq = false; + mbox->txdone_poll = true; + mbox->txpoll_period = 100; + mbox->ops = &sti_mbox_ops; + mbox->dev = mdev->dev; + mbox->of_xlate = sti_mbox_xlate; + mbox->chans = chans; + mbox->num_chans = STI_MBOX_CHAN_MAX; + + ret = mbox_controller_register(mbox); + if (ret) + return ret; + + /* It's okay for Tx Mailboxes to not supply IRQs */ + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + dev_info(&pdev->dev, + "%s: Registered Tx only Mailbox\n", mdev->name); + return 0; + } + + ret = devm_request_threaded_irq(&pdev->dev, irq, + sti_mbox_irq_handler, + sti_mbox_thread_handler, + IRQF_ONESHOT, mdev->name, mdev); + if (ret) { + dev_err(&pdev->dev, "Can't claim IRQ %d\n", irq); + mbox_controller_unregister(mbox); + return -EINVAL; + } + + dev_info(&pdev->dev, "%s: Registered Tx/Rx Mailbox\n", mdev->name); + + return 0; +} + +static int sti_mbox_remove(struct platform_device *pdev) +{ + struct sti_mbox_device *mdev = platform_get_drvdata(pdev); + + mbox_controller_unregister(mdev->mbox); + + return 0; +} + +static struct platform_driver sti_mbox_driver = { + .probe = sti_mbox_probe, + .remove = sti_mbox_remove, + .driver = { + .name = "sti-mailbox", + .of_match_table = sti_mailbox_match, + }, +}; + +static int __init sti_mbox_init(void) +{ + return platform_driver_register(&sti_mbox_driver); +} + +static void __exit sti_mbox_exit(void) +{ + platform_driver_unregister(&sti_mbox_driver); +} + +postcore_initcall(sti_mbox_init); +module_exit(sti_mbox_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("STMicroelectronics Mailbox Controller"); +MODULE_AUTHOR("Lee Jones <lee.jones@linaro.org"); +MODULE_ALIAS("platform:mailbox-sti");
ST's platforms currently support a maximum of 5 Mailboxes, one for each of the supported co-processors situated on the platform. Each Mailbox is divided up into 4 instances which consist of 32 channels. Messages are passed between the application and co-processors using shared memory areas. It is the Client's responsibility to manage these areas. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mailbox/Kconfig | 7 + drivers/mailbox/Makefile | 2 + drivers/mailbox/mailbox-sti.c | 562 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 571 insertions(+) create mode 100644 drivers/mailbox/mailbox-sti.c