Exact.
P.S. Damn forum sw, don’t allow terse answers 
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
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 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
we will see 
sent it out…
1 Like
@ray: @Ryder.Lee posted some comments 
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.
Man who working in Mediatek asking you to replace reverse engineered numbers to names that know Mediatek
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
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 
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 
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 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