Ping, gentle reminder, do we have a recommendation on how to deal with
common driver supporting multiple SoC with different types and number
of clocks.
On Thu, Apr 23, 2020 at 4:12 AM Johan Jonker <jbx6244@xxxxxxxxx> wrote:
>
> Hi robh, Heiko, Karthik, Helen and others,
>
> See comments below.
> Should we change Helen's patch serie a little bit to accommodate other
> isp1 compatibles as well? Could you give advise here? Thanks!
>
> Johan
>
>
> On 4/23/20 7:10 AM, karthik poduval wrote:
> > Hi Johan/Helen,
> >
> > I was attempting to fix the yaml to work for both platforms rk3288 and
> > rk3399. I couldn't find any specific example in the existing yaml files
> > that deal with this exact scenario common driver but different clocks for
> > different chipsets. Could you point me to an appropriate example ?
> >
> > Meanwhile here is the patch I was trying out.
> > diff --git a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > index af246b71eac6..4ca76a1bbb63 100644
> > --- a/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > +++ b/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> > @@ -15,7 +15,11 @@ description: |
> >
> > properties:
> > compatible:
> > - const: rockchip,rk3399-cif-isp
> > + items:
> > + - enum:
> > + - rockchip,rk3288-cif-isp
> > + - rockchip,rk3399-cif-isp
> > + - const: rockchip,rk3399-cif-isp
> >
> > reg:
> > maxItems: 1
> > @@ -37,20 +41,38 @@ properties:
> > const: dphy
> >
>
> > clocks:
> > - items:
> > - - description: ISP clock
> > - - description: ISP AXI clock clock
> > - - description: ISP AXI clock wrapper clock
> > - - description: ISP AHB clock clock
> > - - description: ISP AHB wrapper clock
> > + oneOf:
> > + # rk3399 clocks
> > + - items:
> > + - description: ISP clock
> > + - description: ISP AXI clock clock
> > + - description: ISP AXI clock wrapper clock
> > + - description: ISP AHB clock clock
> > + - description: ISP AHB wrapper clock
> > + # rk3288 clocks
> > + - items:
> > + - description: ISP clock
> > + - description: ISP AXI clock clock
> > + - description: ISP AHB clock clock
> > + - description: ISP Pixel clock
> > + - description: ISP JPEG source clock
> >
>
> We can expect a few more clocks here, so just use:
>
> clocks:
> minItems: 4
> maxItems: 5
>
> or
>
> See question for Helen about 'pclk_isp_wrap':
>
> clocks:
> minItems: 4
> maxItems: 6
>
>
> From Rockchip tree:
>
> static const char * const rk1808_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3288_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp_in",
> "sclk_isp_jpe",
> };
>
> static const char * const rk3326_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3368_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "pclk_isp",
> };
>
> static const char * const rk3399_isp_clks[] = {
> "clk_isp",
> "aclk_isp",
> "hclk_isp",
> "aclk_isp_wrap",
> "hclk_isp_wrap",
> "pclk_isp_wrap"
> };
>
> Question for Helen:
>
> Where did 'pclk_isp_wrap' go in your patch serie?
>
> > clock-names:
> > - items:
> > - - const: clk_isp
> > - - const: aclk_isp
> > - - const: aclk_isp_wrap
> > - - const: hclk_isp
> > - - const: hclk_isp_wrap
> > + oneOf:
> > + # rk3399 clocks
>
> sort on SoC
>
> > + - items:
> > + - const: clk_isp
> > + - const: aclk_isp
> > + - const: aclk_isp_wrap
> > + - const: hclk_isp
> > + - const: hclk_isp_wrap
> > + # rk3288 clocks
>
> sort on SoC
>
> > + - items:
> > + - const: clk_isp
> > + - const: aclk_isp
> > + - const: hclk_isp
> > + - const: pclk_isp_in
> > + - const: sclk_isp_jpe
>
> clock-names:
> oneOf:
> # rk3288 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: pclk_isp_in
> description: ISP Pixel clock
> - const: sclk_isp_jpe
> description: ISP JPEG source clock
> # rk3399 clocks
> - items:
> - const: clk_isp
> description: ISP clock
> - const: aclk_isp
> description: ISP AXI clock clock
> - const: aclk_isp_wrap
> description: ISP AXI clock wrapper clock
> - const: hclk_isp
> description: ISP AHB clock clock
> - const: hclk_isp_wrap
> description: ISP AHB wrapper clock
>
> Question for robh:
> Is this a proper way to add description or is there a beter way?
>
> >
> > on running command.
> > make ARCH=arm dtbs_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >
> > I see following messages for dts nodes.
> > DTC arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> > CHECK arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml
> > /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> > isp@ff910000: clocks: [[7, 107], [7, 205], [7, 469], [7, 371], [7, 108]] is
> > valid under each of {'additionalItems': False, 'items': [{}, {}, {}, {},
> > {}], 'maxItems': 5, 'minItems': 5, 'type': 'array'}, {'additionalItems':
> > False, 'items': [{}, {}, {}, {}, {}], 'maxItems': 5, 'minItems': 5, 'type':
> > 'array'}
> > /home/kpoduval/workspace/tinkerboard-yocto/build/tmp/work/tinker_board-poky-linux-gnueabi/linux-stable/5.5.7+gitAUTOINC+ceab3ac1e6-r0/linux-tinker_board-standard-build/arch/arm/boot/dts/rk3288-firefly-beta.dt.yaml:
> > isp@ff910000: compatible: ['rockchip,rk3288-cif-isp'] is too short
> >
> > Are these cosidered as error messages ? The "too short" is being alerted
> > for the orgianl yaml for rk3399 without any of my chnages.
> > Kindly advise.
> >
> > --
> > Regards,
> > Karthik Poduval
> >
> > On Sat, Apr 11, 2020 at 10:13 PM karthik poduval <karthik.poduval@xxxxxxxxx>
> > wrote:
> >
> >> Thanks Johan for your valuable comments.
> >>
> >> On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@xxxxxxxxx> wrote:
> >>>
> >>> Hi Karthik and others,
> >>>
> >>> Include all mail lists found with:
> >>> ./scripts/get_maintainer.pl --nogit-fallback --nogit
> >>>
> >>> Helen is moving isp1 bindings out of staging.
> >>> Clocks and other things don't fit with her patch serie.
> >>> Maybe fix this while still in staging?
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'phys' is a required property
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'phy-names' is a required property
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'ports' is a required property
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> 'assigned-clock-rates', 'assigned-clocks'
> >>> do not match any of the regexes: 'pinctrl-[0-9]+'
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:2: 'aclk_isp_wrap' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:3: 'hclk_isp' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: isp@ff910000:
> >>> clock-names:4: 'hclk_isp_wrap' was expected
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: 'power-domains'
> >>> is a required property
> >>>
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:1:
> >>> 'dphy-cfg' was expected
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clock-names:
> >>> ['dphy-ref', 'pclk'] is too short
> >>> arch/arm/boot/dts/rk3288-tinker.dt.yaml: mipi-phy-rx0: clocks: [[7,
> >>> 126], [7, 358]] is too short
> >>>
> >>>
> >>> Inside yaml:
> >>> Use enum and sort.
> >>>>> properties:
> >>>>> compatible:
> >>>
> >>>>> const: rockchip,rk3399-cif-isp
> >>>>> + const: rockchip,rk3288-rkisp1
> >>>
> >>> enum:
> >>> - rockchip,rk3288-rkisp1
> >>> - rockchip,rk3399-cif-isp
> >>>
> >>>>> properties:
> >>>>> compatible:
> >>>>> const: rockchip,rk3399-mipi-dphy-rx0
> >>>>> + const: rockchip,rk3288-mipi-dphy-rx0
> >>>
> >>> enum:
> >>> - rockchip,rk3288-mipi-dphy-rx0
> >>> - rockchip,rk3399-mipi-dphy-rx0
> >>>
> >>>>
> >>>> Please, keep consistency, or rk3288-cif-isp, or we change
> >> rk3399-cif-isp to be rk3399-rkisp1.
> >>>
> >>>
> >>> On 4/6/20 9:30 AM, Karthik Poduval wrote:
> >>>> ISP and DPHY device entries missing so add them.
> >>>>
> >>>
> >>>> tested on tinkerbaord with ov5647 using command
> >>>> cam -c 1 -C -F cap
> >>>
> >>> Disclose dts node for ov5647 in cover letter, so people can reproduce
> >>> this experiment.
> >>>
> >>> Caution!
> >>> Without dts node this command doesn't work correct.
> >>>
> >>> make ARCH=arm dtbs_check
> >>>
> >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> >>>
> >>> make ARCH=arm dtbs_check
> >>>
> >> DT_SCHEMA_FILES=Documentation/devicetree/bindings/phy/rockchip-mipi-dphy-rx0.yaml
> >>>
> >>> Needed to detect missing: phys, phy-names and ports ,etc.
> >>>
> >>> &isp {
> >>> status = "okay";
> >>> };
> >>>
> >> Makes sense, that's why my kernel compilation passed and I didn't
> >> these errors. Thanks for the command, I will verify dts for
> >> correctness in next patch series.
> >>
> >>> Needed to detect missing: power-domains, etc.
> >>>
> >>> &mipi_phy_rx0 {
> >>> status = "okay";
> >>> };
> >>>
> >>>>
> >>>> Reported-by: Karthik Poduval <karthik.poduval@xxxxxxxxx>
> >>>> Signed-off-by: Karthik Poduval <karthik.poduval@xxxxxxxxx>
> >>>> ---
> >>>> arch/arm/boot/dts/rk3288.dtsi | 25 +++++++++++++++++++++++++
> >>>> 1 file changed, 25 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/boot/dts/rk3288.dtsi
> >> b/arch/arm/boot/dts/rk3288.dtsi
> >>>> index 9beb662166aa..adea8189abd9 100644
> >>>> --- a/arch/arm/boot/dts/rk3288.dtsi
> >>>> +++ b/arch/arm/boot/dts/rk3288.dtsi
> >>>> @@ -247,6 +247,23 @@
> >>>> ports = <&vopl_out>, <&vopb_out>;
> >>>> };
> >>>>
> >>>
> >>>> + isp: isp@ff910000 {
> >>>
> >>> For nodes:
> >>> Sort things without reg alphabetical first,
> >>> then sort the rest by reg address.
> >>>
> >> Sure
> >>>> + compatible = "rockchip,rk3288-rkisp1";
> >>>> + reg = <0x0 0xff910000 0x0 0x4000>;
> >>>> + interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> >>>> + clocks = <&cru SCLK_ISP>, <&cru ACLK_ISP>,
> >>>> + <&cru HCLK_ISP>, <&cru PCLK_ISP_IN>,
> >>>> + <&cru SCLK_ISP_JPE>;
> >>>> + clock-names = "clk_isp", "aclk_isp",
> >>>> + "hclk_isp", "pclk_isp_in",
> >>>> + "sclk_isp_jpe";
> >>>> + assigned-clocks = <&cru SCLK_ISP>, <&cru SCLK_ISP_JPE>;
> >>>> + assigned-clock-rates = <400000000>, <400000000>;
> >>>
> >>>> + power-domains = <&power RK3288_PD_VIO>;
> >>>> + iommus = <&isp_mmu>;
> >>>
> >>> sort
> >>>
> >>> Something missing?
> >>> Where are the ports and port nodes?
> >>
> >> I was assuming that this would be a part of the board specific dtsi or
> >> dts entry where the isp node would be overriden and the i2c camera
> >> port
> >> and the isp port remote-endpoints would be connected. I had this as a
> >> part of my first patch series. However I was advised by Helen to not
> >> include the ov5647
> >> dtsi as it isn't hardwired to the SoC and resides on an camera adapter
> >> board.
> >>
> >> Should this be defined without the remote-endpoint phandle since we
> >> don't know exactly which sensor gets connected in this dtsi file ?
> >>
> >>>
> >>>> + status = "disabled";
>
> Question for Heiko:
> Should we add status to Helen's serie as well?
>
> >>>> + };
> >>>> +
> >>>> sdmmc: mmc@ff0c0000 {
> >>>> compatible = "rockchip,rk3288-dw-mshc";
> >>>> max-frequency = <150000000>;
> >>>> @@ -891,6 +908,14 @@
> >>>> status = "disabled";
> >>>> };
> >>>>
> >>>
> >>>> + mipi_phy_rx0: mipi-phy-rx0 {
> >>>
> >>> Use separate patch.
> >>>
> >>> For nodes:
> >>> Sort things without reg alphabetical first,
> >>> then sort the rest by reg address.
> >>>
> >> Sure
> >>
> >>>> + compatible = "rockchip,rk3288-mipi-dphy-rx0";
> >>>> + clocks = <&cru SCLK_MIPIDSI_24M>, <&cru
> >> PCLK_MIPI_CSI>;
> >>>> + clock-names = "dphy-ref", "pclk";
> >>> Something missing?
> >>> Does this phy have a power domain?
> >> The tinkerboard debian kernel (where I referred to for this patch)
> >> didn't have it defined for the DPHY. I would guess that it would be
> >> the same as the ISP i.e. RK3288_PD_VIO,
> >> does anyone know the answer to this or do I have to reach out to
> >> Rockchip engineering ?
> >>>
> >>>> + #phy-cells = <0>;
> >>>> + status = "disabled";
> >>>> + };
> >>>> +
> >>>> io_domains: io-domains {
> >>>> compatible = "rockchip,rk3288-io-voltage-domain";
> >>>> status = "disabled";
> >>>>
> >>>
> >>
> >>
> >> --
> >> Regards,
> >> Karthik Poduval
> >>
> >
> >
>
--
Regards,
Karthik Poduval
