On 13-Jun-19 8:54 PM, Lorenzo Pieralisi wrote:
> On Tue, Jun 04, 2019 at 03:22:33PM +0200, Thierry Reding wrote:
>
> [...]
>
>>> + } else {
>>> + value = afi_readl(port->pcie, ctrl);
>>> + value &= ~AFI_PEX_CTRL_RST;
>>> + afi_writel(port->pcie, value, ctrl);
>>> + }
>>>
>>> usleep_range(1000, 2000);
>>>
>>> - value = afi_readl(port->pcie, ctrl);
>>> - value |= AFI_PEX_CTRL_RST;
>>> - afi_writel(port->pcie, value, ctrl);
>>> + if (port->reset_gpiod) {
>>> + gpiod_set_value(port->reset_gpiod, 1);
>> After this the port should be functional, right? I think it'd be better
>> to reverse the logic here and move the polarity of the GPIO into device
>> tree. gpiod_set_value() takes care of inverting the level internally if
>> the GPIO is marked as low-active in DT.
>>
>> The end result is obviously the same, but it makes the usage much
>> clearer. If somebody want to write a DT for their board, they will look
>> at the schematics and see a low-active reset line and may be tempted to
>> describe it as such in DT, but with your current code that would be
>> exactly the wrong way around.
> I agree with Thierry here, you should change the logic.
>
> Question: what's the advantage of adding GPIO reset support if that's
> architected already in port registers ? I am pretty sure there is a
> reason behind it (and forgive me the dumb question) and I would like to
> have it written in the commit log.
>
> Thanks,
> Lorenzo
Each PCIe controller has a dedicated SFIO pin to support PERST# signal. Port register
can control only this particular SFIO pin. However, in one of the Nvidia platform,
instead of using PCIe SFIO pin, different gpio is routed PCIe slot. This happened
because of a confusion in IO ball naming convention. To support this particular
platform, driver has provide gpio support. I will update the commit log in V5.
Manikanta
>
>>> + } else {
>>> + value = afi_readl(port->pcie, ctrl);
>>> + value |= AFI_PEX_CTRL_RST;
>>> + afi_writel(port->pcie, value, ctrl);
>>> + }
>>> }
>>>
>>> static void tegra_pcie_enable_rp_features(struct tegra_pcie_port *port)
>>> @@ -2238,6 +2249,7 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>> struct tegra_pcie_port *rp;
>>> unsigned int index;
>>> u32 value;
>>> + char *label;
>>>
>>> err = of_pci_get_devfn(port);
>>> if (err < 0) {
>>> @@ -2296,6 +2308,23 @@ static int tegra_pcie_parse_dt(struct tegra_pcie *pcie)
>>> if (IS_ERR(rp->base))
>>> return PTR_ERR(rp->base);
>>>
>>> + label = kasprintf(GFP_KERNEL, "pex-reset-%u", index);
>> devm_kasprintf()?
>>
>> Thierry
>>
>>> + if (!label) {
>>> + dev_err(dev, "failed to create reset GPIO label\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + rp->reset_gpiod = devm_gpiod_get_from_of_node(dev, port,
>>> + "reset-gpios", 0,
>>> + GPIOD_OUT_LOW,
>>> + label);
>>> + kfree(label);
>>> + if (IS_ERR(rp->reset_gpiod)) {
>>> + err = PTR_ERR(rp->reset_gpiod);
>>> + dev_err(dev, "failed to get reset GPIO: %d\n", err);
>>> + return err;
>>> + }
>>> +
>>> list_add_tail(&rp->list, &pcie->ports);
>>> }
>>>
>>> --
>>> 2.17.1
>>>
>
