* Synopsys Ethernet QoS Driver
From: Giuseppe CAVALLARO @ 2016-11-21 15:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <48776f8b-4e06-1456-1b52-3ea08a22b2a4@synopsys.com>
On 11/21/2016 4:00 PM, Joao Pinto wrote:
> On 21-11-2016 14:36, Giuseppe CAVALLARO wrote:
>> Hello Joao
>>
>> On 11/21/2016 2:48 PM, Joao Pinto wrote:
>>> Synopsys QoS IP is a separated hardware component, so it should be reusable by
>>> all implementations using it and so have its own "core driver" and platform +
>>> pci glue drivers. This is necessary for example in hardware validation, where
>>> you prototype an IP and instantiate its drivers and test it.
>>>
>>> Was there a strong reason to integrate QoS features directly in stmmac and not
>>> in synopsys/dwc_eth_qos.*?
>>
>> We decided to enhance the stmmac on supporting the QoS for several
>> reasons; for example the common APIs that the driver already exposed and
>> actually suitable for other SYNP chips. Then, PTP, EEE,
>> S/RGMII, MMC could be shared among different chips with a minimal
>> effort. This meant a lot of code already ready.
>>
>> For sure, the net-core, Ethtool, mdio parts were reused. Same for the
>> glue logic files.
>> For the latter, this helped to easily bring-up new platforms also
>> because the stmmac uses the HW cap register to auto-configure many
>> parts of the MAC core, DMA and modules. This helped many users, AFAIK.
>>
>> For validation purpose, this is my experience, the stmmac helped
>> a lot because people used the same code to validate different HW
>> and it was easy to switch to a platform to another one in order to
>> verify / check if the support was ok or if a regression was introduced.
>> This is important for complex supports like PTP or EEE.
>>
>> Hoping this can help.
>>
>> Do not hesitate to contact me for further details
>
> Thanks for the highly detailed info.
> My target application is to prototype the Ethernet QoS IP in a FPGA, with a PHY
> attached and make hardware validation.
>
> In your opinion a refactored stmmac with the missing QoS features would be
> suitable for it?
I think so; somebody also added code for FPGA.
In any case, step-by-step we can explore and understand
how to proceed. I wonder if you could start looking at the internal
of the stmmac. Then welcome doubts and open question...
>
> Thanks.
welcome
peppe
>
>>
>> peppe
>
>
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-21 15:00 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <937252db-9538-2cf6-c8fa-82b558531c51@st.com>
On 21-11-2016 14:36, Giuseppe CAVALLARO wrote:
> Hello Joao
>
> On 11/21/2016 2:48 PM, Joao Pinto wrote:
>> Synopsys QoS IP is a separated hardware component, so it should be reusable by
>> all implementations using it and so have its own "core driver" and platform +
>> pci glue drivers. This is necessary for example in hardware validation, where
>> you prototype an IP and instantiate its drivers and test it.
>>
>> Was there a strong reason to integrate QoS features directly in stmmac and not
>> in synopsys/dwc_eth_qos.*?
>
> We decided to enhance the stmmac on supporting the QoS for several
> reasons; for example the common APIs that the driver already exposed and
> actually suitable for other SYNP chips. Then, PTP, EEE,
> S/RGMII, MMC could be shared among different chips with a minimal
> effort. This meant a lot of code already ready.
>
> For sure, the net-core, Ethtool, mdio parts were reused. Same for the
> glue logic files.
> For the latter, this helped to easily bring-up new platforms also
> because the stmmac uses the HW cap register to auto-configure many
> parts of the MAC core, DMA and modules. This helped many users, AFAIK.
>
> For validation purpose, this is my experience, the stmmac helped
> a lot because people used the same code to validate different HW
> and it was easy to switch to a platform to another one in order to
> verify / check if the support was ok or if a regression was introduced.
> This is important for complex supports like PTP or EEE.
>
> Hoping this can help.
>
> Do not hesitate to contact me for further details
Thanks for the highly detailed info.
My target application is to prototype the Ethernet QoS IP in a FPGA, with a PHY
attached and make hardware validation.
In your opinion a refactored stmmac with the missing QoS features would be
suitable for it?
Thanks.
>
> peppe
^ permalink raw reply
* [BUG] hdlcd gets confused about base address
From: Russell King - ARM Linux @ 2016-11-21 14:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121143053.GH1041@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 02:30:53PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote:
> > > That is mostly due to the check in hdlcd_crtc_disable() which I should
> > > remove, I've added it because I was getting a ->disable() hook call
> > > before any ->enable() was called at startup time. I need to revisit
> > > this as I remember Daniel was commenting that this was not needed.
> >
> > Removing that test results in:
> >
> > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out
> >
> > and the kernel hanging, seemingly in an IRQs-off region.
>
> Annoyingly, enabling DRM debug prevents the kernel hanging...
I've been trying to trace through what's happening with this flip_done
stuff, but I'm finding it _extremely_ difficult to follow the atomic
code.
(Sorry, I'm going to go over my usual 72 column limit for this due to
the damn long DRM function names.)
I can see that drm_atomic_helper_commit() calls drm_atomic_helper_setup_commit()
which sets up commit->flip_done for each CRTC, and sets up an event for
each.
drm_atomic_helper_commit() continues on to eventually call drm_atomic_helper_swap_state()
which then swaps the state for the CRTCs, but then ends up dropping
the event reference:
state->crtcs[i].commit->event = NULL;
What I can't see is why this isn't a leaked pointer - I don't see
anything inbetween taking charge of that structure. The _commit_
hasn't been swapped from what I can see, it's just state->crtcs[i].state
that have been swapped.
So I can't see who's responsible for generating this event, or how the
backend DRM drivers get to know about this event, and that they should
complete the flip.
What I also don't get is why DRM is wanting to wait for a flip event
when we're disabling the CRTC. None of this makes sense to me, like
much of the atomic modeset code...
(I'm probably never going to convert Armada DRM to atomic modeset, I
just don't seem to be capable of understanding atomic modeset. Maybe
I'm too old?)
[drm:drm_ioctl] pid=2178, dev=0xe200, auth=1, DRM_IOCTL_MODE_RMFB
[drm:drm_mode_object_unreference] OBJ ID: 38 (4)
[drm:drm_atomic_state_init] Allocated atomic state ffffffc974c7c300
[drm:drm_mode_object_reference] OBJ ID: 43 (1)
[drm:drm_atomic_get_crtc_state] Added [CRTC:24:crtc-0] ffffffc975e59400 state to ffffffc974c7c300
[drm:drm_mode_object_reference] OBJ ID: 38 (3)
[drm:drm_atomic_get_plane_state] Added [PLANE:23:plane-0] ffffffc974c7c100 state to ffffffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 43 (2)
[drm:drm_atomic_set_mode_for_crtc] Set [NOMODE] for CRTC state ffffffc975e59400
[drm:drm_atomic_set_crtc_for_plane] Link plane state ffffffc974c7c100 to [NOCRTC]
[drm:drm_mode_object_unreference] OBJ ID: 38 (4)
[drm:drm_atomic_set_fb_for_plane] Set [NOFB] for plane state ffffffc974c7c100
[drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:24:crtc-0] to ffffffc974c7c300
[drm:drm_mode_object_reference] OBJ ID: 26 (4)
[drm:drm_mode_object_reference] OBJ ID: 26 (5)
[drm:drm_atomic_get_connector_state] Added [CONNECTOR:26] ffffffc974c7c000 state to ffffffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 26 (6)
[drm:drm_atomic_set_crtc_for_connector] Link connector state ffffffc974c7c000 to [NOCRTC]
[drm:drm_atomic_check_only] checking ffffffc974c7c300
[drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] mode changed
[drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] enable changed
[drm:drm_atomic_helper_check_modeset] Updating routing for [CONNECTOR:26:HDMI-A-1]
[drm:drm_atomic_helper_check_modeset] Disabling [CONNECTOR:26:HDMI-A-1]
[drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] active changed
[drm:drm_atomic_helper_check_modeset] [CRTC:24:crtc-0] needs all connectors, enable: n, active: n
[drm:drm_atomic_add_affected_connectors] Adding all current connectors for [CRTC:24:crtc-0] to ffffffc974c7c300
[drm:drm_atomic_commit] commiting ffffffc974c7c300
[drm:drm_atomic_helper_commit_modeset_disables] disabling [ENCODER:25:TMDS-25]
[drm:drm_atomic_helper_commit_modeset_disables] disabling [CRTC:24:crtc-0]
hdlcd_crtc_disable: active 0
[drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out
[drm:drm_atomic_state_default_clear] Clearing atomic state ffffffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 26 (5)
[drm:drm_mode_object_unreference] OBJ ID: 26 (4)
[drm:drm_mode_object_unreference] OBJ ID: 43 (1)
[drm:drm_mode_object_unreference] OBJ ID: 38 (3)
[drm:drm_atomic_state_free] Freeing atomic state ffffffc974c7c300
[drm:drm_mode_object_unreference] OBJ ID: 38 (2)
[drm:drm_mode_object_unreference] OBJ ID: 38 (1)
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v6 0/3] Add Mediatek JPEG Decoder
From: Hans Verkuil @ 2016-11-21 14:53 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479353915-5043-1-git-send-email-rick.chang@mediatek.com>
I'm missing a MAINTAINERS patch for this new driver.
Can you post a patch for that?
It's the only thing preventing this from being merged.
Regards,
Hans
On 17/11/16 04:38, Rick Chang wrote:
> This series of patches provide a v4l2 driver to control Mediatek JPEG decoder
> for decoding JPEG image and Motion JPEG bitstream.
>
> changes since v5:
> - remove redundant name from struct mtk_jpeg_fmt
> - Set state of all buffers to VB2_BUF_STATE_QUEUED if fail in start streaming
> - Remove VB2_USERPTR
> - Add check for buffer index
>
> changes since v4:
> - Change file name of binding documentation
> - Revise DT binding documentation
> - Revise compatible string
>
> changes since v3:
> - Revise DT binding documentation
> - Revise compatible string
>
> changes since v2:
> - Revise DT binding documentation
>
> changes since v1:
> - Rebase for v4.9-rc1.
> - Update Compliance test version and result
> - Remove redundant path in Makefile
> - Fix potential build error without CONFIG_PM_RUNTIME and CONFIG_PM_SLEEP
> - Fix warnings from patch check and smatch check
>
> * Dependency
> The patch "arm: dts: mt2701: Add node for JPEG decoder" depends on:
> CCF "Add clock support for Mediatek MT2701"[1]
> iommu and smi "Add the dtsi node of iommu and smi for mt2701"[2]
>
> [1] http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007271.html
> [2] https://patchwork.kernel.org/patch/9164013/
>
> * Compliance test
> v4l2-compliance SHA : 4ad7174b908a36c4f315e3fe2efa7e2f8a6f375a
>
> Driver Info:
> Driver name : mtk-jpeg decode
> Card type : mtk-jpeg decoder
> Bus info : platform:15004000.jpegdec
> Driver version: 4.9.0
> Capabilities : 0x84204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
> Device Capabilities
> Device Caps : 0x04204000
> Video Memory-to-Memory Multiplanar
> Streaming
> Extended Pix Format
>
> Compliance test for device /dev/video3 (not using libv4l2):
>
> Required ioctls:
> test VIDIOC_QUERYCAP: OK
>
> Allow for multiple opens:
> test second video open: OK
> test VIDIOC_QUERYCAP: OK
> test VIDIOC_G/S_PRIORITY: OK
> test for unlimited opens: OK
>
> Debug ioctls:
> test VIDIOC_DBG_G/S_REGISTER: OK (Not Supported)
> test VIDIOC_LOG_STATUS: OK (Not Supported)
>
> Input ioctls:
> test VIDIOC_G/S_TUNER/ENUM_FREQ_BANDS: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_S_HW_FREQ_SEEK: OK (Not Supported)
> test VIDIOC_ENUMAUDIO: OK (Not Supported)
> test VIDIOC_G/S/ENUMINPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDIO: OK (Not Supported)
> Inputs: 0 Audio Inputs: 0 Tuners: 0
>
> Output ioctls:
> test VIDIOC_G/S_MODULATOR: OK (Not Supported)
> test VIDIOC_G/S_FREQUENCY: OK (Not Supported)
> test VIDIOC_ENUMAUDOUT: OK (Not Supported)
> test VIDIOC_G/S/ENUMOUTPUT: OK (Not Supported)
> test VIDIOC_G/S_AUDOUT: OK (Not Supported)
> Outputs: 0 Audio Outputs: 0 Modulators: 0
>
> Input/Output configuration ioctls:
> test VIDIOC_ENUM/G/S/QUERY_STD: OK (Not Supported)
> test VIDIOC_ENUM/G/S/QUERY_DV_TIMINGS: OK (Not Supported)
> test VIDIOC_DV_TIMINGS_CAP: OK (Not Supported)
> test VIDIOC_G/S_EDID: OK (Not Supported)
>
> Control ioctls:
> test VIDIOC_QUERY_EXT_CTRL/QUERYMENU: OK (Not Supported)
> test VIDIOC_QUERYCTRL: OK (Not Supported)
> test VIDIOC_G/S_CTRL: OK (Not Supported)
> test VIDIOC_G/S/TRY_EXT_CTRLS: OK (Not Supported)
> test VIDIOC_(UN)SUBSCRIBE_EVENT/DQEVENT: OK (Not Supported)
> test VIDIOC_G/S_JPEGCOMP: OK (Not Supported)
> Standard Controls: 0 Private Controls: 0
>
> Format ioctls:
> test VIDIOC_ENUM_FMT/FRAMESIZES/FRAMEINTERVALS: OK
> test VIDIOC_G/S_PARM: OK (Not Supported)
> test VIDIOC_G_FBUF: OK (Not Supported)
> test VIDIOC_G_FMT: OK
> test VIDIOC_TRY_FMT: OK
> test VIDIOC_S_FMT: OK
> test VIDIOC_G_SLICED_VBI_CAP: OK (Not Supported)
> test Cropping: OK (Not Supported)
> test Composing: OK
> test Scaling: OK
>
> Codec ioctls:
> test VIDIOC_(TRY_)ENCODER_CMD: OK (Not Supported)
> test VIDIOC_G_ENC_INDEX: OK (Not Supported)
> test VIDIOC_(TRY_)DECODER_CMD: OK (Not Supported)
>
> Buffer ioctls:
> test VIDIOC_REQBUFS/CREATE_BUFS/QUERYBUF: OK
> test VIDIOC_EXPBUF: OK
>
> Test input 0:
>
>
> Total: 43, Succeeded: 43, Failed: 0, Warnings: 0
>
> Rick Chang (3):
> dt-bindings: mediatek: Add a binding for Mediatek JPEG Decoder
> vcodec: mediatek: Add Mediatek JPEG Decoder Driver
> arm: dts: mt2701: Add node for Mediatek JPEG Decoder
>
> .../bindings/media/mediatek-jpeg-decoder.txt | 37 +
> arch/arm/boot/dts/mt2701.dtsi | 14 +
> drivers/media/platform/Kconfig | 15 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/mtk-jpeg/Makefile | 2 +
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 1303 ++++++++++++++++++++
> drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h | 139 +++
> drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c | 417 +++++++
> drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h | 91 ++
> drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c | 160 +++
> drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h | 25 +
> drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h | 58 +
> 12 files changed, 2263 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/media/mediatek-jpeg-decoder.txt
> create mode 100644 drivers/media/platform/mtk-jpeg/Makefile
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.h
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.c
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_hw.h
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.c
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_parse.h
> create mode 100644 drivers/media/platform/mtk-jpeg/mtk_jpeg_reg.h
>
^ permalink raw reply
* [PATCH v6 3/3] arm: dts: mt2701: Add node for Mediatek JPEG Decoder
From: Hans Verkuil @ 2016-11-21 14:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479353915-5043-4-git-send-email-rick.chang@mediatek.com>
On 17/11/16 04:38, Rick Chang wrote:
> Signed-off-by: Rick Chang <rick.chang@mediatek.com>
> Signed-off-by: Minghsiu Tsai <minghsiu.tsai@mediatek.com>
> ---
> This patch depends on:
> CCF "Add clock support for Mediatek MT2701"[1]
> iommu and smi "Add the dtsi node of iommu and smi for mt2701"[2]
>
> [1] http://lists.infradead.org/pipermail/linux-mediatek/2016-October/007271.html
> [2] https://patchwork.kernel.org/patch/9164013/
I assume that 1 & 2 will appear in 4.10? So this patch needs to go in
after the
other two are merged in 4.10?
Regards,
Hans
> ---
> arch/arm/boot/dts/mt2701.dtsi | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
> index 8f13c70..4dd5048 100644
> --- a/arch/arm/boot/dts/mt2701.dtsi
> +++ b/arch/arm/boot/dts/mt2701.dtsi
> @@ -298,6 +298,20 @@
> power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> };
>
> + jpegdec: jpegdec at 15004000 {
> + compatible = "mediatek,mt2701-jpgdec";
> + reg = <0 0x15004000 0 0x1000>;
> + interrupts = <GIC_SPI 143 IRQ_TYPE_LEVEL_LOW>;
> + clocks = <&imgsys CLK_IMG_JPGDEC_SMI>,
> + <&imgsys CLK_IMG_JPGDEC>;
> + clock-names = "jpgdec-smi",
> + "jpgdec";
> + power-domains = <&scpsys MT2701_POWER_DOMAIN_ISP>;
> + mediatek,larb = <&larb2>;
> + iommus = <&iommu MT2701_M4U_PORT_JPGDEC_WDMA>,
> + <&iommu MT2701_M4U_PORT_JPGDEC_BSDMA>;
> + };
> +
> vdecsys: syscon at 16000000 {
> compatible = "mediatek,mt2701-vdecsys", "syscon";
> reg = <0 0x16000000 0 0x1000>;
>
^ permalink raw reply
* [PATCH v16 08/15] clocksource/drivers/arm_arch_timer: Refactor arch_timer_needs_probing, and call it only if acpi disabled.
From: Fu Wei @ 2016-11-21 14:38 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118195657.GM1197@leverpostej>
Hi Mark,
On 19 November 2016 at 03:56, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 16, 2016 at 09:49:01PM +0800, fu.wei at linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> The patch refactor original arch_timer_needs_probing function:
>> (1) Separate out arch_timer_needs_probing from arch_timer_common_init,
>> and call it only if acpi disabled.
>> (2) Rename arch_timer_needs_probing to arch_timer_needs_of_probing.
>
> Please write a real commit message.
OK, how about this:
clocksource/drivers/arm_arch_timer: Refactor
arch_timer_needs_probing, only use it in the DT init code
Currently, the arch_timer_common_init uses arch_timer_needs_probing call to
wait for both timers been probed which is only for booting from DT.
For booting from ACPI, we don't need these in arch_timer_common_init, these code
should be only in the DT init code.
This patch seperates arch_timer_needs_probing related code out into a
new function
named arch_timer_needs_of_probing, and then reworks arch_timer_of_init
to use it,
so that we can keep this only in the DT init code.
>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> ---
>> drivers/clocksource/arm_arch_timer.c | 34 +++++++++++++++++++---------------
>> 1 file changed, 19 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index fe4e812..9ddc091 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -792,15 +792,28 @@ static const struct of_device_id arch_timer_mem_of_match[] __initconst = {
>> {},
>> };
>>
>> -static bool __init
>> -arch_timer_needs_probing(int type, const struct of_device_id *matches)
>> +static bool __init arch_timer_needs_of_probing(void)
>> {
>> struct device_node *dn;
>> bool needs_probing = false;
>> + unsigned int mask = ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM;
>> +
>> + /* We have two timers, and both device-tree nodes are probed. */
>> + if ((arch_timers_present & mask) == mask)
>> + return false;
>> +
>> + /*
>> + * Only one type of timer is probed,
>> + * check if we have another type of timer node in device-tree.
>> + */
>> + if (arch_timers_present & ARCH_TIMER_TYPE_CP15)
>> + dn = of_find_matching_node(NULL, arch_timer_mem_of_match);
>> + else
>> + dn = of_find_matching_node(NULL, arch_timer_of_match);
>>
>> - dn = of_find_matching_node(NULL, matches);
>> - if (dn && of_device_is_available(dn) && !(arch_timers_present & type))
>> + if (dn && of_device_is_available(dn))
>> needs_probing = true;
>> +
>> of_node_put(dn);
>>
>> return needs_probing;
>> @@ -808,17 +821,8 @@ arch_timer_needs_probing(int type, const struct of_device_id *matches)
>>
>> static int __init arch_timer_common_init(void)
>> {
>> - unsigned int mask = ARCH_TIMER_TYPE_CP15 | ARCH_TIMER_TYPE_MEM;
>> -
>> - /* Wait until both nodes are probed if we have two timers */
>> - if ((arch_timers_present & mask) != mask) {
>> - if (arch_timer_needs_probing(ARCH_TIMER_TYPE_MEM,
>> - arch_timer_mem_of_match))
>> - return 0;
>> - if (arch_timer_needs_probing(ARCH_TIMER_TYPE_CP15,
>> - arch_timer_of_match))
>> - return 0;
>> - }
>
> Why can't we just move this into the DT-specific caller of
> arch_timer_common_init()?
yes, I have thought about this, but that means we have to split arch_timer_init,
and use arch_timer_register and arch_timer_common_init directly in
arch_timer_of_init.
In another word, I also need to modify arch_timer_of_init, not only
arch_timer_common_init,
but let me have a try in v17, see if you like it.
>
> Thanks
> Mark.
>
>> + if (acpi_disabled && arch_timer_needs_of_probing())
>> + return 0;
>>
>> arch_timer_banner(arch_timers_present);
>> arch_counter_register(arch_timers_present);
>> --
>> 2.7.4
>>
--
Best regards,
Fu Wei
Software Engineer
Red Hat
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Giuseppe CAVALLARO @ 2016-11-21 14:36 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <afc18eb0-4c11-9668-498d-982624c6de78@synopsys.com>
Hello Joao
On 11/21/2016 2:48 PM, Joao Pinto wrote:
> Synopsys QoS IP is a separated hardware component, so it should be reusable by
> all implementations using it and so have its own "core driver" and platform +
> pci glue drivers. This is necessary for example in hardware validation, where
> you prototype an IP and instantiate its drivers and test it.
>
> Was there a strong reason to integrate QoS features directly in stmmac and not
> in synopsys/dwc_eth_qos.*?
We decided to enhance the stmmac on supporting the QoS for several
reasons; for example the common APIs that the driver already exposed and
actually suitable for other SYNP chips. Then, PTP, EEE,
S/RGMII, MMC could be shared among different chips with a minimal
effort. This meant a lot of code already ready.
For sure, the net-core, Ethtool, mdio parts were reused. Same for the
glue logic files.
For the latter, this helped to easily bring-up new platforms also
because the stmmac uses the HW cap register to auto-configure many
parts of the MAC core, DMA and modules. This helped many users, AFAIK.
For validation purpose, this is my experience, the stmmac helped
a lot because people used the same code to validate different HW
and it was easy to switch to a platform to another one in order to
verify / check if the support was ok or if a regression was introduced.
This is important for complex supports like PTP or EEE.
Hoping this can help.
Do not hesitate to contact me for further details
peppe
^ permalink raw reply
* [BUG] hdlcd gets confused about base address
From: Russell King - ARM Linux @ 2016-11-21 14:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121132419.GF1041@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote:
> > That is mostly due to the check in hdlcd_crtc_disable() which I should
> > remove, I've added it because I was getting a ->disable() hook call
> > before any ->enable() was called at startup time. I need to revisit
> > this as I remember Daniel was commenting that this was not needed.
>
> Removing that test results in:
>
> [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out
>
> and the kernel hanging, seemingly in an IRQs-off region.
Annoyingly, enabling DRM debug prevents the kernel hanging...
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v9 02/10] drm/mediatek: add *driver_data for different hardware settings
From: YT Shen @ 2016-11-21 14:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGS+omAa_SLg8WKfXXvZ-UT6-Mdep28FQVJuuwfskNqP2x50eg@mail.gmail.com>
Hi Daniel,
On Fri, 2016-11-18 at 12:56 +0800, Daniel Kurtz wrote:
> Hi YT,
>
> I don't see a reason to handle device_data in such a generic way at
> the generic mtk_ddp_comp layer.
> The device data is very component specific, so just define different
> structs for different comp types, ie:
>
> struct mtk_disp_ovl_driver_data {
> unsigned int reg_ovl_addr;
> unsigned int fmt_rgb565;
> unsigned int fmt_rgb888;
> };
>
> struct mtk_disp_rdma_driver_data {
> unsigned int fifo_pseudo_size;
> };
>
> struct mtk_disp_color_driver_data {
> unsigned int color_offset;
> };
>
> Then add typed pointers to the local structs that use them, for example:
>
> struct mtk_disp_ovl {
> struct mtk_ddp_comp ddp_comp;
> struct drm_crtc *crtc;
> const struct mtk_disp_ovl_driver_data *data;
> };
>
> And fetch the device specific driver data directly in .probe, as you
> are already doing:
>
> static int mtk_disp_ovl_probe(struct platform_device *pdev) {
> ...
> priv->data = of_device_get_match_data(dev);
> ...
> }
These suggestions make code more readable. We will change ovl and rdma
part, and keep mtk_disp_color_driver_data in its original place.
Because ovl and rdma have its files, other modules share
mtk_drm_ddp_comp.c.
>
> More comments in-line...
>
> On Fri, Nov 11, 2016 at 7:55 PM, YT Shen <yt.shen@mediatek.com> wrote:
> > There are some hardware settings changed, between MT8173 & MT2701:
> > DISP_OVL address offset changed, color format definition changed.
> > DISP_RDMA fifo size changed.
> > DISP_COLOR offset changed.
> > MIPI_TX pll setting changed.
> > And add prefix for mtk_ddp_main & mtk_ddp_ext & mutex_mod.
>
> Nit: I think it would make sense to combine this patch with
> drm/mediatek: rename macros, add chip prefix
Will do.
>
> >
> > Signed-off-by: YT Shen <yt.shen@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 27 ++++++++++++++++-----------
> > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 11 +++++++++--
> > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 11 +++++++----
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 27 +++++++++++++++++++++------
> > drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 13 +++++++++++++
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 25 ++++++++++++++++++-------
> > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 8 ++++++++
> > drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 24 +++++++++++++++++++++++-
> > 8 files changed, 115 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index 019b7ca..1139834 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -35,13 +35,10 @@
> > #define DISP_REG_OVL_PITCH(n) (0x0044 + 0x20 * (n))
> > #define DISP_REG_OVL_RDMA_CTRL(n) (0x00c0 + 0x20 * (n))
> > #define DISP_REG_OVL_RDMA_GMC(n) (0x00c8 + 0x20 * (n))
> > -#define DISP_REG_OVL_ADDR(n) (0x0f40 + 0x20 * (n))
>
> Also, I would still use the "#define macros", for example
> "DISP_REG_OVL_ADDR offsets, and use the named constant in the
> driver_data:
>
> #define DISP_REG_OVL_ADDR_MT8173 0x0f40
>
> (and in a later patch:
> #define DISP_REG_OVL_ADDR_MT2701 0x0040
> )
>
> Also, I would still use the macro rather than open coding the "0x20 *
> (n)", and just pass 'ovl' to the overlay macros that depend on
> hardware type.
> Something like the following:
>
> #define DISP_REG_OVL_ADDR(ovl, n) ((ovl)->data->ovl_addr + 0x20 * (n))
Will use the "#define macros" here.
>
> >
> > #define OVL_RDMA_MEM_GMC 0x40402020
> >
> > #define OVL_CON_BYTE_SWAP BIT(24)
> > -#define OVL_CON_CLRFMT_RGB565 (0 << 12)
> > -#define OVL_CON_CLRFMT_RGB888 (1 << 12)
>
> This seems like a really random and unnecessary hardware change.
> Why chip designers, why!!?!?
There are many reasons for software bugs. Unnecessary hardware change
should be one of them...
>
> For this one, it seems the polarity is either one way or the other, so
> we can just use a bool to distinguish:
>
> bool fmt_rgb565_is_0;
>
> > +static const struct mtk_ddp_comp_driver_data mt8173_ovl_driver_data = {
> > + .ovl = { DISP_REG_OVL_ADDR_MT8173, .fmt_rgb565_is_0 = true }
> > +};
>
> For use at runtime, the defines could become:
>
> #define OVL_CON_CLRFMT_RGB565(ovl) ((ovl)->data->fmt_rgb565_is_0 ? 0
> : OVL_CON_CLRFMT_RGB888)
> #define OVL_CON_CLRFMT_RGB888(ovl) ((ovl)->data->fmt_rgb565_is_0 ?
> OVL_CON_CLRFMT_RGB888 : 0)
OK, will do.
>
> > #define OVL_CON_CLRFMT_RGBA8888 (2 << 12)
> > #define OVL_CON_CLRFMT_ARGB8888 (3 << 12)
> > #define OVL_CON_AEN BIT(8)
> > @@ -137,18 +134,18 @@ static void mtk_ovl_layer_off(struct mtk_ddp_comp *comp, unsigned int idx)
> > writel(0x0, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
> > }
> >
> > -static unsigned int ovl_fmt_convert(unsigned int fmt)
> > +static unsigned int ovl_fmt_convert(struct mtk_ddp_comp *comp, unsigned int fmt)
> > {
> > switch (fmt) {
> > default:
> > case DRM_FORMAT_RGB565:
> > - return OVL_CON_CLRFMT_RGB565;
> > + return comp->data->ovl.fmt_rgb565;
>
> It will be nice to define a helper function for converting from the
> generic 'mtk_ddp_comp' to the specific 'mtk_disp_ovl':
>
> static inline struct mtk_disp_ovl *comp_to_ovl(struct mtk_ddp_comp *comp) {
> return container_of(comp, struct mtk_disp_ovl, ddp_comp);
> }
>
> Then these could become:
> return OVL_CON_CLRFMT_RGB565(comp_to_ovl(comp));
>
> Or maybe cleaner, do the conversion once at the top of the function:
> struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
>
> And then just:
> return OVL_CON_CLRFMT_RGB565(ovl);
>
>
Will use a helper function for the converting.
> > case DRM_FORMAT_BGR565:
> > - return OVL_CON_CLRFMT_RGB565 | OVL_CON_BYTE_SWAP;
> > + return comp->data->ovl.fmt_rgb565 | OVL_CON_BYTE_SWAP;
> > case DRM_FORMAT_RGB888:
> > - return OVL_CON_CLRFMT_RGB888;
> > + return comp->data->ovl.fmt_rgb888;
> > case DRM_FORMAT_BGR888:
> > - return OVL_CON_CLRFMT_RGB888 | OVL_CON_BYTE_SWAP;
> > + return comp->data->ovl.fmt_rgb888 | OVL_CON_BYTE_SWAP;
> > case DRM_FORMAT_RGBX8888:
> > case DRM_FORMAT_RGBA8888:
> > return OVL_CON_CLRFMT_ARGB8888;
>
> [snip]
>
>
> > diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > index 1c366f8..935a8ef 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > @@ -16,6 +16,7 @@
> > #include <linux/delay.h>
> > #include <linux/io.h>
> > #include <linux/module.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/phy/phy.h>
> >
> > @@ -87,6 +88,9 @@
> >
> > #define MIPITX_DSI_PLL_CON2 0x58
> >
> > +#define MIPITX_DSI_PLL_TOP 0x64
> > +#define RG_DSI_MPPLL_PRESERVE (0xff << 8)
> > +
> > #define MIPITX_DSI_PLL_PWR 0x68
> > #define RG_DSI_MPPLL_SDM_PWR_ON BIT(0)
> > #define RG_DSI_MPPLL_SDM_ISO_EN BIT(1)
> > @@ -123,10 +127,15 @@
> > #define SW_LNT2_HSTX_PRE_OE BIT(24)
> > #define SW_LNT2_HSTX_OE BIT(25)
> >
> > +struct mtk_mipitx_data {
> > + const u32 data;
>
> Use a better name, like "mppll_preserve".
OK.
Regards,
yt.shen
>
> Ok, that's it for now.
> Actually, the patch set in general looks pretty good.
>
> -Dan
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Giuseppe CAVALLARO @ 2016-11-21 14:25 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <BFA799B0-BB22-4515-AC14-3544A3167B0A@axis.com>
On 11/21/2016 2:28 PM, Lars Persson wrote:
>
>
>> 21 nov. 2016 kl. 13:53 skrev Giuseppe CAVALLARO <peppe.cavallaro@st.com>:
>>
>> Hello Joao
>>
>>> On 11/21/2016 1:32 PM, Joao Pinto wrote:
>>> Hello,
>>>
>>>> On 21-11-2016 05:29, Rayagond Kokatanur wrote:
>>>>> On Sat, Nov 19, 2016 at 7:26 PM, Rabin Vincent <rabin@rab.in> wrote:
>>>>>> On Fri, Nov 18, 2016 at 02:20:27PM +0000, Joao Pinto wrote:
>>>>>> For now we are interesting in improving the synopsys QoS driver under
>>>>>> /nect/ethernet/synopsys. For now the driver structure consists of a single file
>>>>>> called dwc_eth_qos.c, containing synopsys ethernet qos common ops and platform
>>>>>> related stuff.
>>>>>>
>>>>>> Our strategy would be:
>>>>>>
>>>>>> a) Implement a platform glue driver (dwc_eth_qos_pltfm.c)
>>>>>> b) Implement a pci glue driver (dwc_eth_qos_pci.c)
>>>>>> c) Implement a "core driver" (dwc_eth_qos.c) that would only have Ethernet QoS
>>>>>> related stuff to be reused by the platform / pci drivers
>>>>>> d) Add a set of features to the "core driver" that we have available internally
>>>>>
>>>>> Note that there are actually two drivers in mainline for this hardware:
>>>>>
>>>>> drivers/net/ethernet/synopsis/
>>>>> drivers/net/ethernet/stmicro/stmmac/
>>>>
>>>> Yes the later driver (drivers/net/ethernet/stmicro/stmmac/) supports
>>>> both 3.x and 4.x. It has glue layer for pci, platform, core etc,
>>>> please refer this driver once before you start.
>>>>
>>>> You can start adding missing feature of 4.x in stmmac driver.
>>>
>>> Thanks you all for all the info.
>>> Well, I think we are in a good position to organize the ethernet drivers
>>> concerning Synopsys IPs.
>>>
>>> First of all, in my opinion, it does not make sense to have a ethernet/synopsis
>>> (typo :)) when ethernet/stmicro is also for a synopsys IP. If we have another
>>> vendor using the same IP it should be able to reuse the commonn operations. But
>>> I would put that discussion for later :)
>>>
>>> For now I suggest that for we create ethernet/qos and create there a folder
>>> called dwc (designware controller) where all the synopsys qos IP specific code
>>> in order to be reused for example by ethernet/stmicro/stmmac/. We just have to
>>> figure out a clean interface for "client drivers" like stmmac to interact with
>>> the new qos driver.
>>>
>>> What do you think about this approach?
>>
>> The stmmac drivers run since many years on several platforms
>> (sh4, stm32, arm, x86, mips ...) and it supports an huge of amount of
>> configurations starting from 3.1x to 3.7x databooks.
>>
>> It also supports QoS hardware; for example, 4.00a, 4.10a and 4.20a
>> are fully working.
>>
>> Also the stmmac has platform, device-tree and pcie supports and
>> a lot of maintained glue-logic files.
>>
>> It is fully documented inside the kernel tree.
>>
>> I am happy to have new enhancements from other developers.
>> So, on my side, if you want to spend your time on improving it on your
>> platforms please feel free to do it!
>>
>> Concerning the stmicro/stmmac naming, these come from a really old
>> story and have no issue to adopt new folder/file names.
>>
>> I am also open to merge fixes and changes from ethernet/synopsis.
>> I want to point you on some benchmarks made by Alex some months ago
>> (IIRC) that showed an stmmac winner (due to the several optimizations
>> analyzed and reviewed in this mailing list).
>>
>> Peppe
>>
>
> Hello Joao and others,
>
> As the maintainer of dwc_eth_qos.c I prefer also that we put efforts on the most mature driver, the stmmac.
>
> I hope that the code can migrate into an ethernet/synopsys folder to keep the convention of naming the folder after the vendor. This makes it easy for others to find the driver.
>
> The dwc_eth_qos.c will eventually be removed and its DT binding interface can then be implemented in the stmmac driver.
Thanks Lars, I will be happy to support all you on this transition
and I agree on renaming all.
peppe
> - Lars
>
>>>
>>>
>>>>
>>>>>
>>>>> (See http://lists.openwall.net/netdev/2016/02/29/127)
>>>>>
>>>>> The former only supports 4.x of the hardware.
>>>>>
>>>>> The later supports 4.x and 3.x and already has a platform glue driver
>>>>> with support for several platforms, a PCI glue driver, and a core driver
>>>>> with several features not present in the former (for example: TX/RX
>>>>> interrupt coalescing, EEE, PTP).
>>>>>
>>>>> Have you evaluated both drivers? Why have you decided to work on the
>>>>> former rather than the latter?
>>>>
>>>>
>>>
>>> Thanks.
>>>
>>>
>>>
>>>
>>
>
^ permalink raw reply
* [PATCH v16 07/15] clocksource/drivers/arm_arch_timer: Refactor arch_timer_detect_rate to keep dt code in *_of_init
From: Fu Wei @ 2016-11-21 14:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161118195244.GL1197@leverpostej>
Hi Mark
On 19 November 2016 at 03:52, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Nov 16, 2016 at 09:49:00PM +0800, fu.wei at linaro.org wrote:
>> From: Fu Wei <fu.wei@linaro.org>
>>
>> The patch refactor original arch_timer_detect_rate function:
>> (1) Separate out device-tree code, keep them in device-tree init
>> function:
>> arch_timer_of_init,
>> arch_timer_mem_init;
>
> Please write a real commit message.
Sorry, will do
Since this patch will be separated into two patches.
the first patch will be separating out device-tree code, so commit
message can be:
-----------------
clocksource/drivers/arm_arch_timer: Separate out device-tree code from
arch_timer_detect_rate
Currently, the arch_timer_detect_rate can get system counter frequency
from device-tree, sysreg timers and MMIO timers. This is unnecessary and
confusing. For ACPI, we don't need a function included device-tree code.
This patch factors the device-tree related code out into device-tree
init function:
arch_timer_of_init and arch_timer_mem_init.
-----------------
the second patch will be split arch_timer_detect_rate in two, one is
for the MMIO timer,
another is for the CP15 timers, so commit message can be:
-----------------
clocksource/drivers/arm_arch_timer:split arch_timer_detect_rate for
the MMIO and CP15 timers
The arch_timer_detect_rate can get system counter frequency sysreg timers and
MMIO timers. This is unnecessary. For initializing sysreg timers, we
shouldn't try to
access MMIO timers.
This patch split arch_timer_detect_rate into two function:
arch_timer_detect_rate and arch_timer_mem_detect_rate.
-----------------
Please correct me if these commit message are inappropriate.
Great thanks
>
>> (2) Improve original mechanism, if getting from memory-mapped timer
>> fail, try arch_timer_get_cntfrq() again.
>
> This is *not* a refactoring. It's completely unrelated to the supposed
> refactoring from point (1), and if necessary, should be a separate
> patch.
>
> *Why* are you maknig this change? Does some ACPI platform have an MMIO
> timer with an ill-configured CNTFRQ register? If so, report that to the
> vendor. Don't add yet another needless bodge.
>
> I'd really like to split the MMIO and CP15 timers, and this is yet
> another hack that'll make it harder to do so.
you are right, I should split this founction for the MMIO and CP15 timers
>
>> Signed-off-by: Fu Wei <fu.wei@linaro.org>
>> ---
>> drivers/clocksource/arm_arch_timer.c | 45 +++++++++++++++++++++++-------------
>> 1 file changed, 29 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index af22953..fe4e812 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -487,27 +487,31 @@ static int arch_timer_starting_cpu(unsigned int cpu)
>> return 0;
>> }
>>
>> -static void
>> -arch_timer_detect_rate(void __iomem *cntbase, struct device_node *np)
>> +static void arch_timer_detect_rate(void __iomem *cntbase)
>> {
>> /* Who has more than one independent system counter? */
>> if (arch_timer_rate)
>> return;
>> -
>> /*
>> - * Try to determine the frequency from the device tree or CNTFRQ,
>> - * if ACPI is enabled, get the frequency from CNTFRQ ONLY.
>> + * If we got memory-mapped timer(cntbase != NULL),
>> + * try to determine the frequency from CNTFRQ in memory-mapped timer.
>> */
>
> *WHY* ?
>
> If we're sharing arch_timer_rate across MMIO and sysreg timers, the
> sysreg value is alreayd sufficient.
yes, we are sharing arch_timer_rate across MMIO and sysreg timers,
But for booting with device-tree, we can't make sure which timer will
be initialized first,
so we may need :
if (arch_timer_rate)
return;
>
> If we're not, they should be completely independent.
>
>> - if (!acpi_disabled ||
>> - of_property_read_u32(np, "clock-frequency", &arch_timer_rate)) {
>> - if (cntbase)
>> - arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>> - else
>> - arch_timer_rate = arch_timer_get_cntfrq();
>> - }
>> + if (cntbase)
>> + arch_timer_rate = readl_relaxed(cntbase + CNTFRQ);
>> + /*
>> + * Because in a system that implements both Secure and
>> + * Non-secure states, CNTFRQ is only accessible in Secure state.
>
> That's true for the CNTCTLBase frame, but that doesn't matter.
>
> The CNTBase<n> frames should have a readable CNTFRQ.
Sorry, maybe I misunderstand the ARM doc, but in I3.5.7
CNTFRQ, Counter-timer Frequency, it says:
-----------------
For the CNTBaseN and CNTEL0BaseN frames:
? A RO copy of CNTFRQ is implemented in the CNTBaseN frame when both:
? CNTACR<n>.RFRQ is 1.
? Bit[0] of CNTTIDR.Frame<n> is 1.
Otherwise the encoding in CNTBaseN is RES 0.
? When CNTFRQ is RO in the CNTBaseN frame, it is also RO in the
CNTEL0BaseN frame
if bit[2] of CNTTIDR.Frame<n> is 1 and either:
? The value of CNTEL0ACR.EL0VCTEN is 1.
? The value of CNTEL0ACR.EL0PCTEN is 1.
Otherwise the CNTFRQ encoding in CNTEL0BaseN frame is RES 0.
In a system that implements both Secure and Non-secure states, this
register is only accessible by
Secure accesses.
-----------------
So I think this is for both CNTBaseN and CNTEL0BaseN frames?
Please correct me.
When I tested my patchset on Foundation model, I got "0" from
CNTBaseN's CNTFRQ, so mybe is not implemented?
>
>> + * So the operation above may fail, even if (cntbase != NULL),
>> + * especially on ARM64.
>> + * In this case, we can try cntfrq_el0(system coprocessor register).
>> + */
>> + if (!arch_timer_rate)
>> + arch_timer_rate = arch_timer_get_cntfrq();
>> + else
>> + return;
>
> Urrgh.
>
> Please have separate paths to determine the MMIO frequency and the
> sysreg frequency, and use the appropriate one for the counter you want
> to know the frequency of.
OK, will do
>
> Thanks,
> Mark.
--
Best regards,
Fu Wei
Software Engineer
Red Hat
^ permalink raw reply
* [BUG] hdlcd gets confused about base address
From: Russell King - ARM Linux @ 2016-11-21 14:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121135031.GK1005@e106497-lin.cambridge.arm.com>
On Mon, Nov 21, 2016 at 01:50:31PM +0000, Liviu Dudau wrote:
> On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote:
> > On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote:
> > > That is mostly due to the check in hdlcd_crtc_disable() which I should
> > > remove, I've added it because I was getting a ->disable() hook call
> > > before any ->enable() was called at startup time. I need to revisit
> > > this as I remember Daniel was commenting that this was not needed.
> >
> > Removing that test results in:
> >
> > [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out
> >
> > and the kernel hanging, seemingly in an IRQs-off region.
>
> Right, I need to sort this one out. Are you doing these tests out of
> some tagged branch that I can get in sync with?
No, not yet, and some of the changes I have are rather hacky.
I do always build my full tree of patches (which is currently running at
around 320 patches at the moment) but I never share that entire patch
set. However, none of those touch i2c (apart from the ones I've recently
posted) and the only patches touching hdlcd are those I've posted so far.
Most of the problems I'm finding are through trying basic stuff - I'm not
doing anything special or unusual to find them, at the moment quite
literally just starting Xorg up and shutting it down. For example, the
above was caused by logging in on serial, running:
Xorg -terminate -verbose
and then hitting ^C. (I have lxdm disabled so systemd boots to VT login
prompts on both the "framebuffer" and serial - I don't want Xorg coming
up when the machine is booting for its nightly KVM boot tests.)
I'm afraid that when I try someone elses code, I have a tendency to find
loads of seemingly trivial bugs when I try putting it through some basic
tests.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v3] ARM: at91/dt: add dts file for sama5d36ek CMP board
From: Alexandre Belloni @ 2016-11-21 14:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479705283-3052-1-git-send-email-wenyou.yang@atmel.com>
Hi,
I fixed it up this time but please use the proper subject prefix next
time (ARM: dts: at91:).
On 21/11/2016 at 13:14:42 +0800, Wenyou Yang wrote :
> The sama5d36ek CMP board is the variant of sama5d3xek board.
> It is equipped with the low-power DDR2 SDRAM, PMIC ACT8865 and
> some power rail. Its main purpose is used to measure the power
> consumption.
> The difference of the sama5d36ek CMP dts from sama5d36ek dts is
> listed as below.
> 1. The USB host nodes are removed, that is, the USB host is disabled.
> 2. The gpio_keys node is added to wake up from the sleep.
> 3. The LCD isn't supported due to the pins for LCD are conflicted
> with gpio_keys.
> 4. The adc0 node support the pinctrl sleep state to fix the over
> consumption on VDDANA.
>
> As said in errata, "When the USB host ports are used in high speed
> mode (EHCI), it is not possible to suspend the ports if no device is
> attached on each port. This leads to increased power consumption even
> if the system is in a low power mode." That is why the the USB host
> is disabled.
>
> Signed-off-by: Wenyou Yang <wenyou.yang@atmel.com>
> ---
>
> Changes in v3:
> - Use a dual license scheme for DT files.
> - Use the proper model name and the compatible string to reflect
> the nature of this new "CMP" board.
> - Change name of wakeup property to "wakeup-source".
> - Remove unnecessary comments.
> - Remove bootargs.
>
> Changes in v2:
> - Add the pinctrl sleep state for adc0 node to fix the over
> consumption on VDDANA.
> - Improve the commit log.
>
> arch/arm/boot/dts/sama5d36ek_cmp.dts | 87 ++++++++++
> arch/arm/boot/dts/sama5d3xcm_cmp.dtsi | 201 +++++++++++++++++++++++
> arch/arm/boot/dts/sama5d3xmb_cmp.dtsi | 301 ++++++++++++++++++++++++++++++++++
> 3 files changed, 589 insertions(+)
> create mode 100644 arch/arm/boot/dts/sama5d36ek_cmp.dts
> create mode 100644 arch/arm/boot/dts/sama5d3xcm_cmp.dtsi
> create mode 100644 arch/arm/boot/dts/sama5d3xmb_cmp.dtsi
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH] ARM: dts: at91: sama5d3_uart: fix reg sizes to match documentation
From: Alexandre Belloni @ 2016-11-21 14:01 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1478764000-32233-1-git-send-email-peda@axentia.se>
On 10/11/2016 at 08:46:40 +0100, Peter Rosin wrote :
> The new size (0x100) also matches the size given in sama5d3.dtsi
>
> Documentation reference: section 43.6 "Universal Asynchronous
> Receiver Transmitter (UART) User Interface", table 43-4 "Register
> Mapping" in [1].
>
> [1] Atmel-11121F-ATARM-SAMA5D3-Series-Datasheet_02-Feb-16
>
> Signed-off-by: Peter Rosin <peda@axentia.se>
> ---
> arch/arm/boot/dts/sama5d3_uart.dtsi | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Applied, thanks.
--
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
^ permalink raw reply
* [PATCH v9 09/10] drm/mediatek: update DSI sub driver flow for sending commands to panel
From: YT Shen @ 2016-11-21 13:59 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAGS+omBCN5BpYZFozkjpnGPHUS2DUZw+B3Vo9LVZYyHLGD+iKQ@mail.gmail.com>
Hi Daniel,
Thanks for the review.
On Fri, 2016-11-18 at 11:21 +0800, Daniel Kurtz wrote:
> Hi YT,
>
> Sorry for the very late review.
>
> My biggest problem with this patch is it describes itself as adding
> support for a new use case "DSI -> panel", but makes many changes to
> the existing working flow "DSI -> bridge -> panel".
> If these changes are really needed, or improve the existing flow, I'd
> expect to see those changes added first in a preparatory patch,
> followed by a second smaller, simpler
> patch that adds any additional functionality required to enable the new flow.
We will split this patch into several smaller preparatory patches
necessary in the next version.
>
> See detailed comments inline.
>
>
> On Fri, Nov 11, 2016 at 7:55 PM, YT Shen <yt.shen@mediatek.com> wrote:
> >
> > This patch update enable/disable flow of DSI module and MIPI TX module.
> > Original flow works on there is a bridge chip: DSI -> bridge -> panel.
> > In this case: DSI -> panel, the DSI sub driver flow should be updated.
> > We need to initialize DSI first so that we can send commands to panel.
> >
> > Signed-off-by: shaoming chen <shaoming.chen@mediatek.com>
> > Signed-off-by: YT Shen <yt.shen@mediatek.com>
> > ---
> > drivers/gpu/drm/mediatek/mtk_dsi.c | 110 ++++++++++++++++++++++++++-------
> > drivers/gpu/drm/mediatek/mtk_mipi_tx.c | 32 +++++-----
> > 2 files changed, 103 insertions(+), 39 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_dsi.c b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > index 860b84f..12a1206 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_dsi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dsi.c
> > @@ -94,6 +94,8 @@
> > #define DSI_RACK 0x84
> > #define RACK BIT(0)
> >
> > +#define DSI_MEM_CONTI 0x90
> > +
> > #define DSI_PHY_LCCON 0x104
> > #define LC_HS_TX_EN BIT(0)
> > #define LC_ULPM_EN BIT(1)
> > @@ -126,6 +128,10 @@
> > #define CLK_HS_POST (0xff << 8)
> > #define CLK_HS_EXIT (0xff << 16)
> >
> > +#define DSI_VM_CMD_CON 0x130
> > +#define VM_CMD_EN BIT(0)
> > +#define TS_VFP_EN BIT(5)
> > +
> > #define DSI_CMDQ0 0x180
> > #define CONFIG (0xff << 0)
> > #define SHORT_PACKET 0
> > @@ -219,12 +225,12 @@ static void mtk_dsi_phy_timconfig(struct mtk_dsi *dsi)
> > writel(timcon3, dsi->regs + DSI_PHY_TIMECON3);
> > }
> >
> > -static void mtk_dsi_enable(struct mtk_dsi *dsi)
> > +static void mtk_dsi_engine_enable(struct mtk_dsi *dsi)
>
> I don't think we need to change these names.
OK.
>
> > {
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, DSI_EN);
> > }
> >
> > -static void mtk_dsi_disable(struct mtk_dsi *dsi)
> > +static void mtk_dsi_engine_disable(struct mtk_dsi *dsi)
> > {
> > mtk_dsi_mask(dsi, DSI_CON_CTRL, DSI_EN, 0);
> > }
> > @@ -249,7 +255,9 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > * mipi_ratio is mipi clk coefficient for balance the pixel clk in mipi.
> > * we set mipi_ratio is 1.05.
> > */
> > - dsi->data_rate = dsi->vm.pixelclock * 3 * 21 / (1 * 1000 * 10);
> > + dsi->data_rate = dsi->vm.pixelclock * 12 * 21;
> > + dsi->data_rate /= (dsi->lanes * 1000 * 10);
> > + dev_info(dev, "set mipitx's data rate: %dMHz\n", dsi->data_rate);
>
> I don't think we want to spam the log like this. Use dev_dbg.... or
> use the DRM_() messaging like elsewhere in this driver?
OK, we will remove logs like this in the patch series.
>
> >
> > ret = clk_set_rate(dsi->hs_clk, dsi->data_rate * 1000000);
> > if (ret < 0) {
> > @@ -271,7 +279,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > goto err_disable_engine_clk;
> > }
> >
> > - mtk_dsi_enable(dsi);
> > + mtk_dsi_engine_enable(dsi);
> > mtk_dsi_reset_engine(dsi);
> > mtk_dsi_phy_timconfig(dsi);
> >
> > @@ -289,7 +297,7 @@ static int mtk_dsi_poweron(struct mtk_dsi *dsi)
> > static void mtk_dsi_clk_ulp_mode_enter(struct mtk_dsi *dsi)
> > {
> > mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_HS_TX_EN, 0);
> > - mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, 0);
> > + mtk_dsi_mask(dsi, DSI_PHY_LCCON, LC_ULPM_EN, LC_ULPM_EN);
>
> What does this change do?
> It looks like a pure bug fix (ie, previoulsy we were'nt actually
> enabling ULP MODE before).
> If so, can you please move it to a separate preliminary patch.
OK.
>
> > }
> >
> > static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)
> > @@ -302,7 +310,7 @@ static void mtk_dsi_clk_ulp_mode_leave(struct mtk_dsi *dsi)
> > static void mtk_dsi_lane0_ulp_mode_enter(struct mtk_dsi *dsi)
> > {
> > mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_HS_TX_EN, 0);
> > - mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, 0);
> > + mtk_dsi_mask(dsi, DSI_PHY_LD0CON, LD0_ULPM_EN, LD0_ULPM_EN);
>
> Same here.
>
> > }
> >
> > static void mtk_dsi_lane0_ulp_mode_leave(struct mtk_dsi *dsi)
> > @@ -338,11 +346,21 @@ static void mtk_dsi_set_mode(struct mtk_dsi *dsi)
> > if ((dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) &&
> > !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_SYNC_PULSE))
> > vid_mode = BURST_MODE;
> > + else
> > + vid_mode = SYNC_EVENT_MODE;
>
> So, when do we use SYNC_PULSE_MODE (set just before the 'if')?
We will update this part.
>
> > }
> >
> > writel(vid_mode, dsi->regs + DSI_MODE_CTRL);
> > }
> >
> > +static void mtk_dsi_set_vm_cmd(struct mtk_dsi *dsi)
> > +{
> > + writel(0x3c, dsi->regs + DSI_MEM_CONTI);
>
> Please use #defined constants, especially if this register is a bit field.
> Also, this looks like new behavior which doesn't seem related to
> changing the enable order.
> If this is a general fix, please use a separate patch.
We will remove this part. This change is not necessary.
>
> > +
> > + mtk_dsi_mask(dsi, DSI_VM_CMD_CON, VM_CMD_EN, VM_CMD_EN);
> > + mtk_dsi_mask(dsi, DSI_VM_CMD_CON, TS_VFP_EN, TS_VFP_EN);
> > +}
> > +
> > static void mtk_dsi_ps_control_vact(struct mtk_dsi *dsi)
> > {
> > struct videomode *vm = &dsi->vm;
> > @@ -399,6 +417,9 @@ static void mtk_dsi_rxtx_control(struct mtk_dsi *dsi)
> > break;
> > }
> >
> > + tmp_reg |= (dsi->mode_flags & MIPI_DSI_CLOCK_NON_CONTINUOUS) << 6;
> > + tmp_reg |= (dsi->mode_flags & MIPI_DSI_MODE_EOT_PACKET) >> 3;
> > +
>
> ditto
>
> > writel(tmp_reg, dsi->regs + DSI_TXRX_CTRL);
> > }
> >
> > @@ -477,6 +498,16 @@ static void mtk_dsi_start(struct mtk_dsi *dsi)
> > writel(1, dsi->regs + DSI_START);
> > }
> >
> > +static void mtk_dsi_stop(struct mtk_dsi *dsi)
> > +{
> > + writel(0, dsi->regs + DSI_START);
> > +}
> > +
> > +static void mtk_dsi_set_cmd_mode(struct mtk_dsi *dsi)
> > +{
> > + writel(CMD_MODE, dsi->regs + DSI_MODE_CTRL);
> > +}
> > +
> > static void mtk_dsi_set_interrupt_enable(struct mtk_dsi *dsi)
> > {
> > u32 inten = LPRX_RD_RDY_INT_FLAG | CMD_DONE_INT_FLAG | VM_DONE_INT_FLAG;
> > @@ -506,7 +537,7 @@ static s32 mtk_dsi_wait_for_irq_done(struct mtk_dsi *dsi, u32 irq_flag,
> > if (ret == 0) {
> > dev_info(dsi->dev, "Wait DSI IRQ(0x%08x) Timeout\n", irq_flag);
> >
> > - mtk_dsi_enable(dsi);
> > + mtk_dsi_engine_enable(dsi);
> > mtk_dsi_reset_engine(dsi);
> > }
> >
> > @@ -535,6 +566,17 @@ static irqreturn_t mtk_dsi_irq(int irq, void *dev_id)
> > return IRQ_HANDLED;
> > }
> >
> > +static s32 mtk_dsi_switch_to_cmd_mode(struct mtk_dsi *dsi, u8 irq_flag, u32 t)
> > +{
> > + mtk_dsi_irq_data_clear(dsi, irq_flag);
> > + mtk_dsi_set_cmd_mode(dsi);
> > +
> > + if (!mtk_dsi_wait_for_irq_done(dsi, irq_flag, t))
> > + return -1;
>
> No, use a real linux errno, and return an int, and print an error
> message if this is unexpected.
Will use a real errno: ETIME.
>
> > + else
> > + return 0;
> > +}
> > +
> > static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
> > {
> > if (WARN_ON(dsi->refcount == 0))
> > @@ -543,11 +585,6 @@ static void mtk_dsi_poweroff(struct mtk_dsi *dsi)
> > if (--dsi->refcount != 0)
> > return;
> >
> > - mtk_dsi_lane0_ulp_mode_enter(dsi);
> > - mtk_dsi_clk_ulp_mode_enter(dsi);
> > -
> > - mtk_dsi_disable(dsi);
> > -
> > clk_disable_unprepare(dsi->engine_clk);
> > clk_disable_unprepare(dsi->digital_clk);
> >
> > @@ -561,35 +598,45 @@ static void mtk_output_dsi_enable(struct mtk_dsi *dsi)
> > if (dsi->enabled)
> > return;
> >
> > - if (dsi->panel) {
> > - if (drm_panel_prepare(dsi->panel)) {
> > - DRM_ERROR("failed to setup the panel\n");
> > - return;
> > - }
> > - }
> > -
> > ret = mtk_dsi_poweron(dsi);
> > if (ret < 0) {
> > DRM_ERROR("failed to power on dsi\n");
> > return;
> > }
> >
> > + usleep_range(20000, 21000);
> > +
>
> Why are you adding a 20 ms delay where there was none before?
After checking, we will remove redundant codes and the delay.
>
> > mtk_dsi_rxtx_control(dsi);
> > + mtk_dsi_phy_timconfig(dsi);
> > + mtk_dsi_ps_control_vact(dsi);
> > + mtk_dsi_set_vm_cmd(dsi);
> > + mtk_dsi_config_vdo_timing(dsi);
> > + mtk_dsi_set_interrupt_enable(dsi);
> >
> > + mtk_dsi_engine_enable(dsi);
> > mtk_dsi_clk_ulp_mode_leave(dsi);
> > mtk_dsi_lane0_ulp_mode_leave(dsi);
> > mtk_dsi_clk_hs_mode(dsi, 0);
> > - mtk_dsi_set_mode(dsi);
> >
> > - mtk_dsi_ps_control_vact(dsi);
> > - mtk_dsi_config_vdo_timing(dsi);
> > - mtk_dsi_set_interrupt_enable(dsi);
> > + if (dsi->panel) {
> > + if (drm_panel_prepare(dsi->panel)) {
> > + DRM_ERROR("failed to prepare the panel\n");
> > + return;
> > + }
> > + }
> >
> > mtk_dsi_set_mode(dsi);
> > mtk_dsi_clk_hs_mode(dsi, 1);
> >
> > mtk_dsi_start(dsi);
> >
> > + if (dsi->panel) {
> > + if (drm_panel_enable(dsi->panel)) {
> > + DRM_ERROR("failed to enable the panel\n");
>
> In case of error, you must undo everything done to this point. At least:
> (1) unprepare the panel
> (2) stop dsi
> (3) poweroff dsi
OK.
>
> > + return;
> > + }
> > + }
> > +
> > dsi->enabled = true;
> > }
> >
> > @@ -605,6 +652,21 @@ static void mtk_output_dsi_disable(struct mtk_dsi *dsi)
> > }
> > }
> >
> > + mtk_dsi_stop(dsi);
> > + mtk_dsi_switch_to_cmd_mode(dsi, VM_DONE_INT_FLAG, 500);
>
> This function can return an error, so please check it. Although,
> there probably isn't much you can do here about it.
OK.
>
> > +
> > + if (dsi->panel) {
> > + if (drm_panel_unprepare(dsi->panel)) {
> > + DRM_ERROR("failed to unprepare the panel\n");
> > + return;
>
> I think you should probably just ignore this error and continue
> disabling dsi, since it isn't really recoverable and you can't roll
> back and re-enable dsi.
OK.
>
>
> > + }
> > + }
> > +
> > + mtk_dsi_reset_engine(dsi);
> > + mtk_dsi_lane0_ulp_mode_enter(dsi);
> > + mtk_dsi_clk_ulp_mode_enter(dsi);
> > + mtk_dsi_engine_disable(dsi);
> > +
> > mtk_dsi_poweroff(dsi);
> >
> > dsi->enabled = false;
> > @@ -845,7 +907,7 @@ static void mtk_dsi_wait_for_idle(struct mtk_dsi *dsi)
> > if (timeout_ms == 0) {
> > dev_info(dsi->dev, "polling dsi wait not busy timeout!\n");
> >
> > - mtk_dsi_enable(dsi);
> > + mtk_dsi_engine_enable(dsi);
> > mtk_dsi_reset_engine(dsi);
> > }
> > }
> > diff --git a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > index 108d31a..34e95c6 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_mipi_tx.c
> > @@ -177,7 +177,9 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> >
> > dev_dbg(mipi_tx->dev, "prepare: %u Hz\n", mipi_tx->data_rate);
> >
> > - if (mipi_tx->data_rate >= 500000000) {
> > + if (mipi_tx->data_rate > 1250000000) {
> > + return -EINVAL;
> > + } else if (mipi_tx->data_rate >= 500000000) {
>
> Capping the max data rate looks like an unrelated fix.
Will prepare additional patch for max data rate.
>
> > txdiv = 1;
> > txdiv0 = 0;
> > txdiv1 = 0;
> > @@ -201,6 +203,10 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> > return -EINVAL;
> > }
> >
> > + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,
> > + RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
> > + (8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
> > +
> > mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_BG_CON,
> > RG_DSI_VOUT_MSK |
> > RG_DSI_BG_CKEN | RG_DSI_BG_CORE_EN,
> > @@ -210,24 +216,18 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> >
> > usleep_range(30, 100);
> >
> > - mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_TOP_CON,
> > - RG_DSI_LNT_IMP_CAL_CODE | RG_DSI_LNT_HS_BIAS_EN,
> > - (8 << 4) | RG_DSI_LNT_HS_BIAS_EN);
> > -
> > - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_CON,
> > - RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
> > + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_CON,
> > + RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN,
> > + RG_DSI_CKG_LDOOUT_EN | RG_DSI_LDOCORE_EN);
>
> Changing from set_bits to update_bits does not do anything. Please
> leave this alone.
OK.
>
> >
> > mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_PWR,
> > RG_DSI_MPPLL_SDM_PWR_ON |
> > RG_DSI_MPPLL_SDM_ISO_EN,
> > RG_DSI_MPPLL_SDM_PWR_ON);
> >
> > - mtk_mipi_tx_clear_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> > - RG_DSI_MPPLL_PLL_EN);
> > -
>
> Why don't you need to disable the PLL first now?
Yes, we need. Will fix this.
>
> > mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> > - RG_DSI_MPPLL_TXDIV0 | RG_DSI_MPPLL_TXDIV1 |
> > - RG_DSI_MPPLL_PREDIV,
> > + RG_DSI_MPPLL_PREDIV | RG_DSI_MPPLL_TXDIV0 |
> > + RG_DSI_MPPLL_TXDIV1 | RG_DSI_MPPLL_POSDIV,
> > (txdiv0 << 3) | (txdiv1 << 5));
>
> If I read this right, the only thing you are changing is clearing
> "RG_DSI_MPPLL_POSDIV".
> This would be more clear if you kept the field order: TXDIV0, TXDIV1, PREDIV.
>
> And why are you making this change in this patch?
Hmm, we will provide another patch for this part if necessary.
Sometimes settings are changed not in kernel stage (maybe display from
bootloader) This change just make sure kernel have the right
configuration.
>
>
> >
> > /*
> > @@ -242,10 +242,12 @@ static int mtk_mipi_tx_pll_prepare(struct clk_hw *hw)
> > 26000000);
> > writel(pcw, mipi_tx->regs + MIPITX_DSI_PLL_CON2);
> >
> > - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON1,
> > - RG_DSI_MPPLL_SDM_FRA_EN);
> > + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON1,
> > + RG_DSI_MPPLL_SDM_FRA_EN,
> > + RG_DSI_MPPLL_SDM_FRA_EN);
>
> AFAICT, this change does not do anything but make the code more confusing.
OK.
>
> >
> > - mtk_mipi_tx_set_bits(mipi_tx, MIPITX_DSI_PLL_CON0, RG_DSI_MPPLL_PLL_EN);
> > + mtk_mipi_tx_update_bits(mipi_tx, MIPITX_DSI_PLL_CON0,
> > + RG_DSI_MPPLL_PLL_EN, RG_DSI_MPPLL_PLL_EN);
>
> AFAICT, this change does not do anything but make the code more confusing.
OK.
>
> >
> > usleep_range(20, 100);
> >
> > --
> > 1.9.1
> >
^ permalink raw reply
* [PATCH] PCI: Add information about describing PCI in ACPI
From: Bjorn Helgaas @ 2016-11-21 13:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAJZ5v0h3ax4+UCm57VYqV3SVzOZ3A5Ajykks10J49rj1O4VkvQ@mail.gmail.com>
On Sat, Nov 19, 2016 at 12:02:24AM +0100, Rafael J. Wysocki wrote:
> On Thu, Nov 17, 2016 at 6:59 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > Add a writeup about how PCI host bridges should be described in ACPI
> > using PNP0A03/PNP0A08 devices, PNP0C02 devices, and the MCFG table.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Looks great overall, but I have a few comments (below).
Thanks a lot for taking a look, Rafael! I applied all your suggestions.
Bjorn
^ permalink raw reply
* [PATCH 8/9] mtd: nand: mxc: implement onfi get/set features
From: Boris Brezillon @ 2016-11-21 13:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1473928373-8680-9-git-send-email-s.hauer@pengutronix.de>
Hi Sascha,
On Thu, 15 Sep 2016 10:32:52 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:
> To be able to support different ONFI timing modes we have to implement
> the onfi_set_features and onfi_get_features. Tested on an i.MX25 SoC.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> drivers/mtd/nand/mxc_nand.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
>
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 5173fad..1db8299 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -1239,6 +1239,57 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
> }
> }
>
> +static int mxc_nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
> + int addr, uint8_t *subfeature_param)
> +{
> + struct nand_chip *nand_chip = mtd_to_nand(mtd);
> + struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
> + int i;
> +
> + if (!chip->onfi_version ||
> + !(le16_to_cpu(chip->onfi_params.opt_cmd)
> + & ONFI_OPT_CMD_SET_GET_FEATURES))
> + return -EINVAL;
> +
> + host->buf_start = 0;
> +
> + for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> + chip->write_byte(mtd, subfeature_param[i]);
> +
> + memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
> + host->devtype_data->send_cmd(host, NAND_CMD_SET_FEATURES, false);
> + mxc_do_addr_cycle(mtd, addr, -1);
> + host->devtype_data->send_page(mtd, NFC_INPUT);
I've been working with an mx27 board embedding a NAND device lately,
and had a closer look at the NAND controller IP.
With this IP, you're not able to send only 4 bytes of data, and I'm
sure sure what you're doing here (sending a full page of data) works
for a SET_FEATURE command.
Do you have a way to test it (my NAND is not ONFI compliant)? By test
it, I mean, set a timing mode using SET_FEATURE and check if the new
mode has been applied using GET_FEATURE.
> +
> + return 0;
> +}
> +
> +static int mxc_nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
> + int addr, uint8_t *subfeature_param)
> +{
> + struct nand_chip *nand_chip = mtd_to_nand(mtd);
> + struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
> + int i;
> +
> + if (!chip->onfi_version ||
> + !(le16_to_cpu(chip->onfi_params.opt_cmd)
> + & ONFI_OPT_CMD_SET_GET_FEATURES))
> + return -EINVAL;
> +
> + *(uint32_t *)host->main_area0 = 0xdeadbeef;
> +
> + host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
> + mxc_do_addr_cycle(mtd, addr, -1);
> + host->devtype_data->send_page(mtd, NFC_OUTPUT);
> + memcpy32_fromio(host->data_buf, host->main_area0, 512);
> + host->buf_start = 0;
> +
> + for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
> + *subfeature_param++ = chip->read_byte(mtd);
> +
> + return 0;
> +}
> +
> /*
> * The generic flash bbt decriptors overlap with our ecc
> * hardware, so define some i.MX specific ones.
> @@ -1513,6 +1564,8 @@ static int mxcnd_probe(struct platform_device *pdev)
> this->read_word = mxc_nand_read_word;
> this->write_buf = mxc_nand_write_buf;
> this->read_buf = mxc_nand_read_buf;
> + this->onfi_set_features = mxc_nand_onfi_set_features;
> + this->onfi_get_features = mxc_nand_onfi_get_features;
>
> host->clk = devm_clk_get(&pdev->dev, NULL);
> if (IS_ERR(host->clk))
^ permalink raw reply
* [BUG] hdlcd gets confused about base address
From: Liviu Dudau @ 2016-11-21 13:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121132419.GF1041@n2100.armlinux.org.uk>
On Mon, Nov 21, 2016 at 01:24:19PM +0000, Russell King - ARM Linux wrote:
> On Mon, Nov 21, 2016 at 12:56:53PM +0000, Liviu Dudau wrote:
> > That is mostly due to the check in hdlcd_crtc_disable() which I should
> > remove, I've added it because I was getting a ->disable() hook call
> > before any ->enable() was called at startup time. I need to revisit
> > this as I remember Daniel was commenting that this was not needed.
>
> Removing that test results in:
>
> [drm:drm_atomic_helper_commit_cleanup_done] *ERROR* [CRTC:24:crtc-0] flip_done timed out
>
> and the kernel hanging, seemingly in an IRQs-off region.
Right, I need to sort this one out. Are you doing these tests out of
some tagged branch that I can get in sync with?
Best regards,
Liviu
>
> --
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
?\_(?)_/?
^ permalink raw reply
* Synopsys Ethernet QoS Driver
From: Joao Pinto @ 2016-11-21 13:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <87c8a24b-0812-7850-cb3f-7be691bab432@st.com>
Hello Peppe,
On 21-11-2016 12:52, Giuseppe CAVALLARO wrote:
> Hello Joao
>
> On 11/21/2016 1:32 PM, Joao Pinto wrote:
>> Hello,
>>
>> On 21-11-2016 05:29, Rayagond Kokatanur wrote:
>>> On Sat, Nov 19, 2016 at 7:26 PM, Rabin Vincent <rabin@rab.in> wrote:
>>>> On Fri, Nov 18, 2016 at 02:20:27PM +0000, Joao Pinto wrote:
>>>>> For now we are interesting in improving the synopsys QoS driver under
>>>>> /nect/ethernet/synopsys. For now the driver structure consists of a single
>>>>> file
>>>>> called dwc_eth_qos.c, containing synopsys ethernet qos common ops and platform
>>>>> related stuff.
>>>>>
>>>>> Our strategy would be:
>>>>>
>>>>> a) Implement a platform glue driver (dwc_eth_qos_pltfm.c)
>>>>> b) Implement a pci glue driver (dwc_eth_qos_pci.c)
>>>>> c) Implement a "core driver" (dwc_eth_qos.c) that would only have Ethernet QoS
>>>>> related stuff to be reused by the platform / pci drivers
>>>>> d) Add a set of features to the "core driver" that we have available
>>>>> internally
>>>>
>>>> Note that there are actually two drivers in mainline for this hardware:
>>>>
>>>> drivers/net/ethernet/synopsis/
>>>> drivers/net/ethernet/stmicro/stmmac/
>>>
>>> Yes the later driver (drivers/net/ethernet/stmicro/stmmac/) supports
>>> both 3.x and 4.x. It has glue layer for pci, platform, core etc,
>>> please refer this driver once before you start.
>>>
>>> You can start adding missing feature of 4.x in stmmac driver.
>>
>> Thanks you all for all the info.
>> Well, I think we are in a good position to organize the ethernet drivers
>> concerning Synopsys IPs.
>>
>> First of all, in my opinion, it does not make sense to have a ethernet/synopsis
>> (typo :)) when ethernet/stmicro is also for a synopsys IP. If we have another
>> vendor using the same IP it should be able to reuse the commonn operations. But
>> I would put that discussion for later :)
>>
>> For now I suggest that for we create ethernet/qos and create there a folder
>> called dwc (designware controller) where all the synopsys qos IP specific code
>> in order to be reused for example by ethernet/stmicro/stmmac/. We just have to
>> figure out a clean interface for "client drivers" like stmmac to interact with
>> the new qos driver.
>>
>> What do you think about this approach?
>
> The stmmac drivers run since many years on several platforms
> (sh4, stm32, arm, x86, mips ...) and it supports an huge of amount of
> configurations starting from 3.1x to 3.7x databooks.
Great!
>
> It also supports QoS hardware; for example, 4.00a, 4.10a and 4.20a
> are fully working.
Synopsys QoS IP is a separated hardware component, so it should be reusable by
all implementations using it and so have its own "core driver" and platform +
pci glue drivers. This is necessary for example in hardware validation, where
you prototype an IP and instantiate its drivers and test it.
Was there a strong reason to integrate QoS features directly in stmmac and not
in synopsys/dwc_eth_qos.*?
>
> Also the stmmac has platform, device-tree and pcie supports and
> a lot of maintained glue-logic files.
>
> It is fully documented inside the kernel tree.
Thanks for the information, I am going to check it out.
>
> I am happy to have new enhancements from other developers.
> So, on my side, if you want to spend your time on improving it on your
> platforms please feel free to do it!
>
> Concerning the stmicro/stmmac naming, these come from a really old
> story and have no issue to adopt new folder/file names.
Thank you for your availability!
>
> I am also open to merge fixes and changes from ethernet/synopsis.
> I want to point you on some benchmarks made by Alex some months ago
> (IIRC) that showed an stmmac winner (due to the several optimizations
> analyzed and reviewed in this mailing list).
>
> Peppe
>
Thanks
Joao
^ permalink raw reply
* [PATCH v3 1/2] mfd: pm8xxx: add support to pm8821
From: Lee Jones @ 2016-11-21 13:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1479211312-1431-1-git-send-email-srinivas.kandagatla@linaro.org>
On Tue, 15 Nov 2016, Srinivas Kandagatla wrote:
> This patch adds support to PM8821 PMIC and interrupt support.
> PM8821 is companion device that supplements primary PMIC PM8921 IC.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> Changes since v2:
> - removed few empty lines spotted by Bjorn
> - rephrased some debug prints suggested by Bjorn
>
> .../devicetree/bindings/mfd/qcom-pm8xxx.txt | 1 +
> drivers/mfd/qcom-pm8xxx.c | 231 ++++++++++++++++++++-
> 2 files changed, 222 insertions(+), 10 deletions(-)
Applied, thanks.
> diff --git a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> index 37a088f..9e5eba4 100644
> --- a/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> +++ b/Documentation/devicetree/bindings/mfd/qcom-pm8xxx.txt
> @@ -10,6 +10,7 @@ voltages and other various functionality to Qualcomm SoCs.
> Value type: <string>
> Definition: must be one of:
> "qcom,pm8058"
> + "qcom,pm8821"
> "qcom,pm8921"
>
> - #address-cells:
> diff --git a/drivers/mfd/qcom-pm8xxx.c b/drivers/mfd/qcom-pm8xxx.c
> index 7f9620e..f08758f 100644
> --- a/drivers/mfd/qcom-pm8xxx.c
> +++ b/drivers/mfd/qcom-pm8xxx.c
> @@ -39,6 +39,20 @@
> #define SSBI_REG_ADDR_IRQ_CONFIG (SSBI_REG_ADDR_IRQ_BASE + 7)
> #define SSBI_REG_ADDR_IRQ_RT_STATUS (SSBI_REG_ADDR_IRQ_BASE + 8)
>
> +#define PM8821_SSBI_REG_ADDR_IRQ_BASE 0x100
> +#define PM8821_SSBI_REG_ADDR_IRQ_MASTER0 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0x30)
> +#define PM8821_SSBI_REG_ADDR_IRQ_MASTER1 (PM8821_SSBI_REG_ADDR_IRQ_BASE + 0xb0)
> +#define PM8821_SSBI_REG(m, b, offset) \
> + ((m == 0) ? \
> + (PM8821_SSBI_REG_ADDR_IRQ_MASTER0 + b + offset) : \
> + (PM8821_SSBI_REG_ADDR_IRQ_MASTER1 + b + offset))
> +#define PM8821_SSBI_ADDR_IRQ_ROOT(m, b) PM8821_SSBI_REG(m, b, 0x0)
> +#define PM8821_SSBI_ADDR_IRQ_CLEAR(m, b) PM8821_SSBI_REG(m, b, 0x01)
> +#define PM8821_SSBI_ADDR_IRQ_MASK(m, b) PM8821_SSBI_REG(m, b, 0x08)
> +#define PM8821_SSBI_ADDR_IRQ_RT_STATUS(m, b) PM8821_SSBI_REG(m, b, 0x0f)
> +
> +#define PM8821_BLOCKS_PER_MASTER 7
> +
> #define PM_IRQF_LVL_SEL 0x01 /* level select */
> #define PM_IRQF_MASK_FE 0x02 /* mask falling edge */
> #define PM_IRQF_MASK_RE 0x04 /* mask rising edge */
> @@ -54,6 +68,7 @@
> #define REG_HWREV_2 0x0E8 /* PMIC4 revision 2 */
>
> #define PM8XXX_NR_IRQS 256
> +#define PM8821_NR_IRQS 112
>
> struct pm_irq_chip {
> struct regmap *regmap;
> @@ -65,6 +80,12 @@ struct pm_irq_chip {
> u8 config[0];
> };
>
> +struct pm_irq_data {
> + int num_irqs;
> + const struct irq_domain_ops *irq_domain_ops;
> + void (*irq_handler)(struct irq_desc *desc);
> +};
> +
> static int pm8xxx_read_block_irq(struct pm_irq_chip *chip, unsigned int bp,
> unsigned int *ip)
> {
> @@ -182,6 +203,78 @@ static void pm8xxx_irq_handler(struct irq_desc *desc)
> chained_irq_exit(irq_chip, desc);
> }
>
> +static void pm8821_irq_block_handler(struct pm_irq_chip *chip,
> + int master, int block)
> +{
> + int pmirq, irq, i, ret;
> + unsigned int bits;
> +
> + ret = regmap_read(chip->regmap,
> + PM8821_SSBI_ADDR_IRQ_ROOT(master, block), &bits);
> + if (ret) {
> + pr_err("Reading block %d failed ret=%d", block, ret);
> + return;
> + }
> +
> + /* Convert block offset to global block number */
> + block += (master * PM8821_BLOCKS_PER_MASTER) - 1;
> +
> + /* Check IRQ bits */
> + for (i = 0; i < 8; i++) {
> + if (bits & BIT(i)) {
> + pmirq = block * 8 + i;
> + irq = irq_find_mapping(chip->irqdomain, pmirq);
> + generic_handle_irq(irq);
> + }
> + }
> +}
> +
> +static inline void pm8821_irq_master_handler(struct pm_irq_chip *chip,
> + int master, u8 master_val)
> +{
> + int block;
> +
> + for (block = 1; block < 8; block++)
> + if (master_val & BIT(block))
> + pm8821_irq_block_handler(chip, master, block);
> +}
> +
> +static void pm8821_irq_handler(struct irq_desc *desc)
> +{
> + struct pm_irq_chip *chip = irq_desc_get_handler_data(desc);
> + struct irq_chip *irq_chip = irq_desc_get_chip(desc);
> + unsigned int master;
> + int ret;
> +
> + chained_irq_enter(irq_chip, desc);
> + ret = regmap_read(chip->regmap,
> + PM8821_SSBI_REG_ADDR_IRQ_MASTER0, &master);
> + if (ret) {
> + pr_err("Failed to read master 0 ret=%d\n", ret);
> + goto done;
> + }
> +
> + /* bits 1 through 7 marks the first 7 blocks in master 0 */
> + if (master & GENMASK(7, 1))
> + pm8821_irq_master_handler(chip, 0, master);
> +
> + /* bit 0 marks if master 1 contains any bits */
> + if (!(master & BIT(0)))
> + goto done;
> +
> + ret = regmap_read(chip->regmap,
> + PM8821_SSBI_REG_ADDR_IRQ_MASTER1, &master);
> + if (ret) {
> + pr_err("Failed to read master 1 ret=%d\n", ret);
> + goto done;
> + }
> +
> + pm8821_irq_master_handler(chip, 1, master);
> +
> +done:
> + chained_irq_exit(irq_chip, desc);
> +}
> +
> static void pm8xxx_irq_mask_ack(struct irq_data *d)
> {
> struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> @@ -299,6 +392,104 @@ static const struct irq_domain_ops pm8xxx_irq_domain_ops = {
> .map = pm8xxx_irq_domain_map,
> };
>
> +static void pm8821_irq_mask_ack(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int pmirq = irqd_to_hwirq(d);
> + u8 block, master;
> + int irq_bit, rc;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
> +
> + rc = regmap_update_bits(chip->regmap,
> + PM8821_SSBI_ADDR_IRQ_MASK(master, block),
> + BIT(irq_bit), BIT(irq_bit));
> + if (rc) {
> + pr_err("Failed to mask IRQ:%d rc=%d\n", pmirq, rc);
> + return;
> + }
> +
> + rc = regmap_update_bits(chip->regmap,
> + PM8821_SSBI_ADDR_IRQ_CLEAR(master, block),
> + BIT(irq_bit), BIT(irq_bit));
> + if (rc)
> + pr_err("Failed to CLEAR IRQ:%d rc=%d\n", pmirq, rc);
> +}
> +
> +static void pm8821_irq_unmask(struct irq_data *d)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + unsigned int pmirq = irqd_to_hwirq(d);
> + int irq_bit, rc;
> + u8 block, master;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
> +
> + rc = regmap_update_bits(chip->regmap,
> + PM8821_SSBI_ADDR_IRQ_MASK(master, block),
> + BIT(irq_bit), ~BIT(irq_bit));
> + if (rc)
> + pr_err("Failed to read/write unmask IRQ:%d rc=%d\n", pmirq, rc);
> +
> +}
> +
> +static int pm8821_irq_get_irqchip_state(struct irq_data *d,
> + enum irqchip_irq_state which,
> + bool *state)
> +{
> + struct pm_irq_chip *chip = irq_data_get_irq_chip_data(d);
> + int rc, pmirq = irqd_to_hwirq(d);
> + u8 block, irq_bit, master;
> + unsigned int bits;
> +
> + block = pmirq / 8;
> + master = block / PM8821_BLOCKS_PER_MASTER;
> + irq_bit = pmirq % 8;
> + block %= PM8821_BLOCKS_PER_MASTER;
> +
> + rc = regmap_read(chip->regmap,
> + PM8821_SSBI_ADDR_IRQ_RT_STATUS(master, block), &bits);
> + if (rc) {
> + pr_err("Reading Status of IRQ %d failed rc=%d\n", pmirq, rc);
> + return rc;
> + }
> +
> + *state = !!(bits & BIT(irq_bit));
> +
> + return rc;
> +}
> +
> +static struct irq_chip pm8821_irq_chip = {
> + .name = "pm8821",
> + .irq_mask_ack = pm8821_irq_mask_ack,
> + .irq_unmask = pm8821_irq_unmask,
> + .irq_get_irqchip_state = pm8821_irq_get_irqchip_state,
> + .flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE,
> +};
> +
> +static int pm8821_irq_domain_map(struct irq_domain *d, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + struct pm_irq_chip *chip = d->host_data;
> +
> + irq_set_chip_and_handler(irq, &pm8821_irq_chip, handle_level_irq);
> + irq_set_chip_data(irq, chip);
> + irq_set_noprobe(irq);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops pm8821_irq_domain_ops = {
> + .xlate = irq_domain_xlate_twocell,
> + .map = pm8821_irq_domain_map,
> +};
> +
> static const struct regmap_config ssbi_regmap_config = {
> .reg_bits = 16,
> .val_bits = 8,
> @@ -308,22 +499,41 @@ static const struct regmap_config ssbi_regmap_config = {
> .reg_write = ssbi_reg_write
> };
>
> +static const struct pm_irq_data pm8xxx_data = {
> + .num_irqs = PM8XXX_NR_IRQS,
> + .irq_domain_ops = &pm8xxx_irq_domain_ops,
> + .irq_handler = pm8xxx_irq_handler,
> +};
> +
> +static const struct pm_irq_data pm8821_data = {
> + .num_irqs = PM8821_NR_IRQS,
> + .irq_domain_ops = &pm8821_irq_domain_ops,
> + .irq_handler = pm8821_irq_handler,
> +};
> +
> static const struct of_device_id pm8xxx_id_table[] = {
> - { .compatible = "qcom,pm8018", },
> - { .compatible = "qcom,pm8058", },
> - { .compatible = "qcom,pm8921", },
> + { .compatible = "qcom,pm8018", .data = &pm8xxx_data},
> + { .compatible = "qcom,pm8058", .data = &pm8xxx_data},
> + { .compatible = "qcom,pm8821", .data = &pm8821_data},
> + { .compatible = "qcom,pm8921", .data = &pm8xxx_data},
> { }
> };
> MODULE_DEVICE_TABLE(of, pm8xxx_id_table);
>
> static int pm8xxx_probe(struct platform_device *pdev)
> {
> + const struct pm_irq_data *data;
> struct regmap *regmap;
> int irq, rc;
> unsigned int val;
> u32 rev;
> struct pm_irq_chip *chip;
> - unsigned int nirqs = PM8XXX_NR_IRQS;
> +
> + data = of_device_get_match_data(&pdev->dev);
> + if (!data) {
> + dev_err(&pdev->dev, "No matching driver data found\n");
> + return -EINVAL;
> + }
>
> irq = platform_get_irq(pdev, 0);
> if (irq < 0)
> @@ -354,25 +564,26 @@ static int pm8xxx_probe(struct platform_device *pdev)
> rev |= val << BITS_PER_BYTE;
>
> chip = devm_kzalloc(&pdev->dev, sizeof(*chip) +
> - sizeof(chip->config[0]) * nirqs,
> - GFP_KERNEL);
> + sizeof(chip->config[0]) * data->num_irqs,
> + GFP_KERNEL);
> if (!chip)
> return -ENOMEM;
>
> platform_set_drvdata(pdev, chip);
> chip->regmap = regmap;
> - chip->num_irqs = nirqs;
> + chip->num_irqs = data->num_irqs;
> chip->num_blocks = DIV_ROUND_UP(chip->num_irqs, 8);
> chip->num_masters = DIV_ROUND_UP(chip->num_blocks, 8);
> spin_lock_init(&chip->pm_irq_lock);
>
> - chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node, nirqs,
> - &pm8xxx_irq_domain_ops,
> + chip->irqdomain = irq_domain_add_linear(pdev->dev.of_node,
> + data->num_irqs,
> + data->irq_domain_ops,
> chip);
> if (!chip->irqdomain)
> return -ENODEV;
>
> - irq_set_chained_handler_and_data(irq, pm8xxx_irq_handler, chip);
> + irq_set_chained_handler_and_data(irq, data->irq_handler, chip);
> irq_set_irq_wake(irq, 1);
>
> rc = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
--
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access
From: Christoffer Dall @ 2016-11-21 13:43 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALicx6snqsSbaKYcj9kt0nt8y2wdTbrY1PKOq-SrGTepCfSZSg@mail.gmail.com>
On Mon, Nov 21, 2016 at 06:56:08PM +0530, Vijay Kilari wrote:
> On Mon, Nov 21, 2016 at 3:49 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
> >> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
> >> <christoffer.dall@linaro.org> wrote:
> >> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
> >> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
> >> >> <christoffer.dall@linaro.org> wrote:
> >> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kilari at gmail.com wrote:
> >> >> >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> >> >>
> >> >> >> VGICv3 CPU interface registers are accessed using
> >> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
> >> >> >> is used to identify the cpu for registers access.
> >> >> >>
> >> >> >> The version of VGIC v3 specification is define here
> >> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >> >>
> >> >> >> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> >> >> ---
> >> >> >> arch/arm64/include/uapi/asm/kvm.h | 3 +
> >> >> >> arch/arm64/kvm/Makefile | 1 +
> >> >> >> include/kvm/arm_vgic.h | 9 +
> >> >> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
> >> >> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
> >> >> >> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
> >> >> >> virt/kvm/arm/vgic/vgic-v3.c | 8 +
> >> >> >> virt/kvm/arm/vgic/vgic.h | 4 +
> >> >> >> 8 files changed, 395 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> index 56dc08d..91c7137 100644
> >> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> >> >> >> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> >> >> >> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
> >> >> >> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >> >> +#define KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> >> >> >> +
> >> >> >> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >> >>
> >> >> >> /* Device Control API on vcpu fd */
> >> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> >> >> index d50a82a..1a14e29 100644
> >> >> >> --- a/arch/arm64/kvm/Makefile
> >> >> >> +++ b/arch/arm64/kvm/Makefile
> >> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> >> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> >> >> >
> >> >> > Thi is making me wonder: Are we properly handling GICv3 save/restore
> >> >> > for AArch32 now that we have GICv3 support for AArch32? By properly I
> >> >> > mean that either it is clearly only supported on AArch64 systems or it's
> >> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
> >> >> > on AArch32.
> >> >>
> >> >> It supports both AArch64 and AArch64 in handling of system registers
> >> >> save/restore.
> >> >> All system registers that we save/restore are 32-bit for both aarch64
> >> >> and aarch32.
> >> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
> >> >> are same. However the codes sent by qemu is matched and register
> >> >> are handled properly irrespective of AArch32 or AArch64.
> >> >>
> >> >> I don't have platform which support AArch32 guests to verify.
> >> >
> >> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
> >> > that has a GICv3.
> >> >
> >> > I just tried to do a v7 compile with your patches, and it results in an
> >> > epic failure, so there's something for you to look at.
> >> >
> >> >> >
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> >> >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> >> >> index 002f092..730a18a 100644
> >> >> >> --- a/include/kvm/arm_vgic.h
> >> >> >> +++ b/include/kvm/arm_vgic.h
> >> >> >> @@ -71,6 +71,9 @@ struct vgic_global {
> >> >> >>
> >> >> >> /* GIC system register CPU interface */
> >> >> >> struct static_key_false gicv3_cpuif;
> >> >> >> +
> >> >> >> + /* Cache ICH_VTR_EL2 reg value */
> >> >> >> + u32 ich_vtr_el2;
> >> >> >> };
> >> >> >>
> >> >> >> extern struct vgic_global kvm_vgic_global_state;
> >> >> >> @@ -269,6 +272,12 @@ struct vgic_cpu {
> >> >> >> u64 pendbaser;
> >> >> >>
> >> >> >> bool lpis_enabled;
> >> >> >> +
> >> >> >> + /* Cache guest priority bits */
> >> >> >> + u32 num_pri_bits;
> >> >> >> +
> >> >> >> + /* Cache guest interrupt ID bits */
> >> >> >> + u32 num_id_bits;
> >> >> >> };
> >> >> >>
> >> >> >> extern struct static_key_false vgic_v2_cpuif_trap;
> >> >> >> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> >> >> index 6c7d30c..da532d1 100644
> >> >> >> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> >> >> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> >> >> >> @@ -504,6 +504,14 @@ static int vgic_v3_attr_regs_access(struct kvm_device *dev,
> >> >> >> if (!is_write)
> >> >> >> *reg = tmp32;
> >> >> >> break;
> >> >> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> >> >> >> + u64 regid;
> >> >> >> +
> >> >> >> + regid = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> >> >> >> + ret = vgic_v3_cpu_sysregs_uaccess(vcpu, is_write,
> >> >> >> + regid, reg);
> >> >> >> + break;
> >> >> >> + }
> >> >> >> default:
> >> >> >> ret = -EINVAL;
> >> >> >> break;
> >> >> >> @@ -537,6 +545,15 @@ static int vgic_v3_set_attr(struct kvm_device *dev,
> >> >> >> reg = tmp32;
> >> >> >> return vgic_v3_attr_regs_access(dev, attr, ®, true);
> >> >> >> }
> >> >> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> >> >> >> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> >> >> >> + u64 reg;
> >> >> >> +
> >> >> >> + if (get_user(reg, uaddr))
> >> >> >> + return -EFAULT;
> >> >> >> +
> >> >> >> + return vgic_v3_attr_regs_access(dev, attr, ®, true);
> >> >> >> + }
> >> >> >> }
> >> >> >> return -ENXIO;
> >> >> >> }
> >> >> >> @@ -563,6 +580,15 @@ static int vgic_v3_get_attr(struct kvm_device *dev,
> >> >> >> tmp32 = reg;
> >> >> >> return put_user(tmp32, uaddr);
> >> >> >> }
> >> >> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> >> >> >> + u64 __user *uaddr = (u64 __user *)(long)attr->addr;
> >> >> >> + u64 reg;
> >> >> >> +
> >> >> >> + ret = vgic_v3_attr_regs_access(dev, attr, ®, false);
> >> >> >> + if (ret)
> >> >> >> + return ret;
> >> >> >> + return put_user(reg, uaddr);
> >> >> >> + }
> >> >> >> }
> >> >> >>
> >> >> >> return -ENXIO;
> >> >> >> @@ -581,6 +607,7 @@ static int vgic_v3_has_attr(struct kvm_device *dev,
> >> >> >> break;
> >> >> >> case KVM_DEV_ARM_VGIC_GRP_DIST_REGS:
> >> >> >> case KVM_DEV_ARM_VGIC_GRP_REDIST_REGS:
> >> >> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS:
> >> >> >> return vgic_v3_has_attr_regs(dev, attr);
> >> >> >> case KVM_DEV_ARM_VGIC_GRP_NR_IRQS:
> >> >> >> return 0;
> >> >> >> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> >> >> index b35fb83..519b919 100644
> >> >> >> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> >> >> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> >> >> >> @@ -23,6 +23,7 @@
> >> >> >>
> >> >> >> #include "vgic.h"
> >> >> >> #include "vgic-mmio.h"
> >> >> >> +#include "sys_regs.h"
> >> >> >>
> >> >> >> /* extract @num bytes at @offset bytes offset in data */
> >> >> >> unsigned long extract_bytes(u64 data, unsigned int offset,
> >> >> >> @@ -639,6 +640,24 @@ int vgic_v3_has_attr_regs(struct kvm_device *dev, struct kvm_device_attr *attr)
> >> >> >> nr_regions = ARRAY_SIZE(vgic_v3_rdbase_registers);
> >> >> >> break;
> >> >> >> }
> >> >> >> + case KVM_DEV_ARM_VGIC_CPU_SYSREGS: {
> >> >> >> + u64 reg, id;
> >> >> >> + unsigned long vgic_mpidr, mpidr_reg;
> >> >> >> + struct kvm_vcpu *vcpu;
> >> >> >> +
> >> >> >> + vgic_mpidr = (attr->attr & KVM_DEV_ARM_VGIC_V3_MPIDR_MASK) >>
> >> >> >> + KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT;
> >> >> >> +
> >> >> >> + /* Convert plain mpidr value to MPIDR reg format */
> >> >> >> + mpidr_reg = VGIC_TO_MPIDR(vgic_mpidr);
> >> >> >> +
> >> >> >> + vcpu = kvm_mpidr_to_vcpu(dev->kvm, mpidr_reg);
> >> >> >> + if (!vcpu)
> >> >> >> + return -EINVAL;
> >> >> >> +
> >> >> >> + id = (attr->attr & KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK);
> >> >> >> + return vgic_v3_has_cpu_sysregs_attr(vcpu, 0, id, ®);
> >> >> >> + }
> >> >> >> default:
> >> >> >> return -ENXIO;
> >> >> >> }
> >> >> >> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> >> >> >> new file mode 100644
> >> >> >> index 0000000..69d8597
> >> >> >> --- /dev/null
> >> >> >> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> >> >> >
> >> >> > Shouldn't we have a GPL header here?
> >> >> >
> >> >> >> @@ -0,0 +1,324 @@
> >> >> >> +#include <linux/irqchip/arm-gic-v3.h>
> >> >> >> +#include <linux/kvm.h>
> >> >> >> +#include <linux/kvm_host.h>
> >> >> >> +#include <kvm/iodev.h>
> >> >> >> +#include <kvm/arm_vgic.h>
> >> >> >> +#include <asm/kvm_emulate.h>
> >> >> >> +#include <asm/kvm_arm.h>
> >> >> >> +#include <asm/kvm_mmu.h>
> >> >> >> +
> >> >> >> +#include "vgic.h"
> >> >> >> +#include "vgic-mmio.h"
> >> >> >> +#include "sys_regs.h"
> >> >> >> +
> >> >> >> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >> >> >> + const struct sys_reg_desc *r)
> >> >> >> +{
> >> >> >> + struct vgic_cpu *vgic_v3_cpu = &vcpu->arch.vgic_cpu;
> >> >> >> + struct vgic_vmcr vmcr;
> >> >> >> + u64 val;
> >> >> >> + u32 num_pri_bits, num_id_bits;
> >> >> >> +
> >> >> >> + vgic_get_vmcr(vcpu, &vmcr);
> >> >> >> + if (p->is_write) {
> >> >> >> + val = p->regval;
> >> >> >> +
> >> >> >> + /*
> >> >> >> + * Does not allow update of ICC_CTLR_EL1 if HW does not support
> >> >> >> + * guest programmed ID and PRI bits
> >> >> >> + */
> >> >> >
> >> >> > I would suggest rewording this comment:
> >> >> > Disallow restoring VM state not supported by this hardware.
> >> >> >
> >> >> >> + num_pri_bits = ((val & ICC_CTLR_EL1_PRI_BITS_MASK) >>
> >> >> >> + ICC_CTLR_EL1_PRI_BITS_SHIFT) + 1;
> >> >> >> + if (num_pri_bits > vgic_v3_cpu->num_pri_bits)
> >> >> >> + return false;
> >> >> >> +
> >> >> >> + vgic_v3_cpu->num_pri_bits = num_pri_bits;
> >> >> >
> >> >> > hmmm, this looks weird to me, because vgic_v3_cpu->num_pri_bits I don't
> >> >> > understand which effect this is intended to have?
> >> >> >
> >> >> > Sure, it may limit what you do with other registers later, but since
> >> >> > there's no ordering requirement that the ctlr be restored first, I'm not
> >> >> > sure it makes sense.
> >> >> >
> >> >> > Also, since this field is RO in the ICH_VTR, we'll have a strange
> >> >> > situation during runtime after a GICv3 restore where the
> >> >> > vgic_v3_cpu->num_pri_its differs from the hardware's ICH_VTR_EL2 field,
> >> >> > which is never the case if you didn't do a save/restore.
> >> >>
> >> >> Yes, but in any case, vgic_v3_cpu->num_pri_bits will be always less
> >> >> than HW supported
> >> >> value.
> >> >>
> >> >
> >> > So answer my question: What is the intended effect of writing this
> >> > value? Is it just so that if you migrate this platform back again, then
> >> > you're checking compatibility with what the guest would potentially do,
> >>
> >> Yes
> >
> > Then add a comment explaining that
> >
> >> and also to limit the valid aprn registers access as you said above.
> >> But that has ordering restriction. Which I think we should follow.
> >>
> >
> > I'm sorry, now I'm confused. Is there an ordering requirement in the
> > API, or how should we follow this?
>
> There is no ordering requirement mentioned in the API doc.
> However the APRn registers depends on num_pri_bits. Hence first
> ICC_CTLR_EL1 should be restored before APRn restore.
>
> If ordering is not followed then APRn registers restore is allowed
> as per hw supported num_pri_bits.
>
> This should be mentioned in doc.
How about just having a consistency check function that you call from
uaccess updates to both functions, and in that way avoid requireing any
ordering which is likely to not be followed etc.?
Thanks,
-Christoffer
^ permalink raw reply
* [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access
From: Christoffer Dall @ 2016-11-21 13:41 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CALicx6sxchAbDJ2rioTytwFG1C0tD-Fnr4HC-DweSdkkCF7BFw@mail.gmail.com>
On Mon, Nov 21, 2016 at 07:02:36PM +0530, Vijay Kilari wrote:
> On Sun, Nov 20, 2016 at 6:50 PM, Christoffer Dall
> <christoffer.dall@linaro.org> wrote:
> > On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
> >> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
> >> <christoffer.dall@linaro.org> wrote:
> >> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
> >> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
> >> >> <christoffer.dall@linaro.org> wrote:
> >> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kilari at gmail.com wrote:
> >> >> >> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> >> >>
> >> >> >> VGICv3 CPU interface registers are accessed using
> >> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
> >> >> >> is used to identify the cpu for registers access.
> >> >> >>
> >> >> >> The version of VGIC v3 specification is define here
> >> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >> >>
> >> >> >> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >> >> >> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >> >> >> ---
> >> >> >> arch/arm64/include/uapi/asm/kvm.h | 3 +
> >> >> >> arch/arm64/kvm/Makefile | 1 +
> >> >> >> include/kvm/arm_vgic.h | 9 +
> >> >> >> virt/kvm/arm/vgic/vgic-kvm-device.c | 27 +++
> >> >> >> virt/kvm/arm/vgic/vgic-mmio-v3.c | 19 +++
> >> >> >> virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 ++++++++++++++++++++++++++++++++++++
> >> >> >> virt/kvm/arm/vgic/vgic-v3.c | 8 +
> >> >> >> virt/kvm/arm/vgic/vgic.h | 4 +
> >> >> >> 8 files changed, 395 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> index 56dc08d..91c7137 100644
> >> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> >> >> >> (0xffffffffULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> >> >> >> #define KVM_DEV_ARM_VGIC_OFFSET_SHIFT 0
> >> >> >> #define KVM_DEV_ARM_VGIC_OFFSET_MASK (0xffffffffULL << KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >> >> +#define KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0xffff)
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_CTRL 4
> >> >> >> #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS 6
> >> >> >> +
> >> >> >> #define KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >> >>
> >> >> >> /* Device Control API on vcpu fd */
> >> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> >> >> index d50a82a..1a14e29 100644
> >> >> >> --- a/arch/arm64/kvm/Makefile
> >> >> >> +++ b/arch/arm64/kvm/Makefile
> >> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> >> >> >> kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> >> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> >> >> >
> >> >> > Thi is making me wonder: Are we properly handling GICv3 save/restore
> >> >> > for AArch32 now that we have GICv3 support for AArch32? By properly I
> >> >> > mean that either it is clearly only supported on AArch64 systems or it's
> >> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
> >> >> > on AArch32.
> >> >>
> >> >> It supports both AArch64 and AArch64 in handling of system registers
> >> >> save/restore.
> >> >> All system registers that we save/restore are 32-bit for both aarch64
> >> >> and aarch32.
> >> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn codes
> >> >> are same. However the codes sent by qemu is matched and register
> >> >> are handled properly irrespective of AArch32 or AArch64.
> >> >>
> >> >> I don't have platform which support AArch32 guests to verify.
> >> >
> >> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
> >> > that has a GICv3.
> >> >
> >> > I just tried to do a v7 compile with your patches, and it results in an
> >> > epic failure, so there's something for you to look at.
> >> >
> >>
> >> Could you please share you config file?. I tried with multi_v7 defconfig with
> >> CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
> >
> > I think this has to do with which branch you apply your patches to.
> > When applied to kvmarm/next, it fails.
> >
> > Here's the integration I did:
> > https://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git tmp-gicv3-migrate-v8
> >
> > Here's the config:
> > https://transfer.sh/xkAxp/.config
> >
>
> Thanks for shareing the details, I could reproduce them.
> However virt/kvm/arm/vgic/vgic-sys-reg-v3.c is written with
> sys_regs_desc for AArch64.
> For AArch32/v7, it has be to coproc_reg. I propose to add separate file for arm
> which handles ICC* reg save/restore using coproc_reg.
That might make sense. In that case they want to be moved into
arch/arm/kvm/ and arch/arm64/kvm/
-Christoffer
^ permalink raw reply
* [PATCH] mfd: twl-core: export twl_get_regmap
From: Russell King - ARM Linux @ 2016-11-21 13:37 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAKP5S=28xVyNE=Q7hd2UbtOmT=12ayrcWM5m1OTvk5kGN5pZrw@mail.gmail.com>
On Mon, Nov 21, 2016 at 12:03:03PM +0200, Nicolae Rosia wrote:
> On Mon, Nov 21, 2016 at 11:31 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Passing data between drivers using *_set_drvdata() is a layering
> > violation:
> >
> > 1. Driver data is supposed to be driver private data associated with
> > the currently bound driver.
> > 2. The driver data pointer is NULL'd when the driver unbinds from the
> > device. See __device_release_driver() and the
> > dev_set_drvdata(dev, NULL).
> > 3. It will break with CONFIG_DEBUG_TEST_DRIVER_REMOVE enabled for a
> > similar reason to (2).
> >
> > So, do not pass data between drivers using *_set_drvdata() - any
> > examples in the kernel already are founded on bad practice, are
> > fragile, and are already broken for some kernel configurations.
>
> After inspecting mfd_add_device, it seems that it creates a
> platform_device which has the parent set to the driver calling the
> function.
> Isn't module unloading forbidden if there is a parent->child
> relationship in place and you're removing the parent?
Forget this idea that there's any connection between modules and
the struct device relationships - there isn't anything of the kind!
Each struct device is refcounted, and child devices will hold a
reference to their parent device, so the parent device doesn't get
freed before its children are all gone.
That's a completely separate issue to when a struct device is bound
to a struct device_driver - it's entirely possible for parent drivers
to be unbound at any time, even when there are child drivers in place.
There are cases where we want that to happen - think of any driver
which is a bus driver in itself - eg, PCMCIA, MMC, USB, etc. These
drivers enumerate their children, and destroy their children when
the driver is unbound - but the driver has to be in the process of
being unbound for that to happen. That process may very well start
with the child devices being bound to their drivers.
What makes the child drivers unbind is when the bus driver deletes
the child struct devices.
> What should be the best practice to share data between drivers?
> Reference counted data?
I guess so, but you will still have a race if you do something like:
struct parent_private_data *parent_priv = dev_get_drvdata(dev->parent);
Yes, that'll get the parent's driver private data, but what you don't
know is whether the pointer remains valid, and even if you do as the
very next step:
kref_get(&parent_priv->kref);
you don't know whether parent_priv was kfree()d between these two
statements.
However, if the parent driver creates the struct device that you're
using and deletes the struct device before it frees its private data,
then you can be sure that parent_priv will be valid, because the child
drivers will be unbound during the parent driver's ->remove function,
_before_ the private data is freed.
> In the case of TWL, the twl-core is just a simple container for
> regmaps - all other "sub devices" are using those regmaps to access
> the I2C device's registers, it makes no sense to remove the parent
> driver since it does *nothing*.
I can't comment on what twl-core is doing, I haven't looked at it in
ages, but most MFD drivers have the parent device creating and destroying
their children, so it should be fine.
My original comment was more along the lines of a parent device poking
driver-private data into the child devices it was creating for the
child drivers to pick up. However, it's worth discussing the validity
cases of the parent's driver data too, as per the above.
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* [PATCH v5 3/3] dt-bindings: i2c: pxa: Update the documentation for the Armada 3700
From: Romain Perier @ 2016-11-21 13:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121133247.29889-1-romain.perier@free-electrons.com>
This commit documents the compatible string to have the compatibility for
the I2C unit found in the Armada 3700.
Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Added the tag 'Acked-by', by Rob Herring
Changes in v2:
- Fixed wrong compatible string, it should be "marvell,armada-3700-i2c"
and not "marvell,armada-3700".
Documentation/devicetree/bindings/i2c/i2c-pxa.txt | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
index 12b78ac..d30f0b1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-pxa.txt
@@ -7,6 +7,7 @@ Required properties :
compatible processor, e.g. pxa168, pxa910, mmp2, mmp3.
For the pxa2xx/pxa3xx, an additional node "mrvl,pxa-i2c" is required
as shown in the example below.
+ For the Armada 3700, the compatible should be "marvell,armada-3700-i2c".
Recommended properties :
--
2.9.3
^ permalink raw reply related
* [PATCH v5 2/3] arm64: dts: marvell: Add I2C definitions for the Armada 3700
From: Romain Perier @ 2016-11-21 13:32 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20161121133247.29889-1-romain.perier@free-electrons.com>
The Armada 3700 has two i2c bus interface units, this commit adds the
definitions of the corresponding device nodes. It also enables the node
on the development board for this SoC.
Signed-off-by: Romain Perier <romain.perier@free-electrons.com>
Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>
---
arch/arm64/boot/dts/marvell/armada-3720-db.dts | 4 ++++
arch/arm64/boot/dts/marvell/armada-37xx.dtsi | 18 ++++++++++++++++++
2 files changed, 22 insertions(+)
diff --git a/arch/arm64/boot/dts/marvell/armada-3720-db.dts b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
index 1372e9a6..16d84af 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-db.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-db.dts
@@ -62,6 +62,10 @@
};
};
+&i2c0 {
+ status = "okay";
+};
+
/* CON3 */
&sata {
status = "okay";
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index e9bd587..1b0fd21 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -98,6 +98,24 @@
/* 32M internal register @ 0xd000_0000 */
ranges = <0x0 0x0 0xd0000000 0x2000000>;
+ i2c0: i2c at 11000 {
+ compatible = "marvell,armada-3700-i2c";
+ reg = <0x11000 0x24>;
+ clocks = <&nb_periph_clk 10>;
+ interrupts = <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>;
+ mrvl,i2c-fast-mode;
+ status = "disabled";
+ };
+
+ i2c1: i2c at 11080 {
+ compatible = "marvell,armada-3700-i2c";
+ reg = <0x11080 0x24>;
+ clocks = <&nb_periph_clk 9>;
+ interrupts = <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>;
+ mrvl,i2c-fast-mode;
+ status = "disabled";
+ };
+
uart0: serial at 12000 {
compatible = "marvell,armada-3700-uart";
reg = <0x12000 0x400>;
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox