| Message ID | 1549290167-876-2-git-send-email-rogerq@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for TI PRU ICSS | expand |
Hi, * Roger Quadros <rogerq@ti.com> [190204 14:23]: > From: Suman Anna <s-anna@ti.com> ... > +Example: > +======== > +1. /* AM33xx PRU-ICSS */ > + > + pruss: pruss@0 { > + compatible = "ti,am3356-pruss"; > + reg = <0x0 0x2000>, > + <0x2000 0x2000>, > + <0x10000 0x3000>; > + reg-names = "dram0", "dram1", > + "shrdram2"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; Thanks for fixing up the reg ranges for the top level node. Ideally there would not even be a top level node here as AFAIK the whole PRUSS is a collection of devices on a PRU internal interconnect. So following that path a bit further.. How about just get rid of the top level node and just do: pruss: pruss@0 { dram0: memory@0 { device_type = "memory"; reg = <0x0 0x2000>; }; dram1: memory@2000 { device_type = "memory"; reg = <0x2000 0x2000>; }; shrdram2: memory@10000 { device_type = "memory"; reg = <0x10000 0x3000>; }; pruss_cfg: cfg@26000 { ... }; ... }; If the device_type = "memory" cannot be used here for being specific to the top level properties, then there's probably some other generic property usable here :) > + pruss_mii_rt: mii_rt@32000 { > + reg = <0x32000 0x58>; > + }; The node name should not have underscores so pruss_mii_rt: mii-rt@32000. Please check the others too, like app_node. > + app_node: app_node { > + prus = <&pru0>, <&pru1>; > + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; > + ti,pruss-gp-mux-sel = <2>, <1>; > + /* setup interrupts for prus: > + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, > + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ > + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; > + } If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are firmware configuration options, maybe leave them out of the dts completely and make the app-node optional. And have a proper compatible for this node such as "ti,pruss-app-xyz". And this should be only set if the the hardware is wired up in such way that things need to be configured in the dts rather than by the firmware. And then you can just hide mux-sel and interrupt-map behind the compatible property for that hardware. And leave them out from the dts and have the handling driver would set mux-sel and interrupt-map based on the match->data during probe. Regards, Tony
Hi Tony & Suman, On 04/02/19 18:33, Tony Lindgren wrote: > Hi, > > * Roger Quadros <rogerq@ti.com> [190204 14:23]: >> From: Suman Anna <s-anna@ti.com> > ... >> +Example: >> +======== >> +1. /* AM33xx PRU-ICSS */ >> + >> + pruss: pruss@0 { >> + compatible = "ti,am3356-pruss"; >> + reg = <0x0 0x2000>, >> + <0x2000 0x2000>, >> + <0x10000 0x3000>; >> + reg-names = "dram0", "dram1", >> + "shrdram2"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; > > Thanks for fixing up the reg ranges for the top level node. > > Ideally there would not even be a top level node here as > AFAIK the whole PRUSS is a collection of devices on a PRU > internal interconnect. So following that path a bit further.. > How about just get rid of the top level node and just do: > > pruss: pruss@0 { > dram0: memory@0 { > device_type = "memory"; > reg = <0x0 0x2000>; > }; > > dram1: memory@2000 { > device_type = "memory"; > reg = <0x2000 0x2000>; > }; Actually dram0 and dram1 are data memories for PRU0 and PRU1 respectively. Isn't it better if they are moved to the pru node? e.g. pru0: pru@34000 { compatible = "ti,am3356-pru"; reg = <0x34000 0x2000>, <0x22000 0x400>, <0x22400 0x100>, <0x0 0x2000>; reg-names = "iram", "control", "debug", "dram"; ... }; pru1: pru@38000 { compatible = "ti,am3356-pru"; reg = <0x38000 0x2000>, <0x24000 0x400>, <0x24400 0x100>, <0x2000 0x2000>; reg-names = "iram", "control", "debug", "dram"; ... }; I think it is better to place a restriction that firmware on PRU0 cannot use data memory of PRU1 and vice versa. Application drivers do sometimes need to read/write to data memory. The pru_rproc driver could provide a API for the application drivers to get virtual address of the respective PRU's data memory. > > shrdram2: memory@10000 { > device_type = "memory"; > reg = <0x10000 0x3000>; > }; Shared RAM is not so straight forward. Both PRU firmwares and both application drivers might need to read/write here. The area split is decided by firmware design and there is no hardware protection to prevent from stomping on each others toes. We need a carveout based memory allocator at least I think that can do a allocate(base_offset, size); into shared RAM. This could be used by pru_rproc driver at firmware load time and by application drivers at initialization time. Thoughts? > > pruss_cfg: cfg@26000 { > ... > }; > ... > }; > > If the device_type = "memory" cannot be used here for > being specific to the top level properties, then > there's probably some other generic property usable > here :) > >> + pruss_mii_rt: mii_rt@32000 { >> + reg = <0x32000 0x58>; >> + }; > > The node name should not have underscores so > pruss_mii_rt: mii-rt@32000. Please check the others > too, like app_node. > OK. >> + app_node: app_node { >> + prus = <&pru0>, <&pru1>; >> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >> + ti,pruss-gp-mux-sel = <2>, <1>; >> + /* setup interrupts for prus: >> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >> + } > > If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are > firmware configuration options, maybe leave them out of > the dts completely and make the app-node optional. Yes the app-node is optional. I will mention it. No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. But these settings are application/firmware specific. ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt controller. ti,pruss-gp-mux-sel is used to configure this register. "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf "29:26 PR1_PRU0_GP_MUX_SEL" It configures how the pins from the PRUSS module are routed internally to the various modules. see "30.2.1 PRU-ICSS I/O Interface" and "Table 30-1. PRU-ICSS1 I/O Signals" > > And have a proper compatible for this node such as > "ti,pruss-app-xyz". And this should be only set if the the > hardware is wired up in such way that things need to be > configured in the dts rather than by the firmware. Yes, compatible is a required property as we need to load the appropriate application (kernel space) driver for it. I will fix the example. > > And then you can just hide mux-sel and interrupt-map > behind the compatible property for that hardware. And > leave them out from the dts and have the handling driver > would set mux-sel and interrupt-map based on the > match->data during probe. To summarize: I'll mark the app node as optional. Only required if a kernel driver is required for the application. Compatible is mandatory for app node. ti,pruss-gp-mux-sel and ti,pru-interrupt-map are optional for app node. cheers, -roger
Hi Roger, On 02/05/2019 04:39 AM, Roger Quadros wrote: > Hi Tony & Suman, > > On 04/02/19 18:33, Tony Lindgren wrote: >> Hi, >> >> * Roger Quadros <rogerq@ti.com> [190204 14:23]: >>> From: Suman Anna <s-anna@ti.com> >> ... >>> +Example: >>> +======== >>> +1. /* AM33xx PRU-ICSS */ >>> + >>> + pruss: pruss@0 { >>> + compatible = "ti,am3356-pruss"; >>> + reg = <0x0 0x2000>, >>> + <0x2000 0x2000>, >>> + <0x10000 0x3000>; >>> + reg-names = "dram0", "dram1", >>> + "shrdram2"; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges; >> >> Thanks for fixing up the reg ranges for the top level node. >> >> Ideally there would not even be a top level node here as >> AFAIK the whole PRUSS is a collection of devices on a PRU >> internal interconnect. So following that path a bit further.. >> How about just get rid of the top level node and just do: >> >> pruss: pruss@0 { >> dram0: memory@0 { >> device_type = "memory"; >> reg = <0x0 0x2000>; >> }; >> >> dram1: memory@2000 { >> device_type = "memory"; >> reg = <0x2000 0x2000>; >> }; > > Actually dram0 and dram1 are data memories for PRU0 and PRU1 respectively. > Isn't it better if they are moved to the pru node? > e.g. > > pru0: pru@34000 { > compatible = "ti,am3356-pru"; > reg = <0x34000 0x2000>, > <0x22000 0x400>, > <0x22400 0x100>, > <0x0 0x2000>; > reg-names = "iram", "control", "debug", "dram"; > ... > }; > > pru1: pru@38000 { > compatible = "ti,am3356-pru"; > reg = <0x38000 0x2000>, > <0x24000 0x400>, > <0x24400 0x100>, > <0x2000 0x2000>; > reg-names = "iram", "control", "debug", "dram"; > ... > }; > > I think it is better to place a restriction that firmware on PRU0 cannot use data > memory of PRU1 and vice versa. > That will not work as there are switch firmware cases where PRU access DRAM of other PRU and is a valid case to support in the future. So let us not do that. Murali > Application drivers do sometimes need to read/write to data memory. The pru_rproc > driver could provide a API for the application drivers to get virtual address of > the respective PRU's data memory. > >> >> shrdram2: memory@10000 { >> device_type = "memory"; >> reg = <0x10000 0x3000>; >> }; > > Shared RAM is not so straight forward. Both PRU firmwares and both application drivers > might need to read/write here. The area split is decided by firmware design and there > is no hardware protection to prevent from stomping on each others toes. > > We need a carveout based memory allocator at least I think that can do a > allocate(base_offset, size); into shared RAM. > > This could be used by pru_rproc driver at firmware load time and by application drivers > at initialization time. > > Thoughts? > >> >> pruss_cfg: cfg@26000 { >> ... >> }; >> ... >> }; >> >> If the device_type = "memory" cannot be used here for >> being specific to the top level properties, then >> there's probably some other generic property usable >> here :) >> >>> + pruss_mii_rt: mii_rt@32000 { >>> + reg = <0x32000 0x58>; >>> + }; >> >> The node name should not have underscores so >> pruss_mii_rt: mii-rt@32000. Please check the others >> too, like app_node. >> > > OK. > >>> + app_node: app_node { >>> + prus = <&pru0>, <&pru1>; >>> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >>> + ti,pruss-gp-mux-sel = <2>, <1>; >>> + /* setup interrupts for prus: >>> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >>> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >>> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >>> + } >> >> If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are >> firmware configuration options, maybe leave them out of >> the dts completely and make the app-node optional. > > Yes the app-node is optional. I will mention it. > > No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. > But these settings are application/firmware specific. > > ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt > controller. > > ti,pruss-gp-mux-sel is used to configure this register. > "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf > "29:26 PR1_PRU0_GP_MUX_SEL" > > It configures how the pins from the PRUSS module are routed internally > to the various modules. > > see "30.2.1 PRU-ICSS I/O Interface" > and "Table 30-1. PRU-ICSS1 I/O Signals" > >> >> And have a proper compatible for this node such as >> "ti,pruss-app-xyz". And this should be only set if the the >> hardware is wired up in such way that things need to be >> configured in the dts rather than by the firmware. > > Yes, compatible is a required property as we need to load > the appropriate application (kernel space) driver for it. > I will fix the example. > >> >> And then you can just hide mux-sel and interrupt-map >> behind the compatible property for that hardware. And >> leave them out from the dts and have the handling driver >> would set mux-sel and interrupt-map based on the >> match->data during probe. > > To summarize: > > I'll mark the app node as optional. Only required if a kernel > driver is required for the application. > Compatible is mandatory for app node. > ti,pruss-gp-mux-sel and ti,pru-interrupt-map are optional > for app node. > > cheers, > -roger >
Murali, On 05/02/19 17:08, Murali Karicheri wrote: > Hi Roger, > > On 02/05/2019 04:39 AM, Roger Quadros wrote: >> Hi Tony & Suman, >> >> On 04/02/19 18:33, Tony Lindgren wrote: >>> Hi, >>> >>> * Roger Quadros <rogerq@ti.com> [190204 14:23]: >>>> From: Suman Anna <s-anna@ti.com> >>> ... >>>> +Example: >>>> +======== >>>> +1. /* AM33xx PRU-ICSS */ >>>> + >>>> + pruss: pruss@0 { >>>> + compatible = "ti,am3356-pruss"; >>>> + reg = <0x0 0x2000>, >>>> + <0x2000 0x2000>, >>>> + <0x10000 0x3000>; >>>> + reg-names = "dram0", "dram1", >>>> + "shrdram2"; >>>> + #address-cells = <1>; >>>> + #size-cells = <1>; >>>> + ranges; >>> >>> Thanks for fixing up the reg ranges for the top level node. >>> >>> Ideally there would not even be a top level node here as >>> AFAIK the whole PRUSS is a collection of devices on a PRU >>> internal interconnect. So following that path a bit further.. >>> How about just get rid of the top level node and just do: >>> >>> pruss: pruss@0 { >>> dram0: memory@0 { >>> device_type = "memory"; >>> reg = <0x0 0x2000>; >>> }; >>> >>> dram1: memory@2000 { >>> device_type = "memory"; >>> reg = <0x2000 0x2000>; >>> }; >> >> Actually dram0 and dram1 are data memories for PRU0 and PRU1 respectively. >> Isn't it better if they are moved to the pru node? >> e.g. >> >> pru0: pru@34000 { >> compatible = "ti,am3356-pru"; >> reg = <0x34000 0x2000>, >> <0x22000 0x400>, >> <0x22400 0x100>, >> <0x0 0x2000>; >> reg-names = "iram", "control", "debug", "dram"; >> ... >> }; >> >> pru1: pru@38000 { >> compatible = "ti,am3356-pru"; >> reg = <0x38000 0x2000>, >> <0x24000 0x400>, >> <0x24400 0x100>, >> <0x2000 0x2000>; >> reg-names = "iram", "control", "debug", "dram"; >> ... >> }; >> >> I think it is better to place a restriction that firmware on PRU0 cannot use data >> memory of PRU1 and vice versa. >> > That will not work as there are switch firmware cases where PRU access > DRAM of other PRU and is a valid case to support in the future. So let > us not do that. PRU firmware accessing DRAM of other PRU is a design contract and that use case requires both PRUs to be loaded with matching firmware. That should continue to work. What I'm suggesting here is that kernel remoteproc driver should have nothing to do with the other PRU's data RAM. The application driver if needs both PRUs then it can obviously access both DRAMs as it has a phandle to both PRUs. cheers, -roger > > Murali >> Application drivers do sometimes need to read/write to data memory. The pru_rproc >> driver could provide a API for the application drivers to get virtual address of >> the respective PRU's data memory. >> >>> >>> shrdram2: memory@10000 { >>> device_type = "memory"; >>> reg = <0x10000 0x3000>; >>> }; >> >> Shared RAM is not so straight forward. Both PRU firmwares and both application drivers >> might need to read/write here. The area split is decided by firmware design and there >> is no hardware protection to prevent from stomping on each others toes. >> >> We need a carveout based memory allocator at least I think that can do a >> allocate(base_offset, size); into shared RAM. >> >> This could be used by pru_rproc driver at firmware load time and by application drivers >> at initialization time. >> >> Thoughts? >> >>> >>> pruss_cfg: cfg@26000 { >>> ... >>> }; >>> ... >>> }; >>> >>> If the device_type = "memory" cannot be used here for >>> being specific to the top level properties, then >>> there's probably some other generic property usable >>> here :) >>> >>>> + pruss_mii_rt: mii_rt@32000 { >>>> + reg = <0x32000 0x58>; >>>> + }; >>> >>> The node name should not have underscores so >>> pruss_mii_rt: mii-rt@32000. Please check the others >>> too, like app_node. >>> >> >> OK. >> >>>> + app_node: app_node { >>>> + prus = <&pru0>, <&pru1>; >>>> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >>>> + ti,pruss-gp-mux-sel = <2>, <1>; >>>> + /* setup interrupts for prus: >>>> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >>>> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >>>> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >>>> + } >>> >>> If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are >>> firmware configuration options, maybe leave them out of >>> the dts completely and make the app-node optional. >> >> Yes the app-node is optional. I will mention it. >> >> No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. >> But these settings are application/firmware specific. >> >> ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt >> controller. >> >> ti,pruss-gp-mux-sel is used to configure this register. >> "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf >> "29:26 PR1_PRU0_GP_MUX_SEL" >> >> It configures how the pins from the PRUSS module are routed internally >> to the various modules. >> >> see "30.2.1 PRU-ICSS I/O Interface" >> and "Table 30-1. PRU-ICSS1 I/O Signals" >> >>> >>> And have a proper compatible for this node such as >>> "ti,pruss-app-xyz". And this should be only set if the the >>> hardware is wired up in such way that things need to be >>> configured in the dts rather than by the firmware. >> >> Yes, compatible is a required property as we need to load >> the appropriate application (kernel space) driver for it. >> I will fix the example. >> >>> >>> And then you can just hide mux-sel and interrupt-map >>> behind the compatible property for that hardware. And >>> leave them out from the dts and have the handling driver >>> would set mux-sel and interrupt-map based on the >>> match->data during probe. >> >> To summarize: >> >> I'll mark the app node as optional. Only required if a kernel >> driver is required for the application. >> Compatible is mandatory for app node. >> ti,pruss-gp-mux-sel and ti,pru-interrupt-map are optional >> for app node. >> >> cheers, >> -roger >>
Roger, On 02/05/2019 10:41 AM, Roger Quadros wrote: > Murali, > > On 05/02/19 17:08, Murali Karicheri wrote: >> Hi Roger, >> >> On 02/05/2019 04:39 AM, Roger Quadros wrote: >>> Hi Tony & Suman, >>> >>> On 04/02/19 18:33, Tony Lindgren wrote: >>>> Hi, >>>> >>>> * Roger Quadros <rogerq@ti.com> [190204 14:23]: >>>>> From: Suman Anna <s-anna@ti.com> >>>> ... >>>>> +Example: >>>>> +======== >>>>> +1. /* AM33xx PRU-ICSS */ >>>>> + >>>>> + pruss: pruss@0 { >>>>> + compatible = "ti,am3356-pruss"; >>>>> + reg = <0x0 0x2000>, >>>>> + <0x2000 0x2000>, >>>>> + <0x10000 0x3000>; >>>>> + reg-names = "dram0", "dram1", >>>>> + "shrdram2"; >>>>> + #address-cells = <1>; >>>>> + #size-cells = <1>; >>>>> + ranges; >>>> >>>> Thanks for fixing up the reg ranges for the top level node. >>>> >>>> Ideally there would not even be a top level node here as >>>> AFAIK the whole PRUSS is a collection of devices on a PRU >>>> internal interconnect. So following that path a bit further.. >>>> How about just get rid of the top level node and just do: >>>> >>>> pruss: pruss@0 { >>>> dram0: memory@0 { >>>> device_type = "memory"; >>>> reg = <0x0 0x2000>; >>>> }; >>>> >>>> dram1: memory@2000 { >>>> device_type = "memory"; >>>> reg = <0x2000 0x2000>; >>>> }; >>> >>> Actually dram0 and dram1 are data memories for PRU0 and PRU1 respectively. >>> Isn't it better if they are moved to the pru node? >>> e.g. >>> >>> pru0: pru@34000 { >>> compatible = "ti,am3356-pru"; >>> reg = <0x34000 0x2000>, >>> <0x22000 0x400>, >>> <0x22400 0x100>, >>> <0x0 0x2000>; >>> reg-names = "iram", "control", "debug", "dram"; >>> ... >>> }; >>> >>> pru1: pru@38000 { >>> compatible = "ti,am3356-pru"; >>> reg = <0x38000 0x2000>, >>> <0x24000 0x400>, >>> <0x24400 0x100>, >>> <0x2000 0x2000>; >>> reg-names = "iram", "control", "debug", "dram"; >>> ... >>> }; >>> >>> I think it is better to place a restriction that firmware on PRU0 cannot use data >>> memory of PRU1 and vice versa. >>> >> That will not work as there are switch firmware cases where PRU access >> DRAM of other PRU and is a valid case to support in the future. So let >> us not do that. > > PRU firmware accessing DRAM of other PRU is a design contract and that use case > requires both PRUs to be loaded with matching firmware. That should continue to work. > > What I'm suggesting here is that kernel remoteproc driver should have nothing to do > with the other PRU's data RAM. > > The application driver if needs both PRUs then it can obviously access both DRAMs > as it has a phandle to both PRUs. > That should be fine. Regards, Murali > cheers, > -roger > >> >> Murali >>> Application drivers do sometimes need to read/write to data memory. The pru_rproc >>> driver could provide a API for the application drivers to get virtual address of >>> the respective PRU's data memory. >>> >>>> >>>> shrdram2: memory@10000 { >>>> device_type = "memory"; >>>> reg = <0x10000 0x3000>; >>>> }; >>> >>> Shared RAM is not so straight forward. Both PRU firmwares and both application drivers >>> might need to read/write here. The area split is decided by firmware design and there >>> is no hardware protection to prevent from stomping on each others toes. >>> >>> We need a carveout based memory allocator at least I think that can do a >>> allocate(base_offset, size); into shared RAM. >>> >>> This could be used by pru_rproc driver at firmware load time and by application drivers >>> at initialization time. >>> >>> Thoughts? >>> >>>> >>>> pruss_cfg: cfg@26000 { >>>> ... >>>> }; >>>> ... >>>> }; >>>> >>>> If the device_type = "memory" cannot be used here for >>>> being specific to the top level properties, then >>>> there's probably some other generic property usable >>>> here :) >>>> >>>>> + pruss_mii_rt: mii_rt@32000 { >>>>> + reg = <0x32000 0x58>; >>>>> + }; >>>> >>>> The node name should not have underscores so >>>> pruss_mii_rt: mii-rt@32000. Please check the others >>>> too, like app_node. >>>> >>> >>> OK. >>> >>>>> + app_node: app_node { >>>>> + prus = <&pru0>, <&pru1>; >>>>> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >>>>> + ti,pruss-gp-mux-sel = <2>, <1>; >>>>> + /* setup interrupts for prus: >>>>> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >>>>> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >>>>> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >>>>> + } >>>> >>>> If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are >>>> firmware configuration options, maybe leave them out of >>>> the dts completely and make the app-node optional. >>> >>> Yes the app-node is optional. I will mention it. >>> >>> No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. >>> But these settings are application/firmware specific. >>> >>> ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt >>> controller. >>> >>> ti,pruss-gp-mux-sel is used to configure this register. >>> "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf >>> "29:26 PR1_PRU0_GP_MUX_SEL" >>> >>> It configures how the pins from the PRUSS module are routed internally >>> to the various modules. >>> >>> see "30.2.1 PRU-ICSS I/O Interface" >>> and "Table 30-1. PRU-ICSS1 I/O Signals" >>> >>>> >>>> And have a proper compatible for this node such as >>>> "ti,pruss-app-xyz". And this should be only set if the the >>>> hardware is wired up in such way that things need to be >>>> configured in the dts rather than by the firmware. >>> >>> Yes, compatible is a required property as we need to load >>> the appropriate application (kernel space) driver for it. >>> I will fix the example. >>> >>>> >>>> And then you can just hide mux-sel and interrupt-map >>>> behind the compatible property for that hardware. And >>>> leave them out from the dts and have the handling driver >>>> would set mux-sel and interrupt-map based on the >>>> match->data during probe. >>> >>> To summarize: >>> >>> I'll mark the app node as optional. Only required if a kernel >>> driver is required for the application. >>> Compatible is mandatory for app node. >>> ti,pruss-gp-mux-sel and ti,pru-interrupt-map are optional >>> for app node. >>> >>> cheers, >>> -roger >>> >
* Murali Karicheri <m-karicheri2@ti.com> [190205 16:13]: > On 02/05/2019 10:41 AM, Roger Quadros wrote: > > What I'm suggesting here is that kernel remoteproc driver should have nothing to do > > with the other PRU's data RAM. > > > > The application driver if needs both PRUs then it can obviously access both DRAMs > > as it has a phandle to both PRUs. > > > That should be fine. That sounds good to me too. For dts, yeah please allocate the resources for the modules where the resources belong to on the PRUSS internal interconnect :) Devices can move around on the interconnect between SoCs and the modules can get swapped or added. Regards, Tony
* Roger Quadros <rogerq@ti.com> [190205 09:40]: > On 04/02/19 18:33, Tony Lindgren wrote: > > > > shrdram2: memory@10000 { > > device_type = "memory"; > > reg = <0x10000 0x3000>; > > }; > > Shared RAM is not so straight forward. Both PRU firmwares and both application drivers > might need to read/write here. The area split is decided by firmware design and there > is no hardware protection to prevent from stomping on each others toes. > > We need a carveout based memory allocator at least I think that can do a > allocate(base_offset, size); into shared RAM. > > This could be used by pru_rproc driver at firmware load time and by application drivers > at initialization time. > > Thoughts? That sounds sane to me :) > > If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are > > firmware configuration options, maybe leave them out of > > the dts completely and make the app-node optional. > > Yes the app-node is optional. I will mention it. > > No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. > But these settings are application/firmware specific. > > ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt > controller. OK. So just to see if we have a standard solution available already.. It sounds a bit similar to what we're doing with omap-wakeupgen.c and stacked interrupts? I wonder if something similar might help here? > ti,pruss-gp-mux-sel is used to configure this register. > "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf > "29:26 PR1_PRU0_GP_MUX_SEL" > > It configures how the pins from the PRUSS module are routed internally > to the various modules. > > see "30.2.1 PRU-ICSS I/O Interface" > and "Table 30-1. PRU-ICSS1 I/O Signals" Well these are external signals for PRUSS processor (although not necessarily external signals for the SoC). So why not handle them with a standard pinctlr binding with #pinctrl-cells? Sure it may not even be the Linux pinctrl framework running on the main SoC handling these pins, but after all you're describing hardware for a processor. Maybe Linus W has some comments on this? Regards, Tony
On 05/02/19 18:19, Tony Lindgren wrote: > * Murali Karicheri <m-karicheri2@ti.com> [190205 16:13]: >> On 02/05/2019 10:41 AM, Roger Quadros wrote: >>> What I'm suggesting here is that kernel remoteproc driver should have nothing to do >>> with the other PRU's data RAM. >>> >>> The application driver if needs both PRUs then it can obviously access both DRAMs >>> as it has a phandle to both PRUs. >>> >> That should be fine. > > That sounds good to me too. > > For dts, yeah please allocate the resources for the modules > where the resources belong to on the PRUSS internal interconnect :) > Devices can move around on the interconnect between SoCs and the > modules can get swapped or added. If you take a look at "Figure 30-1. PRU-ICSS Overview" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf You can see that DRAM0 and DRAM1 are not part of PRU. That means they shouldn't be in the PRU node then. cheers, -roger
On Mon, Feb 4, 2019 at 3:24 PM Roger Quadros <rogerq@ti.com> wrote: > From: Suman Anna <s-anna@ti.com> > > This patch adds the bindings for the Programmable Real-Time Unit > and Industrial Communication Subsystem (PRU-ICSS) present on various > SoCs such as AM33xx, AM437x, AM57xx, Keystone 66AK2G SoC, etc. It is > present on the Davinci based OMAPL138 SoCs and K3 architecture > based AM65x SoCs as well (not covered for now). > > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> (...) > + pruss_intc: intc@20000 { > + compatible = "ti,am3356-pruss-intc"; > + reg = <0x20000 0x2000>; > + reg-names = "intc"; > + interrupt-controller; > + #interrupt-cells = <1>; > + interrupts = <20 21 22 23 24 25 26 27>; > + interrupt-names = "host2", "host3", "host4", > + "host5", "host6", "host7", > + "host8", "host9"; If thsese interrupts are mapped 1-to-1 to a parent interrupt controller then this is a hierarchical interrupt domain and then these should be handled locally in the driver as offset from child to parent statically encoded in the driver. Several old drivers and old device tree bindings make this kind of maps, but it is not how we do it anymore, if we can avoid it. To be able to use hierarchical interrupt domain in the kernel, the top interrupt controller must use the hierarchical (v2) irqdomain, so if this is anything else than the ARM GIC it will be an interesting undertaking to handle this. The more I understand of hierarchical irqdomains, the more of workarounds where we should be using it I see, we really need to spread this knowledge. Using it requires a lot of upfront work sometimes, sorry about that but the end result is so much better. Yours, Linus Walleij
On 2/6/19 9:04 AM, Roger Quadros wrote: > > > On 05/02/19 18:19, Tony Lindgren wrote: >> * Murali Karicheri <m-karicheri2@ti.com> [190205 16:13]: >>> On 02/05/2019 10:41 AM, Roger Quadros wrote: >>>> What I'm suggesting here is that kernel remoteproc driver should have nothing to do >>>> with the other PRU's data RAM. >>>> >>>> The application driver if needs both PRUs then it can obviously access both DRAMs >>>> as it has a phandle to both PRUs. >>>> >>> That should be fine. >> >> That sounds good to me too. >> >> For dts, yeah please allocate the resources for the modules >> where the resources belong to on the PRUSS internal interconnect :) >> Devices can move around on the interconnect between SoCs and the >> modules can get swapped or added. > > If you take a look at "Figure 30-1. PRU-ICSS Overview" in > http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf > > You can see that DRAM0 and DRAM1 are not part of PRU. That means > they shouldn't be in the PRU node then. Yes, they do not belong to a PRU, and should not be defined underneath one. Both are accessible from both PRU cores, so it is upto the application on how they can partition the usage. regards Suman
Hi Roger, On 2/4/19 8:22 AM, Roger Quadros wrote: > From: Suman Anna <s-anna@ti.com> > > This patch adds the bindings for the Programmable Real-Time Unit > and Industrial Communication Subsystem (PRU-ICSS) present on various > SoCs such as AM33xx, AM437x, AM57xx, Keystone 66AK2G SoC, etc. It is > present on the Davinci based OMAPL138 SoCs and K3 architecture > based AM65x SoCs as well (not covered for now). > > Signed-off-by: Suman Anna <s-anna@ti.com> > Signed-off-by: Roger Quadros <rogerq@ti.com> > --- > .../devicetree/bindings/soc/ti/ti,pruss.txt | 212 +++++++++++++++++++++ > 1 file changed, 212 insertions(+) > create mode 100644 Documentation/devicetree/bindings/soc/ti/ti,pruss.txt > > diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt > new file mode 100644 > index 0000000..5ac76fd > --- /dev/null > +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt > @@ -0,0 +1,212 @@ > +PRU-ICSS on TI SoCs > +=================== > + > +The Programmable Real-Time Unit and Industrial Communication Subsystem > +(PRU-ICSS) is present on various TI SoCs such as AM335x, AM437x, Keystone > +66AK2G, etc. A PRUSS consists of dual 32-bit RISC cores (Programmable > +Real-Time Units, or PRUs) with program memory and data memory. > + > +The programmable nature of the PRUs provide flexibility to implement > +custom peripheral interfaces, fast real-time responses, or specialized > +data handling. The common peripheral modules include the following, > + > + - Enhanced GPIO with async capture and serial support > + - an Ethernet MII_RT module with two MII ports > + - an MDIO port to control external Ethernet PHYs > + - an Industrial Ethernet Peripheral (IEP) to manage/generate Industrial > + Ethernet functions > + - an Enhanced Capture Module (eCAP) > + - a 16550-compatible UART to support PROFIBUS > + - Interrupt controller with 64 input events and 10 Host interrupts. > + > +A shared Data RAM, if present, can be accessed by both the PRU cores. The > +Interrupt Controller (INTC) and a CFG module are common to both the PRU > +cores. > + > +Various sub-modules within a PRU-ICSS subsystem are represented as individual > +nodes. > + > +PRUSS Node > +============= > + > +This node represents the entire ICSS instance and the various modules are > +contained as children. The PRUSS driver is responsible for managing the > +common resources i.e. DRAM0, DRAM1, SHARED_RAM and CFG space. > + > +Required Properties: > +-------------------- > +- compatible : should be one of, > + "ti,am3356-pruss" for AM335x family of SoCs > + "ti,am4376-pruss" for AM437x family of SoCs > + "ti,am5728-pruss" for AM57xx family of SoCs > + "ti,k2g-pruss" for 66AK2G family of SoCs > +- reg : base address and size for each of the Data RAMs as > + mentioned in reg-names, and in the same order as the > + reg-names Hmm, not sure what prompted you to deviate from the design we had on TI SDK kernels. Your revised bindings does not allow us to use this device for the scalability with either UIO/VFIO. Let's sort this out before you post the next series. regards Suman > +- reg-names : should contain a string(s) from among the following names, > + each representing a specific Data RAM region. Some PRU-ICSS > + instances on certain SoCs might not have Shared DRAM. > + "dram0" for Data RAM0, > + "dram1" for Data RAM1, > + "shrdram2" for Shared Data RAM, > +- #address-cells : should be 1 > +- #size-cells : should be 1 > +- ranges : no specific range translations required, child nodes have the > + same address view as the parent, so should be mentioned without > + any value for the property > + > +Optional Properties: > +-------------------- > +- no-shared-ram : Should be present if the instance doesn't have Shared RAM. > + e.g. AM4376 ICSS0 instance doesn't have Shared RAM. > + > +The PRUSS node will have one or more of the folowing child nodes. > + > +PRU CORES > +========= > +ICSS typically has 2 PRU cores. These should be represented as remoteproc devices. > + > +INTC node > +========= > +ICSS has one INTC interrupt controller module. This should be represented as > +a standard interrupt-controller node. > + > +CFG, IEP, MII_RT > +================ > +The individual sub-modules CFG, IEP and MII_RT are represented as a syscon > +node each with specific node names as below: > + "cfg" for CFG sub-module, > + "iep" for IEP sub-module, > + "mii_rt" for MII-RT sub-module, > + > +See Documentation/devicetree/bindings/mfd/syscon.txt for details. > + > +MDIO > +==== > +Each PRUSS has an MDIO module that can be used to control external PHYs. The > +MDIO module used within the PRU-ICSS is an instance of the MDIO Controller > +used in TI Davinci SoCs. Please refer to the corresponding binding document, > +Documentation/devicetree/bindings/net/davinci-mdio.txt for details. > + > +Application/User Nodes > +======================= > +A PRU application/user node typically uses one or more PRU device nodes to > +implement a PRU application/functionality. Each application/client node would > +need a reference to at least a PRU node, and optionally pass some configuration > +parameters. > + > +Required Properties: > +-------------------- > +- prus : phandles to the PRU nodes used > + > +Optional Properties: > +-------------------- > +- firmware-name : firmwares for the PRU cores, the default firmware > + for the core from the PRU node will be used if not > + provided. The firmware names should correspond to > + the PRU cores listed in the 'prus' property > +- ti,pruss-gp-mux-sel : array of values for the GP_MUX_SEL under PRUSS_GPCFG > + register for a PRU. This selects the internal muxing > + scheme for the PRU instance. If not provided, the > + default out-of-reset value (0) for the PRU core is > + used. Values should correspond to the PRU cores listed > + in the 'prus' property > +- ti,pru-interrupt-map : PRU interrupt mappings, containing an array of entries > + with each entry consisting of 4 cell-values. First one > + is an index towards the "prus" property to identify the > + PRU core for the interrupt map, second is the PRU > + System Event id, third is the PRU interrupt channel id > + and fourth is the PRU host interrupt id. If provided, > + this map will supercede any other configuration > + provided through firmware > + > +Example: > +======== > +1. /* AM33xx PRU-ICSS */ > + > + pruss: pruss@0 { > + compatible = "ti,am3356-pruss"; > + reg = <0x0 0x2000>, > + <0x2000 0x2000>, > + <0x10000 0x3000>; > + reg-names = "dram0", "dram1", > + "shrdram2"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + pruss_cfg: cfg@26000 { > + compatible = "syscon"; > + reg = <0x26000 0x2000>; > + }; > + > + pruss_iep: iep@2e000 { > + compatible = "syscon"; > + reg = <0x2e000 0x31c>; > + }; > + > + pruss_mii_rt: mii_rt@32000 { > + compatible = "syscon"; > + reg = <0x32000 0x58>; > + }; > + > + pruss_intc: intc@20000 { > + compatible = "ti,am3356-pruss-intc"; > + reg = <0x20000 0x2000>; > + reg-names = "intc"; > + interrupt-controller; > + #interrupt-cells = <1>; > + interrupts = <20 21 22 23 24 25 26 27>; > + interrupt-names = "host2", "host3", "host4", > + "host5", "host6", "host7", > + "host8", "host9"; > + }; > + > + pru0: pru@34000 { > + compatible = "ti,am3356-pru"; > + reg = <0x34000 0x2000>, > + <0x22000 0x400>, > + <0x22400 0x100>; > + reg-names = "iram", "control", "debug"; > + gpcfg = <&pruss_cfg 0x8>; > + firmware-name = "am335x-pru0-fw"; > + interrupt-parent = <&pruss_intc>; > + interrupts = <16>, <17>; > + interrupt-names = "vring", "kick"; > + }; > + > + pru1: pru@38000 { > + compatible = "ti,am3356-pru"; > + reg = <0x38000 0x2000>, > + <0x24000 0x400>, > + <0x24400 0x100>; > + reg-names = "iram", "control", "debug"; > + gpcfg = <&pruss_cfg 0xc>; > + firmware-name = "am335x-pru1-fw"; > + interrupt-parent = <&pruss_intc>; > + interrupts = <18>, <19>; > + interrupt-names = "vring", "kick"; > + }; > + > + pruss_mdio: mdio@32400 { > + compatible = "ti,davinci_mdio"; > + reg = <0x32400 0x90>; > + clocks = <&dpll_core_m4_ck>; > + clock-names = "fck"; > + bus_freq = <1000000>; > + #address-cells = <1>; > + #size-cells = <0>; > + status = "disabled"; > + }; > + }; > + > +2: /* PRU application node example */ > + app_node: app_node { > + prus = <&pru0>, <&pru1>; > + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; > + ti,pruss-gp-mux-sel = <2>, <1>; > + /* setup interrupts for prus: > + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, > + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ > + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; > + } >
On 2/5/19 10:41 AM, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [190205 09:40]: >> On 04/02/19 18:33, Tony Lindgren wrote: >>> >>> shrdram2: memory@10000 { >>> device_type = "memory"; >>> reg = <0x10000 0x3000>; >>> }; >> >> Shared RAM is not so straight forward. Both PRU firmwares and both application drivers >> might need to read/write here. The area split is decided by firmware design and there >> is no hardware protection to prevent from stomping on each others toes. >> >> We need a carveout based memory allocator at least I think that can do a >> allocate(base_offset, size); into shared RAM. >> >> This could be used by pru_rproc driver at firmware load time and by application drivers >> at initialization time. >> >> Thoughts? > > That sounds sane to me :) > >>> If the ti,pruss-gp-mux-sel and ti,pru-interrupt-map are >>> firmware configuration options, maybe leave them out of >>> the dts completely and make the app-node optional. >> >> Yes the app-node is optional. I will mention it. >> >> No, ti,pruss-gp-mux-sel and ti,pru-interrupt-map are not firmware options. >> But these settings are application/firmware specific. >> >> ti,pru-interrupt-map specifies the configuration to be used for the INTC interrupt >> controller. > > OK. So just to see if we have a standard solution available already.. > It sounds a bit similar to what we're doing with omap-wakeupgen.c > and stacked interrupts? I wonder if something similar might help > here? > >> ti,pruss-gp-mux-sel is used to configure this register. >> "Table 30-20. PRUSS_GPCFG0" in http://www.tij.co.jp/jp/lit/ug/spruhz7h/spruhz7h.pdf >> "29:26 PR1_PRU0_GP_MUX_SEL" >> >> It configures how the pins from the PRUSS module are routed internally >> to the various modules. Actually, that's not entirely accurate. This is an internal pinmux (not controllable per pin, but rather dictates a different sets of groups of pins at the PRUSS boundary, which are then again multiplexed at the SoC level using the standard padconf/pinmux. It is a single register per core into which you can set some values between 0 through 4 IIRC (unfortunately the values are also not uniform across the various SoCs). regards Suman >> >> see "30.2.1 PRU-ICSS I/O Interface" >> and "Table 30-1. PRU-ICSS1 I/O Signals" > > Well these are external signals for PRUSS processor (although not > necessarily external signals for the SoC). So why not handle them > with a standard pinctlr binding with #pinctrl-cells? > > Sure it may not even be the Linux pinctrl framework running on the > main SoC handling these pins, but after all you're describing > hardware for a processor. Maybe Linus W has some comments on this? > > Regards, > > Tony >
On 2/8/19 7:51 AM, Linus Walleij wrote: > On Mon, Feb 4, 2019 at 3:24 PM Roger Quadros <rogerq@ti.com> wrote: > >> From: Suman Anna <s-anna@ti.com> >> >> This patch adds the bindings for the Programmable Real-Time Unit >> and Industrial Communication Subsystem (PRU-ICSS) present on various >> SoCs such as AM33xx, AM437x, AM57xx, Keystone 66AK2G SoC, etc. It is >> present on the Davinci based OMAPL138 SoCs and K3 architecture >> based AM65x SoCs as well (not covered for now). >> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> > > (...) >> + pruss_intc: intc@20000 { >> + compatible = "ti,am3356-pruss-intc"; >> + reg = <0x20000 0x2000>; >> + reg-names = "intc"; >> + interrupt-controller; >> + #interrupt-cells = <1>; >> + interrupts = <20 21 22 23 24 25 26 27>; >> + interrupt-names = "host2", "host3", "host4", >> + "host5", "host6", "host7", >> + "host8", "host9"; > > If thsese interrupts are mapped 1-to-1 to a parent interrupt controller > then this is a hierarchical interrupt domain and then these should > be handled locally in the driver as offset from child to parent > statically encoded in the driver. > > Several old drivers and old device tree bindings make this kind > of maps, but it is not how we do it anymore, if we can avoid it. > > To be able to use hierarchical interrupt domain in the kernel, the top > interrupt controller must use the hierarchical (v2) irqdomain, so > if this is anything else than the ARM GIC it will be an interesting > undertaking to handle this. These are interrupt lines coming towards the host processor running Linux and are directly connected to the ARM GIC. This INTC module is actually an PRUSS internal interrupt controller that can take in 64 (on most SoCs) external events/interrupt sources and multiplexing them through two layers of many-to-one events-to-intr channels & intr-channels-to-host interrupts. Couple of the host interrupts go to the PRU cores themselves while the remaining ones come out of the IP to connect to other GICs in the SoC. We have implemented this as an irqchip using chained interrupt handlers with the consumers using the event numbers on the Linux-side. The PRUs also access some of the associated registers for clearing an event source. regards Suman > > The more I understand of hierarchical irqdomains, the more of > workarounds where we should be using it I see, we really need > to spread this knowledge. Using it requires a lot of upfront work > sometimes, sorry about that but the end result is so much better. > > Yours, > Linus Walleij >
On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: > [Me] > > To be able to use hierarchical interrupt domain in the kernel, the top > > interrupt controller must use the hierarchical (v2) irqdomain, so > > if this is anything else than the ARM GIC it will be an interesting > > undertaking to handle this. > > These are interrupt lines coming towards the host processor running > Linux and are directly connected to the ARM GIC. This INTC module is > actually an PRUSS internal interrupt controller that can take in 64 (on > most SoCs) external events/interrupt sources and multiplexing them > through two layers of many-to-one events-to-intr channels & > intr-channels-to-host interrupts. Couple of the host interrupts go to > the PRU cores themselves while the remaining ones come out of the IP to > connect to other GICs in the SoC. If the muxing is static (like set up once at probe) so that while the system is running, there is one and one only event mapped to the GIC from the component below it, then it is hierarchical. > We have implemented this as an irqchip using chained interrupt handlers > with the consumers using the event numbers on the Linux-side. The PRUs > also access some of the associated registers for clearing an event source. Chaining with cascading is when two or more interrupts fire the same upper level (say GIC) IRQ. If there is a 1:1 mapping, it is not chained/cascaded but hierarchical. I understand you used old irqdomain/chip frameworks in the past, because everyone was working around the fact that they didn't have an abstraction for hierarchical IRQs. Using chained interrupts and custom 1:1 maps and assigning a long list of IRQs like this patch does was the most common workaround. But we should step out of that habit now. Different levels of the IRQ handling having to do different stuff is what hierarchical irqdomains do best, so it sounds like a good fit. We handle some stuff at our level of the hierarchy and then fall up to the next higher level using calls such as irq_chip_ack_parent(), irq_chip_mask_parent() and friends. Yours, Linus Walleij
On 14/02/19 10:37, Linus Walleij wrote: > On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >> [Me] > >>> To be able to use hierarchical interrupt domain in the kernel, the top >>> interrupt controller must use the hierarchical (v2) irqdomain, so >>> if this is anything else than the ARM GIC it will be an interesting >>> undertaking to handle this. >> >> These are interrupt lines coming towards the host processor running >> Linux and are directly connected to the ARM GIC. This INTC module is >> actually an PRUSS internal interrupt controller that can take in 64 (on >> most SoCs) external events/interrupt sources and multiplexing them >> through two layers of many-to-one events-to-intr channels & >> intr-channels-to-host interrupts. Couple of the host interrupts go to >> the PRU cores themselves while the remaining ones come out of the IP to >> connect to other GICs in the SoC. > > If the muxing is static (like set up once at probe) so that while the system is > running, there is one and one only event mapped to the GIC from > the component below it, then it is hierarchical. This is how it looks. [GIC]<---8---[INTC]<---64---[events from peripherals] The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed per SoC. The muxing between 64 inputs to INTC and its 8 outputs are programmable and might not necessarily be static per boot/probe as it depends on what firmware is loaded on the PRU. A typical PRUSS use case will usually use just one firmware per boot but if required it can switch at runtime and the muxing might change. > >> We have implemented this as an irqchip using chained interrupt handlers >> with the consumers using the event numbers on the Linux-side. The PRUs >> also access some of the associated registers for clearing an event source. > > Chaining with cascading is when two or more interrupts fire the > same upper level (say GIC) IRQ. If there is a 1:1 mapping, > it is not chained/cascaded but hierarchical. > > I understand you used old irqdomain/chip frameworks in the past, > because everyone was working around the fact that they didn't have > an abstraction for hierarchical IRQs. Using chained interrupts > and custom 1:1 maps and assigning a long list of IRQs like this > patch does was the most common workaround. But we should > step out of that habit now. > > Different levels of the IRQ handling having to do different stuff is > what hierarchical irqdomains do best, so it sounds like a good fit. > > We handle some stuff at our level of the hierarchy and then fall > up to the next higher level using calls such as > irq_chip_ack_parent(), irq_chip_mask_parent() and friends. > > Yours, > Linus Walleij > -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
+Robert, Dan, Matthijs Hi Suman, On 14/02/19 04:52, Suman Anna wrote: > Hi Roger, > > On 2/4/19 8:22 AM, Roger Quadros wrote: >> From: Suman Anna <s-anna@ti.com> >> >> This patch adds the bindings for the Programmable Real-Time Unit >> and Industrial Communication Subsystem (PRU-ICSS) present on various >> SoCs such as AM33xx, AM437x, AM57xx, Keystone 66AK2G SoC, etc. It is >> present on the Davinci based OMAPL138 SoCs and K3 architecture >> based AM65x SoCs as well (not covered for now). >> >> Signed-off-by: Suman Anna <s-anna@ti.com> >> Signed-off-by: Roger Quadros <rogerq@ti.com> >> --- >> .../devicetree/bindings/soc/ti/ti,pruss.txt | 212 +++++++++++++++++++++ >> 1 file changed, 212 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >> >> diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >> new file mode 100644 >> index 0000000..5ac76fd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >> @@ -0,0 +1,212 @@ >> +PRU-ICSS on TI SoCs >> +=================== >> + >> +The Programmable Real-Time Unit and Industrial Communication Subsystem >> +(PRU-ICSS) is present on various TI SoCs such as AM335x, AM437x, Keystone >> +66AK2G, etc. A PRUSS consists of dual 32-bit RISC cores (Programmable >> +Real-Time Units, or PRUs) with program memory and data memory. >> + >> +The programmable nature of the PRUs provide flexibility to implement >> +custom peripheral interfaces, fast real-time responses, or specialized >> +data handling. The common peripheral modules include the following, >> + >> + - Enhanced GPIO with async capture and serial support >> + - an Ethernet MII_RT module with two MII ports >> + - an MDIO port to control external Ethernet PHYs >> + - an Industrial Ethernet Peripheral (IEP) to manage/generate Industrial >> + Ethernet functions >> + - an Enhanced Capture Module (eCAP) >> + - a 16550-compatible UART to support PROFIBUS >> + - Interrupt controller with 64 input events and 10 Host interrupts. >> + >> +A shared Data RAM, if present, can be accessed by both the PRU cores. The >> +Interrupt Controller (INTC) and a CFG module are common to both the PRU >> +cores. >> + >> +Various sub-modules within a PRU-ICSS subsystem are represented as individual >> +nodes. >> + >> +PRUSS Node >> +============= >> + >> +This node represents the entire ICSS instance and the various modules are >> +contained as children. The PRUSS driver is responsible for managing the >> +common resources i.e. DRAM0, DRAM1, SHARED_RAM and CFG space. >> + >> +Required Properties: >> +-------------------- >> +- compatible : should be one of, >> + "ti,am3356-pruss" for AM335x family of SoCs >> + "ti,am4376-pruss" for AM437x family of SoCs >> + "ti,am5728-pruss" for AM57xx family of SoCs >> + "ti,k2g-pruss" for 66AK2G family of SoCs >> +- reg : base address and size for each of the Data RAMs as >> + mentioned in reg-names, and in the same order as the >> + reg-names > > Hmm, not sure what prompted you to deviate from the design we had on TI > SDK kernels. Your revised bindings does not allow us to use this device > for the scalability with either UIO/VFIO. My understanding was that the device tree node will have to be modified in any case to support the current uio_pruss.c driver so we don't need to add things that we don't need right away. > > Let's sort this out before you post the next series. Let's discuss this here so I don't have to explain the whole thing again in the next series. Tony, Suman is mainly concerned about the following changes in v2 1) pruss node does not contain reg property representing entire ICSS. 2) pruss node does not contain interrupts. Both of these are required if drivers/uio/uio_pruss.c or in future if VFIO is to be used. The beagleboard community is a primary user of this driver and we need to find a solution so that PRUSS is usable either via remoteproc or via UIO. Ideal case should allow user to use either of the drivers by just doing a unbind and bind. I don't have a better idea than having a encapsulating node that has the appropriate reg and interrupt properties. cheers, -roger > > regards > Suman > >> +- reg-names : should contain a string(s) from among the following names, >> + each representing a specific Data RAM region. Some PRU-ICSS >> + instances on certain SoCs might not have Shared DRAM. >> + "dram0" for Data RAM0, >> + "dram1" for Data RAM1, >> + "shrdram2" for Shared Data RAM, >> +- #address-cells : should be 1 >> +- #size-cells : should be 1 >> +- ranges : no specific range translations required, child nodes have the >> + same address view as the parent, so should be mentioned without >> + any value for the property >> + >> +Optional Properties: >> +-------------------- >> +- no-shared-ram : Should be present if the instance doesn't have Shared RAM. >> + e.g. AM4376 ICSS0 instance doesn't have Shared RAM. >> + >> +The PRUSS node will have one or more of the folowing child nodes. >> + >> +PRU CORES >> +========= >> +ICSS typically has 2 PRU cores. These should be represented as remoteproc devices. >> + >> +INTC node >> +========= >> +ICSS has one INTC interrupt controller module. This should be represented as >> +a standard interrupt-controller node. >> + >> +CFG, IEP, MII_RT >> +================ >> +The individual sub-modules CFG, IEP and MII_RT are represented as a syscon >> +node each with specific node names as below: >> + "cfg" for CFG sub-module, >> + "iep" for IEP sub-module, >> + "mii_rt" for MII-RT sub-module, >> + >> +See Documentation/devicetree/bindings/mfd/syscon.txt for details. >> + >> +MDIO >> +==== >> +Each PRUSS has an MDIO module that can be used to control external PHYs. The >> +MDIO module used within the PRU-ICSS is an instance of the MDIO Controller >> +used in TI Davinci SoCs. Please refer to the corresponding binding document, >> +Documentation/devicetree/bindings/net/davinci-mdio.txt for details. >> + >> +Application/User Nodes >> +======================= >> +A PRU application/user node typically uses one or more PRU device nodes to >> +implement a PRU application/functionality. Each application/client node would >> +need a reference to at least a PRU node, and optionally pass some configuration >> +parameters. >> + >> +Required Properties: >> +-------------------- >> +- prus : phandles to the PRU nodes used >> + >> +Optional Properties: >> +-------------------- >> +- firmware-name : firmwares for the PRU cores, the default firmware >> + for the core from the PRU node will be used if not >> + provided. The firmware names should correspond to >> + the PRU cores listed in the 'prus' property >> +- ti,pruss-gp-mux-sel : array of values for the GP_MUX_SEL under PRUSS_GPCFG >> + register for a PRU. This selects the internal muxing >> + scheme for the PRU instance. If not provided, the >> + default out-of-reset value (0) for the PRU core is >> + used. Values should correspond to the PRU cores listed >> + in the 'prus' property >> +- ti,pru-interrupt-map : PRU interrupt mappings, containing an array of entries >> + with each entry consisting of 4 cell-values. First one >> + is an index towards the "prus" property to identify the >> + PRU core for the interrupt map, second is the PRU >> + System Event id, third is the PRU interrupt channel id >> + and fourth is the PRU host interrupt id. If provided, >> + this map will supercede any other configuration >> + provided through firmware >> + >> +Example: >> +======== >> +1. /* AM33xx PRU-ICSS */ >> + >> + pruss: pruss@0 { >> + compatible = "ti,am3356-pruss"; >> + reg = <0x0 0x2000>, >> + <0x2000 0x2000>, >> + <0x10000 0x3000>; >> + reg-names = "dram0", "dram1", >> + "shrdram2"; >> + #address-cells = <1>; >> + #size-cells = <1>; >> + ranges; >> + >> + pruss_cfg: cfg@26000 { >> + compatible = "syscon"; >> + reg = <0x26000 0x2000>; >> + }; >> + >> + pruss_iep: iep@2e000 { >> + compatible = "syscon"; >> + reg = <0x2e000 0x31c>; >> + }; >> + >> + pruss_mii_rt: mii_rt@32000 { >> + compatible = "syscon"; >> + reg = <0x32000 0x58>; >> + }; >> + >> + pruss_intc: intc@20000 { >> + compatible = "ti,am3356-pruss-intc"; >> + reg = <0x20000 0x2000>; >> + reg-names = "intc"; >> + interrupt-controller; >> + #interrupt-cells = <1>; >> + interrupts = <20 21 22 23 24 25 26 27>; >> + interrupt-names = "host2", "host3", "host4", >> + "host5", "host6", "host7", >> + "host8", "host9"; >> + }; >> + >> + pru0: pru@34000 { >> + compatible = "ti,am3356-pru"; >> + reg = <0x34000 0x2000>, >> + <0x22000 0x400>, >> + <0x22400 0x100>; >> + reg-names = "iram", "control", "debug"; >> + gpcfg = <&pruss_cfg 0x8>; >> + firmware-name = "am335x-pru0-fw"; >> + interrupt-parent = <&pruss_intc>; >> + interrupts = <16>, <17>; >> + interrupt-names = "vring", "kick"; >> + }; >> + >> + pru1: pru@38000 { >> + compatible = "ti,am3356-pru"; >> + reg = <0x38000 0x2000>, >> + <0x24000 0x400>, >> + <0x24400 0x100>; >> + reg-names = "iram", "control", "debug"; >> + gpcfg = <&pruss_cfg 0xc>; >> + firmware-name = "am335x-pru1-fw"; >> + interrupt-parent = <&pruss_intc>; >> + interrupts = <18>, <19>; >> + interrupt-names = "vring", "kick"; >> + }; >> + >> + pruss_mdio: mdio@32400 { >> + compatible = "ti,davinci_mdio"; >> + reg = <0x32400 0x90>; >> + clocks = <&dpll_core_m4_ck>; >> + clock-names = "fck"; >> + bus_freq = <1000000>; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "disabled"; >> + }; >> + }; >> + >> +2: /* PRU application node example */ >> + app_node: app_node { >> + prus = <&pru0>, <&pru1>; >> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >> + ti,pruss-gp-mux-sel = <2>, <1>; >> + /* setup interrupts for prus: >> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >> + } >> >
On 14/02/19 14:52, Marc Zyngier wrote: > On Thu, 14 Feb 2019 10:55:10 +0000, > Roger Quadros <rogerq@ti.com> wrote: >> >> >> On 14/02/19 10:37, Linus Walleij wrote: >>> On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >>>> [Me] >>> >>>>> To be able to use hierarchical interrupt domain in the kernel, the top >>>>> interrupt controller must use the hierarchical (v2) irqdomain, so >>>>> if this is anything else than the ARM GIC it will be an interesting >>>>> undertaking to handle this. >>>> >>>> These are interrupt lines coming towards the host processor running >>>> Linux and are directly connected to the ARM GIC. This INTC module is >>>> actually an PRUSS internal interrupt controller that can take in 64 (on >>>> most SoCs) external events/interrupt sources and multiplexing them >>>> through two layers of many-to-one events-to-intr channels & >>>> intr-channels-to-host interrupts. Couple of the host interrupts go to >>>> the PRU cores themselves while the remaining ones come out of the IP to >>>> connect to other GICs in the SoC. >>> >>> If the muxing is static (like set up once at probe) so that while >>> the system is running, there is one and one only event mapped to >>> the GIC from the component below it, then it is hierarchical. >> >> This is how it looks. >> >> [GIC]<---8---[INTC]<---64---[events from peripherals] >> >> The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed >> per SoC. The muxing between 64 inputs to INTC and its 8 outputs are >> programmable and might not necessarily be static per boot/probe as >> it depends on what firmware is loaded on the PRU. > > But the point is that at any given time, there are at most 8 out of 64 > inputs that are used, right? You *never* end-up with two (or more) of > these "events" being multiplexed on a single output line. > Since the INTC's internal logic allows assigning more than one event each outputs, at most all 64 events can be assigned to one output or distributed among the 8 outputs. > If these assertions do hold, then your design is typical of a > hierarchy, for which we have countless examples in the tree (including > for some TI HW). OK. Suman, Andrew, Lokesh, thoughts?
fixed DTML id. On 14/02/19 17:44, Roger Quadros wrote: > On 14/02/19 14:52, Marc Zyngier wrote: >> On Thu, 14 Feb 2019 10:55:10 +0000, >> Roger Quadros <rogerq@ti.com> wrote: >>> >>> >>> On 14/02/19 10:37, Linus Walleij wrote: >>>> On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >>>>> [Me] >>>> >>>>>> To be able to use hierarchical interrupt domain in the kernel, the top >>>>>> interrupt controller must use the hierarchical (v2) irqdomain, so >>>>>> if this is anything else than the ARM GIC it will be an interesting >>>>>> undertaking to handle this. >>>>> >>>>> These are interrupt lines coming towards the host processor running >>>>> Linux and are directly connected to the ARM GIC. This INTC module is >>>>> actually an PRUSS internal interrupt controller that can take in 64 (on >>>>> most SoCs) external events/interrupt sources and multiplexing them >>>>> through two layers of many-to-one events-to-intr channels & >>>>> intr-channels-to-host interrupts. Couple of the host interrupts go to >>>>> the PRU cores themselves while the remaining ones come out of the IP to >>>>> connect to other GICs in the SoC. >>>> >>>> If the muxing is static (like set up once at probe) so that while >>>> the system is running, there is one and one only event mapped to >>>> the GIC from the component below it, then it is hierarchical. >>> >>> This is how it looks. >>> >>> [GIC]<---8---[INTC]<---64---[events from peripherals] >>> >>> The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed >>> per SoC. The muxing between 64 inputs to INTC and its 8 outputs are >>> programmable and might not necessarily be static per boot/probe as >>> it depends on what firmware is loaded on the PRU. >> >> But the point is that at any given time, there are at most 8 out of 64 >> inputs that are used, right? You *never* end-up with two (or more) of >> these "events" being multiplexed on a single output line. >> > > Since the INTC's internal logic allows assigning more than one event each outputs, > at most all 64 events can be assigned to one output or distributed among the 8 outputs. > >> If these assertions do hold, then your design is typical of a >> hierarchy, for which we have countless examples in the tree (including >> for some TI HW). > > OK. > Suman, Andrew, Lokesh, thoughts? >
On 14/02/2019 15:44, Roger Quadros wrote: > On 14/02/19 14:52, Marc Zyngier wrote: >> On Thu, 14 Feb 2019 10:55:10 +0000, >> Roger Quadros <rogerq@ti.com> wrote: >>> >>> >>> On 14/02/19 10:37, Linus Walleij wrote: >>>> On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >>>>> [Me] >>>> >>>>>> To be able to use hierarchical interrupt domain in the kernel, the top >>>>>> interrupt controller must use the hierarchical (v2) irqdomain, so >>>>>> if this is anything else than the ARM GIC it will be an interesting >>>>>> undertaking to handle this. >>>>> >>>>> These are interrupt lines coming towards the host processor running >>>>> Linux and are directly connected to the ARM GIC. This INTC module is >>>>> actually an PRUSS internal interrupt controller that can take in 64 (on >>>>> most SoCs) external events/interrupt sources and multiplexing them >>>>> through two layers of many-to-one events-to-intr channels & >>>>> intr-channels-to-host interrupts. Couple of the host interrupts go to >>>>> the PRU cores themselves while the remaining ones come out of the IP to >>>>> connect to other GICs in the SoC. >>>> >>>> If the muxing is static (like set up once at probe) so that while >>>> the system is running, there is one and one only event mapped to >>>> the GIC from the component below it, then it is hierarchical. >>> >>> This is how it looks. >>> >>> [GIC]<---8---[INTC]<---64---[events from peripherals] >>> >>> The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed >>> per SoC. The muxing between 64 inputs to INTC and its 8 outputs are >>> programmable and might not necessarily be static per boot/probe as >>> it depends on what firmware is loaded on the PRU. >> >> But the point is that at any given time, there are at most 8 out of 64 >> inputs that are used, right? You *never* end-up with two (or more) of >> these "events" being multiplexed on a single output line. >> > > Since the INTC's internal logic allows assigning more than one event each outputs, > at most all 64 events can be assigned to one output or distributed among the 8 outputs. OK. Do you get individual masking and status bits for each input? Thanks, M.
* Roger Quadros <rogerq@ti.com> [190214 11:09]: > Suman is mainly concerned about the following changes in v2 > 1) pruss node does not contain reg property representing entire ICSS. > 2) pruss node does not contain interrupts. > > Both of these are required if drivers/uio/uio_pruss.c or in future if > VFIO is to be used. > > The beagleboard community is a primary user of this driver and we need to > find a solution so that PRUSS is usable either via remoteproc or via UIO. > > Ideal case should allow user to use either of the drivers by just doing > a unbind and bind. > > I don't have a better idea than having a encapsulating node that has > the appropriate reg and interrupt properties. If there are existing use cases that need to be supported you should list them as non-standard usage in the binding and not recommended for future use. Rob may have some comments on how to deal with this. Then you can have device driver that needs to pass them parse them from the PRUSS parent node. That does not mean there needs to be a top level device driver for PRUSS, the child control module can just parse the non-standard bindings for compability from the parent node. And naturally in addition to handling the non-standard binding we need to have a proper standardized binding too :) Regards, Tony
On 14/02/19 17:51, Marc Zyngier wrote: > On 14/02/2019 15:44, Roger Quadros wrote: >> On 14/02/19 14:52, Marc Zyngier wrote: >>> On Thu, 14 Feb 2019 10:55:10 +0000, >>> Roger Quadros <rogerq@ti.com> wrote: >>>> >>>> >>>> On 14/02/19 10:37, Linus Walleij wrote: >>>>> On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >>>>>> [Me] >>>>> >>>>>>> To be able to use hierarchical interrupt domain in the kernel, the top >>>>>>> interrupt controller must use the hierarchical (v2) irqdomain, so >>>>>>> if this is anything else than the ARM GIC it will be an interesting >>>>>>> undertaking to handle this. >>>>>> >>>>>> These are interrupt lines coming towards the host processor running >>>>>> Linux and are directly connected to the ARM GIC. This INTC module is >>>>>> actually an PRUSS internal interrupt controller that can take in 64 (on >>>>>> most SoCs) external events/interrupt sources and multiplexing them >>>>>> through two layers of many-to-one events-to-intr channels & >>>>>> intr-channels-to-host interrupts. Couple of the host interrupts go to >>>>>> the PRU cores themselves while the remaining ones come out of the IP to >>>>>> connect to other GICs in the SoC. >>>>> >>>>> If the muxing is static (like set up once at probe) so that while >>>>> the system is running, there is one and one only event mapped to >>>>> the GIC from the component below it, then it is hierarchical. >>>> >>>> This is how it looks. >>>> >>>> [GIC]<---8---[INTC]<---64---[events from peripherals] >>>> >>>> The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed >>>> per SoC. The muxing between 64 inputs to INTC and its 8 outputs are >>>> programmable and might not necessarily be static per boot/probe as >>>> it depends on what firmware is loaded on the PRU. >>> >>> But the point is that at any given time, there are at most 8 out of 64 >>> inputs that are used, right? You *never* end-up with two (or more) of >>> these "events" being multiplexed on a single output line. >>> >> >> Since the INTC's internal logic allows assigning more than one event each outputs, >> at most all 64 events can be assigned to one output or distributed among the 8 outputs. > > OK. Do you get individual masking and status bits for each input? Yes, we have individual enable/disable and status bits for each of the 64 events. In addition to that it is possible to determine priority if multiple events come to the same output by reading a register specific to that output. -- cheers, -roger Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 2/14/19 9:48 AM, Roger Quadros wrote: > fixed DTML id. > > On 14/02/19 17:44, Roger Quadros wrote: >> On 14/02/19 14:52, Marc Zyngier wrote: >>> On Thu, 14 Feb 2019 10:55:10 +0000, >>> Roger Quadros <rogerq@ti.com> wrote: >>>> >>>> >>>> On 14/02/19 10:37, Linus Walleij wrote: >>>>> On Thu, Feb 14, 2019 at 4:13 AM Suman Anna <s-anna@ti.com> wrote: >>>>>> [Me] >>>>> >>>>>>> To be able to use hierarchical interrupt domain in the kernel, the top >>>>>>> interrupt controller must use the hierarchical (v2) irqdomain, so >>>>>>> if this is anything else than the ARM GIC it will be an interesting >>>>>>> undertaking to handle this. >>>>>> >>>>>> These are interrupt lines coming towards the host processor running >>>>>> Linux and are directly connected to the ARM GIC. This INTC module is >>>>>> actually an PRUSS internal interrupt controller that can take in 64 (on >>>>>> most SoCs) external events/interrupt sources and multiplexing them >>>>>> through two layers of many-to-one events-to-intr channels & >>>>>> intr-channels-to-host interrupts. Couple of the host interrupts go to >>>>>> the PRU cores themselves while the remaining ones come out of the IP to >>>>>> connect to other GICs in the SoC. >>>>> >>>>> If the muxing is static (like set up once at probe) so that while >>>>> the system is running, there is one and one only event mapped to >>>>> the GIC from the component below it, then it is hierarchical. >>>> >>>> This is how it looks. >>>> >>>> [GIC]<---8---[INTC]<---64---[events from peripherals] >>>> >>>> The 8 interrupt lines from INTC to the GIC are 1:1 mapped and fixed >>>> per SoC. The muxing between 64 inputs to INTC and its 8 outputs are >>>> programmable and might not necessarily be static per boot/probe as >>>> it depends on what firmware is loaded on the PRU. >>> >>> But the point is that at any given time, there are at most 8 out of 64 >>> inputs that are used, right? You *never* end-up with two (or more) of >>> these "events" being multiplexed on a single output line. >>> >> >> Since the INTC's internal logic allows assigning more than one event each outputs, >> at most all 64 events can be assigned to one output or distributed among the 8 outputs. >> >>> If these assertions do hold, then your design is typical of a >>> hierarchy, for which we have countless examples in the tree (including >>> for some TI HW). >> >> OK. >> Suman, Andrew, Lokesh, thoughts? >> Mark, Linus, So, I hope it is clear from Roger's responses that above assertions do not hold true to this INTC, and so want to confirm that we are good with the current non-hierarchical design. regards Suman
Hi Roger, On 2/14/19 5:08 AM, Roger Quadros wrote: > +Robert, Dan, Matthijs > > Hi Suman, > > On 14/02/19 04:52, Suman Anna wrote: >> Hi Roger, >> >> On 2/4/19 8:22 AM, Roger Quadros wrote: >>> From: Suman Anna <s-anna@ti.com> >>> >>> This patch adds the bindings for the Programmable Real-Time Unit >>> and Industrial Communication Subsystem (PRU-ICSS) present on various >>> SoCs such as AM33xx, AM437x, AM57xx, Keystone 66AK2G SoC, etc. It is >>> present on the Davinci based OMAPL138 SoCs and K3 architecture >>> based AM65x SoCs as well (not covered for now). >>> >>> Signed-off-by: Suman Anna <s-anna@ti.com> >>> Signed-off-by: Roger Quadros <rogerq@ti.com> >>> --- >>> .../devicetree/bindings/soc/ti/ti,pruss.txt | 212 +++++++++++++++++++++ >>> 1 file changed, 212 insertions(+) >>> create mode 100644 Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >>> >>> diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >>> new file mode 100644 >>> index 0000000..5ac76fd >>> --- /dev/null >>> +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt >>> @@ -0,0 +1,212 @@ >>> +PRU-ICSS on TI SoCs >>> +=================== >>> + >>> +The Programmable Real-Time Unit and Industrial Communication Subsystem >>> +(PRU-ICSS) is present on various TI SoCs such as AM335x, AM437x, Keystone >>> +66AK2G, etc. A PRUSS consists of dual 32-bit RISC cores (Programmable >>> +Real-Time Units, or PRUs) with program memory and data memory. >>> + >>> +The programmable nature of the PRUs provide flexibility to implement >>> +custom peripheral interfaces, fast real-time responses, or specialized >>> +data handling. The common peripheral modules include the following, >>> + >>> + - Enhanced GPIO with async capture and serial support >>> + - an Ethernet MII_RT module with two MII ports >>> + - an MDIO port to control external Ethernet PHYs >>> + - an Industrial Ethernet Peripheral (IEP) to manage/generate Industrial >>> + Ethernet functions >>> + - an Enhanced Capture Module (eCAP) >>> + - a 16550-compatible UART to support PROFIBUS >>> + - Interrupt controller with 64 input events and 10 Host interrupts. >>> + >>> +A shared Data RAM, if present, can be accessed by both the PRU cores. The >>> +Interrupt Controller (INTC) and a CFG module are common to both the PRU >>> +cores. >>> + >>> +Various sub-modules within a PRU-ICSS subsystem are represented as individual >>> +nodes. >>> + >>> +PRUSS Node >>> +============= >>> + >>> +This node represents the entire ICSS instance and the various modules are >>> +contained as children. The PRUSS driver is responsible for managing the >>> +common resources i.e. DRAM0, DRAM1, SHARED_RAM and CFG space. >>> + >>> +Required Properties: >>> +-------------------- >>> +- compatible : should be one of, >>> + "ti,am3356-pruss" for AM335x family of SoCs >>> + "ti,am4376-pruss" for AM437x family of SoCs >>> + "ti,am5728-pruss" for AM57xx family of SoCs >>> + "ti,k2g-pruss" for 66AK2G family of SoCs >>> +- reg : base address and size for each of the Data RAMs as >>> + mentioned in reg-names, and in the same order as the >>> + reg-names >> >> Hmm, not sure what prompted you to deviate from the design we had on TI >> SDK kernels. Your revised bindings does not allow us to use this device >> for the scalability with either UIO/VFIO. > > My understanding was that the device tree node will have to be modified > in any case to support the current uio_pruss.c driver so we don't > need to add things that we don't need right away. Well, the previous design allows it to scale (as in add new properties if needed to satisfy the UIO case) maintaining compatibility, but your current binding does not even allow that. The modification you are saying that can be done in the future cannot be done without breaking the binding ABI with your current design. > >> >> Let's sort this out before you post the next series. > > Let's discuss this here so I don't have to explain the whole thing again > in the next series. Yeah, thanks for the below summary. regards Suman > > Tony, > > Suman is mainly concerned about the following changes in v2 > 1) pruss node does not contain reg property representing entire ICSS. > 2) pruss node does not contain interrupts. > > Both of these are required if drivers/uio/uio_pruss.c or in future if > VFIO is to be used. > > The beagleboard community is a primary user of this driver and we need to > find a solution so that PRUSS is usable either via remoteproc or via UIO. > > Ideal case should allow user to use either of the drivers by just doing > a unbind and bind. > > I don't have a better idea than having a encapsulating node that has > the appropriate reg and interrupt properties. > > cheers, > -roger > >> >> regards >> Suman >> >>> +- reg-names : should contain a string(s) from among the following names, >>> + each representing a specific Data RAM region. Some PRU-ICSS >>> + instances on certain SoCs might not have Shared DRAM. >>> + "dram0" for Data RAM0, >>> + "dram1" for Data RAM1, >>> + "shrdram2" for Shared Data RAM, >>> +- #address-cells : should be 1 >>> +- #size-cells : should be 1 >>> +- ranges : no specific range translations required, child nodes have the >>> + same address view as the parent, so should be mentioned without >>> + any value for the property >>> + >>> +Optional Properties: >>> +-------------------- >>> +- no-shared-ram : Should be present if the instance doesn't have Shared RAM. >>> + e.g. AM4376 ICSS0 instance doesn't have Shared RAM. >>> + >>> +The PRUSS node will have one or more of the folowing child nodes. >>> + >>> +PRU CORES >>> +========= >>> +ICSS typically has 2 PRU cores. These should be represented as remoteproc devices. >>> + >>> +INTC node >>> +========= >>> +ICSS has one INTC interrupt controller module. This should be represented as >>> +a standard interrupt-controller node. >>> + >>> +CFG, IEP, MII_RT >>> +================ >>> +The individual sub-modules CFG, IEP and MII_RT are represented as a syscon >>> +node each with specific node names as below: >>> + "cfg" for CFG sub-module, >>> + "iep" for IEP sub-module, >>> + "mii_rt" for MII-RT sub-module, >>> + >>> +See Documentation/devicetree/bindings/mfd/syscon.txt for details. >>> + >>> +MDIO >>> +==== >>> +Each PRUSS has an MDIO module that can be used to control external PHYs. The >>> +MDIO module used within the PRU-ICSS is an instance of the MDIO Controller >>> +used in TI Davinci SoCs. Please refer to the corresponding binding document, >>> +Documentation/devicetree/bindings/net/davinci-mdio.txt for details. >>> + >>> +Application/User Nodes >>> +======================= >>> +A PRU application/user node typically uses one or more PRU device nodes to >>> +implement a PRU application/functionality. Each application/client node would >>> +need a reference to at least a PRU node, and optionally pass some configuration >>> +parameters. >>> + >>> +Required Properties: >>> +-------------------- >>> +- prus : phandles to the PRU nodes used >>> + >>> +Optional Properties: >>> +-------------------- >>> +- firmware-name : firmwares for the PRU cores, the default firmware >>> + for the core from the PRU node will be used if not >>> + provided. The firmware names should correspond to >>> + the PRU cores listed in the 'prus' property >>> +- ti,pruss-gp-mux-sel : array of values for the GP_MUX_SEL under PRUSS_GPCFG >>> + register for a PRU. This selects the internal muxing >>> + scheme for the PRU instance. If not provided, the >>> + default out-of-reset value (0) for the PRU core is >>> + used. Values should correspond to the PRU cores listed >>> + in the 'prus' property >>> +- ti,pru-interrupt-map : PRU interrupt mappings, containing an array of entries >>> + with each entry consisting of 4 cell-values. First one >>> + is an index towards the "prus" property to identify the >>> + PRU core for the interrupt map, second is the PRU >>> + System Event id, third is the PRU interrupt channel id >>> + and fourth is the PRU host interrupt id. If provided, >>> + this map will supercede any other configuration >>> + provided through firmware >>> + >>> +Example: >>> +======== >>> +1. /* AM33xx PRU-ICSS */ >>> + >>> + pruss: pruss@0 { >>> + compatible = "ti,am3356-pruss"; >>> + reg = <0x0 0x2000>, >>> + <0x2000 0x2000>, >>> + <0x10000 0x3000>; >>> + reg-names = "dram0", "dram1", >>> + "shrdram2"; >>> + #address-cells = <1>; >>> + #size-cells = <1>; >>> + ranges; >>> + >>> + pruss_cfg: cfg@26000 { >>> + compatible = "syscon"; >>> + reg = <0x26000 0x2000>; >>> + }; >>> + >>> + pruss_iep: iep@2e000 { >>> + compatible = "syscon"; >>> + reg = <0x2e000 0x31c>; >>> + }; >>> + >>> + pruss_mii_rt: mii_rt@32000 { >>> + compatible = "syscon"; >>> + reg = <0x32000 0x58>; >>> + }; >>> + >>> + pruss_intc: intc@20000 { >>> + compatible = "ti,am3356-pruss-intc"; >>> + reg = <0x20000 0x2000>; >>> + reg-names = "intc"; >>> + interrupt-controller; >>> + #interrupt-cells = <1>; >>> + interrupts = <20 21 22 23 24 25 26 27>; >>> + interrupt-names = "host2", "host3", "host4", >>> + "host5", "host6", "host7", >>> + "host8", "host9"; >>> + }; >>> + >>> + pru0: pru@34000 { >>> + compatible = "ti,am3356-pru"; >>> + reg = <0x34000 0x2000>, >>> + <0x22000 0x400>, >>> + <0x22400 0x100>; >>> + reg-names = "iram", "control", "debug"; >>> + gpcfg = <&pruss_cfg 0x8>; >>> + firmware-name = "am335x-pru0-fw"; >>> + interrupt-parent = <&pruss_intc>; >>> + interrupts = <16>, <17>; >>> + interrupt-names = "vring", "kick"; >>> + }; >>> + >>> + pru1: pru@38000 { >>> + compatible = "ti,am3356-pru"; >>> + reg = <0x38000 0x2000>, >>> + <0x24000 0x400>, >>> + <0x24400 0x100>; >>> + reg-names = "iram", "control", "debug"; >>> + gpcfg = <&pruss_cfg 0xc>; >>> + firmware-name = "am335x-pru1-fw"; >>> + interrupt-parent = <&pruss_intc>; >>> + interrupts = <18>, <19>; >>> + interrupt-names = "vring", "kick"; >>> + }; >>> + >>> + pruss_mdio: mdio@32400 { >>> + compatible = "ti,davinci_mdio"; >>> + reg = <0x32400 0x90>; >>> + clocks = <&dpll_core_m4_ck>; >>> + clock-names = "fck"; >>> + bus_freq = <1000000>; >>> + #address-cells = <1>; >>> + #size-cells = <0>; >>> + status = "disabled"; >>> + }; >>> + }; >>> + >>> +2: /* PRU application node example */ >>> + app_node: app_node { >>> + prus = <&pru0>, <&pru1>; >>> + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; >>> + ti,pruss-gp-mux-sel = <2>, <1>; >>> + /* setup interrupts for prus: >>> + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, >>> + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ >>> + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; >>> + } >>> >> >
Hi Tony, On 2/14/19 9:56 AM, Tony Lindgren wrote: > * Roger Quadros <rogerq@ti.com> [190214 11:09]: >> Suman is mainly concerned about the following changes in v2 >> 1) pruss node does not contain reg property representing entire ICSS. >> 2) pruss node does not contain interrupts. >> >> Both of these are required if drivers/uio/uio_pruss.c or in future if >> VFIO is to be used. >> >> The beagleboard community is a primary user of this driver and we need to >> find a solution so that PRUSS is usable either via remoteproc or via UIO. >> >> Ideal case should allow user to use either of the drivers by just doing >> a unbind and bind. >> >> I don't have a better idea than having a encapsulating node that has >> the appropriate reg and interrupt properties. > > If there are existing use cases that need to be supported > you should list them as non-standard usage in the binding > and not recommended for future use. Rob may have some > comments on how to deal with this. > > Then you can have device driver that needs to pass them > parse them from the PRUSS parent node. That does not mean > there needs to be a top level device driver for PRUSS, > the child control module can just parse the non-standard > bindings for compability from the parent node. The PRUSS SoC bus driver was handling all possible architectures (OMAP, K2 and K3) which have different clocking and reset integration, and also catering to the UIO vs remoteproc usecases, by taking care of clocks and resets. I am ok to replace this layer with the ti-sysc layer on OMAP SoCs since most of the functionality added to the driver is associated with OCP, but we would still need a PRUSS driver. Not all sub-modules are peripherals and managed by respective peripheral drivers, and we still need a central entity managing the sub-system wide resources. Layering wise - it is similar if we would have done a device for the PRUSS local interconnect, but that driver wouldn't have much to do with interconnect functionality. K2 and K3 families uses TI-SCI and so you don't have a similar target-module concept that allows you to query the PRUSS parent node for PRUSS specific ranges or properties. In anycase, I don't think these drivers should depend on a parent interconnect driver. regards Suman > > And naturally in addition to handling the non-standard > binding we need to have a proper standardized binding > too :) > > Regards, > > Tony >
On Thu, 14 Feb 2019 at 12:08, Roger Quadros <rogerq@ti.com> wrote: > The beagleboard community is a primary user of this driver and we need to > find a solution so that PRUSS is usable either via remoteproc or via UIO. While being able to switch drivers without changing the DT by forcibly binding a different driver would definitely be a nice feature, and would have been possible with the older remoteproc-pru bindings, I think it's a stretch to say this "needs" a solution. Right now, in practice, selection between uio-pruss and remoteproc-pru is done simply by modifying the device tree appropriately (typically by having u-boot apply an overlay to the DT), and I don't think anyone views this as unduly burdensome? Matthijs
On Fri, Feb 15, 2019 at 2:00 AM Suman Anna <s-anna@ti.com> wrote: > Mark, Linus, > > So, I hope it is clear from Roger's responses that above assertions do > not hold true to this INTC, and so want to confirm that we are good with > the current non-hierarchical design. IIUC the 64 lines are latched onto 8 lines, but all 64 lines have individual masking and ACKing bits and can all be used at the same time, so yes that is cascading and then you should indeed use the chained (or nested) IRQ handler. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt new file mode 100644 index 0000000..5ac76fd --- /dev/null +++ b/Documentation/devicetree/bindings/soc/ti/ti,pruss.txt @@ -0,0 +1,212 @@ +PRU-ICSS on TI SoCs +=================== + +The Programmable Real-Time Unit and Industrial Communication Subsystem +(PRU-ICSS) is present on various TI SoCs such as AM335x, AM437x, Keystone +66AK2G, etc. A PRUSS consists of dual 32-bit RISC cores (Programmable +Real-Time Units, or PRUs) with program memory and data memory. + +The programmable nature of the PRUs provide flexibility to implement +custom peripheral interfaces, fast real-time responses, or specialized +data handling. The common peripheral modules include the following, + + - Enhanced GPIO with async capture and serial support + - an Ethernet MII_RT module with two MII ports + - an MDIO port to control external Ethernet PHYs + - an Industrial Ethernet Peripheral (IEP) to manage/generate Industrial + Ethernet functions + - an Enhanced Capture Module (eCAP) + - a 16550-compatible UART to support PROFIBUS + - Interrupt controller with 64 input events and 10 Host interrupts. + +A shared Data RAM, if present, can be accessed by both the PRU cores. The +Interrupt Controller (INTC) and a CFG module are common to both the PRU +cores. + +Various sub-modules within a PRU-ICSS subsystem are represented as individual +nodes. + +PRUSS Node +============= + +This node represents the entire ICSS instance and the various modules are +contained as children. The PRUSS driver is responsible for managing the +common resources i.e. DRAM0, DRAM1, SHARED_RAM and CFG space. + +Required Properties: +-------------------- +- compatible : should be one of, + "ti,am3356-pruss" for AM335x family of SoCs + "ti,am4376-pruss" for AM437x family of SoCs + "ti,am5728-pruss" for AM57xx family of SoCs + "ti,k2g-pruss" for 66AK2G family of SoCs +- reg : base address and size for each of the Data RAMs as + mentioned in reg-names, and in the same order as the + reg-names +- reg-names : should contain a string(s) from among the following names, + each representing a specific Data RAM region. Some PRU-ICSS + instances on certain SoCs might not have Shared DRAM. + "dram0" for Data RAM0, + "dram1" for Data RAM1, + "shrdram2" for Shared Data RAM, +- #address-cells : should be 1 +- #size-cells : should be 1 +- ranges : no specific range translations required, child nodes have the + same address view as the parent, so should be mentioned without + any value for the property + +Optional Properties: +-------------------- +- no-shared-ram : Should be present if the instance doesn't have Shared RAM. + e.g. AM4376 ICSS0 instance doesn't have Shared RAM. + +The PRUSS node will have one or more of the folowing child nodes. + +PRU CORES +========= +ICSS typically has 2 PRU cores. These should be represented as remoteproc devices. + +INTC node +========= +ICSS has one INTC interrupt controller module. This should be represented as +a standard interrupt-controller node. + +CFG, IEP, MII_RT +================ +The individual sub-modules CFG, IEP and MII_RT are represented as a syscon +node each with specific node names as below: + "cfg" for CFG sub-module, + "iep" for IEP sub-module, + "mii_rt" for MII-RT sub-module, + +See Documentation/devicetree/bindings/mfd/syscon.txt for details. + +MDIO +==== +Each PRUSS has an MDIO module that can be used to control external PHYs. The +MDIO module used within the PRU-ICSS is an instance of the MDIO Controller +used in TI Davinci SoCs. Please refer to the corresponding binding document, +Documentation/devicetree/bindings/net/davinci-mdio.txt for details. + +Application/User Nodes +======================= +A PRU application/user node typically uses one or more PRU device nodes to +implement a PRU application/functionality. Each application/client node would +need a reference to at least a PRU node, and optionally pass some configuration +parameters. + +Required Properties: +-------------------- +- prus : phandles to the PRU nodes used + +Optional Properties: +-------------------- +- firmware-name : firmwares for the PRU cores, the default firmware + for the core from the PRU node will be used if not + provided. The firmware names should correspond to + the PRU cores listed in the 'prus' property +- ti,pruss-gp-mux-sel : array of values for the GP_MUX_SEL under PRUSS_GPCFG + register for a PRU. This selects the internal muxing + scheme for the PRU instance. If not provided, the + default out-of-reset value (0) for the PRU core is + used. Values should correspond to the PRU cores listed + in the 'prus' property +- ti,pru-interrupt-map : PRU interrupt mappings, containing an array of entries + with each entry consisting of 4 cell-values. First one + is an index towards the "prus" property to identify the + PRU core for the interrupt map, second is the PRU + System Event id, third is the PRU interrupt channel id + and fourth is the PRU host interrupt id. If provided, + this map will supercede any other configuration + provided through firmware + +Example: +======== +1. /* AM33xx PRU-ICSS */ + + pruss: pruss@0 { + compatible = "ti,am3356-pruss"; + reg = <0x0 0x2000>, + <0x2000 0x2000>, + <0x10000 0x3000>; + reg-names = "dram0", "dram1", + "shrdram2"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + pruss_cfg: cfg@26000 { + compatible = "syscon"; + reg = <0x26000 0x2000>; + }; + + pruss_iep: iep@2e000 { + compatible = "syscon"; + reg = <0x2e000 0x31c>; + }; + + pruss_mii_rt: mii_rt@32000 { + compatible = "syscon"; + reg = <0x32000 0x58>; + }; + + pruss_intc: intc@20000 { + compatible = "ti,am3356-pruss-intc"; + reg = <0x20000 0x2000>; + reg-names = "intc"; + interrupt-controller; + #interrupt-cells = <1>; + interrupts = <20 21 22 23 24 25 26 27>; + interrupt-names = "host2", "host3", "host4", + "host5", "host6", "host7", + "host8", "host9"; + }; + + pru0: pru@34000 { + compatible = "ti,am3356-pru"; + reg = <0x34000 0x2000>, + <0x22000 0x400>, + <0x22400 0x100>; + reg-names = "iram", "control", "debug"; + gpcfg = <&pruss_cfg 0x8>; + firmware-name = "am335x-pru0-fw"; + interrupt-parent = <&pruss_intc>; + interrupts = <16>, <17>; + interrupt-names = "vring", "kick"; + }; + + pru1: pru@38000 { + compatible = "ti,am3356-pru"; + reg = <0x38000 0x2000>, + <0x24000 0x400>, + <0x24400 0x100>; + reg-names = "iram", "control", "debug"; + gpcfg = <&pruss_cfg 0xc>; + firmware-name = "am335x-pru1-fw"; + interrupt-parent = <&pruss_intc>; + interrupts = <18>, <19>; + interrupt-names = "vring", "kick"; + }; + + pruss_mdio: mdio@32400 { + compatible = "ti,davinci_mdio"; + reg = <0x32400 0x90>; + clocks = <&dpll_core_m4_ck>; + clock-names = "fck"; + bus_freq = <1000000>; + #address-cells = <1>; + #size-cells = <0>; + status = "disabled"; + }; + }; + +2: /* PRU application node example */ + app_node: app_node { + prus = <&pru0>, <&pru1>; + firmware-name = "pruss-app-fw", "pruss-app-fw-2"; + ti,pruss-gp-mux-sel = <2>, <1>; + /* setup interrupts for prus: + prus[0] => pru1_0: ev=16, chnl=2, host-irq=7, + prus[1] => pru1_1: ev=19, chnl=1, host-irq=3 */ + ti,pru-interrupt-map = <0 16 2 7 >, <1 19 1 3>; + }