Help to review: porting Mainline U-boot and Linux to Tmall MagicBox M16S (Amlogic S912-H Tv Box)

[chewitt]

The v5 series has no vendor binding. See here:

 

1. Add the "tmall" vendor binding: https://github.com/chewitt/linux/commit/35fd1261ccd8ef0fae50e8dde26f728685f80a44

2. Add the "magicbox-m16s" board binding: https://github.com/chewitt/linux/commit/907be915c59639ecb58b1504880ec0c59956cd3b

3. Add the board dts with the tmall,magicbox-m16s compatible: https://github.com/chewitt/linux/commit/d6a0784dfce8e704cecf5c6f7fe7d46d7642b81a

 

You can do the vendor as "magicbox" and board as "m16s" if you prefer, but the number of patches and sequence should be the same. NB: As the purpose of the vendor binding is to get the true name of the manufacturer, they are often something a bit obscure, e.g. Beelink are actually "Shenzen AZW" so their binding is "azw" not "beelink" .. I couldn't find anything online about this box though - but only looking in English language sources, so if it was a Chinese market box I'm probably looking in the wrong language.

[ning]

it's full chinese name is "天猫魔盒 M16S”

天猫 is Tmall, 魔盒 is magicbox. 

 

Alibaba is manufactor, tmall magicbox is product line name, m16s product name.

 

if follow manufactor,product-line_product-name this will be too long in this case. so I choose a short one.

 

 

[chewitt]

Some comments (based on v7):

 

a) The sdio_pwrdeq node is in the wrong place in the dts, it should be in the initial indented section, see https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gx-p23x-q20x.dtsi#L94-L99 - it is not a top-level node.

 

b) I'll say again (third or fourth time now) .. the vendor binding MUST be submitted in its own patch first, THEN you submit the board binding separately in its own patch. THEN you submit the board dts in its own patch. Do not combine the vendor and board bindings into a single patch.

 

c) You have added "Signed-off-by: Sean Young <sean@mess.org>" to the RC patch. Sean is the RC subsystem maintainer so he made a couple of comments. He is not an author of the patch and I see no claim of authorship in his comments. Comments are not substantial changes requiring a share of authorship so you are the single author and only your SOB is required. Sean will provide a Reviewed-By tag once he reviews it. NB: Searn normally he does a single review of all pending patches per kernel dev cycle (around rc-6 stage) to act/review them, so this patch will sit unanswered for a while - be patient - but you need to submit it to the correct mailing list first.

 

d) The RC patch should be submitted separatedly and versioned separately from the board bindings/dts as this patch is (should be) submitted to a different mail-list list/subsystem with different maintainers (linux-media, Sean) from the Amlogic SOC list/subsystem (Neil/Kevin/Martin). The RC patches are now v2 or v3 patches. I don't see the latest patches on the linux-media list. NB: Note that linux-media maintainers use a different patchwork host https://patchwork.linuxtv.org/project/linux-media/list/ if you want to mark old patches superseded.

 

e) Use ./scripts/get_maintainer.pl /path/to/your.patch to get the list of all people for the To/Cc fields when you submit. I personally use some aliases in ~/.gitconfig to avoid re-typing all the addresses out when I use git send, so I have git send-amlogic and git send-rc to make life simple.

[ning]

errr... sorry, I don't understand. upstream patch is so painful. this is the second try. much better than 1st. my 1st try no body reply my patch, even later days I found same patch with different author in kernel.

[chewitt]

It's normal Everyone makes "etiquette" mistakes with first submissions and "learns the hard way" (me too).

 

I can help you two ways (choose one):

 

a) Push your patches to a GitHub repo - then I can review them to point out the mistakes and offer suggestions or corrections.

 

b) I can replicate the patches and fixup the mistakes - then you can cherry-pick the patches and resubmit them.

[ning]

On 7/27/2022 at 11:20 AM, chewitt said:

a) The sdio_pwrdeq node is in the wrong place in the dts, it should be in the initial indented section, see https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gx-p23x-q20x.dtsi#L94-L99 - it is not a top-level node

 

I don't understand this part, could you explain it more? what is top-level node?

in my mind, if a node has phandle, I can modify it in any place.

if you can add a patch to your github, this will help a lot.

 

 

I already sent a email only to Sean ask how to handle RC keymap patch.

 

if this patch is already accepted, I will send new version without this patch.

new version will have 1 dts patch following 3 bindings patches, 2 with Acked-by.

 

I plan to:

add bt/wifi node to indicate bt wifi is ready, and comments about what missing in these nodes.

(from sd8897 bindings, missing interrupt pin in soc side, meson gpio doesn't support interrup mode, and wake-up pin in device side, no impact to main function)

add LED node quoted in "/* */" and comments to let others to know why LED is not enabled.

(from info from Neil, need more patch to enable GPIO_TEST_N)

in dts patch, not seprate patches.

 

do you agree?

 

I want to wait Sean's email for some days then new version.

 

 

[chewitt]

Using the GXBB 'WeTek Hub' as an example:

 

This is the top (indented) section: https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts#L12-L50

This is the main 'body' of the dts: https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gxbb-wetek-hub.dts#L51-L58

 

This is the default sdio_pwrseq node (inherited from include): https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gx-p23x-q20x.dtsi#L94-L99  - it is in the top/indented section. So if you add the node to your board dts (to override a property) the node should be in the same top/indented section, not the 'body' area.

 

The device-tree file can only describe hardware that has driver support. If you add nodes for not-working LED items or WiFi the patch will be rejected. Once the required drivers are added to the kernel or fixed, or GPIO configs are figured out, then you can send a patch to add those nodes/items. NB: It is technically wrong to add the ir-node referencing an RC driver that does not exist (yet) in the kernel .. but nobody cares about IR remotes so that will be allowed

 

I will push the RC driver and board dts patches to branches in my repo .. I will share links when done.

[ning]

&sd_emmc_a {
        mwifiex@1 {
                compatible = "marvell,sd8897";
                reg = <1>;
        };

        btmrvl@2 {
                compatible = "marvell,sd8897-bt";
                reg = <2>;
        };
};

 

 

this is Wifi/Bt nodes I plan to add, and wifi/bt are work with these nodes. because other properties are optional.

 

 

 

 

 

[ning]

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/marvell-bt-8xxx.txt?h=v5.19-rc8

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt?h=v5.19-rc8

 

missing interrupts and marvell,wakeup-pin,  

 

interrupt pins are GPIOX_7 for wifi, GPIOX_17 for bt.

 

this is infor from vendor OS:

 

gpio-241 (sdio_wifi ) in lo GPIOX_7

gpio-251 (bt_rfkill ) in lo   GPIOX_17 ( the name is bt_rfkill, but its direction is in, thus it is interrupt.

 

but wakeup-pin is device side pin to wakeup soc, this is unknown.

 

as said in bindings file, these are optional, wifi/bt are still work without them. but can't fill them with wrong data.

 

 

[chewitt]

WiFi is an SDIO device, but BT is serial (UART) - and the GPIOX_17 is the same as other GXL/GXM devices. Does this work? (replacing the sd_emmc_a node)

 

/* This is connected to the Bluetooth module: */
&uart_A {
	status = "okay";
	pinctrl-0 = <&uart_a_pins>, <&uart_a_cts_rts_pins>;
	pinctrl-names = "default";
	uart-has-rtscts;

	bluetooth {
		compatible = "marvell,sd8897-bt";
		reg = <2>;
		shutdown-gpios = <&gpio GPIOX_17 GPIO_ACTIVE_HIGH>;
	};
};

 

Edited July 28, 2022 by chewitt

[ning]

no, according its bindings file, it's SDIO device or USB device, no UART device type.

 

I think I need postpone wifi/bt node, because, I need to enable gpio_intc, I think it may different to default value. need time.

 

 

[chewitt]

Most WiFi/BT chips are available in two variants; one for SDIO (with BT as a serial/uart device) and one for USB. I guarantee the box is not SDIO/USB on the same chip. All other GXL/GXM boxes are using serial BT devices on GPIOX_17 so the box is based on the Amlogic reference design (like all GXL/GXM Android boxes) and we just need to figure out the correct way to represent this in device-tree.

 

Please share the Android dtb file - if you have it.

[ning]

I don't have any line of code. just have the box.

 

here is an example of this wifi/bt module. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi?h=v5.19-rc8#n402

 

 

[ning]

gpio-241 (sdio_wifi ) in lo GPIOX_7  

 

interrupt-parent = <&gpio_intc>;
           interrupts = <96 IRQ_TYPE_EDGE_RISING>; //89 + 7; GPIOX irq number from 89.. 

 

gpio-251 (bt_rfkill ) in lo   GPIOX_17 ( the name is bt_rfkill, but its direction is in, thus it is interrupt.

interrupt-parent = <&gpio_intc>;
           interrupts = <106 IRQ_TYPE_EDGE_RISING>;

 

both gpios are normal low, thus rising edge for irq.

 

 

and Vendor OS log:

<6>[ 0.538183] c0 1 (swapper/0) aml_wifi wifi.33: [wifi_dev_probe] interrupt_pin=241

<6>[ 0.538190] c0 1 (swapper/0) aml_wifi wifi.33: [wifi_dev_probe] irq_num=100, irq_trigger_type=1

 

take me sometime to understand gpio to irq number....

 

now left device side wakeup-pin.... this is too difficult.

[ning]

for 

11 hours ago, chewitt said:

This is the default sdio_pwrseq node (inherited from include): https://github.com/torvalds/linux/blob/master/arch/arm64/boot/dts/amlogic/meson-gx-p23x-q20x.dtsi#L94-L99  - it is in the top/indented section. So if you add the node to your board dts (to override a property) the node should be in the same top/indented section, not the 'body' area.

 

is below correct?

 

/ {
        compatible = "tmall,magicbox-m16s", "amlogic,s912", "amlogic,meson-gxm";
        model = "Tmall MagicBox M16S";

        gpio-keys {
                compatible = "gpio-keys-polled";
                poll-interval = <100>;

                key-power {
                        label = "power";
                        linux,code = <KEY_POWER>;
                        gpios = <&gpio_ao GPIOAO_2 GPIO_ACTIVE_LOW>;
                };
        };
                  
       sdio_pwrseq :emmc-pwrseq {
           reset-gpios = <&gpio GPIODV_2 GPIO_ACTIVE_LOW>;
       };
};

 

 

 

[chewitt]

Yes, that's correct.

 

I have created board changes in this branch: https://github.com/chewitt/linux/commits/m16s-board

and the RC changes are in this branch: https://github.com/chewitt/linux/commits/m16s-rc

 

I created two branches because patches should be generated against the 'next' version of the subsystem you are submitting to. For board patches this is the v5.20/arm64-dt branch here: https://git.kernel.org/pub/scm/linux/kernel/git/amlogic/linux.git and for the RC patches this is the master branch of https://git.linuxtv.org/media_tree.git

 

I normally combine RC bindings and driver changes into a single patch. This is technically wrong (bindings should be separate) but as the binding and driver will be accepted by the same maintainer (Sean) through the same tree (Linux Media) combining makes review easier and in the past, nobody has complained about me doing this. However, as you already submitted bindings/driver in two patches I kept them as two patches for continuity. I made minor changes to the driver to move the inline comment to the header. I would not ask for Sean to Signoff again as this is a minor cosmetic change - I would simply make a note below the --- in the patch. I also revised the subject for the binding to match historic submissions .. "dt-bindings: rc: description" appears to be for structural changes to the RC bindings file, and "media: dt-bindings: rc: description" is used for adding new remotes to the bindings file. it's confusing

 

I added the WiFi changes with some /* commented */ items - it's not clear to me if these are optional/required?

 

** !!! DO NOT (RE)SUBMIT !!! **

 

Amlogic maintainers will not merge new  board patches for Linux 5.21 until approximately Linux 5.20-rc5/rc6 timeframe (in 2.5 months time) so there is lots of time to investigate the board and get more things working (LEDs, Keys, etc.) before submitting the next series. The RC patches can be resent earlier.

 

For now, keep working on the board changes. We can discuss how and where to send the board/rc patches in future posts!

Edited July 29, 2022 by chewitt

[ning]

Thank you for telling about Amlogic maintainer will not merge new boards. I have more time for more investigation on vendor OS to find more.

 

could I add Co-develeped-by you in furture patches?

[chewitt]

1 minute ago, ning said:

could I add Co-develeped-by you in furture patches?

 

Not needed. It's your board and you're doing all the real work. I'm just helping with minor formating ... but thanks

[ning]

I suddenly understand why they don't accept new board patches, normally 5.19 would be release, and 5.20 merge window would be opened, and only fix would be accept. but due intel silicon bug, everything are delayed.

[chewitt]

Yes, upstreaming is all about timing and the process is deliberately not quick, to avoid mistakes. Anything you submit now will be for Linux v5.22 as the content for Linux v5.21 is already fixed and queued waiting for v5.20 to be released and the v5.21 merge window to open. Only fixes are handled quickly, so there's no rush

[ning]

U-boot.bin

boot.scr

meson-gxm-magicbox-m16s.dtb

u-boot.bin boot.scr meson-gxm-magicbox-m16s.dtb

[fundon]

Hi @ning, I sent an email to you. Do you have time to help and guide me in installing the system?