[Replicant] [RFC PATCH 2/2] Initial commit of herolte support

Denis 'GNUtoo' Carikli GNUtoo at cyberdimension.org
Mon Sep 14 19:16:40 UTC 2020


Hi,

Thanks a lot for your patch.

I didn't review everything in it yet, but I didn't want to delay too
much, so I'll probably point out other things to fix in a new mail.

For instance I didn't try to compile it yet, nor looked for common C
issues.

I also took the opportunity to look a bit on how it works behind the
hood by looking at the kernel driver. Most of the answers to your
questions are also there.

Here I following kernel/commit for that:
> commit: 776608ab3a887c3cc5f2e71a6e13b2c066fb9b21
> branch: lineage-15.1
> url:
> https://github.com/LineageOS/android_kernel_samsung_universal8890.git

On Mon, 14 Sep 2020 11:45:30 +0200
Tony Garnock-Jones <tonyg at leastfixedpoint.com> wrote:
> +struct __attribute__((__packed__)) firmware_toc_entry {
> +        char name[12];
> +        uint32_t offset;        /* offset within firmware
> file/partition */
> +        uint32_t loadaddr;      /* target memory address for this
> blob */
> +        uint32_t size;          /* size of this blob in bytes */
> +        uint32_t crc;
Do you know how to calculate this CRC? Is it a standard one?
> +        uint32_t entryid;
> +};

> +#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.

> +int herolte_boot(struct ipc_client *client)
> +{
[...]
> +        /* cbd acquires the "ss310" wakelock here. TODO: Should we do
> +         * the same? */
While this modem driver is tied to a specific kernel API that is not
upstream, it's often possible to recompile such kernel and have
different power management policies. Also libsamsung-ipc is a low level
library and the integration with various OS or frameworks are supposed
to be done at the higher level, for instance in daemons that use
libsamsung-ipc. It's probably because of that that support for
warkelocks were removed by the following commit:
> c99663fe46c3fc426d6f74ae146d1c5842764f1a
> c99663f Get rid of wakelocks, this should be dealt with on the upper
>         layers
So this comment can be removed.

> +        boot0_fd = open(XMM626_SEC_MODEM_BOOT0_DEVICE, O_RDWR |
> O_NOCTTY);
> +        if (boot0_fd < 0)
> +                goto exit;

> +        /* 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

> 	ipc_client_log(client, "Resetting modem");
>         if (ioctl(boot0_fd, IOCTL_MODEM_RESET, 0) < 0)
>                 goto exit;
First you put the modem SOC in reset (IOCTL_MODEM_RESET).
This calls int the exynos_cp_reset kernel function. This seems to power
on/off the modem, which makes the call to the (commented)
xmm626_kernel_smdk4412_power / IOCTL_MODEM_OFF useless.

>         if (select_secure_mode(client, boot0_fd, 0, 0, 0) < 0)
>                 goto exit;
> 
>         if (upload_chunk(client, boot0_fd, imagefd, toc, "BOOT",
> &size_boot) < 0) goto exit;
> 
>         if (upload_chunk(client, boot0_fd, imagefd, toc, "MAIN",
> &size_main) < 0) goto exit;
> 
>         if (upload_chunk(client, boot0_fd, nvfd, toc, "NV", NULL) < 0)
>                 goto exit;
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?

>         if (select_secure_mode(client, boot0_fd, 1, size_boot,
> size_main) < 0) goto exit;
Then since it uses shared memory, you start loading the firmwares in
memory, while still having the modem in reset.

For that more recent kernels have that instead:
> modem_prj.h:#define IOCTL_MODEM_XMIT_BOOT _IO('o', 0x40)
however we should keep the IOCTL as-is here as it's already defined in
libsamsung-ipc. In the worst case we can add a comment.

For that it uses the shmem_xmit_boot function which writes the firmware
memory.

>         ipc_client_log(client, "Powering on modem");
>         if (xmm626_kernel_smdk4412_power(client, boot0_fd, 1) < 0)
>                 goto exit;
Then you power the modem on. Here it uses the ss310ap_on function
which is very interesting: it does things like 
> mbox_set_value(mc->mbx_pda_active, 1)
which are memory-only operations. This probably explains why the
handshake that you do later on is done in this way: no GPIOs are used
for telling the modem that the host is not in suspend-to-ram. This
communication is needed as the modem need to create an IRQ to wakeup the
host in the case a call arrives for instance. Other GPIOs/memory
locations are used for other things too. We tried to document some in
the XMMBoot wiki page[1].

>         ipc_client_log(client, "Starting modem boot process");
>         if (xmm626_kernel_smdk4412_boot_power(client, boot0_fd, 1) <
> 0) goto exit;
> 
>         ipc_client_log(client, "Kicking off firmware download");
>         if (ioctl(boot0_fd, IOCTL_MODEM_DL_START, 0) < 0)
>                 goto exit;
Then you tell the modem to start loading the firmware with both 
IOCTL_MODEM_BOOT_ON and IOCTL_MODEM_DL_START. The issue here is that in
libsamsung-ipc there is modem_boot and modem_boot_on functions which
are named after the ioctl names.

So I've been trying to understand what BOOT_ON is supposed to do by
reading the source code of various devices. On the Galaxy Nexus it does 
switch the modem UART to the one used to load code (through the
gpio_flm_uart_sel).

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...

>         ipc_client_log(client, "Handshaking with modem");
>         /* At this point, cbd engages in a little dance with the
>          * newly-booted modem, apparently to verify that it is running
>          * as expected. I don’t know the sources of these magic
>          * numbers, I just faithfully reproduce them. */
That is the right thing to do. As for what the number do, they are
probably specific to the modem firmware and/or hardware. They may or
may not be documented in the the kernel source code, but I'm unsure if
spending hours looking for that is really useful here.

On my side I'm really interested in such things because I might try to
upstream the modem driver if I find the time and also to better
architecture libsamsung-ipc, but for this patch you don't need to do
that research as it's very time consuming.

We also support (in libsamsung-ipc but not Replicant) devices with
shared memory between the modem and the SOC but I didn't have the time
yet to document how they work in more details, though I vaguely recall
that they work in a somewhat similar way, but I may also mix that in my
mind with information gathered for other research to understand what
the IOCTL and GPIOs did.

>         {
>                 uint32_t buf;
> 
>                 buf = 0x900d;
>                 if (write(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
>                         goto exit;
>                 if (read(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
>                         goto exit;
>                 if (buf != 0xa00d)
>                         goto exit;
>                 ipc_client_log(client, "Handshake stage I passed");
> 
>                 buf = 0x9f00;
>                 if (write(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
>                         goto exit;
>                 if (read(boot0_fd, &buf, sizeof(buf)) != sizeof(buf))
>                         goto exit;
>                 if (buf != 0xaf00)
>                         goto exit;
>                 ipc_client_log(client, "Handshake stage II passed");
>         }
[...]

>         /* 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.

>         /* One thing that could be an issue, though, is the kernel's
>          * use of its function rild_ready(). The phone wants *both*
>          * umts_ipc0 and umts_rfs0 to be open for it to be fully
>          * happy. Will it work well enough though only one of the two
>          * (presumably umts_ipc0) is available? We'll see! */
I've responded to that in my previous mail.

> +int herolte_gprs_activate(__attribute__((unused)) struct ipc_client
> *client,
> +			__attribute__((unused)) void *data,
> +			__attribute__((unused)) unsigned int cid)
> +{
> +	return 0;
> +}
> +
> +int herolte_gprs_deactivate(__attribute__((unused)) struct
> ipc_client *client,
> +			  __attribute__((unused)) void *data,
> +			  __attribute__((unused)) unsigned int cid)
> +{
> +	return 0;
> +}
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?

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.

> +#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 at i9300:/ # ls -la /dev/disk
> ls: /dev/disk: No such file or directory

But /dev/block/* works:
> root at 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?

Also:
- What exact mmcblk<x>p<y> does /dev/disk/by-partlabel/RADIO points to?
- 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?

That information would help if the code has to be changed later on.

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.

In the long run we need to rework that, to try multiple paths instead
to be able to try various paths to support multiple combinations of
operating systems and kernels.

> +++ b/samsung-ipc/devices/ipc_devices.c
[...]
> +		.name = "herolte",
> +		.board_name = "smdk4x12",
> +		.kernel_version = NULL,
Do you really have "Hardware: smdk4x12" in /proc/cpuinfo?

I also forgot about the fact that we had modem driver that shared
the smdk4x12 board name. That could be an issue: In Android, the driver
is selected in the Android.mk at compile time, but in GNU/Linux that
could create an issue if the wrong driver is selected at runtime.
We need to fix that in libsamsung-ipc somehow at some point.

In the meantime if that device really has smdk4x12 in /proc/cpuinfo a
workaround for that could be to run configure like that for now:
> ./configure CFLAGS="-DIPC_DEVICE_NAME=herolte"

> --- a/samsung-ipc/devices/ipc_devices.h
> +++ b/samsung-ipc/devices/ipc_devices.h
> @@ -28,6 +28,7 @@
>  #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.

References:
-----------
[1]https://redmine.replicant.us/projects/replicant/wiki/XMMBoot

Denis.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20200914/c0b4016e/attachment-0001.asc>


More information about the Replicant mailing list