memory: tegra: Add MC channels and error logging
v5 cover memory: tegra: Add MC channels and error logging
From tegra186 onward, memory controllers support multiple channels. Add memory controller channels in device tree and add support to map address spaces of these channels in tegra MC driver.
When memory controller interrupt occurs, registers from these channels are required to be read in order to get error information. Add error logging support from tegra186 onward for memory controller interrupts.
Ashish Mhetre (4):
memory: tegra: Add memory controller channels support
memory: tegra: Add MC error logging on tegra186 onward
dt-bindings: memory: Update reg maxitems for tegra186
arm64: tegra: Add memory controller channels
---
Changes in v5:
- Updated patch sequence such that driver patches are before DT patches
- Fixed DT ABI break from v4
- Fixed smatch bug
- Updated description in DT binding documentation
- Updated variable names
Changes in v4:
- Added memory controller channels support
- Added newlines after every break statement of all switch cases
- Fixed compile error with W=1 build
- Fixed the interrupt mask bit logic
Changes in v3:
- Removed unnecessary ifdefs
- Grouped newly added MC registers with existing MC registers
- Removed unnecessary initialization of variables
- Updated code to use newly added field 'has_addr_hi_reg' instead of ifdefs
Changes in v2:
- Updated patch subject and commit message
- Removed separate irq handlers
- Updated tegra30_mc_handle_irq to be used for tegra186 onwards as well
.../nvidia,tegra186-mc.yaml | 16 ++- arch/arm64/boot/dts/nvidia/tegra186.dtsi | 7 +- arch/arm64/boot/dts/nvidia/tegra194.dtsi | 21 +++- arch/arm64/boot/dts/nvidia/tegra234.dtsi | 21 +++- drivers/memory/tegra/mc.c | 114 +++++++++++++++--- drivers/memory/tegra/mc.h | 37 +++++- drivers/memory/tegra/tegra186.c | 87 +++++++++++++ drivers/memory/tegra/tegra194.c | 44 +++++++ drivers/memory/tegra/tegra234.c | 59 +++++++++ include/soc/tegra/mc.h | 11 ++ 10 files changed, 389 insertions(+), 28 deletions(-)
Signature · 1 line
2.17.1
PATCH v5 1/4 memory: tegra: Add memory controller channels support
From tegra186 onwards, memory controller support multiple channels. Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.
--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)
drivers/memory/tegra/mc.c +6 -0
@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {
drivers/memory/tegra/tegra186.c +52 -0
69 lines not on this page · raw message
drivers/memory/tegra/tegra194.c +1 -0
@@ -1347,5 +1347,6 @@ const struct tegra_mc_soc tegra194_mc_soc = { .num_clients = ARRAY_SIZE(tegra194_mc_clients), .clients = tegra194_mc_clients, .num_address_bits = 40, + .num_channels = 16, .ops = &tegra186_mc_ops, };
drivers/memory/tegra/tegra234.c +1 -0
@@ -97,5 +97,6 @@ const struct tegra_mc_soc tegra234_mc_soc = { .num_clients = ARRAY_SIZE(tegra234_mc_clients), .clients = tegra234_mc_clients, .num_address_bits = 40, + .num_channels = 16, .ops = &tegra186_mc_ops, };
include/soc/tegra/mc.h +7 -0
@@ -13,6 +13,9 @@#include <linux/irq.h> #include <linux/reset-controller.h> #include <linux/types.h> +#include <linux/platform_device.h> + +#define MC_MAX_CHANNELS 16struct clk ;struct device ;
@@ -181,6 +184,7 @@ struct tegra_mc_ops {int (*resume)(struct tegra_mc *mc);irqreturn_t (*handle_irq)(int irq,void *data);int (*probe_device)(struct tegra_mc *mc,struct device *dev); +int (*map_regs)(struct tegra_mc *mc,struct platform_device *pdev); };struct tegra_mc_soc {
@@ -194,6 +198,7 @@ struct tegra_mc_soc {unsigned int atom_size;u8 client_id_mask; +u8 num_channels;const struct tegra_smmu_soc *smmu;
@@ -212,6 +217,8 @@ struct tegra_mc {struct tegra_smmu *smmu;struct gart_device *gart;void __iomem *regs; +void __iomem *mcb_regs; +void __iomem *mc_regs[MC_MAX_CHANNELS ];struct clk *clk;int irq;
Signature · 1 line
2.17.1
Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 64 lines
From tegra186 onwards, memory controller support multiple channels. Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.
Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +24
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val;
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+ + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels. +* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );
Update what?
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 179
Quoted 4 lines
drivers/memory/tegra/tegra186.c +4
-0+return 0 ; + } + + mc->mcb_regs =devm_platform_get_and_ioremap_resource (pdev,1 , &res);
Can't we name each reg bank individually in the DT and then use devm_platform_ioremap_resource_byname()?
...
drivers/memory/tegra/tegra186.c struct tegra_mc { · line 221
Quoted 6 lines
drivers/memory/tegra/tegra186.c +2
-0@@ -212,6 +217,8 @@ struct tegra_mc {struct tegra_smmu *smmu;struct gart_device *gart;void __iomem *regs; +void __iomem *mcb_regs; +void __iomem *mc_regs[MC_MAX_CHANNELS ];
s/mc_regs/ch_regs/ ?
Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
Quoted 72 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 68 lines
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +28
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val; + + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/*
Quoted 8 lines
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 6 lines
drivers/memory/tegra/tegra186.c +6
-0+* On tegra186 onwards, memory controller support multiple channels. +* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );Update what >
"Update memory controller DT node with MC channels". Yes, it's unclear. I will update in next version.
Quoted 9 lines
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 179
Quoted 4 lines
drivers/memory/tegra/tegra186.c +4
-0+return 0 ; + } + + mc->mcb_regs =devm_platform_get_and_ioremap_resource (pdev,1 , &res);Can't we name each reg bank individually in the DT and then use
devm_platform_ioremap_resource_byname()?...
Quoted 8 lines
drivers/memory/tegra/tegra186.c struct tegra_mc { · line 221
Quoted 6 lines
drivers/memory/tegra/tegra186.c +2
-0@@ -212,6 +217,8 @@ struct tegra_mc {struct tegra_smmu *smmu;struct gart_device *gart;void __iomem *regs; +void __iomem *mcb_regs; +void __iomem *mc_regs[MC_MAX_CHANNELS ];s/mc_regs/ch_regs/ ?
Sure, will update in next version.
Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
In reply to Dmitry OsipenkoQuoted 81 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 64 lines
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +24
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val;drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+ + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels. +* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );Update what?
Quoted 8 lines
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 179
Quoted 4 lines
drivers/memory/tegra/tegra186.c +4
-0+return 0 ; + } + + mc->mcb_regs =devm_platform_get_and_ioremap_resource (pdev,1 , &res);Can't we name each reg bank individually in the DT and then use
devm_platform_ioremap_resource_byname()?
That can be done but I think current logic will be better as we can simply ioremap them by running in loop and assigning the mc_regs array. Otherwise there will be like 17 ioremap_byname() individual calls for Tegra194 and Tegra234.
Will it be fine having that many ioremap_byname() calls? Also, Tegra186 has 5 channels which are less than Tegra194 and Tegra234. If we go with ioremap_byname() then we'll have to differentiate number of ioremap_byname() calls.
Quoted 9 lines
...
drivers/memory/tegra/tegra186.c struct tegra_mc { · line 221
Quoted 6 lines
drivers/memory/tegra/tegra186.c +2
-0@@ -212,6 +217,8 @@ struct tegra_mc {struct tegra_smmu *smmu;struct gart_device *gart;void __iomem *regs; +void __iomem *mcb_regs; +void __iomem *mc_regs[MC_MAX_CHANNELS ];s/mc_regs/ch_regs/ ?
Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
Quoted 101 lines · Ashish Mhetre
Quoted 89 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 74 lines
From tegra186 onwards, memory controller support multiple channels. Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.
Signed-off-by Ashish Mhetre <[email protected]>---
drivers/memory/tegra/mc.c | 6 ++++drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev) if (IS_ERR(mc->regs)) return PTR_ERR(mc->regs); + if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs(mc, pdev); + if (err < 0) + return err; + } + mc->debugfs.root = debugfs_create_dir("mc", NULL);if (
mc->soc->ops&&mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +29
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) return 0; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, + struct platform_device *pdev) +{ + struct device_node *np = pdev->dev.parent->of_node; + int num_dt_channels, reg_cells = 0; + struct resource *res; + int i, ret; + u32 val; + + ret = of_property_read_u32(np, "#address-cells", &val); + if (ret) { + dev_err(&pdev->dev, "missing #address-cells property\n"); + return ret; + } + + reg_cells = val; + + ret = of_property_read_u32(np, "#size-cells", &val); + if (ret) { + dev_err(&pdev->dev, "missing #size-cells property\n"); + return ret; + } + + reg_cells += val; + + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); + /* +* On tegra186 onwards, memory controller support multiple Quoted 8 lines
channels.
+ * Apart from regular memory controller channels, there is one broadcast
+ * channel and one for stream-id registers.
+ */
+ if (num_dt_channels<mc->soc->num_channels+ 2) { +dev_warn(&pdev->dev, "MC channels are missing, please update\n");Update what?
Quoted 9 lines
Quoted 5 lines
+ return 0;
+ }
+
+mc->mcb_regs=devm_platform_get_and_ioremap_resource(pdev, 1, &res);Can't we name each reg bank individually in the DT and then use
devm_platform_ioremap_resource_byname()?
Quoted 8 lines
That can be done but I think current logic will be better as we can simply ioremap them by running in loop and assigning the
mc_regsarray. Otherwise there will be like 17ioremap_byname()individual calls for Tegra194 and Tegra234.
Will it be fine having that manyioremap_byname()calls? Also, Tegra186 has 5 channels which are less than Tegra194 and Tegra234. If we go withioremap_byname()then we'll have to differentiate number ofioremap_byname()calls.
for (i = 0; i < mc->soc->num_channels; i++) {
sprintf(name, "mc%u", i);
err = devm_platform_ioremap_resource_byname(dev, name);
...
}Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
Quoted 113 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 101 lines · Ashish Mhetre
Quoted 89 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 81 lines
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +29
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) return 0; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val; + + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels.
+ * Apart from regular memory controller channels, there is one
broadcastdrivers/memory/tegra/tegra186.c +4
-0+* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { + dev_warn(&pdev->dev, "MC channels are missing, pleaseQuoted 1 line
update\n");
Update what?
Quoted 9 lines
Quoted 5 lines
drivers/memory/tegra/tegra186.c +4
-0+return 0 ; + } + + mc->mcb_regs = devm_platform_get_and_ioremap_resource(pdev, 1,&res);
Can't we name each reg bank individually in the DT and then use
devm_platform_ioremap_resource_byname()?Quoted 8 lines
That can be done but I think current logic will be better as we can simply ioremap them by running in loop and assigning the
mc_regsarray. Otherwise there will be like 17ioremap_byname()individual calls for Tegra194 and Tegra234.
Will it be fine having that manyioremap_byname()calls? Also, Tegra186 has 5 channels which are less than Tegra194 and Tegra234. If we go withioremap_byname()then we'll have to differentiate number ofioremap_byname()calls.
Quoted 5 lines
for (i = 0; i < mc->soc->num_channels; i++) {
sprintf(name, "mc%u", i);
err = devm_platform_ioremap_resource_byname(dev, name);
...
}Okay, for this reg banks should be named "mc0", "mc1", and so on. I will update this in v6.
Krzysztof Kozlowski
maintainerRe: [Patch v5 1/4] memory: tegra: Add memory controller channels support
In reply to Ashish MhetreQuoted 64 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels. Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.
Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +24
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val;
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+ + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels. +* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );
How did you address our previous comments about ABI break? I really do not see it.
Best regards,
Krzysztof
Re: [Patch v5 1/4] memory: tegra: Add memory controller channels support
Quoted 73 lines · Krzysztof Kozlowski
External email: Use caution opening links or attachments
Quoted 69 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +29
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val; + + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels.
Quoted 9 lines
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );How did you address our previous comments about ABI break? I really do not see it.
In v4 patch, error was returned from here and probe failed causing ABI break. In v5, we are checking if number of reg items in DT is as expected or not. If number of reg items are less then we are just printing warning to update DT and returning 0. So probe won't fail and driver will work as expected.
Also I had tested just driver patches with existing DT and it worked fine.
Quoted 2 lines
Best regards,
Krzysztof
Krzysztof Kozlowski
maintainerRe: [Patch v5 1/4] memory: tegra: Add memory controller channels support
Quoted 85 lines · Ashish Mhetre
Quoted 73 lines · Krzysztof Kozlowski
External email: Use caution opening links or attachments
Quoted 69 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels.
Add support for mapping address spaces of these channels. Make sure that number of channels are as expected on each SOC. During error interrupts from memory controller, appropriate registers from these channels need to be accessed for logging error info.Signed-off-by Ashish Mhetre <[email protected]>--- drivers/memory/tegra/mc.c | 6 ++++ drivers/memory/tegra/tegra186.c | 52 +++++++++++++++++++++++++++++++++ drivers/memory/tegra/tegra194.c | 1 + drivers/memory/tegra/tegra234.c | 1 + include/soc/tegra/mc.h | 7 +++++ 5 files changed, 67 insertions(+)drivers/memory/tegra/mc.c +6
-0@@ -749,6 +749,12 @@ static int tegra_mc_probe(struct platform_device *pdev)if (IS_ERR (mc->regs))return PTR_ERR (mc->regs); +if (mc->soc->ops && mc->soc->ops->map_regs) { + err = mc->soc->ops->map_regs (mc, pdev); +if (err <0 ) +return err; + } + mc->debugfs.root =debugfs_create_dir ("mc" ,NULL );if (mc->soc->ops && mc->soc->ops->probe) {drivers/memory/tegra/tegra186.c +29
-0@@ -139,11 +139,62 @@ static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev)return 0 ; } +static int tegra186_mc_map_regs (struct tegra_mc *mc, +struct platform_device *pdev) +{ +struct device_node *np = pdev->dev.parent->of_node; +int num_dt_channels, reg_cells =0 ; +struct resource *res; +int i, ret; +u32 val; + + ret =of_property_read_u32 (np,"#address-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #address-cells property\n" ); +return ret; + } + + reg_cells = val; + + ret =of_property_read_u32 (np,"#size-cells" , &val); +if (ret) { +dev_err (&pdev->dev,"missing #size-cells property\n" ); +return ret; + } + + reg_cells += val; + + num_dt_channels =of_property_count_elems_of_size (pdev->dev.of_node,"reg" , + reg_cells *sizeof (u32)); +/* +* On tegra186 onwards, memory controller support multiple channels. Quoted 9 lines
drivers/memory/tegra/tegra186.c static int tegra186_mc_probe_device(struct tegra_mc *mc, struct device *dev) · line 175
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+* Apart from regular memory controller channels, there is one broadcast +* channel and one for stream-id registers. +*/ +if (num_dt_channels < mc->soc->num_channels +2 ) { +dev_warn (&pdev->dev,"MC channels are missing, please update\n" );How did you address our previous comments about ABI break? I really do not see it.
Quoted 7 lines
In v4 patch, error was returned from here and probe failed causing ABI break. In v5, we are checking if number of reg items in DT is as expected or not. If number of reg items are less then we are just printing warning to update DT and returning 0. So probe won't fail and driver will work as expected.
Also I had tested just driver patches with existing DT and it worked fine.
Ah, right, thanks. I missed the return 0. Looks good, thanks for the changes and for explanation.
Best regards,
Krzysztof
PATCH v5 2/4 memory: tegra: Add MC error logging on tegra186 onward
In reply to Ashish MhetreAdd new function 'get_int_channel' in tegra_mc_soc struture which is implemented by tegra SOCs which support multiple MC channels. This function returns the channel which should be used to get the information of interrupts.
Remove static from tegra30_mc_handle_irq and use it as interrupt handler for MC interrupts on tegra186, tegra194 and tegra234 to log the errors. Add error specific MC status and address register bits and use them on tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194 and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234. Add register for higher bits of error address which is available on tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' in tegra_mc_soc struture which will be true if soc has register for higher bits of memory controller error address. Set it true for tegra194 and tegra234.
--- drivers/memory/tegra/mc.c | 108 +++++++++++++++++++++++++++----- drivers/memory/tegra/mc.h | 37 ++++++++++- drivers/memory/tegra/tegra186.c | 44 +++++++++++++ drivers/memory/tegra/tegra194.c | 43 +++++++++++++ drivers/memory/tegra/tegra234.c | 58 +++++++++++++++++ include/soc/tegra/mc.h | 4 ++ 6 files changed, 278 insertions(+), 16 deletions(-)
drivers/memory/tegra/mc.c +93 -15
@@ -508,14 +508,34 @@ int tegra30_mc_probe(struct tegra_mc *mc)return 0 ; } -static irqreturn_t tegra30_mc_handle_irq (int irq,void *data) +const struct tegra_mc_ops tegra30_mc_ops = { + .probe = tegra30_mc_probe, + .handle_irq = tegra30_mc_handle_irq, +}; +#endif + +irqreturn_t tegra30_mc_handle_irq (int irq,void *data) {struct tegra_mc *mc = data;unsigned long status; +bool mc_has_channels;unsigned int bit; +int channel; + + mc_has_channels = mc->soc->num_channels && mc->soc->get_int_channel; +if (mc_has_channels) { +int err; + + err = mc->soc->get_int_channel (mc, &channel); +if (err <0 ) +return IRQ_NONE ; + +/* mask all interrupts to avoid flooding */ + status =mc_ch_readl (mc, channel,MC_INTSTATUS ) & mc->soc->intmask; + }else { + status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask; + } -/* mask all interrupts to avoid flooding */ - status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask;if (!status)return IRQ_NONE ;
@@ -523,18 +543,70 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)const char *error = tegra_mc_status_names[bit] ?:"unknown" ;const char *client ="unknown" , *desc;const char *direction, *secure; +u32 status_reg, addr_reg; +u32 intmask =BIT (bit);phys_addr_t addr =0 ; +#ifdef CONFIG_PHYS_ADDR_T_64BIT +u32 addr_hi_reg =0 ; +#endif unsigned int i;char perm[7 ];u8 id, type;u32 value; - value =mc_readl (mc,MC_ERR_STATUS ); +switch (intmask) { +case MC_INT_DECERR_VPR : + status_reg =MC_ERR_VPR_STATUS ; + addr_reg =MC_ERR_VPR_ADR ; +break ; + +case MC_INT_SECERR_SEC : + status_reg =MC_ERR_SEC_STATUS ; + addr_reg =MC_ERR_SEC_ADR ; +break ; + +case MC_INT_DECERR_MTS : + status_reg =MC_ERR_MTS_STATUS ; + addr_reg =MC_ERR_MTS_ADR ; +break ; + +case MC_INT_DECERR_GENERALIZED_CARVEOUT : + status_reg =MC_ERR_GENERALIZED_CARVEOUT_STATUS ; + addr_reg =MC_ERR_GENERALIZED_CARVEOUT_ADR ; +break ; + +case MC_INT_DECERR_ROUTE_SANITY : + status_reg =MC_ERR_ROUTE_SANITY_STATUS ; + addr_reg =MC_ERR_ROUTE_SANITY_ADR ; +break ; + +default : + status_reg =MC_ERR_STATUS ; + addr_reg =MC_ERR_ADR ; + +#ifdef CONFIG_PHYS_ADDR_T_64BIT +if (mc->soc->has_addr_hi_reg) + addr_hi_reg =MC_ERR_ADR_HI ; +#endif +break ; + } + +if (mc_has_channels) + value =mc_ch_readl (mc, channel, status_reg); +else + value =mc_readl (mc, status_reg);#ifdef CONFIG_PHYS_ADDR_T_64BIT if (mc->soc->num_address_bits >32 ) { - addr = ((value >>MC_ERR_STATUS_ADR_HI_SHIFT ) & -MC_ERR_STATUS_ADR_HI_MASK ); +if (addr_hi_reg) { +if (mc_has_channels) + addr =mc_ch_readl (mc, channel, addr_hi_reg); +else + addr =mc_readl (mc, addr_hi_reg); + }else { + addr = ((value >>MC_ERR_STATUS_ADR_HI_SHIFT ) & +MC_ERR_STATUS_ADR_HI_MASK ); + } addr <<=32 ; }#endif
@@ -591,7 +663,10 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data)break ; } - value =mc_readl (mc,MC_ERR_ADR ); +if (mc_has_channels) + value =mc_ch_readl (mc, channel, addr_reg); +else + value =mc_readl (mc, addr_reg); addr |= value; dev_err_ratelimited(mc->dev, "%s: %s%s @%pa: %s (%s%s)\n",
@@ -600,17 +675,14 @@ static irqreturn_t tegra30_mc_handle_irq(int irq, void *data) }/* clear interrupts */ -mc_writel (mc, status,MC_INTSTATUS ); +if (mc_has_channels) +mc_ch_writel (mc, channel, status,MC_INTSTATUS ); +else +mc_writel (mc, status,MC_INTSTATUS );return IRQ_HANDLED ; } -const struct tegra_mc_ops tegra30_mc_ops = { - .probe = tegra30_mc_probe, - .handle_irq = tegra30_mc_handle_irq, -}; -#endif -const char *const tegra_mc_status_names[32 ] = { [1 ] ="External interrupt" , [6 ] ="EMEM address decode error" ,
@@ -622,6 +694,8 @@ const char *const tegra_mc_status_names[32] = { [12] = "VPR violation", [13] = "Secure carveout violation", [16] = "MTS carveout violation", + [17] = "Generalized carveout violation", + [20] = "Route Sanity error", };const char *const tegra_mc_error_names[8 ] = {
@@ -770,7 +844,11 @@ static int tegra_mc_probe(struct platform_device *pdev)WARN (!mc->soc->client_id_mask,"missing client ID mask for this SoC\n" ); -mc_writel (mc, mc->soc->intmask,MC_INTMASK ); +if (mc->soc->num_channels && mc->mcb_regs) +mc_ch_writel (mc,MC_BROADCAST_CHANNEL , mc->soc->intmask, +MC_INTMASK ); +else +mc_writel (mc, mc->soc->intmask,MC_INTMASK ); err =devm_request_irq (&pdev->dev, mc->irq, mc->soc->ops->handle_irq,0 ,dev_name (&pdev->dev), mc);
drivers/memory/tegra/mc.h +36 -1
@@ -43,7 +43,21 @@#define MC_EMEM_ARB_OVERRIDE 0xe8#define MC_TIMING_CONTROL_DBG 0xf8#define MC_TIMING_CONTROL 0xfc - +#define MC_ERR_VPR_STATUS 0x654 +#define MC_ERR_VPR_ADR 0x658 +#define MC_ERR_SEC_STATUS 0x67c +#define MC_ERR_SEC_ADR 0x680 +#define MC_ERR_MTS_STATUS 0x9b0 +#define MC_ERR_MTS_ADR 0x9b4 +#define MC_ERR_ROUTE_SANITY_STATUS 0x9c0 +#define MC_ERR_ROUTE_SANITY_ADR 0x9c4 +#define MC_ERR_GENERALIZED_CARVEOUT_STATUS 0xc00 +#define MC_ERR_GENERALIZED_CARVEOUT_ADR 0xc04 +#define MC_GLOBAL_INTSTATUS 0xf24 +#define MC_ERR_ADR_HI 0x11fc + +#define MC_INT_DECERR_ROUTE_SANITY BIT(20) +#define MC_INT_DECERR_GENERALIZED_CARVEOUT BIT(17)#define MC_INT_DECERR_MTS BIT(16)#define MC_INT_SECERR_SEC BIT(13)#define MC_INT_DECERR_VPR BIT(12)
@@ -78,6 +92,8 @@#define MC_TIMING_UPDATE BIT(0) +#define MC_BROADCAST_CHANNEL ~0 +static inline u32 tegra_mc_scale_percents (u64 val,unsigned int percents) { val = val * percents;
@@ -92,6 +108,24 @@ icc_provider_to_tegra_mc(struct icc_provider *provider) return container_of(provider, struct tegra_mc, provider); } +static inline u32 mc_ch_readl (const struct tegra_mc *mc,int ch, +unsigned long offset) +{ +if (ch ==MC_BROADCAST_CHANNEL ) +return readl_relaxed (mc->mcb_regs + offset); + +return readl_relaxed (mc->mc_regs[ch] + offset); +} + +static inline void mc_ch_writel (const struct tegra_mc *mc,int ch, +u32 value,unsigned long offset) +{ +if (ch ==MC_BROADCAST_CHANNEL ) +writel_relaxed (value, mc->mcb_regs + offset); +else +writel_relaxed (value, mc->mc_regs[ch] + offset); +} +static inline u32 mc_readl (const struct tegra_mc *mc,unsigned long offset) {return readl_relaxed (mc->regs + offset);
@@ -156,6 +190,7 @@ extern const struct tegra_mc_ops tegra30_mc_ops;extern const struct tegra_mc_ops tegra186_mc_ops;#endif +irqreturn_t tegra30_mc_handle_irq (int irq,void *data);extern const char *const tegra_mc_status_names[32 ];extern const char *const tegra_mc_error_names[8 ];
drivers/memory/tegra/tegra186.c +44 -0
67 lines not on this page · raw message
drivers/memory/tegra/tegra194.c +43 -0
53 lines not on this page · raw message
drivers/memory/tegra/tegra234.c +58 -0
68 lines not on this page · raw message
include/soc/tegra/mc.h +4 -0
@@ -203,6 +203,8 @@ struct tegra_mc_soc {const struct tegra_smmu_soc *smmu;u32 intmask; +u32 int_channel_mask; +bool has_addr_hi_reg;const struct tegra_mc_reset_ops *reset_ops;const struct tegra_mc_reset *resets;
@@ -210,6 +212,8 @@ struct tegra_mc_soc {const struct tegra_mc_icc_ops *icc_ops;const struct tegra_mc_ops *ops; + +int (*get_int_channel)(const struct tegra_mc *mc,int *mc_channel); };struct tegra_mc {
Signature · 1 line
2.17.1
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
16.03.2022 12:25, Ashish Mhetre пишет:
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 523
Quoted 7 lines
drivers/memory/tegra/mc.c +3
-0+irqreturn_t tegra30_mc_handle_irq (int irq,void *data) {struct tegra_mc *mc = data;unsigned long status; +bool mc_has_channels;unsigned int bit; +int channel;
unsigned int
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 537
Quoted 13 lines
drivers/memory/tegra/mc.c +13
-0+ mc_has_channels = mc->soc->num_channels && mc->soc->get_int_channel; +if (mc_has_channels) { +int err; + + err = mc->soc->get_int_channel (mc, &channel); +if (err <0 ) +return IRQ_NONE ; + +/* mask all interrupts to avoid flooding */ + status =mc_ch_readl (mc, channel,MC_INTSTATUS ) & mc->soc->intmask; + }else { + status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask; + }
So if mc_has_channels=false, while it should be true, then you're going to handle interrupt incorrectly?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
19.03.2022 18:50, Dmitry Osipenko пишет:
Quoted 7 lines
Quoted 7 lines
drivers/memory/tegra/mc.c +7
-0+ mc_has_channels = mc->soc->num_channels && mc->soc->get_int_channel; +if (mc_has_channels) { +int err; + + err = mc->soc->get_int_channel (mc, &channel); +if (err <0 ) +return IRQ_NONE ;
Quoted 8 lines
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 537
Quoted 6 lines
drivers/memory/tegra/mc.c +6
-0+ +/* mask all interrupts to avoid flooding */ + status =mc_ch_readl (mc, channel,MC_INTSTATUS ) & mc->soc->intmask; + }else { + status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask; + }So if mc_has_channels=false, while it should be true, then you're going to handle interrupt incorrectly?
I see now that num_channels and get_int_channel are const, so I don't see why mc_has_channels variable is needed. Use mc->soc->num_channels.
if (mc->soc->num_channels) {
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 20 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
19.03.2022 18:50, Dmitry Osipenko пишет:
Quoted 7 lines
Quoted 7 lines
drivers/memory/tegra/mc.c +7
-0+ mc_has_channels = mc->soc->num_channels && mc->soc->get_int_channel; +if (mc_has_channels) { +int err; + + err = mc->soc->get_int_channel (mc, &channel); +if (err <0 ) +return IRQ_NONE ;Quoted 8 lines
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 537
Quoted 6 lines
drivers/memory/tegra/mc.c +6
-0+ +/* mask all interrupts to avoid flooding */ + status =mc_ch_readl (mc, channel,MC_INTSTATUS ) & mc->soc->intmask; + }else { + status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask; + }So if mc_has_channels=false, while it should be true, then you're going to handle interrupt incorrectly?
Quoted 4 lines
I see now that
num_channelsandget_int_channelare const, so I don't see whymc_has_channelsvariable is needed. Usemc->soc->num_channels.if (
mc->soc->num_channels) {
Okay, I will remove mc_has_channels and replace it with mc->soc->num_channels.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
In reply to Dmitry OsipenkoQuoted 12 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 523
Quoted 7 lines
drivers/memory/tegra/mc.c +3
-0+irqreturn_t tegra30_mc_handle_irq (int irq,void *data) {struct tegra_mc *mc = data;unsigned long status; +bool mc_has_channels;unsigned int bit; +int channel;
Quoted 2 lines
unsigned int
Okay, I will update in next version.
Quoted 8 lines
Quoted 8 lines
drivers/memory/tegra/mc.c +8
-0+ mc_has_channels = mc->soc->num_channels && mc->soc->get_int_channel; +if (mc_has_channels) { +int err; + + err = mc->soc->get_int_channel (mc, &channel); +if (err <0 ) +return IRQ_NONE ; +
Quoted 8 lines
drivers/memory/tegra/mc.c int tegra30_mc_probe(struct tegra_mc *mc) · line 537
Quoted 5 lines
drivers/memory/tegra/mc.c +5
-0+/* mask all interrupts to avoid flooding */ + status =mc_ch_readl (mc, channel,MC_INTSTATUS ) & mc->soc->intmask; + }else { + status =mc_readl (mc,MC_INTSTATUS ) & mc->soc->intmask; + }So if mc_has_channels=false, while it should be true, then you're going to handle interrupt incorrectly?
I am not able to understand the case where this can happen? num_channels and get_int_channels are both getting defined on T186 onwards where mc_has_channels is expected to be true. Do you mean to say that we need to add more chip specific checks in case of mc_has_channels is false?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
In reply to Ashish Mhetre16.03.2022 12:25, Ashish Mhetre пишет:
include/soc/tegra/mc.h struct tegra_mc_soc { · line 206
Quoted 9 lines
include/soc/tegra/mc.h +1
-0@@ -203,6 +203,8 @@ struct tegra_mc_soc {const struct tegra_smmu_soc *smmu;u32 intmask; +u32 int_channel_mask;
ch_intmask
include/soc/tegra/mc.h struct tegra_mc_soc { · line 216
Quoted 10 lines
include/soc/tegra/mc.h +3
-0+bool has_addr_hi_reg;const struct tegra_mc_reset_ops *reset_ops;const struct tegra_mc_reset *resets;@@ -210,6 +212,8 @@ struct tegra_mc_soc {const struct tegra_mc_icc_ops *icc_ops;const struct tegra_mc_ops *ops; + +int (*get_int_channel)(const struct tegra_mc *mc,int *mc_channel);
This should be a part of tegra_mc_ops.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 7 lines
include/soc/tegra/mc.h +0
-0@@ -203,6 +203,8 @@ struct tegra_mc_soc {const struct tegra_smmu_soc *smmu;
Quoted 5 lines
include/soc/tegra/mc.h struct tegra_mc_soc { · line 206
Quoted 2 lines
include/soc/tegra/mc.h +1
-0u32 intmask; +u32 int_channel_mask;
ch_intmask
Okay, I will update,
Quoted 6 lines
Quoted 6 lines
include/soc/tegra/mc.h +1
-0+bool has_addr_hi_reg;const struct tegra_mc_reset_ops *reset_ops;const struct tegra_mc_reset *resets;@@ -210,6 +212,8 @@ struct tegra_mc_soc {
Quoted 6 lines
include/soc/tegra/mc.h struct tegra_mc_soc { · line 216
Quoted 4 lines
include/soc/tegra/mc.h +2
-0const struct tegra_mc_icc_ops *icc_ops;const struct tegra_mc_ops *ops; + +int (*get_int_channel)(const struct tegra_mc *mc,int *mc_channel);This should be a part of
tegra_mc_ops.
tegra_mc_ops is common for T186, T194 and T234 i.e. all of them use tegra186_mc_ops. get_int_channel function has to be differently implemented for all of these SOCs. So I had put it in tegra_mc_soc.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 35 lines · Ashish Mhetre
Quoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 7 lines
include/soc/tegra/mc.h +0
-0@@ -203,6 +203,8 @@ struct tegra_mc_soc { const struct tegra_smmu_soc *smmu;Quoted 5 lines
include/soc/tegra/mc.h struct tegra_mc_soc { · line 206
Quoted 2 lines
include/soc/tegra/mc.h +1
-0u32 intmask; + u32 int_channel_mask;
ch_intmaskOkay, I will update,
Quoted 6 lines
Quoted 6 lines
include/soc/tegra/mc.h +1
-0+ bool has_addr_hi_reg; const struct tegra_mc_reset_ops *reset_ops; const struct tegra_mc_reset *resets;@@ -210,6 +212,8 @@ struct tegra_mc_soc {Quoted 7 lines
include/soc/tegra/mc.h struct tegra_mc_soc {
Quoted 5 lines
include/soc/tegra/mc.h +2
-0const struct tegra_mc_icc_ops *icc_ops; const struct tegra_mc_ops *ops; + + int (*get_int_channel)(const struct tegra_mc *mc, int *mc_channel);This should be a part of
tegra_mc_ops.
Quoted 3 lines
tegra_mc_opsis common for T186, T194 and T234 i.e. all of them usetegra186_mc_ops.get_int_channelfunction has to be differently implemented for all of these SOCs. So I had put it integra_mc_soc.
Then tegra_mc_ops shouldn't be common anymore?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 39 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 35 lines · Ashish Mhetre
Quoted 11 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 7 lines
include/soc/tegra/mc.h +0
-0@@ -203,6 +203,8 @@ struct tegra_mc_soc {const struct tegra_smmu_soc *smmu;Quoted 5 lines
include/soc/tegra/mc.h struct tegra_mc_soc { · line 206
Quoted 2 lines
include/soc/tegra/mc.h +1
-0u32 intmask; +u32 int_channel_mask;
ch_intmaskOkay, I will update,
Quoted 13 lines
include/soc/tegra/mc.h struct tegra_mc_soc { · line 217
Quoted 11 lines
include/soc/tegra/mc.h +3
-0+bool has_addr_hi_reg;const struct tegra_mc_reset_ops *reset_ops;const struct tegra_mc_reset *resets;@@ -210,6 +212,8 @@ struct tegra_mc_soc {const struct tegra_mc_icc_ops *icc_ops;const struct tegra_mc_ops *ops; + +int (*get_int_channel)(const struct tegra_mc *mc,int *mc_channel);This should be a part of
tegra_mc_ops.
Quoted 5 lines
Quoted 3 lines
tegra_mc_opsis common for T186, T194 and T234 i.e. all of them usetegra186_mc_ops.get_int_channelfunction has to be differently implemented for all of these SOCs. So I had put it integra_mc_soc.Then
tegra_mc_opsshouldn't be common anymore?
Yes, that can be done. But the tegra186_mc_ops functions are common for Tegra186, Tegra194 and Tegra234.
We can separate tegra_mc_ops and keep the callbacks to same tegra186 functions by removing static from them.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
In reply to Ashish Mhetre16.03.2022 12:25, Ashish Mhetre пишет:
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 932
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+static int tegra186_mc_get_channel (const struct tegra_mc *mc,int *mc_channel) +{ +u32 status; + + status =mc_ch_readl (mc,MC_BROADCAST_CHANNEL ,MC_GLOBAL_INTSTATUS );
This mc_ch_readl(MC_GLOBAL_INTSTATUS) is replicated by every tegraxxx_mc_get_channel(), it should be a part of common interrupt handler, IMO.
And then I'd rename that callback to global_intstatus_to_channel().
Quoted 13 lines
drivers/memory/tegra/tegra186.c +13
-0+switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ): + *mc_channel =1 ; +break ; + +case BIT (2 ): + *mc_channel =2 ; +break ; +
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 956
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+case BIT (3 ): + *mc_channel =3 ; +break ; + +case BIT (24 ): + *mc_channel =MC_BROADCAST_CHANNEL ; +break ; + +default : +pr_err ("Unknown interrupt source\n" );
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status) and should be moved to the common interrupt handler.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 14 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 932
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+static int tegra186_mc_get_channel (const struct tegra_mc *mc,int *mc_channel) +{ +u32 status; + + status =mc_ch_readl (mc,MC_BROADCAST_CHANNEL ,MC_GLOBAL_INTSTATUS );This
mc_ch_readl(MC_GLOBAL_INTSTATUS)is replicated by everytegraxxx_mc_get_channel(), it should be a part of common interrupt handler, IMO.
Quoted 2 lines
And then I'd rename that callback to
global_intstatus_to_channel().
Okay, I'll do that in next version.
Quoted 18 lines
Quoted 18 lines
drivers/memory/tegra/tegra186.c +18
-0+switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ): + *mc_channel =1 ; +break ; + +case BIT (2 ): + *mc_channel =2 ; +break ; + +case BIT (3 ): + *mc_channel =3 ; +break ; + +case BIT (24 ):
Quoted 9 lines
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 956
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+ *mc_channel =MC_BROADCAST_CHANNEL ; +break ; + +default : +pr_err ("Unknown interrupt source\n" );
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status)and should be moved to the common interrupt handler.
So return just error from default case and handle error in common interrupt handler with this print, right? I'll update this in next version.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 27 lines · Ashish Mhetre
Quoted 18 lines
Quoted 18 lines
+ switch (status &
mc->soc->int_channel_mask) { + case BIT(0):
+*mc_channel= 0;
+ break;drivers/memory/tegra/tegra186.c +14
-0+ + case BIT(1): + *mc_channel = 1; + break; + + case BIT(2): + *mc_channel = 2; + break; + + case BIT(3): + *mc_channel = 3; + break; + + case BIT(24):Quoted 9 lines
Quoted 5 lines
+
*mc_channel=MC_BROADCAST_CHANNEL; + break;
+
+ default:
+pr_err("Unknown interrupt source\n");
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status)and should be moved to the common interrupt handler.
Quoted 3 lines
So return just error from default case and handle error in common interrupt handler with this print, right? I'll update this in next version.
Yes, just move out the common print.
Although, you could parameterize the shift per SoC and then have a common helper that does "status >> intmask_chan_shift", couldn't you?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 31 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 27 lines · Ashish Mhetre
Quoted 27 lines
Quoted 13 lines
drivers/memory/tegra/tegra186.c +13
-0+switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ): + *mc_channel =1 ; +break ; + +case BIT (2 ): + *mc_channel =2 ; +break ; +drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 956
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+case BIT (3 ): + *mc_channel =3 ; +break ; + +case BIT (24 ): + *mc_channel =MC_BROADCAST_CHANNEL ; +break ; + +default : +pr_err ("Unknown interrupt source\n" );
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status)and should be moved to the common interrupt handler.
Quoted 8 lines
Quoted 3 lines
So return just error from default case and handle error in common interrupt handler with this print, right? I'll update this in next version.
Yes, just move out the common print.
Although, you could parameterize the shift per SoC and then have a common helper that does "status >>
intmask_chan_shift", couldn't you?
Do you mean shift to get the channel, like
"channel = status >> intmask_chan_shift"?
So to get rid of this callback completely and adding a variable in tegra_mc_soc for intmask_chan_shift, right? Or compute shift in this callback and use it in common handler?
If we are to remove this callback then how to handle unknown interrupt channel error?
Also we want to handle interrupts on one channel at a time and then clear it from status register. There can be interrupts on multiple channel. So multiple bits from status will be set. Hence it will be hard to parameterize shift such that it gives appropriate channel. So I think current approach is fine. Please correct me if I am wrong somewhere.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 40 lines · Ashish Mhetre
Quoted 37 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 27 lines · Ashish Mhetre
Quoted 18 lines
Quoted 18 lines
+ switch (status &
mc->soc->int_channel_mask) { + case BIT(0):
+*mc_channel= 0;
+ break;drivers/memory/tegra/tegra186.c +14
-0+ + case BIT(1): + *mc_channel = 1; + break; + + case BIT(2): + *mc_channel = 2; + break; + + case BIT(3): + *mc_channel = 3; + break; + + case BIT(24):Quoted 9 lines
Quoted 5 lines
+
*mc_channel=MC_BROADCAST_CHANNEL; + break;
+
+ default:
+pr_err("Unknown interrupt source\n");
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status)and should be moved to the common interrupt handler.Quoted 3 lines
So return just error from default case and handle error in common interrupt handler with this print, right? I'll update this in next version.
Yes, just move out the common print.
Quoted 8 lines
Quoted 2 lines
Although, you could parameterize the shift per SoC and then have a common helper that does "status >>
intmask_chan_shift", couldn't you?Do you mean shift to get the channel, like
"channel = status >>intmask_chan_shift"?
So to get rid of this callback completely and adding a variable integra_mc_socforintmask_chan_shift, right? Or compute shift in this callback and use it in common handler?
Add variable to tegra_mc_soc.
The intmask_chan_shift is a misnomer, perhaps something like status_reg_chan_shift will be a better name.
Quoted 2 lines
If we are to remove this callback then how to handle unknown interrupt channel error?
Create a common helper function that returns ID of the raised channel or errorno if not bits are set.
Quoted 6 lines
Also we want to handle interrupts on one channel at a time and then clear it from status register. There can be interrupts on multiple channel. So multiple bits from status will be set. Hence it will be hard to parameterize shift such that it gives appropriate channel. So I think current approach is fine. Please correct me if I am wrong somewhere.
You may do the following:
1. find the first channel bit set in the status reg
2. handle that channel
3. clear only the handled status bit, don't clear the other bits
4. return from interrupt
If there are other bits set, then interrupt handler will fire again and next channel will be handled.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 53 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 40 lines · Ashish Mhetre
Quoted 37 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 27 lines · Ashish Mhetre
Quoted 18 lines
Quoted 18 lines
drivers/memory/tegra/tegra186.c +18
-0+switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ): + *mc_channel =1 ; +break ; + +case BIT (2 ): + *mc_channel =2 ; +break ; + +case BIT (3 ): + *mc_channel =3 ; +break ; + +case BIT (24 ):Quoted 9 lines
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 956
Quoted 5 lines
drivers/memory/tegra/tegra186.c +5
-0+ *mc_channel =MC_BROADCAST_CHANNEL ; +break ; + +default : +pr_err ("Unknown interrupt source\n" );
dev_err_ratelimited("unknown interrupt channel 0x%08x\n", status)and should be moved to the common interrupt handler.Quoted 3 lines
So return just error from default case and handle error in common interrupt handler with this print, right? I'll update this in next version.
Yes, just move out the common print.
Quoted 8 lines
Quoted 2 lines
Although, you could parameterize the shift per SoC and then have a common helper that does "status >>
intmask_chan_shift", couldn't you?Do you mean shift to get the channel, like
"channel = status >>intmask_chan_shift"?
So to get rid of this callback completely and adding a variable integra_mc_socforintmask_chan_shift, right? Or compute shift in this callback and use it in common handler?
Quoted 5 lines
Add variable to
tegra_mc_soc.The
intmask_chan_shiftis a misnomer, perhaps something likestatus_reg_chan_shiftwill be a better name.
Okay, I will do this.
Quoted 6 lines
Quoted 2 lines
If we are to remove this callback then how to handle unknown interrupt channel error?
Create a common helper function that returns ID of the raised channel or errorno if not bits are set.
So something like this:
int status_to_channel(const struct tegra_mc *mc, u32 status,
unsigned int *mc_channel)
{
if ((status & mc->soc->ch_intmask) == 0)
return -EINVAL;
*mc_channel = __ffs((status & mc->soc->ch_intmask) >>
mc->soc->status_reg_chan_shift);
return 0;
}Correct?
Quoted 9 lines
Quoted 6 lines
Also we want to handle interrupts on one channel at a time and then clear it from status register. There can be interrupts on multiple channel. So multiple bits from status will be set. Hence it will be hard to parameterize shift such that it gives appropriate channel. So I think current approach is fine. Please correct me if I am wrong somewhere.
You may do the following:
Quoted 7 lines
1. find the first channel bit set in the status reg
2. handle that channel
3. clear only the handled status bit, don't clear the other bits
4. return from interruptIf there are other bits set, then interrupt handler will fire again and next channel will be handled.
For clearing status bit after handling, we can retrieve channel bit by something like this:
ch_bit = BIT(*mc_channel) << mc->soc->status_reg_chan_shift;
Correct?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
On 3/30/22 14:22, Ashish Mhetre wrote:
...
Quoted 14 lines
Quoted 6 lines
Quoted 2 lines
If we are to remove this callback then how to handle unknown interrupt channel error?
Create a common helper function that returns ID of the raised channel or errorno if not bits are set.
So something like this:
int
status_to_channel(const struct tegra_mc *mc, u32 status, unsigned int *mc_channel)
{
if ((status &mc->soc->ch_intmask) == 0) return-EINVAL;
Quoted 7 lines
*mc_channel =
__ffs((status & mc->soc->ch_intmask) >> mc->soc->status_reg_chan_shift);return 0;
}Correct?
Yes
Quoted 17 lines
Quoted 9 lines
Quoted 6 lines
Also we want to handle interrupts on one channel at a time and then clear it from status register. There can be interrupts on multiple channel. So multiple bits from status will be set. Hence it will be hard to parameterize shift such that it gives appropriate channel. So I think current approach is fine. Please correct me if I am wrong somewhere.
You may do the following:
Quoted 7 lines
1. find the first channel bit set in the status reg
2. handle that channel
3. clear only the handled status bit, don't clear the other bits
4. return from interruptIf there are other bits set, then interrupt handler will fire again and next channel will be handled.
Quoted 6 lines
For clearing status bit after handling, we can retrieve channel bit by something like this:
ch_bit=BIT(*mc_channel)<<mc->soc->status_reg_chan_shift;Correct?
Yes
Perhaps using FIELD_PREP() and alike helpers could make it look nice in the code.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 49 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
On 3/30/22 14:22, Ashish Mhetre wrote:
...Quoted 20 lines
Quoted 6 lines
Quoted 2 lines
If we are to remove this callback then how to handle unknown interrupt channel error?
Create a common helper function that returns ID of the raised channel or errorno if not bits are set.
So something like this:
int status_to_channel(const struct
tegra_mc*mc, u32 status,
unsigned int*mc_channel){ if ((status & mc->soc->ch_intmask) == 0) return -EINVAL; *mc_channel = __ffs((status & mc->soc->ch_intmask) >> mc->soc->status_reg_chan_shift); return 0; }Quoted 1 line
Correct?
Yes
Quoted 20 lines
Quoted 9 lines
Quoted 6 lines
Also we want to handle interrupts on one channel at a time and then clear it from status register. There can be interrupts on multiple channel. So multiple bits from status will be set. Hence it will be hard to parameterize shift such that it gives appropriate channel. So I think current approach is fine. Please correct me if I am wrong somewhere.
You may do the following:
Quoted 7 lines
1. find the first channel bit set in the status reg
2. handle that channel
3. clear only the handled status bit, don't clear the other bits
4. return from interruptIf there are other bits set, then interrupt handler will fire again and next channel will be handled.
For clearing status bit after handling, we can retrieve channel bit by something like this:
Quoted 8 lines
Quoted 3 lines
ch_bit=BIT(*mc_channel)<<mc->soc->status_reg_chan_shift;Correct?
Yes
Perhaps using
FIELD_PREP()and alike helpers could make it look nice in the code.
I tried using FIELD_PREP() and FIELD_GET() for our use-case but
compilation is failing because these macros require the mask to be compile time constant and our mask "mc->soc->ch_intmask" cannot qualify to be compile time constant.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
In reply to Ashish Mhetre16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 12 lines
drivers/memory/tegra/tegra186.c +12
-0+static int tegra186_mc_get_channel (const struct tegra_mc *mc,int *mc_channel) +{ +u32 status; + + status =mc_ch_readl (mc,MC_BROADCAST_CHANNEL ,MC_GLOBAL_INTSTATUS ); + +switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ):
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 949
Quoted 10 lines
drivers/memory/tegra/tegra186.c +10
-0+ *mc_channel =1 ; +break ; + +case BIT (2 ): + *mc_channel =2 ; +break ; + +case BIT (3 ): + *mc_channel =3 ; +break ;
This won't work if multiple bits are set at once.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 20 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 16 lines
drivers/memory/tegra/tegra186.c +16
-0+static int tegra186_mc_get_channel (const struct tegra_mc *mc,int *mc_channel) +{ +u32 status; + + status =mc_ch_readl (mc,MC_BROADCAST_CHANNEL ,MC_GLOBAL_INTSTATUS ); + +switch (status & mc->soc->int_channel_mask) { +case BIT (0 ): + *mc_channel =0 ; +break ; + +case BIT (1 ): + *mc_channel =1 ; +break ; + +case BIT (2 ):
Quoted 8 lines
drivers/memory/tegra/tegra186.c static const struct tegra_mc_client tegra186_mc_clients[] = { · line 949
Quoted 6 lines
drivers/memory/tegra/tegra186.c +6
-0+ *mc_channel =2 ; +break ; + +case BIT (3 ): + *mc_channel =3 ; +break ;This won't work if multiple bits are set at once.
I talked with our HW team and they said that it's technically possible that interrupts can come at multiple channels at same time. SW can take care of this by logging interrupts at first channel and then clearing bit of that. Then take care of interrupts from next channel and so on. I'll update the patches accordingly in next version.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
In reply to Ashish MhetreQuoted 17 lines · Ashish Mhetre
Add new function '
get_int_channel' integra_mc_socstruture which is implemented by tegra SOCs which support multiple MC channels. This function returns the channel which should be used to get the information of interrupts.
Remove static fromtegra30_mc_handle_irqand use it as interrupt handler for MC interrupts on tegra186, tegra194 and tegra234 to log the errors. Add error specific MC status and address register bits and use them on tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194 and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234. Add register for higher bits of error address which is available on tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' integra_mc_socstruture which will be true if soc has register for higher bits of memory controller error address. Set it true for tegra194 and tegra234.
Quoted 4 lines
Signed-off-by Ashish Mhetre <[email protected]>Reported-by kernel test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>
Reported what? You should add this tag only if patch addresses reported problem. This patch doesn't address anything, hence the tag is inappropriate, you should remove it.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 21 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 17 lines · Ashish Mhetre
Add new function '
get_int_channel' integra_mc_socstruture which is implemented by tegra SOCs which support multiple MC channels. This function returns the channel which should be used to get the information of interrupts.
Remove static fromtegra30_mc_handle_irqand use it as interrupt handler for MC interrupts on tegra186, tegra194 and tegra234 to log the errors. Add error specific MC status and address register bits and use them on tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194 and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234. Add register for higher bits of error address which is available on tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' integra_mc_socstruture which will be true if soc has register for higher bits of memory controller error address. Set it true for tegra194 and tegra234.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Ashish Mhetre <[email protected]>Reported-by kernel test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>Reported what? You should add this tag only if patch addresses reported problem. This patch doesn't address anything, hence the tag is inappropriate, you should remove it.
Okay, smatch warning was reported on v4 of this patch which is fixed in v5. Then I understand that we don't need to add Reported-by if we fix bug in subsequent versions, right?
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 30 lines · Ashish Mhetre
Quoted 27 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 22 lines · Ashish Mhetre
Add new function '
get_int_channel' integra_mc_socstruture which is implemented by tegra SOCs which support multiple MC channels. This function returns the channel which should be used to get the information of interrupts.
Remove static fromtegra30_mc_handle_irqand use it as interrupt handler for MC interrupts on tegra186, tegra194 and tegra234 to log the errors. Add error specific MC status and address register bits and use them on tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194
and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234. Add register for higher bits of error address which is available on tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' integra_mc_socstruture which will be true if soc has register for higher bits of memory controller error address. Set it true for tegra194 and tegra234.Signed-off-by Ashish Mhetre <[email protected]>Reported-by kernel test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>
Quoted 7 lines
Quoted 3 lines
Reported what? You should add this tag only if patch addresses reported problem. This patch doesn't address anything, hence the tag is inappropriate, you should remove it.
Okay, smatch warning was reported on v4 of this patch which is fixed in v5. Then I understand that we don't need to add Reported-by if we fix bug in subsequent versions, right?
Right, if the report was made to the in-progress patch, then you shouldn't add the tag.
If report was made to the patch that was already merged, then you should create a new patch that fixes the reported problem and add the reported-by to this patch.
Re: [Patch v5 2/4] memory: tegra: Add MC error logging on tegra186 onward
Quoted 42 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 30 lines · Ashish Mhetre
Quoted 27 lines · Dmitry Osipenko
External email: Use caution opening links or attachments
Quoted 22 lines · Ashish Mhetre
Add new function '
get_int_channel' integra_mc_socstruture which is implemented by tegra SOCs which support multiple MC channels. This function returns the channel which should be used to get the information of interrupts.
Remove static fromtegra30_mc_handle_irqand use it as interrupt handler for MC interrupts on tegra186, tegra194 and tegra234 to log the errors. Add error specific MC status and address register bits and use them on tegra186, tegra194 and tegra234.
Add error logging for generalized carveout interrupt on tegra186, tegra194
and tegra234.
Add error logging for route sanity interrupt on tegra194 an tegra234. Add register for higher bits of error address which is available on tegra194 and tegra234.
Add a boolean variable 'has_addr_hi_reg' integra_mc_socstruture which will be true if soc has register for higher bits of memory controller error address. Set it true for tegra194 and tegra234.Signed-off-by Ashish Mhetre <[email protected]>Reported-by kernel test robot <[email protected]>Reported-by Dan Carpenter <[email protected]>Quoted 7 lines
Quoted 3 lines
Reported what? You should add this tag only if patch addresses reported problem. This patch doesn't address anything, hence the tag is inappropriate, you should remove it.
Okay, smatch warning was reported on v4 of this patch which is fixed in v5. Then I understand that we don't need to add Reported-by if we fix bug in subsequent versions, right?
Quoted 6 lines
Right, if the report was made to the in-progress patch, then you shouldn't add the tag.
If report was made to the patch that was already merged, then you should create a new patch that fixes the reported problem and add the reported-by to this patch.
Got it, thanks for the explanation. I'll remove tag from next version.
PATCH v5 3/4 dt-bindings: memory: Update reg maxitems for tegra186
In reply to Ashish MhetreFrom tegra186 onwards, memory controller support multiple channels. Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation.
--- .../nvidia,tegra186-mc.yaml | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +14 -6
@@ -34,8 +34,8 @@ properties: - nvidia,tegra234-mc reg: - minItems: 1 - maxItems: 3 + minItems: 6 + maxItems: 18 interrupts: items:
@@ -142,7 +142,8 @@ allOf: then: properties: reg: - maxItems: 1 + maxItems: 6 + description: 5 memory controller channels and 1 for stream-id registers - if: properties:
@@ -151,7 +152,8 @@ allOf: then: properties: reg: - minItems: 3 + minItems: 18 + description: 17 memory controller channels and 1 for stream-id registers - if: properties:
@@ -160,7 +162,8 @@ allOf: then: properties: reg: - minItems: 3 + minItems: 18 + description: 17 memory controller channels and 1 for stream-id registers additionalProperties: false
@@ -198,7 +201,12 @@ examples: external-memory-controller@2c60000 { compatible = "nvidia,tegra186-emc"; - reg = <0x0 0x02c60000 0x0 0x50000>; + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; clocks = <&bpmp TEGRA186_CLK_EMC>; clock-names = "emc";
Signature · 1 line
2.17.1
Re: [Patch v5 3/4] dt-bindings: memory: Update reg maxitems for tegra186
16.03.2022 12:25, Ashish Mhetre пишет:
Quoted 61 lines
From tegra186 onwards, memory controller support multiple channels. Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation.Signed-off-by Ashish Mhetre <[email protected]>--- .../nvidia,tegra186-mc.yaml | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +8
-5@@ -34,8 +34,8 @@ properties: - nvidia,tegra234-mc reg: - minItems: 1 - maxItems: 3 + minItems: 6 + maxItems: 18 interrupts: items:@@ -142,7 +142,8 @@ allOf: then: properties: reg: - maxItems: 1 + maxItems: 6 + description: 5 memory controller channels and 1 for stream-id registers - if: properties:@@ -151,7 +152,8 @@ allOf: then: properties: reg: - minItems: 3 + minItems: 18 + description: 17 memory controller channels and 1 for stream-id registers - if: properties:@@ -160,7 +162,8 @@ allOf: then: properties: reg: - minItems: 3 + minItems: 18 + description: 17 memory controller channels and 1 for stream-id registers additionalProperties: false@@ -198,7 +201,12 @@ examples: external-memory-controller@2c60000 { compatible = "nvidia,tegra186-emc";
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml examples: · line 212
Quoted 10 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +6
-1- reg = <0x0 0x02c60000 0x0 0x50000>; + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ interrupts = <GIC_SPI 224 IRQ_TYPE_LEVEL_HIGH>; clocks = <&bpmp TEGRA186_CLK_EMC>; clock-names = "emc";
This is the EMC node, not MC.
Rob Herring
maintainerRe: [Patch v5 3/4] dt-bindings: memory: Update reg maxitems for tegra186
In reply to Ashish MhetreQuoted 1 line · Ashish Mhetre
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.example.dt.yaml: memory-controller@2c00000: reg: [[0, 46137344, 0, 720896]] is too short
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.example.dt.yaml: memory-controller@2c00000: external-memory-controller@2c60000:reg: [[0, 46137344, 0, 65536], [0, 46202880, 0, 65536], [0, 46268416, 0, 65536], [0, 46333952, 0, 65536], [0, 46399488, 0, 65536], [0, 46465024, 0, 65536]] is too long
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.example.dt.yaml: memory-controller@2c00000: external-memory-controller@2c60000:reg: [[0, 46137344, 0, 65536], [0, 46202880, 0, 65536], [0, 46268416, 0, 65536], [0, 46333952, 0, 65536], [0, 46399488, 0, 65536], [0, 46465024, 0, 65536]] is too long
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.example.dt.yaml: memory-controller@2c00000: reg: [[0, 46137344, 0, 720896]] is too short
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1606062
This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit.
Re: [Patch v5 3/4] dt-bindings: memory: Update reg maxitems for tegra186
In reply to Ashish MhetreQuoted 17 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels. Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation.Signed-off-by Ashish Mhetre <[email protected]>--- .../nvidia,tegra186-mc.yaml | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0
-0
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 38
Quoted 7 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +2
-2@@ -34,8 +34,8 @@ properties: - nvidia,tegra234-mc reg: - minItems: 1 - maxItems: 3 + minItems: 6 + maxItems: 18
Still ABI break and now the in-kernel DTS will report dt check errors.
I think you ignored the comments you got about breaking ABI.
Best regards,
Krzysztof
Re: [Patch v5 3/4] dt-bindings: memory: Update reg maxitems for tegra186
Quoted 23 lines · Krzysztof Kozlowski
External email: Use caution opening links or attachments
Quoted 19 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels.
Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation.Signed-off-by Ashish Mhetre <[email protected]>--- .../nvidia,tegra186-mc.yaml | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0
-0@@ -34,8 +34,8 @@ properties: - nvidia,tegra234-mc
Quoted 8 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 38
Quoted 5 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +2
-2reg: - minItems: 1 - maxItems: 3 + minItems: 6 + maxItems: 18Still ABI break and now the in-kernel DTS will report dt check errors.
The dt check error is because I mistakenly updated example in EMC node instead of MC. I'll fix it in next version.
Quoted 2 lines
I think you ignored the comments you got about breaking ABI.
No, I took care of the ABI break in v5. I have updated details about how we took care of it in first patch.
Quoted 2 lines
Best regards,
Krzysztof
Krzysztof Kozlowski
maintainerRe: [Patch v5 3/4] dt-bindings: memory: Update reg maxitems for tegra186
Quoted 34 lines · Ashish Mhetre
Quoted 23 lines · Krzysztof Kozlowski
External email: Use caution opening links or attachments
Quoted 19 lines · Ashish Mhetre
From tegra186 onwards, memory controller support multiple channels.
Reg items are updated with address and size of these channels. Tegra186 has overall 5 memory controller channels. Tegra194 and tegra234 have overall 17 memory controller channels each.
There is 1 reg item for memory controller stream-id registers. So update the reg maxItems to 18 in tegra186 devicetree documentation.Signed-off-by Ashish Mhetre <[email protected]>--- .../nvidia,tegra186-mc.yaml | 20 +++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +0
-0@@ -34,8 +34,8 @@ properties: - nvidia,tegra234-mcQuoted 8 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml properties: · line 38
Quoted 5 lines
Documentation/devicetree/bindings/memory-controllers/nvidia,tegra186-mc.yaml +2
-2reg: - minItems: 1 - maxItems: 3 + minItems: 6 + maxItems: 18Still ABI break and now the in-kernel DTS will report dt check errors.
Quoted 2 lines
The dt check error is because I mistakenly updated example in EMC node instead of MC. I'll fix it in next version.
The existing DTS will start failing with:
nvidia,tegra186-mc.example.dtb: memory-controller@2c00000: reg: [[0, 46137344, 0, 720896]] is too short
because you require now length of 6 minimum.
Quoted 5 lines
Quoted 3 lines
I think you ignored the comments you got about breaking ABI.
No, I took care of the ABI break in v5. I have updated details about how we took care of it in first patch.
Right, driver part looks ok.
Best regards,
Krzysztof
PATCH v5 4/4 arm64: tegra: Add memory controller channels
In reply to Ashish MhetreFrom tegra186 onwards, memory controller support multiple channels. During the error interrupts from memory controller, corresponding channels need to be accessed for logging error info and clearing the interrupt.
So add address and size of these channels in device tree node of tegra186, tegra194 and tegra234 memory controller.
--- arch/arm64/boot/dts/nvidia/tegra186.dtsi | 7 ++++++- arch/arm64/boot/dts/nvidia/tegra194.dtsi | 21 ++++++++++++++++++--- arch/arm64/boot/dts/nvidia/tegra234.dtsi | 21 ++++++++++++++++++--- 3 files changed, 42 insertions(+), 7 deletions(-)
arch/arm64/boot/dts/nvidia/tegra186.dtsi +6 -1
@@ -521,7 +521,12 @@ mc: memory-controller@2c00000 { compatible = "nvidia,tegra186-mc"; - reg = <0x0 0x02c00000 0x0 0xb0000>; + reg = <0x0 0x02c00000 0x0 0x10000>, /* MC-SID */ + <0x0 0x02c10000 0x0 0x10000>, /* Broadcast channel */ + <0x0 0x02c20000 0x0 0x10000>, /* MC0 */ + <0x0 0x02c30000 0x0 0x10000>, /* MC1 */ + <0x0 0x02c40000 0x0 0x10000>, /* MC2 */ + <0x0 0x02c50000 0x0 0x10000>; /* MC3 */ interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; status = "disabled";
arch/arm64/boot/dts/nvidia/tegra194.dtsi +18 -3
@@ -604,9 +604,24 @@ mc: memory-controller@2c00000 { compatible = "nvidia,tegra194-mc"; - reg = <0x02c00000 0x100000>, - <0x02b80000 0x040000>, - <0x01700000 0x100000>; + reg = <0x02c00000 0x10000>, /* MC-SID */ + <0x02c10000 0x10000>, /* MC Broadcast*/ + <0x02c20000 0x10000>, /* MC0 */ + <0x02c30000 0x10000>, /* MC1 */ + <0x02c40000 0x10000>, /* MC2 */ + <0x02c50000 0x10000>, /* MC3 */ + <0x02b80000 0x10000>, /* MC4 */ + <0x02b90000 0x10000>, /* MC5 */ + <0x02ba0000 0x10000>, /* MC6 */ + <0x02bb0000 0x10000>, /* MC7 */ + <0x01700000 0x10000>, /* MC8 */ + <0x01710000 0x10000>, /* MC9 */ + <0x01720000 0x10000>, /* MC10 */ + <0x01730000 0x10000>, /* MC11 */ + <0x01740000 0x10000>, /* MC12 */ + <0x01750000 0x10000>, /* MC13 */ + <0x01760000 0x10000>, /* MC14 */ + <0x01770000 0x10000>; /* MC15 */ interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; #interconnect-cells = <1>; status = "disabled";
arch/arm64/boot/dts/nvidia/tegra234.dtsi +18 -3
@@ -507,9 +507,24 @@ mc: memory-controller@2c00000 { compatible = "nvidia,tegra234-mc"; - reg = <0x02c00000 0x100000>, - <0x02b80000 0x040000>, - <0x01700000 0x100000>; + reg = <0x02c00000 0x10000>, /* MC-SID */ + <0x02c10000 0x10000>, /* MC Broadcast*/ + <0x02c20000 0x10000>, /* MC0 */ + <0x02c30000 0x10000>, /* MC1 */ + <0x02c40000 0x10000>, /* MC2 */ + <0x02c50000 0x10000>, /* MC3 */ + <0x02b80000 0x10000>, /* MC4 */ + <0x02b90000 0x10000>, /* MC5 */ + <0x02ba0000 0x10000>, /* MC6 */ + <0x02bb0000 0x10000>, /* MC7 */ + <0x01700000 0x10000>, /* MC8 */ + <0x01710000 0x10000>, /* MC9 */ + <0x01720000 0x10000>, /* MC10 */ + <0x01730000 0x10000>, /* MC11 */ + <0x01740000 0x10000>, /* MC12 */ + <0x01750000 0x10000>, /* MC13 */ + <0x01760000 0x10000>, /* MC14 */ + <0x01770000 0x10000>; /* MC15 */ interrupts = <GIC_SPI 223 IRQ_TYPE_LEVEL_HIGH>; #interconnect-cells = <1>; status = "okay";
Signature · 1 line
2.17.1