* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-17 8:32 [PATCHv2 0/3] amba: tegra-ahb: fix base address and register offsets for future chip support Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 1/3] amba: tegra-ahb: fix register offsets in the macros Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 2/3] amba: tegra-ahb: use correct base address for future chip support Paul Walmsley
@ 2015-03-17 8:32 ` Paul Walmsley
2015-03-17 10:38 ` Russell King - ARM Linux
2 siblings, 1 reply; 13+ messages in thread
From: Paul Walmsley @ 2015-03-17 8:32 UTC (permalink / raw)
To: linux-arm-kernel
For Tegra132 and later chips, we can now use the correct hardware base
address for the Tegra AHB IP block in the DT data. Update the DT binding
documentation to reflect this change.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Hiroshi DOYU <hdoyu@nvidia.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: devicetree at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
---
.../bindings/arm/tegra/nvidia,tegra20-ahb.txt | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
index 067c979..7692b4c 100644
--- a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra20-ahb.txt
@@ -2,10 +2,15 @@ NVIDIA Tegra AHB
Required properties:
- compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
- Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
- '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
- tegra132, or tegra210.
-- reg : Should contain 1 register ranges(address and length)
+ Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and Tegra124, must
+ contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
+ or tegra124. For Tegra132, the compatible string must contain
+ "nvidia,tegra132-ahb".
+
+- reg : Should contain 1 register ranges(address and length). On Tegra20,
+ Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
+ address of the IP block must end in 0x04. On DT files for later chips, the
+ actual hardware base address of the IP block should be used.
Example:
ahb: ahb at 6000c004 {
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv2 0/3] amba: tegra-ahb: fix base address and register offsets for future chip support
@ 2015-03-17 8:32 Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 1/3] amba: tegra-ahb: fix register offsets in the macros Paul Walmsley
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-17 8:32 UTC (permalink / raw)
To: linux-arm-kernel
(This second transmission fixes a few errors in the original mails:
notably, the E-mail address of the linux-arm-kernel mailing list was
incorrect, and the patches are based on next-20150316 rather than
next-20150311. The code itself is unchanged.)
>From a hardware SoC integration point of view, the offsets of the
Tegra AHB registers that are currently defined in tegra-ahb.c macros
are all off by four bytes. Similarly, the starting address of this IP
block in our existing DT files is also off by four bytes. This series
fixes the driver such that the macro offsets are correct, and that the
driver is backwards-compatible with previous chip DT data, but that future
chip DT data can use the correct base. See also
http://www.spinics.net/lists/arm-kernel/msg394171.html
This series has been boot-tested on Tegra20 Trimslice, Tegra30
Beaver, Tegra114 Dalmore, Tegra124 Jetson TK1, Tegra132 Norrin64
FFD (with a few additional out-of-tree patches, since T132
support is not yet upstream), and QEMU Versatile Express 64.
Basic build and boot test logs for T30, T114, T124, and QEMU
Versatile Express 64 are here:
http://nvt.pwsan.com/pub/pwalmsley-tester/testlogs/test_20150317011136_159e7763d517804c61a673736660a5a35f2ea5f8/20150317011136/
(The multi_v7_defconfig test failure is unrelated to this series.)
This series is based on next-20150316 and is intended for v4.1.
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 2/3] amba: tegra-ahb: use correct base address for future chip support
2015-03-17 8:32 [PATCHv2 0/3] amba: tegra-ahb: fix base address and register offsets for future chip support Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 1/3] amba: tegra-ahb: fix register offsets in the macros Paul Walmsley
@ 2015-03-17 8:32 ` Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change Paul Walmsley
2 siblings, 0 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-17 8:32 UTC (permalink / raw)
To: linux-arm-kernel
>From a hardware SoC integration point of view, the starting address of
this IP block in our existing DT files is off by 4 bytes from the
actual base address. Since we attempt to make old DT files
forward-compatible with newer kernels, we cannot fix the IP block base
address in old DT data. However, we can fix this for DT files for
newer chips that have not yet been added to the kernel. This patch
defines a new DT 'compatible' string that doesn't require the 4 byte
variance from the hardware integration data. SoC DT data for future
Tegra chips should use this new 'compatible' string and the correct
Tegra AHB base address.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Hiroshi DOYU <hdoyu@nvidia.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: linux-kernel at vger.kernel.org
---
drivers/amba/tegra-ahb.c | 34 ++++++++++++++++++++++++++--------
1 file changed, 26 insertions(+), 8 deletions(-)
diff --git a/drivers/amba/tegra-ahb.c b/drivers/amba/tegra-ahb.c
index 30759a5..eac6934 100644
--- a/drivers/amba/tegra-ahb.c
+++ b/drivers/amba/tegra-ahb.c
@@ -26,6 +26,7 @@
#include <linux/platform_device.h>
#include <linux/io.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <soc/tegra/ahb.h>
@@ -120,16 +121,17 @@ struct tegra_ahb {
void __iomem *regs;
struct device *dev;
u32 ctx[0];
+ short offset;
};
static inline u32 gizmo_readl(struct tegra_ahb *ahb, u32 offset)
{
- return readl(ahb->regs - 4 + offset);
+ return readl(ahb->regs + ahb->offset + offset);
}
static inline void gizmo_writel(struct tegra_ahb *ahb, u32 value, u32 offset)
{
- writel(value, ahb->regs - 4 + offset);
+ writel(value, ahb->regs + ahb->offset + offset);
}
#ifdef CONFIG_TEGRA_IOMMU_SMMU
@@ -246,11 +248,30 @@ static void tegra_ahb_gizmo_init(struct tegra_ahb *ahb)
gizmo_writel(ahb, val, AHB_MEM_PREFETCH_CFG4);
}
+struct ahb_data {
+ short offset;
+};
+
+static const struct ahb_data correct_offset;
+static const struct ahb_data broken_offset = {
+ .offset = -4,
+};
+
+static const struct of_device_id tegra_ahb_of_match[] = {
+ { .compatible = "nvidia,tegra132-ahb", .data = &correct_offset },
+ { .compatible = "nvidia,tegra30-ahb", .data = &broken_offset },
+ { .compatible = "nvidia,tegra20-ahb", .data = &broken_offset },
+ {},
+};
+
static int tegra_ahb_probe(struct platform_device *pdev)
{
struct resource *res;
struct tegra_ahb *ahb;
size_t bytes;
+ const struct of_device_id *of_id =
+ of_match_device(tegra_ahb_of_match, &pdev->dev);
+ const struct ahb_data *ad;
bytes = sizeof(*ahb) + sizeof(u32) * ARRAY_SIZE(tegra_ahb_gizmo);
ahb = devm_kzalloc(&pdev->dev, bytes, GFP_KERNEL);
@@ -262,18 +283,15 @@ static int tegra_ahb_probe(struct platform_device *pdev)
if (IS_ERR(ahb->regs))
return PTR_ERR(ahb->regs);
+ ad = of_id->data;
ahb->dev = &pdev->dev;
+ ahb->offset = ad->offset;
+
platform_set_drvdata(pdev, ahb);
tegra_ahb_gizmo_init(ahb);
return 0;
}
-static const struct of_device_id tegra_ahb_of_match[] = {
- { .compatible = "nvidia,tegra30-ahb", },
- { .compatible = "nvidia,tegra20-ahb", },
- {},
-};
-
static struct platform_driver tegra_ahb_driver = {
.probe = tegra_ahb_probe,
.driver = {
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv2 1/3] amba: tegra-ahb: fix register offsets in the macros
2015-03-17 8:32 [PATCHv2 0/3] amba: tegra-ahb: fix base address and register offsets for future chip support Paul Walmsley
@ 2015-03-17 8:32 ` Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 2/3] amba: tegra-ahb: use correct base address for future chip support Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change Paul Walmsley
2 siblings, 0 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-17 8:32 UTC (permalink / raw)
To: linux-arm-kernel
>From a hardware SoC integration point of view, the offsets of the
Tegra AHB registers that are currently defined in tegra-ahb.c macros
are all off by four bytes. Similarly, the starting address of this IP
block in our existing DT files is also off by four bytes. Since we
attempt to make old DT files forward-compatible with newer kernels, we
cannot fix the IP block base address in old DT data. However, we can
fix the offsets in the driver so that they are correct with respect to
the hardware, which is what this patch does. And a subsequent patch
will allow the offset to be removed for DT 'compatible' strings used
in future DT files for newer Tegra chips that the kernel does not yet
support.
Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Paul Walmsley <pwalmsley@nvidia.com>
Cc: Alexandre Courbot <gnurou@gmail.com>
Cc: Hiroshi DOYU <hdoyu@nvidia.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: linux-kernel at vger.kernel.org
---
drivers/amba/tegra-ahb.c | 63 +++++++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 31 deletions(-)
diff --git a/drivers/amba/tegra-ahb.c b/drivers/amba/tegra-ahb.c
index c6dc354..30759a5 100644
--- a/drivers/amba/tegra-ahb.c
+++ b/drivers/amba/tegra-ahb.c
@@ -25,49 +25,50 @@
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/io.h>
+#include <linux/of.h>
#include <soc/tegra/ahb.h>
#define DRV_NAME "tegra-ahb"
-#define AHB_ARBITRATION_DISABLE 0x00
-#define AHB_ARBITRATION_PRIORITY_CTRL 0x04
+#define AHB_ARBITRATION_DISABLE 0x04
+#define AHB_ARBITRATION_PRIORITY_CTRL 0x08
#define AHB_PRIORITY_WEIGHT(x) (((x) & 0x7) << 29)
#define PRIORITY_SELECT_USB BIT(6)
#define PRIORITY_SELECT_USB2 BIT(18)
#define PRIORITY_SELECT_USB3 BIT(17)
-#define AHB_GIZMO_AHB_MEM 0x0c
+#define AHB_GIZMO_AHB_MEM 0x10
#define ENB_FAST_REARBITRATE BIT(2)
#define DONT_SPLIT_AHB_WR BIT(7)
-#define AHB_GIZMO_APB_DMA 0x10
-#define AHB_GIZMO_IDE 0x18
-#define AHB_GIZMO_USB 0x1c
-#define AHB_GIZMO_AHB_XBAR_BRIDGE 0x20
-#define AHB_GIZMO_CPU_AHB_BRIDGE 0x24
-#define AHB_GIZMO_COP_AHB_BRIDGE 0x28
-#define AHB_GIZMO_XBAR_APB_CTLR 0x2c
-#define AHB_GIZMO_VCP_AHB_BRIDGE 0x30
-#define AHB_GIZMO_NAND 0x3c
-#define AHB_GIZMO_SDMMC4 0x44
-#define AHB_GIZMO_XIO 0x48
-#define AHB_GIZMO_BSEV 0x60
-#define AHB_GIZMO_BSEA 0x70
-#define AHB_GIZMO_NOR 0x74
-#define AHB_GIZMO_USB2 0x78
-#define AHB_GIZMO_USB3 0x7c
+#define AHB_GIZMO_APB_DMA 0x14
+#define AHB_GIZMO_IDE 0x1c
+#define AHB_GIZMO_USB 0x20
+#define AHB_GIZMO_AHB_XBAR_BRIDGE 0x24
+#define AHB_GIZMO_CPU_AHB_BRIDGE 0x28
+#define AHB_GIZMO_COP_AHB_BRIDGE 0x2c
+#define AHB_GIZMO_XBAR_APB_CTLR 0x30
+#define AHB_GIZMO_VCP_AHB_BRIDGE 0x34
+#define AHB_GIZMO_NAND 0x40
+#define AHB_GIZMO_SDMMC4 0x48
+#define AHB_GIZMO_XIO 0x4c
+#define AHB_GIZMO_BSEV 0x64
+#define AHB_GIZMO_BSEA 0x74
+#define AHB_GIZMO_NOR 0x78
+#define AHB_GIZMO_USB2 0x7c
+#define AHB_GIZMO_USB3 0x80
#define IMMEDIATE BIT(18)
-#define AHB_GIZMO_SDMMC1 0x80
-#define AHB_GIZMO_SDMMC2 0x84
-#define AHB_GIZMO_SDMMC3 0x88
-#define AHB_MEM_PREFETCH_CFG_X 0xd8
-#define AHB_ARBITRATION_XBAR_CTRL 0xdc
-#define AHB_MEM_PREFETCH_CFG3 0xe0
-#define AHB_MEM_PREFETCH_CFG4 0xe4
-#define AHB_MEM_PREFETCH_CFG1 0xec
-#define AHB_MEM_PREFETCH_CFG2 0xf0
+#define AHB_GIZMO_SDMMC1 0x84
+#define AHB_GIZMO_SDMMC2 0x88
+#define AHB_GIZMO_SDMMC3 0x8c
+#define AHB_MEM_PREFETCH_CFG_X 0xdc
+#define AHB_ARBITRATION_XBAR_CTRL 0xe0
+#define AHB_MEM_PREFETCH_CFG3 0xe4
+#define AHB_MEM_PREFETCH_CFG4 0xe8
+#define AHB_MEM_PREFETCH_CFG1 0xf0
+#define AHB_MEM_PREFETCH_CFG2 0xf4
#define PREFETCH_ENB BIT(31)
#define MST_ID(x) (((x) & 0x1f) << 26)
#define AHBDMA_MST_ID MST_ID(5)
@@ -77,7 +78,7 @@
#define ADDR_BNDRY(x) (((x) & 0xf) << 21)
#define INACTIVITY_TIMEOUT(x) (((x) & 0xffff) << 0)
-#define AHB_ARBITRATION_AHB_MEM_WRQUE_MST_ID 0xf8
+#define AHB_ARBITRATION_AHB_MEM_WRQUE_MST_ID 0xfc
#define AHB_ARBITRATION_XBAR_CTRL_SMMU_INIT_DONE BIT(17)
@@ -123,12 +124,12 @@ struct tegra_ahb {
static inline u32 gizmo_readl(struct tegra_ahb *ahb, u32 offset)
{
- return readl(ahb->regs + offset);
+ return readl(ahb->regs - 4 + offset);
}
static inline void gizmo_writel(struct tegra_ahb *ahb, u32 value, u32 offset)
{
- writel(value, ahb->regs + offset);
+ writel(value, ahb->regs - 4 + offset);
}
#ifdef CONFIG_TEGRA_IOMMU_SMMU
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-17 8:32 ` [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change Paul Walmsley
@ 2015-03-17 10:38 ` Russell King - ARM Linux
2015-03-19 15:26 ` Paul Walmsley
0 siblings, 1 reply; 13+ messages in thread
From: Russell King - ARM Linux @ 2015-03-17 10:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> Required properties:
> - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
> - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
> - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> - tegra132, or tegra210.
> -- reg : Should contain 1 register ranges(address and length)
> + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and Tegra124, must
> + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
> + or tegra124. For Tegra132, the compatible string must contain
> + "nvidia,tegra132-ahb".
> +
> +- reg : Should contain 1 register ranges(address and length). On Tegra20,
> + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> + address of the IP block must end in 0x04. On DT files for later chips, the
> + actual hardware base address of the IP block should be used.
You could check that in the driver. If you can check it in the driver,
you can also decide to ignore it if it were offset by 0x04 (possibly
printing a warning.) That opens up the ability to fix the older Tegra
DT files going forward while still remaining compatible with existing
DT files, and avoiding the need for a complex note about this.
--
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-17 10:38 ` Russell King - ARM Linux
@ 2015-03-19 15:26 ` Paul Walmsley
2015-03-19 15:42 ` Stephen Warren
0 siblings, 1 reply; 13+ messages in thread
From: Paul Walmsley @ 2015-03-19 15:26 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> > Required properties:
> > - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
> > - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
> > - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > - tegra132, or tegra210.
> > -- reg : Should contain 1 register ranges(address and length)
> > + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and Tegra124, must
> > + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
> > + or tegra124. For Tegra132, the compatible string must contain
> > + "nvidia,tegra132-ahb".
> > +
> > +- reg : Should contain 1 register ranges(address and length). On Tegra20,
> > + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
> > + address of the IP block must end in 0x04. On DT files for later chips, the
> > + actual hardware base address of the IP block should be used.
>
> You could check that in the driver. If you can check it in the driver,
> you can also decide to ignore it if it were offset by 0x04 (possibly
> printing a warning.) That opens up the ability to fix the older Tegra
> DT files going forward while still remaining compatible with existing
> DT files, and avoiding the need for a complex note about this.
That's fine, I'll do that and drop this patch.
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 15:26 ` Paul Walmsley
@ 2015-03-19 15:42 ` Stephen Warren
2015-03-19 16:17 ` Paul Walmsley
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Warren @ 2015-03-19 15:42 UTC (permalink / raw)
To: linux-arm-kernel
On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
>
>> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
>>> Required properties:
>>> - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
>>> - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
>>> - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>> - tegra132, or tegra210.
>>> -- reg : Should contain 1 register ranges(address and length)
>>> + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and Tegra124, must
>>> + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra114
>>> + or tegra124. For Tegra132, the compatible string must contain
>>> + "nvidia,tegra132-ahb".
>>> +
>>> +- reg : Should contain 1 register ranges(address and length). On Tegra20,
>>> + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical base
>>> + address of the IP block must end in 0x04. On DT files for later chips, the
>>> + actual hardware base address of the IP block should be used.
>>
>> You could check that in the driver. If you can check it in the driver,
>> you can also decide to ignore it if it were offset by 0x04 (possibly
>> printing a warning.) That opens up the ability to fix the older Tegra
>> DT files going forward while still remaining compatible with existing
>> DT files, and avoiding the need for a complex note about this.
>
> That's fine, I'll do that and drop this patch.
Don't we still want to update the DT binding documentation to state what
the preferred base address (or at least set of legal base addresses) is/are?
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 15:42 ` Stephen Warren
@ 2015-03-19 16:17 ` Paul Walmsley
2015-03-19 16:46 ` Paul Walmsley
2015-03-19 16:54 ` Stephen Warren
0 siblings, 2 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-19 16:17 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 19 Mar 2015, Stephen Warren wrote:
> On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> > On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
> >
> > > On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> > > > Required properties:
> > > > - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
> > > > - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
> > > > - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > > > - tegra132, or tegra210.
> > > > -- reg : Should contain 1 register ranges(address and length)
> > > > + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and
> > > > Tegra124, must
> > > > + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > tegra114
> > > > + or tegra124. For Tegra132, the compatible string must contain
> > > > + "nvidia,tegra132-ahb".
> > > > +
> > > > +- reg : Should contain 1 register ranges(address and length). On
> > > > Tegra20,
> > > > + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
> > > > base
> > > > + address of the IP block must end in 0x04. On DT files for later
> > > > chips, the
> > > > + actual hardware base address of the IP block should be used.
> > >
> > > You could check that in the driver. If you can check it in the driver,
> > > you can also decide to ignore it if it were offset by 0x04 (possibly
> > > printing a warning.) That opens up the ability to fix the older Tegra
> > > DT files going forward while still remaining compatible with existing
> > > DT files, and avoiding the need for a complex note about this.
> >
> > That's fine, I'll do that and drop this patch.
>
> Don't we still want to update the DT binding documentation to state what the
> preferred base address (or at least set of legal base addresses) is/are?
As far as I know, the DT binding documents are intended to be a
reference for IP block integration data like base addresses. At least,
that is not how they've been used in the past, in the cases that I'm
familiar with.
I can see some marginal utility in changing the base address in the
example. But since the worst possible outcome of using the old address is
a warning message at boot, that margin seems quite small indeed. Anyone
who would blindly use the base address from the example to create a DT
file for a new Tegra SoC isn't doing it correctly.
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 16:17 ` Paul Walmsley
@ 2015-03-19 16:46 ` Paul Walmsley
2015-03-19 16:54 ` Stephen Warren
1 sibling, 0 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-19 16:46 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 19 Mar 2015, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
> > On 03/19/2015 09:26 AM, Paul Walmsley wrote:
> > > On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
> > >
> > > > On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
> > > > > Required properties:
> > > > > - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
> > > > > - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
> > > > > - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
> > > > > - tegra132, or tegra210.
> > > > > -- reg : Should contain 1 register ranges(address and length)
> > > > > + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and
> > > > > Tegra124, must
> > > > > + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
> > > > > tegra114
> > > > > + or tegra124. For Tegra132, the compatible string must contain
> > > > > + "nvidia,tegra132-ahb".
> > > > > +
> > > > > +- reg : Should contain 1 register ranges(address and length). On
> > > > > Tegra20,
> > > > > + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
> > > > > base
> > > > > + address of the IP block must end in 0x04. On DT files for later
> > > > > chips, the
> > > > > + actual hardware base address of the IP block should be used.
> > > >
> > > > You could check that in the driver. If you can check it in the driver,
> > > > you can also decide to ignore it if it were offset by 0x04 (possibly
> > > > printing a warning.) That opens up the ability to fix the older Tegra
> > > > DT files going forward while still remaining compatible with existing
> > > > DT files, and avoiding the need for a complex note about this.
> > >
> > > That's fine, I'll do that and drop this patch.
> >
> > Don't we still want to update the DT binding documentation to state what the
> > preferred base address (or at least set of legal base addresses) is/are?
>
> As far as I know, the DT binding documents are intended to be a
> reference for IP block integration data like base addresses.
The above is missing an important word: it should have read "are _not_
intended"
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 16:17 ` Paul Walmsley
2015-03-19 16:46 ` Paul Walmsley
@ 2015-03-19 16:54 ` Stephen Warren
2015-03-19 17:55 ` Paul Walmsley
1 sibling, 1 reply; 13+ messages in thread
From: Stephen Warren @ 2015-03-19 16:54 UTC (permalink / raw)
To: linux-arm-kernel
On 03/19/2015 10:17 AM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> On 03/19/2015 09:26 AM, Paul Walmsley wrote:
>>> On Tue, 17 Mar 2015, Russell King - ARM Linux wrote:
>>>
>>>> On Tue, Mar 17, 2015 at 01:32:21AM -0700, Paul Walmsley wrote:
>>>>> Required properties:
>>>>> - compatible : For Tegra20, must contain "nvidia,tegra20-ahb". For
>>>>> - Tegra30, must contain "nvidia,tegra30-ahb". Otherwise, must contain
>>>>> - '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is tegra124,
>>>>> - tegra132, or tegra210.
>>>>> -- reg : Should contain 1 register ranges(address and length)
>>>>> + Tegra30, must contain "nvidia,tegra30-ahb". For Tegra114 and
>>>>> Tegra124, must
>>>>> + contain '"nvidia,<chip>-ahb", "nvidia,tegra30-ahb"' where <chip> is
>>>>> tegra114
>>>>> + or tegra124. For Tegra132, the compatible string must contain
>>>>> + "nvidia,tegra132-ahb".
>>>>> +
>>>>> +- reg : Should contain 1 register ranges(address and length). On
>>>>> Tegra20,
>>>>> + Tegra30, Tegra114, and Tegra124 chips, the low byte of the physical
>>>>> base
>>>>> + address of the IP block must end in 0x04. On DT files for later
>>>>> chips, the
>>>>> + actual hardware base address of the IP block should be used.
>>>>
>>>> You could check that in the driver. If you can check it in the driver,
>>>> you can also decide to ignore it if it were offset by 0x04 (possibly
>>>> printing a warning.) That opens up the ability to fix the older Tegra
>>>> DT files going forward while still remaining compatible with existing
>>>> DT files, and avoiding the need for a complex note about this.
>>>
>>> That's fine, I'll do that and drop this patch.
>>
>> Don't we still want to update the DT binding documentation to state what the
>> preferred base address (or at least set of legal base addresses) is/are?
>
> As far as I know, the DT binding documents are intended to be a
> reference for IP block integration data like base addresses. At least,
> that is not how they've been used in the past, in the cases that I'm
> familiar with.
>
> I can see some marginal utility in changing the base address in the
> example. But since the worst possible outcome of using the old address is
> a warning message at boot, that margin seems quite small indeed. Anyone
> who would blindly use the base address from the example to create a DT
> file for a new Tegra SoC isn't doing it correctly.
The binding document is supposed to say what value the reg property
should have. If we require some unusual offset in the reg property (i.e.
something other than what the HW documentation describes as the module
base address), that ought to be documented. We do have this situation
for this module at present, although the documentation unfortunately
doesn't explicitly call this out even though the example alludes to it.
I do think we should at least fix the example so it isn't confusing and
inconsistent with expected practice. We could either switch the example
to Tegra210 so we only provide the best example going forward, or have
separate examples for Tegra20/210 to highlight the difference.
We should also add documentation that Chips before Tegra210 (or
Tegra132?) *require* the extra offset. Any code or DT written to the
existing (admittedly slightly implicit) binding needs to continue to
work, so we should document this unusual requirement, even if we enhance
the Linux driver to accept either mode of operation. Other OSs and old
versions of Linux will still need the exception for older SoCs.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 16:54 ` Stephen Warren
@ 2015-03-19 17:55 ` Paul Walmsley
2015-03-19 18:28 ` Stephen Warren
0 siblings, 1 reply; 13+ messages in thread
From: Paul Walmsley @ 2015-03-19 17:55 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 19 Mar 2015, Stephen Warren wrote:
> The binding document is supposed to say what value the reg property should
> have.
If you look at other DT binding documentation in the kernel, this is
generally not the case. Consider these examples:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-omap.txt
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
For example, the bcm2835 I2C binding documentation only mentions one of
the two I2C controllers apparently available on the system:
$ fgrep -r i2c arch/arm/boot/dts/ | fgrep bcm2835 | fgrep \@
arch/arm/boot/dts/bcm2835.dtsi: i2c0: i2c at 20205000 {
arch/arm/boot/dts/bcm2835.dtsi: i2c1: i2c at 7e804000 {
$
The Exynos documentation contains only one address of many I2C controllers
on the various SoCs:
$ fgrep -r i2c arch/arm/boot/dts/ | fgrep exynos | fgrep \@
...
arch/arm/boot/dts/exynos4415.dtsi: i2c_0: i2c at 13860000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_1: i2c at 13870000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_2: i2c at 13880000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_3: i2c at 13890000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_4: i2c at 138A0000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_5: i2c at 138B0000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_6: i2c at 138C0000 {
arch/arm/boot/dts/exynos4415.dtsi: i2c_7: i2c at 138D0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_0: i2c at 12C60000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_1: i2c at 12C70000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_2: i2c at 12C80000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_3: i2c at 12C90000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_4: i2c at 12CA0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_5: i2c at 12CB0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_6: i2c at 12CC0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_7: i2c at 12CD0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_8: i2c at 12CE0000 {
arch/arm/boot/dts/exynos5250.dtsi: i2c_9: i2c at 121D0000 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_0: i2c at 12C60000 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_1: i2c at 12C70000 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_2: i2c at 12C80000 {
arch/arm/boot/dts/exynos5420.dtsi: i2c_3: i2c at 12C90000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_4: i2c at 12CA0000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_5: i2c at 12CB0000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_6: i2c at 12CC0000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_7: i2c at 12CD0000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_8: i2c at 12E00000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_9: i2c at 12E10000 {
arch/arm/boot/dts/exynos5420.dtsi: hsi2c_10: i2c at 12E20000 {
arch/arm/boot/dts/exynos5440.dtsi: i2c at F0000 {
arch/arm/boot/dts/exynos5440.dtsi: i2c at 10000 {
...
$
And there are many other integration details that would need to be
specified in the documentation using the approach that you advocate - for
example, interrupt and DMA IDs, etc.
> If we require some unusual offset in the reg property (i.e. something
> other than what the HW documentation describes as the module base address),
> that ought to be documented. We do have this situation for this module at
> present, although the documentation unfortunately doesn't explicitly call this
> out even though the example alludes to it.
>
> I do think we should at least fix the example so it isn't confusing and
> inconsistent with expected practice. We could either switch the example to
> Tegra210 so we only provide the best example going forward, or have separate
> examples for Tegra20/210 to highlight the difference.
>
> We should also add documentation that Chips before Tegra210 (or
> Tegra132?) *require* the extra offset. Any code or DT written to the
> existing (admittedly slightly implicit) binding needs to continue to
> work, so we should document this unusual requirement, even if we enhance
> the Linux driver to accept either mode of operation.
After the two driver patches (after rmk's requested changes) are applied,
no unusual offset will be required, but if the legacy offset is specified,
it will be transparently handled.
As I see it, there are three possible cases:
1. the legacy, incorrect base address is used, in which case everything
will still work but there will be a warning;
2. the correct base address (from a hardware SoC integration point of
view) is used, in which case everything will work with no warnings,
3. a novel, completely incorrect base address is used, in which case the
IP block won't work at all and the driver will fail completely
After the patches, the driver now handles the first two cases. If you
would like the DT binding documentation practice changed to attempt to
address the third case, by requiring DT binding documentation to contain
lists of the correct IP integration data for every possible chip that
contains that IP block, as you mention above, such a change would be a
major delta from existing kernel practice, so would certainly mandate
submitting a patch for the common DT binding documentation file at
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt
> Other OSs and old versions of Linux will still need the exception for
> older SoCs.
How about this: I will send a patch for the DT binding documentation to
note that versions of Linux prior to v4.1 (unless Torvalds runs another
poll) require the four-byte-offset base address. Is that sufficient to
address your concerns with this series?
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 17:55 ` Paul Walmsley
@ 2015-03-19 18:28 ` Stephen Warren
2015-03-19 18:46 ` Paul Walmsley
0 siblings, 1 reply; 13+ messages in thread
From: Stephen Warren @ 2015-03-19 18:28 UTC (permalink / raw)
To: linux-arm-kernel
On 03/19/2015 11:55 AM, Paul Walmsley wrote:
> On Thu, 19 Mar 2015, Stephen Warren wrote:
>
>> The binding document is supposed to say what value the reg property should
>> have.
>
> If you look at other DT binding documentation in the kernel, this is
> generally not the case. Consider these examples:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/brcm,bcm2835-i2c.txt
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-omap.txt
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/i2c/i2c-exynos5.txt
That is because there are no special requirements for the reg values
beyond the HW documentation.
However, if we need the reg value to contain something other than the
base address that's in the HW documentation, we clearly need to document
that exception. How else would anyone know about the exception?
The example doesn't count because (a) it's not normative (b) an example
wouldn't explain why an exception needs to be made or how to calculate
the exception value for cases other than the specific example given.
...
>> If we require some unusual offset in the reg property (i.e. something
>> other than what the HW documentation describes as the module base address),
>> that ought to be documented. We do have this situation for this module at
>> present, although the documentation unfortunately doesn't explicitly call this
>> out even though the example alludes to it.
>>
>> I do think we should at least fix the example so it isn't confusing and
>> inconsistent with expected practice. We could either switch the example to
>> Tegra210 so we only provide the best example going forward, or have separate
>> examples for Tegra20/210 to highlight the difference.
>>
>> We should also add documentation that Chips before Tegra210 (or
>> Tegra132?) *require* the extra offset. Any code or DT written to the
>> existing (admittedly slightly implicit) binding needs to continue to
>> work, so we should document this unusual requirement, even if we enhance
>> the Linux driver to accept either mode of operation.
>
> After the two driver patches (after rmk's requested changes) are applied,
> no unusual offset will be required, but if the legacy offset is specified,
> it will be transparently handled.
>
> As I see it, there are three possible cases:
>
> 1. the legacy, incorrect base address is used, in which case everything
> will still work but there will be a warning;
>
> 2. the correct base address (from a hardware SoC integration point of
> view) is used, in which case everything will work with no warnings,
>
> 3. a novel, completely incorrect base address is used, in which case the
> IP block won't work at all and the driver will fail completely
>
> After the patches, the driver now handles the first two cases. If you
> would like the DT binding documentation practice changed to attempt to
> address the third case, by requiring DT binding documentation to contain
> lists of the correct IP integration data for every possible chip that
> contains that IP block, as you mention above, such a change would be a
> major delta from existing kernel practice, so would certainly mandate
> submitting a patch for the common DT binding documentation file at
That's not what I'm asking for. I want exceptions to standard practice
documented, which is that reg contains whatever the HW documentation
says it should. There's no need to enumerate all the valid values; the
HW documentation does that. However, if the DT binding requires
something other than what the HW documentation says, we must document that.
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/submitting-patches.txt
>
>> Other OSs and old versions of Linux will still need the exception for
>> older SoCs.
>
> How about this: I will send a patch for the DT binding documentation to
> note that versions of Linux prior to v4.1 (unless Torvalds runs another
> poll) require the four-byte-offset base address. Is that sufficient to
> address your concerns with this series?
Almost yes.
We should not document Linux 4.1 as the cut-off. DT bindings are
supposed to be OS agnostic. While it's practically unlikely, it is
entirely possible for some other OS to have already implemented support
for this binding, and the current binding is an ABI. We have no control
over if/when any non-Linux code is updated to add support for a 0-based
offset for existing SoCs, and certainly no versions of Linux or any
other OS can be updated retro-actively except perhaps a few linux-stable
versions. We can however write the binding in such a way as support for
new SoCs requires the new 0-based address, since there is no binding
specification for those new chips yet, and the time when you add the new
binding documentation is the first time any OS could possibly add
conformant support for it.
In summary, I believe the binding document must state that
T20/30/114/124 require the offset of 4 in reg value, and newer chips
require no offset in the reg value. We can still always accept either in
the Linux kernel going forward based on the principle of being lenient
re: input data.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change
2015-03-19 18:28 ` Stephen Warren
@ 2015-03-19 18:46 ` Paul Walmsley
0 siblings, 0 replies; 13+ messages in thread
From: Paul Walmsley @ 2015-03-19 18:46 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, 19 Mar 2015, Stephen Warren wrote:
> We should not document Linux 4.1 as the cut-off. DT bindings are supposed to
> be OS agnostic. While it's practically unlikely, it is entirely possible for
> some other OS to have already implemented support for this binding, and the
> current binding is an ABI. We have no control over if/when any non-Linux code
> is updated to add support for a 0-based offset for existing SoCs, and
> certainly no versions of Linux or any other OS can be updated retro-actively
> except perhaps a few linux-stable versions. We can however write the binding
> in such a way as support for new SoCs requires the new 0-based address, since
> there is no binding specification for those new chips yet, and the time when
> you add the new binding documentation is the first time any OS could possibly
> add conformant support for it.
>
> In summary, I believe the binding document must state that T20/30/114/124
> require the offset of 4 in reg value, and newer chips require no offset in the
> reg value. We can still always accept either in the Linux kernel going forward
> based on the principle of being lenient re: input data.
That's fine. I'll send a patch for that.
- Paul
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-03-19 18:46 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-17 8:32 [PATCHv2 0/3] amba: tegra-ahb: fix base address and register offsets for future chip support Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 1/3] amba: tegra-ahb: fix register offsets in the macros Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 2/3] amba: tegra-ahb: use correct base address for future chip support Paul Walmsley
2015-03-17 8:32 ` [PATCHv2 3/3] Documentation: DT bindings: Tegra AHB: note base address change Paul Walmsley
2015-03-17 10:38 ` Russell King - ARM Linux
2015-03-19 15:26 ` Paul Walmsley
2015-03-19 15:42 ` Stephen Warren
2015-03-19 16:17 ` Paul Walmsley
2015-03-19 16:46 ` Paul Walmsley
2015-03-19 16:54 ` Stephen Warren
2015-03-19 17:55 ` Paul Walmsley
2015-03-19 18:28 ` Stephen Warren
2015-03-19 18:46 ` Paul Walmsley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).