__hci_cmd_sync() not suitable for nokia h4p

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

Hi!

Major problem with Nokia H4P driver was, that it uses custom functions
instead of __hci_cmd_sync().

To recap, code was something like this: (You can see ifdefs with old
code and my new replacements).

....
	len = sizeof(*neg_cmd) + sizeof(*neg_hdr) + H4_TYPE_SIZE;
#undef OLD 
#ifdef OLD
	skb = bt_skb_alloc(len, GFP_KERNEL);
	if (!skb)
		return -ENOMEM;

	memset(skb->data, 0x00, len);
	*skb_put(skb, 1) = H4_NEG_PKT;
	neg_hdr = (struct hci_h4p_neg_hdr *)skb_put(skb, sizeof(*neg_hdr));
	neg_cmd = (struct hci_h4p_neg_cmd *)skb_put(skb, sizeof(*neg_cmd));
	neg_hdr->dlen = sizeof(*neg_cmd);
#else      
	struct {
		struct hci_h4p_neg_cmd neg_cmd;
	} data;

	memset(&data, 0, len-1);
	neg_cmd = &data.neg_cmd;
#endif

	neg_cmd->ack = H4P_NEG_REQ;
	neg_cmd->baud = cpu_to_le16(BT_BAUDRATE_DIVIDER/MAX_BAUD_RATE);
	neg_cmd->proto = H4P_PROTO_BYTE;
	neg_cmd->sys_clk = cpu_to_le16(sysclk);

	hci_h4p_change_speed(info, INIT_SPEED);

	printk("Setting up packet\n");
	hci_h4p_set_rts(info, 1);
	info->init_error = 0;
	init_completion(&info->init_completion);
	printk("skb_queue_tail\n");

#ifdef OLD
	skb_queue_tail(&info->txq, skb);

	spin_lock_irqsave(&info->lock, flags);
	hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
		     UART_IER_THRI);
	spin_unlock_irqrestore(&info->lock, flags);
#else
	printk("hci_cmd_sync\n");
//	set_bit(HCI_RUNNING, &info->hdev->flags);

	info->initing = 2;
	
	skb = __hci_cmd_sync(info->hdev, H4_NEG_PKT << 8, sizeof(*neg_cmd), ((void *) &data), 20);

...but there's problem with __hci_cmd_sync(): the packet it produces
is one byte too long; for initialization, I can work around that with
<< 8, so the added zero is@the begining (and device will be still
initialized), but that is a) an extreme hack, b) only works for first
packet and c) means that __hci_cmd_sync() will not recognize the
reply, meaning it does not really help.

Are there different variants of the protocol I should switch to? Any
ideas? Should I forget about __hci_cmd_sync()?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Marcel Holtmann]

Hi Pavel,

> Major problem with Nokia H4P driver was, that it uses custom functions
> instead of __hci_cmd_sync().

the __hci_cmd_sync is for sending HCI commands and not low-level protocol transports like H:4 or similar. So you want to separate the actual transport of HCI from the firmware loading.

Regards

Marcel

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

Hi!

> > Major problem with Nokia H4P driver was, that it uses custom functions
> > instead of __hci_cmd_sync().

>  the __hci_cmd_sync is for sending HCI commands and not low-level
> protocol transports like H:4 or similar. So you want to separate the
> actual transport of HCI from the firmware loading.

The TODO file says:

# > +
# > +     skb_queue_tail(&info->txq, fw_skb);
# > +     spin_lock_irqsave(&info->lock, flags);
# > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
# > +                     UART_IER_THRI);
# > +     spin_unlock_irqrestore(&info->lock, flags);
# > +}
# 
# and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
# +hdev->setup callback for loading firmware and doing other setup details. You can just
# +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
# +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
# +case with the Nokia firmware files.

...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
not suitable after all?

But I don't understand what you want me to do at this point. I guess
skb_queue_tail+hci_h4p_outb should be moved to a helper function
(that's easy), and I already moved initialization to hci_setup().

nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
between initialization and data traffic, but I guess that's fine?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Marcel Holtmann]

Hi Pavel,

>>> Major problem with Nokia H4P driver was, that it uses custom functions
>>> instead of __hci_cmd_sync().
> 
>> the __hci_cmd_sync is for sending HCI commands and not low-level
>> protocol transports like H:4 or similar. So you want to separate the
>> actual transport of HCI from the firmware loading.
> 
> The TODO file says:
> 
> # > +
> # > +     skb_queue_tail(&info->txq, fw_skb);
> # > +     spin_lock_irqsave(&info->lock, flags);
> # > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
> # > +                     UART_IER_THRI);
> # > +     spin_unlock_irqrestore(&info->lock, flags);
> # > +}
> # 
> # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
> # +hdev->setup callback for loading firmware and doing other setup details. You can just
> # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
> # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
> # +case with the Nokia firmware files.
> 
> ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
> not suitable after all?

__hci_cmd_sync is to be used in hdev->setup where you load the firmware. However when hdev->setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev->send and hci_recv_frame.

> But I don't understand what you want me to do at this point. I guess
> skb_queue_tail+hci_h4p_outb should be moved to a helper function
> (that's easy), and I already moved initialization to hci_setup().
> 
> nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
> between initialization and data traffic, but I guess that's fine?

I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things.

Look at how btusb.c is doing this. We only run __hci_cmd_sync there. The details on how commands and events are going over USB are not handling hdev->setup. That is done by the basics of the driver.

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.

Regards

Marcel

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

Hi!

> > The TODO file says:
> > 
> > # > +
> > # > +     skb_queue_tail(&info->txq, fw_skb);
> > # > +     spin_lock_irqsave(&info->lock, flags);
> > # > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
> > # > +                     UART_IER_THRI);
> > # > +     spin_unlock_irqrestore(&info->lock, flags);
> > # > +}
> > # 
> > # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
> > # +hdev->setup callback for loading firmware and doing other setup details. You can just
> > # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
> > # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
> > # +case with the Nokia firmware files.
> > 
> > ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
> > not suitable after all?
> 
> __hci_cmd_sync is to be used in hdev->setup where you load the firmware. However when hdev->setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev->send and hci_recv_frame.
>

h4p changes uart speed again after load of the firmware, but I guess
that's ok.

> > But I don't understand what you want me to do at this point. I guess
> > skb_queue_tail+hci_h4p_outb should be moved to a helper function
> > (that's easy), and I already moved initialization to hci_setup().
> > 
> > nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
> > between initialization and data traffic, but I guess that's fine?
> 
> I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things.
>

Ok, it looks like __hci_cmd_sync() is indeed good match for the
firmware load.

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

diff --git a/drivers/staging/nokia_h4p/nokia_core.c b/drivers/staging/nokia_h4p/nokia_core.c
index 9ece2c8..5cdb86a 100644
--- a/drivers/staging/nokia_h4p/nokia_core.c
+++ b/drivers/staging/nokia_h4p/nokia_core.c
@@ -475,12 +475,6 @@ static inline void hci_h4p_recv_frame(struct hci_h4p_info *info,
 			info->rx_state = WAIT_FOR_PKT_TYPE;
 			return;
 		}
-
-		if (!test_bit(HCI_UP, &info->hdev->flags)) {
-			BT_DBG("fw_event");
-			hci_h4p_parse_fw_event(info, skb);
-			return;
-		}
 	}
 
 	hci_recv_frame(info->hdev, skb);
diff --git a/drivers/staging/nokia_h4p/nokia_fw-bcm.c b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
index 8066b21..89718d4 100644
--- a/drivers/staging/nokia_h4p/nokia_fw-bcm.c
+++ b/drivers/staging/nokia_h4p/nokia_fw-bcm.c
@@ -45,84 +45,17 @@ static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info,
 	return 0;
 }
 
-void hci_h4p_bcm_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
-{
-	struct sk_buff *fw_skb;
-	int err;
-	unsigned long flags;
-
-	if (skb->data[5] != 0x00) {
-		dev_err(info->dev, "Firmware sending command failed 0x%.2x\n",
-			skb->data[5]);
-		info->fw_error = -EPROTO;
-	}
-
-	kfree_skb(skb);
-
-	fw_skb = skb_dequeue(info->fw_q);
-	if (fw_skb == NULL || info->fw_error) {
-		complete(&info->fw_completion);
-		return;
-	}
-
-	if (fw_skb->data[1] == 0x01 && fw_skb->data[2] == 0xfc &&
-			fw_skb->len >= 10) {
-		BT_DBG("Setting bluetooth address");
-		err = hci_h4p_bcm_set_bdaddr(info, fw_skb);
-		if (err < 0) {
-			kfree_skb(fw_skb);
-			info->fw_error = err;
-			complete(&info->fw_completion);
-			return;
-		}
-	}
-
-	hci_h4p_simple_send_frame(info, fw_skb);
-}
-
-
 int hci_h4p_bcm_send_fw(struct hci_h4p_info *info,
 			struct sk_buff_head *fw_queue)
 {
-	struct sk_buff *skb;
-	unsigned long flags, time;
+	unsigned long time;
 
 	info->fw_error = 0;
 
-	BT_DBG("Sending firmware");
+	printk("Sending firmware (not really)\n");
 
 	time = jiffies;
-
-	info->fw_q = fw_queue;
-	skb = skb_dequeue(fw_queue);
-	if (!skb)
-		return -ENODATA;
-
-	BT_DBG("Sending commands");
-
-	/*
-	 * Disable smart-idle as UART TX interrupts
-	 * are not wake-up capable
-	 */
-	hci_h4p_smart_idle(info, 0);
-
-	/* Check if this is bd_address packet */
-	init_completion(&info->fw_completion);
-
-	hci_h4p_simple_send_frame(info, skb);
-
-	if (!wait_for_completion_timeout(&info->fw_completion,
-				msecs_to_jiffies(2000))) {
-		dev_err(info->dev, "No reply to fw command\n");
-		return -ETIMEDOUT;
-	}
-
-	if (info->fw_error) {
-		dev_err(info->dev, "FW error\n");
-		return -EPROTO;
-	}
-
-	BT_DBG("Firmware sent in %d msecs",
+	printk("Firmware sent in %d msec\n",
 		   jiffies_to_msecs(jiffies-time));
 
 	hci_h4p_set_auto_ctsrts(info, 0, UART_EFR_RTS);
diff --git a/drivers/staging/nokia_h4p/nokia_fw.c b/drivers/staging/nokia_h4p/nokia_fw.c
index b5748c8..be5f619 100644
--- a/drivers/staging/nokia_h4p/nokia_fw.c
+++ b/drivers/staging/nokia_h4p/nokia_fw.c
@@ -76,6 +72,7 @@ static int hci_h4p_read_fw_cmd(struct hci_h4p_info *info, struct sk_buff **skb,
 			       const struct firmware *fw_entry, gfp_t how)
 {
 	unsigned int cmd_len;
+	static int num = 0;
 
 	if (fw_pos >= fw_entry->size)
 		return 0;
@@ -95,16 +92,24 @@ static int hci_h4p_read_fw_cmd(struct hci_h4p_info *info, struct sk_buff **skb,
 		return -EMSGSIZE;
 	}
 
-	*skb = bt_skb_alloc(cmd_len, how);
-	if (!*skb) {
-		dev_err(info->dev, "Cannot reserve memory for buffer\n");
-		return -ENOMEM;
+	/* Note that this is timing-critical. If sending packets takes too
+	   long, initialization will fail. */
+	printk("Packet %d...", num);
+	if (num > 1) {
+		int cmd = fw_entry->data[fw_pos+1] + (fw_entry->data[fw_pos+2] << 8);
+		int len = fw_entry->data[fw_pos+3];
+		printk("cmd %x, len %d.", cmd, len);
+		*skb = __hci_cmd_sync(info->hdev, cmd, len, fw_entry->data+fw_pos+4, 500);
+		if (IS_ERR(*skb)) {
+			printk("...sending cmd failed %d\n", PTR_ERR(*skb));
+			return -EIO;
+		}
 	}
-	memcpy(skb_put(*skb, cmd_len), &fw_entry->data[fw_pos], cmd_len);
+	num++;
 
 	fw_pos += cmd_len;
 
-	return (*skb)->len;
+	return 1;
 }
 
 int hci_h4p_read_fw(struct hci_h4p_info *info, struct sk_buff_head *fw_queue)
@@ -113,31 +118,22 @@ int hci_h4p_read_fw(struct hci_h4p_info *info, struct sk_buff_head *fw_queue)
 	struct sk_buff *skb = NULL;
 	int err;
 
+	/*
+	 * Disable smart-idle as UART TX interrupts
+	 * are not wake-up capable
+	 */
+	hci_h4p_smart_idle(info, 0);
+	
 	err = hci_h4p_open_firmware(info, &fw_entry);
 	if (err < 0 || !fw_entry)
 		goto err_clean;
 
+	printk("read firmware\n");
+	/* FIXME: remove skb... */
 	while ((err = hci_h4p_read_fw_cmd(info, &skb, fw_entry, GFP_KERNEL))) {
-		if (err < 0 || !skb)
-			goto err_clean;
-
-		skb_queue_tail(fw_queue, skb);
 	}
 
-	/* Chip detection code does neg and alive stuff
-	 * discard two first skbs */
-	skb = skb_dequeue(fw_queue);
-	if (!skb) {
-		err = -EMSGSIZE;
-		goto err_clean;
-	}
-	kfree_skb(skb);
-	skb = skb_dequeue(fw_queue);
-	if (!skb) {
-		err = -EMSGSIZE;
-		goto err_clean;
-	}
-	kfree_skb(skb);
+	printk("done read firmware\n");
 
 err_clean:
 	hci_h4p_close_firmware(fw_entry);
@@ -160,20 +156,4 @@ int hci_h4p_send_fw(struct hci_h4p_info *info, struct sk_buff_head *fw_queue)
 	return err;
 }
 
-void hci_h4p_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb)
-{
-	switch (info->man_id) {
-	case H4P_ID_BCM2048:
-		hci_h4p_bcm_parse_fw_event(info, skb);
-		break;
-	default:
-		dev_err(info->dev, "Don't know how to parse fw event\n");
-		info->fw_error = -EINVAL;
-	}
-}
-
-MODULE_FIRMWARE(FW_NAME_TI1271_PRELE);
-MODULE_FIRMWARE(FW_NAME_TI1271_LE);
-MODULE_FIRMWARE(FW_NAME_TI1271);
 MODULE_FIRMWARE(FW_NAME_BCM2048);
-MODULE_FIRMWARE(FW_NAME_CSR);


-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Marcel Holtmann]

Hi Pavel,

>>> The TODO file says:
>>> 
>>> # > +
>>> # > +     skb_queue_tail(&info->txq, fw_skb);
>>> # > +     spin_lock_irqsave(&info->lock, flags);
>>> # > +     hci_h4p_outb(info, UART_IER, hci_h4p_inb(info, UART_IER) |
>>> # > +                     UART_IER_THRI);
>>> # > +     spin_unlock_irqrestore(&info->lock, flags);
>>> # > +}
>>> # 
>>> # and as I explained before, this crazy can not continue. Bluetooth drivers can provide a
>>> # +hdev->setup callback for loading firmware and doing other setup details. You can just
>>> # +bring up the HCI transport. We are providing __hci_cmd_sync for easy loading of the
>>> # +firmware. Especially if the firmware just consists of HCI commands. Which is clearly the
>>> # +case with the Nokia firmware files.
>>> 
>>> ...so I take it you (and thus TODO) were wrong and __hci_cmd_sync is
>>> not suitable after all?
>> 
>> __hci_cmd_sync is to be used in hdev->setup where you load the firmware. However when hdev->setup is run, we expect that the HCI transport is fully up and running and that the driver takes care of the transport. That is done via hdev->send and hci_recv_frame.
>> 
> 
> 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.

>>> But I don't understand what you want me to do at this point. I guess
>>> skb_queue_tail+hci_h4p_outb should be moved to a helper function
>>> (that's easy), and I already moved initialization to hci_setup().
>>> 
>>> nokia_core.c uses test_bit(HCI_RUNNING, &info->hdev->flags) to tell
>>> between initialization and data traffic, but I guess that's fine?
>> 
>> I have no idea on how much more I can explain this. There should be code in the driver that handles the HCI transport. That means init of the transport and sending and receiving HCI frames. And then there is the piece to load the firmware etc. These are two independent things.
>> 
> 
> Ok, it looks like __hci_cmd_sync() is indeed good match for the
> firmware load.
> 
>> 
>> 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.

Regards

Marcel

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

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

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

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";
 	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),
 	},
 };
 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Marcel Holtmann]

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

__hci_cmd_sync() not suitable for nokia h4p

[Sebastian Reichel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141212/e0496be9/attachment.sig>

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Pavel Machek]

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
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

__hci_cmd_sync() not suitable for nokia h4p

[Marcel Holtmann]

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

__hci_cmd_sync() not suitable for nokia h4p

[Sebastian Reichel]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20141213/72f92c44/attachment.sig>