| Message ID | 1493733353-25812-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| Headers | show |
| Series | mailbox: arm_mhu: add support for subchannels | expand |
Hi Sudeep, On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Jassi, > > This series adds subchannel support to ARM MHU mailbox controller > driver. Since SCPI never used second slot, we were able to use the > existing driver as is. However, that's changing soon and the new > SCMI protocol under development needs subchannel support. If you > recall when you initially added this driver, I was pushing for some > of these changes like threaded irq. This patch series adds support > for the subchannels on ARM MHU controllers. > There are really no "sub-channels" in the ARM MHU controller. There are exactly three channels that work on 32bit registers. The SET/CLEAR registers are there to prevent races between local and remote firmware, and not to emulate virtual channels operating on single bits. Please remember all 32-bits work together to generate one signal. You arrived at the "sub-channel" idea only because your protocol uses 1-bit messages. This patchset seems rather regressive - reduce from 2^32 possible signals to mere 32, by bloating the MHU driver. If it's difficult to see how your protocol can be implemented over existing controller driver, please let me know. Thanks.
On 03/05/17 04:17, Jassi Brar wrote: > Hi Sudeep, > > On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Hi Jassi, >> >> This series adds subchannel support to ARM MHU mailbox controller >> driver. Since SCPI never used second slot, we were able to use the >> existing driver as is. However, that's changing soon and the new >> SCMI protocol under development needs subchannel support. If you >> recall when you initially added this driver, I was pushing for some >> of these changes like threaded irq. This patch series adds support >> for the subchannels on ARM MHU controllers. >> > There are really no "sub-channels" in the ARM MHU controller. There > are exactly three channels that work on 32bit registers. The SET/CLEAR > registers are there to prevent races between local and remote > firmware, and not to emulate virtual channels operating on single > bits. Please remember all 32-bits work together to generate one > signal. > If you check 3.4.4 Message Handling Unit (MHU) of Juno TRM [1], "..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. For example, each bit of the register can be associated with a type of event that can contribute to raising the interrupt." So yes, they generate one signal, but that doesn't mean anything. We have even PMU interrupts tied to single SPI on some SoC. Since the design of MHU clearly indicates that each bit can be used independently for different event, for all practical purpose, it can be treated as different channel. > You arrived at the "sub-channel" idea only because your protocol uses > 1-bit messages. May be. It now uses BIT 0 for one channel and BIT 1 for another on the same physical channel. How do you propose it support that then ? We have multiple protocols with the same remote, so this is just used as a doorbell bit and not carrier of any message. > This patchset seems rather regressive - reduce from > 2^32 possible signals to mere 32, by bloating the MHU driver. > I don't quite get this. There are only 3 signals as you mentioned above. Yes there are 2^32 possible values for the register, but how can that be used ? I don't think that was the design intention at-least from the above text in the specification. Also the we still continue to support one channel per physical channel. > If it's difficult to see how your protocol can be implemented over > existing controller driver, please let me know. > It was a workaround just because there was no other protocol so far. I just set bit 0 and send it as u32 to send_message. -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.ddi0515f/DDI0515F_juno_arm_development_platform_soc_trm.pdf
On Wed, May 3, 2017 at 2:51 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 03/05/17 04:17, Jassi Brar wrote: >> Hi Sudeep, >> >> On Tue, May 2, 2017 at 7:25 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >>> Hi Jassi, >>> >>> This series adds subchannel support to ARM MHU mailbox controller >>> driver. Since SCPI never used second slot, we were able to use the >>> existing driver as is. However, that's changing soon and the new >>> SCMI protocol under development needs subchannel support. If you >>> recall when you initially added this driver, I was pushing for some >>> of these changes like threaded irq. This patch series adds support >>> for the subchannels on ARM MHU controllers. >>> >> There are really no "sub-channels" in the ARM MHU controller. There >> are exactly three channels that work on 32bit registers. The SET/CLEAR >> registers are there to prevent races between local and remote >> firmware, and not to emulate virtual channels operating on single >> bits. Please remember all 32-bits work together to generate one >> signal. >> > > If you check 3.4.4 Message Handling Unit (MHU) of Juno TRM [1], > > "..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. For example, each bit of the register can be associated > with a type of event that can contribute to raising the interrupt." > > So yes, they generate one signal, but that doesn't mean anything. > That means a lot. That means a MHU signal/message is 32bits, not single bit. > We > have even PMU interrupts tied to single SPI on some SoC. Since the > design of MHU clearly indicates that each bit can be used independently > for different event, for all practical purpose, it can be treated as > different channel. > Please don't mess with controller driver to support your usecase, which is already well supported. >> You arrived at the "sub-channel" idea only because your protocol uses >> 1-bit messages. > > May be. It now uses BIT 0 for one channel and BIT 1 for another on the > same physical channel. How do you propose it support that then ? > There is no "sub-channel", but only physical channel. You are led to believe each bit represents one channel only because your protocol uses (1<<N) type signals. > We have > multiple protocols with the same remote, so this is just used as a > doorbell bit and not carrier of any message. > Doorbell is a single bit message in mailbox framework :) >> This patchset seems rather regressive - reduce from >> 2^32 possible signals to mere 32, by bloating the MHU driver. >> > > I don't quite get this. There are only 3 signals as you mentioned above. > Yes there are 2^32 possible values for the register, but how can that be > used ? > _Your_ protocol don't use more than 32 values, that doesn't mean other protocols don't either.
On 05/05/17 12:12, Jassi Brar wrote: > On Wed, May 3, 2017 at 2:51 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: [...] >> >> I don't quite get this. There are only 3 signals as you mentioned above. >> Yes there are 2^32 possible values for the register, but how can that be >> used ? >> > _Your_ protocol don't use more than 32 values, that doesn't mean other > protocols don't either. > Please read what I quoted from the spec. "..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. For example, each bit of the register can be associated with a type of event that can contribute to raising the interrupt." It is designed exactly for the above use-case. It was designed to fit PCC in ACPI which allows what I am doing with this series. It's very similar to many PMIC drivers we have. Though set of PMIC events are bundled into single register and interrupt doesn't mean they need to be support as event. E.g. look at mc13xxx, wm831x,...etc I am not trying to fit the changes to our use-case. I would counter argue that you did so when you push the driver. Since it's very clear for the spec that the individual bits can be accessed atomically, it was designed to be used as doorbell and not as a message carrier register. I am fine if it can be used in that way but don't disagree to support the use-case for which it was designed. -- Regards, Sudeep