07.04.2020 13:41, Thierry Reding пишет:
> On Mon, Apr 06, 2020 at 10:41:05PM +0300, Dmitry Osipenko wrote:
> [...]
>> diff --git a/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts b/arch/arm/boot/dts/tegra20-acer-a500-picasso.dts
> [...]
>> + host1x@50000000 {
>> + dc@54200000 {
>> + rgb {
>> + status = "okay";
>> + nvidia,panel = <&panel>;
>> +
>> + port@0 {
>> + lvds_output: endpoint {
>> + remote-endpoint = <&lvds_encoder_input>;
>> + bus-width = <18>;
>> + };
>> + };
>> + };
>> + };
>
> This seems a little strange to me, though, admittedly, I've never worked
> with these types of bridges before, so I may be misunderstanding this. I
> was under the impression that we could obtain the panel by traversing an
> OF graph, so that we didn't have to have that extra nvidia,panel
> property. As it is, you seem to describe two different paths, one that
> goes from the RGB output to the panel directly, and another that goes
> from the RGB output to the LVDS encoder and then to the panel.
>
> It doesn't seem to me like a direct link from RGB output to panel does
> actually exist in this setup.
AFAIK, the direct link doesn't exist on any of Tegra boards, they all
have an LVDS bridge. The older device-trees just didn't model it properly.
The LVDS bridge and panel-lvds are relatively new things in DRM, which
allow to model hardware more correctly, like for example the bridge's
powerdown control is now modeled properly.
The nvidia,panel is a mandatory property for the Tegra's DRM output,
panel won't light up without it. I guess it should be possible to get
the panel's phandle from the graph, but this is not supported by the
Tegra's DRM driver + nvidia,panel is also useful to have for older
kernels that do not support panel-lvds. The panel falls back to a
simple-panel in the case of older kernel version, which results in a not
entirely appropriate panel timing (wrong framerate), but this is okay'ish.
> [...]
>> + pwm: pwm@7000a000 {
>> + status = "okay";
>> + power-supply = <&vdd_3v3_sys>;
>> + };
>
> I don't see power-supply defined as a property for the PWM controller.
> Why do you need this?
Yes, looks like it's not needed. I'll remove it in v3, thanks.
> [...]
>> + sdhci@c8000000 {
>> + status = "okay";
>> +
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + max-frequency = <25000000>;
>> + keep-power-in-suspend;
>> + bus-width = <4>;
>> + non-removable;
>> +
>> + mmc-pwrseq = <&brcm_wifi_pwrseq>;
>> + vmmc-supply = <&vdd_3v3_sys>;
>> + vqmmc-supply = <&vdd_3v3_sys>;
>> +
>> + /* Azurewave AW-NH611 BCM4329 */
>> + WiFi@1 {
>
> I think these names are supposed to be lowercase.
The dtbs_check doesn't complain, should be fine :)
But I don't mind, although the camel-case should be a correct way of
spelling WiFi. I'll change it in v3.
>> + reg = <1>;
>> + compatible = "brcm,bcm4329-fmac";
>> + interrupt-parent = <&gpio>;
>> + interrupts = <TEGRA_GPIO(S, 0) IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "host-wake";
>> + };
>> + };
> [...]
>> + clocks {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + clk32k_in: clock@0 {
>> + compatible = "fixed-clock";
>> + reg = <0>;
>> + #clock-cells = <0>;
>> + clock-frequency = <32768>;
>> + clock-output-names = "tps658621-out32k";
>> + };
>> +
>> + rtc_32k_wifi: clock@1 {
>> + compatible = "fixed-clock";
>> + reg = <1>;
>> + #clock-cells = <0>;
>> + clock-frequency = <32768>;
>> + clock-output-names = "kk3270032";
>> + };
>> + };
>
> Are these clocks going to the PMIC and RTC, or are they generated by the
> chips? If they are generated by the chips, which sounds like they might
> be, wouldn't it be better to represent them as children of the
> corresponding chips?
They are generated by the chips.
The PMIC has a built-in 32K oscillator.
The kk3270032 is a dedicated onboard 32K oscillator. This one is not
mandatory to model in the device-tree, but I wanted to model as much as
possible.
> There's probably no infrastructure to do this, so maybe that would be
> overkill.
Yes, PMIC doesn't model the clock. All Tegra boards model the PMIC's
clock this way, although those boards don't set the clock-output-names
property, which makes the DT model more obscure than it could be.
The kk3270032 is a standalone oscillator, so it's fine as-is already.
> But for clarity it might be worth documenting here where
> exactly these clocks come from.
I guess the output clock-output-names are already self-explanatory,
don't you think so? For more details you could always consult the
board's schematics.
> [...]
>> + memory-controller@7000f400 {
>> + nvidia,use-ram-code;
>> +
>> + emc-tables@elpida-8gb {
>
> I don't think unit-addresses are supposed to be freeform text like
> above. These should always reflect the value of the "reg" property,
> though in this case we don't have one...
>
>> + nvidia,ram-code = <0>;
>
> In retrospect it might have been better to just reuse the reg property
> for this.
>
> I think in this case it might be best to reflect the RAM code in the
> unit-address. At least that way we conceptually get things right since
> it's the RAM code that selects which of these tables is used, much like
> a register, I2C slave address, or SPI chip select would select which of
> the subdevices are targetted.
This could be done, although we already have a precedent in a form of
the paz00 board that got memory timings not so long time ago.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=834f1d6cf3647e804e7a80569e42ee7fbee50eb1
I'll change it in v3.
Thank you for the review.
