[U-Boot] [PATCH 1/5] pci: tegra: port to standard clock/reset/pwr domain APIs

Simon Glass sjg at chromium.org
Fri Aug 5 03:36:57 CEST 2016


Hi Stephen,

On 4 August 2016 at 12:52, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 08/03/2016 07:16 PM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 1 August 2016 at 09:22, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>
>>> On 07/31/2016 07:02 PM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 27 July 2016 at 15:48, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren at nvidia.com>
>>>>>
>>>>> Tegra186 supports the new standard clock, reset, and power domain APIs.
>>>>> Older Tegra SoCs still use custom APIs. Enhance the Tegra PCIe driver
>>>>> so
>>>>> that it can operate with either set of APIs.
>>>>>
>>>>> On Tegra186, the BPMP handles all aspects of PCIe PHY (UPHY)
>>>>> programming.
>>>>> Consequently, this logic is disabled too.
>>>>>
>>>>> Signed-off-by: Stephen Warren <swarren at nvidia.com>
>>>>> ---
>>>>> This whole series builds on the other Tegra186 series that I just sent.
>>>>>
>>>>>  drivers/pci/Kconfig     |   1 +
>>>>>  drivers/pci/pci_tegra.c | 154
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++--
>>>>>  2 files changed, 150 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
>>>>> index 26aa2b0930a0..669e37bb5dc5 100644
>>>>> --- a/drivers/pci/Kconfig
>>>>> +++ b/drivers/pci/Kconfig
>>>>> @@ -31,6 +31,7 @@ config PCI_SANDBOX
>>>>>  config PCI_TEGRA
>>>>>         bool "Tegra PCI support"
>>>>>         depends on TEGRA
>>>>> +       depends on (TEGRA186 && POWER_DOMAIN) || (!TEGRA186)
>>>>>         help
>>>>>           Enable support for the PCIe controller found on some
>>>>> generations of
>>>>>           Tegra. Tegra20 has 2 root ports with a total of 4 lanes,
>>>>> Tegra30 has
>>>>> diff --git a/drivers/pci/pci_tegra.c b/drivers/pci/pci_tegra.c
>>>>> index 352cdef56ab4..a6785ad0bbee 100644
>>>>> --- a/drivers/pci/pci_tegra.c
>>>>> +++ b/drivers/pci/pci_tegra.c
>>>>> @@ -13,22 +13,26 @@
>>>>>  #define pr_fmt(fmt) "tegra-pcie: " fmt
>>>>>
>>>>>  #include <common.h>
>>>>> +#include <clk.h>
>>>>>  #include <dm.h>
>>>>>  #include <errno.h>
>>>>>  #include <fdtdec.h>
>>>>>  #include <malloc.h>
>>>>>  #include <pci.h>
>>>>> +#include <power-domain.h>
>>>>> +#include <reset.h>
>>>>>
>>>>>  #include <asm/io.h>
>>>>>  #include <asm/gpio.h>
>>>>>
>>>>> +#include <linux/list.h>
>>>>> +
>>>>> +#ifndef CONFIG_TEGRA186
>>>>>  #include <asm/arch/clock.h>
>>>>>  #include <asm/arch/powergate.h>
>>>>>  #include <asm/arch-tegra/xusb-padctl.h>
>>>>> -
>>>>> -#include <linux/list.h>
>>>>> -
>>>>>  #include <dt-bindings/pinctrl/pinctrl-tegra-xusb.h>
>>>>> +#endif
>>>>>
>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>
>>>>> @@ -103,6 +107,9 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>  #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_222       (0x1 << 20)
>>>>>  #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_X4_X1     (0x1 << 20)
>>>>>  #define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_411       (0x2 << 20)
>>>>> +#define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_T186_401  (0x0 << 20)
>>>>> +#define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_T186_211  (0x1 << 20)
>>>>> +#define  AFI_PCIE_CONFIG_SM2TMS0_XBAR_CONFIG_T186_111  (0x2 << 20)
>>>>>
>>>>>  #define AFI_FUSE                       0x104
>>>>>  #define  AFI_FUSE_PCIE_T0_GEN2_DIS     (1 << 2)
>>>>> @@ -110,6 +117,7 @@ DECLARE_GLOBAL_DATA_PTR;
>>>>>  #define AFI_PEX0_CTRL                  0x110
>>>>>  #define AFI_PEX1_CTRL                  0x118
>>>>>  #define AFI_PEX2_CTRL                  0x128
>>>>> +#define AFI_PEX2_CTRL_T186             0x19c
>>>>>  #define  AFI_PEX_CTRL_RST              (1 << 0)
>>>>>  #define  AFI_PEX_CTRL_CLKREQ_EN                (1 << 1)
>>>>>  #define  AFI_PEX_CTRL_REFCLK_EN                (1 << 3)
>>>>> @@ -173,6 +181,7 @@ enum tegra_pci_id {
>>>>>         TEGRA30_PCIE,
>>>>>         TEGRA124_PCIE,
>>>>>         TEGRA210_PCIE,
>>>>> +       TEGRA186_PCIE,
>>>>>  };
>>>>>
>>>>>  struct tegra_pcie_port {
>>>>> @@ -189,6 +198,7 @@ struct tegra_pcie_soc {
>>>>>         unsigned int num_ports;
>>>>>         unsigned long pads_pll_ctl;
>>>>>         unsigned long tx_ref_sel;
>>>>> +       unsigned long afi_pex2_ctrl;
>>>>>         u32 pads_refclk_cfg0;
>>>>>         u32 pads_refclk_cfg1;
>>>>>         bool has_pex_clkreq_en;
>>>>> @@ -209,7 +219,17 @@ struct tegra_pcie {
>>>>>         unsigned long xbar;
>>>>>
>>>>>         const struct tegra_pcie_soc *soc;
>>>>> +
>>>>> +#ifdef CONFIG_TEGRA186
>>>>> +       struct clk clk_afi;
>>>>> +       struct clk clk_pex;
>>>>> +       struct reset_ctl reset_afi;
>>>>> +       struct reset_ctl reset_pex;
>>>>> +       struct reset_ctl reset_pcie_x;
>>>>> +       struct power_domain pwrdom;
>>>>> +#else
>>>>
>>>>
>>>>
>>>> Eek. This is a compile-time driver configuration. Can you use the
>>>> device tree compatible string to detect which to use?
>>>
>>>
>>>
>>> No. The legacy APIs are simply not available (not compiled in) on the
>>> newer
>>> SoCs, so there must be a compile-time condition.
>>>
>>> It is theoretically possible to convert all the old SoCs to the new APIs
>>> so
>>> that this conditional goes away in the future, but we don't have that
>>> work
>>> scheduled at present.
>>
>>
>> What did you do with Linux?
>
>
> We lucked out and life was a bit simpler:
>
> When we started adding Tegra support, there was already a standard clock API
> header, albeit with an entirely separate implementation for each SoC. When
> the common clock framework/core appeared, it maintained this same API so
> drivers weren't affected, except for a change in a #include filename.
>
> As far as converting the clock driver from a standalone/custom
> implementation to being based on the common clock framework/core, life was
> much easier. At that time, we only supported 2 Tegra SoC generations and
> many fewer boards and drivers, and so significantly less testing was
> required for the converted implementation. Many SoC generations, board, and
> drivers were only written once the new clock implementation was in place, so
> never needed conversion.
>
> Tegra started out with a completely custom reset API and implementation.
> When Tegra converted from its custom standalone clock implementation to the
> common clock framework/core, that same API was re-implemented using the new
> clock code (as you know, clocks and resets on Tegra are very closely tied),
> and so there was no effect on drivers.
>
> Later, we converted to the new reset API/framework/core. At this point, we
> did support 4 SoC generations and many more boards. However, IIRC during
> this time, there was a huge focus on converting to standard APIs, and a lull
> in new SoC and board support, so there was time to spend on large-scale
> conversions; they didn't take time away from other work required to support
> new SoCs/boards. This conversion implemented the new standard reset for all
> SoCs first, converted all drivers to use the new API roughly at once, and
> then removed the old API.
>
> So we got away without ifdefs.
>
> In U-Boot, we now support many more SoCs, boards, and perhaps even drivers,
> so a "convert everything at once" approach is quite a lot more work than it
> was in the kernel, especially on the clock/reset driver implementation side.
> Equally, to be honest, my primary focus is currently on adding Tegra186
> support at the moment. Given we needed a completely new clock/reset/...
> implementation anyway due to the existence of the BPMP, it was a no-brainer
> to create and/or use the new standard clock/reset/... APIs for Tegra186,
> rather than basing the new implementation on the old custom APIs. However,
> converting all the other SoCs/boards first is just going to delay any
> forward progress on Tegra186 and lead to a mega series of code conversions
> that I'd rather avoid doing all at once. Adding these ifdefs, allows
> incremental forward progress to be made pretty easily. They should be
> removed when older SoC generations are converted to the new APIs, so
> everything is unified again. Granted, I have no definite timescale for that
> at present.

OK, well I'd like to see some TODOs in the code around these #ifdefs.
They are definitely not what we want, and should be marked as interim.
Please see if you can find someone to convert things over. I can
perhaps help too.

Regards,
Simon


More information about the U-Boot mailing list