| Message ID | 1406661128-7614-4-git-send-email-heiko@sntech.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Heiko, On Tue, Jul 29, 2014 at 12:12 PM, Heiko Stuebner <heiko@sntech.de> wrote: > Add both the bus and peripheral pl330 dma controllers present in rk3288 socs. > The first dma controller can change between secure and non-secure mode and is > left by the bootloader in secure mode, which gets added here. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > arch/arm/boot/dts/rk3288.dtsi | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) This is outside of my area of expertise, but comparing this to bindings, TRM, and other platforms it looks reasonable to me. > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi > index 3ef8951..3f39d26 100644 > --- a/arch/arm/boot/dts/rk3288.dtsi > +++ b/arch/arm/boot/dts/rk3288.dtsi > @@ -62,6 +62,33 @@ > }; > }; > > + amba { > + compatible = "arm,amba-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + dmac1: dma@ffb20000 { I'm curious: why did you choose "dmac1" for the alias here? I would have called it "dmac_peri", "pdma", or something similar as per the address map in the TRM. I found a single reference in the TRM to "dmac2" but it wasn't immediately clear to me which DMA that was referring to. In a commit in a temporary tree I see that someone at Rockchip called the DMA at ff250000 "pdma1". > + compatible = "arm,pl330", "arm,primecell"; > + reg = <0xffb20000 0x4000>; > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + clocks = <&cru ACLK_DMAC1>; > + clock-names = "apb_pclk"; > + }; > + > + dmac2: dma@ff250000 { nit: when we have easy addresses to compare to, we should probably sort by address? > + compatible = "arm,pl330", "arm,primecell"; > + reg = <0xff250000 0x4000>; > + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + clocks = <&cru ACLK_DMAC2>; > + clock-names = "apb_pclk"; > + }; > + }; > + > xin24m: oscillator { > compatible = "fixed-clock"; > clock-frequency = <24000000>; > -- > 2.0.1 >
Am Montag, 11. August 2014, 10:01:08 schrieb Doug Anderson: > Heiko, > > On Tue, Jul 29, 2014 at 12:12 PM, Heiko Stuebner <heiko@sntech.de> wrote: > > Add both the bus and peripheral pl330 dma controllers present in rk3288 > > socs. The first dma controller can change between secure and non-secure > > mode and is left by the bootloader in secure mode, which gets added here. > > > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > > --- > > > > arch/arm/boot/dts/rk3288.dtsi | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > This is outside of my area of expertise, but comparing this to > bindings, TRM, and other platforms it looks reasonable to me. > > > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi > > index 3ef8951..3f39d26 100644 > > --- a/arch/arm/boot/dts/rk3288.dtsi > > +++ b/arch/arm/boot/dts/rk3288.dtsi > > @@ -62,6 +62,33 @@ > > > > }; > > > > }; > > > > + amba { > > + compatible = "arm,amba-bus"; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + ranges; > > + > > + dmac1: dma@ffb20000 { > > I'm curious: why did you choose "dmac1" for the alias here? I would > have called it "dmac_peri", "pdma", or something similar as per the > address map in the TRM. I found a single reference in the TRM to > "dmac2" but it wasn't immediately clear to me which DMA that was > referring to. In a commit in a temporary tree I see that someone at > Rockchip called the DMA at ff250000 "pdma1". I think I just kept the alias similar to the clock-name. The naming of the dma-controllers seems to be inconsistent ... the clocks are named dmac1 and dmac2, while the interrupt table in the rk3188-TRM calls them dmac0 and dmac1. So I guess I just duplicated the naming for the rk3288. I guess for the rk3288, the one at 0xffb20000 should be named something like dmac_bus and the other one dmac_peri, reflecting what they're called in the TRM, but I'm of course open for suggestions :-) . Also for an upcoming v2, I've also changed the structure a bit, as the first dma-controller has both a secure and non-secure version of it. So currently the rk3288.dtsi looks like [0]: amba { compatible = "arm,amba-bus"; #address-cells = <1>; #size-cells = <1>; ranges; /* dma1 in secure state */ dma-controller@ffb20000 { compatible = "arm,pl330", "arm,primecell"; reg = <0xffb20000 0x4000>; interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; #dma-cells = <1>; clocks = <&cru ACLK_DMAC1>; clock-names = "apb_pclk"; status = "disabled"; }; /* dma1 in non-secure state */ dma-controller@ffb60000 { compatible = "arm,pl330", "arm,primecell"; reg = <0xffb60000 0x4000>; interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; #dma-cells = <1>; clocks = <&cru ACLK_DMAC1>; clock-names = "apb_pclk"; status = "disabled"; }; dmac2: dma-controller@ff250000 { compatible = "arm,pl330", "arm,primecell"; reg = <0xff250000 0x4000>; interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; #dma-cells = <1>; clocks = <&cru ACLK_DMAC2>; clock-names = "apb_pclk"; }; }; and the board is responsible for enabling the correct variant [1], as most likely the bootloader decides in which mode to start the dma controller: amba { /* dma1 in secure state */ dmac1: dma-controller@ffb20000 { status = "okay"; }; }; This is based on some mailinglist discussion, I found at some point, about this but for the life of me am not able to find anymore. So of course feedback would appreciated there too. [0] https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boot/dts/rk3288.dtsi [1] https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boot/dts/rk3288-evb.dtsi > > > + compatible = "arm,pl330", "arm,primecell"; > > + reg = <0xffb20000 0x4000>; > > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > > + #dma-cells = <1>; > > + clocks = <&cru ACLK_DMAC1>; > > + clock-names = "apb_pclk"; > > + }; > > + > > + dmac2: dma@ff250000 { > > nit: when we have easy addresses to compare to, we should probably > sort by address? correct :-) > > > + compatible = "arm,pl330", "arm,primecell"; > > + reg = <0xff250000 0x4000>; > > + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > > + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; > > + #dma-cells = <1>; > > + clocks = <&cru ACLK_DMAC2>; > > + clock-names = "apb_pclk"; > > + }; > > + }; > > + > > > > xin24m: oscillator { > > > > compatible = "fixed-clock"; > > clock-frequency = <24000000>; > > > > -- > > 2.0.1
Am 11.08.2014 19:01, schrieb Heiko Stübner: > Also for an upcoming v2, I've also changed the structure a bit, as the first > dma-controller has both a secure and non-secure version of it. > So currently the rk3288.dtsi looks like [0]: > > amba { > compatible = "arm,amba-bus"; > #address-cells = <1>; > #size-cells = <1>; > ranges; > > /* dma1 in secure state */ > dma-controller@ffb20000 { > compatible = "arm,pl330", "arm,primecell"; > reg = <0xffb20000 0x4000>; > interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > #dma-cells = <1>; > clocks = <&cru ACLK_DMAC1>; > clock-names = "apb_pclk"; > status = "disabled"; > }; > > /* dma1 in non-secure state */ > dma-controller@ffb60000 { > compatible = "arm,pl330", "arm,primecell"; > reg = <0xffb60000 0x4000>; > interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > #dma-cells = <1>; > clocks = <&cru ACLK_DMAC1>; > clock-names = "apb_pclk"; > status = "disabled"; > }; > > dmac2: dma-controller@ff250000 { > compatible = "arm,pl330", "arm,primecell"; > reg = <0xff250000 0x4000>; > interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; > #dma-cells = <1>; > clocks = <&cru ACLK_DMAC2>; > clock-names = "apb_pclk"; > }; > }; > > and the board is responsible for enabling the correct variant [1], as most > likely the bootloader decides in which mode to start the dma controller: > > amba { > /* dma1 in secure state */ > dmac1: dma-controller@ffb20000 { > status = "okay"; > }; > }; > > This is based on some mailinglist discussion, I found at some point, about > this but for the life of me am not able to find anymore. So of course feedback > would appreciated there too. I would suggest to give labels such as dmac1_s and dmac1_ns like you did for dmac2 in the former snippet, so that you can override the status in the latter without replicating the amba/dma-controller hierarchy. However, for the Zynq SoC we chose to just model the secure DMAC though [*]. I was told that either the bootloader or the user should change the DT when running in such a non-secure scenario. Cheers, Andreas [*] https://patchwork.kernel.org/patch/4620251/ > [0] https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boot/dts/rk3288.dtsi > [1] https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boot/dts/rk3288-evb.dtsi [snip]
Am Montag, 11. August 2014, 19:37:29 schrieb Andreas Färber: > Am 11.08.2014 19:01, schrieb Heiko Stübner: > > Also for an upcoming v2, I've also changed the structure a bit, as the > > first dma-controller has both a secure and non-secure version of it. > > > > So currently the rk3288.dtsi looks like [0]: > > amba { > > > > compatible = "arm,amba-bus"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges; > > > > /* dma1 in secure state */ > > dma-controller@ffb20000 { > > > > compatible = "arm,pl330", "arm,primecell"; > > reg = <0xffb20000 0x4000>; > > interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > > > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > > > > #dma-cells = <1>; > > clocks = <&cru ACLK_DMAC1>; > > clock-names = "apb_pclk"; > > status = "disabled"; > > > > }; > > > > /* dma1 in non-secure state */ > > dma-controller@ffb60000 { > > > > compatible = "arm,pl330", "arm,primecell"; > > reg = <0xffb60000 0x4000>; > > interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > > > > <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > > > > #dma-cells = <1>; > > clocks = <&cru ACLK_DMAC1>; > > clock-names = "apb_pclk"; > > status = "disabled"; > > > > }; > > > > dmac2: dma-controller@ff250000 { > > > > compatible = "arm,pl330", "arm,primecell"; > > reg = <0xff250000 0x4000>; > > interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > > > > <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; > > > > #dma-cells = <1>; > > clocks = <&cru ACLK_DMAC2>; > > clock-names = "apb_pclk"; > > > > }; > > > > }; > > > > and the board is responsible for enabling the correct variant [1], as most > > > > likely the bootloader decides in which mode to start the dma controller: > > amba { > > > > /* dma1 in secure state */ > > dmac1: dma-controller@ffb20000 { > > > > status = "okay"; > > > > }; > > > > }; > > > > This is based on some mailinglist discussion, I found at some point, about > > this but for the life of me am not able to find anymore. So of course > > feedback would appreciated there too. > > I would suggest to give labels such as dmac1_s and dmac1_ns like you did > for dmac2 in the former snippet, so that you can override the status in > the latter without replicating the amba/dma-controller hierarchy. I was hoping to just keep it to one handle, so that dma consumers would just work and not have to care which dma controller variant was available :-) . > However, for the Zynq SoC we chose to just model the secure DMAC though > [*]. I was told that either the bootloader or the user should change the > DT when running in such a non-secure scenario. ok, this sounds interesting too, as I haven't seen a board that used the dmac in question in non-secure mode, yet. So I guess one way to handle it could be to declare the non-secure dma, but set it to a disabled state and use the secure one as default for everything: dmac_bus_s: dma-controller@ffb20000 { compatible = "arm,pl330", "arm,primecell"; reg = <0xffb20000 0x4000>; interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; #dma-cells = <1>; clocks = <&cru ACLK_DMAC1>; clock-names = "apb_pclk"; }; dmac_bus_ns: dma-controller@ffb60000 { compatible = "arm,pl330", "arm,primecell"; reg = <0xffb60000 0x4000>; interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; #dma-cells = <1>; clocks = <&cru ACLK_DMAC1>; clock-names = "apb_pclk"; status = "disabled"; }; and a board wanting to use the non-secure variant would then be required to override the status and the dma channels in question (this dmac does mainly i2s and spdif): &dmac_bus_s { status = "disabled"; }: &dmac_bus_ns { status = "okay"; }; &i2s0 { dmas = <&dmac_bus_ns 6>, <&dmac_bus_ns 7>; }; > [*] https://patchwork.kernel.org/patch/4620251/ > > > [0] > > https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boo > > t/dts/rk3288.dtsi [1] > > https://github.com/mmind/linux-rockchip/blob/devel/workbench/arch/arm/boo > > t/dts/rk3288-evb.dtsi > [snip]
On 07/30/2014 03:12 AM, Heiko Stuebner wrote: > Add both the bus and peripheral pl330 dma controllers present in rk3288 socs. > The first dma controller can change between secure and non-secure mode and is > left by the bootloader in secure mode, which gets added here. > > Signed-off-by: Heiko Stuebner <heiko@sntech.de> > --- > arch/arm/boot/dts/rk3288.dtsi | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi > index 3ef8951..3f39d26 100644 > --- a/arch/arm/boot/dts/rk3288.dtsi > +++ b/arch/arm/boot/dts/rk3288.dtsi > @@ -62,6 +62,33 @@ > }; > }; > > + amba { > + compatible = "arm,amba-bus"; > + #address-cells = <1>; > + #size-cells = <1>; > + ranges; > + > + dmac1: dma@ffb20000 { > + compatible = "arm,pl330", "arm,primecell"; > + reg = <0xffb20000 0x4000>; > + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + clocks = <&cru ACLK_DMAC1>; > + clock-names = "apb_pclk"; > + }; > + > + dmac2: dma@ff250000 { > + compatible = "arm,pl330", "arm,primecell"; > + reg = <0xff250000 0x4000>; > + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; > + #dma-cells = <1>; > + clocks = <&cru ACLK_DMAC2>; > + clock-names = "apb_pclk"; > + }; > + }; > + > xin24m: oscillator { > compatible = "fixed-clock"; > clock-frequency = <24000000>; Tested-by: Kever Yang <kever.yang@rock-chips.com>
diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi index 3ef8951..3f39d26 100644 --- a/arch/arm/boot/dts/rk3288.dtsi +++ b/arch/arm/boot/dts/rk3288.dtsi @@ -62,6 +62,33 @@ }; }; + amba { + compatible = "arm,amba-bus"; + #address-cells = <1>; + #size-cells = <1>; + ranges; + + dmac1: dma@ffb20000 { + compatible = "arm,pl330", "arm,primecell"; + reg = <0xffb20000 0x4000>; + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>; + #dma-cells = <1>; + clocks = <&cru ACLK_DMAC1>; + clock-names = "apb_pclk"; + }; + + dmac2: dma@ff250000 { + compatible = "arm,pl330", "arm,primecell"; + reg = <0xff250000 0x4000>; + interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>; + #dma-cells = <1>; + clocks = <&cru ACLK_DMAC2>; + clock-names = "apb_pclk"; + }; + }; + xin24m: oscillator { compatible = "fixed-clock"; clock-frequency = <24000000>;
Add both the bus and peripheral pl330 dma controllers present in rk3288 socs. The first dma controller can change between secure and non-secure mode and is left by the bootloader in secure mode, which gets added here. Signed-off-by: Heiko Stuebner <heiko@sntech.de> --- arch/arm/boot/dts/rk3288.dtsi | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)