CHROMIUM: MALI: Add Mali platform specific source [chromiumos/third

Nick Fan (Gerrit)

unread,

Aug 24, 2018, 1:38:10 AM8/24/18

to chromium-...@chromium.org

Nick Fan has uploaded this change for review.

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_devfreq_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
6 files changed, 1,176 insertions(+), 1 deletion(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Aug 24, 2018, 1:55:23 AM8/24/18

to Nick Fan, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 1:Verified +1Code-Review +1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nicolas Boichat (Gerrit)

unread,

Aug 24, 2018, 2:32:24 AM8/24/18

to Nick Fan, Tomasz Figa, JB Tsai, Nick Fan, Eddie Huang, CK HU, Ben Ho, SJ Huang

Just a few drive-by comments, I think Tomasz had some opinion on whether we need this platform code at all.

View Change

8 comments:

File drivers/gpu/arm/midgard/backend/gpu/Kbuild:
- Patch Set #1, Line 56:
  Why? Seems a bit odd that we need to change the common Kbuild file.
  My concern here is that we have a shared kernel for multiple platforms, so if another SoC came up with a Mali GPU on kernel 4.14, we'd have issues with this change.
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_devfreq_mediatek.c:
- Patch Set #1:
  Quite a bit of this code is copied from backend/gpu/mali_kbase_devfreq.c .
- Patch Set #1, Line 27: #include <linux/pm_opp.h>
  Alphabetical order.
- Patch Set #1, Line 64:
  drop that space
- Patch Set #1, Line 81: (struct mfg_base *)
  Cast not needed, platform_context is a void *.
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #1, Line 20: #include <linux/regulator/consumer.h>
  ordering too.
- Patch Set #1, Line 45: {}
  indentation
- Patch Set #1, Line 77:
  indentation

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Aug 28, 2018, 8:47:39 AM8/28/18

to Nick Fan, Nicolas Boichat, JB Tsai, Nick Fan, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 1:
(8 comments)

Just a few drive-by comments, I think Tomasz had some opinion on whether we need this platform code at all.

Yes and I think this is confirmed by looking quickly through the code being added here. I don't see any specific handling being added, just yet another duplicate code for getting clocks, regulators and settings frequencies, invoking runtime PM, etc.

What's missing in the generic devicetree backend? Could we extend it to handle anything missing instead and have it upstreamed back to ARM?

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Aug 29, 2018, 1:12:11 AM8/29/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #3 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 778 insertions(+), 18 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Aug 29, 2018, 4:26:32 AM8/29/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 3:Verified +1Code-Review +1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Aug 29, 2018, 4:58:38 AM8/29/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 2:

Patch Set 1:
(8 comments)
Just a few drive-by comments, I think Tomasz had some opinion on whether we need this platform code at all.
Yes and I think this is confirmed by looking quickly through the code being added here. I don't see any specific handling being added, just yet another duplicate code for getting clocks, regulators and settings frequencies, invoking runtime PM, etc.
What's missing in the generic devicetree backend? Could we extend it to handle anything missing instead and have it upstreamed back to ARM?

Nick, any comments on this?

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Aug 29, 2018, 4:59:14 AM8/29/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 3:Code-Review -1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Aug 29, 2018, 5:29:29 AM8/29/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 3:

Patch Set 2:
Patch Set 1:
(8 comments)
Just a few drive-by comments, I think Tomasz had some opinion on whether we need this platform code at all.
Yes and I think this is confirmed by looking quickly through the code being added here. I don't see any specific handling being added, just yet another duplicate code for getting clocks, regulators and settings frequencies, invoking runtime PM, etc.
What's missing in the generic devicetree backend? Could we extend it to handle anything missing instead and have it upstreamed back to ARM?
Nick, any comments on this?

I have already moved the mediatek devfreq changes back to mali_kbase_devfreq.c file.
Due to GPU hardware design may not be the same, the clock, regulator and frequencies can be very different in different platforms.
In ARM DDK integration manual, you can find out that ARM suggest to copy devicetree platform directory and override the settings depends on platform needs.
I hope that answers your question.
If you have any questions or concerns, please let me know.

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Aug 29, 2018, 8:43:56 AM8/29/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 3:
Patch Set 3:
Patch Set 2:
Patch Set 1:
(8 comments)
Just a few drive-by comments, I think Tomasz had some opinion on whether we need this platform code at all.
Yes and I think this is confirmed by looking quickly through the code being added here. I don't see any specific handling being added, just yet another duplicate code for getting clocks, regulators and settings frequencies, invoking runtime PM, etc.
What's missing in the generic devicetree backend? Could we extend it to handle anything missing instead and have it upstreamed back to ARM?
Nick, any comments on this?
I have already moved the mediatek devfreq changes back to mali_kbase_devfreq.c file.
Due to GPU hardware design may not be the same, the clock, regulator and frequencies can be very different in different platforms.

That's exactly what you have DeviceTree for - to specify the platform differences there, there are generic bindings for clocks, regulators and frequencies (OPP). It works for all other drivers.

In ARM DDK integration manual, you can find out that ARM suggest to copy devicetree platform directory and override the settings depends on platform needs.
I hope that answers your question.
If you have any questions or concerns, please let me know.

Let's keep it a separate platform for now if that's the case. If we happen to add another platform to this kernel branch later, we should unify them into a generic "cros" platform, though. I'll review the code a bit later.

Patch set 3:-Code-Review

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Sep 4, 2018, 11:51:26 PM9/4/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

4 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #3, Line 134:
```
	idx = get_opp_index(mfg, freq);
	if (idx < 0) {
		dev_dbg(dev, "Opp index not found (%d)\n", idx);
		return -1;
	}

	voltage = mfg->gpu_opp_table[idx].vgpu_sram;
```
  Rather than having a custom voltage table, we should have the opp include all the regulators in question. opp->supplies[] is an array and you can have it include multiple regulators. You can then just list all the voltages in DT. Documentation/devicetree/bindings/opp/opp.txt already includes examples of that.
- Patch Set #3, Line 168: mfg->clk_main_parent
  Why don't we set kbdev->clock to this clock instead and avoid changing this?
- Patch Set #3, Line 204: mfg->current_status.gpu_freq_hz = freq;
  Why don't we just use kbdev->current_freq, which also has "freq" assigned to it?
- Patch Set #3, Line 497: kbdev->devfreq->max_freq = GPU_FREQ_KHZ_MAX * 1000;
  What is this needed for? We should have the OPPs in DT already limited to this min and max and min/max computed automatically from there.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Sep 14, 2018, 3:59:11 AM9/14/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #4 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 680 insertions(+), 13 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Sep 14, 2018, 6:09:58 AM9/14/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

27 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #4, Line 124: svoltage = ((voltage + MIN_VOLT_BIAS) >= VSRAM_GPU_MIN_VOLT) ? (voltage + MIN_VOLT_BIAS) : VSRAM_GPU_MIN_VOLT;
  I still think this should go to separate entry in opp->supplies[] and be specified in DT, because this is generic code and such computation might not apply to other SoCs.
- Patch Set #4, Line 126: mfg->vsram_gpu_regulator
  Could we instead turn kbdev->regulator into kbdev->regulators[] and have the entries match the order of opp->supplies[]? Then we could just have a loop that iterates over kbdev->num_regulators and applies matching voltages from opp->supplies[].
- Patch Set #4, Line 139: mfg->vgpu_regulator
  Why can't we keep using kbdev->regulator here?
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #4, Line 23: static struct platform_device *Probe_GPU_core2_dev;
  Linux kernel uses lowercase for variable names.
- Patch Set #4, Line 25:
```
static int mtk_gpu_core1_probe(struct platform_device *pdev)
{
	if (!pdev->dev.pm_domain)
		return -EPROBE_DEFER;

	Probe_GPU_core1_dev = pdev;

	pm_runtime_enable(&pdev->dev);

	return 0;
}

static int mtk_gpu_core1_remove(struct platform_device *pdev)
{
	pm_runtime_disable(&pdev->dev);
	return 0;
}

static const struct of_device_id mtk_gpu_core1_of_ids[] = {
	{ .compatible = "mediatek,gpu_core1",},
	{}
};

static struct platform_driver mtk_gpu_core1_driver = {
	.probe  = mtk_gpu_core1_probe,
	.remove = mtk_gpu_core1_remove,
	.driver = {
		.name = "gpu_core1",
		.of_match_table = mtk_gpu_core1_of_ids,
	}
};

static int mtk_gpu_core2_probe(struct platform_device *pdev)
{
	if (!pdev->dev.pm_domain)
		return -EPROBE_DEFER;

	Probe_GPU_core2_dev = pdev;

	pm_runtime_enable(&pdev->dev);

	return 0;
}

static int mtk_gpu_core2_remove(struct platform_device *pdev)
{
	pm_runtime_disable(&pdev->dev);
	return 0;
}

static const struct of_device_id mtk_gpu_core2_of_ids[] = {
	{ .compatible = "mediatek,gpu_core2",},
	{}
};

static struct platform_driver mtk_gpu_core2_driver = {
	.probe  = mtk_gpu_core2_probe,
	.remove = mtk_gpu_core2_remove,
	.driver = {
		.name = "gpu_core2",
		.of_match_table = mtk_gpu_core2_of_ids,
	}
};
```
  There is no need to create 2 copies of the same code to handle 2 different compatible string. We can have 1 code and identify the core based on match data, which can be assigned to compatible string in of_device_id array.
- Patch Set #4, Line 98: == 0
  nit: You can use !regulator_is_enabled(). (But as mentioned in other comment, I'd prefer if this check could be avoided by design.)
- Patch Set #4, Line 104: return 0;
  Doesn't 0 mean "GPU was already powered on"? Shouldn't we return negative error code instead?
- Patch Set #4, Line 121: "Power on core 0 failed (err: %d)\n", error);
  Sounds like a critical error.
- Patch Set #4, Line 128: "Power on core 1 failed (err: %d)\n", error);
  Sounds like a critical error.
- Patch Set #4, Line 135: "Power on core 2 failed (err: %d)\n", error);
  Sounds like a critical error.
- Patch Set #4, Line 145: ret = 0;
  Sounds like a critical error.
- Patch Set #4, Line 152: ret = 0;
  Sounds like a critical error.
- Patch Set #4, Line 159: ret = 0;
  Sounds like a critical error.
- Patch Set #4, Line 208:
```
int kbase_device_runtime_init(struct kbase_device *kbdev)
{
	dev_dbg(kbdev->dev, "%s\n", __func__);

	return 0;
}

void kbase_device_runtime_disable(struct kbase_device *kbdev)
{
	dev_dbg(kbdev->dev, "%s\n", __func__);
}
```
  Why kbase_ and not static?
- Patch Set #4, Line 234: pm_callback_runtime_on
  That function doesn't do anything.
- Patch Set #4, Line 240: {
  In Rockchip platform in 4.4 (which should be using the same Kbase), there is a comment saying that the GPU might be still powered up here and we need to power it down:
  https://chromium.googlesource.com/chromiumos/third_party/kernel/+/439d815e28b7993970b4b2ed7089fe91d0b4b72b/drivers/gpu/arm/midgard/platform/rk/mali_kbase_config_rk.c#228
  Is that comment obsolete or we need to do it too?
- Patch Set #4, Line 269: dev_dbg(kbdev->dev,
  dev_err() should be used for printing errors. (Please fix in other places too.)
- Patch Set #4, Line 304: } else {
  No need to put this inside else block, since the then block returns.
- Patch Set #4, Line 309: "Vsram_gpu regulator set voltage failed: %d\n", err);
  Shouldn't we error out here? I feel like it isn't really safe to continue if such failure happens.
- Patch Set #4, Line 321: regulator_put(mfg->vsram_gpu_regulator);
  This looks like a critical error too.
  Also, it was retrieved using devm_regulator_get(), so no need to explicitly put.
- Patch Set #4, Line 327: if (volt == VSRAM_GPU_MAX_VOLT)
  This condition is wrong. regulator_set_voltage() was called with [VSRAM_GPU_MAX_VOLT, VSRAM_GPU_MAX_VOLT+125] range, so it might be any voltage in this range. Also, if regulator_set_voltage() succeeds, it's not possible that the voltage is out of the requested range, so this check is not needed.
- Patch Set #4, Line 337: } else {
  No need to put the code inside else block.
- Patch Set #4, Line 384: result = regulator_is_enabled(mfg->vsram_gpu_regulator);
  The regulator API is reference counted. Calling regulator_disable() doesn't guarantee disabling it, if the refcount was more than 1. You could technically use regulator_force_disable(), but I'd prefer if you could redesign the code to eliminate the need to check or force regulator states.
- Patch Set #4, Line 405: return -EPROBE_DEFER;
  Is this possible to happen? I think the dev core would automatically -EPROBE_DEFER if power domain is not present yet before attempting to probe the device and this code wouldn't be even called.
- Patch Set #4, Line 409: return -ENOMEM;
  It's not clear when this function is called, so I'd just use kzalloc() here and kfree() with setting kbdev->platform_context to NULL in platform_term().
- Patch Set #4, Line 442: .platform_term_func = platform_term_func
  nit: Drop the _func suffix.
- Patch Set #4, Line 445: #if defined(MODULE)
  What does CONFIG_MODULE have to do here? I don't see these functions referenced from any other file, so shouldn't they be always static?

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Sep 18, 2018, 3:32:54 AM9/18/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #5 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 615 insertions(+), 12 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Sep 18, 2018, 9:08:33 AM9/18/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #6 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 581 insertions(+), 12 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 3, 2018, 3:21:12 AM10/3/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #8 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 605 insertions(+), 16 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 4, 2018, 5:28:52 AM10/4/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Hi Nick, I can see many of my comments being addressed, thanks. However, some of them are not. Could you please click "Done" for any comment that you address and reply to any remaining comments that you have some concerns about? Thanks in advance.

View Change

11 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 124:

- I still think this should go to separate entry in opp->supplies[] and be specified in DT, because th […]
  Ping?
- Patch Set #4, Line 126: tage = ((voltage + MIN_V
  Could we instead turn kbdev->regulator into kbdev->regulators[] and have the entries match the order […]
  Ping?
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #8, Line 164: }while(tvolt != voltage || tsvolt != svoltage);
  Why do we need this loop?
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #8, Line 90: "Power on core 0 failed (err: %d)\n", error);
  Shouldn't we return an error here?
- Patch Set #8, Line 97: "Power on core 1 failed (err: %d)\n", error);
  Shouldn't we return an error here?
- Patch Set #8, Line 104: "Power on core 2 failed (err: %d)\n", error);
  Shouldn't we return an error here?
- Patch Set #8, Line 112: error);
  Shouldn't we return an error here?
- Patch Set #8, Line 119: "clk_mux clock enable failed (err: %d)\n", error);
  Shouldn't we return an error here?
- Patch Set #8, Line 126: "subsys_mfg_cg clock enable failed (err: %d)\n", error);
  Shouldn't we return an error here?
- Patch Set #8, Line 268: VSRAM_GPU_MAX_VOLT, VSRAM_GPU_MAX_VOLT);
  I think we may need some tolerance here. What precision is supported depends on the regulator, so the exact voltage as requested might not be available.
- Patch Set #8, Line 284: VGPU_MAX_VOLT, VGPU_MAX_VOLT);
  Ditto.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 4, 2018, 9:44:11 AM10/4/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #9 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---

M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h

A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
5 files changed, 606 insertions(+), 16 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 4, 2018, 9:57:49 AM10/4/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Hi Tomasz,

I just noticed that previous replies did not properly published.
so you might did not received any of my replies.
Sorry for that.
Anyway, I have solved the problems as much as I could.
Please help to review again.
And please let me know if you have any thought about the remaining unsolved questions.
Thanks

Patch set 9:Verified +1Code-Review +1

View Change

40 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #3, Line 134:
```
			tvolt = min(max((int)tvolt+MIN_VOLT_BIAS,VGPU_MIN_VOLT),VGPU_MAX_VOLT);
			tsvolt = min(max((int)tvolt+MIN_VOLT_BIAS,VSRAM_GPU_MIN_VOLT),VSRAM_GPU_MAX_VOLT);
		}
		else
		{
			tvolt = voltage;
			tsvolt = svoltage;
```
  Rather than having a custom voltage table, we should have the opp include all the regulators in ques […]
  I have tried the multiple regulators example (example 4) in opp.txt in early porting, but it did not work as expected.
  However, I will change the way to get voltage data from a custom voltage table.
  This is not necessary to create a custom voltage table anyway.
  Because PTPOD will update new voltage data in opp list, I will set a reasonable voltage for gpu_sram regulator base on the voltage data get from opp table.
- Patch Set #3, Line 168: ed to set clock %lu

- Why don't we set kbdev->clock to this clock instead and avoid changing this?

- I can try to avoid changing this.
- Patch Set #3, Line 204: dev_err(dev,

- Why don't we just use kbdev->current_freq, which also has "freq" assigned to it?

- Good point, I will modify this and update a new patchset
- Patch Set #3, Line 497: kbdev->devfreq = NULL;
  What is this needed for? We should have the OPPs in DT already limited to this min and max and min/m […]
  This is for update max / min frequency values for debugfs.
  It can be removed.
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 124:
  I still think this should go to separate entry in opp->supplies[] and be specified in DT, because th […]

- There are two voltage tables for each frequency.(For regulator Vgpu and Vsram_gpu)
  We need a calculation here is because Vgpu voltage tables will be runtime update by AVS(adaptive voltage scaling).
  However, if we only update Vgpu with new voltage, this may be a problem for a fix Vsram_gpu voltage.
  There are two rules for setting Vgpu and Vsram_gpu voltages listed as following.
  (1) 100mV <= Vsram_gpu – Vgpu <= 250mV
  (2) Vsram >=850mV
  That may cause a problem here when AVS update a new Vgpu voltage, and we still use Vsram_gpu voltage from the original table.
  The voltage difference may larger than 250mV between Vgpu and Vsram_gpu.
  So it should be safer to calculate new voltage for Vsram_gpu here.
  That can make sure the Vsram_gpu can fully obey these rules.
  That is also make the code not so generic for other SoCs, but it is caused by the hardware limitations.
  Similar idea can be found in mtk_cpufreq_voltage_tracking function in drivers/cpufreq/mediatek-cpufreq.c file.

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

- Ping?
  If we need to get the extra voltage from the opp table v2, I may need to modify opp framework API.
  Because the dev_pm_opp_get_voltage always return only the first supply from the opp table.
  Even though I set multiple regulator voltages in the device tree, there is no suitable function to get the second opp supply voltages.
  I am not sure that is it reasonable to modified the opp APIs?
  You might want to see the dev_pm_opp_get_voltage in opp/core.c.
  And you will notice that it always return the first supply voltage.(opp->supplies[0].u_volt;)
  It seems no way to get the second supplies unless I change this or create another function for it.
  Do you want me to change the files under opp folder?
  Or do you have any ideas for this problem?
  Please let me know, thanks
- Patch Set #4, Line 139: olt = voltage;

- Why can't we keep using kbdev->regulator here?

- Patch Set #8, Line 164: }while(tvolt != voltage || tsvolt != svoltage);
  Why do we need this loop?

- Here are the rules for Vgpu and Vsram_gpu regulator voltages
  There are two rules for setting Vgpu and Vsram_gpu voltages listed as following.
  (1) 100mV <= Vsram_gpu – Vgpu <= 250mV
  (2) Vsram >=850mV
  This loop is used to make sure the voltage step down or up in a small step.
  Avoid dramatically changing the voltages in one big step.
  You may be interested in mtk_cpufreq_voltage_tracking function in drivers/cpufreq/mediatek-cpufreq.c file.
  There is a similar case.
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #4, Line 23: static struct platform_device *probe_gpu_core2_dev;

- Linux kernel uses lowercase for variable names.

- Fixed in new patch

- Patch Set #4, Line 25:

static const struct of_device_id mtk_gpu_coreX_of_ids[] = {
	{ .compatible = "mediatek,gpu_core1", .data = "1" },
	{ .compatible = "mediatek,gpu_core2", .data = "2" },
	{}
};

static int mtk_gpu_coreX_probe(struct platform_device *pdev)
{
	struct device *dev = &pdev->dev;
	const struct of_device_id *match;
	const char *tmp;
	match = of_match_device(mtk_gpu_coreX_of_ids, dev);
	if (!match)
		return -ENODEV;
	tmp = match->data;
	if(*tmp == '1')
		probe_gpu_core1_dev = pdev;
	else
		probe_gpu_core2_dev = pdev;

	pm_runtime_enable(&pdev->dev);

	return 0;
}

static int mtk_gpu_coreX_remove(struct platform_device *pdev)
{
	pm_runtime_disable(&pdev->dev);
	return 0;
}

static struct platform_driver mtk_gpu_coreX_driver = {
	.probe  = mtk_gpu_coreX_probe,
	.remove = mtk_gpu_coreX_remove,
	.driver = {
		.name = "gpu_coreX",
		.of_match_table = mtk_gpu_coreX_of_ids,
	}
};

static int pm_callback_power_on(struct kbase_device *kbdev)
{
	int error;
	struct mfg_base *mfg = kbdev->platform_context;

	error = regulator_enable(mfg->vsram_gpu_regulator);
	if (error < 0) {
		dev_err(kbdev->dev,
			"Power on Vsram_gpu failed error = %d\n",
			error);
		return error;
	}

	error = regulator_enable(kbdev->regulator);
	if (error < 0) {
		dev_err(kbdev->dev,
			"Power on Vgpu failed error = %d\n", error);
		return error;
	}

	error = pm_runtime_get_sync(kbdev->dev);
	if (error != 0) {

There is no need to create 2 copies of the same code to handle 2 different compatible string. […]

Fixed in latest patch

- Patch Set #4, Line 98:

- nit: You can use !regulator_is_enabled(). […]
  Fixed in new patch

- Patch Set #4, Line 104: }

- Doesn't 0 mean "GPU was already powered on"? Shouldn't we return negative error code instead?
  Fixed in new patch
- Patch Set #4, Line 121: error = clk_prepare_enable(mfg->subsys_mfg_cg);

- Sounds like a critical error.

- Fixed in new patch
- Patch Set #4, Line 128: return 1;

- Sounds like a critical error.

- Fixed in new patch

- Patch Set #4, Line 135:

- Sounds like a critical error.
  Fixed in new patch
- Patch Set #4, Line 145: "Power off core 2 failed (err: %d)\n", error);

- Sounds like a critical error.

- Fixed in new patch
- Patch Set #4, Line 152: error = pm_runtime_put_sync(kbdev->dev);

- Sounds like a critical error.

- Fixed in new patch
- Patch Set #4, Line 159: dev_err(kbdev->dev,

- Sounds like a critical error.

- Fixed in new patch

- Patch Set #4, Line 208:

	.power_runtime_init_callback = kbase_device_runtime_init,
	.power_runtime_term_callback = kbase_device_runtime_disable,
	.power_runtime_on_callback = pm_callback_runtime_on,
	.power_runtime_off_callback = pm_callback_runtime_off,
#else				/* KBASE_PM_RUNTIME */
	.power_runtime_init_callback = NULL,
	.power_runtime_term_callback = NULL,
	.power_runtime_on_callback = NULL,
	.power_runtime_off_callback = NULL,
#endif				/* KBASE_PM_RUNTIME */
}

- Why kbase_ and not static?

- Fixed in new patch
- Patch Set #4, Line 234: ore1_dev = probe_gpu_c

- That function doesn't do anything.

- Fixed in new patch
- Patch Set #4, Line 240: dev_err(kbdev->dev, "devm_clk_get clk_main_parent failed\n");
  In Rockchip platform in 4. […]
  Fixed in new patch
- Patch Set #4, Line 269: dev_err(kbdev->dev,
  dev_err() should be used for printing errors. (Please fix in other places too. […]
  Fixed in new patch
- Patch Set #4, Line 304: struct mfg_base *mfg = kbdev->platform_context;

- No need to put this inside else block, since the then block returns.

- Fixed in new patch
- Patch Set #4, Line 309: "Vgpu regulator_disable failed: %d\n", result);

- Shouldn't we error out here? I feel like it isn't really safe to continue if such failure happens.

- Fixed in new patch
- Patch Set #4, Line 321: static int platform_init(struct kbase_device *kbdev)
  This looks like a critical error too. […]
  Fixed in new patch
- Patch Set #4, Line 327: if (!mfg)
  This condition is wrong. […]
  Fixed in new patch
- Patch Set #4, Line 337: err = clk_set_rate(mfg->clk_main_parent, GPU_FREQ_KHZ_MAX * 1000);

- No need to put the code inside else block.

- Fixed in new patch

- Patch Set #4, Line 384:

- The regulator API is reference counted. […]
  Fixed in new patch

- Patch Set #4, Line 405:

- Is this possible to happen? I think the dev core would automatically -EPROBE_DEFER if power domain i […]
  Fixed in new patch

- Patch Set #4, Line 409:

- It's not clear when this function is called, so I'd just use kzalloc() here and kfree() with setting […]
  Fixed in new patch

- Patch Set #4, Line 442:

- nit: Drop the _func suffix.
  Fixed in new patch

- Patch Set #4, Line 445:

- What does CONFIG_MODULE have to do here? I don't see these functions referenced from any other file, […]
  Fixed in new patch
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:

- Patch Set #8, Line 90: }

- Shouldn't we return an error here?
  Done

- Patch Set #8, Line 97: }
  Shouldn't we return an error here?

- Done

- Patch Set #8, Line 104: }

- Shouldn't we return an error here?
  Done

- Patch Set #8, Line 112: }

- Shouldn't we return an error here?
  Done

- Patch Set #8, Line 119: }

- Shouldn't we return an error here?
  Done

- Patch Set #8, Line 126: }

- Shouldn't we return an error here?
  Done
- Patch Set #8, Line 268: if (err < 0) {
  I think we may need some tolerance here. […]
  Done
- Patch Set #8, Line 284: if (err < 0) {
  Ditto.
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 4, 2018, 10:02:53 AM10/4/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Update done

View Change

34 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #3, Line 168: ed to set clock %lu
  I can try to avoid changing this.
  Done

- Patch Set #3, Line 204: dev_err(dev,

- Good point, I will modify this and update a new patchset
  Done

- Patch Set #3, Line 497: kbdev->devfreq = NULL;

- This is for update max / min frequency values for debugfs. […]
  Done
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #4, Line 139: olt = voltage;
  Done
  Done
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_devfreq_mediatek.c:

- Patch Set #1:
  Quite a bit of this code is copied from backend/gpu/mali_kbase_devfreq.c .

- Done

- Patch Set #1, Line 27:

- Alphabetical order.
  Done

- Patch Set #1, Line 64:
  drop that space

- Done

- Patch Set #1, Line 81:

- Cast not needed, platform_context is a void *.
  Done
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #1, Line 20: #include <mali_kbase_defs.h>
  ordering too.
  Done
- Patch Set #1, Line 45: pm_runtime_enable(&pdev->dev);
  indentation
  Done

- Patch Set #1, Line 77:
  indentation

- Patch Set #4, Line 23: static struct platform_device *probe_gpu_core2_dev;

- Fixed in new patch
  Done

- Fixed in latest patch
  Done

- Patch Set #4, Line 98:

- Fixed in new patch
  Done

- Patch Set #4, Line 104: }

- Fixed in new patch
  Done

- Patch Set #4, Line 121: error = clk_prepare_enable(mfg->subsys_mfg_cg);

- Fixed in new patch
  Done

- Patch Set #4, Line 128: return 1;

- Fixed in new patch
  Done

- Patch Set #4, Line 135:

- Fixed in new patch
  Done

- Patch Set #4, Line 145: "Power off core 2 failed (err: %d)\n", error);

- Fixed in new patch
  Done

- Patch Set #4, Line 152: error = pm_runtime_put_sync(kbdev->dev);

- Fixed in new patch
  Done

- Patch Set #4, Line 159: dev_err(kbdev->dev,

- Fixed in new patch
  Done

Patch Set #4, Line 208:

	.power_runtime_init_callback = kbase_device_runtime_init,
	.power_runtime_term_callback = kbase_device_runtime_disable,
	.power_runtime_on_callback = pm_callback_runtime_on,
	.power_runtime_off_callback = pm_callback_runtime_off,
#else				/* KBASE_PM_RUNTIME */
	.power_runtime_init_callback = NULL,
	.power_runtime_term_callback = NULL,
	.power_runtime_on_callback = NULL,
	.power_runtime_off_callback = NULL,
#endif				/* KBASE_PM_RUNTIME */
}

- Fixed in new patch
  Done

- Patch Set #4, Line 234: ore1_dev = probe_gpu_c

- Fixed in new patch
  Done

- Patch Set #4, Line 240: dev_err(kbdev->dev, "devm_clk_get clk_main_parent failed\n");

- Fixed in new patch
  Done

- Patch Set #4, Line 269: dev_err(kbdev->dev,

- Fixed in new patch
  Done

- Patch Set #4, Line 304: struct mfg_base *mfg = kbdev->platform_context;

- Fixed in new patch
  Done

- Patch Set #4, Line 309: "Vgpu regulator_disable failed: %d\n", result);

- Fixed in new patch
  Done

- Patch Set #4, Line 321: static int platform_init(struct kbase_device *kbdev)

- Fixed in new patch
  Done

- Patch Set #4, Line 327: if (!mfg)

- Fixed in new patch
  Done

- Patch Set #4, Line 384:

- Fixed in new patch
  Done

- Patch Set #4, Line 405:

- Fixed in new patch
  Done

- Patch Set #4, Line 409:

- Fixed in new patch
  Done

- Patch Set #4, Line 442:

- Fixed in new patch
  Done

- Patch Set #4, Line 445:

- Fixed in new patch
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 4, 2018, 10:06:02 AM10/4/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Update done

View Change

3 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #3, Line 134:

			tvolt = min(max((int)tvolt+MIN_VOLT_BIAS,VGPU_MIN_VOLT),VGPU_MAX_VOLT);
			tsvolt = min(max((int)tvolt+MIN_VOLT_BIAS,VSRAM_GPU_MIN_VOLT),VSRAM_GPU_MAX_VOLT);
		}
		else
		{
			tvolt = voltage;
			tsvolt = svoltage;

- I have tried the multiple regulators example (example 4) in opp. […]
  Done
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_devfreq_mediatek.c:
- Patch Set #1, Line 64:
  Done
  Done

- Patch Set #1, Line 81:

- Done
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 4, 2018, 10:07:54 AM10/4/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Update done

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/Kbuild:

- Patch Set #1, Line 56:

- Why? Seems a bit odd that we need to change the common Kbuild file. […]
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 5, 2018, 5:23:18 AM10/5/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 9: Verified+1 Code-Review+1
(40 comments)

Hi Tomasz,
I just noticed that previous replies did not properly published.
so you might did not received any of my replies.
Sorry for that.
Anyway, I have solved the problems as much as I could.
Please help to review again.
And please let me know if you have any thought about the remaining unsolved questions.
Thanks

Thanks for posting the replies. That's very helpful.

View Change

3 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #4, Line 124:
  There are two voltage tables for each frequency.(For regulator Vgpu and Vsram_gpu) […]
  Ack

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

- If we need to get the extra voltage from the opp table v2, I may need to modify opp framework API. […]
  Couldn't you just access opp->supplies[1].u_volt manually, without using dev_pm_opp_get_voltage()?
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #8, Line 164: }while(tvolt != voltage || tsvolt != svoltage);

- Here are the rules for Vgpu and Vsram_gpu regulator voltages […]
  Hmm, condition (1) makes things quite complicated. Let me think how we can simplify this.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 5, 2018, 9:33:18 AM10/5/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Update reply

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #4, Line 126: tage = ((voltage + MIN_V
  Couldn't you just access opp->supplies[1]. […]
  I could try to directly access to opp->supplies[1].u_volt without using opp APIs and feedback to you later.
  But I am not sure that is it reasonable to directly access to supplies voltage data without using dev_pm_opp_get_voltage?
  Are we necessary to get opp->supplies[1] here?
  It seems that we could use the calculated voltage (svoltage) for Vgpu_sram regulator instead of getting it from opp table.
  If we want multiple regulators for opp table v2, just like the example 4 in the following link.
  https://elixir.bootlin.com/linux/v4.2/source/Documentation/devicetree/bindings/power/opp.txt
  It may need to change the opp framework APIs, or it may cause an error in opp_parse_supplies function. (Due to opp-microvolt property and supplies number mismatch)
  I will need to change for the opp_table->regulator_count number while we adding opp table v2 in _of_add_opp_table_v2 function.
  Please let me know if you have any concerns or ideas for this.
  Thanks

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 9, 2018, 5:45:19 AM10/9/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

- I could try to directly access to opp->supplies[1]. […]
  Okay, after thinking a bit more and checking the OPP framework code, we should indeed add dev_pm_opp_get_voltage_supply(), which takes the supply index as a parameter.
  As for the other concern, where would the mismatch be, if we specify the vgpu and vgpu_sram as separate supplies? Looking at upstream code [1], it seems to handle multiple supplies correctly.
  [1] https://elixir.bootlin.com/linux/v4.19-rc7/source/drivers/opp/of.c#L112

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 9, 2018, 7:23:49 AM10/9/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

Okay, after thinking a bit more and checking the OPP framework code, we should indeed add dev_pm_opp […]

Yes I thought the same idea, that definitely needs a supply index for dev_pm_opp_get_voltage_supply function in order to get multiple supplies.

If I specify the OPP table with two regulator supplies, the opp-microvolt property should be two.
I will get the following log message, because opp_table->regulator_count is not specified by default.
======================Log===========================
opp_parse_supplies: Invalid number of elements in opp-microvolt property (2) with supplies (1)

=============Device tree example====================
gpu_opp_table: gpu_opp_table0 {
    compatible = "operating-points-v2";

    opp@178000000 {
         opp-hz = /bits/ 64 <178000000>;
         opp-microvolt = <870000>, /* Supply 0 */
                         <860000>; /* Supply 1 */
    };
    opp@400000000 {
         opp-hz = /bits/ 64 <400000000>;
         opp-microvolt = <970000>, /* Supply 0 */
                         <960000>; /* Supply 1 */
    };
====================================================

However, I can change the regulator_count by using dev_pm_opp_set_regulators function.
This may need to change the _of_add_opp_table_v2 function in of.c

Here is the patch that how I did in my local.
======================Patch=========================
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 7af0dde..cfd91fe 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -384,6 +384,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 	struct opp_table *opp_table;
 	int ret = 0, count = 0, pstate_count = 0;
 	struct dev_pm_opp *opp;
+	const char *reg_names[] = {"mali","mali_sram"};
 
 	opp_table = _managed_opp(opp_np);
 	if (opp_table) {
@@ -393,7 +394,8 @@ static int _of_add_opp_table_v2(struct device *dev, struct device_node *opp_np)
 		goto put_opp_table;
 	}
 
-	opp_table = dev_pm_opp_get_opp_table(dev);
+	opp_table = dev_pm_opp_set_regulators(dev,reg_names,2);
 	if (!opp_table)
 		return -ENOMEM;

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 15, 2018, 5:34:36 AM10/15/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

- Yes I thought the same idea, that definitely needs a supply index for dev_pm_opp_get_voltage_supply […]
  Okay, I see what you mean now. Looking at how dev_pm_opp_set_regulators() is used in existing code, e.g. https://elixir.bootlin.com/linux/latest/source/drivers/cpufreq/cpufreq-dt.c#L198, one would call it before parsing the OPPs. So it sounds like we should call it somewhere from this Mali code, before _of_add_opp_table_v2() is invoked.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 18, 2018, 9:03:11 AM10/18/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #10 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c

M drivers/gpu/arm/midgard/mali_kbase_core_linux.c


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

6 files changed, 619 insertions(+), 16 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 18, 2018, 9:09:05 AM10/18/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 10:Verified +1Code-Review +1

View Change

1 comment:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #4, Line 126: tage = ((voltage + MIN_V

- Okay, I see what you mean now. […]
  Update in latest patch

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 23, 2018, 11:50:57 PM10/23/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Please make sure that all the errors and warnings reported by "repo upload" (without "--no-verify") are fixed.

View Change

9 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #8, Line 131: {
  coding style: brackets should be on the same line
- Patch Set #8, Line 133: {
  coding style: brackets on the same line, missing spaces between keywords and parentheses and spaces on both sides of binary operators.
- Patch Set #8, Line 135: tsvolt = min(max((int)tvolt+MIN_VOLT_BIAS,VSRAM_GPU_MIN_VOLT),VSRAM_GPU_MAX_VOLT);
  coding style: missing spaces after commas and spaces on both sides of binary operators, line longer than 80 character.
- Patch Set #8, Line 138: {
  coding style: brackets

- Patch Set #8, Line 164: }while(tvolt != voltage || tsvolt != svoltage);

- Hmm, condition (1) makes things quite complicated. Let me think how we can simplify this.
  I think we would ideally want to use the coupled regulator functionality of the regulator core, but it has been reverted from linux-next due to some regressions. For reference, here are the patches: https://lwn.net/ml/linux-kernel/1524494022-22260-1-gi...@samsung.com/ .
  For now, please make the code query the OPP for target voltages of both regulators, fix the coding style and make this code work correctly with just 1 regulator too, since we may need to use this driver for other platforms too.
- Patch Set #8, Line 217: kbdev->current_voltage = voltage;
  How about making current_voltage an array?
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #10, Line 126: svoltage = ((voltage + MIN_VOLT_BIAS) >= VSRAM_GPU_MIN_VOLT) ? (voltage + MIN_VOLT_BIAS) : VSRAM_GPU_MIN_VOLT;
  Okay, so we have now 2 regulators added to the OPP. Why don't we use the voltages from the OPP?
File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:
- Patch Set #10, Line 102: static struct opp_table *opp_table = NULL;
  Could we store this inside kbdev instead?
- Patch Set #10, Line 3265: dev_pm_opp_put_regulators(opp_table);
  We should call this on the error path, when the probe deferral happens. Perhaps power_control_term()? Generally the driver must always clean up all the state when failing the probe (including probe deferral).

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 25, 2018, 8:54:56 AM10/25/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #11 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c
M drivers/gpu/arm/midgard/mali_kbase_core_linux.c

M drivers/gpu/arm/midgard/mali_kbase_defs.h


A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

M drivers/opp/core.c
M include/linux/pm_opp.h
9 files changed, 688 insertions(+), 42 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Oct 25, 2018, 9:05:35 AM10/25/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 11:Verified +1Code-Review +1

View Change

10 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #4, Line 126: urn PTR_ERR(opp);
  Update in latest patch
  Done
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #8, Line 131:
  coding style: brackets should be on the same line

- Done

- Patch Set #8, Line 133:

- coding style: brackets on the same line, missing spaces between keywords and parentheses and spaces […]
  Done
- Patch Set #8, Line 135: * Only update if there is a change of frequency
  coding style: missing spaces after commas and spaces on both sides of binary operators, line longer […]
  Done
- Patch Set #8, Line 138: *target_freq = nominal_freq;
  coding style: brackets
  Done
- Patch Set #8, Line 164: err = regulator_set_voltage(kbdev->regulator[i],
  I think we would ideally want to use the coupled regulator functionality of the regulator core, but […]
  Done
- Patch Set #8, Line 217: dev_err(dev,

- How about making current_voltage an array?

- Done
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #10, Line 126: return PTR_ERR(opp);
  Okay, so we have now 2 regulators added to the OPP. […]
  Done
File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:

- Patch Set #10, Line 102:

- Could we store this inside kbdev instead?
  Done
- Patch Set #10, Line 3265: || defined(LSK_OPPV2_BACKPORT)
  We should call this on the error path, when the probe deferral happens. […]
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Oct 26, 2018, 4:07:35 AM10/26/18

to Nick Fan, Nick Fan, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

15 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

Patch Set #11, Line 94: unsigned long *target_freq, u32 flags)
Why this change?
Patch Set #11, Line 100: REGULATOR_NUM
This number should come from the platform backend, at runtime, not compile time.
Patch Set #11, Line 100:
```
	unsigned long voltage[REGULATOR_NUM];
#ifdef CONFIG_REGULATOR
	unsigned long tvolt[REGULATOR_NUM];
```
Perhaps we could rename this to "target_volt" and "step_volt", for readability and consistency with "target_freq"?
Patch Set #11, Line 146: check_flag = true;
We shouldn't run the block above every time, just before raising the frequency, right?

Patch Set #11, Line 146:

	check_flag = true;
	for (i = 0; i < REGULATOR_NUM; i++)
		tvolt[i] = kbdev->current_voltage[i];
	do {
		for (i = REGULATOR_NUM - 1; i >= 0; i--) {
			regulator_min_volt = (0 == i) ? VGPU_MIN_VOLT : VSRAM_GPU_MIN_VOLT;
			regulator_max_volt = (0 == i) ? VGPU_MAX_VOLT : VSRAM_GPU_MAX_VOLT;
			if ((voltage[1] - tvolt[0]) > MAX_VOLT_BIAS) {
				tvolt[i] = min(max(tvolt[0] + MIN_VOLT_BIAS,
					regulator_min_volt), regulator_max_volt);
			}
			else {
				tvolt[i] = voltage[i];
				check_flag = false;
			}

			if (kbdev->regulator[i] && kbdev->current_voltage[i] != tvolt[i]
				&& kbdev->current_freq < freq) {


				err = regulator_set_voltage(kbdev->regulator[i],

```
					tvolt[i], tvolt[i] + VOLT_TOL);
				if (err) {
					dev_err(dev,
						"Failed to increase reg %d voltage err:(%d)\n", i, err);
					return err;
				}
			}
		}
	}while(check_flag);
```
Actually, this block is almost identical with the one below after the clock settings. Only the direction of the iteration and the order of subtraction changes, but the former shouldn't matter and the latter could be handled using abs(). Could we move it into a helper function?
The function would take an array of current voltages and target voltages and fill in an output array of next voltages. The caller would apply the next voltages using regulator_set_voltage() in a loop, copy next voltages to current voltages array and then iterate again, until target voltages become equal to current voltages.

Patch Set #11, Line 195:

#ifdef CONFIG_REGULATOR
	check_flag = true;
	for (i = 0; i < REGULATOR_NUM; i++)
		tvolt[i] = kbdev->current_voltage[i];
	do {
		for (i = 0; i < REGULATOR_NUM; i++) {
			regulator_min_volt = (0 == i) ? VGPU_MIN_VOLT : VSRAM_GPU_MIN_VOLT;
			regulator_max_volt = (0 == i) ? VGPU_MAX_VOLT : VSRAM_GPU_MAX_VOLT;
			if ((tvolt[1] - voltage[0]) > MAX_VOLT_BIAS) {
				tvolt[i] = min(max(tvolt[0] - MIN_VOLT_BIAS,
					regulator_min_volt), regulator_max_volt);
			}
			else {
				tvolt[i] = voltage[i];
				check_flag = false;
			}

			if (kbdev->regulator[i] && kbdev->current_voltage[i] != tvolt[i]
				&& kbdev->current_freq > freq) {


				err = regulator_set_voltage(kbdev->regulator[i],

- ```
					tvolt[i], tvolt[i] + VOLT_TOL);
				if (err) {
					dev_err(dev,
						"Failed to decrease reg %d voltage err:(%d)\n", i, err);
					return err;
				}
			}
		}
	}while(check_flag);
#endif
```
  Hmm, we shouldn't run this every time, just after lowering the frequency, right?
- Patch Set #11, Line 254: struct devfreq_dev_status *stat)
  Unnecessary change.
- Patch Set #11, Line 319: kbase_devfreq_term_freq_table(struct kbase_device *kbdev)
  Ditto.
- Patch Set #11, Line 334: kbase_devfreq_init_core_mask_table(struct kbase_device *kbdev)
  Ditto.
- Patch Set #11, Line 399: i, opp_freq, real_freq, core_mask);
  Ditto.
File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:
- Patch Set #11, Line 3217: const char *reg_names[] = {"mali", "mali_sram"};
  The names and number of regulator should come from the platform backend at runtime, since we might have other platforms with other regulators, but compiled into the same kernel.

File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h:

Patch Set #11, Line 23:

/* Definition for PMIC regulators */
#define VSRAM_GPU_MAX_VOLT (925000)	/* uV */
#define VSRAM_GPU_MIN_VOLT (850000) /* uV */
#define VGPU_MAX_VOLT (825000)	/* uV */
#define VGPU_MIN_VOLT (625000)	/* uV */

#define MIN_VOLT_BIAS (100000) /* uV */
#define MAX_VOLT_BIAS (250000) /* uV */
#define VOLT_TOL (125) /* uV */

These values are specific to MT8183, so they should be returned at runtime from the platform specific code and not hardcoded at compile time.

File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #11, Line 31: static int mtk_gpu_coreX_probe(struct platform_device *pdev)
  nit: Function names are lowercase in kernel coding style. Please fix the other places too.

File drivers/opp/core.c:

Patch Set #11, Line 110:

/**
 * dev_pm_opp_get_voltage_supply() - Gets the voltage corresponding to an opp
 * with index
 *
 * Return: voltage in micro volt corresponding to the opp with index, else
 * return 0
 *
 * This is useful for devices with multiple power supplies.
 */
unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp,
	struct opp_table *opp_tbl, int supply_index)
{
	if (IS_ERR_OR_NULL(opp)) {
		pr_err("%s: Invalid parameters\n", __func__);
		return 0;
	}

	if(supply_index < 0 || supply_index >= opp_tbl->regulator_count) {
		pr_err("%s: Invalid supply index: %d\n", __func__, supply_index);
		return 0;
	}

	return opp->supplies[supply_index].u_volt;
}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage_supply);

/**

Please move this into a separate CL, send upstream for review and cherry-pick the patch from the mailing list as a FROMLIST CL.

File include/linux/pm_opp.h:
- Patch Set #11, Line 86: unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp, struct opp_table *opp_tbl, int supply_index);
  Line longer than 80 characters. Also this should go together with the separate CL I mentioned before.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 2, 2018, 4:58:49 AM11/2/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #12 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c
M drivers/gpu/arm/midgard/mali_kbase_core_linux.c
M drivers/gpu/arm/midgard/mali_kbase_defs.h
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

7 files changed, 661 insertions(+), 34 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 2, 2018, 5:00:55 AM11/2/18

to Nick Fan, Roger Lu, Tomasz Figa, Nicolas Boichat, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 12:Verified +1Code-Review +1

View Change

13 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #11, Line 94: {
  Why this change?
  Revert this change

- Patch Set #11, Line 100:

- This number should come from the platform backend, at runtime, not compile time.
  Do you mean that get this number from device tree?

- Patch Set #11, Line 100:

- ```
	current_bias = (inc) ?
		target_volt[1] - step_volt[0] : st
```
  Perhaps we could rename this to "target_volt" and "step_volt", for readability and consistency with […]
  Done
- Patch Set #11, Line 146: #endif

- We shouldn't run the block above every time, just before raising the frequency, right?

- Done

- Patch Set #11, Line 146:

#endif
	opp = devfreq_recommended_opp(dev, &freq, flags);
#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 11, 0)
	rcu_read_unlock();
#endif
	if (IS_ERR_OR_NULL(opp)) {
		dev_err(dev, "Failed to get opp (%ld)\n", PTR_ERR(opp));
		return PTR_ERR(opp);

- ```
	}

	for (i = 0; i < REGULATOR_NUM; i++)
```

		target_volt[i] = dev_pm_opp_get_voltage_supply(opp,
			kbdev->dev_opp_table, i);
#if LINUX_VERSION_CODE >= KERNEL_VERSION(4, 11, 0)
	dev_pm_opp_put(opp);
#endif

	nominal_freq = freq;

	/*


	 * Only update if there is a change of frequency

	 */
	if (kbdev->current_nominal_freq == nominal_freq) {
		*target_freq = nominal_freq;
		return 0;
	}

	freq = opp_translat

Actually, this block is almost identical with the one below after the clock settings. […]

Done

- Patch Set #11, Line 195:

	}
#endif

	err = clk_set_parent(mfg->clk_mux, mfg->clk_sub_parent);
	if (err) {
		dev_err(kbdev->dev, "Failed to select sub clock src\n");
		return err;
	}

	err = clk_set_rate(kbdev->clock, freq);
	if (err) {
		dev_err(dev, "Failed to set clock %lu (target %lu)\n",
				freq, *target_freq);
		return err;
	}

	err = clk_set_parent(mfg->clk_mux, mfg->clk_main_parent);
	if (err) {
		dev_err(kbdev->dev, "Failed to select main clock src\n");
		return err;
	}

#ifdef CONFIG_REGULATOR
	if(kbdev->current_freq > freq) {

- ```
		for (i = 0; i < REGULATOR_NUM; i++)
```

			step_volt[i] = kbdev->current_voltage[i];
		while(get_step_volt(&step_volt[0],
			&target_volt[0], REGULATOR_NUM, 0)) {
			for (i = 0; i < REGULATOR_NUM; i++) {
				if

- Hmm, we shouldn't run this every time, just after lowering the frequency, right?

- Done

- Patch Set #11, Line 254:

- Unnecessary change.
  Done
- Patch Set #11, Line 319: #endif
  Ditto.
  Done
- Patch Set #11, Line 334: kfree(dp->freq_table);
  Ditto.
  Done
- Patch Set #11, Line 399: dev_err(kbdev->dev, "OPP has invalid core mask of 0\n");
  Ditto.
  Done
File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:

- Patch Set #11, Line 3217: const char *reg_names[] = {"mali", "mali_sram"};

- The names and number of regulator should come from the platform backend at runtime, since we might h […]
  Do you mean that get regulator name from device tree?
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h:

Patch Set #11, Line 23:

/* Definition for PMIC regulators */
#define VSRAM_GPU_MAX_VOLT (925000)	/* uV */
#define VSRAM_GPU_MIN_VOLT (850000) /* uV */
#define VGPU_MAX_VOLT (825000)	/* uV */
#define VGPU_MIN_VOLT (625000)	/* uV */

#define MIN_VOLT_BIAS (100000) /* uV */
#define MAX_VOLT_BIAS (250000) /* uV */
#define VOLT_TOL (125) /* uV */

- These values are specific to MT8183, so they should be returned at runtime from the platform specifi […]
  Do you mean that get these values from device tree?
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #11, Line 31: static int mtk_gpu_corex_probe(struct platform_device *pdev)

- nit: Function names are lowercase in kernel coding style. Please fix the other places too.

- Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nicolas Boichat (Gerrit)

unread,

Nov 2, 2018, 11:03:02 PM11/2/18

to Nick Fan, Roger Lu, Nick Fan, Tomasz Figa, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Seeing this after rebase:

/mnt/host/source/src/third_party/kernel/v4.14/drivers/gpu/arm/midgard/ipa/mali_kbase_ipa_debugfs.c:199:56: error: invalid operands to binary expression ('unsigned long [2]' and 'int')
kbdev->current_nominal_freq, (kbdev->current_voltage / 1000));
~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
1 error generated.

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 4, 2018, 9:42:08 PM11/4/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Roger Lu, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #13 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c
M drivers/gpu/arm/midgard/mali_kbase_core_linux.c
M drivers/gpu/arm/midgard/mali_kbase_defs.h
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

7 files changed, 660 insertions(+), 34 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 4, 2018, 9:57:06 PM11/4/18

to Nick Fan, Nicolas Boichat, Roger Lu, Tomasz Figa, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 12:

Seeing this after rebase:
/mnt/host/source/src/third_party/kernel/v4.14/drivers/gpu/arm/midgard/ipa/mali_kbase_ipa_debugfs.c:199:56: error: invalid operands to binary expression ('unsigned long [2]' and 'int')
kbdev->current_nominal_freq, (kbdev->current_voltage / 1000));
~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~
1 error generated.

I will rebase to new MTK ToT and try to solve this build fail problem

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 4, 2018, 10:56:50 PM11/4/18

to Nick Fan, Nicolas Boichat, Roger Lu, Tomasz Figa, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

2 comments:

File drivers/opp/core.c:

- Patch Set #11, Line 110:

/**
 * dev_pm_opp_get_voltage_supply() - Gets the voltage corresponding to an opp
 * with index
 *
 * Return: voltage in micro volt corresponding to the opp with index, else
 * return 0
 *
 * This is useful for devices with multiple power supplies.
 */
unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp,

	int supply_index)
{
	if (IS_ERR_OR_NULL(opp)) {
		pr_err("%s: Invalid parameters\n", __func__);
		return 0;
	}

	if(supply_index < 0 || supply_index >= opp->opp_table->regulator_count) {


		pr_err("%s: Invalid supply index: %d\n", __func__, supply_index);
		return 0;
	}

	return opp->supplies[supply_index].u_volt;
}
EXPORT_SYMBOL_GPL(dev_pm_opp_get_voltage_supply);

/**

- Please move this into a separate CL, send upstream for review and cherry-pick the patch from the mai […]
  Done
File include/linux/pm_opp.h:

- Patch Set #11, Line 86: unsigned long dev_pm_opp_get_voltage_supply(struct dev_pm_opp *opp,

- Line longer than 80 characters. […]
  Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 5, 2018, 4:21:00 AM11/5/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Roger Lu, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #14 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c

M drivers/gpu/arm/midgard/ipa/mali_kbase_ipa_debugfs.c


M drivers/gpu/arm/midgard/mali_kbase_core_linux.c
M drivers/gpu/arm/midgard/mali_kbase_defs.h
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

8 files changed, 661 insertions(+), 35 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 6, 2018, 2:15:29 AM11/6/18

to Nick Fan, Nicolas Boichat, Roger Lu, Tomasz Figa, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 14:Verified +1Code-Review +1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 6, 2018, 7:37:39 AM11/6/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Roger Lu, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #17 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c
M drivers/gpu/arm/midgard/ipa/mali_kbase_ipa_debugfs.c
M drivers/gpu/arm/midgard/mali_kbase_core_linux.c
M drivers/gpu/arm/midgard/mali_kbase_defs.h
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c
8 files changed, 661 insertions(+), 35 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 6, 2018, 7:39:37 AM11/6/18

to Nick Fan, Nicolas Boichat, Roger Lu, Tomasz Figa, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 17:Verified +1Code-Review +1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Nov 7, 2018, 7:17:49 AM11/7/18

to Nick Fan, Nick Fan, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

View Change

12 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #11, Line 100:

- Do you mean that get this number from device tree?

- Yes, device tree would be ideal. To clarify, array sizes may be fixed to some hardcoded constant, but any iteration, like the for loop below, should work on the real number of regulators.

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

Patch Set #18, Line 92: int
Maybe bool?

Patch Set #18, Line 101:

	current_bias = (inc) ?
		target_volt[1] - step_volt[0] : step_volt[1] - target_volt[0];
	adjust_step = (inc) ? MIN_VOLT_BIAS : -MIN_VOLT_BIAS;

This would be more readable with a normal if (inc) construct and both variables assigned inside the if/else block.

Patch Set #18, Line 104:

	for (i = 0; i < count; i++) {
		if (step_volt[i] == target_volt[i])
			idx++;
	}

	if (idx == count)
		return 0;

for (i = 0; i < count; ++i)
	if (step_volt[i] != target_volt[i])
		break;
if (i == count)
	return 0;

Patch Set #18, Line 113:

		regulator_min_volt = (0 == i) ? VGPU_MIN_VOLT : VSRAM_GPU_MIN_VOLT;
		regulator_max_volt = (0 == i) ? VGPU_MAX_VOLT : VSRAM_GPU_MAX_VOLT;

- Lines longer than 80 characters. I think we can make this a normal if block, to fix it:
```
if (i) {
	regulator_min_volt = VSRAM_GPU_MIN_VOLT;
	regulator_max_volt = VSRAM_GPU_MAX_VOLT;
} else {
	regulator_min_volt = VGPU_MIN_VOLT;
	regulator_max_volt = VGPU_MAX_VOLT;
}
```
- Patch Set #18, Line 178: &step_volt[0]
  Just pass "step_volt".
- Patch Set #18, Line 179: &target_volt[0]
  Just "target_volt".
- Patch Set #18, Line 181: kbdev->regulator[i]
  If we use the real number of regulators for the loop condition, we don't have to check this anymore.
- Patch Set #18, Line 182: kbdev->current_voltage[i] != step_volt[i]) {
  To avoid so many nested blocks, I'd negate the condition and make it continue to next loop iteration.
- Patch Set #18, Line 217:

		for (i = 0; i < REGULATOR_NUM; i++)
			step_volt[i] = kbdev->current_voltage[i];

- ```
		while (get_step_volt(&step_volt[0],
```


			&target_volt[0], REGULATOR_NUM, 0)) {
			for (i = 0; i < REGULATOR_NUM; i++) {

- ```
				if (kbdev->regulator[i] &&
```

					kbdev->current_voltage[i] != step_volt[i]) {


					err = regulator_set_voltage(kbdev->regulator[i],

						step_volt[i], step_volt[i] + VOLT_TOL);
					if (err) {
						dev_err(dev,


						"Failed to decrease reg %d voltage err:(%d)\n", i, err);
						return err;
					}
				}
			}
		}

This is still almost identical to 176-192. Should be possible to move into a common function:

int set_voltages(kbdev, target_volt, inc)
{
	unsigned long step_volt[REGULATOR_NUM];
	ssize_t first, step;
	ssize_t i;

	for (i = 0; i < kbdev->regulator_num; ++i)


		step_volt[i] = kbdev->current_voltage[i];

	if (inc) {
		first = kbdev->regulator_num - 1;
		step = -1;
	} else {
		first = 0;
		step = 1;
	}

	while (get_step_volt(step_volt, target_volt,
	       kbdev->regulator_num, inc)) {
		for (i = first; i >= 0 && i < kbdev->regulator_num; i += step) {
			if (kbdev->current_voltage[i] == step_volt[i])
				continue;

			err = regulator_set_voltage(kbdev->regulator[i],

						    step_volt[i],
						    step_volt[i] + VOLT_TOL);
			if (err) {
				dev_err(dev,
					"Failed to set reg %d voltage err:(%d)\n", i, err);
				return err;
			}
		}
	}
}

File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:
- Patch Set #11, Line 3217: if (NULL != kbdev->regulator[i]) {

- Do you mean that get regulator name from device tree?

- Nope. I mean the code in platform/mediatek.
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:
- Patch Set #18, Line 261: :
  Missing spaces around the operator.

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 8, 2018, 5:56:18 AM11/8/18

to Eddie Huang, Ben Ho, Tomasz Figa, CK HU, SJ Huang, Roger Lu, Nick Fan, Nicolas Boichat, JB Tsai

Nick Fan uploaded patch set #19 to this change.

View Change

CHROMIUM: MALI: Add Mali platform specific source

Create platform specific folder and source files
Add clock, power and regulator control
Support for GPU devfreq

BUG=b:109911488
Test=emerge-kukui chromeos-kernel-4_14

Change-Id: I4e55458efb008553050e78703e09ac69d07ea21d
Signed-off-by: Nick Fan <Nick...@mediatek.com>
---
M drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c
M drivers/gpu/arm/midgard/ipa/mali_kbase_ipa_debugfs.c
M drivers/gpu/arm/midgard/mali_kbase_core_linux.c
M drivers/gpu/arm/midgard/mali_kbase_defs.h
A drivers/gpu/arm/midgard/platform/mediatek/Kbuild
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_mediatek.c
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_config_platform.h
A drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c

8 files changed, 699 insertions(+), 32 deletions(-)

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 8, 2018, 5:59:43 AM11/8/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 19:Verified +1Code-Review +1

View Change

13 comments:

File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:

- Patch Set #11, Line 100:

- Yes, device tree would be ideal. […]
  Done
File drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c:
- Patch Set #18, Line 92: boo
  Maybe bool?
  Done
- Patch Set #18, Line 92: boo
  Maybe bool?
  Done

- Patch Set #18, Line 101:

- ```
	if (inc) {
		current_bias = target_volt[1] - step_volt[0];
		adjust_step = MIN_VOLT_BIAS;
```
  This would be more readable with a normal if (inc) construct and both variables assigned inside the […]
  Done

- Patch Set #18, Line 104:

	} else {
		current_bias = step_volt[1] - target_volt[0];
		adjust_step = -MIN_VOLT_BIAS;


	}

	for (i = 0; i < count; ++i)
		if (step_

- for (i = 0; i < count; ++i) […]
  Done

- Patch Set #18, Line 113:
```
	if (i == count)
		return 0;
```

- Lines longer than 80 characters. I think we can make this a normal if block, to fix it: […]
  Done
- Patch Set #18, Line 178: ;
  Just pass "step_volt".
  Done
- Patch Set #18, Line 179: signed long nom
  Just "target_volt".
  Done
- Patch Set #18, Line 181: d long target_volt[R

- If we use the real number of regulators for the loop condition, we don't have to check this anymore.

- Done
- Patch Set #18, Line 182: int err, i;
  To avoid so many nested blocks, I'd negate the condition and make it continue to next loop iteration […]
  Done

- Patch Set #18, Line 217:

	voltage_range_check(target_volt);

#ifdef CONFIG_REGULATOR
	if (kbdev->current_freq < freq) {
		err = set_voltages(kbdev, target_volt, 1);
		if (err) {
			dev_err(kbdev->dev, "Failed to increase voltage\n");
			return err;


		}
	}
#endif

	err = clk_set_parent(mfg->clk_mux, mfg->clk_sub_parent);
	if (err) {
		dev_err(kbdev->dev, "Failed to select sub clock src\n");

- ```
		return err;
	}
```
  This is still almost identical to 176-192. Should be possible to move into a common function: […]
  Done
File drivers/gpu/arm/midgard/mali_kbase_core_linux.c:

- Patch Set #11, Line 3217:

- Nope. I mean the code in platform/mediatek.
  Done
File drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c:

- Patch Set #18, Line 261:
  Missing spaces around the operator.

- Done

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 8, 2018, 6:32:34 AM11/8/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 21:Verified +1

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Nov 9, 2018, 8:39:39 AM11/9/18

to Nick Fan, Nick Fan, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch set 21:Code-Review +2

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Tomasz Figa (Gerrit)

unread,

Nov 9, 2018, 8:39:45 AM11/9/18

to Nick Fan, Nick Fan, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Looks much better now. Thanks!

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nick Fan (Gerrit)

unread,

Nov 11, 2018, 8:31:51 PM11/11/18

to Nick Fan, Tomasz Figa, Nicolas Boichat, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Patch Set 21:

Looks much better now. Thanks!

No problem, thanks

View Change

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Nicolas Boichat (Gerrit)

unread,

Jan 2, 2019, 3:42:31 AM1/2/19

to Nick Fan, Nick Fan, Tomasz Figa, Roger Lu, JB Tsai, Eddie Huang, CK HU, Ben Ho, SJ Huang

Nicolas Boichat abandoned this change.

View Change

Abandoned

To view, visit change 1188020. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward

CHROMIUM: MALI: Add Mali platform specific source [chromiumos/third_party/kernel : chromeos-4.14]

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nicolas Boichat (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nicolas Boichat (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Nick Fan (Gerrit)

Tomasz Figa (Gerrit)

Tomasz Figa (Gerrit)

Nick Fan (Gerrit)

Nicolas Boichat (Gerrit)