Hi Denis,
Thanks for the review! I'll prepare another revision incorporating your
comments.
Some responses to your questions below:
On 9/14/20 9:16 PM, Denis 'GNUtoo' Carikli wrote:
+#define N_TOC_ENTRIES 5 /* My herolte has exactly five interesting
entries. */ +/* TODO: Is there some rule for figuring out how many
valid entries
+ * there are in the TOC from the TOC itself? */
Since we now know the TOC structure thanks to your work, we could
simply deduce that with the TOC size:
- On your device the TOC has an entry for itself
- On the devices we support that have a TOC, it's probably possible to
deduce the TOC size from the location of the first partition.
This seems reasonable. We could also guess that reading 512 bytes is
enough, and process entries up to the first blank entry.
It'd be nice to find some way of deciding whether a TOC entry refers to
the MODEM partition or to a file in /efs, e.g. the nvram file. But until
then I suppose it'd be fine to have each specific model list the names
and orders of the TOC entries it expects rather than trying to fully
automate it.
+ /* cbd acquires the "ss310" wakelock here. TODO: Should we do
+ * the same? */
So this comment can be removed.
Excellent, OK.
+ /* ipc_client_log(client, "Powering off modem"); */
+ /* if (xmm626_kernel_smdk4412_power(client, boot0_fd, 0) <
0) */
+ /* goto exit; */
If the cbd daemon does that, you could convert that to a comment
telling that, and point that what the cbd does is useless in this case
Sorry, it was in there during debugging and I mistakenly left it in. I
will remove it.
This part looks strange: on all the other devices we support, we
load the NV modem partition inside the modem, but here you load the
nv_data.bin instead, is it intentional? With the stock RIL/cbd, is the
nv_data.bin really loaded instead of the NV partition?
Yep! Interesting eh!
For the time being we should leave it as-is in this patch, but I'm
looking for more information on how to differentiate 'boot' from
'boot_on' later on. I thought of renaming it to DOWNLOAD_ON but now
there is MODEM_DL_START...
Yes, it'd be interesting to learn more about it. I've concentrated for
now just on getting a working sequence, I don't understand it deeply!
/* Samsung's official daemons continue to read from umts_boot0
* at this point, but at present we don't have any means of
* doing that within the design of this framework. This is
* probably (?) ok, since I have never seen anything actually
* come out of umts_boot0 after booting is complete! */
This is probably to handle modem crashes. If the modem crash, we
probably need to load its firmware again. Else it could be annoying as
you would not know that the modem crashed and assume that your phone is
working fine and miss SMS, calls and other notifications.
On the kernel side, you seem to have a watchdog timer and its
associated interrupt (irq_cp_wdt) for the modem. So if it crash, I
would guess that this should trigger somehow. I need to look on how we
do it for the other devices we support.
In any case we just need to make sure that the boot sequence is tried
again, so we might not need to keep the device nodes open.
This sounds like a broader libsamsung-ipc architectural issue, right?
+int herolte_gprs_activate(...)
+int herolte_gprs_deactivate(...)
Theses are typically used to get data working. Are these not
implemented? Or do you need to do nothing here? Or do you have not
enough information to know if they are to be implemented or not?
I don't have enough information here. I just copied stubs from a similar
neighbouring modem directory. Any clues would be very welcome! I haven't
made much headway in figuring out how to enable the data/LTE connections.
In any case it would be great to have a comment explaining the status
here as we have no other way of knowing if it's supposed to work or not
as there is no use of that code in the wild yet.
Will do, good idea.
+#define HEROLTE_MODEM_IMAGE_DEVICE "/dev/disk/by-partlabel/RADIO"
This path won't work in Android. For instance on Replicant 6 we have:
root@i9300:/ # ls -la /dev/disk
ls: /dev/disk: No such file or directory
But/dev/block/* works:
root@i9300:/ # ls -la /dev/block/platform/dw_mmc/by-name/RADIO
[...] /dev/block/platform/dw_mmc/by-name/RADIO -> /dev/block/mmcblk0p7
In the other hand GNU/Linux doesn't use /dev/block in the same way.
Do you know if there is a path that works for both GNU/Linux and
Android for the kernel you use?
I don't believe so. PostmarketOS and LineageOS use quite different
paths, both the "by-partlabel"/"by-name" entries as well as the
underlying device filenames.
Also:
- What exact mmcblk<x>p<y> does /dev/disk/by-partlabel/RADIO points to?
On my phone, it maps to /dev/sda8 on pmos; on lineage, I think it was
mmcblk0p8 but I'm not 100% certain.
- For the record, what kernel did you test with (kernel version and
repository/branch/commit) and what dts do you use and/or can be used
with the 'herolte' device?
I'm using what's in PostmarketOS by default ("Linux pm 3.18.140
#2-postmarketOS SMP PREEMPT Sun Jul 12 02:36:42 UTC 2020 aarch64 Linux").
That is https://github.com/ivanmeler/android_kernel_samsung_herolte at
rev 4f50ed696fc6ec36d277ba1af73e98d34e7bc0d6.
The dts that it attaches are:
arch/arm64/boot/dts/exynos8890-herolte_eur_open_04.dtb
arch/arm64/boot/dts/exynos8890-herolte_eur_open_08.dtb
arch/arm64/boot/dts/exynos8890-herolte_eur_open_09.dtb
I'm struggling to learn more about how to build kernels for
postmarketOS, since this kernel version/dts panics and reboots instantly
(!) when I try to play any audio; and, separately, when a phone call is
connected (after booting it using my method), no audio flows back or
forth. (Any tips welcome!)
If it's not possible to support both OS with the same path, we could
still workaround by creating udev rules for GNU/Linux or adding ln -s
commands in the init.rc in Android.
I can easily (naively) try a sequence of paths. I'll revise the patch to
do so.
Do you really have "Hardware: smdk4x12" in /proc/cpuinfo?
No, there's nothing useful in /proc/cpuinfo on postmarketOS
unfortunately. I have had to develop with a hardcoded #define selecting
the device variant.
processor : 0
Features : fp asimd evtstrm aes pmull sha1 sha2 crc32
CPU implementer : 0x41
CPU architecture: 8
CPU variant : 0x0
CPU part : 0xd03
CPU revision : 4
[...]
I'll try to figure out a useful value to put in that field to convey
that it shouldn't be used. Probably NULL or something, I suppose.
./configure CFLAGS="-DIPC_DEVICE_NAME=herolte"
Oh yes, this is roughly what I've been doing.
#include "devices/i9300/i9300.h"
#include "devices/n7100/n7100.h"
#include "devices/n5100/n5100.h"
+#include "devices/herolte/herolte.h"
It would be best to order that alphabetically.
Will do.
Regards,
Tony
_______________________________________________
Replicant mailing list
[email protected]
https://lists.osuosl.org/mailman/listinfo/replicant
