Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote: > Signed-off-by: Jay Agarwal > > - Enable pcie root port 2 for cardhu > - Make private data structure for each SOC > - Add required tegra3 clocks and regulators > - Add tegra3 specific code in enable controller > - Modify clock tree to get clocks based on device > - Based on git://gitorious.org/thierryreding/linux.git Jay, I made quite a few comments on this series, and pointed out that it didn't actually work for me. Are you planning on fixes these issues and reposting? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git Jay, I made quite a few comments on this series, and pointed out that it didn't actually work for me. Are you planning on fixes these issues and reposting? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On Fri, Apr 12, 2013 at 09:34:13AM -0600, Stephen Warren wrote: > On 04/12/2013 08:58 AM, Jay Agarwal wrote: > >>> err = regulator_disable(pcie->pex_clk_supply); > >>> if (err < 0) > >>> - dev_err(pcie->dev, "failed to disable pex-clk regulator: > >> %d\n", > >>> + dev_warn(pcie->dev, "failed to disable pex-clk regulator: > >> %d\n", > >>> err); > >>> > >>> err = regulator_disable(pcie->vdd_supply); > >>> if (err < 0) > >>> - dev_err(pcie->dev, "failed to disable VDD regulator: %d\n", > >>> + dev_warn(pcie->dev, "failed to disable VDD regulator: > >> %d\n", > >>> err); > >> > >> Please explain why that change is correct. If the regulators only exist on > >> Tegra20, please represent that fact in the SoC data. Regulators must always > >> exist, so enable/disable should never fail due to missing regulators. > >> Actual > >> run-time failures seem like something that really is an error. > >> > > [>] These regulators are needed for both tegra20 & tegra30. Since we are > > not returning error here, so changed to dev_warn. > > If the regulators are required, then any failure to operate them should > be an error, hence dev_err() seems correct. > > As to why the code doesn't actually return an error? I'm not sure. > Perhaps that should be fixed with a separate patch, although I don't > recall exactly where in the code the above excerpt is; if it's in > remove(), then continuing on without returning an error would be > appropriate. That code is from tegra_pcie_power_off(), which is called only during error cleanup or from tegra_pcie_put_resources() which in turn is also only called in cleanup paths or during module/device removal. Disabling as many regulators as possible is still what we want in that case, so returning an error prematurely might leave more regulators turned on than necessary. Thierry pgpCWpe252nGz.pgp Description: PGP signature
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On Fri, Apr 12, 2013 at 09:34:13AM -0600, Stephen Warren wrote: On 04/12/2013 08:58 AM, Jay Agarwal wrote: err = regulator_disable(pcie-pex_clk_supply); if (err 0) - dev_err(pcie-dev, failed to disable pex-clk regulator: %d\n, + dev_warn(pcie-dev, failed to disable pex-clk regulator: %d\n, err); err = regulator_disable(pcie-vdd_supply); if (err 0) - dev_err(pcie-dev, failed to disable VDD regulator: %d\n, + dev_warn(pcie-dev, failed to disable VDD regulator: %d\n, err); Please explain why that change is correct. If the regulators only exist on Tegra20, please represent that fact in the SoC data. Regulators must always exist, so enable/disable should never fail due to missing regulators. Actual run-time failures seem like something that really is an error. [] These regulators are needed for both tegra20 tegra30. Since we are not returning error here, so changed to dev_warn. If the regulators are required, then any failure to operate them should be an error, hence dev_err() seems correct. As to why the code doesn't actually return an error? I'm not sure. Perhaps that should be fixed with a separate patch, although I don't recall exactly where in the code the above excerpt is; if it's in remove(), then continuing on without returning an error would be appropriate. That code is from tegra_pcie_power_off(), which is called only during error cleanup or from tegra_pcie_put_resources() which in turn is also only called in cleanup paths or during module/device removal. Disabling as many regulators as possible is still what we want in that case, so returning an error prematurely might leave more regulators turned on than necessary. Thierry pgpCWpe252nGz.pgp Description: PGP signature
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 11:06 AM, Jay Agarwal wrote: >> On 04/12/2013 10:43 AM, Jay Agarwal wrote: On 04/08/2013 09:41 AM, Jay Agarwal wrote: > Signed-off-by: Jay Agarwal > > - Enable pcie root port 2 for cardhu > - Make private data structure for each SOC > - Add required tegra3 clocks and regulators > - Add tegra3 specific code in enable controller > - Modify clock tree to get clocks based on device > - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these >> patches. >>> [>] Stephen, I have mentioned it in comment description as Based on >> git://gitorious.org/thierryreding/linux.git, is this not enough? >> >> Well, first off, there are many branches there, and secondly the branch that >> a series is based on doesn't necessarily imply much about what you expect >> people to do with it. >> > I am not clear, What should I mention then? The branch name. Thierry's branch has the following: remotes/gitorious_thierryreding_linux/drm/hdmi-for-3.9 remotes/gitorious_thierryreding_linux/drm/tegra-for-3.9 remotes/gitorious_thierryreding_linux/tegra/drm/for-3.8 remotes/gitorious_thierryreding_linux/tegra/drm/next remotes/gitorious_thierryreding_linux/tegra/next remotes/gitorious_thierryreding_linux/tegra/next-20130122 which of those was it based on? Also, you simply said it was based on that repo. If you intend Thierry to apply the patches to his repo/branch, rather than the usual maintainers of the files you're editing, who you also CC'd, you should specify that. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
> On 04/12/2013 10:43 AM, Jay Agarwal wrote: > >> On 04/08/2013 09:41 AM, Jay Agarwal wrote: > >>> Signed-off-by: Jay Agarwal > >>> > >>> - Enable pcie root port 2 for cardhu > >>> - Make private data structure for each SOC > >>> - Add required tegra3 clocks and regulators > >>> - Add tegra3 specific code in enable controller > >>> - Modify clock tree to get clocks based on device > >>> - Based on git://gitorious.org/thierryreding/linux.git > >> > >> A couple more points on this patch: > >> > >> * You didn't mention that this series is based on Thierry's > >> work-in-progress tree, and not something immediately destined for > >> upstream. As such, only Thierry is expected to actually apply any of these > patches. > >> > > [>] Stephen, I have mentioned it in comment description as Based on > git://gitorious.org/thierryreding/linux.git, is this not enough? > > Well, first off, there are many branches there, and secondly the branch that > a series is based on doesn't necessarily imply much about what you expect > people to do with it. > I am not clear, What should I mention then? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 10:43 AM, Jay Agarwal wrote: >> On 04/08/2013 09:41 AM, Jay Agarwal wrote: >>> Signed-off-by: Jay Agarwal >>> >>> - Enable pcie root port 2 for cardhu >>> - Make private data structure for each SOC >>> - Add required tegra3 clocks and regulators >>> - Add tegra3 specific code in enable controller >>> - Modify clock tree to get clocks based on device >>> - Based on git://gitorious.org/thierryreding/linux.git >> >> A couple more points on this patch: >> >> * You didn't mention that this series is based on Thierry's work-in-progress >> tree, and not something immediately destined for upstream. As such, only >> Thierry is expected to actually apply any of these patches. >> > [>] Stephen, I have mentioned it in comment description as Based on > git://gitorious.org/thierryreding/linux.git, is this not enough? Well, first off, there are many branches there, and secondly the branch that a series is based on doesn't necessarily imply much about what you expect people to do with it. > >> * Your changes to the Tegra PCIe driver require that device tree include >> extra clocks and regulators. You need to update >> Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe >> these new requirements. > > [>] Should I create tegra30-pcie.txt? or made changes in tegra20-pcie.txt > itself? (What is "[>]"?) It'd probably be simplest to edit tegra20-pcie.txt, and just mention the extra requirements for Tegra30. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
> On 04/08/2013 09:41 AM, Jay Agarwal wrote: > > Signed-off-by: Jay Agarwal > > > > - Enable pcie root port 2 for cardhu > > - Make private data structure for each SOC > > - Add required tegra3 clocks and regulators > > - Add tegra3 specific code in enable controller > > - Modify clock tree to get clocks based on device > > - Based on git://gitorious.org/thierryreding/linux.git > > A couple more points on this patch: > > * You didn't mention that this series is based on Thierry's work-in-progress > tree, and not something immediately destined for upstream. As such, only > Thierry is expected to actually apply any of these patches. > [>] Stephen, I have mentioned it in comment description as Based on git://gitorious.org/thierryreding/linux.git, is this not enough? > * Your changes to the Tegra PCIe driver require that device tree include > extra clocks and regulators. You need to update > Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe > these new requirements. [>] Should I create tegra30-pcie.txt? or made changes in tegra20-pcie.txt itself? -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 08:58 AM, Jay Agarwal wrote: >>> err = regulator_disable(pcie->pex_clk_supply); >>> if (err < 0) >>> - dev_err(pcie->dev, "failed to disable pex-clk regulator: >> %d\n", >>> + dev_warn(pcie->dev, "failed to disable pex-clk regulator: >> %d\n", >>> err); >>> >>> err = regulator_disable(pcie->vdd_supply); >>> if (err < 0) >>> - dev_err(pcie->dev, "failed to disable VDD regulator: %d\n", >>> + dev_warn(pcie->dev, "failed to disable VDD regulator: >> %d\n", >>> err); >> >> Please explain why that change is correct. If the regulators only exist on >> Tegra20, please represent that fact in the SoC data. Regulators must always >> exist, so enable/disable should never fail due to missing regulators. Actual >> run-time failures seem like something that really is an error. >> > [>] These regulators are needed for both tegra20 & tegra30. Since we are not > returning error here, so changed to dev_warn. If the regulators are required, then any failure to operate them should be an error, hence dev_err() seems correct. As to why the code doesn't actually return an error? I'm not sure. Perhaps that should be fixed with a separate patch, although I don't recall exactly where in the code the above excerpt is; if it's in remove(), then continuing on without returning an error would be appropriate. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
> > err = regulator_disable(pcie->pex_clk_supply); > > if (err < 0) > > - dev_err(pcie->dev, "failed to disable pex-clk regulator: > %d\n", > > + dev_warn(pcie->dev, "failed to disable pex-clk regulator: > %d\n", > > err); > > > > err = regulator_disable(pcie->vdd_supply); > > if (err < 0) > > - dev_err(pcie->dev, "failed to disable VDD regulator: %d\n", > > + dev_warn(pcie->dev, "failed to disable VDD regulator: > %d\n", > > err); > > Please explain why that change is correct. If the regulators only exist on > Tegra20, please represent that fact in the SoC data. Regulators must always > exist, so enable/disable should never fail due to missing regulators. Actual > run-time failures seem like something that really is an error. > [>] These regulators are needed for both tegra20 & tegra30. Since we are not returning error here, so changed to dev_warn. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
err = regulator_disable(pcie-pex_clk_supply); if (err 0) - dev_err(pcie-dev, failed to disable pex-clk regulator: %d\n, + dev_warn(pcie-dev, failed to disable pex-clk regulator: %d\n, err); err = regulator_disable(pcie-vdd_supply); if (err 0) - dev_err(pcie-dev, failed to disable VDD regulator: %d\n, + dev_warn(pcie-dev, failed to disable VDD regulator: %d\n, err); Please explain why that change is correct. If the regulators only exist on Tegra20, please represent that fact in the SoC data. Regulators must always exist, so enable/disable should never fail due to missing regulators. Actual run-time failures seem like something that really is an error. [] These regulators are needed for both tegra20 tegra30. Since we are not returning error here, so changed to dev_warn. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 08:58 AM, Jay Agarwal wrote: err = regulator_disable(pcie-pex_clk_supply); if (err 0) - dev_err(pcie-dev, failed to disable pex-clk regulator: %d\n, + dev_warn(pcie-dev, failed to disable pex-clk regulator: %d\n, err); err = regulator_disable(pcie-vdd_supply); if (err 0) - dev_err(pcie-dev, failed to disable VDD regulator: %d\n, + dev_warn(pcie-dev, failed to disable VDD regulator: %d\n, err); Please explain why that change is correct. If the regulators only exist on Tegra20, please represent that fact in the SoC data. Regulators must always exist, so enable/disable should never fail due to missing regulators. Actual run-time failures seem like something that really is an error. [] These regulators are needed for both tegra20 tegra30. Since we are not returning error here, so changed to dev_warn. If the regulators are required, then any failure to operate them should be an error, hence dev_err() seems correct. As to why the code doesn't actually return an error? I'm not sure. Perhaps that should be fixed with a separate patch, although I don't recall exactly where in the code the above excerpt is; if it's in remove(), then continuing on without returning an error would be appropriate. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. [] Stephen, I have mentioned it in comment description as Based on git://gitorious.org/thierryreding/linux.git, is this not enough? * Your changes to the Tegra PCIe driver require that device tree include extra clocks and regulators. You need to update Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe these new requirements. [] Should I create tegra30-pcie.txt? or made changes in tegra20-pcie.txt itself? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 10:43 AM, Jay Agarwal wrote: On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. [] Stephen, I have mentioned it in comment description as Based on git://gitorious.org/thierryreding/linux.git, is this not enough? Well, first off, there are many branches there, and secondly the branch that a series is based on doesn't necessarily imply much about what you expect people to do with it. * Your changes to the Tegra PCIe driver require that device tree include extra clocks and regulators. You need to update Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe these new requirements. [] Should I create tegra30-pcie.txt? or made changes in tegra20-pcie.txt itself? (What is []?) It'd probably be simplest to edit tegra20-pcie.txt, and just mention the extra requirements for Tegra30. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 10:43 AM, Jay Agarwal wrote: On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. [] Stephen, I have mentioned it in comment description as Based on git://gitorious.org/thierryreding/linux.git, is this not enough? Well, first off, there are many branches there, and secondly the branch that a series is based on doesn't necessarily imply much about what you expect people to do with it. I am not clear, What should I mention then? -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/12/2013 11:06 AM, Jay Agarwal wrote: On 04/12/2013 10:43 AM, Jay Agarwal wrote: On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. [] Stephen, I have mentioned it in comment description as Based on git://gitorious.org/thierryreding/linux.git, is this not enough? Well, first off, there are many branches there, and secondly the branch that a series is based on doesn't necessarily imply much about what you expect people to do with it. I am not clear, What should I mention then? The branch name. Thierry's branch has the following: remotes/gitorious_thierryreding_linux/drm/hdmi-for-3.9 remotes/gitorious_thierryreding_linux/drm/tegra-for-3.9 remotes/gitorious_thierryreding_linux/tegra/drm/for-3.8 remotes/gitorious_thierryreding_linux/tegra/drm/next remotes/gitorious_thierryreding_linux/tegra/next remotes/gitorious_thierryreding_linux/tegra/next-20130122 which of those was it based on? Also, you simply said it was based on that repo. If you intend Thierry to apply the patches to his repo/branch, rather than the usual maintainers of the files you're editing, who you also CC'd, you should specify that. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote:
> Signed-off-by: Jay Agarwal
>
> - Enable pcie root port 2 for cardhu
> - Make private data structure for each SOC
> - Add required tegra3 clocks and regulators
> - Add tegra3 specific code in enable controller
> - Modify clock tree to get clocks based on device
> - Based on git://gitorious.org/thierryreding/linux.git
Did you test these patches? They don't work for me on my Cardhu A04.
First off, I had to change the num-lanes properties to match Cardhu's
actual configuration:
> diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> index d64d12c..6426226 100644
> --- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
> +++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
> @@ -143,8 +143,17 @@
> vdd-supply = <_reg>;
> avdd-supply = <_reg>;
>
> + pci@1,0 {
> + nvidia,num-lanes = <4>;
> + };
> +
> + pci@2,0 {
> + nvidia,num-lanes = <1>;
> + };
> +
> pci@3,0 {
> status = "okay";
> + nvidia,num-lanes = <1>;
> };
> };
>
However, even after doing that, the driver doesn't detect anything
attached to port@3,0, even though I have the board plugged into the
docking station, and hence the PCI Ethernet should be detected:
> [3.103860] tegra-pcie 3000.pcie-controller: 4x1, 1x2 configuration
> [3.113755] tegra-pcie 3000.pcie-controller: probing port 2, using 1 lanes
> [3.324364] tegra-pcie 3000.pcie-controller: link 2 down, retrying
> [3.534249] tegra-pcie 3000.pcie-controller: link 2 down, retrying
> [3.744160] tegra-pcie 3000.pcie-controller: link 2 down, retrying
> [3.751359] tegra-pcie 3000.pcie-controller: link 2 down, ignoring
(I see the same messages even without fixing the lane configuration,
exception for the first configuration message obviously prints something
different).
Are you testing with U-Boot, or using our binary bootloader? Upstream
code must be tested with U-Boot, to make sure it doesn't rely on any HW
programming performed by the bootloader.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote:
Signed-off-by: Jay Agarwal jagar...@nvidia.com
- Enable pcie root port 2 for cardhu
- Make private data structure for each SOC
- Add required tegra3 clocks and regulators
- Add tegra3 specific code in enable controller
- Modify clock tree to get clocks based on device
- Based on git://gitorious.org/thierryreding/linux.git
Did you test these patches? They don't work for me on my Cardhu A04.
First off, I had to change the num-lanes properties to match Cardhu's
actual configuration:
diff --git a/arch/arm/boot/dts/tegra30-cardhu.dtsi
b/arch/arm/boot/dts/tegra30-cardhu.dtsi
index d64d12c..6426226 100644
--- a/arch/arm/boot/dts/tegra30-cardhu.dtsi
+++ b/arch/arm/boot/dts/tegra30-cardhu.dtsi
@@ -143,8 +143,17 @@
vdd-supply = ldo1_reg;
avdd-supply = ldo2_reg;
+ pci@1,0 {
+ nvidia,num-lanes = 4;
+ };
+
+ pci@2,0 {
+ nvidia,num-lanes = 1;
+ };
+
pci@3,0 {
status = okay;
+ nvidia,num-lanes = 1;
};
};
However, even after doing that, the driver doesn't detect anything
attached to port@3,0, even though I have the board plugged into the
docking station, and hence the PCI Ethernet should be detected:
[3.103860] tegra-pcie 3000.pcie-controller: 4x1, 1x2 configuration
[3.113755] tegra-pcie 3000.pcie-controller: probing port 2, using 1 lanes
[3.324364] tegra-pcie 3000.pcie-controller: link 2 down, retrying
[3.534249] tegra-pcie 3000.pcie-controller: link 2 down, retrying
[3.744160] tegra-pcie 3000.pcie-controller: link 2 down, retrying
[3.751359] tegra-pcie 3000.pcie-controller: link 2 down, ignoring
(I see the same messages even without fixing the lane configuration,
exception for the first configuration message obviously prints something
different).
Are you testing with U-Boot, or using our binary bootloader? Upstream
code must be tested with U-Boot, to make sure it doesn't rely on any HW
programming performed by the bootloader.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote: > Signed-off-by: Jay Agarwal > > - Enable pcie root port 2 for cardhu > - Make private data structure for each SOC > - Add required tegra3 clocks and regulators > - Add tegra3 specific code in enable controller > - Modify clock tree to get clocks based on device > - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. * Your changes to the Tegra PCIe driver require that device tree include extra clocks and regulators. You need to update Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe these new requirements. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote:
> Signed-off-by: Jay Agarwal
>
> - Enable pcie root port 2 for cardhu
> - Make private data structure for each SOC
> - Add required tegra3 clocks and regulators
> - Add tegra3 specific code in enable controller
> - Modify clock tree to get clocks based on device
> - Based on git://gitorious.org/thierryreding/linux.git
There are quite a few capitalization errors above. The correct versions
are: PCIe, Cardhu, SoC, Tegra.
Upstream uses engineering names not marketing names, so Tegra30 not Tegra3.
> drivers/clk/tegra/clk-tegra30.c | 12 ++--
> drivers/pci/host/pci-tegra.c| 146
> ++-
This patch touches two unrelated drivers. There is no dependency between
the changes, since PCIe doesn't work yet on Tegra30, so there's no need
for those unrelated changes to be part of the same patch. Please split
them into separate patches. The clk patch can likely be applied very
soon, without waiting for any of the other PCIe patches.
> - Modify clock tree to get clocks based on device
That description doesn't seem to describe the change made to
clk-tegra30.c. Can you please include more details re: what the patch is
doing and why.
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> +/* used to differente tegra chips code */
differentiate
> +struct tegra_pcie_soc_data {
...
> + bool need_avdd_supply;
> + bool need_cml0_clk;
s/need/has/ would be better.
> @@ -749,6 +778,11 @@ static int tegra_pcie_enable_controller(struct
> tegra_pcie *pcie)
> struct tegra_pcie_port *port;
> unsigned int timeout;
> unsigned long value;
> + struct tegra_pcie_soc_data *soc = pcie->soc_data;
> +
> + /* power down to PCIe slot clock bias pad */
> + if (soc->pex_bias_ctrl)
> + afi_writel(pcie, 0, AFI_PEXBIAS_CTRL_0);
Power down when /enabling/ a controller?
> err = regulator_disable(pcie->pex_clk_supply);
> if (err < 0)
> - dev_err(pcie->dev, "failed to disable pex-clk regulator: %d\n",
> + dev_warn(pcie->dev, "failed to disable pex-clk regulator: %d\n",
> err);
>
> err = regulator_disable(pcie->vdd_supply);
> if (err < 0)
> - dev_err(pcie->dev, "failed to disable VDD regulator: %d\n",
> + dev_warn(pcie->dev, "failed to disable VDD regulator: %d\n",
> err);
Please explain why that change is correct. If the regulators only exist
on Tegra20, please represent that fact in the SoC data. Regulators must
always exist, so enable/disable should never fail due to missing
regulators. Actual run-time failures seem like something that really is
an error.
> @@ -1489,6 +1597,11 @@ static int tegra_pcie_probe(struct platform_device
> *pdev)
> INIT_LIST_HEAD(>busses);
> INIT_LIST_HEAD(>ports);
> pcie->dev = >dev;
> + match = of_match_device(tegra_pcie_of_match, >dev);
> + if (match)
> + pcie->soc_data = (struct tegra_pcie_soc_data *)match->data;
> + else
> + return -ENODEV;
that would be more idiomatic as:
if (!match)
return -ENODEV;
pcie->soc_data = ...;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote:
Signed-off-by: Jay Agarwal jagar...@nvidia.com
- Enable pcie root port 2 for cardhu
- Make private data structure for each SOC
- Add required tegra3 clocks and regulators
- Add tegra3 specific code in enable controller
- Modify clock tree to get clocks based on device
- Based on git://gitorious.org/thierryreding/linux.git
There are quite a few capitalization errors above. The correct versions
are: PCIe, Cardhu, SoC, Tegra.
Upstream uses engineering names not marketing names, so Tegra30 not Tegra3.
drivers/clk/tegra/clk-tegra30.c | 12 ++--
drivers/pci/host/pci-tegra.c| 146
++-
This patch touches two unrelated drivers. There is no dependency between
the changes, since PCIe doesn't work yet on Tegra30, so there's no need
for those unrelated changes to be part of the same patch. Please split
them into separate patches. The clk patch can likely be applied very
soon, without waiting for any of the other PCIe patches.
- Modify clock tree to get clocks based on device
That description doesn't seem to describe the change made to
clk-tegra30.c. Can you please include more details re: what the patch is
doing and why.
diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
+/* used to differente tegra chips code */
differentiate
+struct tegra_pcie_soc_data {
...
+ bool need_avdd_supply;
+ bool need_cml0_clk;
s/need/has/ would be better.
@@ -749,6 +778,11 @@ static int tegra_pcie_enable_controller(struct
tegra_pcie *pcie)
struct tegra_pcie_port *port;
unsigned int timeout;
unsigned long value;
+ struct tegra_pcie_soc_data *soc = pcie-soc_data;
+
+ /* power down to PCIe slot clock bias pad */
+ if (soc-pex_bias_ctrl)
+ afi_writel(pcie, 0, AFI_PEXBIAS_CTRL_0);
Power down when /enabling/ a controller?
err = regulator_disable(pcie-pex_clk_supply);
if (err 0)
- dev_err(pcie-dev, failed to disable pex-clk regulator: %d\n,
+ dev_warn(pcie-dev, failed to disable pex-clk regulator: %d\n,
err);
err = regulator_disable(pcie-vdd_supply);
if (err 0)
- dev_err(pcie-dev, failed to disable VDD regulator: %d\n,
+ dev_warn(pcie-dev, failed to disable VDD regulator: %d\n,
err);
Please explain why that change is correct. If the regulators only exist
on Tegra20, please represent that fact in the SoC data. Regulators must
always exist, so enable/disable should never fail due to missing
regulators. Actual run-time failures seem like something that really is
an error.
@@ -1489,6 +1597,11 @@ static int tegra_pcie_probe(struct platform_device
*pdev)
INIT_LIST_HEAD(pcie-busses);
INIT_LIST_HEAD(pcie-ports);
pcie-dev = pdev-dev;
+ match = of_match_device(tegra_pcie_of_match, pdev-dev);
+ if (match)
+ pcie-soc_data = (struct tegra_pcie_soc_data *)match-data;
+ else
+ return -ENODEV;
that would be more idiomatic as:
if (!match)
return -ENODEV;
pcie-soc_data = ...;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/3] ARM: tegra: pcie: Add tegra3 support
On 04/08/2013 09:41 AM, Jay Agarwal wrote: Signed-off-by: Jay Agarwal jagar...@nvidia.com - Enable pcie root port 2 for cardhu - Make private data structure for each SOC - Add required tegra3 clocks and regulators - Add tegra3 specific code in enable controller - Modify clock tree to get clocks based on device - Based on git://gitorious.org/thierryreding/linux.git A couple more points on this patch: * You didn't mention that this series is based on Thierry's work-in-progress tree, and not something immediately destined for upstream. As such, only Thierry is expected to actually apply any of these patches. * Your changes to the Tegra PCIe driver require that device tree include extra clocks and regulators. You need to update Documentation/devicetree/bindings/pci/nvidia,tegra20-pcie.txt to describe these new requirements. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
