From: Peng Fan <[email protected]>
This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC initial
value after power up is invalid, need set a valid value after related
power domain up.
This patchset also includes two patch[1,2] during my development to enable
the ICC feature for i.MX8MP.
I not include ddrc DVFS in this patchset, ths patchset is only to
support NoC value mode/priority/ext_control being set to a valid value
that suggested by i.MX Chip Design Team. The value is same as NXP
downstream one inside Arm Trusted Firmware:
https://source.codeaurora.org/external/imx/imx-atf/tree/plat/imx/imx8m/imx8mp/gpc.c?h=lf_v2.4#n97
A repo created here: https://github.com/MrVan/linux/tree/imx8mp-interconnect
Peng Fan (8):
dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
interconnect: add device managed bulk API
interconnect: imx: fix max_node_id
interconnect: imx: set src node
interconnect: imx: introduce imx_icc_provider
interconnect: imx: set of_node for interconnect provider
interconnect: imx: configure NoC mode/prioriry/ext_control
interconnect: imx: Add platform driver for imx8mp
.../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
drivers/interconnect/bulk.c | 34 +++
drivers/interconnect/imx/Kconfig | 4 +
drivers/interconnect/imx/Makefile | 2 +
drivers/interconnect/imx/imx.c | 68 +++--
drivers/interconnect/imx/imx.h | 25 +-
drivers/interconnect/imx/imx8mm.c | 2 +-
drivers/interconnect/imx/imx8mn.c | 2 +-
drivers/interconnect/imx/imx8mp.c | 232 ++++++++++++++++++
drivers/interconnect/imx/imx8mq.c | 2 +-
include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
include/linux/interconnect.h | 6 +
12 files changed, 424 insertions(+), 18 deletions(-)
create mode 100644 drivers/interconnect/imx/imx8mp.c
create mode 100644 include/dt-bindings/interconnect/fsl,imx8mp.h
--
2.25.1
All,
> Subject: [PATCH 0/8] interconnect: support i.MX8MP
I am going to send out V2 this week to address the comments until now.
But before that I would like to see if any one has any comments on the
design here.
Georgi, do you have comments on Patch 2 " interconnect: add device
managed bulk API"
Lucas, since you had comments when I first use syscon to configure NoC,
are you ok with the design to use interconnect in this patchset?
Thanks,
Peng.
>
> From: Peng Fan <[email protected]>
>
> This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC initial value
> after power up is invalid, need set a valid value after related power domain up.
>
> This patchset also includes two patch[1,2] during my development to enable the
> ICC feature for i.MX8MP.
>
> I not include ddrc DVFS in this patchset, ths patchset is only to support NoC
> value mode/priority/ext_control being set to a valid value that suggested by
> i.MX Chip Design Team. The value is same as NXP downstream one inside Arm
> Trusted Firmware:
> https://source.codeaurora.org/external/imx/imx-atf/tree/plat/imx/imx8m/imx
> 8mp/gpc.c?h=lf_v2.4#n97
>
> A repo created here:
> https://github.com/MrVan/linux/tree/imx8mp-interconnect
>
> Peng Fan (8):
> dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> interconnect: add device managed bulk API
> interconnect: imx: fix max_node_id
> interconnect: imx: set src node
> interconnect: imx: introduce imx_icc_provider
> interconnect: imx: set of_node for interconnect provider
> interconnect: imx: configure NoC mode/prioriry/ext_control
> interconnect: imx: Add platform driver for imx8mp
>
> .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> drivers/interconnect/bulk.c | 34 +++
> drivers/interconnect/imx/Kconfig | 4 +
> drivers/interconnect/imx/Makefile | 2 +
> drivers/interconnect/imx/imx.c | 68 +++--
> drivers/interconnect/imx/imx.h | 25 +-
> drivers/interconnect/imx/imx8mm.c | 2 +-
> drivers/interconnect/imx/imx8mn.c | 2 +-
> drivers/interconnect/imx/imx8mp.c | 232
> ++++++++++++++++++
> drivers/interconnect/imx/imx8mq.c | 2 +-
> include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> include/linux/interconnect.h | 6 +
> 12 files changed, 424 insertions(+), 18 deletions(-) create mode 100644
> drivers/interconnect/imx/imx8mp.c create mode 100644
> include/dt-bindings/interconnect/fsl,imx8mp.h
>
> --
> 2.25.1
Hi Peng,
Am Montag, dem 13.06.2022 um 01:23 +0000 schrieb Peng Fan:
> All,
>
> > Subject: [PATCH 0/8] interconnect: support i.MX8MP
>
> I am going to send out V2 this week to address the comments until now.
> But before that I would like to see if any one has any comments on the
> design here.
>
> Georgi, do you have comments on Patch 2 " interconnect: add device
> managed bulk API"
>
> Lucas, since you had comments when I first use syscon to configure NoC,
> are you ok with the design to use interconnect in this patchset?
>
I'm still not 100% convinced that the blk-ctrl is the right consumer
for the interconnect, since it doesn't do any busmastering. However,
the design looks much better than the syscon based one.
I mostly worry about being able to extend this to do more than the
current static configuration if/when NXP decides to release more
information about the NoC configuration options or someone reverse
engineers this part of the SoC. I still hope that we could optimize NoC
usage by setting real bandwidth and latency limits for the devices
connected to the NoC. As the blk-ctrl doesn't have any clue about this
right now, we can't really set any more specific requests than the
current INT_MAX ones.
I guess we could extend things in this way by making the blk-ctrl not
only be a simple consumer of the interconnect, but aggregate requests
from the devices in the blk-ctrl domain and forward them to the NOC
provider, right?
Regards,
Lucas
> Thanks,
> Peng.
>
> >
> > From: Peng Fan <[email protected]>
> >
> > This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC initial value
> > after power up is invalid, need set a valid value after related power domain up.
> >
> > This patchset also includes two patch[1,2] during my development to enable the
> > ICC feature for i.MX8MP.
> >
> > I not include ddrc DVFS in this patchset, ths patchset is only to support NoC
> > value mode/priority/ext_control being set to a valid value that suggested by
> > i.MX Chip Design Team. The value is same as NXP downstream one inside Arm
> > Trusted Firmware:
> > https://source.codeaurora.org/external/imx/imx-atf/tree/plat/imx/imx8m/imx
> > 8mp/gpc.c?h=lf_v2.4#n97
> >
> > A repo created here:
> > https://github.com/MrVan/linux/tree/imx8mp-interconnect
> >
> > Peng Fan (8):
> > dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> > interconnect: add device managed bulk API
> > interconnect: imx: fix max_node_id
> > interconnect: imx: set src node
> > interconnect: imx: introduce imx_icc_provider
> > interconnect: imx: set of_node for interconnect provider
> > interconnect: imx: configure NoC mode/prioriry/ext_control
> > interconnect: imx: Add platform driver for imx8mp
> >
> > .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> > drivers/interconnect/bulk.c | 34 +++
> > drivers/interconnect/imx/Kconfig | 4 +
> > drivers/interconnect/imx/Makefile | 2 +
> > drivers/interconnect/imx/imx.c | 68 +++--
> > drivers/interconnect/imx/imx.h | 25 +-
> > drivers/interconnect/imx/imx8mm.c | 2 +-
> > drivers/interconnect/imx/imx8mn.c | 2 +-
> > drivers/interconnect/imx/imx8mp.c | 232
> > ++++++++++++++++++
> > drivers/interconnect/imx/imx8mq.c | 2 +-
> > include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> > include/linux/interconnect.h | 6 +
> > 12 files changed, 424 insertions(+), 18 deletions(-) create mode 100644
> > drivers/interconnect/imx/imx8mp.c create mode 100644
> > include/dt-bindings/interconnect/fsl,imx8mp.h
> >
> > --
> > 2.25.1
>
Hi Lucas,
> Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
>
> Hi Peng,
>
> Am Montag, dem 13.06.2022 um 01:23 +0000 schrieb Peng Fan:
> > All,
> >
> > > Subject: [PATCH 0/8] interconnect: support i.MX8MP
> >
> > I am going to send out V2 this week to address the comments until now.
> > But before that I would like to see if any one has any comments on the
> > design here.
> >
> > Georgi, do you have comments on Patch 2 " interconnect: add device
> > managed bulk API"
> >
> > Lucas, since you had comments when I first use syscon to configure
> > NoC, are you ok with the design to use interconnect in this patchset?
> >
> I'm still not 100% convinced that the blk-ctrl is the right consumer for the
> interconnect, since it doesn't do any busmastering. However, the design looks
> much better than the syscon based one.
>
> I mostly worry about being able to extend this to do more than the current
> static configuration if/when NXP decides to release more information about the
> NoC configuration options or someone reverse engineers this part of the SoC.
I have asked internally, NoC documentation for i.MX8M* is not allowed to public.
I
> still hope that we could optimize NoC usage by setting real bandwidth and
> latency limits for the devices connected to the NoC. As the blk-ctrl doesn't have
> any clue about this right now, we can't really set any more specific requests
> than the current INT_MAX ones.
Actually looking at ATF NoC settings, the values are suggested by Design team,
Design team give SW team such a group of value and not suggest SW team
to change it. And the value in ATF not touch bandwidth registers, as you
could see from the patchset, only mode,priority,ext_control are configured.
Similar to qcom using static settings:
./drivers/interconnect/qcom/qcm2290.c:668.
.qos.qos_mode = NOC_QOS_MODE_FIXED,
I understand that people wanna tune the settings at runtime on demand.
> I guess we could extend things in this way by making the blk-ctrl not only be a
> simple consumer of the interconnect, but aggregate requests from the devices
> in the blk-ctrl domain and forward them to the NOC provider, right?
I am not sure. This patchset is actually only for init NoC settings after
power on, because the initial value is invalid.
I could think how to resolve the INT_MAX settings in next version,
For your upper suggestion, could we start after this version approved for land
in tree?
Thanks,
Peng
>
> Regards,
> Lucas
>
> > Thanks,
> > Peng.
> >
> > >
> > > From: Peng Fan <[email protected]>
> > >
> > > This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC
> > > initial value after power up is invalid, need set a valid value after related
> power domain up.
> > >
> > > This patchset also includes two patch[1,2] during my development to
> > > enable the ICC feature for i.MX8MP.
> > >
> > > I not include ddrc DVFS in this patchset, ths patchset is only to
> > > support NoC value mode/priority/ext_control being set to a valid
> > > value that suggested by i.MX Chip Design Team. The value is same as
> > > NXP downstream one inside Arm Trusted Firmware:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fso
> > >
> urce.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2Ftree%2Fplat%2Fimx%2
> > >
> Fimx8m%2Fimx&data=05%7C01%7Cpeng.fan%40nxp.com%7C6cfad0fcec
> 0d472
> > >
> 408a208da4e2cd96d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C63790
> > >
> 8251778425186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2
> > >
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U
> vIx%
> > >
> 2BAz9rx3Z8Oy7VSCRB90O8M5VICIUaUOiTmYw%2FeI%3D&reserved=0
> > > 8mp/gpc.c?h=lf_v2.4#n97
> > >
> > > A repo created here:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > >
> thub.com%2FMrVan%2Flinux%2Ftree%2Fimx8mp-interconnect&data=05
> %7C
> > >
> 01%7Cpeng.fan%40nxp.com%7C6cfad0fcec0d472408a208da4e2cd96d%7C68
> 6ea1d
> > >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637908251778425186%7CUnkn
> own%7CT
> > >
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> V
> > >
> CI6Mn0%3D%7C3000%7C%7C%7C&sdata=W2iYPMJ6dn%2F4OTalTD2yqB
> Hx%2Bo3%
> > > 2BuBTuP%2BAe4bBz2Gc%3D&reserved=0
> > >
> > > Peng Fan (8):
> > > dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> > > interconnect: add device managed bulk API
> > > interconnect: imx: fix max_node_id
> > > interconnect: imx: set src node
> > > interconnect: imx: introduce imx_icc_provider
> > > interconnect: imx: set of_node for interconnect provider
> > > interconnect: imx: configure NoC mode/prioriry/ext_control
> > > interconnect: imx: Add platform driver for imx8mp
> > >
> > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> > > drivers/interconnect/bulk.c | 34 +++
> > > drivers/interconnect/imx/Kconfig | 4 +
> > > drivers/interconnect/imx/Makefile | 2 +
> > > drivers/interconnect/imx/imx.c | 68 +++--
> > > drivers/interconnect/imx/imx.h | 25 +-
> > > drivers/interconnect/imx/imx8mm.c | 2 +-
> > > drivers/interconnect/imx/imx8mn.c | 2 +-
> > > drivers/interconnect/imx/imx8mp.c | 232
> > > ++++++++++++++++++
> > > drivers/interconnect/imx/imx8mq.c | 2 +-
> > > include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> > > include/linux/interconnect.h | 6 +
> > > 12 files changed, 424 insertions(+), 18 deletions(-) create mode
> > > 100644 drivers/interconnect/imx/imx8mp.c create mode 100644
> > > include/dt-bindings/interconnect/fsl,imx8mp.h
> > >
> > > --
> > > 2.25.1
> >
>
Hi Peng,
Am Dienstag, dem 14.06.2022 um 23:38 +0000 schrieb Peng Fan:
> Hi Lucas,
>
> > Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
> >
> > Hi Peng,
> >
> > Am Montag, dem 13.06.2022 um 01:23 +0000 schrieb Peng Fan:
> > > All,
> > >
> > > > Subject: [PATCH 0/8] interconnect: support i.MX8MP
> > >
> > > I am going to send out V2 this week to address the comments until now.
> > > But before that I would like to see if any one has any comments on the
> > > design here.
> > >
> > > Georgi, do you have comments on Patch 2 " interconnect: add device
> > > managed bulk API"
> > >
> > > Lucas, since you had comments when I first use syscon to configure
> > > NoC, are you ok with the design to use interconnect in this patchset?
> > >
> > I'm still not 100% convinced that the blk-ctrl is the right consumer for the
> > interconnect, since it doesn't do any busmastering. However, the design looks
> > much better than the syscon based one.
> >
> > I mostly worry about being able to extend this to do more than the current
> > static configuration if/when NXP decides to release more information about the
> > NoC configuration options or someone reverse engineers this part of the SoC.
>
> I have asked internally, NoC documentation for i.MX8M* is not allowed to public.
>
Yea, sadly I've heard this many times from NXP.
> I
> > still hope that we could optimize NoC usage by setting real bandwidth and
> > latency limits for the devices connected to the NoC. As the blk-ctrl doesn't have
> > any clue about this right now, we can't really set any more specific requests
> > than the current INT_MAX ones.
>
> Actually looking at ATF NoC settings, the values are suggested by Design team,
> Design team give SW team such a group of value and not suggest SW team
> to change it. And the value in ATF not touch bandwidth registers, as you
> could see from the patchset, only mode,priority,ext_control are configured.
>
> Similar to qcom using static settings:
> ./drivers/interconnect/qcom/qcm2290.c:668.
> .qos.qos_mode = NOC_QOS_MODE_FIXED,
>
> I understand that people wanna tune the settings at runtime on demand.
>
Right. We had the same situation with QoS settings on the i.MX6, where
Freescale/NXP claimed that the values from the design team are optimal
and should not be changed, but we actually had some cases where tuning
those values to the specific use-case of a board was beneficial. With
the i.MX6 we could do this on our own, as things were documented, at
least partially.
I don't request you or anyone from the NXP open source team to do
something here, as I understand that the no documentation policy is an
outside decision that you can not really change. I just want to make
sure that if someone was to do something about this situation, we don't
make that change harder than necessary by locking us into a DT binding
and design that might be hard to change later on.
> > I guess we could extend things in this way by making the blk-ctrl not only be a
> > simple consumer of the interconnect, but aggregate requests from the devices
> > in the blk-ctrl domain and forward them to the NOC provider, right?
>
> I am not sure. This patchset is actually only for init NoC settings after
> power on, because the initial value is invalid.
>
> I could think how to resolve the INT_MAX settings in next version,
> For your upper suggestion, could we start after this version approved for land
> in tree?
>
I just want you to think about how we could extend the design laid down
in this patchset if/when the peripheral drivers are starting to request
their actual bandwidth usage. If the answer to this question is "we'll
simply make the blk-ctl part of the interconnect hierarchy and let it
aggregate the bandwidth requests and forward them to the NoC driver"
then I'm fine with this patchset landing in upstream as-is. I'm just
not sure if I'm overlooking something here which would prevent such an
extension of the design, as I'm not a expert in the interconnect
framework.
Regards,
Lucas
>
> Thanks,
> Peng
>
>
> >
> > Regards,
> > Lucas
> >
> > > Thanks,
> > > Peng.
> > >
> > > >
> > > > From: Peng Fan <[email protected]>
> > > >
> > > > This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC
> > > > initial value after power up is invalid, need set a valid value after related
> > power domain up.
> > > >
> > > > This patchset also includes two patch[1,2] during my development to
> > > > enable the ICC feature for i.MX8MP.
> > > >
> > > > I not include ddrc DVFS in this patchset, ths patchset is only to
> > > > support NoC value mode/priority/ext_control being set to a valid
> > > > value that suggested by i.MX Chip Design Team. The value is same as
> > > > NXP downstream one inside Arm Trusted Firmware:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fso
> > > >
> > urce.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2Ftree%2Fplat%2Fimx%2
> > > >
> > Fimx8m%2Fimx&data=05%7C01%7Cpeng.fan%40nxp.com%7C6cfad0fcec
> > 0d472
> > > >
> > 408a208da4e2cd96d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > C63790
> > > >
> > 8251778425186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > QIjoiV2
> > > >
> > luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U
> > vIx%
> > > >
> > 2BAz9rx3Z8Oy7VSCRB90O8M5VICIUaUOiTmYw%2FeI%3D&reserved=0
> > > > 8mp/gpc.c?h=lf_v2.4#n97
> > > >
> > > > A repo created here:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > >
> > thub.com%2FMrVan%2Flinux%2Ftree%2Fimx8mp-interconnect&data=05
> > %7C
> > > >
> > 01%7Cpeng.fan%40nxp.com%7C6cfad0fcec0d472408a208da4e2cd96d%7C68
> > 6ea1d
> > > >
> > 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637908251778425186%7CUnkn
> > own%7CT
> > > >
> > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> > V
> > > >
> > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=W2iYPMJ6dn%2F4OTalTD2yqB
> > Hx%2Bo3%
> > > > 2BuBTuP%2BAe4bBz2Gc%3D&reserved=0
> > > >
> > > > Peng Fan (8):
> > > > dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> > > > interconnect: add device managed bulk API
> > > > interconnect: imx: fix max_node_id
> > > > interconnect: imx: set src node
> > > > interconnect: imx: introduce imx_icc_provider
> > > > interconnect: imx: set of_node for interconnect provider
> > > > interconnect: imx: configure NoC mode/prioriry/ext_control
> > > > interconnect: imx: Add platform driver for imx8mp
> > > >
> > > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> > > > drivers/interconnect/bulk.c | 34 +++
> > > > drivers/interconnect/imx/Kconfig | 4 +
> > > > drivers/interconnect/imx/Makefile | 2 +
> > > > drivers/interconnect/imx/imx.c | 68 +++--
> > > > drivers/interconnect/imx/imx.h | 25 +-
> > > > drivers/interconnect/imx/imx8mm.c | 2 +-
> > > > drivers/interconnect/imx/imx8mn.c | 2 +-
> > > > drivers/interconnect/imx/imx8mp.c | 232
> > > > ++++++++++++++++++
> > > > drivers/interconnect/imx/imx8mq.c | 2 +-
> > > > include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> > > > include/linux/interconnect.h | 6 +
> > > > 12 files changed, 424 insertions(+), 18 deletions(-) create mode
> > > > 100644 drivers/interconnect/imx/imx8mp.c create mode 100644
> > > > include/dt-bindings/interconnect/fsl,imx8mp.h
> > > >
> > > > --
> > > > 2.25.1
> > >
> >
>
Hi Lucas,
> Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
>
> Hi Peng,
>
> Am Dienstag, dem 14.06.2022 um 23:38 +0000 schrieb Peng Fan:
> > Hi Lucas,
> >
> > > Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
> > >
> > > Hi Peng,
> > >
> > > Am Montag, dem 13.06.2022 um 01:23 +0000 schrieb Peng Fan:
> > > > All,
> > > >
> > > > > Subject: [PATCH 0/8] interconnect: support i.MX8MP
> > > >
> > > > I am going to send out V2 this week to address the comments until now.
> > > > But before that I would like to see if any one has any comments on
> > > > the design here.
> > > >
> > > > Georgi, do you have comments on Patch 2 " interconnect: add device
> > > > managed bulk API"
> > > >
> > > > Lucas, since you had comments when I first use syscon to configure
> > > > NoC, are you ok with the design to use interconnect in this patchset?
> > > >
> > > I'm still not 100% convinced that the blk-ctrl is the right consumer
> > > for the interconnect, since it doesn't do any busmastering. However,
> > > the design looks much better than the syscon based one.
> > >
> > > I mostly worry about being able to extend this to do more than the
> > > current static configuration if/when NXP decides to release more
> > > information about the NoC configuration options or someone reverse
> engineers this part of the SoC.
> >
> > I have asked internally, NoC documentation for i.MX8M* is not allowed to
> public.
> >
> Yea, sadly I've heard this many times from NXP.
>
> > I
> > > still hope that we could optimize NoC usage by setting real
> > > bandwidth and latency limits for the devices connected to the NoC.
> > > As the blk-ctrl doesn't have any clue about this right now, we can't
> > > really set any more specific requests than the current INT_MAX ones.
> >
> > Actually looking at ATF NoC settings, the values are suggested by
> > Design team, Design team give SW team such a group of value and not
> > suggest SW team to change it. And the value in ATF not touch bandwidth
> > registers, as you could see from the patchset, only mode,priority,ext_control
> are configured.
> >
> > Similar to qcom using static settings:
> > ./drivers/interconnect/qcom/qcm2290.c:668.
> > .qos.qos_mode = NOC_QOS_MODE_FIXED,
> >
> > I understand that people wanna tune the settings at runtime on demand.
> >
> Right. We had the same situation with QoS settings on the i.MX6, where
> Freescale/NXP claimed that the values from the design team are optimal and
> should not be changed, but we actually had some cases where tuning those
> values to the specific use-case of a board was beneficial. With the i.MX6 we
> could do this on our own, as things were documented, at least partially.
>
> I don't request you or anyone from the NXP open source team to do something
> here, as I understand that the no documentation policy is an outside decision
> that you can not really change. I just want to make sure that if someone was to
> do something about this situation, we don't make that change harder than
> necessary by locking us into a DT binding and design that might be hard to
> change later on.
Hope I could help on this if you have suggestion on technical direction without
breaking NXP policy.
>
> > > I guess we could extend things in this way by making the blk-ctrl
> > > not only be a simple consumer of the interconnect, but aggregate
> > > requests from the devices in the blk-ctrl domain and forward them to the
> NOC provider, right?
> >
> > I am not sure. This patchset is actually only for init NoC settings
> > after power on, because the initial value is invalid.
> >
> > I could think how to resolve the INT_MAX settings in next version, For
> > your upper suggestion, could we start after this version approved for
> > land in tree?
> >
> I just want you to think about how we could extend the design laid down in this
> patchset if/when the peripheral drivers are starting to request their actual
> bandwidth usage. If the answer to this question is "we'll simply make the blk-ctl
> part of the interconnect hierarchy and let it aggregate the bandwidth requests
> and forward them to the NoC driver"
> then I'm fine with this patchset landing in upstream as-is. I'm just not sure if I'm
> overlooking something here which would prevent such an extension of the
> design, as I'm not a expert in the interconnect framework.
There is only priority level as I write in the driver, the priority level is not suggested
to runtime change according to i.MX design team.
For media related settings, there is GPR hurry level settings which is written
in RM and ATF code.
In normal case, if you really wanna change priority level, you could use
other value. I'll update driver to describe priority bit mask.
In some heavy load mode, you could pre-set a GPR hurry level value to boost
the transaction.
But since design team not suggest SW to runtime update the value, also no
idea to match priority level with SW bandwidth. from driver debug, what
I could improve is the describe the priority register, then people wanna
to change, could update it.
Is this ok?
Thanks,
Peng.
>
> Regards,
> Lucas
>
> >
> > Thanks,
> > Peng
> >
> >
> > >
> > > Regards,
> > > Lucas
> > >
> > > > Thanks,
> > > > Peng.
> > > >
> > > > >
> > > > > From: Peng Fan <[email protected]>
> > > > >
> > > > > This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC
> > > > > initial value after power up is invalid, need set a valid value
> > > > > after related
> > > power domain up.
> > > > >
> > > > > This patchset also includes two patch[1,2] during my development
> > > > > to enable the ICC feature for i.MX8MP.
> > > > >
> > > > > I not include ddrc DVFS in this patchset, ths patchset is only
> > > > > to support NoC value mode/priority/ext_control being set to a
> > > > > valid value that suggested by i.MX Chip Design Team. The value
> > > > > is same as NXP downstream one inside Arm Trusted Firmware:
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > 2Fso
> > > > >
> > >
> urce.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2Ftree%2Fplat%2Fimx%2
> > > > >
> > >
> Fimx8m%2Fimx&data=05%7C01%7Cpeng.fan%40nxp.com%7C6cfad0fcec
> > > 0d472
> > > > >
> > >
> 408a208da4e2cd96d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > > C63790
> > > > >
> > >
> 8251778425186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > QIjoiV2
> > > > >
> > >
> luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U
> > > vIx%
> > > > >
> > >
> 2BAz9rx3Z8Oy7VSCRB90O8M5VICIUaUOiTmYw%2FeI%3D&reserved=0
> > > > > 8mp/gpc.c?h=lf_v2.4#n97
> > > > >
> > > > > A repo created here:
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > 2Fgi
> > > > >
> > >
> thub.com%2FMrVan%2Flinux%2Ftree%2Fimx8mp-interconnect&data=05
> > > %7C
> > > > >
> > >
> 01%7Cpeng.fan%40nxp.com%7C6cfad0fcec0d472408a208da4e2cd96d%7C68
> > > 6ea1d
> > > > >
> > >
> 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637908251778425186%7CUnkn
> > > own%7CT
> > > > >
> > >
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> > > V
> > > > >
> > >
> CI6Mn0%3D%7C3000%7C%7C%7C&sdata=W2iYPMJ6dn%2F4OTalTD2yqB
> > > Hx%2Bo3%
> > > > > 2BuBTuP%2BAe4bBz2Gc%3D&reserved=0
> > > > >
> > > > > Peng Fan (8):
> > > > > dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> > > > > interconnect: add device managed bulk API
> > > > > interconnect: imx: fix max_node_id
> > > > > interconnect: imx: set src node
> > > > > interconnect: imx: introduce imx_icc_provider
> > > > > interconnect: imx: set of_node for interconnect provider
> > > > > interconnect: imx: configure NoC mode/prioriry/ext_control
> > > > > interconnect: imx: Add platform driver for imx8mp
> > > > >
> > > > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> > > > > drivers/interconnect/bulk.c | 34 +++
> > > > > drivers/interconnect/imx/Kconfig | 4 +
> > > > > drivers/interconnect/imx/Makefile | 2 +
> > > > > drivers/interconnect/imx/imx.c | 68 +++--
> > > > > drivers/interconnect/imx/imx.h | 25 +-
> > > > > drivers/interconnect/imx/imx8mm.c | 2 +-
> > > > > drivers/interconnect/imx/imx8mn.c | 2 +-
> > > > > drivers/interconnect/imx/imx8mp.c | 232
> > > > > ++++++++++++++++++
> > > > > drivers/interconnect/imx/imx8mq.c | 2 +-
> > > > > include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> > > > > include/linux/interconnect.h | 6 +
> > > > > 12 files changed, 424 insertions(+), 18 deletions(-) create
> > > > > mode
> > > > > 100644 drivers/interconnect/imx/imx8mp.c create mode 100644
> > > > > include/dt-bindings/interconnect/fsl,imx8mp.h
> > > > >
> > > > > --
> > > > > 2.25.1
> > > >
> > >
> >
>
Am Mittwoch, dem 15.06.2022 um 09:28 +0000 schrieb Peng Fan:
> Hi Lucas,
>
> > Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
> >
> > Hi Peng,
> >
> > Am Dienstag, dem 14.06.2022 um 23:38 +0000 schrieb Peng Fan:
> > > Hi Lucas,
> > >
> > > > Subject: Re: [PATCH 0/8] interconnect: support i.MX8MP
> > > >
> > > > Hi Peng,
> > > >
> > > > Am Montag, dem 13.06.2022 um 01:23 +0000 schrieb Peng Fan:
> > > > > All,
> > > > >
> > > > > > Subject: [PATCH 0/8] interconnect: support i.MX8MP
> > > > >
> > > > > I am going to send out V2 this week to address the comments until now.
> > > > > But before that I would like to see if any one has any comments on
> > > > > the design here.
> > > > >
> > > > > Georgi, do you have comments on Patch 2 " interconnect: add device
> > > > > managed bulk API"
> > > > >
> > > > > Lucas, since you had comments when I first use syscon to configure
> > > > > NoC, are you ok with the design to use interconnect in this patchset?
> > > > >
> > > > I'm still not 100% convinced that the blk-ctrl is the right consumer
> > > > for the interconnect, since it doesn't do any busmastering. However,
> > > > the design looks much better than the syscon based one.
> > > >
> > > > I mostly worry about being able to extend this to do more than the
> > > > current static configuration if/when NXP decides to release more
> > > > information about the NoC configuration options or someone reverse
> > engineers this part of the SoC.
> > >
> > > I have asked internally, NoC documentation for i.MX8M* is not allowed to
> > public.
> > >
> > Yea, sadly I've heard this many times from NXP.
> >
> > > I
> > > > still hope that we could optimize NoC usage by setting real
> > > > bandwidth and latency limits for the devices connected to the NoC.
> > > > As the blk-ctrl doesn't have any clue about this right now, we can't
> > > > really set any more specific requests than the current INT_MAX ones.
> > >
> > > Actually looking at ATF NoC settings, the values are suggested by
> > > Design team, Design team give SW team such a group of value and not
> > > suggest SW team to change it. And the value in ATF not touch bandwidth
> > > registers, as you could see from the patchset, only mode,priority,ext_control
> > are configured.
> > >
> > > Similar to qcom using static settings:
> > > ./drivers/interconnect/qcom/qcm2290.c:668.
> > > .qos.qos_mode = NOC_QOS_MODE_FIXED,
> > >
> > > I understand that people wanna tune the settings at runtime on demand.
> > >
> > Right. We had the same situation with QoS settings on the i.MX6, where
> > Freescale/NXP claimed that the values from the design team are optimal and
> > should not be changed, but we actually had some cases where tuning those
> > values to the specific use-case of a board was beneficial. With the i.MX6 we
> > could do this on our own, as things were documented, at least partially.
> >
> > I don't request you or anyone from the NXP open source team to do something
> > here, as I understand that the no documentation policy is an outside decision
> > that you can not really change. I just want to make sure that if someone was to
> > do something about this situation, we don't make that change harder than
> > necessary by locking us into a DT binding and design that might be hard to
> > change later on.
>
> Hope I could help on this if you have suggestion on technical direction without
> breaking NXP policy.
>
> >
> > > > I guess we could extend things in this way by making the blk-ctrl
> > > > not only be a simple consumer of the interconnect, but aggregate
> > > > requests from the devices in the blk-ctrl domain and forward them to the
> > NOC provider, right?
> > >
> > > I am not sure. This patchset is actually only for init NoC settings
> > > after power on, because the initial value is invalid.
> > >
> > > I could think how to resolve the INT_MAX settings in next version, For
> > > your upper suggestion, could we start after this version approved for
> > > land in tree?
> > >
> > I just want you to think about how we could extend the design laid down in this
> > patchset if/when the peripheral drivers are starting to request their actual
> > bandwidth usage. If the answer to this question is "we'll simply make the blk-ctl
> > part of the interconnect hierarchy and let it aggregate the bandwidth requests
> > and forward them to the NoC driver"
> > then I'm fine with this patchset landing in upstream as-is. I'm just not sure if I'm
> > overlooking something here which would prevent such an extension of the
> > design, as I'm not a expert in the interconnect framework.
>
> There is only priority level as I write in the driver, the priority level is not suggested
> to runtime change according to i.MX design team.
> For media related settings, there is GPR hurry level settings which is written
> in RM and ATF code.
>
> In normal case, if you really wanna change priority level, you could use
> other value. I'll update driver to describe priority bit mask.
> In some heavy load mode, you could pre-set a GPR hurry level value to boost
> the transaction.
>
> But since design team not suggest SW to runtime update the value, also no
> idea to match priority level with SW bandwidth. from driver debug, what
> I could improve is the describe the priority register, then people wanna
> to change, could update it.
But we all know that you could do more interesting things in regulator
mode with bandwidth setting changed according to use-case, right? ;)
>
> Is this ok?
>
I don't worry too much about the specific implementation in the drivers
right now, as this is something we can change later on. What I'm
worried about is more the DT description. With the description in this
patchset we put the blk-ctrl in charge of requesting the bandwidth
limits, as it's the consumer of the interconnect. But it doesn't really
have any information about the real bandwidth/latency/whatever other
QoS settings of the peripherals, so it can't do a proper request.
If it is possible to extend the current design in the future by making
the blk-ctrl be a interconnect provider to the peripherals in the power
domains _without_ changing the DT description between the blk-ctrl and
NoC node, then I'm fine with the design in this patchset.
Regards,
Lucas
> Thanks,
> Peng.
>
> >
> > Regards,
> > Lucas
> >
> > >
> > > Thanks,
> > > Peng
> > >
> > >
> > > >
> > > > Regards,
> > > > Lucas
> > > >
> > > > > Thanks,
> > > > > Peng.
> > > > >
> > > > > >
> > > > > > From: Peng Fan <[email protected]>
> > > > > >
> > > > > > This patchset is to support i.MX8MP NoC settings, i.MX8MP NoC
> > > > > > initial value after power up is invalid, need set a valid value
> > > > > > after related
> > > > power domain up.
> > > > > >
> > > > > > This patchset also includes two patch[1,2] during my development
> > > > > > to enable the ICC feature for i.MX8MP.
> > > > > >
> > > > > > I not include ddrc DVFS in this patchset, ths patchset is only
> > > > > > to support NoC value mode/priority/ext_control being set to a
> > > > > > valid value that suggested by i.MX Chip Design Team. The value
> > > > > > is same as NXP downstream one inside Arm Trusted Firmware:
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > > 2Fso
> > > > > >
> > > >
> > urce.codeaurora.org%2Fexternal%2Fimx%2Fimx-atf%2Ftree%2Fplat%2Fimx%2
> > > > > >
> > > >
> > Fimx8m%2Fimx&data=05%7C01%7Cpeng.fan%40nxp.com%7C6cfad0fcec
> > > > 0d472
> > > > > >
> > > >
> > 408a208da4e2cd96d%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > > > C63790
> > > > > >
> > > >
> > 8251778425186%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> > > > QIjoiV2
> > > > > >
> > > >
> > luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=U
> > > > vIx%
> > > > > >
> > > >
> > 2BAz9rx3Z8Oy7VSCRB90O8M5VICIUaUOiTmYw%2FeI%3D&reserved=0
> > > > > > 8mp/gpc.c?h=lf_v2.4#n97
> > > > > >
> > > > > > A repo created here:
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%
> > > > > > 2Fgi
> > > > > >
> > > >
> > thub.com%2FMrVan%2Flinux%2Ftree%2Fimx8mp-interconnect&data=05
> > > > %7C
> > > > > >
> > > >
> > 01%7Cpeng.fan%40nxp.com%7C6cfad0fcec0d472408a208da4e2cd96d%7C68
> > > > 6ea1d
> > > > > >
> > > >
> > 3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637908251778425186%7CUnkn
> > > > own%7CT
> > > > > >
> > > >
> > WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJX
> > > > V
> > > > > >
> > > >
> > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=W2iYPMJ6dn%2F4OTalTD2yqB
> > > > Hx%2Bo3%
> > > > > > 2BuBTuP%2BAe4bBz2Gc%3D&reserved=0
> > > > > >
> > > > > > Peng Fan (8):
> > > > > > dt-bindings: interconnect: imx8m: Add bindings for imx8mp noc
> > > > > > interconnect: add device managed bulk API
> > > > > > interconnect: imx: fix max_node_id
> > > > > > interconnect: imx: set src node
> > > > > > interconnect: imx: introduce imx_icc_provider
> > > > > > interconnect: imx: set of_node for interconnect provider
> > > > > > interconnect: imx: configure NoC mode/prioriry/ext_control
> > > > > > interconnect: imx: Add platform driver for imx8mp
> > > > > >
> > > > > > .../bindings/interconnect/fsl,imx8m-noc.yaml | 6 +
> > > > > > drivers/interconnect/bulk.c | 34 +++
> > > > > > drivers/interconnect/imx/Kconfig | 4 +
> > > > > > drivers/interconnect/imx/Makefile | 2 +
> > > > > > drivers/interconnect/imx/imx.c | 68 +++--
> > > > > > drivers/interconnect/imx/imx.h | 25 +-
> > > > > > drivers/interconnect/imx/imx8mm.c | 2 +-
> > > > > > drivers/interconnect/imx/imx8mn.c | 2 +-
> > > > > > drivers/interconnect/imx/imx8mp.c | 232
> > > > > > ++++++++++++++++++
> > > > > > drivers/interconnect/imx/imx8mq.c | 2 +-
> > > > > > include/dt-bindings/interconnect/fsl,imx8mp.h | 59 +++++
> > > > > > include/linux/interconnect.h | 6 +
> > > > > > 12 files changed, 424 insertions(+), 18 deletions(-) create
> > > > > > mode
> > > > > > 100644 drivers/interconnect/imx/imx8mp.c create mode 100644
> > > > > > include/dt-bindings/interconnect/fsl,imx8mp.h
> > > > > >
> > > > > > --
> > > > > > 2.25.1
> > > > >
> > > >
> > >
> >
>