Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY

[Johan Jonker <jbx6244@gmail.com>]

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@gmail.com>
> wrote:
> 
>> Thanks Johan for your valuable comments.
>>
>> On Wed, Apr 8, 2020 at 6:19 PM Johan Jonker <jbx6244@gmail.com> 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@gmail.com>
>>>> Signed-off-by: Karthik Poduval <karthik.poduval@gmail.com>
>>>> ---
>>>>  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
>>
> 
>