| Message ID | 20200403094033.8288-12-xia.jiang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for mt2701 JPEG ENC support | expand |
Hi Xia, On Fri, Apr 03, 2020 at 05:40:30PM +0800, Xia Jiang wrote: > Add jpeg enc device tree node document > > Reviewed-by: Rob Herring <robh@kernel.org> > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > --- > v8: no changes > > v7: no changes > > v6: no changes > > v5: no changes > > v4: no changes > > v3: change compatible to SoC specific compatible > > v2: no changes > --- > .../bindings/media/mediatek-jpeg-encoder.txt | 37 +++++++++++++++++++ > 1 file changed, 37 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > Thank you for the patch. Please see my comments inline. > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > new file mode 100644 > index 000000000000..fa8da699493b > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > @@ -0,0 +1,37 @@ > +* MediaTek JPEG Encoder > + > +MediaTek JPEG Encoder is the JPEG encode hardware present in MediaTek SoCs > + > +Required properties: > +- compatible : should be one of: > + "mediatek,mt2701-jpgenc" > + ... What does this "..." mean? > + followed by "mediatek,mtk-jpgenc" > +- reg : physical base address of the JPEG encoder registers and length of > + memory mapped region. > +- interrupts : interrupt number to the interrupt controller. > +- clocks: device clocks, see > + Documentation/devicetree/bindings/clock/clock-bindings.txt for details. > +- clock-names: must contain "jpgenc". It is the clock of JPEG encoder. nit: In principle the clocks should be named after the function the clock performs on the consumer side, i.e. the JPEG block in this case, I guess here it's just a generic clock that does everything, but I guess it comes from somewhere. Is it the AHB clock or something? In that case it would normally be called "ahb". > +- power-domains: a phandle to the power domain, see > + Documentation/devicetree/bindings/power/power_domain.txt for details. > +- mediatek,larb: must contain the local arbiters in the current SoCs, see > + Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt > + for details. I believe this isn't necessary anymore, because larbs are added automatically by the MTK IOMMU driver using device links. +CC Yong who worked on that. > +- iommus: should point to the respective IOMMU block with master port as > + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > + for details. > + > +Example: > + jpegenc: jpegenc@1500a000 { > + compatible = "mediatek,mt2701-jpgenc", > + "mediatek,mtk-jpgenc"; > + reg = <0 0x1500a000 0 0x1000>; > + interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_LOW>; > + clocks = <&imgsys CLK_IMG_VENC>; > + clock-names = "jpgenc"; > + power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>; > + mediatek,larb = <&larb2>; Ditto. Best regards, Tomasz
On Thu, 2020-05-21 at 16:00 +0000, Tomasz Figa wrote: > Hi Xia, > > On Fri, Apr 03, 2020 at 05:40:30PM +0800, Xia Jiang wrote: > > Add jpeg enc device tree node document > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > > --- > > v8: no changes > > > > v7: no changes > > > > v6: no changes > > > > v5: no changes > > > > v4: no changes > > > > v3: change compatible to SoC specific compatible > > > > v2: no changes > > --- > > .../bindings/media/mediatek-jpeg-encoder.txt | 37 +++++++++++++++++++ > > 1 file changed, 37 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > > > Thank you for the patch. Please see my comments inline. Dear Tomasz, Sorry for missing this message. Replied below. > > > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > new file mode 100644 > > index 000000000000..fa8da699493b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > @@ -0,0 +1,37 @@ > > +* MediaTek JPEG Encoder > > + > > +MediaTek JPEG Encoder is the JPEG encode hardware present in MediaTek SoCs > > + > > +Required properties: > > +- compatible : should be one of: > > + "mediatek,mt2701-jpgenc" > > + ... > > What does this "..." mean? "..." means that compatible name is not just "mediatek,mt2701-jpgenc", different project has different compatible name(for example the MT8173's compatible name may be "mediatek,mt8173-jpgenc"). > > > + followed by "mediatek,mtk-jpgenc" > > +- reg : physical base address of the JPEG encoder registers and length of > > + memory mapped region. > > +- interrupts : interrupt number to the interrupt controller. > > +- clocks: device clocks, see > > + Documentation/devicetree/bindings/clock/clock-bindings.txt for details. > > +- clock-names: must contain "jpgenc". It is the clock of JPEG encoder. > > nit: In principle the clocks should be named after the function the clock > performs on the consumer side, i.e. the JPEG block in this case, I guess > here it's just a generic clock that does everything, but I guess it comes > from somewhere. Is it the AHB clock or something? In that case it would > normally be called "ahb". I have confirmed with hardware designer that the jpeg clock is not AHB clock,it follows subsys clock(because 2701 is the old IC,I didn't get the source name).It has the same source with venc clock.We can see that the clocks = <imgsys CLK_IMG_VENC>, Should I name it "venc" or the orignal "jpgenc"? > > > +- power-domains: a phandle to the power domain, see > > + Documentation/devicetree/bindings/power/power_domain.txt for details. > > +- mediatek,larb: must contain the local arbiters in the current SoCs, see > > + Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt > > + for details. > > I believe this isn't necessary anymore, because larbs are added > automatically by the MTK IOMMU driver using device links. +CC Yong who > worked on that. Yes,I have confirmed with Yong that he will help me to modify this.Is it ok that I keep the orignal larb code? Best Regards, Xia Jiang > > > +- iommus: should point to the respective IOMMU block with master port as > > + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > > + for details. > > + > > +Example: > > + jpegenc: jpegenc@1500a000 { > > + compatible = "mediatek,mt2701-jpgenc", > > + "mediatek,mtk-jpgenc"; > > + reg = <0 0x1500a000 0 0x1000>; > > + interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_LOW>; > > + clocks = <&imgsys CLK_IMG_VENC>; > > + clock-names = "jpgenc"; > > + power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>; > > + mediatek,larb = <&larb2>; > > Ditto. > > Best regards, > Tomasz
On Thu, Jun 18, 2020 at 5:43 AM Xia Jiang <xia.jiang@mediatek.com> wrote: > > On Thu, 2020-05-21 at 16:00 +0000, Tomasz Figa wrote: > > Hi Xia, > > > > On Fri, Apr 03, 2020 at 05:40:30PM +0800, Xia Jiang wrote: > > > Add jpeg enc device tree node document > > > > > > Reviewed-by: Rob Herring <robh@kernel.org> > > > Signed-off-by: Xia Jiang <xia.jiang@mediatek.com> > > > --- > > > v8: no changes > > > > > > v7: no changes > > > > > > v6: no changes > > > > > > v5: no changes > > > > > > v4: no changes > > > > > > v3: change compatible to SoC specific compatible > > > > > > v2: no changes > > > --- > > > .../bindings/media/mediatek-jpeg-encoder.txt | 37 +++++++++++++++++++ > > > 1 file changed, 37 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > > > > > > Thank you for the patch. Please see my comments inline. > Dear Tomasz, > > Sorry for missing this message. Replied below. > > > > > diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > > new file mode 100644 > > > index 000000000000..fa8da699493b > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt > > > @@ -0,0 +1,37 @@ > > > +* MediaTek JPEG Encoder > > > + > > > +MediaTek JPEG Encoder is the JPEG encode hardware present in MediaTek SoCs > > > + > > > +Required properties: > > > +- compatible : should be one of: > > > + "mediatek,mt2701-jpgenc" > > > + ... > > > > What does this "..." mean? > "..." means that compatible name is not just "mediatek,mt2701-jpgenc", > different project has different compatible name(for example the MT8173's > compatible name may be "mediatek,mt8173-jpgenc"). The bindings need to list all the currently defined compatible strings explicitly. > > > > > + followed by "mediatek,mtk-jpgenc" > > > +- reg : physical base address of the JPEG encoder registers and length of > > > + memory mapped region. > > > +- interrupts : interrupt number to the interrupt controller. > > > +- clocks: device clocks, see > > > + Documentation/devicetree/bindings/clock/clock-bindings.txt for details. > > > +- clock-names: must contain "jpgenc". It is the clock of JPEG encoder. > > > > nit: In principle the clocks should be named after the function the clock > > performs on the consumer side, i.e. the JPEG block in this case, I guess > > here it's just a generic clock that does everything, but I guess it comes > > from somewhere. Is it the AHB clock or something? In that case it would > > normally be called "ahb". > I have confirmed with hardware designer that the jpeg clock is not AHB > clock,it follows subsys clock(because 2701 is the old IC,I didn't get > the source name).It has the same source with venc clock.We can see that > the clocks = <imgsys CLK_IMG_VENC>, Should I name it "venc" or the > orignal "jpgenc"? The clock name of the device-side bindings is the name of the input of the device itself, no matter where the clock comes from in the SoC. I guess if there is no specific purpose of this clock, "jpgenc" is as good as any other name (e.g. "clock"), so feel free to keep it. > > > > > +- power-domains: a phandle to the power domain, see > > > + Documentation/devicetree/bindings/power/power_domain.txt for details. > > > +- mediatek,larb: must contain the local arbiters in the current SoCs, see > > > + Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt > > > + for details. > > > > I believe this isn't necessary anymore, because larbs are added > > automatically by the MTK IOMMU driver using device links. +CC Yong who > > worked on that. > Yes,I have confirmed with Yong that he will help me to modify this.Is it > ok that I keep the orignal larb code? I guess it depends on the order of landing the patches. If we intend to land this series before the larb removal series, the binding should stay as is. If the other way around, this should be removed. Please coordinate with Yong. Best regards, Tomasz
diff --git a/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt new file mode 100644 index 000000000000..fa8da699493b --- /dev/null +++ b/Documentation/devicetree/bindings/media/mediatek-jpeg-encoder.txt @@ -0,0 +1,37 @@ +* MediaTek JPEG Encoder + +MediaTek JPEG Encoder is the JPEG encode hardware present in MediaTek SoCs + +Required properties: +- compatible : should be one of: + "mediatek,mt2701-jpgenc" + ... + followed by "mediatek,mtk-jpgenc" +- reg : physical base address of the JPEG encoder registers and length of + memory mapped region. +- interrupts : interrupt number to the interrupt controller. +- clocks: device clocks, see + Documentation/devicetree/bindings/clock/clock-bindings.txt for details. +- clock-names: must contain "jpgenc". It is the clock of JPEG encoder. +- power-domains: a phandle to the power domain, see + Documentation/devicetree/bindings/power/power_domain.txt for details. +- mediatek,larb: must contain the local arbiters in the current SoCs, see + Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt + for details. +- iommus: should point to the respective IOMMU block with master port as + argument, see Documentation/devicetree/bindings/iommu/mediatek,iommu.txt + for details. + +Example: + jpegenc: jpegenc@1500a000 { + compatible = "mediatek,mt2701-jpgenc", + "mediatek,mtk-jpgenc"; + reg = <0 0x1500a000 0 0x1000>; + interrupts = <GIC_SPI 141 IRQ_TYPE_LEVEL_LOW>; + clocks = <&imgsys CLK_IMG_VENC>; + clock-names = "jpgenc"; + power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>; + mediatek,larb = <&larb2>; + iommus = <&iommu MT2701_M4U_PORT_JPGENC_RDMA>, + <&iommu MT2701_M4U_PORT_JPGENC_BSDMA>; + };