| Message ID | 1583300977-2327-1-git-send-email-peng.fan@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | mailbox/firmware: imx: support SCU channel type | expand |
On 2020-03-04 7:55 AM, Peng Fan wrote: > From: Peng Fan <peng.fan@nxp.com> > > V6: > Add Oleksij's R-b tag > Patch 3/4, per https://www.kernel.org/doc/Documentation/printk-formats.txt > should use %zu for printk sizeof > > V5: > Move imx_mu_dcfg below imx_mu_priv > Add init hooks to imx_mu_dcfg > drop __packed __aligned > Add more debug msg > code style cleanup > > V4: > Drop IMX_MU_TYPE_[GENERIC, SCU] > Pack MU chans init to separate function > Add separate function for SCU chans init and xlate > Add santity check to msg hdr.size > Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] > > V3: > Rebase to Shawn's for-next > Include fsl,imx8-mu-scu compatible > Per Oleksij's comments, introduce generic tx/rx and added scu mu type > Check fsl,imx8-mu-scu in firmware driver for fast_ipc > > V2: > Drop patch 1/3 which added fsl,scu property > Force to use scu channel type when machine has node compatible "fsl,imx-scu" > Force imx-scu to use fast_ipc > > I not found a generic method to make SCFW message generic enough, SCFW > message is not fixed length including TX and RX. And it use TR0/RR0 > interrupt. > > V1: > Sorry to bind the mailbox/firmware patch together. This is make it > to understand what changed to support using 1 TX and 1 RX channel > for SCFW message. > > Per i.MX8QXP Reference mannual, there are several message using > examples. One of them is: > Passing short messages: Transmit register(s) can be used to pass > short messages from one to four words in length. For example, > when a four-word message is desired, only one of the registers > needs to have its corresponding interrupt enable bit set at the > receiver side. > > This patchset is to using this for SCFW message to replace four TX > and four RX method. Tested-by: Leonard Crestez <leonard.crestez@nxp.com> My stress tests pass on imx8qxp with this patcheset, however performance is not greatly improved. My guess is that this happens because of too many interrupts. Is there really a reason to enable TIE? Spinning on TE bits without any interrupts should be just plain faster. > > Peng Fan (4): > dt-bindings: mailbox: imx-mu: add SCU MU support > mailbox: imx: restructure code to make easy for new MU > mailbox: imx: add SCU MU support > firmware: imx-scu: Support one TX and one RX > > .../devicetree/bindings/mailbox/fsl,mu.txt | 2 + > drivers/firmware/imx/imx-scu.c | 54 ++++- > drivers/mailbox/imx-mailbox.c | 267 +++++++++++++++++---- > 3 files changed, 260 insertions(+), 63 deletions(-) > > > base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17 >
Hi Leonard, > Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel > type > > On 2020-03-04 7:55 AM, Peng Fan wrote: > > From: Peng Fan <peng.fan@nxp.com> > > > > V6: > > Add Oleksij's R-b tag > > Patch 3/4, per > https://www.kernel.org/doc/Documentation/printk-formats.txt > > should use %zu for printk sizeof > > > > V5: > > Move imx_mu_dcfg below imx_mu_priv > > Add init hooks to imx_mu_dcfg > > drop __packed __aligned > > Add more debug msg > > code style cleanup > > > > V4: > > Drop IMX_MU_TYPE_[GENERIC, SCU] > > Pack MU chans init to separate function > > Add separate function for SCU chans init and xlate > > Add santity check to msg hdr.size > > Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] > > > > V3: > > Rebase to Shawn's for-next > > Include fsl,imx8-mu-scu compatible > > Per Oleksij's comments, introduce generic tx/rx and added scu mu type > > Check fsl,imx8-mu-scu in firmware driver for fast_ipc > > > > V2: > > Drop patch 1/3 which added fsl,scu property > > Force to use scu channel type when machine has node compatible > "fsl,imx-scu" > > Force imx-scu to use fast_ipc > > > > I not found a generic method to make SCFW message generic enough, > SCFW > > message is not fixed length including TX and RX. And it use TR0/RR0 > > interrupt. > > > > V1: > > Sorry to bind the mailbox/firmware patch together. This is make it to > > understand what changed to support using 1 TX and 1 RX channel for > > SCFW message. > > > > Per i.MX8QXP Reference mannual, there are several message using > > examples. One of them is: > > Passing short messages: Transmit register(s) can be used to pass short > > messages from one to four words in length. For example, when a > > four-word message is desired, only one of the registers needs to have > > its corresponding interrupt enable bit set at the receiver side. > > > > This patchset is to using this for SCFW message to replace four TX and > > four RX method. > > Tested-by: Leonard Crestez <leonard.crestez@nxp.com> > Thanks for the test. > My stress tests pass on imx8qxp with this patcheset, however performance is > not greatly improved. My guess is that this happens because of too many > interrupts. Might be. Could you share your testcase? > > Is there really a reason to enable TIE? Spinning on TE bits without any > interrupts should be just plain faster. I could try to disable TIE and give a try. If performance improves lot, I could change to non TX interrupt. Oleksij, do you agree? Thanks, Peng. > > > > > Peng Fan (4): > > dt-bindings: mailbox: imx-mu: add SCU MU support > > mailbox: imx: restructure code to make easy for new MU > > mailbox: imx: add SCU MU support > > firmware: imx-scu: Support one TX and one RX > > > > .../devicetree/bindings/mailbox/fsl,mu.txt | 2 + > > drivers/firmware/imx/imx-scu.c | 54 ++++- > > drivers/mailbox/imx-mailbox.c | 267 > +++++++++++++++++---- > > 3 files changed, 260 insertions(+), 63 deletions(-) > > > > > > base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17 > >
> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel > type > > Hi Leonard, > > > Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel > > type > > > > On 2020-03-04 7:55 AM, Peng Fan wrote: > > > From: Peng Fan <peng.fan@nxp.com> > > > > > > V6: > > > Add Oleksij's R-b tag > > > Patch 3/4, per > > https://www.kernel.org/doc/Documentation/printk-formats.txt > > > should use %zu for printk sizeof > > > > > > V5: > > > Move imx_mu_dcfg below imx_mu_priv > > > Add init hooks to imx_mu_dcfg > > > drop __packed __aligned > > > Add more debug msg > > > code style cleanup > > > > > > V4: > > > Drop IMX_MU_TYPE_[GENERIC, SCU] > > > Pack MU chans init to separate function > > > Add separate function for SCU chans init and xlate > > > Add santity check to msg hdr.size > > > Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] > > > > > > V3: > > > Rebase to Shawn's for-next > > > Include fsl,imx8-mu-scu compatible > > > Per Oleksij's comments, introduce generic tx/rx and added scu mu type > > > Check fsl,imx8-mu-scu in firmware driver for fast_ipc > > > > > > V2: > > > Drop patch 1/3 which added fsl,scu property > > > Force to use scu channel type when machine has node compatible > > "fsl,imx-scu" > > > Force imx-scu to use fast_ipc > > > > > > I not found a generic method to make SCFW message generic enough, > > SCFW > > > message is not fixed length including TX and RX. And it use TR0/RR0 > > > interrupt. > > > > > > V1: > > > Sorry to bind the mailbox/firmware patch together. This is make it > > > to understand what changed to support using 1 TX and 1 RX channel > > > for SCFW message. > > > > > > Per i.MX8QXP Reference mannual, there are several message using > > > examples. One of them is: > > > Passing short messages: Transmit register(s) can be used to pass > > > short messages from one to four words in length. For example, when a > > > four-word message is desired, only one of the registers needs to > > > have its corresponding interrupt enable bit set at the receiver side. > > > > > > This patchset is to using this for SCFW message to replace four TX > > > and four RX method. > > > > Tested-by: Leonard Crestez <leonard.crestez@nxp.com> > > > > Thanks for the test. > > > My stress tests pass on imx8qxp with this patcheset, however > > performance is not greatly improved. My guess is that this happens > > because of too many interrupts. > > Might be. Could you share your testcase? > > > > > Is there really a reason to enable TIE? Spinning on TE bits without > > any interrupts should be just plain faster. > > I could try to disable TIE and give a try. If performance improves lot, I could > change to non TX interrupt. After rethinking about this, we need TX interrupt, otherwise we have to use TX_POLL which is slower or let the client kick the TX state machine. Compared with original method, this already reduces to use 1 TX and 1 RX interrupt. This already good for system. Thanks, Peng. > > Oleksij, do you agree? > > Thanks, > Peng. > > > > > > > > > Peng Fan (4): > > > dt-bindings: mailbox: imx-mu: add SCU MU support > > > mailbox: imx: restructure code to make easy for new MU > > > mailbox: imx: add SCU MU support > > > firmware: imx-scu: Support one TX and one RX > > > > > > .../devicetree/bindings/mailbox/fsl,mu.txt | 2 + > > > drivers/firmware/imx/imx-scu.c | 54 ++++- > > > drivers/mailbox/imx-mailbox.c | 267 > > +++++++++++++++++---- > > > 3 files changed, 260 insertions(+), 63 deletions(-) > > > > > > > > > base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17 > > >
On 2020-03-13 9:38 AM, Peng Fan wrote: >> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel >> type >> >> Hi Leonard, >> >>> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel >>> type >>> >>> On 2020-03-04 7:55 AM, Peng Fan wrote: >>>> From: Peng Fan <peng.fan@nxp.com> >>>> >>>> V6: >>>> Add Oleksij's R-b tag >>>> Patch 3/4, per >>> https://www.kernel.org/doc/Documentation/printk-formats.txt >>>> should use %zu for printk sizeof >>>> >>>> V5: >>>> Move imx_mu_dcfg below imx_mu_priv >>>> Add init hooks to imx_mu_dcfg >>>> drop __packed __aligned >>>> Add more debug msg >>>> code style cleanup >>>> >>>> V4: >>>> Drop IMX_MU_TYPE_[GENERIC, SCU] >>>> Pack MU chans init to separate function >>>> Add separate function for SCU chans init and xlate >>>> Add santity check to msg hdr.size >>>> Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] >>>> >>>> V3: >>>> Rebase to Shawn's for-next >>>> Include fsl,imx8-mu-scu compatible >>>> Per Oleksij's comments, introduce generic tx/rx and added scu mu type >>>> Check fsl,imx8-mu-scu in firmware driver for fast_ipc >>>> >>>> V2: >>>> Drop patch 1/3 which added fsl,scu property >>>> Force to use scu channel type when machine has node compatible >>> "fsl,imx-scu" >>>> Force imx-scu to use fast_ipc >>>> >>>> I not found a generic method to make SCFW message generic enough, >>> SCFW >>>> message is not fixed length including TX and RX. And it use TR0/RR0 >>>> interrupt. >>>> >>>> V1: >>>> Sorry to bind the mailbox/firmware patch together. This is make it >>>> to understand what changed to support using 1 TX and 1 RX channel >>>> for SCFW message. >>>> >>>> Per i.MX8QXP Reference mannual, there are several message using >>>> examples. One of them is: >>>> Passing short messages: Transmit register(s) can be used to pass >>>> short messages from one to four words in length. For example, when a >>>> four-word message is desired, only one of the registers needs to >>>> have its corresponding interrupt enable bit set at the receiver side. >>>> >>>> This patchset is to using this for SCFW message to replace four TX >>>> and four RX method. >>> >>> Tested-by: Leonard Crestez <leonard.crestez@nxp.com> >>> >> >> Thanks for the test. >> >>> My stress tests pass on imx8qxp with this patcheset, however >>> performance is not greatly improved. My guess is that this happens >>> because of too many interrupts. >> >> Might be. Could you share your testcase? https://github.com/cdleonard/imx-scu-test >>> Is there really a reason to enable TIE? Spinning on TE bits without >>> any interrupts should be just plain faster. >> >> I could try to disable TIE and give a try. If performance improves lot, I could >> change to non TX interrupt. > > After rethinking about this, we need TX interrupt, otherwise we have to > use TX_POLL which is slower or let the client kick the TX state machine. > > Compared with original method, this already reduces to use 1 TX and 1 RX > interrupt. This already good for system. Sorry, I missed that fact that your patches don't include the required DTS changes. Indeed that is only one TX and one RX irq per call now. Running my test now results in RX timeout :( ----- On an unrelated note: are you sure it is appropriate to change the compat string here? Another way to implement direct SCU communication would be as another channel type, IMX_MU_TYPE_SCUTX. It also strange that you're adding a bool fast_ipc in imx-scu, do we really want to support the old path? If SCU protocol was implemented as a channel type then maybe we could sidestep mbox_request_channel_by_name, parse mboxes manually and always request MU_TYPE_SCUTX. -- Regards, Leonard
> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU channel > type > > On 2020-03-13 9:38 AM, Peng Fan wrote: > >> Subject: RE: [PATCH V6 0/4] mailbox/firmware: imx: support SCU > >> channel type > >> > >> Hi Leonard, > >> > >>> Subject: Re: [PATCH V6 0/4] mailbox/firmware: imx: support SCU > >>> channel type > >>> > >>> On 2020-03-04 7:55 AM, Peng Fan wrote: > >>>> From: Peng Fan <peng.fan@nxp.com> > >>>> > >>>> V6: > >>>> Add Oleksij's R-b tag > >>>> Patch 3/4, per > >>> https://www.kernel.org/doc/Documentation/printk-formats.txt > >>>> should use %zu for printk sizeof > >>>> > >>>> V5: > >>>> Move imx_mu_dcfg below imx_mu_priv > >>>> Add init hooks to imx_mu_dcfg > >>>> drop __packed __aligned > >>>> Add more debug msg > >>>> code style cleanup > >>>> > >>>> V4: > >>>> Drop IMX_MU_TYPE_[GENERIC, SCU] > >>>> Pack MU chans init to separate function > >>>> Add separate function for SCU chans init and xlate > >>>> Add santity check to msg hdr.size > >>>> Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] > >>>> > >>>> V3: > >>>> Rebase to Shawn's for-next > >>>> Include fsl,imx8-mu-scu compatible > >>>> Per Oleksij's comments, introduce generic tx/rx and added scu mu > type > >>>> Check fsl,imx8-mu-scu in firmware driver for fast_ipc > >>>> > >>>> V2: > >>>> Drop patch 1/3 which added fsl,scu property > >>>> Force to use scu channel type when machine has node compatible > >>> "fsl,imx-scu" > >>>> Force imx-scu to use fast_ipc > >>>> > >>>> I not found a generic method to make SCFW message generic > >>>> enough, > >>> SCFW > >>>> message is not fixed length including TX and RX. And it use TR0/RR0 > >>>> interrupt. > >>>> > >>>> V1: > >>>> Sorry to bind the mailbox/firmware patch together. This is make it > >>>> to understand what changed to support using 1 TX and 1 RX channel > >>>> for SCFW message. > >>>> > >>>> Per i.MX8QXP Reference mannual, there are several message using > >>>> examples. One of them is: > >>>> Passing short messages: Transmit register(s) can be used to pass > >>>> short messages from one to four words in length. For example, when > >>>> a four-word message is desired, only one of the registers needs to > >>>> have its corresponding interrupt enable bit set at the receiver side. > >>>> > >>>> This patchset is to using this for SCFW message to replace four TX > >>>> and four RX method. > >>> > >>> Tested-by: Leonard Crestez <leonard.crestez@nxp.com> > >>> > >> > >> Thanks for the test. > >> > >>> My stress tests pass on imx8qxp with this patcheset, however > >>> performance is not greatly improved. My guess is that this happens > >>> because of too many interrupts. > >> > >> Might be. Could you share your testcase? > > https://github.com/cdleonard/imx-scu-test > > >>> Is there really a reason to enable TIE? Spinning on TE bits without > >>> any interrupts should be just plain faster. > >> > >> I could try to disable TIE and give a try. If performance improves > >> lot, I could change to non TX interrupt. > > > > After rethinking about this, we need TX interrupt, otherwise we have > > to use TX_POLL which is slower or let the client kick the TX state machine. > > > > Compared with original method, this already reduces to use 1 TX and 1 > > RX interrupt. This already good for system. > > Sorry, I missed that fact that your patches don't include the required DTS > changes. Indeed that is only one TX and one RX irq per call now. > > Running my test now results in RX timeout :( Might be long that 4 word messages, because not check TX empty and RX full in my patch. > > ----- > > On an unrelated note: are you sure it is appropriate to change the compat > string here? Another way to implement direct SCU communication would be > as another channel type, IMX_MU_TYPE_SCUTX. No. This will introduce more complexity. Per Oleksij's suggestion, I added the compatible string. > > It also strange that you're adding a bool fast_ipc in imx-scu, do we really want > to support the old path? It is to avoid break DT backward compatibility. Thanks, Peng. > > If SCU protocol was implemented as a channel type then maybe we could > sidestep mbox_request_channel_by_name, parse mboxes manually and > always request MU_TYPE_SCUTX. > > -- > Regards, > Leonard
From: Peng Fan <peng.fan@nxp.com> V6: Add Oleksij's R-b tag Patch 3/4, per https://www.kernel.org/doc/Documentation/printk-formats.txt should use %zu for printk sizeof V5: Move imx_mu_dcfg below imx_mu_priv Add init hooks to imx_mu_dcfg drop __packed __aligned Add more debug msg code style cleanup V4: Drop IMX_MU_TYPE_[GENERIC, SCU] Pack MU chans init to separate function Add separate function for SCU chans init and xlate Add santity check to msg hdr.size Limit SCU MU chans to 6, TX0/RX0/RXDB[0-3] V3: Rebase to Shawn's for-next Include fsl,imx8-mu-scu compatible Per Oleksij's comments, introduce generic tx/rx and added scu mu type Check fsl,imx8-mu-scu in firmware driver for fast_ipc V2: Drop patch 1/3 which added fsl,scu property Force to use scu channel type when machine has node compatible "fsl,imx-scu" Force imx-scu to use fast_ipc I not found a generic method to make SCFW message generic enough, SCFW message is not fixed length including TX and RX. And it use TR0/RR0 interrupt. V1: Sorry to bind the mailbox/firmware patch together. This is make it to understand what changed to support using 1 TX and 1 RX channel for SCFW message. Per i.MX8QXP Reference mannual, there are several message using examples. One of them is: Passing short messages: Transmit register(s) can be used to pass short messages from one to four words in length. For example, when a four-word message is desired, only one of the registers needs to have its corresponding interrupt enable bit set at the receiver side. This patchset is to using this for SCFW message to replace four TX and four RX method. Peng Fan (4): dt-bindings: mailbox: imx-mu: add SCU MU support mailbox: imx: restructure code to make easy for new MU mailbox: imx: add SCU MU support firmware: imx-scu: Support one TX and one RX .../devicetree/bindings/mailbox/fsl,mu.txt | 2 + drivers/firmware/imx/imx-scu.c | 54 ++++- drivers/mailbox/imx-mailbox.c | 267 +++++++++++++++++---- 3 files changed, 260 insertions(+), 63 deletions(-) base-commit: 770fbb32d34e5d6298cc2be590c9d2fd6069aa17