Message-ID: <8319b665-dd0d-36fc-999d-f4ae1afbd38b@rock-chips.com>
Date:   Thu, 29 Jun 2017 08:14:53 +0800
From:   Shawn Lin <shawn.lin@...k-chips.com>
To:     Klaus Goger <klaus.goger@...obroma-systems.com>
Cc:     shawn.lin@...k-chips.com, Mark Rutland <mark.rutland@....com>,
        devicetree@...r.kernel.org,
        Heiko Stübner <heiko@...ech.de>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        linux-kernel@...r.kernel.org, kever.yang@...k-chips.com,
        linux-rockchip@...ts.infradead.org,
        Rob Herring <robh+dt@...nel.org>,
        Philipp Tomsich <philipp.tomsich@...obroma-systems.com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 4/4] arm64: dts: add RK3399-Q7 (Puma) SoM

Hi

On 2017/6/28 22:01, Klaus Goger wrote:
> Hi Shawn,
> 
>> On 28 Jun 2017, at 14:41, Shawn Lin <shawn.lin@...k-chips.com> wrote:
>>

--8<-----------

>>>
>>>> +&pcie0 {
>>>> +	ep-gpios = <&gpio4 RK_PC6 GPIO_ACTIVE_LOW>;
>>>> +	num-lanes = <4>;
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&pcie_clkreqn>;
>>>> +	status = "okay";
>>>
>>> vpcie{3v3, 1v8, 0v9}-supply properties?
>>
>> These supply are optional which depend on the HW design.
>> But pcie_clkreqn doesn't really work. I think we should
>> use pcie_clkreqn_cpm instead and fix this for rk3399-evb.dts
>> to prevent the further copy-n-paste.
> 
> I could add the supply properties but since they where optional and
> are generated by dedicated always-on regulators supplied from the
> also always on regulator vcc3v3_sys I didn’t see the need for it.
> But if anyone to have a full model of the power tree visible in the
> devicetree even if not controllable I can add it in v2.

Ok, so you don't need to add these here. :)

> 
> Since the properties are optional maybe we should also change
> the dev_info "no xxx regulator found” in pcie-rockchip.c to something
> less severe sounding.
> 
>>>> +};
>>>> +
>>>
>>> [...]
>>>
>>>> +&sdmmc {
>>>> +	clock-frequency = <150000000>;
>>
>> we deprecates this now, so please use max-frequency instead.
>>
>>>> +	bus-width = <4>;
>>>> +	cap-mmc-highspeed;
>>>> +	cap-sd-highspeed;
>>>> +	cd-gpios = <&gpio0 RK_PA7 GPIO_ACTIVE_LOW>;
>>
>> Really this board use gpio-based card detect pin?
>>
>>>> +	wp-gpios = <&gpio0 RK_PB5 GPIO_ACTIVE_LOW>;
>>>> +	num-slots = <1>;
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&sdmmc_clk &sdmmc_cmd &sdmmc_gpio &sdmmc_bus4>;
>>
>> I guess you don't need sdmmc_gpio and let mmc core request
>> this gpio as irq pin from parsing cd-gpios?
> 
> I tried to just use the PA7 as SDMMC0_DET but didn’t get any state changes when

Hmm.. we use PA7 as SDMMC0_DET for vendor kernel 4.4.
I guess something goes wrong here and will check if later.

> removing it or pugging it in after bootup. I tried to follow the mmc code path to see
> which of the 3 card detection modes get configured. It looked to me as the CDETECT
> register gets used but this would not generate any interrupts and requires polling of the
> register. So not using a a gpio card detect requires the broken-cd property for me.
> If i overlooked something I’m happy to change it, I was planning to take a look at it later
> anyways
> 
>>
>>>> +	status = "okay";
>>>> +};
>>
>> And I would be more happy here to see the present of vqmmc and vmmc
>> supply if possible.
> 
> Since we are a SoM vmmc would be a property required to be provided from the baseboard
> and not part of the module dts.
> I will add vqmmc for the I/O voltage supplying APIO#

good to know that.

> 
>>>> +
>>>> +
>>>
>>> double empty line
>>>
>>>
>>>> +&spi1 {
>>>> +	status = "okay";
>>>> +
>>>> +	flash: norflash@0 {
>>>
>>> norflash: flash@0 maybe?
>>>
>>> You reference the phandle and at the position it gets referenced
>>> the specific name might be more helpful.
>>>
>>>
>>>> +		compatible = "jedec,spi-nor";
>>>> +		reg = <0>;
>>>> +		spi-max-frequency = <50000000>;
>>>> +	};
>>>> +};
>>>
>>>
>>>
>>>> +&pinctrl {
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&puma_pin_hog>;
>>>> +
>>>> +	hog {
>>>> +			puma_pin_hog: puma_pin_hog {
>>>
>>> puma_pin_hog: puma-pin-hog
>>>
>>> Same for more defined pinctrl nodes below that.
>>>
>>>
>>> Heiko
>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@...ts.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip
>>>
>>
>>
>> -- 
>> Best Regards
>> Shawn Lin
> 
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
> 

Powered by blists - more mailing lists