RE: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

4 messages, 2 authors, 2014-10-01 · open the first message on its own page

RE: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

From: Kweh, Hock Leong <hidden>
Date: 2014-10-01 11:06:36

quoted hunk
-----Original Message-----
From: Kweh, Hock Leong
Sent: Thursday, September 18, 2014 11:18 AM
quoted
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, September 17, 2014 12:56 PM
Are you kidding me?  It's a perfect way to identify this device, it
properly uses PCI_CLASS_NETWORK_ETHERNET (0x0200) in both cases and
this will not match any other function on this PCI device at all.

Please do as I suggested and use the PCI class for the differentiation
and matching during probing.
Hi David,

Thanks for your feedback so far. Appreciate it.

Sorry to my poorly written description that may have caused some confusion.
My sincere apology here.
Unfortunately, I don't really grasp your idea clearly based on your responses
which I appreciate them a lot.
Sorry for the long description below but I hope to clearly pen down my
thinking process so that you can follow my thinking incrementally without
being confused.

So, let's roll back a bit so that with my following description, you can help
correct me if my understanding of using PCI function ID to differentiate PHY
port that is associated with each Ethernet controller is wrong:

The high-level idea about the change that I made for STMMAC IP inside
Quark is as follow:

(1) Based on Quark-specific PCI ID declared inside stmmac_id_table[],  the
probe() function is
       called to continue setting-up STMMAC for Quark.
@@ -228,11 +303,13 @@ static int stmmac_pci_resume(struct pci_dev *pdev)

 #define STMMAC_VENDOR_ID 0x700
 #define STMMAC_DEVICE_ID 0x1108
+#define STMMAC_QUARK_X1000_ID 0x0937

 static const struct pci_device_id stmmac_id_table[] = {
 	{PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID),
PCI_ANY_ID,
 			PCI_ANY_ID, CHIP_STMICRO},
 	{PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_MAC),
CHIP_STMICRO},
+	{PCI_VDEVICE(INTEL, STMMAC_QUARK_X1000_ID),
CHIP_QUARK_X1000},
 	{}
 };

(2) Back-ground on STMMAC hardware configuration on Intel Galileo Gen 1 &
Gen 2 platforms:
Intel Quark SoC has 2 MAC controller as described by lspci output below:

00:14.6 Class 0200: 8086:0937    ====> 1st MAC controller
00:14.7 Class 0200: 8086:0937    ====> 2nd MAC controller

These Galileo boards use the same Intel Quark SoC and there is only one PHY
connect to the 1st MAC [00:14.6 Class 0200: 8086:0937] The 2nd MAC [00:14.7
Class 0200: 8086:0937] is NOT connected to any PHY at all.

So, it appears to me that the only way that I can differentiate between 1st &
2nd MAC are based on PCI function ID, i.e. 14.6 & 14.7. Therefore, within the
probe() function, for Intel Quark SoC only, the function performs next-level
discovery of 1st or 2nd MAC controller through quark_run_time_config()
function.
For other PCI ID (currently STMICRO_MAC) there is NO next-level discovery
involved as rt_config is NULL.
Changes shown below:

static struct platform_data platform_info[] = { @@ -59,15 +65,76 @@ static
struct platform_data platform_info[] = {
 		.phy_reset = NULL,
 		.phy_mask = 0,
 		.pbl = 32,
+		.fixed_burst = 0,
 		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = NULL,  ===================> no 2nd-level
discovery for other PCI ID
+	},
+	[CHIP_QUARK_X1000] = {
+		.phy_addr = 1,
+		.interface = PHY_INTERFACE_MODE_RMII,
+		.clk_csr = 2,
+		.has_gmac = 1,
+		.force_sf_dma_mode = 1,
+		.multicast_filter_bins = HASH_TABLE_SIZE,
+		.unicast_filter_entries = 1,
+		.phy_reset = NULL,
+		.phy_mask = 0,
+		.pbl = 16,
+		.fixed_burst = 1,
+		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = &quark_run_time_config,       ==========>
Quark specific 2nd-level discovery
+	},
+};

(3) Within quark_run_time_config(), due to the only way to differentiate 1st
or 2nd MAC controller according to difference in function ID explained above,
the following changes are made:

+static void quark_run_time_config(int chip_id, struct pci_dev *pdev) {
+	const char *board_name =
dmi_get_system_info(DMI_BOARD_NAME);
+	int i;
+	int func_num = PCI_FUNC(pdev->devfn);
+
+	if (!board_name)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(quark_x1000_phy_info); i++) {
+		if ((!strcmp(quark_x1000_phy_info[i].board_name,
board_name)) &&
+		    quark_x1000_phy_info[i].pci_func_num == func_num)
+			platform_info[chip_id].phy_addr =
+				quark_x1000_phy_info[i].phy_address;
+	}
+}

The reasons for the above proposed condition checks, i.e. "board name" &
"pci function name" are below:
  a) As described above, the only difference in both instance of STMMAC IP
inside Intel Quark SoC is the function ID,
       so I have proposed to use function ID to be the decision point here to
differentiate 1st MAC from 2nd MAC.
  b) Allow future expansion of any other Intel Quark platforms with specific
need to fix PHY address
  c) A PHY address set as "-1"  is to mark that the PHY (associated with
function ID) is not connected to MAC, which
      is being used here for the 2 Galileo boards -> 2nd MAC port not connected
with PHY.


Finally, based on the above description, it appears to me that using PCI
function ID to decode seems viable for Intel Quark specific hardware
configuration.

Appreciate your time and any feedback is very much appreciated.

Thanks.


Regards,
Wilson

Hi Guys,

Just gently ping for the discussion to carry on before forgetting the context.
Anyone have any better idea or comments or concern to this topic?
Hope the above explanation clear out your doubt.


Regards,
Wilson

Re: [PATCH 4/4] net: stmmac: add MSI support for Intel Quark X1000

From: "Bryan O'Donoghue" <pure.logic@nexus-software.ie>
Date: 2014-10-01 11:29:11

Hi Guys,

Just gently ping for the discussion to carry on before forgetting the context.
Anyone have any better idea or comments or concern to this topic?
Hope the above explanation clear out your doubt.


Regards,
Wilson
Hi Wilson.

Seeing you post now on the PCI emumeration suggestion from Dave Miller I see

I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so 
can only respond now....

What's missing from your MSI enabling code is the PVM mask/unmask 
required on the Quark X1000 bridge - for *all* downstream devices using MSI.

I realise it's not an upstreaming friendly piece of code - however - 
without the PVM mask operation all MSIs on Quark should be considered 
unreliable.

Maybe you guys have submitted patches to the PCI layer on this already ? 
If so feel free to ignore.

If not then please re-evaluate all MSI enabling code.

 From the original

http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sources_for_Intel_Quark_v1.0.0.7z

+#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
+	#define mask_pvm(x) qrk_pci_pvm_mask(x)
+	#define unmask_pvm(x) qrk_pci_pvm_unmask(x)
+#else
+	#define mask_pvm(x)
+	#define unmask_pvm(x)
+#endif
+
  static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
  {
  	struct net_device *dev = (struct net_device *)dev_id;
@@ -1601,10 +1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, 
void *dev_id)
  		return IRQ_NONE;
  	}

+	mask_pvm(priv->pdev);
+
  	/* To handle GMAC own interrupts */
  	if (priv->plat->has_gmac) {
-		int status = priv->hw->mac->host_irq_status((void __iomem *)
-							    dev->base_addr);
+		int status = priv->hw->mac->host_irq_status(priv);
+
  		if (unlikely(status)) {
  			if (status & core_mmc_tx_irq)
  				priv->xstats.mmc_tx_irq_n++;
@@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void 
*dev_id)
  	/* To handle DMA interrupts */
  	stmmac_dma_interrupt(priv);

+	unmask_pvm(priv->pdev);
+
  	return IRQ_HANDLED;
  }

RE: [PATCH 4/4] net: stmmac: add MSI support for Intel Quark X1000

From: Kweh, Hock Leong <hidden>
Date: 2014-10-01 11:55:45

quoted hunk
-----Original Message-----
From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
Sent: Wednesday, October 01, 2014 7:29 PM
Hi Wilson.

Seeing you post now on the PCI emumeration suggestion from Dave Miller I
see

I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so can
only respond now....

What's missing from your MSI enabling code is the PVM mask/unmask
required on the Quark X1000 bridge - for *all* downstream devices using MSI.

I realise it's not an upstreaming friendly piece of code - however - without
the PVM mask operation all MSIs on Quark should be considered unreliable.

Maybe you guys have submitted patches to the PCI layer on this already ?
If so feel free to ignore.

If not then please re-evaluate all MSI enabling code.

 From the original

http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sour
ces_for_Intel_Quark_v1.0.0.7z

+#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
+	#define mask_pvm(x) qrk_pci_pvm_mask(x)
+	#define unmask_pvm(x) qrk_pci_pvm_unmask(x) #else
+	#define mask_pvm(x)
+	#define unmask_pvm(x)
+#endif
+
  static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
  {
  	struct net_device *dev = (struct net_device *)dev_id; @@ -1601,10
+1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
  		return IRQ_NONE;
  	}

+	mask_pvm(priv->pdev);
+
  	/* To handle GMAC own interrupts */
  	if (priv->plat->has_gmac) {
-		int status = priv->hw->mac->host_irq_status((void __iomem
*)
-							    dev->base_addr);
+		int status = priv->hw->mac->host_irq_status(priv);
+
  		if (unlikely(status)) {
  			if (status & core_mmc_tx_irq)
  				priv->xstats.mmc_tx_irq_n++;
@@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
*dev_id)
  	/* To handle DMA interrupts */
  	stmmac_dma_interrupt(priv);

+	unmask_pvm(priv->pdev);
+
  	return IRQ_HANDLED;
  }
Hi Bryan,

The MSI masking is already implemented in the MSI framework: http://lxr.free-electrons.com/source/drivers/pci/msi.c#L181.
I don't see a reason to upstream a local set implementation to Ethernet subsystem.
Thanks.


Regards,
Wilson

Re: [PATCH 4/4] net: stmmac: add MSI support for Intel Quark X1000

From: "Bryan O'Donoghue" <pure.logic@nexus-software.ie>
Date: 2014-10-01 12:06:03

On 01/10/14 12:55, Kweh, Hock Leong wrote:
quoted
-----Original Message-----
From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
Sent: Wednesday, October 01, 2014 7:29 PM
Hi Wilson.

Seeing you post now on the PCI emumeration suggestion from Dave Miller I
see

I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so can
only respond now....

What's missing from your MSI enabling code is the PVM mask/unmask
required on the Quark X1000 bridge - for *all* downstream devices using MSI.

I realise it's not an upstreaming friendly piece of code - however - without
the PVM mask operation all MSIs on Quark should be considered unreliable.

Maybe you guys have submitted patches to the PCI layer on this already ?
If so feel free to ignore.

If not then please re-evaluate all MSI enabling code.

  From the original

http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sour
ces_for_Intel_Quark_v1.0.0.7z

+#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
+	#define mask_pvm(x) qrk_pci_pvm_mask(x)
+	#define unmask_pvm(x) qrk_pci_pvm_unmask(x) #else
+	#define mask_pvm(x)
+	#define unmask_pvm(x)
+#endif
+
   static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
   {
   	struct net_device *dev = (struct net_device *)dev_id; @@ -1601,10
+1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
   		return IRQ_NONE;
   	}

+	mask_pvm(priv->pdev);
+
   	/* To handle GMAC own interrupts */
   	if (priv->plat->has_gmac) {
-		int status = priv->hw->mac->host_irq_status((void __iomem
*)
-							    dev->base_addr);
+		int status = priv->hw->mac->host_irq_status(priv);
+
   		if (unlikely(status)) {
   			if (status & core_mmc_tx_irq)
   				priv->xstats.mmc_tx_irq_n++;
@@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
*dev_id)
   	/* To handle DMA interrupts */
   	stmmac_dma_interrupt(priv);

+	unmask_pvm(priv->pdev);
Hi Bryan,

The MSI masking is already implemented in the MSI framework: http://lxr.free-electrons.com/source/drivers/pci/msi.c#L181.
I don't see a reason to upstream a local set implementation to Ethernet subsystem.
Thanks.
Hi Wilson.

Understand where you are getting your MSI enabling code from.

What I'm saying to you is that on Quark SoC X1000 there's an 
*additional* requirement with respect to MSIs

That's why the reference code for the Quark BSP does PVM masking for 
*all* MSI enabled code - not just ethernet.....

I'll have a review of the patches for the SoC thus far with a view to 
ensuring the MSI pvm issue is adequately addressed - but just to be 
clear it's emphatically *not* ethernet specific.

In essence the following additional requirement is place on the Quark 
SoC when using MSIs

pvm_mask();

/* handle your interrupt */

pvm_unmask();

It's the same behaviour in the USB gadget driver...
@@ -2779,55 +2788,70 @@ static irqreturn_t pch_udc_isr(int irq, void *pdev)
  {
  	struct pch_udc_dev *dev = (struct pch_udc_dev *) pdev;
  	u32 dev_intr, ep_intr;
-	int i;
-
-	dev_intr = pch_udc_read_device_interrupts(dev);
-	ep_intr = pch_udc_read_ep_interrupts(dev);
-
-	/* For a hot plug, this find that the controller is hung up. */
-	if (dev_intr == ep_intr)
-		if (dev_intr == pch_udc_readl(dev, UDC_DEVCFG_ADDR)) {
-			dev_dbg(&dev->pdev->dev, "UDC: Hung up\n");
-			/* The controller is reset */
-			pch_udc_writel(dev, UDC_SRST, UDC_SRST_ADDR);
-			return IRQ_HANDLED;
+	int i, events = 0;
+
+	mask_pvm(dev->pdev);
	// do stuff

+	unmask_pvm(dev->pdev);
+
  	return IRQ_HANDLED;
  }

And again in the GIP block

+static irqreturn_t intel_qrk_gip_handler(int irq, void *dev_id)
+{
+	irqreturn_t ret_i2c = IRQ_NONE;
+	irqreturn_t ret_gpio = IRQ_NONE;
+	struct intel_qrk_gip_data *data = (struct intel_qrk_gip_data *)dev_id;
+
+	mask_pvm(data->pci_device);
+
+	if (likely(i2c)) {
+		/* Only I2C gets platform data */
+		ret_i2c = i2c_dw_isr(irq, data->i2c_drvdata);
+	}
+
+	if (likely(gpio)) {
+		ret_gpio = intel_qrk_gpio_isr(irq, NULL);
+	}
+
+	unmask_pvm(data->pci_device);
+
+	if (likely(IRQ_HANDLED == ret_i2c || IRQ_HANDLED == ret_gpio))
+		return IRQ_HANDLED;
+
+	/* Each sub-ISR routine returns either IRQ_HANDLED or IRQ_NONE. */
+	return IRQ_NONE;
+}

Best,
	BOD
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help