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(+)
diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
index bf3abb6d8354..3cda1d9ad32a 100644
--- a/drivers/memory/tegra/mc.c
+++ b/drivers/memory/tegra/mc.c
@@ -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) {
diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
index 3d153881abc1..a8a45e6ff1f1 100644
--- a/drivers/memory/tegra/tegra186.c
+++ b/drivers/memory/tegra/tegra186.c
@@ -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 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");
+ return 0;
+ }
+
+ mc->mcb_regs = devm_platform_get_and_ioremap_resource(pdev, 1, &res);
+ if (IS_ERR(mc->mcb_regs))
+ return PTR_ERR(mc->mcb_regs);
+
+ for (i = 0; i < mc->soc->num_channels; i++) {
+ mc->mc_regs[i] = devm_platform_get_and_ioremap_resource(pdev, i + 2, &res);
+ if (IS_ERR(mc->mc_regs[i]))
+ return PTR_ERR(mc->mc_regs[i]);
+ }
+
+ return 0;
+}
+
const struct tegra_mc_ops tegra186_mc_ops = {
.probe = tegra186_mc_probe,
.remove = tegra186_mc_remove,
.resume = tegra186_mc_resume,
.probe_device = tegra186_mc_probe_device,
+ .map_regs = tegra186_mc_map_regs,
};
#if defined(CONFIG_ARCH_TEGRA_186_SOC)
@@ -875,6 +926,7 @@ const struct tegra_mc_soc tegra186_mc_soc = {
.num_clients = ARRAY_SIZE(tegra186_mc_clients),
.clients = tegra186_mc_clients,
.num_address_bits = 40,
+ .num_channels = 4,
.ops = &tegra186_mc_ops,
};
#endif
diff --git a/drivers/memory/tegra/tegra194.c b/drivers/memory/tegra/tegra194.c
index cab998b8bd5c..94001174deaf 100644
--- a/drivers/memory/tegra/tegra194.c
+++ b/drivers/memory/tegra/tegra194.c
@@ -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,
};
diff --git a/drivers/memory/tegra/tegra234.c b/drivers/memory/tegra/tegra234.c
index e22824a79f45..6335a132be2d 100644
--- a/drivers/memory/tegra/tegra234.c
+++ b/drivers/memory/tegra/tegra234.c
@@ -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,
};
diff --git a/include/soc/tegra/mc.h b/include/soc/tegra/mc.h
index 1066b1194a5a..92f810c55b43 100644
--- a/include/soc/tegra/mc.h
+++ b/include/soc/tegra/mc.h
@@ -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 16
struct 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;
--
2.17.1
16.03.2022 12:25, 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(+)
>
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index bf3abb6d8354..3cda1d9ad32a 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -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) {
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 3d153881abc1..a8a45e6ff1f1 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -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 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?
> + 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()?
...
> @@ -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/ ?
On 16/03/2022 10:25, Ashish Mhetre wrote:
> 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(+)
>
> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
> index bf3abb6d8354..3cda1d9ad32a 100644
> --- a/drivers/memory/tegra/mc.c
> +++ b/drivers/memory/tegra/mc.c
> @@ -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) {
> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
> index 3d153881abc1..a8a45e6ff1f1 100644
> --- a/drivers/memory/tegra/tegra186.c
> +++ b/drivers/memory/tegra/tegra186.c
> @@ -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 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
On 3/20/2022 6:01 PM, Krzysztof Kozlowski wrote:
> External email: Use caution opening links or attachments
>
>
> On 16/03/2022 10:25, Ashish Mhetre wrote:
>> 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(+)
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index bf3abb6d8354..3cda1d9ad32a 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -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) {
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 3d153881abc1..a8a45e6ff1f1 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -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 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.
> Best regards,
> Krzysztof
On 22/03/2022 19:04, Ashish Mhetre wrote:
>
>
> On 3/20/2022 6:01 PM, Krzysztof Kozlowski wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On 16/03/2022 10:25, Ashish Mhetre wrote:
>>> 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(+)
>>>
>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> index bf3abb6d8354..3cda1d9ad32a 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -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) {
>>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>>> index 3d153881abc1..a8a45e6ff1f1 100644
>>> --- a/drivers/memory/tegra/tegra186.c
>>> +++ b/drivers/memory/tegra/tegra186.c
>>> @@ -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 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.
Ah, right, thanks. I missed the return 0. Looks good, thanks for the
changes and for explanation.
Best regards,
Krzysztof
On 3/19/2022 9:12 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 16.03.2022 12:25, 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(+)
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index bf3abb6d8354..3cda1d9ad32a 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -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) {
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 3d153881abc1..a8a45e6ff1f1 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -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 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.
>> + 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()?
>
> ...
>> @@ -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.
On 3/19/2022 9:12 PM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> 16.03.2022 12:25, 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(+)
>>
>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>> index bf3abb6d8354..3cda1d9ad32a 100644
>> --- a/drivers/memory/tegra/mc.c
>> +++ b/drivers/memory/tegra/mc.c
>> @@ -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) {
>> diff --git a/drivers/memory/tegra/tegra186.c b/drivers/memory/tegra/tegra186.c
>> index 3d153881abc1..a8a45e6ff1f1 100644
>> --- a/drivers/memory/tegra/tegra186.c
>> +++ b/drivers/memory/tegra/tegra186.c
>> @@ -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 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?
>
>> + 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.
> ...
>> @@ -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/ ?
On 3/25/22 07:50, Ashish Mhetre wrote:
>
>
> On 3/19/2022 9:12 PM, Dmitry Osipenko wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> 16.03.2022 12:25, 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(+)
>>>
>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>> index bf3abb6d8354..3cda1d9ad32a 100644
>>> --- a/drivers/memory/tegra/mc.c
>>> +++ b/drivers/memory/tegra/mc.c
>>> @@ -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) {
>>> diff --git a/drivers/memory/tegra/tegra186.c
>>> b/drivers/memory/tegra/tegra186.c
>>> index 3d153881abc1..a8a45e6ff1f1 100644
>>> --- a/drivers/memory/tegra/tegra186.c
>>> +++ b/drivers/memory/tegra/tegra186.c
>>> @@ -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
>>> 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?
>>
>>> + 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.
for (i = 0; i < mc->soc->num_channels; i++) {
sprintf(name, "mc%u", i);
err = devm_platform_ioremap_resource_byname(dev, name);
...
}
On 3/30/2022 5:18 AM, Dmitry Osipenko wrote:
> External email: Use caution opening links or attachments
>
>
> On 3/25/22 07:50, Ashish Mhetre wrote:
>>
>>
>> On 3/19/2022 9:12 PM, Dmitry Osipenko wrote:
>>> External email: Use caution opening links or attachments
>>>
>>>
>>> 16.03.2022 12:25, 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(+)
>>>>
>>>> diff --git a/drivers/memory/tegra/mc.c b/drivers/memory/tegra/mc.c
>>>> index bf3abb6d8354..3cda1d9ad32a 100644
>>>> --- a/drivers/memory/tegra/mc.c
>>>> +++ b/drivers/memory/tegra/mc.c
>>>> @@ -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) {
>>>> diff --git a/drivers/memory/tegra/tegra186.c
>>>> b/drivers/memory/tegra/tegra186.c
>>>> index 3d153881abc1..a8a45e6ff1f1 100644
>>>> --- a/drivers/memory/tegra/tegra186.c
>>>> +++ b/drivers/memory/tegra/tegra186.c
>>>> @@ -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
>>>> 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?
>>>
>>>> + 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.
> 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.