| Message ID | 20141211221306.GA2905@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pavel, >>> h4p changes uart speed again after load of the firmware, but I guess >>> that's ok. > >> if you can do it the other way around it would result in a faster >> init. Depending on how many patches are actually required to be >> loaded. > > IIRC driver does two speed changes, so it looks to me like someone > already tried that (and it did not work). normally it does not matter which way around. I think some people changed it in hciattach for Broadcom devices actually. >>>> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. >>>> >>> >>> Speed changes at the end of firmware load, but I guess that's detail? >>> Anyway, patch would look like this. >> >> You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware. >> > > I can provide setup() callback and load firmware from there. > > I have created provisional device tree binding, and the driver still > works. > > Some time ago you mentioned that with the "big" issues fixed, you'd be > willing to take it into the tree. What way forward do you see? Would > it make sense to re-enable the driver in staging, so that "big" > changes could be applied, followed by renames? If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. I think the important part is to focus on the N900 derivative with the Broadcom chip inside. And just ignore all the rest. So start small and do not try to carry the N770, N800, N810 hacks over. However you might want to check Neil Brown's patches for TTY-slave devices he just posted. Maybe the N900 devices should be exposed as TTY and we just attach N_HCI line discipline to it. If all the GPIO wiggling can be done automatically at TTY open, then that should be the way to go. I also send an email about using the new unconfigured stage to get this done cleanly. http://permalink.gmane.org/gmane.linux.bluez.kernel/50483 Regards Marcel
Hi, On Thu, Dec 11, 2014 at 11:13:07PM +0100, Pavel Machek wrote: > Hi! > > > > h4p changes uart speed again after load of the firmware, but I guess > > > that's ok. > > > if you can do it the other way around it would result in a faster > > init. Depending on how many patches are actually required to be > > loaded. > > IIRC driver does two speed changes, so it looks to me like someone > already tried that (and it did not work). maybe. maybe not. Should be easy to test it (again) and add a comment, shouldn't it? > > >> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. > > >> > > > > > > Speed changes at the end of firmware load, but I guess that's detail? > > > Anyway, patch would look like this. > > > > You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware. > > > > I can provide setup() callback and load firmware from there. > > I have created provisional device tree binding, and the driver still > works. I don't have time to look at the code now, but I have some comments regarding the binding. > Some time ago you mentioned that with the "big" issues fixed, you'd be > willing to take it into the tree. What way forward do you see? Would > it make sense to re-enable the driver in staging, so that "big" > changes could be applied, followed by renames? > > Thanks, > Pavel > > diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts > index 9e0e5a2..201f21b 100644 > --- a/arch/arm/boot/dts/omap3-n900.dts > +++ b/arch/arm/boot/dts/omap3-n900.dts > @@ -790,9 +776,21 @@ > }; > > &uart2 { > + compatible = "brcm,uart,bcm2048"; This does not look correct. The uart should not be overwritten. The current h4p driver indeed implements a driver for the serial port, but that's a) linux specific and does not belong in the DT and b) should probably be changed in the mainline kernel. > interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>; > pinctrl-names = "default"; > pinctrl-0 = <&uart2_pins>; > + device { > + compatible = "brcm,bcm2048"; > + uart = <&uart2>; You don't need a phandle to the parent device. > + reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */ > + host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */ The host-wakeup should be mapped as irq, gpio2irq conversion will happen ;) > + bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* want 37 */ To be consistent with the n900 DTS file you should probably drop "want " from the comments. > + chip-type = <3>; This should be set in the driver based on the compatible value and not via DT data. > + clocks = <&uart2_fck>, <&uart2_ick>; > + clock-names = "fck", "ick"; These clocks you defined belong to the uart device and not to the uart slave (bluetooth) device. > + bt-sysclk = <2>; I think this should be mapped cleanly in DT by adding a new clock to the DTS file: vctcxo_clock: clock { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <38400000>; }; Then the bluetooth device can reference its clock device: clocks = <&vctcxo_clock>; The same clock reference should be added to the wl1251 DT node :) > ... [code] ... -- Sebastian
Hi! > >>>> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. > >>>> > >>> > >>> Speed changes at the end of firmware load, but I guess that's detail? > >>> Anyway, patch would look like this. > >> > >> You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware. > >> > > > > I can provide setup() callback and load firmware from there. > > > > I have created provisional device tree binding, and the driver still > > works. > > > > Some time ago you mentioned that with the "big" issues fixed, you'd be > > willing to take it into the tree. What way forward do you see? Would > > it make sense to re-enable the driver in staging, so that "big" > > changes could be applied, followed by renames? > > If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. > Ok, so you can revert a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd in your tree, and I can post patches against that? > I think the important part is to focus on the N900 derivative with >the Broadcom chip inside. And just ignore all the rest. So start >small and do not try to carry the N770, N800, N810 hacks over. Well, I did remove non-relevant firmware loaders, and I can remove the switches for different firmware loaders, too, but spending time removing support for different hardware does not sound quite right. > However you might want to check Neil Brown's patches for TTY-slave >devices he just posted. Maybe the N900 devices should be exposed as >TTY and we just attach N_HCI line discipline to it. If all the GPIO >wiggling can be done automatically at TTY open, then that should be >the way to go. I also send an email about using the new unconfigured >stage to get this done cleanly. I seen them, but they don't help, I'm afraid. The GPIOs are used for power saving, and they are used in various places including the serial irq handler. Best regards, Pavel
Hi! > > I have created provisional device tree binding, and the driver still > > works. > > I don't have time to look at the code now, but I have some comments > regarding the binding. > > > > &uart2 { > > + compatible = "brcm,uart,bcm2048"; > > This does not look correct. The uart should not be overwritten. The > current h4p driver indeed implements a driver for the serial port, > but that's a) linux specific and does not belong in the DT and b) > should probably be changed in the mainline kernel. Yes, bettter solution is needed here. But see the code, I don't see how b) would be implemented. > > interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>; > > pinctrl-names = "default"; > > pinctrl-0 = <&uart2_pins>; > > + device { > > + compatible = "brcm,bcm2048"; > > + uart = <&uart2>; > > You don't need a phandle to the parent device. Ok. > > + reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */ > > + host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */ > > The host-wakeup should be mapped as irq, gpio2irq conversion > will happen ;) Why? It is accessed as gpio, too. > > + chip-type = <3>; > > This should be set in the driver based on the compatible > value and not via DT data. Ok > > + clocks = <&uart2_fck>, <&uart2_ick>; > > + clock-names = "fck", "ick"; > > These clocks you defined belong to the uart device and not to the > uart slave (bluetooth) device. Ok. Why are they only needed in the bluetooth case? > > + bt-sysclk = <2>; > > I think this should be mapped cleanly in DT by adding a new clock > to the DTS file: > > vctcxo_clock: clock { > compatible = "fixed-clock"; > #clock-cells = <0>; > clock-frequency = <38400000>; > }; No. It seems that this selects baud rate during the chip init. I guess I can just remove that one. > Then the bluetooth device can reference its clock device: > > clocks = <&vctcxo_clock>; > > The same clock reference should be added to the wl1251 DT node :) Feel free to do that, but I don't see how this one helps...? Pavel
Hi Pavel, >>>>>> What needs to be done is the bring up of the device including the proper UART settings and speed and then just run the firmware downloads. All firmware files on the Nokia devices where just HCI commands with vendor specific details. Some from CSR, some from Broadcom and some from TI. You can actually decode them if you really want to. Shouldn't be that hard. >>>>>> >>>>> >>>>> Speed changes at the end of firmware load, but I guess that's detail? >>>>> Anyway, patch would look like this. >>>> >>>> You should really look into providing hdev->setup() callback. That is normally the callback where you want to load the firmware. >>>> >>> >>> I can provide setup() callback and load firmware from there. >>> >>> I have created provisional device tree binding, and the driver still >>> works. >>> >>> Some time ago you mentioned that with the "big" issues fixed, you'd be >>> willing to take it into the tree. What way forward do you see? Would >>> it make sense to re-enable the driver in staging, so that "big" >>> changes could be applied, followed by renames? >> >> If the driver is clean, there is no point in taking it through staging. It can be cleaned up and then merged all together. >> > > Ok, so you can revert a4102f90e87cfaa3fdbed6fdf469b23f0eeb4bfd in your > tree, and I can post patches against that? just treat this a submission of a new driver. >> I think the important part is to focus on the N900 derivative with >> the Broadcom chip inside. And just ignore all the rest. So start >> small and do not try to carry the N770, N800, N810 hacks over. > > Well, I did remove non-relevant firmware loaders, and I can remove the > switches for different firmware loaders, too, but spending time > removing support for different hardware does not sound quite right. Lets face it, you are not getting it upstream that way. If nobody has the hardware to test it or cares about the hardware anymore, then this should not be upstream in the first place. Make your life easier and get your hardware supported. Then someone can evolve this for older Nokia devices. But I am not taking code that nobody has tested. Keep it simple is really the only way to get this driver merged. There is so much old cruft in there that you are better off only caring about the N900 version of the hardware. >> However you might want to check Neil Brown's patches for TTY-slave >> devices he just posted. Maybe the N900 devices should be exposed as >> TTY and we just attach N_HCI line discipline to it. If all the GPIO >> wiggling can be done automatically at TTY open, then that should be >> the way to go. I also send an email about using the new unconfigured >> stage to get this done cleanly. > > I seen them, but they don't help, I'm afraid. The GPIOs are used for > power saving, and they are used in various places including the serial > irq handler. That is something that might need some extra work on it, but it seems that the general direction of TTY slaves is the right one. Anyway, the main order of business is really that it supports DT and that it can be compiled on every single platform and you could even load the driver on x86 without doing any harm. Regards Marcel
Hi, On Fri, Dec 12, 2014 at 01:14:53PM +0100, Pavel Machek wrote: > > > I have created provisional device tree binding, and the driver still > > > works. > > > > I don't have time to look at the code now, but I have some comments > > regarding the binding. > > > > > > > &uart2 { > > > + compatible = "brcm,uart,bcm2048"; > > > > This does not look correct. The uart should not be overwritten. The > > current h4p driver indeed implements a driver for the serial port, > > but that's a) linux specific and does not belong in the DT and b) > > should probably be changed in the mainline kernel. > > Yes, bettter solution is needed here. But see the code, I don't see > how b) would be implemented. I think it's probably a good idea to start from scratch. The h4p driver in its current state does not fit to the current kernel's style at all. My suggestion would be to have a driver, which implements a hci_dev. The driver would mostly look like the standard uart hci_dev driver, but additionally handles the wakeup and reset gpios. Power Management for the uart device is done via runtime pm, which is supported by OMAP's uart driver. Then there should be a second driver, which implements the h4 protocol extensions from Nokia as hci_uart_proto. It could be put into something like drivers/bluetooth/hci_h4p.c and have support for the additional packet types (namely H4_EVT_PKT, H4_NEG_PKT, H4_ALIVE_PKT and H4_RADIO_PKT). > > > interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>; > > > pinctrl-names = "default"; > > > pinctrl-0 = <&uart2_pins>; > > > + device { > > > + compatible = "brcm,bcm2048"; > > > + uart = <&uart2>; > > > > You don't need a phandle to the parent device. > > Ok. > > > > + reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */ > > > + host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */ > > > > The host-wakeup should be mapped as irq, gpio2irq conversion > > will happen ;) > > Why? It is accessed as gpio, too. Ok. I only did a quick look and saw, that the gpio was converted to irq. > > > + chip-type = <3>; > > > > This should be set in the driver based on the compatible > > value and not via DT data. > > Ok > > > > + clocks = <&uart2_fck>, <&uart2_ick>; > > > + clock-names = "fck", "ick"; > > > > These clocks you defined belong to the uart device and not to the > > uart slave (bluetooth) device. > > Ok. Why are they only needed in the bluetooth case? A quick guess (without a deeper look at the code) is, that the ttys use hwmod provided clock information (which is available from the kernel). btw, h4p not using hwmod would be another problem, if it is ok to reimplement a full serial driver. I guess the clock data should be added to the uart devices in DT, though. This is btw. true for almost all omap internal devices described in DT, since the clock data has only been available in DT for 1 or 2 kernel releases. > > > + bt-sysclk = <2>; > > > > I think this should be mapped cleanly in DT by adding a new clock > > to the DTS file: > > > > vctcxo_clock: clock { > > compatible = "fixed-clock"; > > #clock-cells = <0>; > > clock-frequency = <38400000>; > > }; > > No. It seems that this selects baud rate during the chip init. I guess > I can just remove that one. you can see, that its a descriptor for the speed of the bcm2048's system clock. (from h4p driver) bt-sysclk = 1 => sysclk = 12000; bt-sysclk = 2 => sysclk = 38400; and incidentally there is a 38.4 MHz clock connected to the bcm2048 and the wl1251 in the N900 :) > > Then the bluetooth device can reference its clock device: > > > > clocks = <&vctcxo_clock>; > > > > The same clock reference should be added to the wl1251 DT node :) > > Feel free to do that, but I don't see how this one helps...? The clock is not enabled via the CPU, instead wl1251 and bcm2048 enable it themselfes, so it's not strictly needed. But it means, that a) DT represents the hardware correctly b) one can acquire the sysclk speed from the referenced clock [ using devm_clk_get() and clk_get_rate() ] -- Sebastian
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts index 9e0e5a2..201f21b 100644 --- a/arch/arm/boot/dts/omap3-n900.dts +++ b/arch/arm/boot/dts/omap3-n900.dts @@ -790,9 +776,21 @@ }; &uart2 { + compatible = "brcm,uart,bcm2048"; interrupts-extended = <&intc 73 &omap3_pmx_core OMAP3_UART2_RX>; pinctrl-names = "default"; pinctrl-0 = <&uart2_pins>; + device { + compatible = "brcm,bcm2048"; + uart = <&uart2>; + reset-gpios = <&gpio3 27 GPIO_ACTIVE_HIGH>; /* want 91 */ + host-wakeup-gpios = <&gpio4 5 GPIO_ACTIVE_HIGH>; /* want 101 */ + bluetooth-wakeup-gpios = <&gpio2 5 GPIO_ACTIVE_HIGH>; /* want 37 */ + chip-type = <3>; + bt-sysclk = <2>; + clocks = <&uart2_fck>, <&uart2_ick>; + clock-names = "fck", "ick"; + }; }; &uart3 { diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c index 5cdb86a..ac61cfd 100644 --- a/drivers/staging/nokia_h4p/nokia_core.c +++ b/drivers/staging/nokia_h4p/nokia_core.c @@ -37,6 +37,8 @@ #include <linux/clk.h> #include <linux/interrupt.h> #include <linux/gpio.h> +#include <linux/of_gpio.h> +#include <linux/of_irq.h> #include <linux/timer.h> #include <linux/kthread.h> #include <linux/io.h> @@ -1112,12 +1114,75 @@ free: return -ENODEV; } +static int hci_h4p_probe_pdata(struct platform_device *pdev, struct hci_h4p_info *info, + struct hci_h4p_platform_data *bt_plat_data) +{ + info->chip_type = bt_plat_data->chip_type; + info->bt_wakeup_gpio = bt_plat_data->bt_wakeup_gpio; + info->host_wakeup_gpio = bt_plat_data->host_wakeup_gpio; + info->reset_gpio = bt_plat_data->reset_gpio; + info->reset_gpio_shared = bt_plat_data->reset_gpio_shared; + info->bt_sysclk = bt_plat_data->bt_sysclk; + + info->irq = bt_plat_data->uart_irq; + info->uart_base = devm_ioremap(&pdev->dev, bt_plat_data->uart_base, + SZ_2K); + info->uart_iclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_iclk); + info->uart_fclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_fclk); + return 0; +} + +static int hci_h4p_probe_dt(struct platform_device *pdev, struct hci_h4p_info *info) +{ + struct device_node *node; + struct device_node *uart = pdev->dev.of_node; + u32 val; + struct resource *mem; + + node = of_get_child_by_name(uart, "device"); + + if (!node) + return -ENODATA; + + if (of_property_read_u32(node, "chip-type", &val)) return -EINVAL; + info->chip_type = val; + + if (of_property_read_u32(node, "bt-sysclk", &val)) return -EINVAL; + info->bt_sysclk = val; + + info->reset_gpio = of_get_named_gpio(node, "reset-gpios", 0); + info->host_wakeup_gpio = of_get_named_gpio(node, "host-wakeup-gpios", 0); + info->bt_wakeup_gpio = of_get_named_gpio(node, "bluetooth-wakeup-gpios", 0); + //uart = of_parse_phandle(node, "uart", 0); + if (!uart) { + dev_err(&pdev->dev, "UART link not provided\n"); + return -EINVAL; + } + + info->irq = irq_of_parse_and_map(uart, 0); + + mem = platform_get_resource(pdev, IORESOURCE_MEM, 0); + info->uart_base = devm_ioremap_resource(&pdev->dev, mem); + + info->uart_iclk = of_clk_get_by_name(node, "ick"); + info->uart_fclk = of_clk_get_by_name(node, "fck"); + + printk("DT: have neccessary data\n"); + return 0; +} + + static int hci_h4p_probe(struct platform_device *pdev) { - struct hci_h4p_platform_data *bt_plat_data; + struct hci_h4p_info *info; int err; + printk("HCI h4p probe\n"); + if (pdev->dev.of_node) { + printk("Have platform data.\n"); + } + dev_info(&pdev->dev, "Registering HCI H4P device\n"); info = devm_kzalloc(&pdev->dev, sizeof(struct hci_h4p_info), GFP_KERNEL); @@ -1131,40 +1196,36 @@ static int hci_h4p_probe(struct platform_device *pdev) spin_lock_init(&info->clocks_lock); skb_queue_head_init(&info->txq); - if (pdev->dev.platform_data == NULL) { + if (pdev->dev.platform_data) { + err = hci_h4p_probe_pdata(pdev, info, pdev->dev.platform_data); + } else { + err = hci_h4p_probe_dt(pdev, info); + } + if (err) { dev_err(&pdev->dev, "Could not get Bluetooth config data\n"); return -ENODATA; } - bt_plat_data = pdev->dev.platform_data; - info->chip_type = bt_plat_data->chip_type; - info->bt_wakeup_gpio = bt_plat_data->bt_wakeup_gpio; - info->host_wakeup_gpio = bt_plat_data->host_wakeup_gpio; - info->reset_gpio = bt_plat_data->reset_gpio; - info->reset_gpio_shared = bt_plat_data->reset_gpio_shared; - info->bt_sysclk = bt_plat_data->bt_sysclk; - - BT_DBG("RESET gpio: %d", info->reset_gpio); - BT_DBG("BTWU gpio: %d", info->bt_wakeup_gpio); - BT_DBG("HOSTWU gpio: %d", info->host_wakeup_gpio); - BT_DBG("sysclk: %d", info->bt_sysclk); + printk("base/irq gpio: %lx/%d/%d\n", + info->uart_base, info->irq); + printk("RESET/BTWU/HOSTWU gpio: %d/%d/%d\n", + info->reset_gpio, info->bt_wakeup_gpio, info->host_wakeup_gpio); + printk("chip type, sysclk: %d/%d\n", info->chip_type, info->bt_sysclk); + printk("clock i/f: %lx/%lx\n", info->uart_iclk, info->uart_fclk); init_completion(&info->test_completion); complete_all(&info->test_completion); - if (!info->reset_gpio_shared) { - err = devm_gpio_request_one(&pdev->dev, info->reset_gpio, - GPIOF_OUT_INIT_LOW, "bt_reset"); - if (err < 0) { - dev_err(&pdev->dev, "Cannot get GPIO line %d\n", - info->reset_gpio); - return err; - } + err = devm_gpio_request_one(&pdev->dev, info->reset_gpio, + GPIOF_OUT_INIT_LOW, "bt_reset"); + if (err < 0) { + dev_err(&pdev->dev, "Cannot get GPIO line %d\n", + info->reset_gpio); + return err; } err = devm_gpio_request_one(&pdev->dev, info->bt_wakeup_gpio, GPIOF_OUT_INIT_LOW, "bt_wakeup"); - if (err < 0) { dev_err(info->dev, "Cannot get GPIO line 0x%d", info->bt_wakeup_gpio); @@ -1179,12 +1240,6 @@ static int hci_h4p_probe(struct platform_device *pdev) return err; } - info->irq = bt_plat_data->uart_irq; - info->uart_base = devm_ioremap(&pdev->dev, bt_plat_data->uart_base, - SZ_2K); - info->uart_iclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_iclk); - info->uart_fclk = devm_clk_get(&pdev->dev, bt_plat_data->uart_fclk); - err = devm_request_irq(&pdev->dev, info->irq, hci_h4p_interrupt, IRQF_DISABLED, "hci_h4p", info); if (err < 0) { @@ -1246,12 +1301,38 @@ static int hci_h4p_remove(struct platform_device *pdev) return 0; } +#if 0 +struct hci_h4p_platform_data bt_plat_data = { + .chip_type = 3, + .bt_sysclk = 2, + .bt_wakeup_gpio = RX51_HCI_H4P_BTWU_GPIO, + .host_wakeup_gpio = RX51_HCI_H4P_HOSTWU_GPIO, + .reset_gpio = RX51_HCI_H4P_RESET_GPIO, + .reset_gpio_shared = 0, + // .uart_irq = 73 + OMAP_INTC_START, + /* It seems to be 223 in hci_h4p case */ + .uart_irq = 223, + .uart_base = OMAP3_UART2_BASE, + .uart_iclk = "uart2_ick", + .uart_fclk = "uart2_fck", + .set_pm_limits = rx51_bt_set_pm_limits, +}; +#endif + +static const struct of_device_id hci_h4p_of_match[] = { + { .compatible = "brcm,uart,bcm2048" }, + {}, +}; +MODULE_DEVICE_TABLE(of, hci_h4p_of_match); + static struct platform_driver hci_h4p_driver = { .probe = hci_h4p_probe, .remove = hci_h4p_remove, .driver = { - .name = "hci_h4p", + .name = "disabled" "hci_h4p", + .owner = THIS_MODULE, + .of_match_table = of_match_ptr(hci_h4p_of_match), }, };