| Message ID | 1467150186-11427-2-git-send-email-twp@codeaurora.org (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: > From: Andy Gross <andy.gross@linaro.org> > > This patch removes the crci information from the dma > channel property. At least one client device requires > using more than one CRCI value for a channel. This does > not match the current binding and the crci information > needs to be removed. > > Instead, the client device will provide this information > via other means. > > Signed-off-by: Andy Gross <andy.gross@linaro.org> > Signed-off-by: Thomas Pedersen <twp@codeaurora.org> > --- > Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 ++++++---------- > 1 file changed, 6 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt b/Documentation/devicetree/bindings/dma/qcom_adm.txt > index 9bcab91..38d45f8 100644 > --- a/Documentation/devicetree/bindings/dma/qcom_adm.txt > +++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt > @@ -4,8 +4,7 @@ Required properties: > - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 > - reg: Address range for DMA registers > - interrupts: Should contain one interrupt shared by all channels > -- #dma-cells: must be <2>. First cell denotes the channel number. Second cell > - denotes CRCI (client rate control interface) flow control assignment. > +- #dma-cells: must be <1>. First cell denotes the channel number. I've actually been thinking more about this. The crci being specified in the slave config is probably the wrong approach. What we really need is each physical DMA channel to allow for virtual channels that allow for a CRCI setting (0 being no flow control). This would require the properties to continue to be 2 for the dma definition. This would also require clients to get multiple references to the same DMA channel if they require some communications with and without flow control. For NAND, this would mean having two channels for dma. One for flow controlled and one without. Anyone have reservations with this? Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Andy, On 2016-06-29 14:06, Andy Gross wrote: > On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: >> From: Andy Gross <andy.gross@linaro.org> >> >> This patch removes the crci information from the dma >> channel property. At least one client device requires >> using more than one CRCI value for a channel. This does >> not match the current binding and the crci information >> needs to be removed. >> >> Instead, the client device will provide this information >> via other means. >> >> Signed-off-by: Andy Gross <andy.gross@linaro.org> >> Signed-off-by: Thomas Pedersen <twp@codeaurora.org> >> --- >> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 >> ++++++---------- >> 1 file changed, 6 insertions(+), 10 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> index 9bcab91..38d45f8 100644 >> --- a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> +++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> @@ -4,8 +4,7 @@ Required properties: >> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 >> - reg: Address range for DMA registers >> - interrupts: Should contain one interrupt shared by all channels >> -- #dma-cells: must be <2>. First cell denotes the channel number. >> Second cell >> - denotes CRCI (client rate control interface) flow control >> assignment. >> +- #dma-cells: must be <1>. First cell denotes the channel number. > > I've actually been thinking more about this. The crci being specified > in the > slave config is probably the wrong approach. What we really need is > each > physical DMA channel to allow for virtual channels that allow for a > CRCI setting > (0 being no flow control). This would require the properties to > continue to be > 2 for the dma definition. AFAICT the cmd-crci and data-crci properties in the NAND controller client are really just the slave_id for the DMA engine. Flow control is decided by some other heuristic such as whether it's a read/write/etc. command. > This would also require clients to get multiple references to the same > DMA > channel if they require some communications with and without flow > control. > > For NAND, this would mean having two channels for dma. One for flow > controlled > and one without. So the NAND DT would look something like: dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; dma-names = "rxtx", "rxtx_fc"; qcom,cmd-crci = <15>; qcom,data-crci = <3>; ? We'd still need the cmd-crci and data-crci properties for the slave_id, unless I'm confusing something? Thanks,
On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote: > Hi Andy, > > On 2016-06-29 14:06, Andy Gross wrote: > >On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: > >>From: Andy Gross <andy.gross@linaro.org> > >> > >>This patch removes the crci information from the dma > >>channel property. At least one client device requires > >>using more than one CRCI value for a channel. This does > >>not match the current binding and the crci information > >>needs to be removed. > >> > >>Instead, the client device will provide this information > >>via other means. > >> > >>Signed-off-by: Andy Gross <andy.gross@linaro.org> > >>Signed-off-by: Thomas Pedersen <twp@codeaurora.org> > >>--- > >> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 > >>++++++---------- > >> 1 file changed, 6 insertions(+), 10 deletions(-) > >> > >>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>b/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>index 9bcab91..38d45f8 100644 > >>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>@@ -4,8 +4,7 @@ Required properties: > >> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 > >> - reg: Address range for DMA registers > >> - interrupts: Should contain one interrupt shared by all channels > >>-- #dma-cells: must be <2>. First cell denotes the channel number. > >>Second cell > >>- denotes CRCI (client rate control interface) flow control assignment. > >>+- #dma-cells: must be <1>. First cell denotes the channel number. > > > >I've actually been thinking more about this. The crci being specified in > >the > >slave config is probably the wrong approach. What we really need is each > >physical DMA channel to allow for virtual channels that allow for a CRCI > >setting > >(0 being no flow control). This would require the properties to continue > >to be > >2 for the dma definition. > > AFAICT the cmd-crci and data-crci properties in the NAND controller client > are > really just the slave_id for the DMA engine. Flow control is decided by some > other heuristic such as whether it's a read/write/etc. command. > > >This would also require clients to get multiple references to the same DMA > >channel if they require some communications with and without flow control. > > > >For NAND, this would mean having two channels for dma. One for flow > >controlled > >and one without. > > So the NAND DT would look something like: > > dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; > dma-names = "rxtx", "rxtx_fc"; > qcom,cmd-crci = <15>; > qcom,data-crci = <3>; > > ? We'd still need the cmd-crci and data-crci properties for the slave_id, > unless > I'm confusing something? No, what we would have are separate channels for cmd and data that would be virtual channels sharing the same hardware channel. At least that is what I am thinking. So dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>; dma-names = "rxtx", "rxtx-cmd", "rxtx-data"; # insert better names here All three would use channel 3, but the virtual channel would know about the crci. CRCI = 0 is no flow control. I need to look at the nand a little harder to see how they formulate the dma requests. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-07-01 12:08, Andy Gross wrote: > On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote: >> Hi Andy, >> >> On 2016-06-29 14:06, Andy Gross wrote: >> >On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: >> >>From: Andy Gross <andy.gross@linaro.org> >> >> >> >>This patch removes the crci information from the dma >> >>channel property. At least one client device requires >> >>using more than one CRCI value for a channel. This does >> >>not match the current binding and the crci information >> >>needs to be removed. >> >> >> >>Instead, the client device will provide this information >> >>via other means. >> >> >> >>Signed-off-by: Andy Gross <andy.gross@linaro.org> >> >>Signed-off-by: Thomas Pedersen <twp@codeaurora.org> >> >>--- >> >> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 >> >>++++++---------- >> >> 1 file changed, 6 insertions(+), 10 deletions(-) >> >> >> >>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>index 9bcab91..38d45f8 100644 >> >>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>@@ -4,8 +4,7 @@ Required properties: >> >> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 >> >> - reg: Address range for DMA registers >> >> - interrupts: Should contain one interrupt shared by all channels >> >>-- #dma-cells: must be <2>. First cell denotes the channel number. >> >>Second cell >> >>- denotes CRCI (client rate control interface) flow control assignment. >> >>+- #dma-cells: must be <1>. First cell denotes the channel number. >> > >> >I've actually been thinking more about this. The crci being specified in >> >the >> >slave config is probably the wrong approach. What we really need is each >> >physical DMA channel to allow for virtual channels that allow for a CRCI >> >setting >> >(0 being no flow control). This would require the properties to continue >> >to be >> >2 for the dma definition. >> >> AFAICT the cmd-crci and data-crci properties in the NAND controller >> client >> are >> really just the slave_id for the DMA engine. Flow control is decided >> by some >> other heuristic such as whether it's a read/write/etc. command. >> >> >This would also require clients to get multiple references to the same DMA >> >channel if they require some communications with and without flow control. >> > >> >For NAND, this would mean having two channels for dma. One for flow >> >controlled >> >and one without. >> >> So the NAND DT would look something like: >> >> dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; >> dma-names = "rxtx", "rxtx_fc"; >> qcom,cmd-crci = <15>; >> qcom,data-crci = <3>; >> >> ? We'd still need the cmd-crci and data-crci properties for the >> slave_id, >> unless >> I'm confusing something? > > No, what we would have are separate channels for cmd and data that > would be > virtual channels sharing the same hardware channel. At least that is > what I am > thinking. So > > dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>; > dma-names = "rxtx", "rxtx-cmd", "rxtx-data"; # insert better names > here > > All three would use channel 3, but the virtual channel would know about > the > crci. CRCI = 0 is no flow control. > > I need to look at the nand a little harder to see how they formulate > the dma > requests. Did you have a chance to look into this? If not, can you provide some more specific pointers (like would these virtual channels be in the NAND controller or ADM driver?), and I can give it a try?
On Thu, Jul 28, 2016 at 03:16:07PM -0700, Thomas Pedersen wrote: > On 2016-07-01 12:08, Andy Gross wrote: > >On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote: > >>Hi Andy, > >> > >>On 2016-06-29 14:06, Andy Gross wrote: > >>>On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: > >>>>From: Andy Gross <andy.gross@linaro.org> > >>>> > >>>>This patch removes the crci information from the dma > >>>>channel property. At least one client device requires > >>>>using more than one CRCI value for a channel. This does > >>>>not match the current binding and the crci information > >>>>needs to be removed. > >>>> > >>>>Instead, the client device will provide this information > >>>>via other means. > >>>> > >>>>Signed-off-by: Andy Gross <andy.gross@linaro.org> > >>>>Signed-off-by: Thomas Pedersen <twp@codeaurora.org> > >>>>--- > >>>> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 > >>>>++++++---------- > >>>> 1 file changed, 6 insertions(+), 10 deletions(-) > >>>> > >>>>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>>>b/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>>>index 9bcab91..38d45f8 100644 > >>>>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>>>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt > >>>>@@ -4,8 +4,7 @@ Required properties: > >>>> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 > >>>> - reg: Address range for DMA registers > >>>> - interrupts: Should contain one interrupt shared by all channels > >>>>-- #dma-cells: must be <2>. First cell denotes the channel number. > >>>>Second cell > >>>>- denotes CRCI (client rate control interface) flow control assignment. > >>>>+- #dma-cells: must be <1>. First cell denotes the channel number. > >>> > >>>I've actually been thinking more about this. The crci being specified in > >>>the > >>>slave config is probably the wrong approach. What we really need is each > >>>physical DMA channel to allow for virtual channels that allow for a CRCI > >>>setting > >>>(0 being no flow control). This would require the properties to continue > >>>to be > >>>2 for the dma definition. > >> > >>AFAICT the cmd-crci and data-crci properties in the NAND controller > >>client > >>are > >>really just the slave_id for the DMA engine. Flow control is decided by > >>some > >>other heuristic such as whether it's a read/write/etc. command. > >> > >>>This would also require clients to get multiple references to the same DMA > >>>channel if they require some communications with and without flow control. > >>> > >>>For NAND, this would mean having two channels for dma. One for flow > >>>controlled > >>>and one without. > >> > >>So the NAND DT would look something like: > >> > >> dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; > >> dma-names = "rxtx", "rxtx_fc"; > >> qcom,cmd-crci = <15>; > >> qcom,data-crci = <3>; > >> > >>? We'd still need the cmd-crci and data-crci properties for the > >>slave_id, > >>unless > >>I'm confusing something? > > > >No, what we would have are separate channels for cmd and data that would > >be > >virtual channels sharing the same hardware channel. At least that is what > >I am > >thinking. So > > > >dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>; > >dma-names = "rxtx", "rxtx-cmd", "rxtx-data"; # insert better names > >here > > > >All three would use channel 3, but the virtual channel would know about > >the > >crci. CRCI = 0 is no flow control. > > > >I need to look at the nand a little harder to see how they formulate the > >dma > >requests. > > Did you have a chance to look into this? If not, can you provide some more > specific pointers (like would these virtual channels be in the NAND > controller > or ADM driver?), and I can give it a try? The virtual channels would be in the ADM driver. So when you specify the dmas in the DT, you'd specify adm, channel X, crci Y. This in turn would return a dma channel that encapsulates the channel + crci. This takes away the requirement of having to do the slave_config. As it is right now with the current ADM driver, you grab one channel and then slave_config it before you add descriptors. This simplifies the number of dma channels, but complicates things by requiring you to constantly slave_config if you are going from crci, to no crci. Separating the channel+crci into a virtual channel means that the ADM driver itself will collate the requests as they come in, and apply them to the actual hardware channel. That make sense? If so, this requires modifying the DT doc to specify the crci in the binding, modifying the ADM driver to identify/add channels based on the channel + crci, and modifying the clients which use both flow control and non-flow control. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2016-08-01 13:37, Andy Gross wrote: > On Thu, Jul 28, 2016 at 03:16:07PM -0700, Thomas Pedersen wrote: >> On 2016-07-01 12:08, Andy Gross wrote: >> >On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote: >> >>Hi Andy, >> >> >> >>On 2016-06-29 14:06, Andy Gross wrote: >> >>>On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: >> >>>>From: Andy Gross <andy.gross@linaro.org> >> >>>> >> >>>>This patch removes the crci information from the dma >> >>>>channel property. At least one client device requires >> >>>>using more than one CRCI value for a channel. This does >> >>>>not match the current binding and the crci information >> >>>>needs to be removed. >> >>>> >> >>>>Instead, the client device will provide this information >> >>>>via other means. >> >>>> >> >>>>Signed-off-by: Andy Gross <andy.gross@linaro.org> >> >>>>Signed-off-by: Thomas Pedersen <twp@codeaurora.org> >> >>>>--- >> >>>> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 >> >>>>++++++---------- >> >>>> 1 file changed, 6 insertions(+), 10 deletions(-) >> >>>> >> >>>>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>index 9bcab91..38d45f8 100644 >> >>>>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>@@ -4,8 +4,7 @@ Required properties: >> >>>> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 >> >>>> - reg: Address range for DMA registers >> >>>> - interrupts: Should contain one interrupt shared by all channels >> >>>>-- #dma-cells: must be <2>. First cell denotes the channel number. >> >>>>Second cell >> >>>>- denotes CRCI (client rate control interface) flow control assignment. >> >>>>+- #dma-cells: must be <1>. First cell denotes the channel number. >> >>> >> >>>I've actually been thinking more about this. The crci being specified in >> >>>the >> >>>slave config is probably the wrong approach. What we really need is each >> >>>physical DMA channel to allow for virtual channels that allow for a CRCI >> >>>setting >> >>>(0 being no flow control). This would require the properties to continue >> >>>to be >> >>>2 for the dma definition. >> >> >> >>AFAICT the cmd-crci and data-crci properties in the NAND controller >> >>client >> >>are >> >>really just the slave_id for the DMA engine. Flow control is decided by >> >>some >> >>other heuristic such as whether it's a read/write/etc. command. >> >> >> >>>This would also require clients to get multiple references to the same DMA >> >>>channel if they require some communications with and without flow control. >> >>> >> >>>For NAND, this would mean having two channels for dma. One for flow >> >>>controlled >> >>>and one without. >> >> >> >>So the NAND DT would look something like: >> >> >> >> dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; >> >> dma-names = "rxtx", "rxtx_fc"; >> >> qcom,cmd-crci = <15>; >> >> qcom,data-crci = <3>; >> >> >> >>? We'd still need the cmd-crci and data-crci properties for the >> >>slave_id, >> >>unless >> >>I'm confusing something? >> > >> >No, what we would have are separate channels for cmd and data that would >> >be >> >virtual channels sharing the same hardware channel. At least that is what >> >I am >> >thinking. So >> > >> >dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>; >> >dma-names = "rxtx", "rxtx-cmd", "rxtx-data"; # insert better names >> >here >> > >> >All three would use channel 3, but the virtual channel would know about >> >the >> >crci. CRCI = 0 is no flow control. >> > >> >I need to look at the nand a little harder to see how they formulate the >> >dma >> >requests. >> >> Did you have a chance to look into this? If not, can you provide some >> more >> specific pointers (like would these virtual channels be in the NAND >> controller >> or ADM driver?), and I can give it a try? > > The virtual channels would be in the ADM driver. So when you specify > the dmas > in the DT, you'd specify adm, channel X, crci Y. This in turn would > return a > dma channel that encapsulates the channel + crci. This takes away the > requirement of having to do the slave_config. > > As it is right now with the current ADM driver, you grab one channel > and then > slave_config it before you add descriptors. This simplifies the number > of dma > channels, but complicates things by requiring you to constantly > slave_config if > you are going from crci, to no crci. > > Separating the channel+crci into a virtual channel means that the ADM > driver > itself will collate the requests as they come in, and apply them to the > actual > hardware channel. > > That make sense? If so, this requires modifying the DT doc to specify > the crci > in the binding, modifying the ADM driver to identify/add channels based > on the > channel + crci, and modifying the clients which use both flow control > and > non-flow control. OK thanks for clearing that up. Would be nice to avoid calling slave_config for each command. I'll give this a shot.
diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt b/Documentation/devicetree/bindings/dma/qcom_adm.txt index 9bcab91..38d45f8 100644 --- a/Documentation/devicetree/bindings/dma/qcom_adm.txt +++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt @@ -4,8 +4,7 @@ Required properties: - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 - reg: Address range for DMA registers - interrupts: Should contain one interrupt shared by all channels -- #dma-cells: must be <2>. First cell denotes the channel number. Second cell - denotes CRCI (client rate control interface) flow control assignment. +- #dma-cells: must be <1>. First cell denotes the channel number. - clocks: Should contain the core clock and interface clock. - clock-names: Must contain "core" for the core clock and "iface" for the interface clock. @@ -22,7 +21,7 @@ Example: compatible = "qcom,adm"; reg = <0x18300000 0x100000>; interrupts = <0 170 0>; - #dma-cells = <2>; + #dma-cells = <1>; clocks = <&gcc ADM0_CLK>, <&gcc ADM0_PBUS_CLK>; clock-names = "core", "iface"; @@ -35,15 +34,12 @@ Example: qcom,ee = <0>; }; -DMA clients must use the format descripted in the dma.txt file, using a three +DMA clients must use the format descripted in the dma.txt file, using a two cell specifier for each channel. -Each dmas request consists of 3 cells: +Each dmas request consists of two cells: 1. phandle pointing to the DMA controller 2. channel number - 3. CRCI assignment, if applicable. If no CRCI flow control is required, use 0. - The CRCI is used for flow control. It identifies the peripheral device that - is the source/destination for the transferred data. Example: @@ -56,7 +52,7 @@ Example: cs-gpios = <&qcom_pinmux 20 0>; - dmas = <&adm_dma 6 9>, - <&adm_dma 5 10>; + dmas = <&adm_dma 6>, + <&adm_dma 5>; dma-names = "rx", "tx"; };