mbox series

[v3,0/4] arm64: rockchip: Pine64 PineTab2 support

Message ID 20240102-pinetab2-v3-0-cb1aa69f8c30@mecka.net
Headers show
Series arm64: rockchip: Pine64 PineTab2 support | expand

Message

Manuel Traut Jan. 2, 2024, 4:15 p.m. UTC
This adds support for the BOE TH101MB31IG002 LCD Panel used in PineTab2 [1] and
PineTab-V [2] as well as the devictrees for the PineTab2 v0.1 and v2.0.

The BOE LCD Panel patch was retrieved from [3]. The function-name prefix has
been adapted and the LCD init section was simplified.

The PineTab2 devicetree patch was retrieved from [4]. Some renaming was needed
to pass the dtb-checks, the brightness-levels are specified as range and steps
instead of a list of values.

[5] and [6] was also used as source for this queue.

[1] https://wiki.pine64.org/wiki/PineTab2
[2] https://wiki.pine64.org/wiki/PineTab-V
[3] https://salsa.debian.org/Mobian-team/devices/kernels/rockchip-linux/-/blob/mobian-6.6/debian/patches/display/0018-drm-panel-add-BOE-TH101MB31IG002-28A-driver.patch?ref_type=heads
[4] https://salsa.debian.org/Mobian-team/devices/kernels/rockchip-linux/-/blob/mobian-6.6/debian/patches/device-tree/0134-arch-arm64-add-Pine64-PineTab2-device-trees.patch?ref_type=heads
[5] https://github.com/dreemurrs-embedded/linux-pinetab2/tree/v6.6.7-danctnix1
[6] https://xff.cz/git/linux?h=pt2-6.7

Signed-off-by: Manuel Traut <manut@mecka.net>
---
Changes in v3:
- PineTab2 dts:
    * Remove useless regulator-state-mem nodes for fixed regulators
    * Swap mmc0 and mmc1, so mmc0 is now the internal eMMC
- BOE TH101MB31IG002 LCD Panel:
    * Remove enabled/prepared checks since they are done in core.
- Use consistent naming (PineTab2 and PineTab-V) in commit messages.
- Link to v2: https://lore.kernel.org/r/20231223-pinetab2-v2-0-ec1856d0030e@mecka.net

Changes in v2:
- Removed dtb-checker fixups, cause I am not sure if they are correct
- Applied review comments for dt bindings
- pinetab2 dts:
    * Remove unverified WLAN entries, as in [5]
    * Simplify flash LED definition, as in [5]
    * Fix headphone detection and sound routing, as in [5]
    * Fix CRU clock configuration
- BOE TH101MB31IG002 LCD Panel:
    * Reworked prepare/enable unprepare/disable, as in [5]
- Replaced nicknames by realnames in author and signed-offs

- Link to v1: https://lore.kernel.org/r/20231222-pinetab2-v1-0-e148a7f61bd1@mecka.net

---
Alexander Warnecke (2):
      drm/panel: Add driver for BOE TH101MB31IG002-28A panel
      arm64: dts: rockchip: Add devicetree for Pine64 PineTab2

Manuel Traut (2):
      dt-bindings: display: panel: Add BOE TH101MB31IG002-28A panel
      dt-bindings: arm64: rockchip: Add Pine64 PineTab2

 .../devicetree/bindings/arm/rockchip.yaml          |   8 +
 .../display/panel/boe,th101mb31ig002-28a.yaml      |  58 ++
 arch/arm64/boot/dts/rockchip/Makefile              |   2 +
 .../boot/dts/rockchip/rk3566-pinetab2-v0.1.dts     |  26 +
 .../boot/dts/rockchip/rk3566-pinetab2-v2.0.dts     |  46 +
 arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi  | 959 +++++++++++++++++++++
 drivers/gpu/drm/panel/Kconfig                      |  11 +
 drivers/gpu/drm/panel/Makefile                     |   1 +
 .../gpu/drm/panel/panel-boe-th101mb31ig002-28a.c   | 322 +++++++
 9 files changed, 1433 insertions(+)
---
base-commit: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
change-id: 20231222-pinetab2-faa77e01db6f

Best regards,

Comments

Dang Huynh Jan. 3, 2024, 4:09 a.m. UTC | #1
On Tuesday, January 2, 2024 6:07:56 PM UTC Ondřej Jirman wrote:
> On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > +&pcie2x1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pcie_reset_h>;
> > +	reset-gpios = <&gpio1 RK_PB2 GPIO_ACTIVE_HIGH>;
> > +	vpcie3v3-supply = <&vcc3v3_minipcie>;
> > +	status = "okay";
> > +};
> 
> Does it make sense to enable this HW block by default, when it isn't used on
> actual HW?
> 

PCI-E is hooked up to a connector in the schematics, so I think it make sense 
to enable it when there's one available.
Jonas Karlman Jan. 3, 2024, 2:19 p.m. UTC | #2
Hi Manuel,

On 2024-01-03 14:40, Manuel Traut wrote:
> Hi Jonas and Ondřej,
> 
>>>> +&sfc {
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&fspi_dual_io_pins>;
>>>> +	status = "okay";
>>>> +	#address-cells = <1>;
>>>> +	#size-cells = <0>;
>>>> +
>>>> +	flash@0 {
>>>> +		compatible = "jedec,spi-nor";
>>>> +		reg = <0>;
>>>> +		spi-max-frequency = <24000000>;
>>>
>>> That's a bit on the low side. The flash chip should work for all commands up to
>>> 80MHz https://megous.com/dl/tmp/b428ad9b85ac4633.png and SGM3157YC6 switch
>>> for the FSPI-CLK should have high enough bandwidth, too.
>>
>> I agree that this is a little bit on the low side, it was a safe rate
>> that I used for U-Boot. U-Boot required an exact rate of the supported
>> sfc clk rates: 24, 50, 75, 100, 125 or 150 MHz.
>>
>> Please also note that the SPI NOR flash chip used in PineTab2 is not a
>> GigaDevice GD25LQ128E, it should be a SiliconKaiser SK25LP128, same as
>> found in the Pine64 PinePhone Pro.
> 
> The schematics for v2.0 reference a GD25LQ128EWIGR. I never checked the jedec
> id. How did you retrieve this information, or is it maybe a difference in v0.1
> and 2.0?

This was when working on mainline U-Boot for the PineTab2 (and other
rk356x devices). See [1] for a pending U-Boot patch that is waiting on a
proper mainline linux devicetree for the PT2.

The JEDEC ID is reported as 0x257018 on my v2.0 production unit, and
does not match the JEDEC ID for GD25LQ128E (0xc86018) referenced in
the schematics.

I found that the JEDEC ID 0x257018 was referenced in prior patches
related to the SK25LP128 SPI NOR flash chip used in Pine64 PinePhone Pro.

I have only ever tested the 24 MHz rate, but I am expecting that e.g.
100 MHz also should work. Will not be able to test on my PT2 until at
earliest next week.

[1] https://github.com/Kwiboo/u-boot-rockchip/commit/66562d6eaf2c11a9f97fcdba379d3ceda8aa70ef

Regards,
Jonas

> 
>>>> +		spi-rx-bus-width = <2>;
>>>
>>> GD25LQ128E supports quad I/O. Maybe try 4 if it will work.
>>
>> The schematic only shows fspi D0 and D1 connected, and use the D2 line
>> for eMMC_RSTn, so spi-rx-bus-width = <2> should be correct.
> 
> ack
> 
> Since it is only needed for bootloader updates and environment its maybe better
> to stay on the safe side?
> 
> But I can test faster frequency if you want me to do..
> 
> Regards
> Manuel
Manuel Traut Jan. 5, 2024, 4:11 p.m. UTC | #3
On Wed, Jan 03, 2024 at 10:42:54AM +0100, Ondřej Jirman wrote:
> Hello Manuel,
> 
> a few more things I noticed:
> 
> On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > From: Alexander Warnecke <awarnecke002@hotmail.com>
> > 
> > +	leds {
> > +		compatible = "gpio-leds";
> > +
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&flash_led_en_h>;
> > +
> > +		led-0 {
> > +			gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
> > +			color = <LED_COLOR_ID_WHITE>;
> > +			function = LED_FUNCTION_FLASH;
> > +		};
> 
> This LED is supplied by VCC5V_MIDU, so maybe this should be a regulator-led
> supplied by gpio (FLASH_LED_EN_H) controlled regulator-fixed named f_led which
> is in turn supplied by VCC5V_MIDU.
> 
> https://megous.com/dl/tmp/9bf0d85d78946b5e.png

regulator-leds are controlled by turning on or off the regulator. However
VCC5V_MIDU is also used by other devices (USB, HDMI, ..) so I guess this is
not what we want. I would keep it as is.

> > +	};
> > +
> > [...]
> >
> > +
> > +	speaker_amp: speaker-amplifier {
> > +		compatible = "simple-audio-amplifier";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&spk_ctl>;
> > +		enable-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_HIGH>;
> > +		sound-name-prefix = "Speaker Amplifier";
> > +		VCC-supply = <&vcc_bat>;
> > +	};
> > +
> > +	vcc_3v3: vcc-3v3 {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_3v3";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc3v3_sys>;
> > +	};
> > +
> > +	vcc3v3_minipcie: vcc3v3-minipcie {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio4 RK_PC3 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pcie_pwren_h>;
> > +		regulator-name = "vcc3v3_minipcie";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc_sys>;
> 
> This regulator is supplied by vcc_bat: https://megous.com/dl/tmp/4ec71a4a2aea9498.png

correct, I will update this in v4.

> > +	};
> > +
> > +	vcc3v3_sd: vcc3v3-sd {
> > +		compatible = "regulator-fixed";
> > +		gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&sdmmc_pwren_l>;
> > +		regulator-name = "vcc3v3_sd";
> > +		regulator-min-microvolt = <3300000>;
> > +		regulator-max-microvolt = <3300000>;
> > +		vin-supply = <&vcc3v3_sys>;
> > +	};
> > +
> > +	vcc5v0_usb_host0: vcc5v0-usb-host0 {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio4 RK_PC4 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&usb_host_pwren1_h>;
> > +		regulator-name = "vcc5v0_usb_host0";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc5v_midu>;
> > +	};
> > +
> > +	vcc5v0_usb_host2: vcc5v0-usb-host2 {
> > +		compatible = "regulator-fixed";
> > +		enable-active-high;
> > +		gpio = <&gpio4 RK_PC5 GPIO_ACTIVE_HIGH>;
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&usb_host_pwren2_h>;
> > +		regulator-name = "vcc5v0_usb_host2";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		vin-supply = <&vcc5v_midu>;
> > +	};
> > +
> > +	vcc_bat: vcc-bat {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_bat";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +	};
> > +
> > +	vcc_sys: vcc-sys {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vcc_sys";
> > +		regulator-always-on;
> > +		regulator-boot-on;
> > +		vin-supply = <&vcc_bat>;
> > +	};
> > +
> > +	vdd1v2_dvp: vdd1v2-dvp {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "vdd1v2_dvp";
> > +		regulator-min-microvolt = <1200000>;
> > +		regulator-max-microvolt = <1200000>;
> > +		vin-supply = <&vcc_3v3>;
> > +		/*enable-supply = <&vcc2v8_dvp>;*/
> > +	};
> 
> There's no vdd1v2_dvp in the schematic on the camera sensor connector, or elsewhere:
>
>   https://megous.com/dl/tmp/fd95f003d8f3fbfb.png

It is on page 5 in the power diagram on the right top.

> So I guess, you can drop this, entirely. Maybe it's VDD1V5_DVP but I don't think
> it needs to be described in DT, since it's pretty local to this camera sensor,
> and nothing else uses it.
> 
>   https://megous.com/dl/tmp/7fc384e196c5428f.png

dvdd-supply is a required property of the ov5648 camera, so I would tend to keep
this. But us vcc_sys for vin-supply instead of vcc_3v3.

Regards
Manuel
Manuel Traut Jan. 5, 2024, 4:46 p.m. UTC | #4
Hi Jonas,

On Wed, Jan 03, 2024 at 03:19:25PM +0100, Jonas Karlman wrote:
> Hi Manuel,
> 
> On 2024-01-03 14:40, Manuel Traut wrote:
> > Hi Jonas and Ondřej,
> > 
> >>>> +&sfc {
> >>>> +	pinctrl-names = "default";
> >>>> +	pinctrl-0 = <&fspi_dual_io_pins>;
> >>>> +	status = "okay";
> >>>> +	#address-cells = <1>;
> >>>> +	#size-cells = <0>;
> >>>> +
> >>>> +	flash@0 {
> >>>> +		compatible = "jedec,spi-nor";
> >>>> +		reg = <0>;
> >>>> +		spi-max-frequency = <24000000>;
> >>>
> >>> That's a bit on the low side. The flash chip should work for all commands up to
> >>> 80MHz https://megous.com/dl/tmp/b428ad9b85ac4633.png and SGM3157YC6 switch
> >>> for the FSPI-CLK should have high enough bandwidth, too.
> >>
> >> I agree that this is a little bit on the low side, it was a safe rate
> >> that I used for U-Boot. U-Boot required an exact rate of the supported
> >> sfc clk rates: 24, 50, 75, 100, 125 or 150 MHz.
> >>
> >> Please also note that the SPI NOR flash chip used in PineTab2 is not a
> >> GigaDevice GD25LQ128E, it should be a SiliconKaiser SK25LP128, same as
> >> found in the Pine64 PinePhone Pro.
> > 
> > The schematics for v2.0 reference a GD25LQ128EWIGR. I never checked the jedec
> > id. How did you retrieve this information, or is it maybe a difference in v0.1
> > and 2.0?
> 
> This was when working on mainline U-Boot for the PineTab2 (and other
> rk356x devices). See [1] for a pending U-Boot patch that is waiting on a
> proper mainline linux devicetree for the PT2.
> 
> The JEDEC ID is reported as 0x257018 on my v2.0 production unit, and
> does not match the JEDEC ID for GD25LQ128E (0xc86018) referenced in
> the schematics.
> 
> I found that the JEDEC ID 0x257018 was referenced in prior patches
> related to the SK25LP128 SPI NOR flash chip used in Pine64 PinePhone Pro.
> 
> I have only ever tested the 24 MHz rate, but I am expecting that e.g.
> 100 MHz also should work. Will not be able to test on my PT2 until at
> earliest next week.
> 
> [1] https://github.com/Kwiboo/u-boot-rockchip/commit/66562d6eaf2c11a9f97fcdba379d3ceda8aa70ef

Thanks for the information.

My v2.0 device also reports JEDEC ID 0x257018. I increased the clock-rate
to 100 MHz and it is at least still detected.

I will find out how to test more, currently hexdump /dev/mtd0 just reports
0xff on the hole flash, I expected to see u-boot there..

Regards
Manuel

> >>>> +		spi-rx-bus-width = <2>;
> >>>
> >>> GD25LQ128E supports quad I/O. Maybe try 4 if it will work.
> >>
> >> The schematic only shows fspi D0 and D1 connected, and use the D2 line
> >> for eMMC_RSTn, so spi-rx-bus-width = <2> should be correct.
> > 
> > ack
> > 
> > Since it is only needed for bootloader updates and environment its maybe better
> > to stay on the safe side?
> > 
> > But I can test faster frequency if you want me to do..
> > 
> > Regards
> > Manuel
>
Ondřej Jirman Jan. 5, 2024, 4:48 p.m. UTC | #5
On Fri, Jan 05, 2024 at 05:11:03PM +0100, Manuel Traut wrote:
> On Wed, Jan 03, 2024 at 10:42:54AM +0100, Ondřej Jirman wrote:
> > Hello Manuel,
> > 
> > a few more things I noticed:
> > 
> > On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > > From: Alexander Warnecke <awarnecke002@hotmail.com>
> > > 
> > > +	leds {
> > > +		compatible = "gpio-leds";
> > > +
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&flash_led_en_h>;
> > > +
> > > +		led-0 {
> > > +			gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
> > > +			color = <LED_COLOR_ID_WHITE>;
> > > +			function = LED_FUNCTION_FLASH;
> > > +		};
> > 
> > This LED is supplied by VCC5V_MIDU, so maybe this should be a regulator-led
> > supplied by gpio (FLASH_LED_EN_H) controlled regulator-fixed named f_led which
> > is in turn supplied by VCC5V_MIDU.
> > 
> > https://megous.com/dl/tmp/9bf0d85d78946b5e.png
> 
> regulator-leds are controlled by turning on or off the regulator. However
> VCC5V_MIDU is also used by other devices (USB, HDMI, ..) so I guess this is
> not what we want. I would keep it as is.

It's used by the LED. gpio-leds will not ensure it's on when you enable the LED.

In practice this may only come up if someone tries to save power by unloading
dwc3 USB driver, when using PT2 outside of the keyboard case. Otherwise
VCC5V_MIDU will be enabled by DWC3 driver's use of PHY API.

In any case, I'm not saying you should use VCC5V_MIDU directly in regulator-led,
but as a vin-supply to a new regulator-fixed node (which would be describing
this "fixed voltage regulator" https://megous.com/dl/tmp/cc65ec81ab9af163.png ).

> > > +	};
> > > +
> > > [...]
> > >
> > > +
> > > +	speaker_amp: speaker-amplifier {
> > > +		compatible = "simple-audio-amplifier";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&spk_ctl>;
> > > +		enable-gpios = <&gpio4 RK_PC2 GPIO_ACTIVE_HIGH>;
> > > +		sound-name-prefix = "Speaker Amplifier";
> > > +		VCC-supply = <&vcc_bat>;
> > > +	};
> > > +
> > > +	vcc_3v3: vcc-3v3 {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_3v3";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc3v3_sys>;
> > > +	};
> > > +
> > > +	vcc3v3_minipcie: vcc3v3-minipcie {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC3 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pcie_pwren_h>;
> > > +		regulator-name = "vcc3v3_minipcie";
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc_sys>;
> > 
> > This regulator is supplied by vcc_bat: https://megous.com/dl/tmp/4ec71a4a2aea9498.png
> 
> correct, I will update this in v4.
> 
> > > +	};
> > > +
> > > +	vcc3v3_sd: vcc3v3-sd {
> > > +		compatible = "regulator-fixed";
> > > +		gpio = <&gpio0 RK_PA5 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&sdmmc_pwren_l>;
> > > +		regulator-name = "vcc3v3_sd";
> > > +		regulator-min-microvolt = <3300000>;
> > > +		regulator-max-microvolt = <3300000>;
> > > +		vin-supply = <&vcc3v3_sys>;
> > > +	};
> > > +
> > > +	vcc5v0_usb_host0: vcc5v0-usb-host0 {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC4 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&usb_host_pwren1_h>;
> > > +		regulator-name = "vcc5v0_usb_host0";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		vin-supply = <&vcc5v_midu>;
> > > +	};
> > > +
> > > +	vcc5v0_usb_host2: vcc5v0-usb-host2 {
> > > +		compatible = "regulator-fixed";
> > > +		enable-active-high;
> > > +		gpio = <&gpio4 RK_PC5 GPIO_ACTIVE_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&usb_host_pwren2_h>;
> > > +		regulator-name = "vcc5v0_usb_host2";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		vin-supply = <&vcc5v_midu>;
> > > +	};
> > > +
> > > +	vcc_bat: vcc-bat {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_bat";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +	};
> > > +
> > > +	vcc_sys: vcc-sys {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vcc_sys";
> > > +		regulator-always-on;
> > > +		regulator-boot-on;
> > > +		vin-supply = <&vcc_bat>;
> > > +	};
> > > +
> > > +	vdd1v2_dvp: vdd1v2-dvp {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "vdd1v2_dvp";
> > > +		regulator-min-microvolt = <1200000>;
> > > +		regulator-max-microvolt = <1200000>;
> > > +		vin-supply = <&vcc_3v3>;
> > > +		/*enable-supply = <&vcc2v8_dvp>;*/
> > > +	};
> > 
> > There's no vdd1v2_dvp in the schematic on the camera sensor connector, or elsewhere:
> >
> >   https://megous.com/dl/tmp/fd95f003d8f3fbfb.png
> 
> It is on page 5 in the power diagram on the right top.

That (Power diagram overview) is irrelevant part of the schematic. Can be and
often is wrong. You need to use actual detailed parts of the schematic, which
is what is actually used to route the PCB traces.

kind regards,
	o.

> > So I guess, you can drop this, entirely. Maybe it's VDD1V5_DVP but I don't think
> > it needs to be described in DT, since it's pretty local to this camera sensor,
> > and nothing else uses it.
> > 
> >   https://megous.com/dl/tmp/7fc384e196c5428f.png
> 
> dvdd-supply is a required property of the ov5648 camera, so I would tend to keep
> this. But us vcc_sys for vin-supply instead of vcc_3v3.
> 
> Regards
> Manuel
Neil Armstrong Jan. 12, 2024, 9:25 a.m. UTC | #6
From: Neil Armstrong <neil.armstrong@linaro.org>

Hi,

On Tue, 02 Jan 2024 17:15:43 +0100, Manuel Traut wrote:
> This adds support for the BOE TH101MB31IG002 LCD Panel used in PineTab2 [1] and
> PineTab-V [2] as well as the devictrees for the PineTab2 v0.1 and v2.0.
> 
> The BOE LCD Panel patch was retrieved from [3]. The function-name prefix has
> been adapted and the LCD init section was simplified.
> 
> The PineTab2 devicetree patch was retrieved from [4]. Some renaming was needed
> to pass the dtb-checks, the brightness-levels are specified as range and steps
> instead of a list of values.
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)

[1/4] dt-bindings: display: panel: Add BOE TH101MB31IG002-28A panel
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=baae3a0b10c499d4228514a701602f6fd2a8d6b4
[2/4] drm/panel: Add driver for BOE TH101MB31IG002-28A panel
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=420186db1483da4e16cd5d5a472f511a35dbc1b7
Manuel Traut Jan. 26, 2024, 8:30 p.m. UTC | #7
Hello Ondřej,

On Fri, Jan 05, 2024 at 05:48:46PM +0100, Ondřej Jirman wrote:
> On Fri, Jan 05, 2024 at 05:11:03PM +0100, Manuel Traut wrote:
> > On Wed, Jan 03, 2024 at 10:42:54AM +0100, Ondřej Jirman wrote:
> > > Hello Manuel,
> > > 
> > > a few more things I noticed:
> > > 
> > > On Tue, Jan 02, 2024 at 05:15:47PM +0100, Manuel Traut wrote:
> > > > From: Alexander Warnecke <awarnecke002@hotmail.com>
> > > > 
> > > > +	leds {
> > > > +		compatible = "gpio-leds";
> > > > +
> > > > +		pinctrl-names = "default";
> > > > +		pinctrl-0 = <&flash_led_en_h>;
> > > > +
> > > > +		led-0 {
> > > > +			gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
> > > > +			color = <LED_COLOR_ID_WHITE>;
> > > > +			function = LED_FUNCTION_FLASH;
> > > > +		};
> > > 
> > > This LED is supplied by VCC5V_MIDU, so maybe this should be a regulator-led
> > > supplied by gpio (FLASH_LED_EN_H) controlled regulator-fixed named f_led which
> > > is in turn supplied by VCC5V_MIDU.
> > > 
> > > https://megous.com/dl/tmp/9bf0d85d78946b5e.png
> > 
> > regulator-leds are controlled by turning on or off the regulator. However
> > VCC5V_MIDU is also used by other devices (USB, HDMI, ..) so I guess this is
> > not what we want. I would keep it as is.
> 
> It's used by the LED. gpio-leds will not ensure it's on when you enable the LED.
> 
> In practice this may only come up if someone tries to save power by unloading
> dwc3 USB driver, when using PT2 outside of the keyboard case. Otherwise
> VCC5V_MIDU will be enabled by DWC3 driver's use of PHY API.
> 
> In any case, I'm not saying you should use VCC5V_MIDU directly in regulator-led,
> but as a vin-supply to a new regulator-fixed node (which would be describing
> this "fixed voltage regulator" https://megous.com/dl/tmp/cc65ec81ab9af163.png ).

Sorry for the late response, I was busy with other things in the last weeks.

I changed it to be a regulator led and will post a v4 soon.
Manuel Traut Jan. 27, 2024, 9:31 a.m. UTC | #8
Hi Jonas,

On Wed, Jan 03, 2024 at 03:19:25PM +0100, Jonas Karlman wrote:
> Hi Manuel,
> 
> On 2024-01-03 14:40, Manuel Traut wrote:
> > Hi Jonas and Ondřej,
> > 
> >>>> +&sfc {
> >>>> +	pinctrl-names = "default";
> >>>> +	pinctrl-0 = <&fspi_dual_io_pins>;
> >>>> +	status = "okay";
> >>>> +	#address-cells = <1>;
> >>>> +	#size-cells = <0>;
> >>>> +
> >>>> +	flash@0 {
> >>>> +		compatible = "jedec,spi-nor";
> >>>> +		reg = <0>;
> >>>> +		spi-max-frequency = <24000000>;
> >>>
> >>> That's a bit on the low side. The flash chip should work for all commands up to
> >>> 80MHz https://megous.com/dl/tmp/b428ad9b85ac4633.png and SGM3157YC6 switch
> >>> for the FSPI-CLK should have high enough bandwidth, too.
> >>
> >> I agree that this is a little bit on the low side, it was a safe rate
> >> that I used for U-Boot. U-Boot required an exact rate of the supported
> >> sfc clk rates: 24, 50, 75, 100, 125 or 150 MHz.
> >>
> >> Please also note that the SPI NOR flash chip used in PineTab2 is not a
> >> GigaDevice GD25LQ128E, it should be a SiliconKaiser SK25LP128, same as
> >> found in the Pine64 PinePhone Pro.
> > 
> > The schematics for v2.0 reference a GD25LQ128EWIGR. I never checked the jedec
> > id. How did you retrieve this information, or is it maybe a difference in v0.1
> > and 2.0?
> 
> This was when working on mainline U-Boot for the PineTab2 (and other
> rk356x devices). See [1] for a pending U-Boot patch that is waiting on a
> proper mainline linux devicetree for the PT2.
> 
> The JEDEC ID is reported as 0x257018 on my v2.0 production unit, and
> does not match the JEDEC ID for GD25LQ128E (0xc86018) referenced in
> the schematics.
> 
> I found that the JEDEC ID 0x257018 was referenced in prior patches
> related to the SK25LP128 SPI NOR flash chip used in Pine64 PinePhone Pro.
> 
> I have only ever tested the 24 MHz rate, but I am expecting that e.g.
> 100 MHz also should work. Will not be able to test on my PT2 until at
> earliest next week.
> 
> [1] https://github.com/Kwiboo/u-boot-rockchip/commit/66562d6eaf2c11a9f97fcdba379d3ceda8aa70ef

I found the time to verify that 100 MHz is also working.
Will include this in v4

Thanks for your help
Manuel