Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Thu, Jun 06, 2019 at 04:40:45PM +0100, Sudeep Holla wrote: > On Thu, Jun 06, 2019 at 10:20:40AM -0500, Jassi Brar wrote: > > On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla wrote: > > > > > > > > > BTW, this is not going to be the end of SCMI troubles (I believe > > > > that's what his client is). SCMI will eventually have to be broken up > > > > in layers (protocol and transport) for many legit platforms to use it. > > > > That is mbox_send_message() will have to be replaced by, say, > > > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the > > > > platforms have to have shmem and each mailbox controller driver (that > > > > could ever be used under scmi) will have to implement "doorbell > > > > emulation" mode. That is the reason I am not letting the way paved for > > > > such emulations. > > > > > > > > > > While I don't dislike or disagree with separate transport in SCMI which > > > I have invested time and realised that I will duplicate mailbox framework > > > at the end. > > > > > Can you please share the code? Or is it no more available? > > > > > So I am against it only because of duplication and extra > > > layer of indirection which has performance impact(we have this seen in > > > sched governor for DVFS). > > > > > I don't see why the overhead should increase noticeably. > > > > Simple, if 2 protocols share the same channel, then the requests are > serialised. E.g. if bits 0 and 1 are allocated for protocol#1 > and bits 2 and 3 for protocol#2 and protocol#1 has higher latency > requirements like sched-governor DVFS and there are 3-4 pending requests > on protocol#2, then the incoming request for protocol#1 is blocked. > Any idea on addressing the above with abstraction layer above the driver ? And the bit allotment without the virtual channel representation in DT. These 2 are main issues that needs to be resolved. -- Regards, Sudeep
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Thu, Jun 06, 2019 at 10:20:40AM -0500, Jassi Brar wrote: > On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla wrote: > > > > > > BTW, this is not going to be the end of SCMI troubles (I believe > > > that's what his client is). SCMI will eventually have to be broken up > > > in layers (protocol and transport) for many legit platforms to use it. > > > That is mbox_send_message() will have to be replaced by, say, > > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the > > > platforms have to have shmem and each mailbox controller driver (that > > > could ever be used under scmi) will have to implement "doorbell > > > emulation" mode. That is the reason I am not letting the way paved for > > > such emulations. > > > > > > > While I don't dislike or disagree with separate transport in SCMI which > > I have invested time and realised that I will duplicate mailbox framework > > at the end. > > > Can you please share the code? Or is it no more available? > > > So I am against it only because of duplication and extra > > layer of indirection which has performance impact(we have this seen in > > sched governor for DVFS). > > > I don't see why the overhead should increase noticeably. > Simple, if 2 protocols share the same channel, then the requests are serialised. E.g. if bits 0 and 1 are allocated for protocol#1 and bits 2 and 3 for protocol#2 and protocol#1 has higher latency requirements like sched-governor DVFS and there are 3-4 pending requests on protocol#2, then the incoming request for protocol#1 is blocked. > > So idea wise, it's good and I don't disagree > > with practically seen performance impact. Hence I thought it's sane to > > do something I am proposing. > > > Please suggest how is SCMI supposed to work on ~15 controllers > upstream (except tegra-hsp) ? > Do you mean we have to implement platform layer to make it work ? That's not necessary IMO. > > It also avoids coming up with virtual DT > > nodes for this layer of abstract which I am completely against. > > > I don't see why virtual DT nodes would be needed for platform layer. So how will 2 or more different users of the same mailbox identify the bits allocated for them ? -- Regards, Sudeep
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Thu, Jun 6, 2019 at 7:51 AM Sudeep Holla wrote: > > > BTW, this is not going to be the end of SCMI troubles (I believe > > that's what his client is). SCMI will eventually have to be broken up > > in layers (protocol and transport) for many legit platforms to use it. > > That is mbox_send_message() will have to be replaced by, say, > > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the > > platforms have to have shmem and each mailbox controller driver (that > > could ever be used under scmi) will have to implement "doorbell > > emulation" mode. That is the reason I am not letting the way paved for > > such emulations. > > > > While I don't dislike or disagree with separate transport in SCMI which > I have invested time and realised that I will duplicate mailbox framework > at the end. > Can you please share the code? Or is it no more available? > So I am against it only because of duplication and extra > layer of indirection which has performance impact(we have this seen in > sched governor for DVFS). > I don't see why the overhead should increase noticeably. > So idea wise, it's good and I don't disagree > with practically seen performance impact. Hence I thought it's sane to > do something I am proposing. > Please suggest how is SCMI supposed to work on ~15 controllers upstream (except tegra-hsp) ? > It also avoids coming up with virtual DT > nodes for this layer of abstract which I am completely against. > I don't see why virtual DT nodes would be needed for platform layer.
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Wed, Jun 05, 2019 at 07:51:12PM -0500, Jassi Brar wrote: > On Wed, Jun 5, 2019 at 2:46 PM Mark Brown wrote: > > > > On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote: > > > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote: > > > > > > > > > It feels like the issues with sharing access to the hardware and with > > > > the > > > > API for talking to doorbell hardware are getting tied together and > > > > confusing things. But like I say I might be missing something here. > > > > ... > > > > > So what I am trying to convey here is MHU controller hardware can be > > > used choosing one of the different transport protocols available and > > > that's platform choice based on the use-case. > > > > > The driver in the kernel should identify the same from the firmware/DT > > > and configure it appropriately. > > > > > It may get inefficient and sometime impossible to address all use-case > > > if we stick to one transport protocol in the driver and try to build > > > an abstraction on top to use in different transport mode. > > > > Right, what I was trying to get at was that it feels like the discussion > > is getting wrapped up in the specifics of the MHU rather than > > representing this sort of controller with multiple modes in the > > framework. > > > Usually when a controller could be used in more than one way, we > implement the more generic usecase. And that's what was done for MHU. That's debatable and we have done that so extensively so far. So what I am saying is to implement different modes not just one so that as many use-case are addressed. > Implementing doorbell scheme would have disallowed mhu platforms that > don't have any shmem between the endpoints. Now such platforms could > use 32bits registers to pass/get data. Meanwhile doorbells could be > emulated in client code. > Also, next version of MHU has many (100?) such 32bit registers per > interrupt. Clearly those are not meant to be seen as 3200 doorbells, > but as message passing windows. (or maybe that is an almost different > controller because of the differences) > I disagree. It's configurable and vendors can just choose 2 instead of 100s as you mentioned based on the use-case and needs. So we will still need the same there. > BTW, this is not going to be the end of SCMI troubles (I believe > that's what his client is). SCMI will eventually have to be broken up > in layers (protocol and transport) for many legit platforms to use it. > That is mbox_send_message() will have to be replaced by, say, > platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the > platforms have to have shmem and each mailbox controller driver (that > could ever be used under scmi) will have to implement "doorbell > emulation" mode. That is the reason I am not letting the way paved for > such emulations. > While I don't dislike or disagree with separate transport in SCMI which I have invested time and realised that I will duplicate mailbox framework at the end. So I am against it only because of duplication and extra layer of indirection which has performance impact(we have this seen in sched governor for DVFS). So idea wise, it's good and I don't disagree with practically seen performance impact. Hence I thought it's sane to do something I am proposing. It also avoids coming up with virtual DT nodes for this layer of abstract which I am completely against. -- Regards, Sudeep
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Wed, Jun 5, 2019 at 2:46 PM Mark Brown wrote: > > On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote: > > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote: > > > > > > It feels like the issues with sharing access to the hardware and with the > > > API for talking to doorbell hardware are getting tied together and > > > confusing things. But like I say I might be missing something here. > > ... > > > So what I am trying to convey here is MHU controller hardware can be > > used choosing one of the different transport protocols available and > > that's platform choice based on the use-case. > > > The driver in the kernel should identify the same from the firmware/DT > > and configure it appropriately. > > > It may get inefficient and sometime impossible to address all use-case > > if we stick to one transport protocol in the driver and try to build > > an abstraction on top to use in different transport mode. > > Right, what I was trying to get at was that it feels like the discussion > is getting wrapped up in the specifics of the MHU rather than > representing this sort of controller with multiple modes in the > framework. > Usually when a controller could be used in more than one way, we implement the more generic usecase. And that's what was done for MHU. Implementing doorbell scheme would have disallowed mhu platforms that don't have any shmem between the endpoints. Now such platforms could use 32bits registers to pass/get data. Meanwhile doorbells could be emulated in client code. Also, next version of MHU has many (100?) such 32bit registers per interrupt. Clearly those are not meant to be seen as 3200 doorbells, but as message passing windows. (or maybe that is an almost different controller because of the differences) BTW, this is not going to be the end of SCMI troubles (I believe that's what his client is). SCMI will eventually have to be broken up in layers (protocol and transport) for many legit platforms to use it. That is mbox_send_message() will have to be replaced by, say, platform_mbox_send() in drivers/firmware/arm_scmi/driver.c OR the platforms have to have shmem and each mailbox controller driver (that could ever be used under scmi) will have to implement "doorbell emulation" mode. That is the reason I am not letting the way paved for such emulations. Thanks
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Tue, Jun 04, 2019 at 10:44:24AM +0100, Sudeep Holla wrote: > On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote: > > > It feels like the issues with sharing access to the hardware and with the > > API for talking to doorbell hardware are getting tied together and > > confusing things. But like I say I might be missing something here. ... > So what I am trying to convey here is MHU controller hardware can be > used choosing one of the different transport protocols available and > that's platform choice based on the use-case. > The driver in the kernel should identify the same from the firmware/DT > and configure it appropriately. > It may get inefficient and sometime impossible to address all use-case > if we stick to one transport protocol in the driver and try to build > an abstraction on top to use in different transport mode. Right, what I was trying to get at was that it feels like the discussion is getting wrapped up in the specifics of the MHU rather than representing this sort of controller with multiple modes in the framework. It's important for establishing the use case but ultimately a separate issue. signature.asc Description: PGP signature
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Mon, Jun 03, 2019 at 08:39:46PM +0100, Mark Brown wrote: > On Fri, May 31, 2019 at 05:53:26PM +0100, Sudeep Holla wrote: > > On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote: > > > On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote: > > > > > This is my another attempt to extend mailbox framework to support > > > > doorbell mode mailbox hardware. It also adds doorbell support to ARM > > > > MHU driver. > > > > Nothing has really changed since the last time we discussed many months > > > ago. > > > MHU remains same, and so are my points. > > > Yes, I understand your concern. > > > But as mentioned in the cover letter I did try the suggestions and have > > detailed reasoning why that's still an issue. In short I ended up > > re-inventing mailbox framework with all the queuing and similar APIs > > for this. Worse, we can't even add an extra node for that in DT to > > describe that. It can't be simple shim as we need to allow multiple > > users to access one physical channel at a time. We have use case > > where we can this for CPU DVFS fast switching in scheduler context. > > Forgive me if I'm missing something here (this is partly based on > conversations from months ago so I may be misremembering things) but is > the issue here specifically the doorbell mode or is it the need to have > partly software defined mailboxes implemented using this hardware? I can say it's partially both. 1. The hardware is designed keeping in mind multiple transport protocols: doorbell mode, single word and multiple work(only in newer versions) Using that hardware capability provides access to multiple channels to the software. 2. I can also view this as software defined mailboxes if we go by definition that each channel should have associated dedicated interrupt as Jassi mentions. The main idea is that each bit in these 32-bit registers can be written atomically without the need of read-modify-write enabling software to implement multiple channels in lock-less way. > My understanding is that the hardware is more a component that's intended > to allow potentially multiple more complex mailboxes to be tied to a > single hardware block than a complete mailbox in and of itself. Correct. > It feels like the issues with sharing access to the hardware and with the > API for talking to doorbell hardware are getting tied together and > confusing things. But like I say I might be missing something here. As I tried to simply in my cover letter, I will try to explain in simpler terms. 1. This version of hardware has 3 blocks(one for secure and 2 non-secure) Each block has 3 sets of 32-bit registers(SET, CLEAR and STATUS) SET and CLEAR are write only and STATUS is read-only. Each block has a dedicated interrupt line. 2. The hardware was designed to cater 2 transport protocols. A single word transfer(non-zero) or each bit in doorbell mode. 3. The next version extends with each block having larger than 32-bit window(up to 124 words) allowing it to used it for multiple word as transport protocol. Mainly for some IoT usecase. So what I am trying to convey here is MHU controller hardware can be used choosing one of the different transport protocols available and that's platform choice based on the use-case. The driver in the kernel should identify the same from the firmware/DT and configure it appropriately. It may get inefficient and sometime impossible to address all use-case if we stick to one transport protocol in the driver and try to build an abstraction on top to use in different transport mode. Hope this clarify things little bit more. -- Regards, Sudeep
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Fri, May 31, 2019 at 05:53:26PM +0100, Sudeep Holla wrote: > On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote: > > On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote: > > > This is my another attempt to extend mailbox framework to support > > > doorbell mode mailbox hardware. It also adds doorbell support to ARM > > > MHU driver. > > Nothing has really changed since the last time we discussed many months ago. > > MHU remains same, and so are my points. > Yes, I understand your concern. > But as mentioned in the cover letter I did try the suggestions and have > detailed reasoning why that's still an issue. In short I ended up > re-inventing mailbox framework with all the queuing and similar APIs > for this. Worse, we can't even add an extra node for that in DT to > describe that. It can't be simple shim as we need to allow multiple > users to access one physical channel at a time. We have use case > where we can this for CPU DVFS fast switching in scheduler context. Forgive me if I'm missing something here (this is partly based on conversations from months ago so I may be misremembering things) but is the issue here specifically the doorbell mode or is it the need to have partly software defined mailboxes implemented using this hardware? My understanding is that the hardware is more a component that's intended to allow potentially multiple more complex mailboxes to be tied to a single hardware block than a complete mailbox in and of itself. It feels like the issues with sharing access to the hardware and with the API for talking to doorbell hardware are getting tied together and confusing things. But like I say I might be missing something here. signature.asc Description: PGP signature
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
On Fri, May 31, 2019 at 11:21:08AM -0500, Jassi Brar wrote: > Hi Sudeep, > > On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote: > > > > This is my another attempt to extend mailbox framework to support > > doorbell mode mailbox hardware. It also adds doorbell support to ARM > > MHU driver. > > > Nothing has really changed since the last time we discussed many months ago. > MHU remains same, and so are my points. > Yes, I understand your concern. But as mentioned in the cover letter I did try the suggestions and have detailed reasoning why that's still an issue. In short I ended up re-inventing mailbox framework with all the queuing and similar APIs for this. Worse, we can't even add an extra node for that in DT to describe that. It can't be simple shim as we need to allow multiple users to access one physical channel at a time. We have use case where we can this for CPU DVFS fast switching in scheduler context. > > > > Brief hardware description about MHU > > > > > > For each of the interrupt signals, the MHU drives the signal using a 32-bit > > register, with all 32 bits logically ORed together. The MHU provides a set > > of > > registers to enable software to SET, CLEAR, and check the STATUS of each of > > the bits of this register independently. The use of 32 bits for each > > interrupt > > line enables software to provide more information about the source of the > > interrupt. > > > "32 bits for each interrupt line" => 32 virtual channels or 32bit > data over one physical channel. And that is how the driver is > currently written. > Yes, I understand. > > For example, each bit of the register can be associated with a type > > of event that can contribute to raising the interrupt. > > > Sure there can be a usecase where each bit is associated with an event > (virtual channel). As you said, this is just one example of how MHU > can be used. There are other ways MHU is used already. > Agreed and I am not saying that's incorrect or asking to remove support for that. Future versions of MHU are making it more complex and the specification classify the 3 modes of transport protocol in which the new controller can be configured and used. The choice is left to platform based on use case to get best/optimal results. That's the reason I am trying to convince you that we need to support all those configurations/transport protocols in the Linux mailbox controller driver. As you mention one use-case of MHU, the word transfer with 2^32 - 1 options is optimal for IoT use-case where as doorbell mode is optimal for heavy payloads. The newer versions of MHU are designed keeping both configurations in mind and the same is suggested by h/w teams to various vendors. Arnd has similar suggestions(something like patch 1) to deal with such configurations/transport protocols. Please note I am referring to different transport protocols and not message protocols(SCPI/SCMI fall under this category) > Patch-2/6 looks good though. I will pick that up. > Thanks. -- Regards, Sudeep
Re: [PATCH 0/6] mailbox: arm_mhu: add support to use in doorbell mode
Hi Sudeep, On Fri, May 31, 2019 at 9:33 AM Sudeep Holla wrote: > > This is my another attempt to extend mailbox framework to support > doorbell mode mailbox hardware. It also adds doorbell support to ARM > MHU driver. > Nothing has really changed since the last time we discussed many months ago. MHU remains same, and so are my points. > > Brief hardware description about MHU > > > For each of the interrupt signals, the MHU drives the signal using a 32-bit > register, with all 32 bits logically ORed together. The MHU provides a set of > registers to enable software to SET, CLEAR, and check the STATUS of each of > the bits of this register independently. The use of 32 bits for each interrupt > line enables software to provide more information about the source of the > interrupt. > "32 bits for each interrupt line" => 32 virtual channels or 32bit data over one physical channel. And that is how the driver is currently written. > For example, each bit of the register can be associated with a type > of event that can contribute to raising the interrupt. > Sure there can be a usecase where each bit is associated with an event (virtual channel). As you said, this is just one example of how MHU can be used. There are other ways MHU is used already. Patch-2/6 looks good though. I will pick that up. Thanks.
