Re: [PATCH v4 4/4] arm64: dts: allwinner: h616: Add BigTreeTech Pi support

[Andre Przywara]

On Tue, 05 Sep 2023 22:45:27 +0200
Jernej Škrabec <jernej.skrabec@xxxxxxxxx> wrote:

Hi,

> On Thursday, August 24, 2023 10:48:20 PM CEST Martin Botka wrote:
> > On Mon, Aug 14 2023 at 12:47:59 PM +02:00:00, Martin Botka
> > 
> > <martin.botka@xxxxxxxxxxxxxx> wrote:  
> > > On Mon, Aug 14 2023 at 12:26:07 PM +02:00:00, Jernej Škrabec
> > > 
> > > <jernej.skrabec@xxxxxxxxx> wrote:  
> > >> Dne nedelja, 13. avgust 2023 ob 18:22:49 CEST je Martin Botka
> > >> 
> > >> napisal(a):  
> > >>>  On Sun, Aug 13 2023 at 05:55:35 PM +02:00:00, Jernej Škrabec
> > >>>  
> > >>>  <jernej.skrabec@xxxxxxxxx> wrote:  
> > >>>  > Dne ponedeljek, 07. avgust 2023 ob 16:53:24 CEST je Martin Botka
> > >>>  > 
> > >>>  > napisal(a):  
> > >>>  >>  The BigTreeTech Pi is an H616 based board based on CB1.
> > >>>  >>  Just in Rpi format board.
> > >>>  >>  
> > >>>  >>  It features the same internals as BTT CB1 but adds:
> > >>>  >>      - Fan port
> > >>>  >>      - IR receiver
> > >>>  >>      - ADXL345 Accelerometer connector via SPI
> > >>>  >>      - 24V DC power supply via terminal plugs
> > >>>  >>      - USB to CAN module connector (The actual USB to CAN  
> > >>> 
> > >>> happens on
> > >>>   
> > >>>  >> the
> > >>>  >> 
> > >>>  >>  external module)
> > >>>  >>  
> > >>>  >>  List of currently working things is same as BTT CB1 but also:
> > >>>  >>      - IR receiver
> > >>>  >>      - ADXL345 connector
> > >>>  >>  
> > >>>  >>  Signed-off-by: Martin Botka <martin@xxxxxxxxxx>
> > >>>  >>  Reviewed-by: Andre Przywara <andre.przywara@xxxxxxx>
> > >>>  >>  ---
> > >>>  >>  
> > >>>  >>  Changes in V2:
> > >>>  >>      - Add UART alongside aliases and chosen for it
> > >>>  >>      - Add model string
> > >>>  >>      - Enable IR receiver
> > >>>  >>      - Enable SPI0 for ADXL345 connector
> > >>>  >>  
> > >>>  >>  Changes in V3:
> > >>>  >>      - Add missing semicolons
> > >>>  >>      - Add pinctrl for SPI0
> > >>>  >>   
> > >>>  >>   arch/arm64/boot/dts/allwinner/Makefile        |  1 +
> > >>>  >>   .../allwinner/sun50i-h616-bigtreetech-pi.dts  | 70
> > >>>  >> 
> > >>>  >> +++++++++++++++++++
> > >>>  >> 
> > >>>  >>   2 files changed, 71 insertions(+)
> > >>>  >>   create mode 100644
> > >>>  >>  
> > >>>  >>  arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-pi.dts
> > >>>  >>  
> > >>>  >>  diff --git a/arch/arm64/boot/dts/allwinner/Makefile
> > >>>  >>  b/arch/arm64/boot/dts/allwinner/Makefile index
> > >>>  >> 
> > >>>  >> 7b386428510b..0b6232a7f328
> > >>>  >> 
> > >>>  >>  100644
> > >>>  >>  --- a/arch/arm64/boot/dts/allwinner/Makefile
> > >>>  >>  +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > >>>  >>  @@ -39,5 +39,6 @@ dtb-$(CONFIG_ARCH_SUNXI) +=
> > >>>  >> 
> > >>>  >> sun50i-h6-pine-h64-model-b.dtb
> > >>>  >> 
> > >>>  >>  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6.dtb
> > >>>  >>  
> > >>>  >>   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-tanix-tx6-mini.dtb
> > >>>  >>   dtb-$(CONFIG_ARCH_SUNXI) +=  
> > >>> 
> > >>> sun50i-h616-bigtreetech-cb1-manta.dtb
> > >>>   
> > >>>  >>  +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-bigtreetech-pi.dtb
> > >>>  >>  
> > >>>  >>   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-orangepi-zero2.dtb
> > >>>  >>   dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h616-x96-mate.dtb
> > >>>  >>  
> > >>>  >>  diff --git
> > >>>  >> 
> > >>>  >> a/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-pi.dts
> > >>>  >> 
> > >>>  >>  b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-pi.dts  
> > >>> 
> > >>> new
> > >>>   
> > >>>  >> file
> > >>>  >> 
> > >>>  >>  mode 100644
> > >>>  >>  index 000000000000..b0d0386e8f13
> > >>>  >>  --- /dev/null
> > >>>  >>  +++  
> > >>> 
> > >>> b/arch/arm64/boot/dts/allwinner/sun50i-h616-bigtreetech-pi.dts
> > >>>   
> > >>>  >>  @@ -0,0 +1,70 @@
> > >>>  >>  +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> > >>>  >>  +/*
> > >>>  >>  + * Copyright (C) 2023 Martin Botka <martin@xxxxxxxxxx>.
> > >>>  >>  + */
> > >>>  >>  +
> > >>>  >>  +/dts-v1/;
> > >>>  >>  +
> > >>>  >>  +#include "sun50i-h616-bigtreetech-cb1.dtsi"
> > >>>  >>  +
> > >>>  >>  +/ {
> > >>>  >>  +	model = "BigTreeTech Pi";
> > >>>  >>  +	compatible = "bigtreetech,pi", "allwinner,sun50i-h616";
> > >>>  >>  +
> > >>>  >>  +	aliases {
> > >>>  >>  +		serial0 = &uart0;
> > >>>  >>  +	};
> > >>>  >>  +
> > >>>  >>  +	chosen {
> > >>>  >>  +		stdout-path = "serial0:115200n8";
> > >>>  >>  +	};
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ehci0 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ehci1 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ehci2 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ehci3 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ir {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ohci0 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ohci1 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ohci2 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&ohci3 {
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&spi0 {
> > >>>  >>  +	/* SPI connection for onboard connector for ADXL345  
> > >>> 
> > >>> accelerometer
> > >>>   
> > >>>  > */
> > >>>  >   
> > >>>  >>  +	status = "okay";
> > >>>  >>  +	pinctrl-names = "default";
> > >>>  >>  +	pinctrl-0 = <&spi0_pins>, <&spi0_cs0_pin>;  
> > >>>  > 
> > >>>  > Driver and compatible for ADXL345 already exists, why don't you  
> > >>> 
> > >>> add
> > >>>   
> > >>>  > child node
> > >>>  > for it?
> > >>>  > 
> > >>>  > Best regards,
> > >>>  > Jernej  
> > >>>  
> > >>>  Ah. So the ADXL345 actually wont be driven by kernel.  
> > >> 
> > >> DT is hardware description, it's not concerned what is done on
> > >> software side,
> > >> either kernel or user space.  
> > > 
> > > Im aware. But this is not a device that is on the board. Its simply a
> > > connector for the device.
> > > Like Rpi has connectors for camera module :)  
> 
> Actually it matters only if this other board is firmly connected with base 
> board. If it is not, then SPI node should be dropped and be handled with DT 
> overlays.

So I do understand the reasoning behind not describing those generic pin
header connectors, where people could hook up anything, and use it as a
GPIO or as a special function device, but just wanted to check on this
situation:
Don't we even describe pins dedicated to those custom connectors,
especially if there is an "obvious" external device to connect to? Is it
the same situation like with these FPC camera connectors?

Cheers,
Andre

> > >>>  The SPI connection is enabled so that klipper (3d printer
> > >>> 
> > >>> firmware) can
> > >>> 
> > >>>  be told to look for ADXL345 at this SPI and use it on its own.
> > >>>  
> > >>>  Klipper will initialize and communicate with the ADXL on its own.  
> > >> 
> > >> What do you mean by firmware? User space app? In this case I suppose
> > >> you'll use
> > >> direct SPI commands from user space? AFAIK that's less and less
> > >> supported by
> > >> kernel (in contrast to I2C).  
> > > 
> > > Firmware as in 3d printer firmware. Klipper runs on the board (CB1 or
> > > BTT Pi) and is indeed an userspace app.
> > > And indeed uses direct SPI commands to the device.
> > > 
> > > The reason for this is the flexibility.
> > > If Klipper read the values from kernel or well from the files the
> > > ADXL driver would create
> > > then it would be unable to communicate with ADXL that is on toolhead
> > > board. Or would have to have
> > > direct initialization either way for those. Thus it just controls the
> > > ADXL itself :)
> > > 
> > > I understand that this may be bit confusing. If there is still
> > > something not clear im more then happy to explain in
> > > full detail how the userspace and 3D printer communicate :)  
> 
> As I said, DT doesn't care about implementation. DT is HW description, so 
> either if it's fixed connection (soldered wires), then describe in full, 
> otherwise it's considered addon board and thus non-essential, so it should be 
> handled with DT overlays.
> 
> Can be CB1 bought separately from 3D printer package and thus used without 
> ADXL sensor?
> 
> > > 
> > > Cheers,
> > > Martin.  
> > 
> > Hello,
> > Jernej any comments on this ?
> > I would like to resolve this conversation :)
> > We do have a bunch of time before the new cycle but never hurts to get
> > it figured out ahead of time :)  
> 
> Sorry, not enough free time over summer.
> 
> Best regards,
> Jernej
> 
> > 
> > Cheers,
> > Martin
> >   
> > >> Best regards,
> > >> Jernej
> > >>   
> > >>>  >>  +};
> > >>>  >>  +
> > >>>  >>  +&uart0 {
> > >>>  >>  +	pinctrl-names = "default";
> > >>>  >>  +	pinctrl-0 = <&uart0_ph_pins>;
> > >>>  >>  +	status = "okay";
> > >>>  >>  +};  
> 
> 
> 
> 
>