| Message ID | 20180703123214.23090-6-paul@crapouillou.net |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | dma-jz4780 improvements | expand |
On 3 July 2018 at 18:02, Paul Cercueil <paul@crapouillou.net> wrote: > The JZ4740 SoC has a single DMA core starring six DMA channels. > > Signed-off-by: Paul Cercueil <paul@crapouillou.net> > --- > Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 + > drivers/dma/Kconfig | 2 +- > drivers/dma/dma-jz4780.c | 4 ++++ > 3 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > index 0fd0759053be..d7ca3f925fdf 100644 > --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt > +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt > @@ -5,6 +5,7 @@ Required properties: > - compatible: Should be one of: > * ingenic,jz4780-dma > * ingenic,jz4770-dma > + * ingenic,jz4740-dma > - reg: Should contain the DMA channel registers location and length, followed > by the DMA controller registers location and length. > - interrupts: Should contain the interrupt specifier of the DMA controller. > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig > index 48d25dccedb7..a935d15ec581 100644 > --- a/drivers/dma/Kconfig > +++ b/drivers/dma/Kconfig > @@ -143,7 +143,7 @@ config DMA_JZ4740 > > config DMA_JZ4780 > tristate "JZ4780 DMA support" > - depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST > + depends on MACH_INGENIC || COMPILE_TEST > select DMA_ENGINE > select DMA_VIRTUAL_CHANNELS > help > diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c > index 7b8b2dcd119e..ccadbe61dde7 100644 > --- a/drivers/dma/dma-jz4780.c > +++ b/drivers/dma/dma-jz4780.c > @@ -133,6 +133,7 @@ struct jz4780_dma_chan { > }; > > enum jz_version { > + ID_JZ4740, > ID_JZ4770, > ID_JZ4780, > }; > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > } > > static const unsigned int jz4780_dma_ord_max[] = { > + [ID_JZ4740] = 5, > [ID_JZ4770] = 6, > [ID_JZ4780] = 7, > }; > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > } > > static const unsigned int jz4780_dma_nb_channels[] = { > + [ID_JZ4740] = 6, > [ID_JZ4770] = 6, > [ID_JZ4780] = 32, > }; > > static const struct of_device_id jz4780_dma_dt_match[] = { > + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 }, > { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 }, > { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 }, > {}, > -- > 2.18.0 > > Patch looks good to me. Reviewed-by: PrasannaKumar Muralidharan <prasannatsmkumar@gmail.com>/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03-07-18, 14:32, Paul Cercueil wrote: > enum jz_version { > + ID_JZ4740, > ID_JZ4770, > ID_JZ4780, > }; > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > } > > static const unsigned int jz4780_dma_ord_max[] = { > + [ID_JZ4740] = 5, > [ID_JZ4770] = 6, > [ID_JZ4780] = 7, > }; > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > } > > static const unsigned int jz4780_dma_nb_channels[] = { > + [ID_JZ4740] = 6, > [ID_JZ4770] = 6, > [ID_JZ4780] = 32, > }; I feel these should be done away with if we describe hardware in DT > > static const struct of_device_id jz4780_dma_dt_match[] = { > + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 }, adding .compatible should be the only thing required, if at all for this addition :)
On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote: > On 03-07-18, 14:32, Paul Cercueil wrote: > > > enum jz_version { > > + ID_JZ4740, > > ID_JZ4770, > > ID_JZ4780, > > }; > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > > } > > > > static const unsigned int jz4780_dma_ord_max[] = { > > + [ID_JZ4740] = 5, > > [ID_JZ4770] = 6, > > [ID_JZ4780] = 7, > > }; > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > > } > > > > static const unsigned int jz4780_dma_nb_channels[] = { > > + [ID_JZ4740] = 6, > > [ID_JZ4770] = 6, > > [ID_JZ4780] = 32, > > }; > > I feel these should be done away with if we describe hardware in DT The compatible property can imply things like this. But how this is structured is a bit strange. Normally you have a per compatible struct with these as elements and the compatible matching selects the struct. > > > > > static const struct of_device_id jz4780_dma_dt_match[] = { > > + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 }, > > adding .compatible should be the only thing required, if at all for this > addition :) > > -- > ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Le lun. 16 juil. 2018 à 23:33, Rob Herring <robh@kernel.org> a écrit : > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote: >> On 03-07-18, 14:32, Paul Cercueil wrote: >> >> > enum jz_version { >> > + ID_JZ4740, >> > ID_JZ4770, >> > ID_JZ4780, >> > }; >> > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct >> virt_dma_desc *vdesc) >> > } >> > >> > static const unsigned int jz4780_dma_ord_max[] = { >> > + [ID_JZ4740] = 5, >> > [ID_JZ4770] = 6, >> > [ID_JZ4780] = 7, >> > }; >> > @@ -801,11 +803,13 @@ static struct dma_chan >> *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, >> > } >> > >> > static const unsigned int jz4780_dma_nb_channels[] = { >> > + [ID_JZ4740] = 6, >> > [ID_JZ4770] = 6, >> > [ID_JZ4780] = 32, >> > }; >> >> I feel these should be done away with if we describe hardware in DT > > The compatible property can imply things like this. > > But how this is structured is a bit strange. Normally you have a per > compatible struct with these as elements and the compatible matching > selects the struct. You're right, I'll change that. >> >> > >> > static const struct of_device_id jz4780_dma_dt_match[] = { >> > + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 >> }, >> >> adding .compatible should be the only thing required, if at all for >> this >> addition :) >> >> -- >> ~Vinod -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 16-07-18, 15:33, Rob Herring wrote: > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote: > > On 03-07-18, 14:32, Paul Cercueil wrote: > > > > > enum jz_version { > > > + ID_JZ4740, > > > ID_JZ4770, > > > ID_JZ4780, > > > }; > > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > > > } > > > > > > static const unsigned int jz4780_dma_ord_max[] = { > > > + [ID_JZ4740] = 5, > > > [ID_JZ4770] = 6, > > > [ID_JZ4780] = 7, > > > }; > > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > > > } > > > > > > static const unsigned int jz4780_dma_nb_channels[] = { > > > + [ID_JZ4740] = 6, > > > [ID_JZ4770] = 6, > > > [ID_JZ4780] = 32, > > > }; > > > > I feel these should be done away with if we describe hardware in DT > > The compatible property can imply things like this. So what is the general recommendation, let DT describe hardware including version delta or use compatible to code that in driver? Is it documented anywhere?
On Tue, Jul 17, 2018 at 9:34 AM Vinod <vkoul@kernel.org> wrote: > > On 16-07-18, 15:33, Rob Herring wrote: > > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote: > > > On 03-07-18, 14:32, Paul Cercueil wrote: > > > > > > > enum jz_version { > > > > + ID_JZ4740, > > > > ID_JZ4770, > > > > ID_JZ4780, > > > > }; > > > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > > > > } > > > > > > > > static const unsigned int jz4780_dma_ord_max[] = { > > > > + [ID_JZ4740] = 5, > > > > [ID_JZ4770] = 6, > > > > [ID_JZ4780] = 7, > > > > }; > > > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > > > > } > > > > > > > > static const unsigned int jz4780_dma_nb_channels[] = { > > > > + [ID_JZ4740] = 6, > > > > [ID_JZ4770] = 6, > > > > [ID_JZ4780] = 32, > > > > }; > > > > > > I feel these should be done away with if we describe hardware in DT > > > > The compatible property can imply things like this. > > So what is the general recommendation, let DT describe hardware > including version delta or use compatible to code that in driver? Compatible is the version. Looking at the above, the version or ID isn't even stable. > Is it documented anywhere? Not really. It's a judgment call generally. Maybe # of DMA channels should be a property because that is something most controllers have. But you really have to define the property up front, not when the 2nd version of h/w shows up with different properties. To start defining guidelines, a couple of things come to mind: - Define properties for parameters that vary from board to board (for one SoC). - You can't add new required properties to existing bindings, so the not present default must work for all existing compatibles (or you need per compatible driver data). - Bugs/quirks/errata should be handled by compatible, not adding a property. Because bugs should be fixable without a dtb update and only a kernel update. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17-07-18, 11:40, Rob Herring wrote: > On Tue, Jul 17, 2018 at 9:34 AM Vinod <vkoul@kernel.org> wrote: > > > > On 16-07-18, 15:33, Rob Herring wrote: > > > On Mon, Jul 09, 2018 at 10:42:26PM +0530, Vinod wrote: > > > > On 03-07-18, 14:32, Paul Cercueil wrote: > > > > > > > > > enum jz_version { > > > > > + ID_JZ4740, > > > > > ID_JZ4770, > > > > > ID_JZ4780, > > > > > }; > > > > > @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) > > > > > } > > > > > > > > > > static const unsigned int jz4780_dma_ord_max[] = { > > > > > + [ID_JZ4740] = 5, > > > > > [ID_JZ4770] = 6, > > > > > [ID_JZ4780] = 7, > > > > > }; > > > > > @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, > > > > > } > > > > > > > > > > static const unsigned int jz4780_dma_nb_channels[] = { > > > > > + [ID_JZ4740] = 6, > > > > > [ID_JZ4770] = 6, > > > > > [ID_JZ4780] = 32, > > > > > }; > > > > > > > > I feel these should be done away with if we describe hardware in DT > > > > > > The compatible property can imply things like this. > > > > So what is the general recommendation, let DT describe hardware > > including version delta or use compatible to code that in driver? > > Compatible is the version. Looking at the above, the version or ID > isn't even stable. > > > Is it documented anywhere? > > Not really. It's a judgment call generally. Maybe # of DMA channels > should be a property because that is something most controllers have. > But you really have to define the property up front, not when the 2nd > version of h/w shows up with different properties. > > To start defining guidelines, a couple of things come to mind: > > - Define properties for parameters that vary from board to board (for one SoC). > - You can't add new required properties to existing bindings, so the > not present default must work for all existing compatibles (or you > need per compatible driver data). > - Bugs/quirks/errata should be handled by compatible, not adding a > property. Because bugs should be fixable without a dtb update and only > a kernel update. Sounds good to me, thanks for the guide.
diff --git a/Documentation/devicetree/bindings/dma/jz4780-dma.txt b/Documentation/devicetree/bindings/dma/jz4780-dma.txt index 0fd0759053be..d7ca3f925fdf 100644 --- a/Documentation/devicetree/bindings/dma/jz4780-dma.txt +++ b/Documentation/devicetree/bindings/dma/jz4780-dma.txt @@ -5,6 +5,7 @@ Required properties: - compatible: Should be one of: * ingenic,jz4780-dma * ingenic,jz4770-dma + * ingenic,jz4740-dma - reg: Should contain the DMA channel registers location and length, followed by the DMA controller registers location and length. - interrupts: Should contain the interrupt specifier of the DMA controller. diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig index 48d25dccedb7..a935d15ec581 100644 --- a/drivers/dma/Kconfig +++ b/drivers/dma/Kconfig @@ -143,7 +143,7 @@ config DMA_JZ4740 config DMA_JZ4780 tristate "JZ4780 DMA support" - depends on MACH_JZ4780 || MACH_JZ4770 || COMPILE_TEST + depends on MACH_INGENIC || COMPILE_TEST select DMA_ENGINE select DMA_VIRTUAL_CHANNELS help diff --git a/drivers/dma/dma-jz4780.c b/drivers/dma/dma-jz4780.c index 7b8b2dcd119e..ccadbe61dde7 100644 --- a/drivers/dma/dma-jz4780.c +++ b/drivers/dma/dma-jz4780.c @@ -133,6 +133,7 @@ struct jz4780_dma_chan { }; enum jz_version { + ID_JZ4740, ID_JZ4770, ID_JZ4780, }; @@ -247,6 +248,7 @@ static void jz4780_dma_desc_free(struct virt_dma_desc *vdesc) } static const unsigned int jz4780_dma_ord_max[] = { + [ID_JZ4740] = 5, [ID_JZ4770] = 6, [ID_JZ4780] = 7, }; @@ -801,11 +803,13 @@ static struct dma_chan *jz4780_of_dma_xlate(struct of_phandle_args *dma_spec, } static const unsigned int jz4780_dma_nb_channels[] = { + [ID_JZ4740] = 6, [ID_JZ4770] = 6, [ID_JZ4780] = 32, }; static const struct of_device_id jz4780_dma_dt_match[] = { + { .compatible = "ingenic,jz4740-dma", .data = (void *)ID_JZ4740 }, { .compatible = "ingenic,jz4770-dma", .data = (void *)ID_JZ4770 }, { .compatible = "ingenic,jz4780-dma", .data = (void *)ID_JZ4780 }, {},
The JZ4740 SoC has a single DMA core starring six DMA channels. Signed-off-by: Paul Cercueil <paul@crapouillou.net> --- Documentation/devicetree/bindings/dma/jz4780-dma.txt | 1 + drivers/dma/Kconfig | 2 +- drivers/dma/dma-jz4780.c | 4 ++++ 3 files changed, 6 insertions(+), 1 deletion(-)