[v2,08/10] PCI: layerscape: Add EP mode support for ls1088a and ls2088a

Commit Message

Xiaowei Bao Aug. 22, 2019, 11:22 a.m. UTC

Add PCIe EP mode support for ls1088a and ls2088a, there are some
difference between LS1 and LS2 platform, so refactor the code of
the EP driver.

Signed-off-by: Xiaowei Bao <[e-mail]>
---
v2:
 - New mechanism for layerscape EP driver.

 drivers/pci/controller/dwc/pci-layerscape-ep.c | 76 ++++++++++++++++++++------
 1 file changed, 58 insertions(+), 18 deletions(-)

Comments

Andrew Murray Aug. 23, 2019, 2:27 p.m. UTC | #1

On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> difference between LS1 and LS2 platform, so refactor the code of
> the EP driver.
> 
> Signed-off-by: Xiaowei Bao <[e-mail]>
> ---
> v2:
>  - New mechanism for layerscape EP driver.

Was there a v1 of this patch?

> 
>  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76 ++++++++++++++++++++------
>  1 file changed, 58 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index 7ca5fe8..2a66f07 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -20,27 +20,29 @@
>  
>  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
>  
> -struct ls_pcie_ep {
> -	struct dw_pcie		*pci;
> -	struct pci_epc_features	*ls_epc;
> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> +
> +struct ls_pcie_ep_drvdata {
> +	u32				func_offset;
> +	const struct dw_pcie_ep_ops	*ops;
> +	const struct dw_pcie_ops	*dw_pcie_ops;
>  };
>  
> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> +struct ls_pcie_ep {
> +	struct dw_pcie			*pci;
> +	struct pci_epc_features		*ls_epc;
> +	const struct ls_pcie_ep_drvdata *drvdata;
> +};
>  
>  static int ls_pcie_establish_link(struct dw_pcie *pci)
>  {
>  	return 0;
>  }
>  
> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
>  	.start_link = ls_pcie_establish_link,
>  };
>  
> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> -	{ .compatible = "fsl,ls-pcie-ep",},
> -	{ },
> -};
> -
>  static const struct pci_epc_features*
>  ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
>  {
> @@ -82,10 +84,44 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>  	}
>  }
>  
> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> +						u8 func_no)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> +	u8 header_type;
> +
> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> +
> +	if (header_type & (1 << 7))
> +		return pcie->drvdata->func_offset * func_no;
> +	else
> +		return 0;

It looks like there isn't a PCI define for multi function, the nearest I
could find was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
above the test might be helpful to explain the test.

As the ls_pcie_ep_drvdata structures are static, the unset .func_offset
will be initialised to 0, so you could just drop the test above.

However something to the effect of the following may help spot
misconfiguration:

WARN_ON(func_no && !pcie->drvdata->func_offset);
return pcie->drvdata->func_offset * func_no;

The WARN is probably quite useful as if you are attempting to use
non-zero functions and func_offset isn't set - then things may appear to work
normally but actually will break horribly.

Thanks,

Andrew Murray

> +}
> +
> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
>  	.ep_init = ls_pcie_ep_init,
>  	.raise_irq = ls_pcie_ep_raise_irq,
>  	.get_features = ls_pcie_ep_get_features,
> +	.func_conf_select = ls_pcie_ep_func_conf_select,
> +};
> +
> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> +	.ops = &ls_pcie_ep_ops,
> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> +};
> +
> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> +	.func_offset = 0x20000,
> +	.ops = &ls_pcie_ep_ops,
> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> +};
> +
> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> +	{ },
>  };
>  
>  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> @@ -98,7 +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
>  	int ret;
>  
>  	ep = &pci->ep;
> -	ep->ops = &pcie_ep_ops;
> +	ep->ops = pcie->drvdata->ops;
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
>  	if (!res)
> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  	if (!ls_epc)
>  		return -ENOMEM;
>  
> -	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> -	if (IS_ERR(pci->dbi_base))
> -		return PTR_ERR(pci->dbi_base);
> +	pcie->drvdata = of_device_get_match_data(dev);
>  
> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
>  	pci->dev = dev;
> -	pci->ops = &ls_pcie_ep_ops;
> +	pci->ops = pcie->drvdata->dw_pcie_ops;
> +
>  	pcie->pci = pci;
>  
>  	ls_epc->linkup_notifier = false,
> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct platform_device *pdev)
>  
>  	pcie->ls_epc = ls_epc;
>  
> +	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> +	if (IS_ERR(pci->dbi_base))
> +		return PTR_ERR(pci->dbi_base);
> +
> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> +
>  	platform_set_drvdata(pdev, pcie);
>  
>  	ret = ls_add_pcie_ep(pcie, pdev);
> -- 
> 2.9.5
>

Xiaowei Bao Aug. 24, 2019, 12:18 a.m. UTC | #2

> -----Original Message-----
> From: Andrew Murray <[e-mail]>
> Sent: 2019年8月23日 22:28
> To: Xiaowei Bao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; M.h.
> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> Zang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > difference between LS1 and LS2 platform, so refactor the code of the
> > EP driver.
> >
> > Signed-off-by: Xiaowei Bao <[e-mail]>
> > ---
> > v2:
> >  - New mechanism for layerscape EP driver.
> 
> Was there a v1 of this patch?

Yes, but I don't know how to comments, ^_^

> 
> >
> >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > ++++++++++++++++++++------
> >  1 file changed, 58 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index 7ca5fe8..2a66f07 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -20,27 +20,29 @@
> >
> >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> >
> > -struct ls_pcie_ep {
> > -	struct dw_pcie		*pci;
> > -	struct pci_epc_features	*ls_epc;
> > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > +
> > +struct ls_pcie_ep_drvdata {
> > +	u32				func_offset;
> > +	const struct dw_pcie_ep_ops	*ops;
> > +	const struct dw_pcie_ops	*dw_pcie_ops;
> >  };
> >
> > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > +struct ls_pcie_ep {
> > +	struct dw_pcie			*pci;
> > +	struct pci_epc_features		*ls_epc;
> > +	const struct ls_pcie_ep_drvdata *drvdata; };
> >
> >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> >  	return 0;
> >  }
> >
> > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >  	.start_link = ls_pcie_establish_link,  };
> >
> > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > -	{ .compatible = "fsl,ls-pcie-ep",},
> > -	{ },
> > -};
> > -
> >  static const struct pci_epc_features*  ls_pcie_ep_get_features(struct
> > dw_pcie_ep *ep)  { @@ -82,10 +84,44 @@ static int
> > ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >  	}
> >  }
> >
> > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > +						u8 func_no)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > +	u8 header_type;
> > +
> > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > +
> > +	if (header_type & (1 << 7))
> > +		return pcie->drvdata->func_offset * func_no;
> > +	else
> > +		return 0;
> 
> It looks like there isn't a PCI define for multi function, the nearest I could find
> was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
> above the test might be helpful to explain the test.

Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I will add
The comments in next version patch.

> 
> As the ls_pcie_ep_drvdata structures are static, the unset .func_offset will be
> initialised to 0, so you could just drop the test above.

OK, thanks

> 
> However something to the effect of the following may help spot
> misconfiguration:
> 
> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> pcie->drvdata->func_offset * func_no;

Thanks a lot, this looks better.

> 
> The WARN is probably quite useful as if you are attempting to use non-zero
> functions and func_offset isn't set - then things may appear to work normally
> but actually will break horribly.

got it, thanks.

> 
> Thanks,
> 
> Andrew Murray
> 
> > +}
> > +
> > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >  	.ep_init = ls_pcie_ep_init,
> >  	.raise_irq = ls_pcie_ep_raise_irq,
> >  	.get_features = ls_pcie_ep_get_features,
> > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > +
> > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > +	.func_offset = 0x20000,
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ },
> >  };
> >
> >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >  	int ret;
> >
> >  	ep = &pci->ep;
> > -	ep->ops = &pcie_ep_ops;
> > +	ep->ops = pcie->drvdata->ops;
> >
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "addr_space");
> >  	if (!res)
> > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >  	if (!ls_epc)
> >  		return -ENOMEM;
> >
> > -	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "regs");
> > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > -	if (IS_ERR(pci->dbi_base))
> > -		return PTR_ERR(pci->dbi_base);
> > +	pcie->drvdata = of_device_get_match_data(dev);
> >
> > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >  	pci->dev = dev;
> > -	pci->ops = &ls_pcie_ep_ops;
> > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > +
> >  	pcie->pci = pci;
> >
> >  	ls_epc->linkup_notifier = false,
> > @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> > platform_device *pdev)
> >
> >  	pcie->ls_epc = ls_epc;
> >
> > +	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "regs");
> > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > +	if (IS_ERR(pci->dbi_base))
> > +		return PTR_ERR(pci->dbi_base);
> > +
> > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > +
> >  	platform_set_drvdata(pdev, pcie);
> >
> >  	ret = ls_add_pcie_ep(pcie, pdev);
> > --
> > 2.9.5
> >

Christophe Leroy Aug. 24, 2019, 6:45 a.m. UTC | #3

Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> 
> 
>> -----Original Message-----
>> From: Andrew Murray <[e-mail]>
>> Sent: 2019年8月23日 22:28
>> To: Xiaowei Bao <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Leo Li <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail]; [e-mail]; M.h.
>> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
>> Zang <[e-mail]>; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail];
>> [e-mail]; [e-mail]
>> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
>> ls1088a and ls2088a
>>
>> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
>>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
>>> difference between LS1 and LS2 platform, so refactor the code of the
>>> EP driver.
>>>
>>> Signed-off-by: Xiaowei Bao <[e-mail]>
>>> ---
>>> v2:
>>>   - New mechanism for layerscape EP driver.
>>
>> Was there a v1 of this patch?
> 
> Yes, but I don't know how to comments, ^_^

As far as I can see, in the previous version of the series 
(https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=125315&state=*), 
the 8/10 was something completely different, and I can't find any other 
patch in the series that could have been the v1 of this patch.

Christophe

> 
>>
>>>
>>>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
>>> ++++++++++++++++++++------
>>>   1 file changed, 58 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> index 7ca5fe8..2a66f07 100644
>>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
>>> @@ -20,27 +20,29 @@
>>>
>>>   #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
>>>
>>> -struct ls_pcie_ep {
>>> -	struct dw_pcie		*pci;
>>> -	struct pci_epc_features	*ls_epc;
>>> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
>>> +
>>> +struct ls_pcie_ep_drvdata {
>>> +	u32				func_offset;
>>> +	const struct dw_pcie_ep_ops	*ops;
>>> +	const struct dw_pcie_ops	*dw_pcie_ops;
>>>   };
>>>
>>> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
>>> +struct ls_pcie_ep {
>>> +	struct dw_pcie			*pci;
>>> +	struct pci_epc_features		*ls_epc;
>>> +	const struct ls_pcie_ep_drvdata *drvdata; };
>>>
>>>   static int ls_pcie_establish_link(struct dw_pcie *pci)  {
>>>   	return 0;
>>>   }
>>>
>>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
>>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
>>>   	.start_link = ls_pcie_establish_link,  };
>>>
>>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
>>> -	{ .compatible = "fsl,ls-pcie-ep",},
>>> -	{ },
>>> -};
>>> -
>>>   static const struct pci_epc_features*  ls_pcie_ep_get_features(struct
>>> dw_pcie_ep *ep)  { @@ -82,10 +84,44 @@ static int
>>> ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
>>>   	}
>>>   }
>>>
>>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
>>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
>>> +						u8 func_no)
>>> +{
>>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
>>> +	u8 header_type;
>>> +
>>> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
>>> +
>>> +	if (header_type & (1 << 7))
>>> +		return pcie->drvdata->func_offset * func_no;
>>> +	else
>>> +		return 0;
>>
>> It looks like there isn't a PCI define for multi function, the nearest I could find
>> was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
>> above the test might be helpful to explain the test.
> 
> Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I will add
> The comments in next version patch.
> 
>>
>> As the ls_pcie_ep_drvdata structures are static, the unset .func_offset will be
>> initialised to 0, so you could just drop the test above.
> 
> OK, thanks
> 
>>
>> However something to the effect of the following may help spot
>> misconfiguration:
>>
>> WARN_ON(func_no && !pcie->drvdata->func_offset); return
>> pcie->drvdata->func_offset * func_no;
> 
> Thanks a lot, this looks better.
> 
>>
>> The WARN is probably quite useful as if you are attempting to use non-zero
>> functions and func_offset isn't set - then things may appear to work normally
>> but actually will break horribly.
> 
> got it, thanks.
> 
>>
>> Thanks,
>>
>> Andrew Murray
>>
>>> +}
>>> +
>>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
>>>   	.ep_init = ls_pcie_ep_init,
>>>   	.raise_irq = ls_pcie_ep_raise_irq,
>>>   	.get_features = ls_pcie_ep_get_features,
>>> +	.func_conf_select = ls_pcie_ep_func_conf_select, };
>>> +
>>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
>>> +	.ops = &ls_pcie_ep_ops,
>>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
>>> +};
>>> +
>>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
>>> +	.func_offset = 0x20000,
>>> +	.ops = &ls_pcie_ep_ops,
>>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
>>> +};
>>> +
>>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
>>> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
>>> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
>>> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
>>> +	{ },
>>>   };
>>>
>>>   static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
>>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
>>>   	int ret;
>>>
>>>   	ep = &pci->ep;
>>> -	ep->ops = &pcie_ep_ops;
>>> +	ep->ops = pcie->drvdata->ops;
>>>
>>>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "addr_space");
>>>   	if (!res)
>>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
>> platform_device *pdev)
>>>   	if (!ls_epc)
>>>   		return -ENOMEM;
>>>
>>> -	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "regs");
>>> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
>>> -	if (IS_ERR(pci->dbi_base))
>>> -		return PTR_ERR(pci->dbi_base);
>>> +	pcie->drvdata = of_device_get_match_data(dev);
>>>
>>> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
>>>   	pci->dev = dev;
>>> -	pci->ops = &ls_pcie_ep_ops;
>>> +	pci->ops = pcie->drvdata->dw_pcie_ops;
>>> +
>>>   	pcie->pci = pci;
>>>
>>>   	ls_epc->linkup_notifier = false,
>>> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
>>> platform_device *pdev)
>>>
>>>   	pcie->ls_epc = ls_epc;
>>>
>>> +	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> "regs");
>>> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
>>> +	if (IS_ERR(pci->dbi_base))
>>> +		return PTR_ERR(pci->dbi_base);
>>> +
>>> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
>>> +
>>>   	platform_set_drvdata(pdev, pcie);
>>>
>>>   	ret = ls_add_pcie_ep(pcie, pdev);
>>> --
>>> 2.9.5
>>>

---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel antivirus Avast.
https://www.avast.com/antivirus

Xiaowei Bao Aug. 25, 2019, 3:07 a.m. UTC | #4

> -----Original Message-----
> From: christophe leroy <[e-mail]>
> Sent: 2019年8月24日 14:45
> To: Xiaowei Bao <[e-mail]>; Andrew Murray
> <[e-mail]>
> Cc: [e-mail]; Roy Zang <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; M.h.
> Lian <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> Mingkai Hu <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> 
> 
> Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> >
> >
> >> -----Original Message-----
> >> From: Andrew Murray <[e-mail]>
> >> Sent: 2019年8月23日 22:28
> >> To: Xiaowei Bao <[e-mail]>
> >> Cc: [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> M.h.
> >> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> >> Zang <[e-mail]>; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> >> for ls1088a and ls2088a
> >>
> >> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> >>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> >>> difference between LS1 and LS2 platform, so refactor the code of the
> >>> EP driver.
> >>>
> >>> Signed-off-by: Xiaowei Bao <[e-mail]>
> >>> ---
> >>> v2:
> >>>   - New mechanism for layerscape EP driver.
> >>
> >> Was there a v1 of this patch?
> >
> > Yes, but I don't know how to comments, ^_^
> 
> As far as I can see, in the previous version of the series
> (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.ozlabs.org%2Fproject%2Flinuxppc-dev%2Flist%2F%3Fseries%3D125315
> %26state%3D*&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7C1befe9
> a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> 7C0%7C0%7C637022259387139020&amp;sdata=p4wbycd04Z7qRUfAoZtwc
> UP7pR%2FuA3%2FjVcWMz6YyQVQ%3D&amp;reserved=0),
> the 8/10 was something completely different, and I can't find any other patch
> in the series that could have been the v1 of this patch.

Thanks, I will correct it to v1 in next version patch.

> 
> Christophe
> 
> >
> >>
> >>>
> >>>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> >>> ++++++++++++++++++++------
> >>>   1 file changed, 58 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> index 7ca5fe8..2a66f07 100644
> >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> >>> @@ -20,27 +20,29 @@
> >>>
> >>>   #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> >>>
> >>> -struct ls_pcie_ep {
> >>> -	struct dw_pcie		*pci;
> >>> -	struct pci_epc_features	*ls_epc;
> >>> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> >>> +
> >>> +struct ls_pcie_ep_drvdata {
> >>> +	u32				func_offset;
> >>> +	const struct dw_pcie_ep_ops	*ops;
> >>> +	const struct dw_pcie_ops	*dw_pcie_ops;
> >>>   };
> >>>
> >>> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> >>> +struct ls_pcie_ep {
> >>> +	struct dw_pcie			*pci;
> >>> +	struct pci_epc_features		*ls_epc;
> >>> +	const struct ls_pcie_ep_drvdata *drvdata; };
> >>>
> >>>   static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> >>>   	return 0;
> >>>   }
> >>>
> >>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> >>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >>>   	.start_link = ls_pcie_establish_link,  };
> >>>
> >>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> -	{ .compatible = "fsl,ls-pcie-ep",},
> >>> -	{ },
> >>> -};
> >>> -
> >>>   static const struct pci_epc_features*
> >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10 +84,44
> >>> @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >>>   	}
> >>>   }
> >>>
> >>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> >>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> >>> +						u8 func_no)
> >>> +{
> >>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> >>> +	u8 header_type;
> >>> +
> >>> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> >>> +
> >>> +	if (header_type & (1 << 7))
> >>> +		return pcie->drvdata->func_offset * func_no;
> >>> +	else
> >>> +		return 0;
> >>
> >> It looks like there isn't a PCI define for multi function, the
> >> nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> >> hotplug/ibmphp.h. A comment above the test might be helpful to explain
> the test.
> >
> > Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I
> > will add The comments in next version patch.
> >
> >>
> >> As the ls_pcie_ep_drvdata structures are static, the unset
> >> .func_offset will be initialised to 0, so you could just drop the test above.
> >
> > OK, thanks
> >
> >>
> >> However something to the effect of the following may help spot
> >> misconfiguration:
> >>
> >> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> >> pcie->drvdata->func_offset * func_no;
> >
> > Thanks a lot, this looks better.
> >
> >>
> >> The WARN is probably quite useful as if you are attempting to use
> >> non-zero functions and func_offset isn't set - then things may appear
> >> to work normally but actually will break horribly.
> >
> > got it, thanks.
> >
> >>
> >> Thanks,
> >>
> >> Andrew Murray
> >>
> >>> +}
> >>> +
> >>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >>>   	.ep_init = ls_pcie_ep_init,
> >>>   	.raise_irq = ls_pcie_ep_raise_irq,
> >>>   	.get_features = ls_pcie_ep_get_features,
> >>> +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> >>> +	.ops = &ls_pcie_ep_ops,
> >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> >>> +	.func_offset = 0x20000,
> >>> +	.ops = &ls_pcie_ep_ops,
> >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> >>> +
> >>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> >>> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> >>> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> >>> +	{ },
> >>>   };
> >>>
> >>>   static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> >>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >>>   	int ret;
> >>>
> >>>   	ep = &pci->ep;
> >>> -	ep->ops = &pcie_ep_ops;
> >>> +	ep->ops = pcie->drvdata->ops;
> >>>
> >>>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> >> "addr_space");
> >>>   	if (!res)
> >>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> >> platform_device *pdev)
> >>>   	if (!ls_epc)
> >>>   		return -ENOMEM;
> >>>
> >>> -	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> -	if (IS_ERR(pci->dbi_base))
> >>> -		return PTR_ERR(pci->dbi_base);
> >>> +	pcie->drvdata = of_device_get_match_data(dev);
> >>>
> >>> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>>   	pci->dev = dev;
> >>> -	pci->ops = &ls_pcie_ep_ops;
> >>> +	pci->ops = pcie->drvdata->dw_pcie_ops;
> >>> +
> >>>   	pcie->pci = pci;
> >>>
> >>>   	ls_epc->linkup_notifier = false,
> >>> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> >>> platform_device *pdev)
> >>>
> >>>   	pcie->ls_epc = ls_epc;
> >>>
> >>> +	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> >> "regs");
> >>> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> >>> +	if (IS_ERR(pci->dbi_base))
> >>> +		return PTR_ERR(pci->dbi_base);
> >>> +
> >>> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >>> +
> >>>   	platform_set_drvdata(pdev, pcie);
> >>>
> >>>   	ret = ls_add_pcie_ep(pcie, pdev);
> >>> --
> >>> 2.9.5
> >>>
> 
> ---
> L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
> antivirus Avast.
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> avast.com%2Fantivirus&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7
> C1befe9a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637022259387139020&amp;sdata=JAYds7X%2FHVxgtrg
> e%2F%2FvnP84zdb2yReXcctQUiSLC11I%3D&amp;reserved=0

Xiaowei Bao Aug. 26, 2019, 9:49 a.m. UTC | #5

> -----Original Message-----
> From: Andrew Murray <[e-mail]>
> Sent: 2019年8月23日 22:28
> To: Xiaowei Bao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; M.h.
> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> Zang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > difference between LS1 and LS2 platform, so refactor the code of the
> > EP driver.
> >
> > Signed-off-by: Xiaowei Bao <[e-mail]>
> > ---
> > v2:
> >  - New mechanism for layerscape EP driver.
> 
> Was there a v1 of this patch?
> 
> >
> >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > ++++++++++++++++++++------
> >  1 file changed, 58 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index 7ca5fe8..2a66f07 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -20,27 +20,29 @@
> >
> >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> >
> > -struct ls_pcie_ep {
> > -	struct dw_pcie		*pci;
> > -	struct pci_epc_features	*ls_epc;
> > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > +
> > +struct ls_pcie_ep_drvdata {
> > +	u32				func_offset;
> > +	const struct dw_pcie_ep_ops	*ops;
> > +	const struct dw_pcie_ops	*dw_pcie_ops;
> >  };
> >
> > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > +struct ls_pcie_ep {
> > +	struct dw_pcie			*pci;
> > +	struct pci_epc_features		*ls_epc;
> > +	const struct ls_pcie_ep_drvdata *drvdata; };
> >
> >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> >  	return 0;
> >  }
> >
> > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> >  	.start_link = ls_pcie_establish_link,  };
> >
> > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > -	{ .compatible = "fsl,ls-pcie-ep",},
> > -	{ },
> > -};
> > -
> >  static const struct pci_epc_features*  ls_pcie_ep_get_features(struct
> > dw_pcie_ep *ep)  { @@ -82,10 +84,44 @@ static int
> > ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> >  	}
> >  }
> >
> > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > +						u8 func_no)
> > +{
> > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > +	u8 header_type;
> > +
> > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > +
> > +	if (header_type & (1 << 7))
> > +		return pcie->drvdata->func_offset * func_no;
> > +	else
> > +		return 0;
> 
> It looks like there isn't a PCI define for multi function, the nearest I could find
> was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
> above the test might be helpful to explain the test.

OK, I will add a comment above this code.

> 
> As the ls_pcie_ep_drvdata structures are static, the unset .func_offset will be
> initialised to 0, so you could just drop the test above.

Due to the different PCIe controller have different property, e.g. PCIe controller1 support
multiple function feature, but PCIe controller2 don't support this feature, so I need to check
which controller support it and return the correct offset value, but each board only have one
ls_pcie_ep_drvdata, ^_^.

> 
> However something to the effect of the following may help spot
> misconfiguration:
> 
> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> pcie->drvdata->func_offset * func_no;
> 
> The WARN is probably quite useful as if you are attempting to use non-zero
> functions and func_offset isn't set - then things may appear to work normally
> but actually will break horribly.

As discussion before, I think the func_offset should not depends on the function
number, even if other platforms of NXP may be use write registers way to access 
the different function config space. 

I have added the comments above the code, as follow, do you have any advice?
+static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
+                                               u8 func_no)
+{
+       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
+       u8 header_type;
+
+       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
+
+       /*
+        * Read the Header Type register of config space to check
+        * whether this PCI device support the multiple function.
+        */
+       if (header_type & (1 << 7))
+               return pcie->drvdata->func_offset * func_no;
+
+       return 0;
+}

Thanks a lot for your detail comments.

> 
> Thanks,
> 
> Andrew Murray
> 
> > +}
> > +
> > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> >  	.ep_init = ls_pcie_ep_init,
> >  	.raise_irq = ls_pcie_ep_raise_irq,
> >  	.get_features = ls_pcie_ep_get_features,
> > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > +
> > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > +	.func_offset = 0x20000,
> > +	.ops = &ls_pcie_ep_ops,
> > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > +};
> > +
> > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > +	{ },
> >  };
> >
> >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> >  	int ret;
> >
> >  	ep = &pci->ep;
> > -	ep->ops = &pcie_ep_ops;
> > +	ep->ops = pcie->drvdata->ops;
> >
> >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "addr_space");
> >  	if (!res)
> > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> platform_device *pdev)
> >  	if (!ls_epc)
> >  		return -ENOMEM;
> >
> > -	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "regs");
> > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > -	if (IS_ERR(pci->dbi_base))
> > -		return PTR_ERR(pci->dbi_base);
> > +	pcie->drvdata = of_device_get_match_data(dev);
> >
> > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> >  	pci->dev = dev;
> > -	pci->ops = &ls_pcie_ep_ops;
> > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > +
> >  	pcie->pci = pci;
> >
> >  	ls_epc->linkup_notifier = false,
> > @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> > platform_device *pdev)
> >
> >  	pcie->ls_epc = ls_epc;
> >
> > +	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> "regs");
> > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > +	if (IS_ERR(pci->dbi_base))
> > +		return PTR_ERR(pci->dbi_base);
> > +
> > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > +
> >  	platform_set_drvdata(pdev, pcie);
> >
> >  	ret = ls_add_pcie_ep(pcie, pdev);
> > --
> > 2.9.5
> >

Andrew Murray Aug. 27, 2019, 1:34 p.m. UTC | #6

On Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Murray <[e-mail]>
> > Sent: 2019年8月23日 22:28
> > To: Xiaowei Bao <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; M.h.
> > Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > Zang <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> > ls1088a and ls2088a
> > 
> > On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > > difference between LS1 and LS2 platform, so refactor the code of the
> > > EP driver.
> > >
> > > Signed-off-by: Xiaowei Bao <[e-mail]>
> > > ---
> > > v2:
> > >  - New mechanism for layerscape EP driver.
> > 
> > Was there a v1 of this patch?
> > 
> > >
> > >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > ++++++++++++++++++++------
> > >  1 file changed, 58 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > index 7ca5fe8..2a66f07 100644
> > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > @@ -20,27 +20,29 @@
> > >
> > >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > >
> > > -struct ls_pcie_ep {
> > > -	struct dw_pcie		*pci;
> > > -	struct pci_epc_features	*ls_epc;
> > > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > +
> > > +struct ls_pcie_ep_drvdata {
> > > +	u32				func_offset;
> > > +	const struct dw_pcie_ep_ops	*ops;
> > > +	const struct dw_pcie_ops	*dw_pcie_ops;
> > >  };
> > >
> > > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > +struct ls_pcie_ep {
> > > +	struct dw_pcie			*pci;
> > > +	struct pci_epc_features		*ls_epc;
> > > +	const struct ls_pcie_ep_drvdata *drvdata; };
> > >
> > >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > >  	return 0;
> > >  }
> > >
> > > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > >  	.start_link = ls_pcie_establish_link,  };
> > >
> > > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > -	{ .compatible = "fsl,ls-pcie-ep",},
> > > -	{ },
> > > -};
> > > -
> > >  static const struct pci_epc_features*  ls_pcie_ep_get_features(struct
> > > dw_pcie_ep *ep)  { @@ -82,10 +84,44 @@ static int
> > > ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > >  	}
> > >  }
> > >
> > > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > > +						u8 func_no)
> > > +{
> > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > +	u8 header_type;
> > > +
> > > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > +
> > > +	if (header_type & (1 << 7))
> > > +		return pcie->drvdata->func_offset * func_no;
> > > +	else
> > > +		return 0;
> > 
> > It looks like there isn't a PCI define for multi function, the nearest I could find
> > was PCI_HEADER_TYPE_MULTIDEVICE in hotplug/ibmphp.h. A comment
> > above the test might be helpful to explain the test.
> 
> OK, I will add a comment above this code.
> 
> > 
> > As the ls_pcie_ep_drvdata structures are static, the unset .func_offset will be
> > initialised to 0, so you could just drop the test above.
> 
> Due to the different PCIe controller have different property, e.g. PCIe controller1 support
> multiple function feature, but PCIe controller2 don't support this feature, so I need to check
> which controller support it and return the correct offset value, but each board only have one
> ls_pcie_ep_drvdata, ^_^.

Yes but if they don't support the feature then func_offset will be 0.

> 
> > 
> > However something to the effect of the following may help spot
> > misconfiguration:
> > 
> > WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > pcie->drvdata->func_offset * func_no;
> > 
> > The WARN is probably quite useful as if you are attempting to use non-zero
> > functions and func_offset isn't set - then things may appear to work normally
> > but actually will break horribly.
> 
> As discussion before, I think the func_offset should not depends on the function
> number, even if other platforms of NXP may be use write registers way to access 
> the different function config space. 

I agree that func_offset is an optional parameter. But if you are attempting
to determine the offset of a function and you are given a non-zero function
number - then something has gone wrong if func_offset is 0.

Thanks,

Andrew Murray

> 
> I have added the comments above the code, as follow, do you have any advice?
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> +                                               u8 func_no)
> +{
> +       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> +       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> +       u8 header_type;
> +
> +       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> +
> +       /*
> +        * Read the Header Type register of config space to check
> +        * whether this PCI device support the multiple function.
> +        */
> +       if (header_type & (1 << 7))
> +               return pcie->drvdata->func_offset * func_no;
> +
> +       return 0;
> +}
> 
> Thanks a lot for your detail comments.
> 
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > > +}
> > > +
> > > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > >  	.ep_init = ls_pcie_ep_init,
> > >  	.raise_irq = ls_pcie_ep_raise_irq,
> > >  	.get_features = ls_pcie_ep_get_features,
> > > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > > +
> > > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > +	.ops = &ls_pcie_ep_ops,
> > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > > +};
> > > +
> > > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > +	.func_offset = 0x20000,
> > > +	.ops = &ls_pcie_ep_ops,
> > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
> > > +};
> > > +
> > > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > +	{ },
> > >  };
> > >
> > >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> > > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> > >  	int ret;
> > >
> > >  	ep = &pci->ep;
> > > -	ep->ops = &pcie_ep_ops;
> > > +	ep->ops = pcie->drvdata->ops;
> > >
> > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "addr_space");
> > >  	if (!res)
> > > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > platform_device *pdev)
> > >  	if (!ls_epc)
> > >  		return -ENOMEM;
> > >
> > > -	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "regs");
> > > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > -	if (IS_ERR(pci->dbi_base))
> > > -		return PTR_ERR(pci->dbi_base);
> > > +	pcie->drvdata = of_device_get_match_data(dev);
> > >
> > > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > >  	pci->dev = dev;
> > > -	pci->ops = &ls_pcie_ep_ops;
> > > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > > +
> > >  	pcie->pci = pci;
> > >
> > >  	ls_epc->linkup_notifier = false,
> > > @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> > > platform_device *pdev)
> > >
> > >  	pcie->ls_epc = ls_epc;
> > >
> > > +	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > "regs");
> > > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > +	if (IS_ERR(pci->dbi_base))
> > > +		return PTR_ERR(pci->dbi_base);
> > > +
> > > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > +
> > >  	platform_set_drvdata(pdev, pcie);
> > >
> > >  	ret = ls_add_pcie_ep(pcie, pdev);
> > > --
> > > 2.9.5
> > >

Andrew Murray Aug. 27, 2019, 2:48 p.m. UTC | #7

On Sun, Aug 25, 2019 at 03:07:32AM +0000, Xiaowei Bao wrote:
> 
> 
> > -----Original Message-----
> > From: christophe leroy <[e-mail]>
> > Sent: 2019年8月24日 14:45
> > To: Xiaowei Bao <[e-mail]>; Andrew Murray
> > <[e-mail]>
> > Cc: [e-mail]; Roy Zang <[e-mail]>;
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; M.h.
> > Lian <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > Mingkai Hu <[e-mail]>; [e-mail]
> > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> > ls1088a and ls2088a
> > 
> > 
> > 
> > Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> > >
> > >
> > >> -----Original Message-----
> > >> From: Andrew Murray <[e-mail]>
> > >> Sent: 2019年8月23日 22:28
> > >> To: Xiaowei Bao <[e-mail]>
> > >> Cc: [e-mail]; [e-mail]; [e-mail];
> > >> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail]; [e-mail];
> > M.h.
> > >> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > >> Zang <[e-mail]>; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail]
> > >> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > >> for ls1088a and ls2088a
> > >>
> > >> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > >>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > >>> difference between LS1 and LS2 platform, so refactor the code of the
> > >>> EP driver.
> > >>>
> > >>> Signed-off-by: Xiaowei Bao <[e-mail]>
> > >>> ---
> > >>> v2:
> > >>>   - New mechanism for layerscape EP driver.
> > >>
> > >> Was there a v1 of this patch?
> > >
> > > Yes, but I don't know how to comments, ^_^
> > 
> > As far as I can see, in the previous version of the series
> > (https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> > work.ozlabs.org%2Fproject%2Flinuxppc-dev%2Flist%2F%3Fseries%3D125315
> > %26state%3D*&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7C1befe9
> > a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> > 7C0%7C0%7C637022259387139020&amp;sdata=p4wbycd04Z7qRUfAoZtwc
> > UP7pR%2FuA3%2FjVcWMz6YyQVQ%3D&amp;reserved=0),
> > the 8/10 was something completely different, and I can't find any other patch
> > in the series that could have been the v1 of this patch.
> 
> Thanks, I will correct it to v1 in next version patch.

I think you numbered it correctly (so please leave it as v2, referring to
the patch series revision) - I got confused trying to find a previous
version of this patch.

Perhaps in the future when new patches are introduced in a series you can
indicate that in the description patch revision history (e.g. introduced
in v2).

Thanks,

Andrew Murray 

> 
> > 
> > Christophe
> > 
> > >
> > >>
> > >>>
> > >>>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > >>> ++++++++++++++++++++------
> > >>>   1 file changed, 58 insertions(+), 18 deletions(-)
> > >>>
> > >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> index 7ca5fe8..2a66f07 100644
> > >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > >>> @@ -20,27 +20,29 @@
> > >>>
> > >>>   #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > >>>
> > >>> -struct ls_pcie_ep {
> > >>> -	struct dw_pcie		*pci;
> > >>> -	struct pci_epc_features	*ls_epc;
> > >>> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > >>> +
> > >>> +struct ls_pcie_ep_drvdata {
> > >>> +	u32				func_offset;
> > >>> +	const struct dw_pcie_ep_ops	*ops;
> > >>> +	const struct dw_pcie_ops	*dw_pcie_ops;
> > >>>   };
> > >>>
> > >>> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > >>> +struct ls_pcie_ep {
> > >>> +	struct dw_pcie			*pci;
> > >>> +	struct pci_epc_features		*ls_epc;
> > >>> +	const struct ls_pcie_ep_drvdata *drvdata; };
> > >>>
> > >>>   static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > >>>   	return 0;
> > >>>   }
> > >>>
> > >>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > >>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > >>>   	.start_link = ls_pcie_establish_link,  };
> > >>>
> > >>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > >>> -	{ .compatible = "fsl,ls-pcie-ep",},
> > >>> -	{ },
> > >>> -};
> > >>> -
> > >>>   static const struct pci_epc_features*
> > >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10 +84,44
> > >>> @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > >>>   	}
> > >>>   }
> > >>>
> > >>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > >>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > >>> +						u8 func_no)
> > >>> +{
> > >>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > >>> +	u8 header_type;
> > >>> +
> > >>> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > >>> +
> > >>> +	if (header_type & (1 << 7))
> > >>> +		return pcie->drvdata->func_offset * func_no;
> > >>> +	else
> > >>> +		return 0;
> > >>
> > >> It looks like there isn't a PCI define for multi function, the
> > >> nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > >> hotplug/ibmphp.h. A comment above the test might be helpful to explain
> > the test.
> > >
> > > Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I
> > > will add The comments in next version patch.
> > >
> > >>
> > >> As the ls_pcie_ep_drvdata structures are static, the unset
> > >> .func_offset will be initialised to 0, so you could just drop the test above.
> > >
> > > OK, thanks
> > >
> > >>
> > >> However something to the effect of the following may help spot
> > >> misconfiguration:
> > >>
> > >> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > >> pcie->drvdata->func_offset * func_no;
> > >
> > > Thanks a lot, this looks better.
> > >
> > >>
> > >> The WARN is probably quite useful as if you are attempting to use
> > >> non-zero functions and func_offset isn't set - then things may appear
> > >> to work normally but actually will break horribly.
> > >
> > > got it, thanks.
> > >
> > >>
> > >> Thanks,
> > >>
> > >> Andrew Murray
> > >>
> > >>> +}
> > >>> +
> > >>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > >>>   	.ep_init = ls_pcie_ep_init,
> > >>>   	.raise_irq = ls_pcie_ep_raise_irq,
> > >>>   	.get_features = ls_pcie_ep_get_features,
> > >>> +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > >>> +
> > >>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > >>> +	.ops = &ls_pcie_ep_ops,
> > >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > >>> +
> > >>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > >>> +	.func_offset = 0x20000,
> > >>> +	.ops = &ls_pcie_ep_ops,
> > >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > >>> +
> > >>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > >>> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > >>> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > >>> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > >>> +	{ },
> > >>>   };
> > >>>
> > >>>   static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@ -98,7
> > >>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
> > >>>   	int ret;
> > >>>
> > >>>   	ep = &pci->ep;
> > >>> -	ep->ops = &pcie_ep_ops;
> > >>> +	ep->ops = pcie->drvdata->ops;
> > >>>
> > >>>   	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > >> "addr_space");
> > >>>   	if (!res)
> > >>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > >> platform_device *pdev)
> > >>>   	if (!ls_epc)
> > >>>   		return -ENOMEM;
> > >>>
> > >>> -	dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > >> "regs");
> > >>> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > >>> -	if (IS_ERR(pci->dbi_base))
> > >>> -		return PTR_ERR(pci->dbi_base);
> > >>> +	pcie->drvdata = of_device_get_match_data(dev);
> > >>>
> > >>> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > >>>   	pci->dev = dev;
> > >>> -	pci->ops = &ls_pcie_ep_ops;
> > >>> +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > >>> +
> > >>>   	pcie->pci = pci;
> > >>>
> > >>>   	ls_epc->linkup_notifier = false,
> > >>> @@ -152,6 +185,13 @@ static int __init ls_pcie_ep_probe(struct
> > >>> platform_device *pdev)
> > >>>
> > >>>   	pcie->ls_epc = ls_epc;
> > >>>
> > >>> +	dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > >> "regs");
> > >>> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > >>> +	if (IS_ERR(pci->dbi_base))
> > >>> +		return PTR_ERR(pci->dbi_base);
> > >>> +
> > >>> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > >>> +
> > >>>   	platform_set_drvdata(pdev, pcie);
> > >>>
> > >>>   	ret = ls_add_pcie_ep(pcie, pdev);
> > >>> --
> > >>> 2.9.5
> > >>>
> > 
> > ---
> > L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
> > antivirus Avast.
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.
> > avast.com%2Fantivirus&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7
> > C1befe9a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c3
> > 01635%7C0%7C0%7C637022259387139020&amp;sdata=JAYds7X%2FHVxgtrg
> > e%2F%2FvnP84zdb2yReXcctQUiSLC11I%3D&amp;reserved=0
>

Xiaowei Bao Aug. 28, 2019, 3:25 a.m. UTC | #8

> -----Original Message-----
> From: Andrew Murray <[e-mail]>
> Sent: 2019年8月27日 22:49
> To: Xiaowei Bao <[e-mail]>
> Cc: christophe leroy <[e-mail]>; [e-mail]; Roy
> Zang <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; M.h. Lian
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> Mingkai Hu <[e-mail]>; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Sun, Aug 25, 2019 at 03:07:32AM +0000, Xiaowei Bao wrote:
> >
> >
> > > -----Original Message-----
> > > From: christophe leroy <[e-mail]>
> > > Sent: 2019年8月24日 14:45
> > > To: Xiaowei Bao <[e-mail]>; Andrew Murray
> > > <[e-mail]>
> > > Cc: [e-mail]; Roy Zang <[e-mail]>;
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> M.h.
> > > Lian <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; Leo Li <[e-mail]>;
> > > [e-mail]; Mingkai Hu <[e-mail]>;
> > > [e-mail]
> > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > > for ls1088a and ls2088a
> > >
> > >
> > >
> > > Le 24/08/2019 à 02:18, Xiaowei Bao a écrit :
> > > >
> > > >
> > > >> -----Original Message-----
> > > >> From: Andrew Murray <[e-mail]>
> > > >> Sent: 2019年8月23日 22:28
> > > >> To: Xiaowei Bao <[e-mail]>
> > > >> Cc: [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail]; Leo Li
> > > >> <[e-mail]>; [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > M.h.
> > > >> Lian <[e-mail]>; Mingkai Hu <[e-mail]>;
> > > >> Roy Zang <[e-mail]>; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > >> [e-mail];
> > > >> [e-mail]
> > > >> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode
> > > >> support for ls1088a and ls2088a
> > > >>
> > > >> On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > >>> Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > > >>> difference between LS1 and LS2 platform, so refactor the code of
> > > >>> the EP driver.
> > > >>>
> > > >>> Signed-off-by: Xiaowei Bao <[e-mail]>
> > > >>> ---
> > > >>> v2:
> > > >>>   - New mechanism for layerscape EP driver.
> > > >>
> > > >> Was there a v1 of this patch?
> > > >
> > > > Yes, but I don't know how to comments, ^_^
> > >
> > > As far as I can see, in the previous version of the series
> > > (https://patch
> > >
> work.ozlabs.org%2Fproject%2Flinuxppc-dev%2Flist%2F%3Fseries%3D125315
> > > %26state%3D*&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7C1b
> efe9
> > >
> a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c301635%
> > >
> 7C0%7C0%7C637022259387139020&amp;sdata=p4wbycd04Z7qRUfAoZtwc
> > > UP7pR%2FuA3%2FjVcWMz6YyQVQ%3D&amp;reserved=0),
> > > the 8/10 was something completely different, and I can't find any
> > > other patch in the series that could have been the v1 of this patch.
> >
> > Thanks, I will correct it to v1 in next version patch.
> 
> I think you numbered it correctly (so please leave it as v2, referring to the
> patch series revision) - I got confused trying to find a previous version of this
> patch.
> 
> Perhaps in the future when new patches are introduced in a series you can
> indicate that in the description patch revision history (e.g. introduced in v2).

OK, thanks for your help, I will update it in the next version patch.

Thanks 
Xiaowei

> 
> Thanks,
> 
> Andrew Murray
> 
> >
> > >
> > > Christophe
> > >
> > > >
> > > >>
> > > >>>
> > > >>>   drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > >>> ++++++++++++++++++++------
> > > >>>   1 file changed, 58 insertions(+), 18 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > >>> b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > >>> index 7ca5fe8..2a66f07 100644
> > > >>> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > >>> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > >>> @@ -20,27 +20,29 @@
> > > >>>
> > > >>>   #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > > >>>
> > > >>> -struct ls_pcie_ep {
> > > >>> -	struct dw_pcie		*pci;
> > > >>> -	struct pci_epc_features	*ls_epc;
> > > >>> +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > >>> +
> > > >>> +struct ls_pcie_ep_drvdata {
> > > >>> +	u32				func_offset;
> > > >>> +	const struct dw_pcie_ep_ops	*ops;
> > > >>> +	const struct dw_pcie_ops	*dw_pcie_ops;
> > > >>>   };
> > > >>>
> > > >>> -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > >>> +struct ls_pcie_ep {
> > > >>> +	struct dw_pcie			*pci;
> > > >>> +	struct pci_epc_features		*ls_epc;
> > > >>> +	const struct ls_pcie_ep_drvdata *drvdata; };
> > > >>>
> > > >>>   static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > > >>>   	return 0;
> > > >>>   }
> > > >>>
> > > >>> -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > >>> +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > > >>>   	.start_link = ls_pcie_establish_link,  };
> > > >>>
> > > >>> -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > >>> -	{ .compatible = "fsl,ls-pcie-ep",},
> > > >>> -	{ },
> > > >>> -};
> > > >>> -
> > > >>>   static const struct pci_epc_features*
> > > >>> ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10
> > > >>> +84,44 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8
> func_no,
> > > >>>   	}
> > > >>>   }
> > > >>>
> > > >>> -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > >>> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> *ep,
> > > >>> +						u8 func_no)
> > > >>> +{
> > > >>> +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > >>> +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > >>> +	u8 header_type;
> > > >>> +
> > > >>> +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > >>> +
> > > >>> +	if (header_type & (1 << 7))
> > > >>> +		return pcie->drvdata->func_offset * func_no;
> > > >>> +	else
> > > >>> +		return 0;
> > > >>
> > > >> It looks like there isn't a PCI define for multi function, the
> > > >> nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > > >> hotplug/ibmphp.h. A comment above the test might be helpful to
> > > >> explain
> > > the test.
> > > >
> > > > Yes, I have not find the PCI_HEADER_TYPE_MULTIDEVICE define. OK, I
> > > > will add The comments in next version patch.
> > > >
> > > >>
> > > >> As the ls_pcie_ep_drvdata structures are static, the unset
> > > >> .func_offset will be initialised to 0, so you could just drop the test
> above.
> > > >
> > > > OK, thanks
> > > >
> > > >>
> > > >> However something to the effect of the following may help spot
> > > >> misconfiguration:
> > > >>
> > > >> WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > > >> pcie->drvdata->func_offset * func_no;
> > > >
> > > > Thanks a lot, this looks better.
> > > >
> > > >>
> > > >> The WARN is probably quite useful as if you are attempting to use
> > > >> non-zero functions and func_offset isn't set - then things may
> > > >> appear to work normally but actually will break horribly.
> > > >
> > > > got it, thanks.
> > > >
> > > >>
> > > >> Thanks,
> > > >>
> > > >> Andrew Murray
> > > >>
> > > >>> +}
> > > >>> +
> > > >>> +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > > >>>   	.ep_init = ls_pcie_ep_init,
> > > >>>   	.raise_irq = ls_pcie_ep_raise_irq,
> > > >>>   	.get_features = ls_pcie_ep_get_features,
> > > >>> +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > > >>> +
> > > >>> +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > >>> +	.ops = &ls_pcie_ep_ops,
> > > >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > >>> +
> > > >>> +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > >>> +	.func_offset = 0x20000,
> > > >>> +	.ops = &ls_pcie_ep_ops,
> > > >>> +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > >>> +
> > > >>> +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > >>> +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > > >>> +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > >>> +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > >>> +	{ },
> > > >>>   };
> > > >>>
> > > >>>   static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@
> > > >>> -98,7
> > > >>> +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep
> > > >>> +*pcie,
> > > >>>   	int ret;
> > > >>>
> > > >>>   	ep = &pci->ep;
> > > >>> -	ep->ops = &pcie_ep_ops;
> > > >>> +	ep->ops = pcie->drvdata->ops;
> > > >>>
> > > >>>   	res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> > > >> "addr_space");
> > > >>>   	if (!res)
> > > >>> @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > > >> platform_device *pdev)
> > > >>>   	if (!ls_epc)
> > > >>>   		return -ENOMEM;
> > > >>>
> > > >>> -	dbi_base = platform_get_resource_byname(pdev,
> > > IORESOURCE_MEM,
> > > >> "regs");
> > > >>> -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > >>> -	if (IS_ERR(pci->dbi_base))
> > > >>> -		return PTR_ERR(pci->dbi_base);
> > > >>> +	pcie->drvdata = of_device_get_match_data(dev);
> > > >>>
> > > >>> -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > >>>   	pci->dev = dev;
> > > >>> -	pci->ops = &ls_pcie_ep_ops;
> > > >>> +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > > >>> +
> > > >>>   	pcie->pci = pci;
> > > >>>
> > > >>>   	ls_epc->linkup_notifier = false, @@ -152,6 +185,13 @@ static
> > > >>> int __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > >>>
> > > >>>   	pcie->ls_epc = ls_epc;
> > > >>>
> > > >>> +	dbi_base = platform_get_resource_byname(pdev,
> > > IORESOURCE_MEM,
> > > >> "regs");
> > > >>> +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > >>> +	if (IS_ERR(pci->dbi_base))
> > > >>> +		return PTR_ERR(pci->dbi_base);
> > > >>> +
> > > >>> +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > >>> +
> > > >>>   	platform_set_drvdata(pdev, pcie);
> > > >>>
> > > >>>   	ret = ls_add_pcie_ep(pcie, pdev);
> > > >>> --
> > > >>> 2.9.5
> > > >>>
> > >
> > > ---
> > > L'absence de virus dans ce courrier électronique a été vérifiée par
> > > le logiciel antivirus Avast.
> > > https://www.
> > >
> avast.com%2Fantivirus&amp;data=02%7C01%7Cxiaowei.bao%40nxp.com%7
> > >
> C1befe9a67c8046f9535e08d7285eaab6%7C686ea1d3bc2b4c6fa92cd99c5c3
> > >
> 01635%7C0%7C0%7C637022259387139020&amp;sdata=JAYds7X%2FHVxgtrg
> > > e%2F%2FvnP84zdb2yReXcctQUiSLC11I%3D&amp;reserved=0
> >

Xiaowei Bao Aug. 28, 2019, 4:29 a.m. UTC | #9

> -----Original Message-----
> From: Andrew Murray <[e-mail]>
> Sent: 2019年8月27日 21:34
> To: Xiaowei Bao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; M.h.
> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> Zang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Murray <[e-mail]>
> > > Sent: 2019年8月23日 22:28
> > > To: Xiaowei Bao <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> M.h.
> > > Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > > Zang <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > > for ls1088a and ls2088a
> > >
> > > On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > > > difference between LS1 and LS2 platform, so refactor the code of
> > > > the EP driver.
> > > >
> > > > Signed-off-by: Xiaowei Bao <[e-mail]>
> > > > ---
> > > > v2:
> > > >  - New mechanism for layerscape EP driver.
> > >
> > > Was there a v1 of this patch?
> > >
> > > >
> > > >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > > ++++++++++++++++++++------
> > > >  1 file changed, 58 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > index 7ca5fe8..2a66f07 100644
> > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > @@ -20,27 +20,29 @@
> > > >
> > > >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > > >
> > > > -struct ls_pcie_ep {
> > > > -	struct dw_pcie		*pci;
> > > > -	struct pci_epc_features	*ls_epc;
> > > > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > +
> > > > +struct ls_pcie_ep_drvdata {
> > > > +	u32				func_offset;
> > > > +	const struct dw_pcie_ep_ops	*ops;
> > > > +	const struct dw_pcie_ops	*dw_pcie_ops;
> > > >  };
> > > >
> > > > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > +struct ls_pcie_ep {
> > > > +	struct dw_pcie			*pci;
> > > > +	struct pci_epc_features		*ls_epc;
> > > > +	const struct ls_pcie_ep_drvdata *drvdata; };
> > > >
> > > >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > > >  	return 0;
> > > >  }
> > > >
> > > > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > > >  	.start_link = ls_pcie_establish_link,  };
> > > >
> > > > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > -	{ .compatible = "fsl,ls-pcie-ep",},
> > > > -	{ },
> > > > -};
> > > > -
> > > >  static const struct pci_epc_features*
> > > > ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10 +84,44
> > > > @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > > >  	}
> > > >  }
> > > >
> > > > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> *ep,
> > > > +						u8 func_no)
> > > > +{
> > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > > +	u8 header_type;
> > > > +
> > > > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > > +
> > > > +	if (header_type & (1 << 7))
> > > > +		return pcie->drvdata->func_offset * func_no;
> > > > +	else
> > > > +		return 0;
> > >
> > > It looks like there isn't a PCI define for multi function, the
> > > nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > > hotplug/ibmphp.h. A comment above the test might be helpful to explain
> the test.
> >
> > OK, I will add a comment above this code.
> >
> > >
> > > As the ls_pcie_ep_drvdata structures are static, the unset
> > > .func_offset will be initialised to 0, so you could just drop the test above.
> >
> > Due to the different PCIe controller have different property, e.g.
> > PCIe controller1 support multiple function feature, but PCIe
> > controller2 don't support this feature, so I need to check which
> > controller support it and return the correct offset value, but each board only
> have one ls_pcie_ep_drvdata, ^_^.
> 
> Yes but if they don't support the feature then func_offset will be 0.
> 
> >
> > >
> > > However something to the effect of the following may help spot
> > > misconfiguration:
> > >
> > > WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > > pcie->drvdata->func_offset * func_no;
> > >
> > > The WARN is probably quite useful as if you are attempting to use
> > > non-zero functions and func_offset isn't set - then things may
> > > appear to work normally but actually will break horribly.
> >
> > As discussion before, I think the func_offset should not depends on
> > the function number, even if other platforms of NXP may be use write
> > registers way to access the different function config space.
> 
> I agree that func_offset is an optional parameter. But if you are attempting to
> determine the offset of a function and you are given a non-zero function
> number - then something has gone wrong if func_offset is 0.

I have understood you means, maybe I need to set a flag in the driver_data struct,
because I may add other platform of NXP, these platform use the write register 
method to access different function, e.g. 
write func_num to register, then we can access this func_num config space.

I will modify the code like this? Do you have better advice?
Case1:
diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index 004a7e8..8a0d6df 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -23,6 +23,7 @@
 #define to_ls_pcie_ep(x)       dev_get_drvdata((x)->dev)

 struct ls_pcie_ep_drvdata {
+       u8                              func_config_flag;
        u32                             func_offset;
        const struct dw_pcie_ep_ops     *ops;
        const struct dw_pcie_ops        *dw_pcie_ops;
@@ -97,8 +98,14 @@ static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
         * Read the Header Type register of config space to check
         * whether this PCI device support the multiple function.
         */
-       if (header_type & (1 << 7))
-               return pcie->drvdata->func_offset * func_no;
+       if (header_type & (1 << 7)) {
+               if (pcie->drvdata->func_config_flag) {
+                       iowrite32((func_num << n), pci->dbi_base + PCI_XXXX_XXX);
+               } else {
+                       WARN_ON(func_no && !pcie->drvdata->func_offset);
+                       return pcie->drvdata->func_offset * func_no;
+               }
+       }

        return 0;
 }

Of course, I don't need to set the flag this time, because I don't use the second method(write
register method), so the code like this:
case2:
+static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
                                               u8 func_no) {
       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
       u8 header_type;

	   of course, this code is not requied, due to the 
	   pcie->drvdata->func_offset is 0, but I think this is more clear
	   if use this code.
       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);

       /*
        * Read the Header Type register of config space to check
        * whether this PCI device support the multiple function.
        */
       if (header_type & (1 << 7)) {
			   WARN_ON(func_no && !pcie->drvdata->func_offset);
               return pcie->drvdata->func_offset * func_no; 
		}
		
       return 0;
}

Or like this:
Case3:
+static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
                                               u8 func_no) {
       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);

	   WARN_ON(func_no && !pcie->drvdata->func_offset);
       return pcie->drvdata->func_offset * func_no;

}
Of course, we can return a -1 by adjuring the (func_no && !pcie->drvdata->func_offset) 
Valu in case1

Thanks 
Xiaowei

> 
> Thanks,
> 
> Andrew Murray
> 
> >
> > I have added the comments above the code, as follow, do you have any
> advice?
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > +                                               u8 func_no) {
> > +       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > +       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > +       u8 header_type;
> > +
> > +       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > +
> > +       /*
> > +        * Read the Header Type register of config space to check
> > +        * whether this PCI device support the multiple function.
> > +        */
> > +       if (header_type & (1 << 7))
> > +               return pcie->drvdata->func_offset * func_no;
> > +
> > +       return 0;
> > +}
> >
> > Thanks a lot for your detail comments.
> >
> > >
> > > Thanks,
> > >
> > > Andrew Murray
> > >
> > > > +}
> > > > +
> > > > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > > >  	.ep_init = ls_pcie_ep_init,
> > > >  	.raise_irq = ls_pcie_ep_raise_irq,
> > > >  	.get_features = ls_pcie_ep_get_features,
> > > > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > > > +
> > > > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > > +	.ops = &ls_pcie_ep_ops,
> > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > +
> > > > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > > +	.func_offset = 0x20000,
> > > > +	.ops = &ls_pcie_ep_ops,
> > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > +
> > > > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > > > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > +	{ },
> > > >  };
> > > >
> > > >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@
> > > > -98,7
> > > > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep
> > > > +*pcie,
> > > >  	int ret;
> > > >
> > > >  	ep = &pci->ep;
> > > > -	ep->ops = &pcie_ep_ops;
> > > > +	ep->ops = pcie->drvdata->ops;
> > > >
> > > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > "addr_space");
> > > >  	if (!res)
> > > > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > > platform_device *pdev)
> > > >  	if (!ls_epc)
> > > >  		return -ENOMEM;
> > > >
> > > > -	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> > > "regs");
> > > > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > -	if (IS_ERR(pci->dbi_base))
> > > > -		return PTR_ERR(pci->dbi_base);
> > > > +	pcie->drvdata = of_device_get_match_data(dev);
> > > >
> > > > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > >  	pci->dev = dev;
> > > > -	pci->ops = &ls_pcie_ep_ops;
> > > > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > > > +
> > > >  	pcie->pci = pci;
> > > >
> > > >  	ls_epc->linkup_notifier = false, @@ -152,6 +185,13 @@ static int
> > > > __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > >
> > > >  	pcie->ls_epc = ls_epc;
> > > >
> > > > +	dbi_base = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> > > "regs");
> > > > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > +	if (IS_ERR(pci->dbi_base))
> > > > +		return PTR_ERR(pci->dbi_base);
> > > > +
> > > > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > +
> > > >  	platform_set_drvdata(pdev, pcie);
> > > >
> > > >  	ret = ls_add_pcie_ep(pcie, pdev);
> > > > --
> > > > 2.9.5
> > > >

Andrew Murray Aug. 28, 2019, 9:01 a.m. UTC | #10

On Wed, Aug 28, 2019 at 04:29:32AM +0000, Xiaowei Bao wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Murray <[e-mail]>
> > Sent: 2019年8月27日 21:34
> > To: Xiaowei Bao <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; M.h.
> > Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > Zang <[e-mail]>; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; [e-mail]
> > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> > ls1088a and ls2088a
> > 
> > On Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Andrew Murray <[e-mail]>
> > > > Sent: 2019年8月23日 22:28
> > > > To: Xiaowei Bao <[e-mail]>
> > > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail];
> > M.h.
> > > > Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > > > Zang <[e-mail]>; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail]
> > > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > > > for ls1088a and ls2088a
> > > >
> > > > On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > > > Add PCIe EP mode support for ls1088a and ls2088a, there are some
> > > > > difference between LS1 and LS2 platform, so refactor the code of
> > > > > the EP driver.
> > > > >
> > > > > Signed-off-by: Xiaowei Bao <[e-mail]>
> > > > > ---
> > > > > v2:
> > > > >  - New mechanism for layerscape EP driver.
> > > >
> > > > Was there a v1 of this patch?
> > > >
> > > > >
> > > > >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > > > ++++++++++++++++++++------
> > > > >  1 file changed, 58 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > index 7ca5fe8..2a66f07 100644
> > > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > @@ -20,27 +20,29 @@
> > > > >
> > > > >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > > > >
> > > > > -struct ls_pcie_ep {
> > > > > -	struct dw_pcie		*pci;
> > > > > -	struct pci_epc_features	*ls_epc;
> > > > > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > > +
> > > > > +struct ls_pcie_ep_drvdata {
> > > > > +	u32				func_offset;
> > > > > +	const struct dw_pcie_ep_ops	*ops;
> > > > > +	const struct dw_pcie_ops	*dw_pcie_ops;
> > > > >  };
> > > > >
> > > > > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > > +struct ls_pcie_ep {
> > > > > +	struct dw_pcie			*pci;
> > > > > +	struct pci_epc_features		*ls_epc;
> > > > > +	const struct ls_pcie_ep_drvdata *drvdata; };
> > > > >
> > > > >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > > > >  	return 0;
> > > > >  }
> > > > >
> > > > > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > > > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > > > >  	.start_link = ls_pcie_establish_link,  };
> > > > >
> > > > > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > -	{ .compatible = "fsl,ls-pcie-ep",},
> > > > > -	{ },
> > > > > -};
> > > > > -
> > > > >  static const struct pci_epc_features*
> > > > > ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10 +84,44
> > > > > @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > > > >  	}
> > > > >  }
> > > > >
> > > > > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> > *ep,
> > > > > +						u8 func_no)
> > > > > +{
> > > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > > > +	u8 header_type;
> > > > > +
> > > > > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > > > +
> > > > > +	if (header_type & (1 << 7))
> > > > > +		return pcie->drvdata->func_offset * func_no;
> > > > > +	else
> > > > > +		return 0;
> > > >
> > > > It looks like there isn't a PCI define for multi function, the
> > > > nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > > > hotplug/ibmphp.h. A comment above the test might be helpful to explain
> > the test.
> > >
> > > OK, I will add a comment above this code.
> > >
> > > >
> > > > As the ls_pcie_ep_drvdata structures are static, the unset
> > > > .func_offset will be initialised to 0, so you could just drop the test above.
> > >
> > > Due to the different PCIe controller have different property, e.g.
> > > PCIe controller1 support multiple function feature, but PCIe
> > > controller2 don't support this feature, so I need to check which
> > > controller support it and return the correct offset value, but each board only
> > have one ls_pcie_ep_drvdata, ^_^.
> > 
> > Yes but if they don't support the feature then func_offset will be 0.
> > 
> > >
> > > >
> > > > However something to the effect of the following may help spot
> > > > misconfiguration:
> > > >
> > > > WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > > > pcie->drvdata->func_offset * func_no;
> > > >
> > > > The WARN is probably quite useful as if you are attempting to use
> > > > non-zero functions and func_offset isn't set - then things may
> > > > appear to work normally but actually will break horribly.
> > >
> > > As discussion before, I think the func_offset should not depends on
> > > the function number, even if other platforms of NXP may be use write
> > > registers way to access the different function config space.
> > 
> > I agree that func_offset is an optional parameter. But if you are attempting to
> > determine the offset of a function and you are given a non-zero function
> > number - then something has gone wrong if func_offset is 0.
> 
> I have understood you means, maybe I need to set a flag in the driver_data struct,
> because I may add other platform of NXP, these platform use the write register 
> method to access different function, e.g. 
> write func_num to register, then we can access this func_num config space.
> 
> I will modify the code like this? Do you have better advice?
> Case1:
> diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> index 004a7e8..8a0d6df 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> @@ -23,6 +23,7 @@
>  #define to_ls_pcie_ep(x)       dev_get_drvdata((x)->dev)
> 
>  struct ls_pcie_ep_drvdata {
> +       u8                              func_config_flag;
>         u32                             func_offset;
>         const struct dw_pcie_ep_ops     *ops;
>         const struct dw_pcie_ops        *dw_pcie_ops;
> @@ -97,8 +98,14 @@ static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
>          * Read the Header Type register of config space to check
>          * whether this PCI device support the multiple function.
>          */
> -       if (header_type & (1 << 7))
> -               return pcie->drvdata->func_offset * func_no;
> +       if (header_type & (1 << 7)) {
> +               if (pcie->drvdata->func_config_flag) {
> +                       iowrite32((func_num << n), pci->dbi_base + PCI_XXXX_XXX);
> +               } else {
> +                       WARN_ON(func_no && !pcie->drvdata->func_offset);
> +                       return pcie->drvdata->func_offset * func_no;
> +               }
> +       }
> 
>         return 0;
>  }
> 
> Of course, I don't need to set the flag this time, because I don't use the second method(write
> register method), so the code like this:
> case2:
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
>                                                u8 func_no) {
>        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>        struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
>        u8 header_type;
> 
> 	   of course, this code is not requied, due to the 
> 	   pcie->drvdata->func_offset is 0, but I think this is more clear
> 	   if use this code.
>        header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> 
>        /*
>         * Read the Header Type register of config space to check
>         * whether this PCI device support the multiple function.
>         */
>        if (header_type & (1 << 7)) {
> 			   WARN_ON(func_no && !pcie->drvdata->func_offset);
>                return pcie->drvdata->func_offset * func_no; 
> 		}
> 		
>        return 0;
> }
> 
> Or like this:
> Case3:
> +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
>                                                u8 func_no) {
>        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
>        struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> 
> 	   WARN_ON(func_no && !pcie->drvdata->func_offset);
>        return pcie->drvdata->func_offset * func_no;

This is better. Given there is only currently one method of calculating
an offset for layerscape, I'd recommend you add additional methods when
the need arises.

Thanks,

Andrew Murray

> 
> }
> Of course, we can return a -1 by adjuring the (func_no && !pcie->drvdata->func_offset) 
> Valu in case1
> 
> Thanks 
> Xiaowei
> 
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > >
> > > I have added the comments above the code, as follow, do you have any
> > advice?
> > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> > > +                                               u8 func_no) {
> > > +       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > +       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > +       u8 header_type;
> > > +
> > > +       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > +
> > > +       /*
> > > +        * Read the Header Type register of config space to check
> > > +        * whether this PCI device support the multiple function.
> > > +        */
> > > +       if (header_type & (1 << 7))
> > > +               return pcie->drvdata->func_offset * func_no;
> > > +
> > > +       return 0;
> > > +}
> > >
> > > Thanks a lot for your detail comments.
> > >
> > > >
> > > > Thanks,
> > > >
> > > > Andrew Murray
> > > >
> > > > > +}
> > > > > +
> > > > > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > > > >  	.ep_init = ls_pcie_ep_init,
> > > > >  	.raise_irq = ls_pcie_ep_raise_irq,
> > > > >  	.get_features = ls_pcie_ep_get_features,
> > > > > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > > > > +
> > > > > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > > > +	.ops = &ls_pcie_ep_ops,
> > > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > +
> > > > > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > > > +	.func_offset = 0x20000,
> > > > > +	.ops = &ls_pcie_ep_ops,
> > > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > +
> > > > > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
> > > > > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
> > > > > +	{ },
> > > > >  };
> > > > >
> > > > >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@
> > > > > -98,7
> > > > > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep
> > > > > +*pcie,
> > > > >  	int ret;
> > > > >
> > > > >  	ep = &pci->ep;
> > > > > -	ep->ops = &pcie_ep_ops;
> > > > > +	ep->ops = pcie->drvdata->ops;
> > > > >
> > > > >  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> > > > "addr_space");
> > > > >  	if (!res)
> > > > > @@ -137,14 +173,11 @@ static int __init ls_pcie_ep_probe(struct
> > > > platform_device *pdev)
> > > > >  	if (!ls_epc)
> > > > >  		return -ENOMEM;
> > > > >
> > > > > -	dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > > > "regs");
> > > > > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > > -	if (IS_ERR(pci->dbi_base))
> > > > > -		return PTR_ERR(pci->dbi_base);
> > > > > +	pcie->drvdata = of_device_get_match_data(dev);
> > > > >
> > > > > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > >  	pci->dev = dev;
> > > > > -	pci->ops = &ls_pcie_ep_ops;
> > > > > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > > > > +
> > > > >  	pcie->pci = pci;
> > > > >
> > > > >  	ls_epc->linkup_notifier = false, @@ -152,6 +185,13 @@ static int
> > > > > __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > > >
> > > > >  	pcie->ls_epc = ls_epc;
> > > > >
> > > > > +	dbi_base = platform_get_resource_byname(pdev,
> > IORESOURCE_MEM,
> > > > "regs");
> > > > > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
> > > > > +	if (IS_ERR(pci->dbi_base))
> > > > > +		return PTR_ERR(pci->dbi_base);
> > > > > +
> > > > > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > > +
> > > > >  	platform_set_drvdata(pdev, pcie);
> > > > >
> > > > >  	ret = ls_add_pcie_ep(pcie, pdev);
> > > > > --
> > > > > 2.9.5
> > > > >

Xiaowei Bao Aug. 29, 2019, 2:03 a.m. UTC | #11

> -----Original Message-----
> From: Andrew Murray <[e-mail]>
> Sent: 2019年8月28日 17:01
> To: Xiaowei Bao <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Leo Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; M.h.
> Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> Zang <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support for
> ls1088a and ls2088a
> 
> On Wed, Aug 28, 2019 at 04:29:32AM +0000, Xiaowei Bao wrote:
> >
> >
> > > -----Original Message-----
> > > From: Andrew Murray <[e-mail]>
> > > Sent: 2019年8月27日 21:34
> > > To: Xiaowei Bao <[e-mail]>
> > > Cc: [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; Leo Li <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> M.h.
> > > Lian <[e-mail]>; Mingkai Hu <[e-mail]>; Roy
> > > Zang <[e-mail]>; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]
> > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode support
> > > for ls1088a and ls2088a
> > >
> > > On Mon, Aug 26, 2019 at 09:49:35AM +0000, Xiaowei Bao wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Andrew Murray <[e-mail]>
> > > > > Sent: 2019年8月23日 22:28
> > > > > To: Xiaowei Bao <[e-mail]>
> > > > > Cc: [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail]; Leo Li
> > > > > <[e-mail]>; [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > M.h.
> > > > > Lian <[e-mail]>; Mingkai Hu <[e-mail]>;
> > > > > Roy Zang <[e-mail]>; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];
> > > > > [e-mail];
> > > > > [e-mail]
> > > > > Subject: Re: [PATCH v2 08/10] PCI: layerscape: Add EP mode
> > > > > support for ls1088a and ls2088a
> > > > >
> > > > > On Thu, Aug 22, 2019 at 07:22:40PM +0800, Xiaowei Bao wrote:
> > > > > > Add PCIe EP mode support for ls1088a and ls2088a, there are
> > > > > > some difference between LS1 and LS2 platform, so refactor the
> > > > > > code of the EP driver.
> > > > > >
> > > > > > Signed-off-by: Xiaowei Bao <[e-mail]>
> > > > > > ---
> > > > > > v2:
> > > > > >  - New mechanism for layerscape EP driver.
> > > > >
> > > > > Was there a v1 of this patch?
> > > > >
> > > > > >
> > > > > >  drivers/pci/controller/dwc/pci-layerscape-ep.c | 76
> > > > > > ++++++++++++++++++++------
> > > > > >  1 file changed, 58 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > > index 7ca5fe8..2a66f07 100644
> > > > > > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > > > > > @@ -20,27 +20,29 @@
> > > > > >
> > > > > >  #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
> > > > > >
> > > > > > -struct ls_pcie_ep {
> > > > > > -	struct dw_pcie		*pci;
> > > > > > -	struct pci_epc_features	*ls_epc;
> > > > > > +#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > > > +
> > > > > > +struct ls_pcie_ep_drvdata {
> > > > > > +	u32				func_offset;
> > > > > > +	const struct dw_pcie_ep_ops	*ops;
> > > > > > +	const struct dw_pcie_ops	*dw_pcie_ops;
> > > > > >  };
> > > > > >
> > > > > > -#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
> > > > > > +struct ls_pcie_ep {
> > > > > > +	struct dw_pcie			*pci;
> > > > > > +	struct pci_epc_features		*ls_epc;
> > > > > > +	const struct ls_pcie_ep_drvdata *drvdata; };
> > > > > >
> > > > > >  static int ls_pcie_establish_link(struct dw_pcie *pci)  {
> > > > > >  	return 0;
> > > > > >  }
> > > > > >
> > > > > > -static const struct dw_pcie_ops ls_pcie_ep_ops = {
> > > > > > +static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
> > > > > >  	.start_link = ls_pcie_establish_link,  };
> > > > > >
> > > > > > -static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > > -	{ .compatible = "fsl,ls-pcie-ep",},
> > > > > > -	{ },
> > > > > > -};
> > > > > > -
> > > > > >  static const struct pci_epc_features*
> > > > > > ls_pcie_ep_get_features(struct dw_pcie_ep *ep)  { @@ -82,10
> > > > > > +84,44 @@ static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep,
> u8 func_no,
> > > > > >  	}
> > > > > >  }
> > > > > >
> > > > > > -static const struct dw_pcie_ep_ops pcie_ep_ops = {
> > > > > > +static unsigned int ls_pcie_ep_func_conf_select(struct
> > > > > > +dw_pcie_ep
> > > *ep,
> > > > > > +						u8 func_no)
> > > > > > +{
> > > > > > +	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > > > +	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > > > > +	u8 header_type;
> > > > > > +
> > > > > > +	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > > > > +
> > > > > > +	if (header_type & (1 << 7))
> > > > > > +		return pcie->drvdata->func_offset * func_no;
> > > > > > +	else
> > > > > > +		return 0;
> > > > >
> > > > > It looks like there isn't a PCI define for multi function, the
> > > > > nearest I could find was PCI_HEADER_TYPE_MULTIDEVICE in
> > > > > hotplug/ibmphp.h. A comment above the test might be helpful to
> > > > > explain
> > > the test.
> > > >
> > > > OK, I will add a comment above this code.
> > > >
> > > > >
> > > > > As the ls_pcie_ep_drvdata structures are static, the unset
> > > > > .func_offset will be initialised to 0, so you could just drop the test
> above.
> > > >
> > > > Due to the different PCIe controller have different property, e.g.
> > > > PCIe controller1 support multiple function feature, but PCIe
> > > > controller2 don't support this feature, so I need to check which
> > > > controller support it and return the correct offset value, but
> > > > each board only
> > > have one ls_pcie_ep_drvdata, ^_^.
> > >
> > > Yes but if they don't support the feature then func_offset will be 0.
> > >
> > > >
> > > > >
> > > > > However something to the effect of the following may help spot
> > > > > misconfiguration:
> > > > >
> > > > > WARN_ON(func_no && !pcie->drvdata->func_offset); return
> > > > > pcie->drvdata->func_offset * func_no;
> > > > >
> > > > > The WARN is probably quite useful as if you are attempting to
> > > > > use non-zero functions and func_offset isn't set - then things
> > > > > may appear to work normally but actually will break horribly.
> > > >
> > > > As discussion before, I think the func_offset should not depends
> > > > on the function number, even if other platforms of NXP may be use
> > > > write registers way to access the different function config space.
> > >
> > > I agree that func_offset is an optional parameter. But if you are
> > > attempting to determine the offset of a function and you are given a
> > > non-zero function number - then something has gone wrong if func_offset
> is 0.
> >
> > I have understood you means, maybe I need to set a flag in the
> > driver_data struct, because I may add other platform of NXP, these
> > platform use the write register method to access different function, e.g.
> > write func_num to register, then we can access this func_num config space.
> >
> > I will modify the code like this? Do you have better advice?
> > Case1:
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > index 004a7e8..8a0d6df 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
> > @@ -23,6 +23,7 @@
> >  #define to_ls_pcie_ep(x)       dev_get_drvdata((x)->dev)
> >
> >  struct ls_pcie_ep_drvdata {
> > +       u8                              func_config_flag;
> >         u32                             func_offset;
> >         const struct dw_pcie_ep_ops     *ops;
> >         const struct dw_pcie_ops        *dw_pcie_ops;
> > @@ -97,8 +98,14 @@ static unsigned int
> ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
> >          * Read the Header Type register of config space to check
> >          * whether this PCI device support the multiple function.
> >          */
> > -       if (header_type & (1 << 7))
> > -               return pcie->drvdata->func_offset * func_no;
> > +       if (header_type & (1 << 7)) {
> > +               if (pcie->drvdata->func_config_flag) {
> > +                       iowrite32((func_num << n), pci->dbi_base +
> PCI_XXXX_XXX);
> > +               } else {
> > +                       WARN_ON(func_no
> && !pcie->drvdata->func_offset);
> > +                       return pcie->drvdata->func_offset * func_no;
> > +               }
> > +       }
> >
> >         return 0;
> >  }
> >
> > Of course, I don't need to set the flag this time, because I don't use
> > the second method(write register method), so the code like this:
> > case2:
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> > +*ep,
> >                                                u8 func_no) {
> >        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >        struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> >        u8 header_type;
> >
> > 	   of course, this code is not requied, due to the
> > 	   pcie->drvdata->func_offset is 0, but I think this is more clear
> > 	   if use this code.
> >        header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> >
> >        /*
> >         * Read the Header Type register of config space to check
> >         * whether this PCI device support the multiple function.
> >         */
> >        if (header_type & (1 << 7)) {
> > 			   WARN_ON(func_no && !pcie->drvdata->func_offset);
> >                return pcie->drvdata->func_offset * func_no;
> > 		}
> >
> >        return 0;
> > }
> >
> > Or like this:
> > Case3:
> > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> > +*ep,
> >                                                u8 func_no) {
> >        struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >        struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> >
> > 	   WARN_ON(func_no && !pcie->drvdata->func_offset);
> >        return pcie->drvdata->func_offset * func_no;
> 
> This is better. Given there is only currently one method of calculating an offset
> for layerscape, I'd recommend you add additional methods when the need
> arises.

OK, thanks

> 
> Thanks,
> 
> Andrew Murray
> 
> >
> > }
> > Of course, we can return a -1 by adjuring the (func_no &&
> > !pcie->drvdata->func_offset) Valu in case1
> >
> > Thanks
> > Xiaowei
> >
> > >
> > > Thanks,
> > >
> > > Andrew Murray
> > >
> > > >
> > > > I have added the comments above the code, as follow, do you have
> > > > any
> > > advice?
> > > > +static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep
> *ep,
> > > > +                                               u8 func_no)
> {
> > > > +       struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> > > > +       struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
> > > > +       u8 header_type;
> > > > +
> > > > +       header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
> > > > +
> > > > +       /*
> > > > +        * Read the Header Type register of config space to check
> > > > +        * whether this PCI device support the multiple function.
> > > > +        */
> > > > +       if (header_type & (1 << 7))
> > > > +               return pcie->drvdata->func_offset * func_no;
> > > > +
> > > > +       return 0;
> > > > +}
> > > >
> > > > Thanks a lot for your detail comments.
> > > >
> > > > >
> > > > > Thanks,
> > > > >
> > > > > Andrew Murray
> > > > >
> > > > > > +}
> > > > > > +
> > > > > > +static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
> > > > > >  	.ep_init = ls_pcie_ep_init,
> > > > > >  	.raise_irq = ls_pcie_ep_raise_irq,
> > > > > >  	.get_features = ls_pcie_ep_get_features,
> > > > > > +	.func_conf_select = ls_pcie_ep_func_conf_select, };
> > > > > > +
> > > > > > +static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
> > > > > > +	.ops = &ls_pcie_ep_ops,
> > > > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > > +
> > > > > > +static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
> > > > > > +	.func_offset = 0x20000,
> > > > > > +	.ops = &ls_pcie_ep_ops,
> > > > > > +	.dw_pcie_ops = &dw_ls_pcie_ep_ops, };
> > > > > > +
> > > > > > +static const struct of_device_id ls_pcie_ep_of_match[] = {
> > > > > > +	{ .compatible = "fsl,ls1046a-pcie-ep", .data =
> &ls1_ep_drvdata },
> > > > > > +	{ .compatible = "fsl,ls1088a-pcie-ep", .data =
> &ls2_ep_drvdata },
> > > > > > +	{ .compatible = "fsl,ls2088a-pcie-ep", .data =
> &ls2_ep_drvdata },
> > > > > > +	{ },
> > > > > >  };
> > > > > >
> > > > > >  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie, @@
> > > > > > -98,7
> > > > > > +134,7 @@ static int __init ls_add_pcie_ep(struct ls_pcie_ep
> > > > > > +*pcie,
> > > > > >  	int ret;
> > > > > >
> > > > > >  	ep = &pci->ep;
> > > > > > -	ep->ops = &pcie_ep_ops;
> > > > > > +	ep->ops = pcie->drvdata->ops;
> > > > > >
> > > > > >  	res = platform_get_resource_byname(pdev,
> IORESOURCE_MEM,
> > > > > "addr_space");
> > > > > >  	if (!res)
> > > > > > @@ -137,14 +173,11 @@ static int __init
> > > > > > ls_pcie_ep_probe(struct
> > > > > platform_device *pdev)
> > > > > >  	if (!ls_epc)
> > > > > >  		return -ENOMEM;
> > > > > >
> > > > > > -	dbi_base = platform_get_resource_byname(pdev,
> > > IORESOURCE_MEM,
> > > > > "regs");
> > > > > > -	pci->dbi_base = devm_pci_remap_cfg_resource(dev,
> dbi_base);
> > > > > > -	if (IS_ERR(pci->dbi_base))
> > > > > > -		return PTR_ERR(pci->dbi_base);
> > > > > > +	pcie->drvdata = of_device_get_match_data(dev);
> > > > > >
> > > > > > -	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > > >  	pci->dev = dev;
> > > > > > -	pci->ops = &ls_pcie_ep_ops;
> > > > > > +	pci->ops = pcie->drvdata->dw_pcie_ops;
> > > > > > +
> > > > > >  	pcie->pci = pci;
> > > > > >
> > > > > >  	ls_epc->linkup_notifier = false, @@ -152,6 +185,13 @@ static
> > > > > > int __init ls_pcie_ep_probe(struct platform_device *pdev)
> > > > > >
> > > > > >  	pcie->ls_epc = ls_epc;
> > > > > >
> > > > > > +	dbi_base = platform_get_resource_byname(pdev,
> > > IORESOURCE_MEM,
> > > > > "regs");
> > > > > > +	pci->dbi_base = devm_pci_remap_cfg_resource(dev,
> dbi_base);
> > > > > > +	if (IS_ERR(pci->dbi_base))
> > > > > > +		return PTR_ERR(pci->dbi_base);
> > > > > > +
> > > > > > +	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
> > > > > > +
> > > > > >  	platform_set_drvdata(pdev, pcie);
> > > > > >
> > > > > >  	ret = ls_add_pcie_ep(pcie, pdev);
> > > > > > --
> > > > > > 2.9.5
> > > > > >

Patch

diff --git a/drivers/pci/controller/dwc/pci-layerscape-ep.c b/drivers/pci/controller/dwc/pci-layerscape-ep.c
index 7ca5fe8..2a66f07 100644
--- a/drivers/pci/controller/dwc/pci-layerscape-ep.c
+++ b/drivers/pci/controller/dwc/pci-layerscape-ep.c
@@ -20,27 +20,29 @@ 
 
 #define PCIE_DBI2_OFFSET		0x1000	/* DBI2 base address*/
 
-struct ls_pcie_ep {
-	struct dw_pcie		*pci;
-	struct pci_epc_features	*ls_epc;
+#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
+
+struct ls_pcie_ep_drvdata {
+	u32				func_offset;
+	const struct dw_pcie_ep_ops	*ops;
+	const struct dw_pcie_ops	*dw_pcie_ops;
 };
 
-#define to_ls_pcie_ep(x)	dev_get_drvdata((x)->dev)
+struct ls_pcie_ep {
+	struct dw_pcie			*pci;
+	struct pci_epc_features		*ls_epc;
+	const struct ls_pcie_ep_drvdata *drvdata;
+};
 
 static int ls_pcie_establish_link(struct dw_pcie *pci)
 {
 	return 0;
 }
 
-static const struct dw_pcie_ops ls_pcie_ep_ops = {
+static const struct dw_pcie_ops dw_ls_pcie_ep_ops = {
 	.start_link = ls_pcie_establish_link,
 };
 
-static const struct of_device_id ls_pcie_ep_of_match[] = {
-	{ .compatible = "fsl,ls-pcie-ep",},
-	{ },
-};
-
 static const struct pci_epc_features*
 ls_pcie_ep_get_features(struct dw_pcie_ep *ep)
 {
@@ -82,10 +84,44 @@  static int ls_pcie_ep_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
 	}
 }
 
-static const struct dw_pcie_ep_ops pcie_ep_ops = {
+static unsigned int ls_pcie_ep_func_conf_select(struct dw_pcie_ep *ep,
+						u8 func_no)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
+	struct ls_pcie_ep *pcie = to_ls_pcie_ep(pci);
+	u8 header_type;
+
+	header_type = ioread8(pci->dbi_base + PCI_HEADER_TYPE);
+
+	if (header_type & (1 << 7))
+		return pcie->drvdata->func_offset * func_no;
+	else
+		return 0;
+}
+
+static const struct dw_pcie_ep_ops ls_pcie_ep_ops = {
 	.ep_init = ls_pcie_ep_init,
 	.raise_irq = ls_pcie_ep_raise_irq,
 	.get_features = ls_pcie_ep_get_features,
+	.func_conf_select = ls_pcie_ep_func_conf_select,
+};
+
+static const struct ls_pcie_ep_drvdata ls1_ep_drvdata = {
+	.ops = &ls_pcie_ep_ops,
+	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
+};
+
+static const struct ls_pcie_ep_drvdata ls2_ep_drvdata = {
+	.func_offset = 0x20000,
+	.ops = &ls_pcie_ep_ops,
+	.dw_pcie_ops = &dw_ls_pcie_ep_ops,
+};
+
+static const struct of_device_id ls_pcie_ep_of_match[] = {
+	{ .compatible = "fsl,ls1046a-pcie-ep", .data = &ls1_ep_drvdata },
+	{ .compatible = "fsl,ls1088a-pcie-ep", .data = &ls2_ep_drvdata },
+	{ .compatible = "fsl,ls2088a-pcie-ep", .data = &ls2_ep_drvdata },
+	{ },
 };
 
 static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
@@ -98,7 +134,7 @@  static int __init ls_add_pcie_ep(struct ls_pcie_ep *pcie,
 	int ret;
 
 	ep = &pci->ep;
-	ep->ops = &pcie_ep_ops;
+	ep->ops = pcie->drvdata->ops;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "addr_space");
 	if (!res)
@@ -137,14 +173,11 @@  static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 	if (!ls_epc)
 		return -ENOMEM;
 
-	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
-	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
-	if (IS_ERR(pci->dbi_base))
-		return PTR_ERR(pci->dbi_base);
+	pcie->drvdata = of_device_get_match_data(dev);
 
-	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
 	pci->dev = dev;
-	pci->ops = &ls_pcie_ep_ops;
+	pci->ops = pcie->drvdata->dw_pcie_ops;
+
 	pcie->pci = pci;
 
 	ls_epc->linkup_notifier = false,
@@ -152,6 +185,13 @@  static int __init ls_pcie_ep_probe(struct platform_device *pdev)
 
 	pcie->ls_epc = ls_epc;
 
+	dbi_base = platform_get_resource_byname(pdev, IORESOURCE_MEM, "regs");
+	pci->dbi_base = devm_pci_remap_cfg_resource(dev, dbi_base);
+	if (IS_ERR(pci->dbi_base))
+		return PTR_ERR(pci->dbi_base);
+
+	pci->dbi_base2 = pci->dbi_base + PCIE_DBI2_OFFSET;
+
 	platform_set_drvdata(pdev, pcie);
 
 	ret = ls_add_pcie_ep(pcie, pdev);