diff mbox

[09/37] PCI: dwc: designware: Parse *num-lanes* property in dw_pcie_setup_rc

Message ID 1484216786-17292-10-git-send-email-kishon@ti.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Kishon Vijay Abraham I Jan. 12, 2017, 10:25 a.m. UTC
*num-lanes* dt property is parsed in dw_pcie_host_init. However
*num-lanes* property is applicable to both root complex mode and
endpoint mode. As a first step, move the parsing of this property
outside dw_pcie_host_init. This is in preparation for splitting
pcie-designware.c to pcie-designware.c and pcie-designware-host.c

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/dwc/pcie-designware.c |   18 +++++++++++-------
 drivers/pci/dwc/pcie-designware.h |    1 -
 2 files changed, 11 insertions(+), 8 deletions(-)

Comments

Joao Pinto Jan. 13, 2017, 5:13 p.m. UTC | #1
Hi,

Às 10:25 AM de 1/12/2017, Kishon Vijay Abraham I escreveu:
> *num-lanes* dt property is parsed in dw_pcie_host_init. However
> *num-lanes* property is applicable to both root complex mode and
> endpoint mode. As a first step, move the parsing of this property
> outside dw_pcie_host_init. This is in preparation for splitting
> pcie-designware.c to pcie-designware.c and pcie-designware-host.c
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/dwc/pcie-designware.c |   18 +++++++++++-------
>  drivers/pci/dwc/pcie-designware.h |    1 -
>  2 files changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
> index 00a0fdc..89cdb6b 100644
> --- a/drivers/pci/dwc/pcie-designware.c
> +++ b/drivers/pci/dwc/pcie-designware.c
> @@ -551,10 +551,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>  		}
>  	}
>  
> -	ret = of_property_read_u32(np, "num-lanes", &pci->lanes);
> -	if (ret)
> -		pci->lanes = 0;
> -
>  	ret = of_property_read_u32(np, "num-viewport", &pci->num_viewport);
>  	if (ret)
>  		pci->num_viewport = 2;
> @@ -751,18 +747,26 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>  
>  void dw_pcie_setup_rc(struct pcie_port *pp)
>  {
> +	int ret;
> +	u32 lanes;
>  	u32 val;
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct device *dev = pci->dev;
> +	struct device_node *np = dev->of_node;
>  
>  	/* get iATU unroll support */
>  	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>  	dev_dbg(pci->dev, "iATU unroll: %s\n",
>  		pci->iatu_unroll_enabled ? "enabled" : "disabled");
>  
> +	ret = of_property_read_u32(np, "num-lanes", &lanes);
> +	if (ret)
> +		lanes = 0;

You moved from host_init to root complex setup function, which in my opinion did
not improve (in this scope).

I suggest that instead of making so much intermediary patches, which is nice to
understand your development sequence, but hard to review. Wouldn't be better to
condense some of the patches? We would have a cloear vision of the final product :)

Joao

> +
>  	/* set the number of lanes */
>  	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
>  	val &= ~PORT_LINK_MODE_MASK;
> -	switch (pci->lanes) {
> +	switch (lanes) {
>  	case 1:
>  		val |= PORT_LINK_MODE_1_LANES;
>  		break;
> @@ -776,7 +780,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  		val |= PORT_LINK_MODE_8_LANES;
>  		break;
>  	default:
> -		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->lanes);
> +		dev_err(pci->dev, "num-lanes %u: invalid value\n", lanes);
>  		return;
>  	}
>  	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
> @@ -784,7 +788,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)
>  	/* set link width speed control register */
>  	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
>  	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
> -	switch (pci->lanes) {
> +	switch (lanes) {
>  	case 1:
>  		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
>  		break;
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index d4b3d43..491fbe3 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -148,7 +148,6 @@ struct dw_pcie_ops {
>  struct dw_pcie {
>  	struct device		*dev;
>  	void __iomem		*dbi_base;
> -	u32			lanes;
>  	u32			num_viewport;
>  	u8			iatu_unroll_enabled;
>  	struct pcie_port	pp;
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kishon Vijay Abraham I Jan. 16, 2017, 5:19 a.m. UTC | #2
Hi,

On Friday 13 January 2017 10:43 PM, Joao Pinto wrote:
> Hi,
> 
> Às 10:25 AM de 1/12/2017, Kishon Vijay Abraham I escreveu:
>> *num-lanes* dt property is parsed in dw_pcie_host_init. However
>> *num-lanes* property is applicable to both root complex mode and
>> endpoint mode. As a first step, move the parsing of this property
>> outside dw_pcie_host_init. This is in preparation for splitting
>> pcie-designware.c to pcie-designware.c and pcie-designware-host.c
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/dwc/pcie-designware.c |   18 +++++++++++-------
>>  drivers/pci/dwc/pcie-designware.h |    1 -
>>  2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
>> index 00a0fdc..89cdb6b 100644
>> --- a/drivers/pci/dwc/pcie-designware.c
>> +++ b/drivers/pci/dwc/pcie-designware.c
>> @@ -551,10 +551,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>  		}
>>  	}
>>  
>> -	ret = of_property_read_u32(np, "num-lanes", &pci->lanes);
>> -	if (ret)
>> -		pci->lanes = 0;
>> -
>>  	ret = of_property_read_u32(np, "num-viewport", &pci->num_viewport);
>>  	if (ret)
>>  		pci->num_viewport = 2;
>> @@ -751,18 +747,26 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>>  
>>  void dw_pcie_setup_rc(struct pcie_port *pp)
>>  {
>> +	int ret;
>> +	u32 lanes;
>>  	u32 val;
>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct device *dev = pci->dev;
>> +	struct device_node *np = dev->of_node;
>>  
>>  	/* get iATU unroll support */
>>  	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>>  	dev_dbg(pci->dev, "iATU unroll: %s\n",
>>  		pci->iatu_unroll_enabled ? "enabled" : "disabled");
>>  
>> +	ret = of_property_read_u32(np, "num-lanes", &lanes);
>> +	if (ret)
>> +		lanes = 0;
> 
> You moved from host_init to root complex setup function, which in my opinion did
> not improve (in this scope).
> 
> I suggest that instead of making so much intermediary patches, which is nice to
> understand your development sequence, but hard to review. Wouldn't be better to
> condense some of the patches? We would have a cloear vision of the final product :)

I thought the other way. If squashing patches is easier to review, I'll do it.

Btw, thanks for reviewing.

Cheers
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joao Pinto Jan. 16, 2017, 10:23 a.m. UTC | #3
Hi,

Às 5:19 AM de 1/16/2017, Kishon Vijay Abraham I escreveu:
> Hi,
> 
> On Friday 13 January 2017 10:43 PM, Joao Pinto wrote:
>> Hi,
>>
>> Às 10:25 AM de 1/12/2017, Kishon Vijay Abraham I escreveu:
>>> *num-lanes* dt property is parsed in dw_pcie_host_init. However
>>> *num-lanes* property is applicable to both root complex mode and
>>> endpoint mode. As a first step, move the parsing of this property
>>> outside dw_pcie_host_init. This is in preparation for splitting
>>> pcie-designware.c to pcie-designware.c and pcie-designware-host.c
>>>
>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>> ---
>>>  drivers/pci/dwc/pcie-designware.c |   18 +++++++++++-------
>>>  drivers/pci/dwc/pcie-designware.h |    1 -
>>>  2 files changed, 11 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
>>> index 00a0fdc..89cdb6b 100644
>>> --- a/drivers/pci/dwc/pcie-designware.c
>>> +++ b/drivers/pci/dwc/pcie-designware.c
>>> @@ -551,10 +551,6 @@ int dw_pcie_host_init(struct pcie_port *pp)
>>>  		}
>>>  	}
>>>  
>>> -	ret = of_property_read_u32(np, "num-lanes", &pci->lanes);
>>> -	if (ret)
>>> -		pci->lanes = 0;
>>> -
>>>  	ret = of_property_read_u32(np, "num-viewport", &pci->num_viewport);
>>>  	if (ret)
>>>  		pci->num_viewport = 2;
>>> @@ -751,18 +747,26 @@ static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
>>>  
>>>  void dw_pcie_setup_rc(struct pcie_port *pp)
>>>  {
>>> +	int ret;
>>> +	u32 lanes;
>>>  	u32 val;
>>>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +	struct device *dev = pci->dev;
>>> +	struct device_node *np = dev->of_node;
>>>  
>>>  	/* get iATU unroll support */
>>>  	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
>>>  	dev_dbg(pci->dev, "iATU unroll: %s\n",
>>>  		pci->iatu_unroll_enabled ? "enabled" : "disabled");
>>>  
>>> +	ret = of_property_read_u32(np, "num-lanes", &lanes);
>>> +	if (ret)
>>> +		lanes = 0;
>>
>> You moved from host_init to root complex setup function, which in my opinion did
>> not improve (in this scope).
>>
>> I suggest that instead of making so much intermediary patches, which is nice to
>> understand your development sequence, but hard to review. Wouldn't be better to
>> condense some of the patches? We would have a cloear vision of the final product :)
> 
> I thought the other way. If squashing patches is easier to review, I'll do it.

I understand. To break it in small pieces is good to understand clearly what is
done and how was done, but I would break too much. That's a personal opinion of
course, lets see what others say :).

Thanks,
Joao

> 
> Btw, thanks for reviewing.
> 
> Cheers
> Kishon
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
index 00a0fdc..89cdb6b 100644
--- a/drivers/pci/dwc/pcie-designware.c
+++ b/drivers/pci/dwc/pcie-designware.c
@@ -551,10 +551,6 @@  int dw_pcie_host_init(struct pcie_port *pp)
 		}
 	}
 
-	ret = of_property_read_u32(np, "num-lanes", &pci->lanes);
-	if (ret)
-		pci->lanes = 0;
-
 	ret = of_property_read_u32(np, "num-viewport", &pci->num_viewport);
 	if (ret)
 		pci->num_viewport = 2;
@@ -751,18 +747,26 @@  static int dw_pcie_wr_conf(struct pci_bus *bus, u32 devfn,
 
 void dw_pcie_setup_rc(struct pcie_port *pp)
 {
+	int ret;
+	u32 lanes;
 	u32 val;
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct device *dev = pci->dev;
+	struct device_node *np = dev->of_node;
 
 	/* get iATU unroll support */
 	pci->iatu_unroll_enabled = dw_pcie_iatu_unroll_enabled(pci);
 	dev_dbg(pci->dev, "iATU unroll: %s\n",
 		pci->iatu_unroll_enabled ? "enabled" : "disabled");
 
+	ret = of_property_read_u32(np, "num-lanes", &lanes);
+	if (ret)
+		lanes = 0;
+
 	/* set the number of lanes */
 	val = dw_pcie_readl_dbi(pci, PCIE_PORT_LINK_CONTROL);
 	val &= ~PORT_LINK_MODE_MASK;
-	switch (pci->lanes) {
+	switch (lanes) {
 	case 1:
 		val |= PORT_LINK_MODE_1_LANES;
 		break;
@@ -776,7 +780,7 @@  void dw_pcie_setup_rc(struct pcie_port *pp)
 		val |= PORT_LINK_MODE_8_LANES;
 		break;
 	default:
-		dev_err(pci->dev, "num-lanes %u: invalid value\n", pci->lanes);
+		dev_err(pci->dev, "num-lanes %u: invalid value\n", lanes);
 		return;
 	}
 	dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, val);
@@ -784,7 +788,7 @@  void dw_pcie_setup_rc(struct pcie_port *pp)
 	/* set link width speed control register */
 	val = dw_pcie_readl_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL);
 	val &= ~PORT_LOGIC_LINK_WIDTH_MASK;
-	switch (pci->lanes) {
+	switch (lanes) {
 	case 1:
 		val |= PORT_LOGIC_LINK_WIDTH_1_LANES;
 		break;
diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
index d4b3d43..491fbe3 100644
--- a/drivers/pci/dwc/pcie-designware.h
+++ b/drivers/pci/dwc/pcie-designware.h
@@ -148,7 +148,6 @@  struct dw_pcie_ops {
 struct dw_pcie {
 	struct device		*dev;
 	void __iomem		*dbi_base;
-	u32			lanes;
 	u32			num_viewport;
 	u8			iatu_unroll_enabled;
 	struct pcie_port	pp;