| Message ID | 20200722155533.252844-9-helen.koike@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | move Rockchip ISP bindings out of staging / add ISP DT nodes for RK3399 | expand |
Hi Helen, On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: > From: Shunqian Zheng <zhengsq@rock-chips.com> > > RK3399 has two ISPs, but only isp0 was tested. > Add isp0 node in rk3399 dtsi > > Verified with: > make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml > > Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com> > Signed-off-by: Helen Koike <helen.koike@collabora.com> > > --- > > V4: > - update clock names > > V3: > - clean up clocks > > V2: > - re-order power-domains property > > V1: > This patch was originally part of this patchset: > > https://patchwork.kernel.org/patch/10267431/ > > The only difference is: > - add phy properties > - add ports > --- > arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > index dba9641947a3a..ed8ba75dbbce8 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { > status = "disabled"; > }; > > + isp0: isp0@ff910000 { > + compatible = "rockchip,rk3399-cif-isp"; > + reg = <0x0 0xff910000 0x0 0x4000>; > + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; > + clocks = <&cru SCLK_ISP0>, > + <&cru ACLK_ISP0_WRAPPER>, > + <&cru HCLK_ISP0_WRAPPER>; > + clock-names = "isp", "aclk", "hclk"; > + iommus = <&isp0_mmu>; > + phys = <&mipi_dphy_rx0>; > + phy-names = "dphy"; > + power-domains = <&power RK3399_PD_ISP0>; Should this have status = "disabled" too? The mipi_dphy_rx0 node is disabled by default too, so in the default configuration the driver would always fail to probe. Best regards, Tomasz
Hi Tomasz, On 9/26/20 10:00 AM, Tomasz Figa wrote: > Hi Helen, > > On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: >> From: Shunqian Zheng <zhengsq@rock-chips.com> >> >> RK3399 has two ISPs, but only isp0 was tested. >> Add isp0 node in rk3399 dtsi >> >> Verified with: >> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml >> >> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> >> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com> >> Signed-off-by: Helen Koike <helen.koike@collabora.com> >> >> --- >> >> V4: >> - update clock names >> >> V3: >> - clean up clocks >> >> V2: >> - re-order power-domains property >> >> V1: >> This patch was originally part of this patchset: >> >> https://patchwork.kernel.org/patch/10267431/ >> >> The only difference is: >> - add phy properties >> - add ports >> --- >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> index dba9641947a3a..ed8ba75dbbce8 100644 >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { >> status = "disabled"; >> }; >> >> + isp0: isp0@ff910000 { >> + compatible = "rockchip,rk3399-cif-isp"; >> + reg = <0x0 0xff910000 0x0 0x4000>; >> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; >> + clocks = <&cru SCLK_ISP0>, >> + <&cru ACLK_ISP0_WRAPPER>, >> + <&cru HCLK_ISP0_WRAPPER>; >> + clock-names = "isp", "aclk", "hclk"; >> + iommus = <&isp0_mmu>; >> + phys = <&mipi_dphy_rx0>; >> + phy-names = "dphy"; >> + power-domains = <&power RK3399_PD_ISP0>; > > Should this have status = "disabled" too? The mipi_dphy_rx0 node is > disabled by default too, so in the default configuration the driver > would always fail to probe. I'm thinking what is the overall guideline here. Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled? Or since they are only useful if a sensor is present, we should let the dts of the board to enable it? Thanks Helen > > Best regards, > Tomasz >
On 2020-10-14 17:27, Helen Koike wrote: > Hi Tomasz, > > On 9/26/20 10:00 AM, Tomasz Figa wrote: >> Hi Helen, >> >> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: >>> From: Shunqian Zheng <zhengsq@rock-chips.com> >>> >>> RK3399 has two ISPs, but only isp0 was tested. >>> Add isp0 node in rk3399 dtsi >>> >>> Verified with: >>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml >>> >>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> >>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com> >>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>> >>> --- >>> >>> V4: >>> - update clock names >>> >>> V3: >>> - clean up clocks >>> >>> V2: >>> - re-order power-domains property >>> >>> V1: >>> This patch was originally part of this patchset: >>> >>> https://patchwork.kernel.org/patch/10267431/ >>> >>> The only difference is: >>> - add phy properties >>> - add ports >>> --- >>> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ >>> 1 file changed, 25 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>> index dba9641947a3a..ed8ba75dbbce8 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { >>> status = "disabled"; >>> }; >>> >>> + isp0: isp0@ff910000 { >>> + compatible = "rockchip,rk3399-cif-isp"; >>> + reg = <0x0 0xff910000 0x0 0x4000>; >>> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; >>> + clocks = <&cru SCLK_ISP0>, >>> + <&cru ACLK_ISP0_WRAPPER>, >>> + <&cru HCLK_ISP0_WRAPPER>; >>> + clock-names = "isp", "aclk", "hclk"; >>> + iommus = <&isp0_mmu>; >>> + phys = <&mipi_dphy_rx0>; >>> + phy-names = "dphy"; >>> + power-domains = <&power RK3399_PD_ISP0>; >> >> Should this have status = "disabled" too? The mipi_dphy_rx0 node is >> disabled by default too, so in the default configuration the driver >> would always fail to probe. > > I'm thinking what is the overall guideline here. > Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled? > Or since they are only useful if a sensor is present, we should let the dts of the board to > enable it? Yes, the usual pattern is that anything which needs additional hardware outside the SoC to be useful is disabled by default in the SoC DTSI, and enabled in individual board DTSs as appropriate. See USB, HDMI, etc. for instance. There's probably a further debate about how much the board itself should enable if it only breaks out a connector for the user to add their own camera module, but hey, one step at a time ;) Robin.
On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <helen.koike@collabora.com> wrote: > > Hi Tomasz, > > On 9/26/20 10:00 AM, Tomasz Figa wrote: > > Hi Helen, > > > > On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: > >> From: Shunqian Zheng <zhengsq@rock-chips.com> > >> > >> RK3399 has two ISPs, but only isp0 was tested. > >> Add isp0 node in rk3399 dtsi > >> > >> Verified with: > >> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml > >> > >> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> > >> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com> > >> Signed-off-by: Helen Koike <helen.koike@collabora.com> > >> > >> --- > >> > >> V4: > >> - update clock names > >> > >> V3: > >> - clean up clocks > >> > >> V2: > >> - re-order power-domains property > >> > >> V1: > >> This patch was originally part of this patchset: > >> > >> https://patchwork.kernel.org/patch/10267431/ > >> > >> The only difference is: > >> - add phy properties > >> - add ports > >> --- > >> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ > >> 1 file changed, 25 insertions(+) > >> > >> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> index dba9641947a3a..ed8ba75dbbce8 100644 > >> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi > >> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { > >> status = "disabled"; > >> }; > >> > >> + isp0: isp0@ff910000 { > >> + compatible = "rockchip,rk3399-cif-isp"; > >> + reg = <0x0 0xff910000 0x0 0x4000>; > >> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; > >> + clocks = <&cru SCLK_ISP0>, > >> + <&cru ACLK_ISP0_WRAPPER>, > >> + <&cru HCLK_ISP0_WRAPPER>; > >> + clock-names = "isp", "aclk", "hclk"; > >> + iommus = <&isp0_mmu>; > >> + phys = <&mipi_dphy_rx0>; > >> + phy-names = "dphy"; > >> + power-domains = <&power RK3399_PD_ISP0>; > > > > Should this have status = "disabled" too? The mipi_dphy_rx0 node is > > disabled by default too, so in the default configuration the driver > > would always fail to probe. > > I'm thinking what is the overall guideline here. > Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled? > Or since they are only useful if a sensor is present, we should let the dts of the board to > enable it? I don't have a strong opinion. I'm fine with enabling both by default as well, as it shouldn't hurt. That said, I recall some alternative CIF IP block being present on this SoC as well (and patches posted recently), which AFAIR can't be activated at the same time as the ISP, so perhaps both of the alternatives should be disabled by default? Best regards, Tomasz
Thank you Tomasz and Robin for your comments, On 10/14/20 1:43 PM, Tomasz Figa wrote: > On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <helen.koike@collabora.com> wrote: >> >> Hi Tomasz, >> >> On 9/26/20 10:00 AM, Tomasz Figa wrote: >>> Hi Helen, >>> >>> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: >>>> From: Shunqian Zheng <zhengsq@rock-chips.com> >>>> >>>> RK3399 has two ISPs, but only isp0 was tested. >>>> Add isp0 node in rk3399 dtsi >>>> >>>> Verified with: >>>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml >>>> >>>> Signed-off-by: Shunqian Zheng <zhengsq@rock-chips.com> >>>> Signed-off-by: Jacob Chen <jacob2.chen@rock-chips.com> >>>> Signed-off-by: Helen Koike <helen.koike@collabora.com> >>>> >>>> --- >>>> >>>> V4: >>>> - update clock names >>>> >>>> V3: >>>> - clean up clocks >>>> >>>> V2: >>>> - re-order power-domains property >>>> >>>> V1: >>>> This patch was originally part of this patchset: >>>> >>>> https://patchwork.kernel.org/patch/10267431/ >>>> >>>> The only difference is: >>>> - add phy properties >>>> - add ports >>>> --- >>>> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ >>>> 1 file changed, 25 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> index dba9641947a3a..ed8ba75dbbce8 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { >>>> status = "disabled"; >>>> }; >>>> >>>> + isp0: isp0@ff910000 { >>>> + compatible = "rockchip,rk3399-cif-isp"; >>>> + reg = <0x0 0xff910000 0x0 0x4000>; >>>> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; >>>> + clocks = <&cru SCLK_ISP0>, >>>> + <&cru ACLK_ISP0_WRAPPER>, >>>> + <&cru HCLK_ISP0_WRAPPER>; >>>> + clock-names = "isp", "aclk", "hclk"; >>>> + iommus = <&isp0_mmu>; >>>> + phys = <&mipi_dphy_rx0>; >>>> + phy-names = "dphy"; >>>> + power-domains = <&power RK3399_PD_ISP0>; >>> >>> Should this have status = "disabled" too? The mipi_dphy_rx0 node is >>> disabled by default too, so in the default configuration the driver >>> would always fail to probe. >> >> I'm thinking what is the overall guideline here. >> Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled? >> Or since they are only useful if a sensor is present, we should let the dts of the board to >> enable it? > > I don't have a strong opinion. I'm fine with enabling both by default > as well, as it shouldn't hurt. > > That said, I recall some alternative CIF IP block being present on > this SoC as well (and patches posted recently), which AFAIR can't be > activated at the same time as the ISP, so perhaps both of the > alternatives should be disabled by default? > > Best regards, > Tomasz > Based on these two last emails, I think it make sense to disable them by default. I'll just wait for feedback on patch 5/9 to submit an updated version. Thanks Helen
diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi index dba9641947a3a..ed8ba75dbbce8 100644 --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { status = "disabled"; }; + isp0: isp0@ff910000 { + compatible = "rockchip,rk3399-cif-isp"; + reg = <0x0 0xff910000 0x0 0x4000>; + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; + clocks = <&cru SCLK_ISP0>, + <&cru ACLK_ISP0_WRAPPER>, + <&cru HCLK_ISP0_WRAPPER>; + clock-names = "isp", "aclk", "hclk"; + iommus = <&isp0_mmu>; + phys = <&mipi_dphy_rx0>; + phy-names = "dphy"; + power-domains = <&power RK3399_PD_ISP0>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + }; + }; + }; + isp0_mmu: iommu@ff914000 { compatible = "rockchip,iommu"; reg = <0x0 0xff914000 0x0 0x100>, <0x0 0xff915000 0x0 0x100>;