* Re: [PATCH V2 3/4] regulator: pv88080: Update Regulator driver for MFD support
From: Mark Brown @ 2016-10-27 11:03 UTC (permalink / raw)
To: Eric Jeong
Cc: LINUX-KERNEL, Liam Girdwood, Alexandre Courbot, DEVICETREE,
LINUX-GPIO, Lee Jones, Linus Walleij, Mark Rutland, Rob Herring,
Support Opensource
In-Reply-To: <711afefe3e65c166111473024bf9b54f3d4cc8b6.1477530195.git.eric.jeong@diasemi.com>
[-- Attachment #1: Type: text/plain, Size: 990 bytes --]
On Thu, Oct 27, 2016 at 10:03:14AM +0900, Eric Jeong wrote:
> config REGULATOR_PV88080
> - tristate "Powerventure Semiconductor PV88080 regulator"
> - depends on I2C
> - select REGMAP_I2C
> + bool "Powerventure Semiconductor PV88080 regulator"
> + depends on MFD_PV88080
Forcing the driver to be built in looks like a regression, why would we
want to do that?
> + irq = platform_get_irq_byname(pdev, "regulator-irq");
> + if (irq < 0) {
> + dev_err(&pdev->dev, "Failed to get IRQ.\n");
> + return irq;
> }
What's the _byname() adding here given that the name is so generic? It
feels like if the name ever becomes important then this particular name
is going to be a problem.
> -module_i2c_driver(pv88080_regulator_driver);
> +static int __init pv88080_regulator_init(void)
> +{
> + return platform_driver_register(&pv88080_regulator_driver);
> +}
> +subsys_initcall(pv88080_regulator_init);
Why are you converting this to subsys_initcall()? This looks like
another regression.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/4] arm64: arch_timer: Work around Erratum Hisilicon-161601
From: Marc Zyngier @ 2016-10-27 10:58 UTC (permalink / raw)
To: Ding Tianhong, catalin.marinas, will.deacon, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
In-Reply-To: <1477553651-13428-3-git-send-email-dingtianhong@huawei.com>
On 27/10/16 08:34, Ding Tianhong wrote:
> Erratum Hisilicon-161601 says that the ARM generic timer counter "has the
> potential to contain an erroneous value when the timer value changes".
> Accesses to TVAL (both read and write) are also affected due to the implicit counter
> read. Accesses to CVAL are not affected.
>
> The workaround is to reread the system count registers until the value of the second
> read is larger than the first one by less than 32, the system counter can be guaranteed
> not to return wrong value twice by back-to-back read and the error value is always larger
> than the correct one by 32. Writes to TVAL are replaced with an equivalent write to CVAL.
>
> The workaround is enabled if the hisilicon,erratum-161601 property is found in
> the timer node in the device tree. This can be overridden with the
> clocksource.arm_arch_timer.hisilicon-161601 boot parameter, which allows KVM
> users to enable the workaround until a mechanism is implemented to
> automatically communicate this information.
>
> Fix some description for fsl erratum a008585.
>
> v2: Significant rework based on feedback, including seperate the fsl erratum a008585
> to another patch, update the erratum name and remove unwanted code.
>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
> Documentation/arm64/silicon-errata.txt | 1 +
> Documentation/kernel-parameters.txt | 9 ++++
> arch/arm64/include/asm/arch_timer.h | 28 ++++++++++-
> drivers/clocksource/Kconfig | 14 +++++-
> drivers/clocksource/arm_arch_timer.c | 88 ++++++++++++++++++++++++++--------
> 5 files changed, 118 insertions(+), 22 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.txt b/Documentation/arm64/silicon-errata.txt
> index 405da11..70c5d5e 100644
> --- a/Documentation/arm64/silicon-errata.txt
> +++ b/Documentation/arm64/silicon-errata.txt
> @@ -63,3 +63,4 @@ stable kernels.
> | Cavium | ThunderX SMMUv2 | #27704 | N/A |
> | | | | |
> | Freescale/NXP | LS2080A/LS1043A | A-008585 | FSL_ERRATUM_A008585 |
> +| Hisilicon | Hip05/Hip06/Hip07 | #161601 | HISILICON_ERRATUM_161601|
I've already commented on the alignment. Please read my initial review.
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 6fa1d8a..735b4b6 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -707,6 +707,15 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
> erratum. If unspecified, the workaround is
> enabled based on the device tree.
>
> + clocksource.arm_arch_timer.hisilicon-161601=
> + [ARM64]
> + Format: <bool>
> + Enable/disable the workaround of Hisilicon
> + erratum 161601. This can be useful for KVM
> + guests, if the guest device tree doesn't show the
> + erratum. If unspecified, the workaround is
> + enabled based on the device tree.
> +
> clearcpuid=BITNUM [X86]
> Disable CPUID feature X for the kernel. See
> arch/x86/include/asm/cpufeatures.h for the valid bit
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index 118719d8..49b3041 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -29,7 +29,7 @@
>
> #include <clocksource/arm_arch_timer.h>
>
> -#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585)
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
> extern struct static_key_false arch_timer_read_ool_enabled;
> #define needs_timer_erratum_workaround() \
> static_branch_unlikely(&arch_timer_read_ool_enabled)
> @@ -65,11 +65,35 @@ extern struct arch_timer_erratum_workaround *erratum_workaround;
> _new; \
> })
>
> +
> +
> +/*
> + * The number of retries is an arbitrary value well beyond the highest number
> + * of iterations the loop has been observed to take.
> + * Verify whether the value of the second read is larger than the first by
> + * less than 32 is the only way to confirm the value is correct, the system
> + * counter can be guaranteed not to return wrong value twice by back-to-back read
> + * and the error value is always larger than the correct one by 32.
> + */
> +#define __hisi_161601_read_reg(reg) ({ \
> + u64 _old, _new; \
> + int _retries = 200; \
Please document how this value was found (either in the code or in the
commit message).
> + \
> + do { \
> + _old = read_sysreg(reg); \
> + _new = read_sysreg(reg); \
> + _retries--; \
> + } while (unlikely((_new - _old) >> 5) && _retries); \
> + \
> + WARN_ON_ONCE(!_retries); \
> + _new; \
> +})
Same remark as in the previous patch.
> +
> #define arch_timer_reg_read_stable(reg) \
> ({ \
> u64 _val; \
> if (needs_timer_erratum_workaround()) \
> - _val = erratum_workaround->read_##reg(); \
> + _val = erratum_workaround->read_##reg();\
> else \
> _val = read_sysreg(reg); \
> _val; \
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 8a753fd..4aafb6a 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -312,8 +312,20 @@ config FSL_ERRATUM_A008585
> help
> This option enables a workaround for Freescale/NXP Erratum
> A-008585 ("ARM generic timer may contain an erroneous
> - value"). The workaround will only be active if the
> + value"). The workaround will be active if the
> fsl,erratum-a008585 property is found in the timer node.
> + This can be overridden with the clocksource.arm_arch_timer.fsl-a008585
> + boot parameter.
Move this hunk to the previous patch.
> +
> +config HISILICON_ERRATUM_161601
> + bool "Workaround for Hisilicon Erratum 161601"
> + default y
> + depends on ARM_ARCH_TIMER && ARM64
> + help
> + This option enables a workaround for Hisilicon Erratum
> + 161601. The workaround will be active if the hisilicon,erratum-161601
> + property is found in the timer node. This can be overridden with
> + the clocksource.arm_arch_timer.hisilicon-161601 boot parameter.
>
> config ARM_GLOBAL_TIMER
> bool "Support for the ARM global timer" if COMPILE_TEST
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index e4f7fa1..89f1895 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -94,13 +94,14 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> * Architected system timer support.
> */
>
> -#ifdef CONFIG_FSL_ERRATUM_A008585
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
> struct arch_timer_erratum_workaround *erratum_workaround = NULL;
>
> DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
> EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
> +#endif
>
> -
> +#ifdef CONFIG_FSL_ERRATUM_A008585
> static u32 fsl_a008585_read_cntp_tval_el0(void)
> {
> return __fsl_a008585_read_reg(cntp_tval_el0);
> @@ -139,6 +140,45 @@ static int __init early_fsl_a008585_cfg(char *buf)
> early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg);
> #endif /* CONFIG_FSL_ERRATUM_A008585 */
>
> +#ifdef CONFIG_HISILICON_ERRATUM_161601
> +static u32 hisi_161601_read_cntp_tval_el0(void)
> +{
> + return __hisi_161601_read_reg(cntp_tval_el0);
> +}
> +
> +static u32 hisi_161601_read_cntv_tval_el0(void)
> +{
> + return __hisi_161601_read_reg(cntv_tval_el0);
> +}
> +
> +static u64 hisi_161601_read_cntvct_el0(void)
> +{
> + return __hisi_161601_read_reg(cntvct_el0);
> +}
> +
> +static struct arch_timer_erratum_workaround arch_timer_hisi_161601 = {
> + .read_cntp_tval_el0 = hisi_161601_read_cntp_tval_el0,
> + .read_cntv_tval_el0 = hisi_161601_read_cntv_tval_el0,
> + .read_cntvct_el0 = hisi_161601_read_cntvct_el0,
> +};
> +
> +static int __init early_hisi_161601_cfg(char *buf)
> +{
> + int ret;
> + bool val;
> +
> + ret = strtobool(buf, &val);
> + if (ret)
> + return ret;
> +
> + if (val)
> + erratum_workaround = &arch_timer_hisi_161601;
> +
> + return 0;
> +}
> +early_param("clocksource.arm_arch_timer.hisilicon-161601", early_hisi_161601_cfg);
> +#endif /* CONFIG_HISILICON_ERRATUM_161601 */
> +
> static __always_inline
> void arch_timer_reg_write(int access, enum arch_timer_reg reg, u32 val,
> struct clock_event_device *clk)
> @@ -288,8 +328,8 @@ static __always_inline void set_next_event(const int access, unsigned long evt,
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> -#ifdef CONFIG_FSL_ERRATUM_A008585
> -static __always_inline void fsl_a008585_set_next_event(const int access,
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
> +static __always_inline void erratum_set_next_event(const int access,
> unsigned long evt, struct clock_event_device *clk)
> {
> unsigned long ctrl;
> @@ -307,20 +347,20 @@ static __always_inline void fsl_a008585_set_next_event(const int access,
> arch_timer_reg_write(access, ARCH_TIMER_REG_CTRL, ctrl, clk);
> }
>
> -static int fsl_a008585_set_next_event_virt(unsigned long evt,
> +static int erratum_set_next_event_virt(unsigned long evt,
> struct clock_event_device *clk)
> {
> - fsl_a008585_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> + erratum_set_next_event(ARCH_TIMER_VIRT_ACCESS, evt, clk);
> return 0;
> }
>
> -static int fsl_a008585_set_next_event_phys(unsigned long evt,
> +static int erratum_set_next_event_phys(unsigned long evt,
> struct clock_event_device *clk)
> {
> - fsl_a008585_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> + erratum_set_next_event(ARCH_TIMER_PHYS_ACCESS, evt, clk);
> return 0;
> }
> -#endif /* CONFIG_FSL_ERRATUM_A008585 */
> +#endif
>
> static int arch_timer_set_next_event_virt(unsigned long evt,
> struct clock_event_device *clk)
> @@ -350,16 +390,16 @@ static int arch_timer_set_next_event_phys_mem(unsigned long evt,
> return 0;
> }
>
> -static void fsl_a008585_set_sne(struct clock_event_device *clk)
> +static void erratum_set_sne(struct clock_event_device *clk)
> {
> -#ifdef CONFIG_FSL_ERRATUM_A008585
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
> if (!static_branch_unlikely(&arch_timer_read_ool_enabled))
> return;
>
> if (arch_timer_uses_ppi == VIRT_PPI)
> - clk->set_next_event = fsl_a008585_set_next_event_virt;
> + clk->set_next_event = erratum_set_next_event_virt;
> else
> - clk->set_next_event = fsl_a008585_set_next_event_phys;
> + clk->set_next_event = erratum_set_next_event_phys;
> #endif
This should be in the previous patch as well, as it only messes with the
FSL erratum.
> }
>
> @@ -392,7 +432,7 @@ static void __arch_timer_setup(unsigned type,
> BUG();
> }
>
> - fsl_a008585_set_sne(clk);
> + erratum_set_sne(clk);
> } else {
> clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
> clk->name = "arch_mem_timer";
> @@ -612,7 +652,7 @@ static void __init arch_counter_register(unsigned type)
>
> clocksource_counter.archdata.vdso_direct = true;
>
> -#ifdef CONFIG_FSL_ERRATUM_A008585
> +#if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585) || IS_ENABLED(CONFIG_HISILICON_ERRATUM_161601)
> /*
> * Don't use the vdso fastpath if errata require using
> * the out-of-line counter accessor.
> @@ -899,12 +939,22 @@ static int __init arch_timer_of_init(struct device_node *np)
> arch_timer_c3stop = !of_property_read_bool(np, "always-on");
>
> #ifdef CONFIG_FSL_ERRATUM_A008585
> - if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585"))
> + if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585")) {
> erratum_workaround = &arch_timer_fsl_a008585;
> + if (erratum_workaround) {
Brilliant!
> + static_branch_enable(&arch_timer_read_ool_enabled);
> + pr_info("Enabling workaround for FSL erratum A-008585\n");
> + }
> + }
> +#endif
>
> - if (erratum_workaround) {
> - static_branch_enable(&arch_timer_read_ool_enabled);
> - pr_info("Enabling workaround for FSL erratum A-008585\n");
> +#ifdef CONFIG_HISILICON_ERRATUM_161601
> + if (!erratum_workaround && of_property_read_bool(np, "hisilicon,erratum-161601")) {
> + erratum_workaround = &arch_timer_hisi_161601;
> + if (erratum_workaround) {
> + static_branch_enable(&arch_timer_read_ool_enabled);
> + pr_info("Enabling workaround for HISILICON erratum 161601\n");
> + }
> }
> #endif
Do you see a pattern here? Surely you can factor a bit of that code (and
remove the nonsensical bits).
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation
From: Felipe Balbi @ 2016-10-27 10:47 UTC (permalink / raw)
To: John Youn
In-Reply-To: <e7a864e5a2af12d5a8d97f2591065f4aa85f86cb.1477424426.git.johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 976 bytes --]
Hi,
John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> writes:
> Add interrupt moderation interval binding for dwc3.
>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---
> Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
> - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> register for post-silicon frame length adjustment when the
> fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.
on top of all other comments, what's the unit here? nanoseconds? clock cycles?
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]
^ permalink raw reply
* [PATCH v2] iommu/ipmmu-vmsa: Add r8a7796 DT binding
From: Magnus Damm @ 2016-10-27 10:45 UTC (permalink / raw)
To: devicetree-u79uwXL29TY76Z2rM5mHXA
Cc: mark.rutland-5wv7dgnIgG8,
laurent.pinchart+renesas-ryLnwIuWjnjg/C1BVhZhaw,
geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ, Magnus Damm,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
horms+renesas-/R6kz+dDXgpPR4JQBCEnsQ
From: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
Update the IPMMU DT binding documentation to include the r8a7796 compat
string for R-Car M3-W.
Signed-off-by: Magnus Damm <damm+renesas-yzvPICuk2ACczHhG9Qg4qA@public.gmane.org>
Acked-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
Changes since V1:
- Added Acked-by from Laurent - thanks!
Now broken out, however earlier V1 posted as part of:
[PATCH 0/3] iommu/ipmmu-vmsa: Initial r8a7796 support
Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt | 1 +
1 file changed, 1 insertion(+)
--- 0001/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt
+++ work/Documentation/devicetree/bindings/iommu/renesas,ipmmu-vmsa.txt 2016-06-06 11:27:37.560607110 +0900
@@ -16,6 +16,7 @@ Required Properties:
- "renesas,ipmmu-r8a7793" for the R8A7793 (R-Car M2-N) IPMMU.
- "renesas,ipmmu-r8a7794" for the R8A7794 (R-Car E2) IPMMU.
- "renesas,ipmmu-r8a7795" for the R8A7795 (R-Car H3) IPMMU.
+ - "renesas,ipmmu-r8a7796" for the R8A7796 (R-Car M3-W) IPMMU.
- "renesas,ipmmu-vmsa" for generic R-Car Gen2 VMSA-compatible IPMMU.
- reg: Base address and size of the IPMMU registers.
^ permalink raw reply
* Re: [PATCH] ARM: DT: stm32: move dma translation to board files
From: Bruno Herrera @ 2016-10-27 10:43 UTC (permalink / raw)
To: Alexandre Torgue
Cc: mark.rutland, devicetree, Arnd Bergmann, linux, >, >,
Maxime Coquelin, Lee Jones, linux-arm-kernel
In-Reply-To: <404315f0-fc9e-93e2-54df-f85e484d9389@st.com>
Hi Alex,
On Wed, Oct 26, 2016 at 7:09 AM, Alexandre Torgue
<alexandre.torgue@st.com> wrote:
> Hi Bruno,
>
> On 10/25/2016 11:06 PM, Bruno Herrera wrote:
>>
>> Hi Alexandre,
>>
>>>
>>> stm32f469-disco and stm32f429-eval boards use SDRAM start address
>>> remapping
>>> (to @0) to boost performances. A DMA translation through "dma-ranges"
>>> property was needed for other masters than the M4 CPU.
>>> stm32f429-disco doesn't use remapping so doesn't need this DMA
>>> translation.
>>> This patches moves this DMA translation definition from stm32f429 soc
>>> file
>>> to board files.
>>>
>>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
>>>
>>> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts
>>> b/arch/arm/boot/dts/stm32429i-eval.dts
>>> index 13c7cd2..a763c15 100644
>>> --- a/arch/arm/boot/dts/stm32429i-eval.dts
>>> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
>>> @@ -82,6 +82,10 @@
>>> };
>>> };
>>>
>>> + soc {
>>> + dma-ranges = <0xc0000000 0x0 0x10000000>;
>>> + };
>>> +
>>> usbotg_hs_phy: usbphy {
>>> #phy-cells = <0>;
>>> compatible = "usb-nop-xceiv";
>>
>>
>> Shouldn't also the peripheral dma-ranges property move to board specific
>> too?
>> I had this patch for while but I didn't had the time to submit:
>
>
> Well spot I forgot it. Actually, discussing with Arnd ysterday on IIRC,
> empty dma-ranges is not needed. Can you test on your side by removing
> dma-ranges in usb node please ?
Unfortunately will take a time for me to set up this environment on
the STM32F4-EVAL board.
And on the discovery boards we dont have this scenario. That was the
main reason I did not submit the patch right away.
My conclusion and I might be wrong but is based on the my tests with
SDIO device at STM32F469I-DISCO board.
I started this issue as discussion at ST Forum but Maxime gave me the hint.
https://my.st.com/public/STe2ecommunities/mcu/Lists/cortex_mx_stm32/Flat.aspx?RootFolder=https%3a%2f%2fmy%2est%2ecom%2fpublic%2fSTe2ecommunities%2fmcu%2fLists%2fcortex_mx_stm32%2fDMA2%20and%20SYSCFG_MEMRMP%20relationship&FolderCTID=0x01200200770978C69A1141439FE559EB459D7580009C4E14902C3CDE46A77F0FFD06506F5B¤tviews=44
> I will push a v2 by removing empty dma-ranges if tests are ok in your side.
>From my understating/conclusion is: when empty property(dma-ranges) is
the device node, the mapping will be taken in consideration when using
DMA otherwise the mapping is ignored.
And in the SDIO case it is needed for DEV->MEM(SDRAM) and
MEM(SDRAM)->DEV. If it is not the case for the devices in question so
I suppose it can work without the property.
>
> Thanks in advance
> Alex
>
>
>>
>> Author: Bruno Herrera <bruherrera@gmail.com>
>> Date: Sun Oct 16 14:50:00 2016 -0200
>>
>> ARM: DT: STM32: Use dma-ranges property per board not at dtsi file
>>
>> diff --git a/arch/arm/boot/dts/stm32429i-eval.dts
>> b/arch/arm/boot/dts/stm32429i-eval.dts
>> index 6bfc595..2a22a82 100644
>> --- a/arch/arm/boot/dts/stm32429i-eval.dts
>> +++ b/arch/arm/boot/dts/stm32429i-eval.dts
>> @@ -52,6 +52,10 @@
>> model = "STMicroelectronics STM32429i-EVAL board";
>> compatible = "st,stm32429i-eval", "st,stm32f429";
>>
>> + soc {
>> + dma-ranges = <0xC0000000 0x0 0x10000000>;
>> + };
>> +
>> chosen {
>> bootargs = "root=/dev/ram rdinit=/linuxrc";
>> stdout-path = "serial0:115200n8";
>> @@ -96,6 +100,7 @@
>>
>> ðernet0 {
>> status = "okay";
>> + dma-ranges;
>> pinctrl-0 = <ðernet0_mii>;
>> pinctrl-names = "default";
>> phy-mode = "mii-id";
>> @@ -116,6 +121,7 @@
>> };
>>
>> &usbotg_hs {
>> + dma-ranges;
>> dr_mode = "host";
>> phys = <&usbotg_hs_phy>;
>> phy-names = "usb2-phy";
>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi
>> b/arch/arm/boot/dts/stm32f429.dtsi
>> index 7d624a2..697a133 100644
>> --- a/arch/arm/boot/dts/stm32f429.dtsi
>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>> @@ -59,7 +59,6 @@
>> };
>>
>> soc {
>> - dma-ranges = <0xc0000000 0x0 0x10000000>;
>>
>> timer2: timer@40000000 {
>> compatible = "st,stm32-timer";
>> @@ -472,13 +471,11 @@
>> st,syscon = <&syscfg 0x4>;
>> snps,pbl = <8>;
>> snps,mixed-burst;
>> - dma-ranges;
>> status = "disabled";
>> };
>>
>> usbotg_hs: usb@40040000 {
>> compatible = "snps,dwc2";
>> - dma-ranges;
>> reg = <0x40040000 0x40000>;
>> interrupts = <77>;
>> clocks = <&rcc 0 29>;
>>
>>
>>> diff --git a/arch/arm/boot/dts/stm32f429.dtsi
>>> b/arch/arm/boot/dts/stm32f429.dtsi
>>> index 0596d60..3a1cfdd 100644
>>> --- a/arch/arm/boot/dts/stm32f429.dtsi
>>> +++ b/arch/arm/boot/dts/stm32f429.dtsi
>>> @@ -59,8 +59,6 @@
>>> };
>>>
>>> soc {
>>> - dma-ranges = <0xc0000000 0x0 0x10000000>;
>>> -
>>> timer2: timer@40000000 {
>>> compatible = "st,stm32-timer";
>>> reg = <0x40000000 0x400>;
>>> diff --git a/arch/arm/boot/dts/stm32f469-disco.dts
>>> b/arch/arm/boot/dts/stm32f469-disco.dts
>>> index 9e73656..c2213c0 100644
>>> --- a/arch/arm/boot/dts/stm32f469-disco.dts
>>> +++ b/arch/arm/boot/dts/stm32f469-disco.dts
>>> @@ -64,6 +64,10 @@
>>> aliases {
>>> serial0 = &usart3;
>>> };
>>> +
>>> + soc {
>>> + dma-ranges = <0xc0000000 0x0 0x10000000>;
>>> + };
>>> };
>>>
>>> &clk_hse {
>>> --
>>
>>
>>
>> Br.,
>> Bruno
>>
>
^ permalink raw reply
* Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq
From: Jerome Brunet @ 2016-10-27 10:42 UTC (permalink / raw)
To: Linus Walleij
Cc: devicetree@vger.kernel.org, Jason Cooper, Marc Zyngier,
Kevin Hilman, Will Deacon, linux-kernel@vger.kernel.org,
Russell King, linux-gpio@vger.kernel.org, Rob Herring,
Catalin Marinas, Carlo Caione, open list:ARM/Amlogic Meson...,
Thomas Gleixner, linux-arm-kernel@lists.infradead.org
In-Reply-To: <CACRpkdZR8T8W5WMmEJVhPR0sUQyfPzgHpjhDiRsyXvybKDYJWQ@mail.gmail.com>
On Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote:
> On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet <jbrunet@baylibre.com>
> wrote:
> >
> > [Me]
> > >
> > > We usually refer to the local numberspace on the GPIO controller
> > > as "offsets", so line offsets 0...31 on a gpiochip with 31 lines.
> > >
> > > The ngpio in struct gpio_chip is the number of lines on that
> > > controller,
> > > and should nominally map 1:1 to hwirq sources.
> >
> > Indeed it should be the the case, and for meson, it is pretty
> > close.
> > The irqchip controller provide a number of hwirq. Each hwirq maps
> > to
> > one, and only one, pin. But since not every pins are connected to
> > the
> > irqchip controller, the opposite is not true.
> >
> > Taking an example with 16 gpios, here is what it could look like
> > with
> > the exception we have on meson :
> >
> > gpio offset [ 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 ]
> > hwirq num [ 0 1 2 3] NC NC[4 5 6 7 8 9 10]NC NC NC
> >
> > Like gpio offset are used (internally) in the driver to find
> > appropriate gpio registers and bit, the hwirq has a meaning too.
> > It is the setting you put in the channel multiplexer of the
> > controller
> > to select the proper pin to spy on.
> >
> > In the end, these gpio offset and hwirq number are different. I
> > would
> > prefer to have hwirq == gpio and go your way, it would make my life
> > easier, but I don't see how it would work.
> >
> > The irqchip controller cares only about the hwirq number. You can
> > actually request an interrupt directly to the controller by asking
> > the
> > proper hwirq number (in DT for example), without involving the gpio
> > driver (tested).
> >
> > The relation between the pins and the interrupt number is provided
> > by
> > the manufacturer in the Datasheet [1], in the section GPIO
> > Interrupt.
>
> I think I kind of get it.
>
> This reminds me of recent patches to the generic GPIOLIB_IRQCHIP
> where we made it possible to "mask" set of IRQ lines, saying
> "these are in the irqdomain, but you cannot map them".
>
> See
> commit 79b804cb6af4f128b2c53f0887c02537a7eb5824
> "gpiolib: Make it possible to exclude GPIOs from IRQ domain"
> commit 96b2cca64fa3e1a31b573bb308b2944c802aacf3
> "gpio: stmpe: forbid unused lines to be mapped as IRQs"
>
> So what we do in the generic case is put a linear map over all
> the lines/offsets, then punch holes in that map and say
> "this and this and this can not be mapped as IRQ".
>
> As you can see in _gpiochip_irqchip_add() we pre-map all
> except those with irq_create_mapping().
>
> Does this scheme fit with your usecase? I would guess so,
> just that your domain is hierarchic, not simple/linear.
Thanks for pointing this out, however I don't think this solve my
issue. I'll try to be as clear as possible but feel free to ask me
question if needed:
Ressource issue : When you create an irq mapping, in case of hierarchic
domain, it calls the "alloc" function of the domain, which will
eventually call the "alloc" function of the parent domain ... until you
reach the "root" domain (here the gic).
The particular HW at hand (meson gpio interrupt controller) is a set of
8 muxes (or channels). Each channel output its signal on 1 specific GIC
input. Its the HW wiring, not programmable.
The inputs are the all pad that can be seen by the controller (*almost*
all the SoC gpio, but not all, as explain earlier). When you call
"alloc", the driver find an available channel, set the mux input to
forward the appropriate signal to the GIC.
As you may understand, the driver can accept only 8 mappings at a time
before being out of GIC irqs, and returning -ENOSPC.
If we were trying the use the punch hole method, we would have to know
at boot time the only eight pin we want, and this for every platform.
Also there not might be 8 available to the gpio subsys, since someone
could request an irq directly to controller, w/o going through the gpio
subsys. This would be putting restriction on the gpio because of an
issue in the controller. This looks very complicated to setup, static
and platform specific. That's not really what we were aiming for.
We are looking to create mapping "on-demand" to make the best use of
the little number of interrupts we have. To catch request of drivers,
like gpio-keys, which use gpio_to_irq, it looks the only viable place
is the to_irq callback (at the moment).
Drivers using gpio_to_irq in their probe function expect that this will
give them the corresponding virq, so create the mapping if need be.
However, I now get why you don't want that, it seems we have 2 types of
platforms in the kernel right now:
1. The one creating the mapping in the to_irq callback. It might be
because they just copy/paste from another driver doing it, or they may
have a good reason for it (like I think we do)
2. the one which call gpio_to_irq in interrupt handlers. Honestly I did
not know that one could that, but they are in the mainline too, and
probably have a good reason for doing it.
irq_find_mapping looks safe in interrupt handler, I does not seem to
sleep (please correct me if I'm wrong).
irq_create_mapping definitely isn't, because of the irq_domain mutex.
We probably got into this situation because it wasn't clear enough
whether to_irq was fast or slow (at least it took me a few mails to
understand this ...)
The two platform groups are most likely exclusive so nobody is sleeping
in irq, everybody is happy. As a maintainer, I understand that you
can't allow a dangerous situation like this to continue.
To fix the situation we could add a few things in the gpio subsys:
- Make it clear that to_irq is fast (like you just did)
- add a new callback (to_irq_slow ? provide_irq ? something else) which
would be allowed to sleep and create mappings.
- in gpio_to_irq function keeps calling to_irq like it does but also
call to_irq_slow only if we are not in an interrupt context and a
mapping could not be found. We could maybe use "in_interrupt()" for
this ?
This way, we could keep the existing drivers working as they are (even
if they are wrong) and slowly fix things up.
What do you think about this ? Do you have something else in mind ?
I'd be happy to help on this.
Sorry, it was kind of long explanation but I hope things are more clear
now.
>
> Maybe the takeaway is that your map does not need to
> be "dense", i.e. every hwirq is in use. It can be sparse.
> It is stored in a radix tree anyways.
>
> >
> > Looking at other gpio drivers, it is not uncommon to have some
> > simple
> > calculation to get from gpio offset to the hwirq number. I don't
> > get
> > what is the specific problem here ?
>
> It's OK to use the offset vs hwirq.
>
> I just misunderstand it as the global GPIO number, that is
> not OK.
Ok. Just to be clear, you are ok with the function
"meson_gpio_to_hwirq" which just does this translation, right ?
>
> Yours,
> Linus Walleij
Cheers
Jerome
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/4] Input: goodix - Support interchanging x and y coordinates in hardware
From: Bastien Nocera @ 2016-10-27 10:34 UTC (permalink / raw)
To: Rob Herring, Franklin S Cooper Jr
Cc: dmitry.torokhov, octavian.purdila, irina.tirdea, merker,
linux-input, devicetree, linux-kernel, nsekhar, nm
In-Reply-To: <20161026231816.a35orxe3mpsvcx3m@rob-hp-laptop>
On Wed, 2016-10-26 at 18:18 -0500, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 02:59:17PM -0500, Franklin S Cooper Jr wrote:
> >
<snip>
> I'm not sure I follow why existing properties don't cover this.
Me neither. I certainly don't understand why the driver can't mangle
the data from the touchscreen all it wants. It's not like user-space is
talking to the touchscreen directly.
^ permalink raw reply
* Re: [PATCH 3/4] Input: goodix - Tweak configuration to use passed in touchscreen resolution
From: Bastien Nocera @ 2016-10-27 10:34 UTC (permalink / raw)
To: Franklin S Cooper Jr, dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
octavian.purdila-ral2JQCrhuEAvxtiuMwx3w,
irina.tirdea-ral2JQCrhuEAvxtiuMwx3w,
merker-8fiUuRrzOP0dnm+yROfE0A, linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0,
nm-l0cyMroinI0
In-Reply-To: <20161020195917.20051-4-fcooper-l0cyMroinI0@public.gmane.org>
On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Some goodix touchscreen controllers aren't programed properly to
> match the
> display panel it is used on. Although the defaults may largely work
> it is
> also likely that the screen resolution will be incorrect. Therefore,
> add support to allow via DT for the touchscreen resolution to be set
> and
> reprogram the controller to use this updated resolution.
>
> Signed-off-by: Franklin S Cooper Jr <fcooper-l0cyMroinI0@public.gmane.org>
> ---
> .../bindings/input/touchscreen/goodix.txt | 2 ++
> drivers/input/touchscreen/goodix.c | 26
> +++++++++++++++++++++-
> 2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git
> a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> index c98757a..ebc7cb7 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.txt
> @@ -19,6 +19,8 @@ Optional properties:
> interrupt gpio pin as output to reset the
> device.
> - reset-gpios : GPIO pin used for reset
>
> + - touchscreen-size-x : horizontal resolution of touchscreen
> (in pixels)
> + - touchscreen-size-y : vertical resolution of touchscreen (in
> pixels)
> - touchscreen-inverted-x : X axis is inverted (boolean)
> - touchscreen-inverted-y : Y axis is inverted (boolean)
> - touchscreen-swapped-x-y : X and Y axis are swapped (boolean)
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index 01e12f8..c2428e1 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -496,6 +496,10 @@ static void goodix_tweak_config(struct
> goodix_ts_data *ts)
> return;
> }
>
> + /* Setting X and Y Resolution */
> + put_unaligned_le16(ts->abs_x_max, &config[RESOLUTION_LOC]);
> + put_unaligned_le16(ts->abs_y_max, &config[RESOLUTION_LOC +
> 2]);
> +
> check_sum = goodix_calculate_checksum(ts->cfg_len, config);
>
> config[raw_cfg_len] = check_sum;
> @@ -669,6 +673,8 @@ static int goodix_request_input_dev(struct
> goodix_ts_data *ts)
> static int goodix_configure_dev(struct goodix_ts_data *ts)
> {
> int error;
> + bool alter_config = false;
> + u32 max_x, max_y;
>
> ts->swapped_x_y = device_property_read_bool(&ts->client-
> >dev,
> "touchscreen-
> swapped-x-y");
> @@ -676,9 +682,27 @@ static int goodix_configure_dev(struct
> goodix_ts_data *ts)
> "touchscreen-
> inverted-x");
> ts->inverted_y = device_property_read_bool(&ts->client->dev,
> "touchscreen-
> inverted-y");
> -
No need for that linefeed removal.
> goodix_read_config(ts);
>
> + if (device_property_present(&ts->client->dev, "touchscreen-
> size-x") &&
Is it expected that incomplete or invalid arguments do not generate
errors? I'd expect that the presence of just one of those properties,
or...
> + device_property_present(&ts->client->dev, "touchscreen-
> size-y")) {
> +
> + device_property_read_u32(&ts->client->dev,
> "touchscreen-size-x",
> + &max_x);
> +
> + device_property_read_u32(&ts->client->dev,
> "touchscreen-size-y",
> + &max_y);
> +
> + if (max_x > 0 && max_y > 0) {
... invalid values for those properties would throw errors (either
warnings, or lower severity messages).
> + ts->abs_x_max = max_x;
> + ts->abs_y_max = max_y;
> + alter_config = true;
> + }
> + }
> +
> + if (alter_config)
> + goodix_tweak_config(ts);
> +
> error = goodix_request_input_dev(ts);
> if (error)
> return error;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/4] Input: goodix - Allow tweaking of configuration file dynamically
From: Bastien Nocera @ 2016-10-27 10:33 UTC (permalink / raw)
To: Franklin S Cooper Jr, dmitry.torokhov, robh+dt, octavian.purdila,
irina.tirdea, merker, linux-input, devicetree, linux-kernel,
nsekhar, nm
In-Reply-To: <20161020195917.20051-3-fcooper@ti.com>
On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Some goodix touchscreen controllers don't have the correct
> configuration
> firmware for the display panel it is attached to. Therefore, settings
> such
> as touchscreen x and y size may need to be passed in via DT and have
> the
> panel reprogrammed for the updated touchscreen resolution.
>
> This patchset adds support for reading the current configuration
> firmware
> on the panel and allowing it to be modified and rewritten back to the
> device.
>
> Currently this function is unused but later patches will make use of
> it.
>
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> drivers/input/touchscreen/goodix.c | 28 ++++++++++++++++++++++++++++
> 1 file changed, 28 insertions(+)
>
> diff --git a/drivers/input/touchscreen/goodix.c
> b/drivers/input/touchscreen/goodix.c
> index a43c8ca..01e12f8 100644
> --- a/drivers/input/touchscreen/goodix.c
> +++ b/drivers/input/touchscreen/goodix.c
> @@ -478,6 +478,34 @@ static int goodix_get_gpio_config(struct
> goodix_ts_data *ts)
> return 0;
> }
>
> +static void goodix_tweak_config(struct goodix_ts_data *ts)
> +{
> + u8 config[GOODIX_CONFIG_MAX_LENGTH];
> + int error;
> + int raw_cfg_len;
> + u8 check_sum = 0;
> +
> + raw_cfg_len = ts->cfg_len - 2;
> +
> + error = goodix_i2c_read(ts->client, GOODIX_REG_CONFIG_DATA,
> + config, ts->cfg_len);
> + if (error) {
> + dev_warn(&ts->client->dev,
> + "Error reading config (%d), avoid tweaking
> config\n",
> + error);
> + return;
> + }
Please add a placeholder comment here, to go along with the commit
message explanation.
> + check_sum = goodix_calculate_checksum(ts->cfg_len, config);
> +
> + config[raw_cfg_len] = check_sum;
> + config[raw_cfg_len + 1] = 1;
> +
> + error = goodix_send_cfg(ts, ts->cfg_len, config);
> + if (error)
> + dev_warn(&ts->client->dev,
> + "Error writing config (%d)\n", error);
> +}
> /**
> * goodix_read_config - Read the embedded configuration of the panel
> *
^ permalink raw reply
* Re: [PATCH 1/4] Input: goodix - Restructure cfg checksum function
From: Bastien Nocera @ 2016-10-27 10:32 UTC (permalink / raw)
To: Franklin S Cooper Jr, dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
octavian.purdila-ral2JQCrhuEAvxtiuMwx3w,
irina.tirdea-ral2JQCrhuEAvxtiuMwx3w,
merker-8fiUuRrzOP0dnm+yROfE0A, linux-input-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, nsekhar-l0cyMroinI0,
nm-l0cyMroinI0
In-Reply-To: <20161020195917.20051-2-fcooper-l0cyMroinI0@public.gmane.org>
On Thu, 2016-10-20 at 14:59 -0500, Franklin S Cooper Jr wrote:
> Split generation of checksum into is own function. Currently it is used
"its own".
> only to determine if the checksum for the user provided configuration file
> is valid or not. However, support to dynamically alter the configuration
> file will be supported and needs a way to calculate the updated file's
> checksum. Therefore, by splitting calculating checksum into its own
> function the code can be reused.
>
After the fixup above:
Reviewed-by: Bastien Nocera <hadess-0MeiytkfxGOsTnJN9+BGXg@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v2 2/4] arm64: arch_timer: Introduce a generic erratum handing mechanism for fsl-a008585
From: Marc Zyngier @ 2016-10-27 10:29 UTC (permalink / raw)
To: Ding Tianhong, catalin.marinas, will.deacon, mark.rutland, oss,
devicetree, shawnguo, stuart.yoder, linux-arm-kernel, linuxarm
In-Reply-To: <1477553651-13428-2-git-send-email-dingtianhong@huawei.com>
On 27/10/16 08:34, Ding Tianhong wrote:
> The workaround for hisilicon,161601 will check the return value of the system counter
> by different way, in order to distinguish with the fsl-a008585 workaround, introduce
> a new generic erratum handing mechanism for fsl-a008585 and rename some functions.
>
> v2: Introducing a new generic erratum handling mechanism for fsl erratum a008585.
>
> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> ---
> arch/arm64/include/asm/arch_timer.h | 20 +++++++++-----
> drivers/clocksource/arm_arch_timer.c | 51 +++++++++++++++++++++---------------
> 2 files changed, 43 insertions(+), 28 deletions(-)
>
> diff --git a/arch/arm64/include/asm/arch_timer.h b/arch/arm64/include/asm/arch_timer.h
> index eaa5bbe..118719d8 100644
> --- a/arch/arm64/include/asm/arch_timer.h
> +++ b/arch/arm64/include/asm/arch_timer.h
> @@ -31,15 +31,21 @@
>
> #if IS_ENABLED(CONFIG_FSL_ERRATUM_A008585)
> extern struct static_key_false arch_timer_read_ool_enabled;
> -#define needs_fsl_a008585_workaround() \
> +#define needs_timer_erratum_workaround() \
> static_branch_unlikely(&arch_timer_read_ool_enabled)
This is too generic a name. Please make it more specific.
> #else
> -#define needs_fsl_a008585_workaround() false
> +#define needs_timer_erratum_workaround() false
> #endif
>
> -u32 __fsl_a008585_read_cntp_tval_el0(void);
> -u32 __fsl_a008585_read_cntv_tval_el0(void);
> -u64 __fsl_a008585_read_cntvct_el0(void);
> +
> +struct arch_timer_erratum_workaround {
> + int erratum;
What is the use of this field?
> + u32 (*read_cntp_tval_el0)(void);
> + u32 (*read_cntv_tval_el0)(void);
> + u64 (*read_cntvct_el0)(void);
> +};
> +
> +extern struct arch_timer_erratum_workaround *erratum_workaround;
This is a very generic name for something that has a global visibility.
Please choose a more specific variable name.
>
> /*
> * The number of retries is an arbitrary value well beyond the highest number
> @@ -62,8 +68,8 @@ u64 __fsl_a008585_read_cntvct_el0(void);
> #define arch_timer_reg_read_stable(reg) \
> ({ \
> u64 _val; \
> - if (needs_fsl_a008585_workaround()) \
> - _val = __fsl_a008585_read_##reg(); \
> + if (needs_timer_erratum_workaround()) \
> + _val = erratum_workaround->read_##reg(); \
As mentioned in my initial review, you've now broken modular access to
any of the registers. Please fix it.
> else \
> _val = read_sysreg(reg); \
> _val; \
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 73c487d..e4f7fa1 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -95,10 +95,32 @@ early_param("clocksource.arm_arch_timer.evtstrm", early_evtstrm_cfg);
> */
>
> #ifdef CONFIG_FSL_ERRATUM_A008585
> +struct arch_timer_erratum_workaround *erratum_workaround = NULL;
> +
> DEFINE_STATIC_KEY_FALSE(arch_timer_read_ool_enabled);
> EXPORT_SYMBOL_GPL(arch_timer_read_ool_enabled);
>
> -static int fsl_a008585_enable = -1;
> +
> +static u32 fsl_a008585_read_cntp_tval_el0(void)
> +{
> + return __fsl_a008585_read_reg(cntp_tval_el0);
> +}
> +
> +static u32 fsl_a008585_read_cntv_tval_el0(void)
> +{
> + return __fsl_a008585_read_reg(cntv_tval_el0);
> +}
> +
> +static u64 fsl_a008585_read_cntvct_el0(void)
> +{
> + return __fsl_a008585_read_reg(cntvct_el0);
> +}
So now that you've indirected all calls through a set of pointers, why
do you keep the __fsl_a008585_read_reg() macro inside the include file?
I don't think it has any purpose in being globally visible now.
> +
> +static struct arch_timer_erratum_workaround arch_timer_fsl_a008585 = {
And here's the proof that the erratum field is useless.
> + .read_cntp_tval_el0 = fsl_a008585_read_cntp_tval_el0,
> + .read_cntv_tval_el0 = fsl_a008585_read_cntv_tval_el0,
> + .read_cntvct_el0 = fsl_a008585_read_cntvct_el0,
> +};
>
> static int __init early_fsl_a008585_cfg(char *buf)
> {
> @@ -109,26 +131,12 @@ static int __init early_fsl_a008585_cfg(char *buf)
> if (ret)
> return ret;
>
> - fsl_a008585_enable = val;
> + if (val)
> + erratum_workaround = &arch_timer_fsl_a008585;
> +
> return 0;
> }
> early_param("clocksource.arm_arch_timer.fsl-a008585", early_fsl_a008585_cfg);
> -
> -u32 __fsl_a008585_read_cntp_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntp_tval_el0);
> -}
> -
> -u32 __fsl_a008585_read_cntv_tval_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntv_tval_el0);
> -}
> -
> -u64 __fsl_a008585_read_cntvct_el0(void)
> -{
> - return __fsl_a008585_read_reg(cntvct_el0);
> -}
> -EXPORT_SYMBOL(__fsl_a008585_read_cntvct_el0);
> #endif /* CONFIG_FSL_ERRATUM_A008585 */
>
> static __always_inline
> @@ -891,9 +899,10 @@ static int __init arch_timer_of_init(struct device_node *np)
> arch_timer_c3stop = !of_property_read_bool(np, "always-on");
>
> #ifdef CONFIG_FSL_ERRATUM_A008585
> - if (fsl_a008585_enable < 0)
> - fsl_a008585_enable = of_property_read_bool(np, "fsl,erratum-a008585");
> - if (fsl_a008585_enable) {
> + if (!erratum_workaround && of_property_read_bool(np, "fsl,erratum-a008585"))
> + erratum_workaround = &arch_timer_fsl_a008585;
It used to be possible to disable the erratum workaround on the command
line, and you've just removed that feature. Please restore it.
> +
> + if (erratum_workaround) {
> static_branch_enable(&arch_timer_read_ool_enabled);
> pr_info("Enabling workaround for FSL erratum A-008585\n");
> }
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] arm64: dts: r8a7795: salvator-x: add bias setting for usb1_pins
From: Yoshihiro Shimoda @ 2016-10-27 10:29 UTC (permalink / raw)
To: horms-/R6kz+dDXgpPR4JQBCEnsQ, magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA, Yoshihiro Shimoda
Since this board doesn't mount pull-up/down registers for
USB1_{OVC,PWEN} pins, we should enable bias setting to pull these
pins up/down.
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
index da3e996b..bcaf400 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
+++ b/arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts
@@ -309,8 +309,20 @@
};
usb1_pins: usb1 {
- groups = "usb1";
- function = "usb1";
+ mux {
+ groups = "usb1";
+ function = "usb1";
+ };
+
+ ovc {
+ pins = "GP_6_27";
+ bias-pull-up;
+ };
+
+ pwen {
+ pins = "GP_6_26";
+ bias-pull-down;
+ };
};
usb2_pins: usb2 {
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 5/5] ARM: dts: Add LEGO MINDSTORTMS EV3 dts
From: Sekhar Nori @ 2016-10-27 10:05 UTC (permalink / raw)
To: David Lechner, Kevin Hilman
Cc: Mark Rutland, devicetree, Russell King, linux-kernel, Rob Herring,
linux-arm-kernel
In-Reply-To: <1593441f-d147-4c91-8aab-8622dd8ced19@lechnology.com>
On Monday 24 October 2016 09:20 PM, David Lechner wrote:
> On 10/24/2016 06:58 AM, Sekhar Nori wrote:
>> On Saturday 22 October 2016 12:06 AM, David Lechner wrote:
>>> This adds a device tree definition file for LEGO MINDSTORMS EV3.
>>
>> Thanks for the patch!
>>
>>>
>>> What is working:
>>>
>>> * Pin muxing
>>> * MicroSD card reader
>>> * UART on input port 1
>>>
>>> What is partially working:
>>>
>>> * Buttons - working after GPIO fix
>>> * LEDs - working after GPIO fix
>>> * Poweroff/reset - working after GPIO fix
>>
>> Is the GPIO fix something that will go in v4.9-rc cycle ?
>
> Not sure. This is still being discussed.
>
> http://www.gossamer-threads.com/lists/linux/kernel/2550178
>
>>
>>> * Flash memory - driver loads but can't read the block devices - this is
>>> probably due to the fact that we are not able to configure the SPI to
>>> use DMA via device tree
>>
>> Hmm, I would not have expected PIO mode to be so inefficient that you
>> are unable to even read the block device.
>
> I am getting a -EIO error. I haven't been able to trace down exactly
> what is causing it yet though.
Okay. Seems unrelated to DMA though. Will check the status of SPI on my
DA850 EVM once I get the chance.
>
>>
> ...
>>> +/ {
>>> + compatible = "lego,ev3", "ti,da850";
>>> + model = "LEGO MINDSTORMS EV3";
>>> +
>>> + soc@1c00000 {
>>> + /*
>>> + * (ab)using pinctrl-single to disable all internal pullups/
>>> + * pulldowns on I/O.
>>> + */
>>> + pinmux@22c00c {
>>> + compatible = "pinctrl-single";
>>> + reg = <0x22c00c 0x4>;
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + pinctrl-single,bit-per-mux;
>>> + pinctrl-single,register-width = <32>;
>>> + pinctrl-single,function-mask = <0xf>;
>>> + /*
>>> + * There is a bug in pinctrl-single that prevents us
>>> + * from setting function-mask to 1, so doing things
>>> + * in groups of 4. Doesn't really matter since we are
>>> + * disabling all at once anyway.
>>> + */
>>> +
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&pupu_disable>;
>>> +
>>> + pupu_disable: pinmux_all_pins {
>>> + pinctrl-single,bits = <
>>> + 0x0 0x00000000 0xffffffff
>>> + >;
>>> + };
>>
>> Sigh. This is quite an abuse :)
>>
>> I know we don't have a good way to configure this in kernel today. And I
>> am surprised we never had to care about disabling pullups so far. Can
>> you clarify why you need it? I assume there is some contention you want
>> to avoid, but on which interface?
>
> The EV3 was designed with external pullup/pulldown everywhere. I know
> for certain that it breaks one of the buttons if you do not disable the
> internal ones. I imagine that it would have subtle effects elsewhere if
> they are not disabled.
>
> I have not gone through each pullup/pulldown bank individually, but it
> would not surprise me at all if there was at least one thing on most of
> them that would be adversely affected.
>
>>
>> I dont think this can be done this way using pinctrl-single. A small
>> driver to handle pullup/down control for da850 may have to be added to
>> drivers/pinctrl. It will be better to check with Linus Walleij on his
>> thoughts using a new thread ccing the pinctrl subsystem list as well.
>
> I will be glad to try to make a driver, but when I ran into this problem
> I could not find much information on how to handle banks of
> pullup/pulldown. Most of what I saw was for ones that can be
> individually controlled. If anyone knows something like this already
> that I could look at, it would be helpful to me.
pinctrl-single supports a different compatible "pinconf-single" which
supports the pinconf interface that can be used to set pullup and
pulldown bias.
Unfortunately, in case of AM18x, there are two registers to touch:
PUPD_ENA and PUPD_SEL. So I believe the pinctrl-single driver cannot be
used.
There are pinconf operations pin_config_group_{set|get}(). Based on
documentation these are meant to act on a group of pins. There are quite
a few drivers implementing this interface. Perhaps they can be used as
an example.
Also, this needs to be done in a way that can be used by other boards
too, so please move the common properties to da850.dtsi. Only the board
specific pullup/down configuration should be in this file.
>
>
>> [...]
>>
>>> + in1_pins: pinmux_in1_pins {
>>> + pinctrl-single,bits = <
>>> + /* GP0[15] */
>>> + 0x0 0x00000008 0x0000000f
>>> + /* GP0[2] */
>>> + 0x4 0x00800000 0x00f00000
>>> + /* GP2[2] */
>>> + 0x18 0x00800000 0x00f00000
>>> + /* GP8[10], GP8[11] */
>>> + 0x48 0x88000000 0xff000000
>>> + >;
>>> + };
>>
>> I see that this is not really used. Can you add these when you actually
>> use them. Looks like that applies to some other definitions like this
>> below.
>
> It will be possible to uses these gpios via sysfs (until a proper driver
> for input and output ports is merged). So how about I attach these to
> the gpio node for now?
pins are usually attached to the node that consumes the GPIO pins, not
to the gpio node itself.
If the pins are not connected to anything and can be used via gpio
sysfs, then "pinctrl hog" or "board level pinmux" is to be used.
Something like below:
+&pmx_core {
+ pinctrl-names = "default";
+ pinctrl-0 = <&in1_pins>;
+
+ in1_pins: pinmux_in1_pins {
+ pinctrl-single,bits = <
+ 0x0 0x00000008 0x0000000f
+ ...
+ >;
+ };
+};
That said, device tree is supposed to describe hardware and not change
depend on current driver support. So I would wait for the driver to get
merged before merging the pins that are used by it. It looks like you
wont be able to use the support in a meaningful way till then anyway.
>
>>
>>> +&ehrpwm1 {
>>> + status = "disabled";
>>
>> Hmm, disabled? Can you add this node when you actually use it?
>
> Not sure why I have this disabled. Like the gpios, the pwms can be used
> via sysfs, so I would like to leave them.
Okay, then please keep the node enabled.
>
>>
>>> + pinctrl-names = "default";
>>> + /* MBPWM, MAPWM */
>>> + pinctrl-0 = <&ehrpwm1a_pins>, <&ehrpwm1b_pins>;
>>> +};
>>> +
>>> +&ecap1 {
>>> + status = "disabled";
>>
>> same here and other places below.
>>
>>> + pinctrl-names = "default";
>>> + /* MDPWM */
>>> + pinctrl-0 = <&ecap1_pins>;
>>> +};
>>> +
>>> +&spi0 {
>>> + status = "okay";
>>> + pinctrl-names = "default";
>>> + pinctrl-0 = <&spi0_pins>, <&spi0_cs0_pin>, <&spi0_cs3_pin>;
>>> + dmas = <&edma0 14 0>, <&edma0 15 0>;
>>> + dma-names = "rx", "tx";
>>> +
>>> + spi-flash@0 {
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> + compatible = "n25q128a13", "jedec,spi-nor";
>>> + reg = <0>;
>>> + spi-max-frequency = <50000000>;
>>> + ti,spi-wdelay = <8>;
>>> +
>>> + partition@0 {
>>> + label = "U-Boot";
>>> + reg = <0 0x40000>;
>>
>> Thats 256KB for U-Boot and MLO (I assume in concatenated AIS image). Is
>> that sufficient for future too? Moving partitions later is tough ask
>> because that means users will lose data when they upgrade the kernel
>> because of partitions moving around. Just a suggestion to keep future
>> U-Boot bloat in mind and not use a "just fits" number.
>
> The MLO is on an EEPROM in the EV3, so the U-Boot partition is just
> U-boot. The SoC boots from I2C, which then runs whatever is as 0x0 on
> the flash memory.
okay.
> This partition table matches the partition scheme used on the official
> LEGO firmware that ships with the devices. Most people running their own
> kernel will probably be loading it from a microSD card, leaving the
> official firmware intact and therefore will always have this partition
> table.
>
> My thinking is that if someone does want to use a different partitioning
> scheme, they can build their own U-Boot and configure it to modify the
> device tree with a new partition table.
>
> The way the LEGO firmware flashing utility works, it wipes out the
> entire flash memory each time you flash the firmware. So, data loss is
> not a concern - you will loose your data anyway.
Alright, thanks for the detailed explanation.
Thanks,
Sekhar
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: interrupt-controller: add DT binding for meson GPIO interrupt controller
From: Jerome Brunet @ 2016-10-27 9:40 UTC (permalink / raw)
To: Mark Rutland, Rob Herring
Cc: devicetree, Jason Cooper, Marc Zyngier, Kevin Hilman,
Linus Walleij, Will Deacon, linux-kernel, Russell King,
linux-gpio, Catalin Marinas, Carlo Caione, linux-amlogic,
Thomas Gleixner, linux-arm-kernel
In-Reply-To: <20161027093205.GA27135@leverpostej>
On Thu, 2016-10-27 at 10:32 +0100, Mark Rutland wrote:
> On Wed, Oct 26, 2016 at 04:42:35PM -0500, Rob Herring wrote:
> >
> > On Wed, Oct 19, 2016 at 05:21:13PM +0200, Jerome Brunet wrote:
> > >
> > >
> > > This commit adds the device tree bindings description for
> > > Amlogic's GPIO
> > > interrupt controller available on the meson8, meson8b and gxbb
> > > SoC families
> > >
> > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > > ---
> > > Rob, I did not include the Ack you gave for the RFC as bindings
> > > have slightly
> > > changed. Only the interrupt property has be removed following a
> > > discussion I
> > > had with Marc
> >
> > As Mark R already said, you should keep the interrupts property.
>
> To be clear, the interrupt routing should be described *somehow*,
> though
> I don't think the generic interrupts property is correct, as this is
> an
> interrupt *router*, i.e. this device doesn't own the interrupt, it
> just
> joins two parts of the line together (and so flags are meaningless).
>
> Thanks,
> Mark.
Indeed Mark,
I already rewritten the driver taking Marc's comment into account.
There will be 2 properties added to the DT:
- meson,upstream-interrupts : an array with the 8 gic interrupt used
as output of the controller
- meson,num-input-mux : the number of inputs (pads) available to the
muxes
I'm looking for solution to the "create mapping" issue we have in
pinctrl (patch 4) before posting a v3.
If you think these properties should be different, feel free to tell me
now.
Thx
Jerome
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH v3 4/4] ARM: dts: da850: Add the usb otg device node
From: Alexandre Bailon @ 2016-10-27 9:34 UTC (permalink / raw)
To: khilman, robh+dt, b-liu
Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel,
Alexandre Bailon
In-Reply-To: <1477560847-8929-1-git-send-email-abailon@baylibre.com>
This adds the device tree node for the usb otg
controller present in the da850 family of SoC's.
This also enables the otg usb controller for the lcdk board.
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
arch/arm/boot/dts/da850-lcdk.dts | 8 ++++++++
arch/arm/boot/dts/da850.dtsi | 15 +++++++++++++++
2 files changed, 23 insertions(+)
diff --git a/arch/arm/boot/dts/da850-lcdk.dts b/arch/arm/boot/dts/da850-lcdk.dts
index 7b8ab21..9f5040c 100644
--- a/arch/arm/boot/dts/da850-lcdk.dts
+++ b/arch/arm/boot/dts/da850-lcdk.dts
@@ -158,6 +158,14 @@
rx-num-evt = <32>;
};
+&usb_phy {
+ status = "okay";
+ };
+
+&usb0 {
+ status = "okay";
+};
+
&aemif {
pinctrl-names = "default";
pinctrl-0 = <&nand_pins>;
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..322a31a 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -372,6 +372,21 @@
>;
status = "disabled";
};
+ usb_phy: usb-phy {
+ compatible = "ti,da830-usb-phy";
+ #phy-cells = <1>;
+ status = "disabled";
+ };
+ usb0: usb@200000 {
+ compatible = "ti,da830-musb";
+ reg = <0x200000 0x10000>;
+ interrupts = <58>;
+ interrupt-names = "mc";
+ dr_mode = "otg";
+ phys = <&usb_phy 0>;
+ phy-names = "usb-phy";
+ status = "disabled";
+ };
gpio: gpio@226000 {
compatible = "ti,dm6441-gpio";
gpio-controller;
--
2.7.3
^ permalink raw reply related
* [PATCH v3 3/4] usb: musb: da8xx: Add DT support for the DA8xx driver
From: Alexandre Bailon @ 2016-10-27 9:34 UTC (permalink / raw)
To: khilman, robh+dt, b-liu
Cc: devicetree, linux-usb, linux-kernel, Petr Kulhavy,
Alexandre Bailon, linux-arm-kernel
In-Reply-To: <1477560847-8929-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
This adds DT support for TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver
Signed-off-by: Petr Kulhavy <petr@barix.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
Tested-by: David Lechner <david@lechnology.com>
---
drivers/usb/musb/da8xx.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 210b7e4..51ae3b6 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -6,6 +6,9 @@
* Based on the DaVinci "glue layer" code.
* Copyright (C) 2005-2006 by Texas Instruments
*
+ * DT support
+ * Copyright (c) 2016 Petr Kulhavy <petr@barix.com>
+ *
* This file is part of the Inventra Controller Driver for Linux.
*
* The Inventra Controller Driver for Linux is free software; you
@@ -433,6 +436,21 @@ static int da8xx_musb_exit(struct musb *musb)
return 0;
}
+static inline u8 get_vbus_power(struct device *dev)
+{
+ struct regulator *vbus_supply;
+ int current_uA;
+
+ vbus_supply = regulator_get_optional(dev, "vbus");
+ if (IS_ERR(vbus_supply))
+ return 255;
+ current_uA = regulator_get_current_limit(vbus_supply);
+ regulator_put(vbus_supply);
+ if (current_uA <= 0 || current_uA > 510000)
+ return 255;
+ return current_uA / 1000 / 2;
+}
+
static const struct musb_platform_ops da8xx_ops = {
.quirks = MUSB_DMA_CPPI | MUSB_INDEXED_EP,
.init = da8xx_musb_init,
@@ -458,6 +476,12 @@ static const struct platform_device_info da8xx_dev_info = {
.dma_mask = DMA_BIT_MASK(32),
};
+static const struct musb_hdrc_config da8xx_config = {
+ .ram_bits = 10,
+ .num_eps = 5,
+ .multipoint = 1,
+};
+
static int da8xx_probe(struct platform_device *pdev)
{
struct resource musb_resources[2];
@@ -465,6 +489,7 @@ static int da8xx_probe(struct platform_device *pdev)
struct da8xx_glue *glue;
struct platform_device_info pinfo;
struct clk *clk;
+ struct device_node *np = pdev->dev.of_node;
int ret;
glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
@@ -486,6 +511,16 @@ static int da8xx_probe(struct platform_device *pdev)
glue->dev = &pdev->dev;
glue->clk = clk;
+ if (IS_ENABLED(CONFIG_OF) && np) {
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return -ENOMEM;
+
+ pdata->config = &da8xx_config;
+ pdata->mode = musb_get_mode(&pdev->dev);
+ pdata->power = get_vbus_power(&pdev->dev);
+ }
+
pdata->platform_ops = &da8xx_ops;
glue->usb_phy = usb_phy_generic_register();
@@ -536,11 +571,22 @@ static int da8xx_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_OF
+static const struct of_device_id da8xx_id_table[] = {
+ {
+ .compatible = "ti,da830-musb",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, da8xx_id_table);
+#endif
+
static struct platform_driver da8xx_driver = {
.probe = da8xx_probe,
.remove = da8xx_remove,
.driver = {
.name = "musb-da8xx",
+ .of_match_table = of_match_ptr(da8xx_id_table),
},
};
--
2.7.3
^ permalink raw reply related
* [PATCH v3 2/4] usb: musb: core: added helper function for parsing DT
From: Alexandre Bailon @ 2016-10-27 9:34 UTC (permalink / raw)
To: khilman, robh+dt, b-liu
Cc: devicetree, linux-usb, linux-kernel, Petr Kulhavy,
Alexandre Bailon, linux-arm-kernel
In-Reply-To: <1477560847-8929-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
This adds the function musb_get_mode() to get the DT property "dr_mode"
Signed-off-by: Petr Kulhavy <petr@barix.com>
Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
Tested-by: David Lechner <david@lechnology.com>
---
drivers/usb/musb/musb_core.c | 19 +++++++++++++++++++
drivers/usb/musb/musb_core.h | 5 +++++
2 files changed, 24 insertions(+)
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 27dadc0..bba07e7 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -100,6 +100,7 @@
#include <linux/io.h>
#include <linux/dma-mapping.h>
#include <linux/usb.h>
+#include <linux/usb/of.h>
#include "musb_core.h"
#include "musb_trace.h"
@@ -130,6 +131,24 @@ static inline struct musb *dev_to_musb(struct device *dev)
return dev_get_drvdata(dev);
}
+enum musb_mode musb_get_mode(struct device *dev)
+{
+ enum usb_dr_mode mode;
+
+ mode = usb_get_dr_mode(dev);
+ switch (mode) {
+ case USB_DR_MODE_HOST:
+ return MUSB_HOST;
+ case USB_DR_MODE_PERIPHERAL:
+ return MUSB_PERIPHERAL;
+ case USB_DR_MODE_OTG:
+ case USB_DR_MODE_UNKNOWN:
+ default:
+ return MUSB_OTG;
+ }
+}
+EXPORT_SYMBOL_GPL(musb_get_mode);
+
/*-------------------------------------------------------------------------*/
#ifndef CONFIG_BLACKFIN
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 2cb88a49..a406468 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -617,4 +617,9 @@ static inline void musb_platform_post_root_reset_end(struct musb *musb)
musb->ops->post_root_reset_end(musb);
}
+/* gets the "dr_mode" property from DT and converts it into musb_mode
+ * if the property is not found or not recognized returns MUSB_OTG
+ */
+extern enum musb_mode musb_get_mode(struct device *dev);
+
#endif /* __MUSB_CORE_H__ */
--
2.7.3
^ permalink raw reply related
* [PATCH v3 1/4] dt/bindings: Add binding for the DA8xx MUSB driver
From: Alexandre Bailon @ 2016-10-27 9:34 UTC (permalink / raw)
To: khilman, robh+dt, b-liu
Cc: devicetree, linux-usb, linux-kernel, Petr Kulhavy,
Alexandre Bailon, linux-arm-kernel
In-Reply-To: <1477560847-8929-1-git-send-email-abailon@baylibre.com>
From: Petr Kulhavy <petr@barix.com>
DT binding for the TI DA8xx/OMAP-L1x/AM17xx/AM18xx MUSB driver.
Signed-off-by: Petr Kulhavy <petr@barix.com>
Signed-off-by: Alexandre Bailon <abailon@baylibre.com>
---
.../devicetree/bindings/usb/da8xx-usb.txt | 43 ++++++++++++++++++++++
1 file changed, 43 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
diff --git a/Documentation/devicetree/bindings/usb/da8xx-usb.txt b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
new file mode 100644
index 0000000..ccb844a
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/da8xx-usb.txt
@@ -0,0 +1,43 @@
+TI DA8xx MUSB
+~~~~~~~~~~~~~
+For DA8xx/OMAP-L1x/AM17xx/AM18xx platforms.
+
+Required properties:
+~~~~~~~~~~~~~~~~~~~~
+ - compatible : Should be set to "ti,da830-musb".
+
+ - reg: Offset and length of the USB controller register set.
+
+ - interrupts: The USB interrupt number.
+
+ - interrupt-names: Should be set to "mc".
+
+ - dr_mode: The USB operation mode. Should be one of "host", "peripheral" or "otg".
+
+ - phys: Phandle for the PHY device
+
+ - phy-names: Should be "usb-phy"
+
+Optional properties:
+~~~~~~~~~~~~~~~~~~~~
+ - vbus-supply: Phandle to a regulator providing the USB bus power.
+
+Example:
+ usb_phy: usb-phy {
+ compatible = "ti,da830-usb-phy";
+ #phy-cells = <0>;
+ status = "okay";
+ };
+ usb0: usb@200000 {
+ compatible = "ti,da830-musb";
+ reg = <0x00200000 0x10000>;
+ interrupts = <58>;
+ interrupt-names = "mc";
+
+ dr_mode = "host";
+ vbus-supply = <&usb_vbus>;
+ phys = <&usb_phy 0>;
+ phy-names = "usb-phy";
+
+ status = "okay";
+ };
--
2.7.3
^ permalink raw reply related
* [PATCH v3 0/4] Add DT support for DA8xx
From: Alexandre Bailon @ 2016-10-27 9:34 UTC (permalink / raw)
To: khilman, robh+dt, b-liu
Cc: devicetree, linux-usb, linux-kernel, linux-arm-kernel,
Alexandre Bailon
Changes in v2:
* Remove unrelated changes in patch 3
* Rename the device node in patch 4
Changes in v3:
* Fix few mistakes in DT binding sample
* Only build the device table if DT is enabled
Alexandre Bailon (1):
ARM: dts: da850: Add the usb otg device node
Petr Kulhavy (3):
dt/bindings: Add binding for the DA8xx MUSB driver
usb: musb: core: added helper function for parsing DT
usb: musb: da8xx: Add DT support for the DA8xx driver
.../devicetree/bindings/usb/da8xx-usb.txt | 43 ++++++++++++++++++++
arch/arm/boot/dts/da850-lcdk.dts | 8 ++++
arch/arm/boot/dts/da850.dtsi | 15 +++++++
drivers/usb/musb/da8xx.c | 46 ++++++++++++++++++++++
drivers/usb/musb/musb_core.c | 19 +++++++++
drivers/usb/musb/musb_core.h | 5 +++
6 files changed, 136 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/da8xx-usb.txt
--
2.7.3
^ permalink raw reply
* Re: [PATCH v2 2/9] dt-bindings: interrupt-controller: add DT binding for meson GPIO interrupt controller
From: Mark Rutland @ 2016-10-27 9:32 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree, Jason Cooper, Marc Zyngier, Kevin Hilman,
Linus Walleij, Will Deacon, linux-kernel, Russell King,
linux-gpio, Catalin Marinas, Carlo Caione, linux-amlogic,
Thomas Gleixner, linux-arm-kernel, Jerome Brunet
In-Reply-To: <20161026214234.4q6wmecehqh6q32o@rob-hp-laptop>
On Wed, Oct 26, 2016 at 04:42:35PM -0500, Rob Herring wrote:
> On Wed, Oct 19, 2016 at 05:21:13PM +0200, Jerome Brunet wrote:
> >
> > This commit adds the device tree bindings description for Amlogic's GPIO
> > interrupt controller available on the meson8, meson8b and gxbb SoC families
> >
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > ---
> > Rob, I did not include the Ack you gave for the RFC as bindings have slightly
> > changed. Only the interrupt property has be removed following a discussion I
> > had with Marc
>
> As Mark R already said, you should keep the interrupts property.
To be clear, the interrupt routing should be described *somehow*, though
I don't think the generic interrupts property is correct, as this is an
interrupt *router*, i.e. this device doesn't own the interrupt, it just
joins two parts of the line together (and so flags are meaningless).
Thanks,
Mark.
^ permalink raw reply
* Re: [PATCH V4 2/3] ARM64 LPC: Add missing range exception for special ISA
From: zhichang.yuan @ 2016-10-27 9:30 UTC (permalink / raw)
To: Rob Herring, catalin.marinas, will.deacon, bhelgaas, mark.rutland
Cc: devicetree, lorenzo.pieralisi, minyard, arnd, linux-pci,
gabriele.paoloni, john.garry, liviu.dudau, linuxarm, xuwei5,
linux-kernel, linux-serial, benh, zourongrong, kantyzc,
zhichang.yuan02, linux-arm-kernel
In-Reply-To: <20161026222542.g4etrqesqb7febid@rob-hp-laptop>
Hi, Rob,
Thanks for your comments!
Please check the response blow.
BTW, Are there any comments from other maintainers/hackers??
Thanks in advance!
On 2016/10/27 6:25, Rob Herring wrote:
> On Thu, Oct 20, 2016 at 05:15:39PM +0800, zhichang.yuan wrote:
>> Currently if the range property is not specified of_translate_one
>> returns an error. There are some special devices that work on a
>> range of I/O ports where it's is not correct to specify a range
>> property as the cpu addresses are used by special accessors.
>> Here we add a new exception in of_translate_one to return
>> the cpu address if the range property is not there. The exception
>> checks if the parent bus is ISA and if the special accessors are
>> defined.
>>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>> arch/arm64/include/asm/io.h | 7 +++++++
>> arch/arm64/kernel/extio.c | 24 +++++++++++++++++++++++
>> drivers/of/address.c | 47 +++++++++++++++++++++++++++++++++++++++++++--
>> drivers/pci/pci.c | 6 +++---
>> include/linux/of_address.h | 17 ++++++++++++++++
>> 5 files changed, 96 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 136735d..e480199 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -175,6 +175,13 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>> #define outsl outsl
>>
>> DECLARE_EXTIO(l, u32)
>> +
>> +
>> +#define indirect_io_ison indirect_io_ison
>> +extern int indirect_io_ison(void);
>> +
>> +#define chk_indirect_range chk_indirect_range
>> +extern int chk_indirect_range(u64 taddr);
>> #endif
>>
>>
>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>> index 80cafd5..55df8dc 100644
>> --- a/arch/arm64/kernel/extio.c
>> +++ b/arch/arm64/kernel/extio.c
>> @@ -19,6 +19,30 @@
>>
>> struct extio_ops *arm64_extio_ops;
>>
>> +/**
>> + * indirect_io_ison - check whether indirectIO can work well. This function only call
>> + * before the target I/O address was obtained.
>> + *
>> + * Returns 1 when indirectIO can work.
>> + */
>> +int indirect_io_ison()
>> +{
>> + return arm64_extio_ops ? 1 : 0;
>> +}
>> +
>> +/**
>> + * check_indirect_io - check whether the input taddr is for indirectIO.
>> + * @taddr: the io address to be checked.
>> + *
>> + * Returns 1 when taddr is in the range; otherwise return 0.
>> + */
>> +int chk_indirect_range(u64 taddr)
>> +{
>> + if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)
>> + return 0;
>> +
>> + return 1;
>> +}
>>
>> BUILD_EXTIO(b, u8)
>>
>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>> index 02b2903..0bee822 100644
>> --- a/drivers/of/address.c
>> +++ b/drivers/of/address.c
>> @@ -479,6 +479,39 @@ static int of_empty_ranges_quirk(struct device_node *np)
>> return false;
>> }
>>
>> +
>> +/*
>> + * Check whether the current device being translating use indirectIO.
>> + *
>> + * return 1 if the check is past, or 0 represents fail checking.
>> + */
>> +static int of_isa_indirect_io(struct device_node *parent,
>
> Make the return bool.
ok. will update it.
>
>> + struct of_bus *bus, __be32 *addr,
>> + int na, u64 *presult)
>> +{
>> + unsigned int flags;
>> + unsigned int rlen;
>> +
>> + /* whether support indirectIO */
>> + if (!indirect_io_ison())
>
> indirect_io_is_enabled() would be a bit more readable.
Ok.
>
>> + return 0;
>> +
>> + if (!of_bus_isa_match(parent))
>> + return 0;
>> +
>> + flags = bus->get_flags(addr);
>> + if (!(flags & IORESOURCE_IO))
>> + return 0;
>> +
>> + /* there is ranges property, apply the normal translation directly. */
>> + if (of_get_property(parent, "ranges", &rlen))
>> + return 0;
>> +
>> + *presult = of_read_number(addr + 1, na - 1);
>> +
>> + return chk_indirect_range(*presult);
>> +}
>> +
>> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>> struct of_bus *pbus, __be32 *addr,
>> int na, int ns, int pna, const char *rprop)
>> @@ -532,7 +565,7 @@ static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>> }
>> memcpy(addr, ranges + na, 4 * pna);
>>
>> - finish:
>> +finish:
>
> This hunk is unrelated. Drop it.
Yes. Will drop this change.
>
>> of_dump_addr("parent translation for:", addr, pna);
>> pr_debug("with offset: %llx\n", (unsigned long long)offset);
>>
>> @@ -595,6 +628,15 @@ static u64 __of_translate_address(struct device_node *dev,
>> result = of_read_number(addr, na);
>> break;
>> }
>> + /*
>> + * For indirectIO device which has no ranges property, get
>> + * the address from reg directly.
>> + */
>> + if (of_isa_indirect_io(dev, bus, addr, na, &result)) {
>> + pr_info("isa indirectIO matched(%s)..addr = 0x%llx\n",
>> + of_node_full_name(dev), result);
>
> This should be debugging.
ok. will replace with pr_debug.
thanks!
Zhichang
>
>> + break;
>> + }
>>
>> /* Get new parent bus and counts */
>> pbus = of_match_bus(parent);
>> @@ -688,8 +730,9 @@ static int __of_address_to_resource(struct device_node *dev,
>> if (taddr == OF_BAD_ADDR)
>> return -EINVAL;
>> memset(r, 0, sizeof(struct resource));
>> - if (flags & IORESOURCE_IO) {
>> + if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>> unsigned long port;
>> +
>> port = pci_address_to_pio(taddr);
>> if (port == (unsigned long)-1)
>> return -EINVAL;
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index ba34907..1a08511 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>>
>> #ifdef PCI_IOBASE
>> struct io_range *range;
>> - resource_size_t allocated_size = 0;
>> + resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>
>> /* check if the range hasn't been previously recorded */
>> spin_lock(&io_range_lock);
>> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
>>
>> #ifdef PCI_IOBASE
>> struct io_range *range;
>> - resource_size_t allocated_size = 0;
>> + resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>
>> if (pio > IO_SPACE_LIMIT)
>> return address;
>> @@ -3335,7 +3335,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>> {
>> #ifdef PCI_IOBASE
>> struct io_range *res;
>> - resource_size_t offset = 0;
>> + resource_size_t offset = PCIBIOS_MIN_IO;
>> unsigned long addr = -1;
>>
>> spin_lock(&io_range_lock);
>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>> index 3786473..0ba7e21 100644
>> --- a/include/linux/of_address.h
>> +++ b/include/linux/of_address.h
>> @@ -24,6 +24,23 @@ struct of_pci_range {
>> #define for_each_of_pci_range(parser, range) \
>> for (; of_pci_range_parser_one(parser, range);)
>>
>> +
>> +#ifndef indirect_io_ison
>> +#define indirect_io_ison indirect_io_ison
>> +static inline int indirect_io_ison(void)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> +#ifndef chk_indirect_range
>> +#define chk_indirect_range chk_indirect_range
>> +static inline int chk_indirect_range(u64 taddr)
>> +{
>> + return 0;
>> +}
>> +#endif
>> +
>> /* Translate a DMA address from device space to CPU space */
>> extern u64 of_translate_dma_address(struct device_node *dev,
>> const __be32 *in_addr);
>> --
>> 1.9.1
>>
>
> .
>
^ permalink raw reply
* Re: [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1
From: Matthias Brugger @ 2016-10-27 9:19 UTC (permalink / raw)
To: Antoine Tenart, maxime.ripard, pantelis.antoniou, mark.rutland,
sboyd
Cc: thomas.petazzoni, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20161026145756.21689-6-antoine.tenart@free-electrons.com>
On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
Please provide a commit message.
Thanks,
Matthias
^ permalink raw reply
* Re: [RFC PATCH 5/5] of: overlay-mgr: add a detector for headers stored on a ds2431 eeprom over w1
From: Matthias Brugger @ 2016-10-27 9:18 UTC (permalink / raw)
To: Antoine Tenart, maxime.ripard, pantelis.antoniou, mark.rutland,
sboyd
Cc: thomas.petazzoni, devicetree, linux-kernel, linux-arm-kernel
In-Reply-To: <20161026145756.21689-6-antoine.tenart@free-electrons.com>
On 10/26/2016 04:57 PM, Antoine Tenart wrote:
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
Please provide a commit message.
> drivers/of/overlay-manager/Kconfig | 10 ++++++++++
> drivers/w1/slaves/w1_ds2431.c | 39 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 49 insertions(+)
>
> diff --git a/drivers/of/overlay-manager/Kconfig b/drivers/of/overlay-manager/Kconfig
> index 1a36613c0c53..ad0a5b8e9e5e 100644
> --- a/drivers/of/overlay-manager/Kconfig
> +++ b/drivers/of/overlay-manager/Kconfig
> @@ -16,4 +16,14 @@ config OF_OVERLAY_MGR_FORMAT_CHIP
>
> endmenu
>
> +menu "Overlay Manager detectors"
> +
> +config OF_OVERLAY_MGR_DETECTOR_DS2431
> + bool "Dip header on a DS2431 EEPROM"
> + depends on W1_SLAVE_DS2431
> + help
> + Enable dip header DS2431 EEPROM support.
> +
> +endmenu
> +
> endif
> diff --git a/drivers/w1/slaves/w1_ds2431.c b/drivers/w1/slaves/w1_ds2431.c
> index 80572cb63ba8..760325f9a2bd 100644
> --- a/drivers/w1/slaves/w1_ds2431.c
> +++ b/drivers/w1/slaves/w1_ds2431.c
> @@ -15,6 +15,9 @@
> #include <linux/device.h>
> #include <linux/types.h>
> #include <linux/delay.h>
> +#include <linux/slab.h>
> +
> +#include <linux/overlay-manager.h>
>
> #include "../w1.h"
> #include "../w1_int.h"
> @@ -280,7 +283,43 @@ static const struct attribute_group *w1_f2d_groups[] = {
> NULL,
> };
>
> +#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
> +static int chip_dip_callback(struct w1_slave *sl)
> +{
> + char **candidates = NULL;
> + int i, n, err = 0;
> + u8 *data;
> +
> + data = kzalloc(OVERLAY_MGR_DIP_MAX_SZ, GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + /* sizeof(struct chip_header) is a mulitple of 8 */
> + for (i = 0; i < OVERLAY_MGR_DIP_MAX_SZ; i += 8) {
> + if (w1_f2d_readblock(sl, i, 8, &data[i])) {
> + err = -EIO;
> + goto end;
> + }
> + }
> +
> + overlay_mgr_parse(&sl->dev, data, &candidates, &n);
> + if (!n) {
> + err = -EINVAL;
> + goto end;
> + }
> +
> + err = overlay_mgr_apply(&sl->dev, candidates, n);
> +
> +end:
> + kfree(data);
> + return err;
> +}
> +#endif
> +
> static struct w1_family_ops w1_f2d_fops = {
> +#if IS_ENABLED(CONFIG_OF_OVERLAY_MGR_DETECTOR_DS2431)
> + .callback = chip_dip_callback,
> +#endif
> .groups = w1_f2d_groups,
> };
>
>
^ permalink raw reply
* Re: [PATCH 2/5] Documentation: devicetree: net: add NS2 bindings to amac
From: Andrew Lunn @ 2016-10-27 9:17 UTC (permalink / raw)
To: Jon Mason
Cc: Mark Rutland, devicetree, Florian Fainelli, netdev, linux-kernel,
Rob Herring, bcm-kernel-feedback-list, rafal, David Miller,
linux-arm-kernel
In-Reply-To: <1477510561-17035-3-git-send-email-jon.mason@broadcom.com>
On Wed, Oct 26, 2016 at 03:35:58PM -0400, Jon Mason wrote:
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
> Documentation/devicetree/bindings/net/brcm,amac.txt | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/net/brcm,amac.txt b/Documentation/devicetree/bindings/net/brcm,amac.txt
> index ba5ecc1..f92caee 100644
> --- a/Documentation/devicetree/bindings/net/brcm,amac.txt
> +++ b/Documentation/devicetree/bindings/net/brcm,amac.txt
> @@ -2,15 +2,18 @@ Broadcom AMAC Ethernet Controller Device Tree Bindings
> -------------------------------------------------------------
>
> Required properties:
> - - compatible: "brcm,amac" or "brcm,nsp-amac"
> + - compatible: "brcm,amac", "brcm,nsp-amac", or "brcm,ns2-amac"
> - reg: Address and length of the GMAC registers,
> Address and length of the GMAC IDM registers
> + Address and length of the NIC Port Manager registers (optional)
> - reg-names: Names of the registers. Must have both "amac_base" and
> - "idm_base"
> + "idm_base". "nicpm_base" is optional (required for NS2)
> - interrupts: Interrupt number
>
> Optional properties:
> - mac-address: See ethernet.txt file in the same directory
> +- brcm,enet-phy-lane-swap:
> + boolean; Swap the PHY lanes (needed on some SKUs of NS2)
Maybe i'm missing something here, but the patch to the PHY swapped the
lanes. This seems to be a PHY property, not a MAC property. And it
swapped them unconditionally....
Andrew
^ permalink raw reply
* Re: [PATCH 1/5] net: phy: broadcom: Add BCM54810 phy entry
From: Andrew Lunn @ 2016-10-27 9:15 UTC (permalink / raw)
To: Jon Mason
Cc: Mark Rutland, devicetree, Florian Fainelli, netdev, linux-kernel,
Vikas Soni, Rob Herring, bcm-kernel-feedback-list, rafal,
David Miller, linux-arm-kernel
In-Reply-To: <1477510561-17035-2-git-send-email-jon.mason@broadcom.com>
On Wed, Oct 26, 2016 at 03:35:57PM -0400, Jon Mason wrote:
> From: Vikas Soni <vsoni@broadcom.com>
>
> Add BCM54810 phy entry
Hi Jon, Vikis
The subject line is a bit misleading. It does more than add a PHY ID
entry.
> Signed-off-by: Vikas Soni <vsoni@broadcom.com>
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
> ---
> drivers/net/phy/Kconfig | 2 +-
> drivers/net/phy/broadcom.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/brcmphy.h | 7 +++++
> 3 files changed, 73 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 45f68ea..31967ca 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -217,7 +217,7 @@ config BROADCOM_PHY
> select BCM_NET_PHYLIB
> ---help---
> Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464,
> - BCM5481 and BCM5482 PHYs.
> + BCM5481, BCM54810 and BCM5482 PHYs.
>
> config CICADA_PHY
> tristate "Cicada PHYs"
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 870327e..cdce761 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -35,6 +35,35 @@ static int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val)
> return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val);
> }
>
> +static int bcm54810_config(struct phy_device *phydev)
> +{
> + int rc;
> +
> + /* Disable BroadR-Reach */
> + rc = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, 0);
> + if (rc < 0)
> + return rc;
> +
> + /* SKEW DISABLE */
> + rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
> + 0xF0E0);
> + if (rc < 0)
> + return rc;
> +
> + /* DELAY DISABLE */
> + rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
> + 0x7000);
This driver mostly uses symbolic names, not #defines. Please can you
use #defines here and else were in this patch.
> + if (rc < 0)
> + return rc;
> +
> + /* DELAY DISABLE */
> + rc = bcm_phy_write_shadow(phydev, BCM54810_SHD_CLK_CTL, 0);
> + if (rc < 0)
> + return rc;
Twice the same comment?
> +
> + return 0;
> +}
> +
> /* Needs SMDSP clock enabled via bcm54xx_phydsp_config() */
> static int bcm50610_a0_workaround(struct phy_device *phydev)
> {
> @@ -207,6 +236,20 @@ static int bcm54xx_config_init(struct phy_device *phydev)
> (phydev->dev_flags & PHY_BRCM_AUTO_PWRDWN_ENABLE))
> bcm54xx_adjust_rxrefclk(phydev);
>
> + if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810) {
> + err = bcm54810_config(phydev);
> + if (err)
> + return err;
> +
> + reg = phy_read(phydev, MII_BMCR);
> + if (reg < 0)
> + return reg;
> +
> + err = phy_write(phydev, MII_BMCR, reg & ~BMCR_PDOWN);
> + if (err)
> + return err;
This seems a bit odd. I would expect the PHY core correctly handles
the PHY being powered down. Can you explain this a bit more, why it is
needed.
Thanks
Andrew
^ permalink raw reply
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