Re: [PATCHv2] gnss: motmdm: Add support for Motorola Mapphone MDM6600 modem

[Johan Hovold <johan@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5955 bytes --]

On Sun, Feb 28, 2021 at 09:46:01PM +0100, Pavel Machek wrote:
> Hi!
> 
> > Motorola is using a custom TS 27.010 based multiplexer protocol
> > for various devices on the modem. These devices can be accessed on
> > dedicated channels using Linux kernel serdev-ngsm driver.
> > 
> > For the GNSS on these devices, we need to kick the GNSS device at a
> > desired rate. Otherwise the GNSS device stops sending data after a
> > few minutes. The rate we poll data defaults to 1000 ms, and can be
> > specified with a module option rate_ms between 1 to 16 seconds.
> > 
> > [Tony Lindgren did most of the work here, I just converted it to be
> > normal serdev.]
> 
> Could I get some comments on the gnss patch? It is fairly simple, and
> I believe it is ready for merge.

Let's get the mux driver in shape first.

> Here's new version of the serdev multiplexing patch for reference. And
> yes, I'd like you to review the design here, too. Yes,
> gsm_serdev_register_tty_port() needs to be cleaned up, but with ifdefs
> you can see alternative approach I tried to work around "controller
> busy" problem.

As I said before, if you're trying to work around the one
client-per-port assumption of serdev, you're doing it wrong. You still
have one client per *virtual* port.

> Signed-off-by: Pavel Machek <pavel@ucw.cz>
> 
> Best regards,
> 								Pavel
> 
> diff --git a/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml
> new file mode 100644
> index 000000000000..deec491ee821
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serdev/serdev-ngsm.yaml

> +  ttymask:
> +    $ref: /schemas/types.yaml#/definitions/uint64
> +    description: Mask of the TS 27.010 channel TTY interfaces to start (64 bit)

How is this intended to be used? Looks too Linux-specific for a binding.

> +examples:
> +  - |
> +    modem {
> +      compatible = "motorola,mapphone-mdm6600-serial";
> +      ttymask = <0 0x00001fee>;
> +      phys = <&fsusb1_phy>;
> +      phy-names = "usb";
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      gnss@4 {
> +         compatible = "motorola,mapphone-mdm6600-gnss";
> +         reg = <4>;
> +      };
> +    };

So is this is probably the issue: you're skipping one level of the
topology and do not describe the virtual ports.

This should instead be something like below; I'm including one level
above for the full picture:

	serial@nnn {
		compatible = "ns16550a";
		reg = <nnn>;

		modem {
			compatible = "ts27010-mux, motorola,mapphone-mdm6600-serial";

			serial@4 {
				reg = <4>

				gnss {
					compatible = "motorola,mapphone-mdm6600-gnss";
				};
			};
		};
	};

Here serial@nnn is a normal serial port, modem is the modem (TS 27.010
mux) connected to the serial port, serial@4 is virtual port number four
of the mux, and gnss is the gnss device connected to the virtual port.

You should probably just replace the ttymask property with explicit
nodes for the virtual ports to enable. With no client devices described
in devicetree, these will then show up as regular tty devices under
Linux.

If you add a compatible property for the virtual ports (e.g.
"ts27010-port") you might be able to just use of_platform_populate()
instead of walking the child nodes in gsm_serdev_register_tty_port().

> +int gsm_serdev_register_tty_port(struct gsm_serdev *gsd, int line)
> +{
> +	struct gsm_serdev_dlci_operations *ops;
> +	unsigned int base;
> +	int error;
> +	struct device *dev;
> +	struct device_node *node;
> +	struct device *dev2 = NULL;
> +	struct device *ndev;
> +		
> +	if (line < 1)
> +		return -EINVAL;
> +
> +	ops = kzalloc(sizeof(*ops), GFP_KERNEL);
> +	if (!ops)
> +		return -ENOMEM;
> +
> +	ops->line = line;
> +	ops->receive_buf = gsd_dlci_receive_buf;
> +
> +	error = gsm_serdev_register_dlci(gsd, ops);
> +	if (error)
> +		goto error;
> +
> +	base = mux_num_to_base(gsd->gsm);
> +	printk("register_tty_port: have port: %p, %d+%d\n", &gsd->gsm->dlci[line]->port, base, ops->line);
> +	dev = &gsd->serdev->dev;
> +
> +	for_each_available_child_of_node(dev->of_node, node) {
> +		struct platform_device_info devinfo = {};
> +		static int idx;
> +		struct platform_device *pdev;
> +		const char *c = of_get_property(node, "compatible", NULL);
> +		int reg;
> +		
> +		dev_info(dev, "register_tty: child -- %pOF -- compatible %s\n", node, c);
> +		
> +		if (!c)
> +			continue;
> +#ifdef OLD		
> +		if (strcmp(c, "gsmmux,port"))
> +			continue;
> +#endif
> +		if (of_property_read_u32(node, "reg", &reg)) {
> +			printk("no reg property\n");
> +			continue;
> +		}
> +
> +		if (reg != line)
> +			continue;
> +
> +		printk("n_gsm: line %d reg is %d, compatible %s\n", line, reg, c);
> +		/* Hmm, gnss does not work now? */
> +		/* Should we pass the "master" serdev here? */
> +#ifndef OLD
> +		/* does not work, controller is "BUSY". We already have that in variable "dev" */
> +		dev2 = dev;
> +#else
> +		devinfo.name = kasprintf(GFP_KERNEL, "gsm-mux-%d", idx++);
> +		devinfo.parent = dev;
> +
> +		/* Thanks to core.c: serdev_device_add */
> +		pdev = platform_device_register_full(&devinfo);
> +		pdev->dev.of_node = node;
> +
> +		dev2 = &pdev->dev;
> +		printk("device name is %s / %s\n", dev2->init_name, dev2->kobj.name);
> +#endif
> +		break;
> +	}
> +
> +	printk("n_gsm: Registering device at line %d, device is %p\n", line, dev2);
> +	ndev = tty_port_register_device_serdev(&gsd->gsm->dlci[line]->port,
> +					       gsm_tty_driver, base + ops->line, dev2);
> +	if (dev2)
> +		printk("device name is now %s / %s\n", dev2->init_name, dev2->kobj.name);
> +	
> +	printk("register_tty_port: got %p\n", ndev);
> +	return 0;
> +error:
> +	kfree(ops);
> +	return error;
> +}
> +EXPORT_SYMBOL_GPL(gsm_serdev_register_tty_port);

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]