Add latest U-boot support for BPI R2 & BPI R64 (not yet)

Exact.

P.S. Damn forum sw, don’t allow terse answers :slight_smile:

have squashed the fixes, but still some messages reported by checkpatch…

[10:20:18]$ scripts/checkpatch.pl 0003-pci-mediatek-Add-pci-driver-for-mt2701.patch 
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#43: 
new file mode 100644

WARNING: externs should be avoided in .c files
#187: FILE: drivers/pci/pci-mt2701.c:140:
+int mtk_pcie_probe(void);

CHECK: Macro argument reuse 'p' - possible side-effects?
#193: FILE: drivers/pci/pci-mt2701.c:146:
+#define mtk_foreach_port(p)		\
+		for ((p) = mtk_pcie_port;	\
+		(p) != &mtk_pcie_port[ARRAY_SIZE(mtk_pcie_port)]; (p)++)

CHECK: Macro argument reuse 'n' - possible side-effects?
#196: FILE: drivers/pci/pci-mt2701.c:149:
+#define BITS(m, n)   (~(BIT(m) - 1) & ((BIT(n) - 1) | BIT(n)))

CHECK: if this code is redundant consider removing it
#410: FILE: drivers/pci/pci-mt2701.c:363:
+#if 0

total: 0 errors, 2 warnings, 3 checks, 524 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

0003-pci-mediatek-Add-pci-driver-for-mt2701.patch has style problems, please review.

NOTE: Ignored message types: COMPLEX_MACRO CONSIDER_KSTRTO MINMAX MULTISTATEMENT_MACRO_USE_DO_WHILE NETWORKING_BLOCK_COMMENT_STYLE PREFER_ETHER_ADDR_COPY USLEEP_RANGE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

content of mtk_pcie_set_usb_pcie_co_phy is not used (#if 0 … #endif), we should remove it, right?

the second warning should be placed in a header-file, right…which one to use?

btw. volatile-changes work as expected…

and we need a description for “ata: ahci: Don’t forget to clear upper address regs.”

Imho “external” (forward declaration) not needed because first access is after complete definition…seems to be needed

drivers/pci/pci-mt2701.c:412:1: warning: function declaration isn’t a prototype [-Wstrict-prototypes]
 mtk_pcie_probe()
 ^~~~~~~~~~~~~~

but maybe i can fix this by defining it static like the other functions

for the 2 reuse: first one (mtk_foreach_port) should change the param (pointer to mtk_pcie_port) inside the define to use it outside. second one (BITS) imho only calculate something without modifying arguments, so both no side-effects

last one i guess is only used if usb needed (over pcie)…so maybe future…but i don’t know if i should remove it for posting or let it deactivated by “#if 0”…whats your opinion @ray?

mtk_pcie_probe used nowhere outside of driver module. Just make it static.

#if 0 … #endif can be removed

then i can remove complete function and its call in mtk_pcie_probe() like this i’ve pushed now

we can add it later if we got usb working

@ray you ack this and ignoring the 2 “Macro argument reuse” ?

then i can remove complete function and its call in mtk_pcie_probe() like this i've pushed now

No, it is used in that module, just no external use. Just add static before int mtk_pcie_probe

we can add it later if we got usb working

ok

@ray you ack this and ignoring the 2 "Macro argument reuse" ?

yup, in first case it have to. in second case it’s ok.

I meant remove mtk_pcie_set_usb_pcie_co_phy completely like i’ve done in last commit because it will be empty if i remove code surrounded by “if 0”

any description for the "ata: ahci: Don’t forget to clear upper address regs. "? seems to be only relevant if 64bit is used…have now added one, hoping it is right :slight_smile:

Ahh, ok.

Noooo, it relevant only in 32bits mode.

In 64bits mode you have to put upper address part.

In 32bits mode you have to set upper bits to 0, otherwise controller will try to DMA into not existing memory and stops with error.

https://github.com/frank-w/u-boot/commits/2019-07-bpi-r2-sata_new

ready for submission :slight_smile: anything i should write in coverletter else why we want to add this and that it depends on ryders-patches for hifsys?

Subject: [PATCH 0/4] add pcie/ahci for mt7623/bpi-r2

with this Patchseries i want to add ahci-support (sata) for bananapi-r2

pci-driver may support other devices too using mt2701 for pcie, but in first step pcie is only activated for mt7623 (dts)

Patches depend on hifsys-patches from ryder.lee:

https://patchwork.ozlabs.org/project/uboot/list/?series=121987

also applies cleanly on 2019-10-rc1 (with ryders Patches first)

just a detail…shouldn’t i remove the commented lines in dts?

Yup, remove them.

Anyway them useless, until we replace to original DTS.

all ok, including cover-letter? then i send it out…

Nobody punish us for that :slight_smile:

we will see :stuck_out_tongue: sent it out…

1 Like

@ray: @Ryder.Lee posted some comments :wink: can you try to change it according to it?

first one is easy…replace defines directly by the readX/writeX…but the others (remove magic numbers or put something from code to dts is imho bit tricky)

i try to fix what i can…but i cannot decode magic-numbers, move things from code to dts or rewrite driver to match other devices based on linux-driver/tphy

Looks funny. :slight_smile:

Man who working in Mediatek asking you to replace reverse engineered numbers to names that know Mediatek :slight_smile:

i think we will need his help for changing this, we cannot rewrite the driver to state of linux’ one

but i made a basic change to access the dts :slight_smile:

now we can access the dts by functions defined in ofnode.h

cute

Extract “ranges” and use them instead of defines @Ryder.Lee said.

But I like to left defines, and use them if you have problem extracting FDT data.

is there any example/description how to decode the ranges the right way? There seem to be a helper function:

https://elixir.bootlin.com/u-boot/latest/source/common/fdt_support.c#L1591

But the address-params are still unclear to me and how to get the right index

board/freescale/qemu-ppce500/qemu-ppce500.c should help

Basicly use fdt_getprop…but how to the right index? The file seems to decode to physical adresses, but i need the values like they are listed…

Btw. Ryder posted some code for pcie driver,but i’ve not found access to dts yet :frowning:

Seems to be this:

pcie->base = dev_remap_addr_name(dev, "subsys");

similar call for Ports (snprintf(…“port%d”, slot) as second param).

have added the Patch in my repo

compiles without warnings, but hangs on pci enum till watchdog resets the board…

at a first check it seems that this is not successful:

266     dev_for_each_subnode(subnode, dev) {
267         struct fdt_pci_addr addr;
268         u32 slot = 0;
269 printk(KERN_ALERT "DEBUG: Passed %s %d \n",__FUNCTION__,__LINE__); //this is printed 3 times (for each pci-port)
270         if (!ofnode_is_available(subnode))
271             continue;
272
273 printk(KERN_ALERT "DEBUG: Passed %s %d \n",__FUNCTION__,__LINE__); //this printk isn't called => !ofnode_is_available(subnode)
274
275         err = ofnode_read_pci_addr(subnode, 0, "reg", &addr);

as far as i see, each port needs to have status-field with “okay”…and i see that is disabled in my dts :smiley:

after fixed this (should not reset the board…only print nothing found or similar) i hang here:

221 printk(KERN_ALERT "DEBUG: Passed %s %d \n",__FUNCTION__,__LINE__); //gets printed
222     err = reset_get_by_index(dev, slot, &port->reset);
223     if (err)
224         return err;
225 printk(KERN_ALERT "DEBUG: Passed %s %d \n",__FUNCTION__,__LINE__); //gets not printed
226     err = generic_phy_get_by_index(dev, slot, &port->phy);

after getting the reset-values printed, i see that is not set (0,0,0), seems we need to get reset-values first

should be

reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";

in pci-controller-root-node. do we need ./drivers/reset/reset-mediatek.c here or only read the dts-property into the reset-struct?

seems there is something missing in dts…i see reset-names but no “resets”, this property was disabled by // in old dtsi :slight_smile: i try to add it back…strange after i added them back i got no message on pci enum…only a watchdog-reset after some time

Oh, look at that driver: https://elixir.bootlin.com/u-boot/latest/source/drivers/pci/pci-rcar-gen3.c

Looks like regions automatically parsed. So you need just:

struct pci_controller *hose = dev_get_uclass_priv(dev);
hose->regions[0].phys_start - as MEM region base
hose->regions[1].phys_start - as IO region base