* [PATCH v4] pcie: Add Xilinx PCIe Host Bridge IP driver
From: Michal Simek @ 2014-07-22 17:02 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAErSpo41HwMferWm8q69Sz0t-Cs1WHgwwVp1VKsoxxyvrgQhfQ@mail.gmail.com>
On 07/22/2014 06:08 PM, Bjorn Helgaas wrote:
> On Mon, Jul 21, 2014 at 11:10 PM, Srikanth Thokala <sthokal@xilinx.com> wrote:
>> On Wed, Jul 16, 2014 at 11:08 PM, Bjorn Helgaas <bhelgaas@google.com> wrote:
>>> ...
>>> I see I forgot to ask for a MAINTAINERS entry for this driver. Can
>>> you add one?
>>
>> There was a discussion on this earlier and Michal mentioned it is not
>> required as it is
>> handled by our Xilinx record.
>>
>> Here is the reply from Michal to the MAINTAINERS update comment,
>>
>> < Reply from Michal >
>>
>>> Please also include a MAINTAINERS update for drivers/pci/host/pci-xilinx.c.
>>
>> This should be handle by our record that's why MAINTAINERS update is
>> not necessary.
>> (N: xilinx below)
>
> That's technically true in the sense that get_maintainer.pl will do
> the right thing, but I often review patches in email (without
> extracting them), so it's more convenient to just look in MAINTAINERS
> to see if they have the right acks. But I guess I can deal with it
> either way.
There is also another reason for using just this fragment.
Because developers and their responsibilities are changing so often
and I just know who is responsible for it at that time.
Thanks,
Michal
^ permalink raw reply
* [PATCH 8/8] clk: tegra: Add EMC clock driver
From: Stephen Warren @ 2014-07-22 16:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405088313-20048-9-git-send-email-mperttunen@nvidia.com>
On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> The driver is currently only tested on Tegra124 Jetson TK1, but should
> work with other Tegra124 boards, provided that correct EMC tables are
> provided through the device tree. Older chip models have differing
> timing change sequences, so they are not currently supported.
> diff --git a/drivers/clk/tegra/clk-emc.c b/drivers/clk/tegra/clk-emc.c
> +struct emc_timing {
> + unsigned long rate, parent_rate;
> + u8 parent_index;
> + struct clk *parent;
> +
> + /* Store EMC burst data in a union to minimize mistakes. This allows
> + * us to use the same burst data lists as used by the downstream and
> + * ChromeOS kernels. */
Nit: */ should be on its own line. This applies to many comments in the
file.
> +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Timing change sequence functions *
> + * * * * * * * * * * * * * * * * * * * * * * * * * */
Nit: This kind of banner comment is unusual, but I guess it's fine.
> +static void emc_seq_update_timing(struct tegra_emc *tegra)
...
> + dev_err(&tegra->pdev->dev, "timing update failed\n");
> + BUG();
> +}
Is there any way to avoid all these BUGs? Can we just continue on and
retry the next time, or disallow any further clock rate changes or
something?
> +/* * * * * * * * * * * * * * * * * * * * * * * * * *
> + * Debugfs entry *
> + * * * * * * * * * * * * * * * * * * * * * * * * * */
> +
> +static int emc_debug_rate_get(void *data, u64 *rate)
> +{
> + struct tegra_emc *tegra = data;
> +
> + *rate = clk_get_rate(tegra->hw.clk);
> +
> + return 0;
> +}
> +
> +static int emc_debug_rate_set(void *data, u64 rate)
> +{
> + struct tegra_emc *tegra = data;
> +
> + return clk_set_rate(tegra->hw.clk, rate);
> +}
> +
> +DEFINE_SIMPLE_ATTRIBUTE(emc_debug_rate_fops, emc_debug_rate_get,
> + emc_debug_rate_set, "%lld\n");
I think the rate can already be obtained through
...debug/clock/clock_summary. I'm not sure about changing the rate, but
shouldn't that be a feature of the common clock core, not individual
drivers?
> +static int load_timings_from_dt(struct tegra_emc *tegra,
> + struct device_node *node)
> +{
...
> + for_each_child_of_node(node, child) {
...
> + if (timing->rate <= prev_rate) {
> + dev_err(&tegra->pdev->dev,
> + "timing %s: rate not increasing\n",
> + child->name);
I don't believe there's any guaranteed node enumeration order. If the
driver needs the child nodes sorted, it should sort them itself.
> +static const struct of_device_id tegra_car_of_match[] = {
> + { .compatible = "nvidia,tegra124-car" },
> + {}
> +};
> +
> +static const struct of_device_id tegra_mc_of_match[] = {
> + { .compatible = "nvidia,tegra124-mc" },
> + {}
> +};
It would be better if this driver explicitly called into the driver for
other modules, rather than directly touching their registers.
^ permalink raw reply
* [GIT PULL 2/3] ARM: tegra: move fuse code out of arch/arm
From: Catalin Marinas @ 2014-07-22 16:54 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CE90ED.8030900@wwwdotorg.org>
On Tue, Jul 22, 2014 at 05:27:25PM +0100, Stephen Warren wrote:
> On 07/22/2014 04:27 AM, Catalin Marinas wrote:
> ...
> > ...It's also aimed at
> > pushing back on hardware people who think they can mess up, for example,
> > a GIC implementation because the rest is just software and you can
> > always add #ifdefs.
>
> I'm very concerned about this statement.
>
> Yes, it would be nice if all HW was sanely designed, but it's not
> always. The time to push back on bad HW design is during IP licensing
> agreements or HW design reviews, not upstreaming.
I agree about the HW design reviews. But I suspect some software people
in such companies, concerned with upstreaming Linux support, are allowed
to give feedback. And the feedback could be something like (just an
example) "if we use a standard GIC and the architected timer, sane
(!PL310-like) system caches, we get the benefit of no additional
upstream work, better virtualisation support etc". I hope someone in the
HW team would take such feedback into account rather than assume that
software and upstreaming comes for free.
Of course, we've never rejected Linux upstreaming because of the
hardware design (well, as long as Linux could reliably run on it) and I
don't plan to do this in the future, but just show that there are clear
benefits in doing something in a more standardised way (e.g. SBSA for
servers).
Even though ARM licenses the architecture, there is no way to enforce a
HW design as part of the agreement (or risk not licensing the IP at
all). But there is effort to standardise things like SBSA, Trusted
Firmware.
> Any pushback during SW upstreaming simply serves to make it difficult
> for the SW people doing the upstreaming. It has zero impact on the
> current HW design (it's already shipped). It probably has zero impact on
> the next N revisions of the HW (they're already baked). It's possible it
> has zero-to-minimal impact on any future HW (end-user product
> requirements are what matter most).
I agree that pushing back during SW upstreaming is very late. But I hope
to raise the awareness earlier for ARMv8 hardware.
--
Catalin
^ permalink raw reply
* [PATCH 2/2] ARM: DRA7: hwmod: Add dra74x and dra72x specific ocp interface lists
From: Nishanth Menon @ 2014-07-22 16:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405499774-19770-3-git-send-email-lokeshvutla@ti.com>
On 07/16/2014 03:36 AM, Lokesh Vutla wrote:
> From: Rajendra Nayak <rnayak@ti.com>
>
> To deal with IPs which are specific to dra74x and dra72x, maintain seperate
> ocp interface lists, while keeping the common list for all common IPs.
>
> Move USB OTG SS4 to dra74x only list since its unavailable in
> dra72x and is giving an abort during boot. The dra72x only list
> is empty for now and a placeholder for future hwmod additions which
> are specific to dra72x.
>
> Fixes: d904b38 ARM: DRA7: hwmod: Add SYSCONFIG for usb_otg_ss
please use a format as following:
Fixes: d904b38df0db13 ("ARM: DRA7: hwmod: Add SYSCONFIG for usb_otg_ss")
> Reported-by: Keerthy <j-keerthy@ti.com>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> arch/arm/mach-omap2/omap_hwmod.c | 3 +++
> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 22 ++++++++++++++++++++--
> 2 files changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 6c074f3..14f8370 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -3345,6 +3345,9 @@ int __init omap_hwmod_register_links(struct omap_hwmod_ocp_if **ois)
> if (!ois)
> return 0;
>
> + if (ois[0] == NULL) /*empty list*/
/* Empty list */ ?
> + return 0;
> +
This change looks like a different patch?
> if (!linkspace) {
> if (_alloc_linkspace(ois)) {
> pr_err("omap_hwmod: could not allocate link space\n");
> diff --git a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> index 284324f..c95033c 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_7xx_data.c
> @@ -35,6 +35,7 @@
> #include "i2c.h"
> #include "mmc.h"
> #include "wd_timer.h"
> +#include "soc.h"
>
> /* Base offset for all DRA7XX interrupts external to MPUSS */
> #define DRA7XX_IRQ_GIC_START 32
> @@ -2705,7 +2706,6 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = {
> &dra7xx_l4_per3__usb_otg_ss1,
> &dra7xx_l4_per3__usb_otg_ss2,
> &dra7xx_l4_per3__usb_otg_ss3,
> - &dra7xx_l4_per3__usb_otg_ss4,
> &dra7xx_l3_main_1__vcp1,
> &dra7xx_l4_per2__vcp1,
> &dra7xx_l3_main_1__vcp2,
> @@ -2714,8 +2714,26 @@ static struct omap_hwmod_ocp_if *dra7xx_hwmod_ocp_ifs[] __initdata = {
> NULL,
> };
>
> +static struct omap_hwmod_ocp_if *dra74x_hwmod_ocp_ifs[] __initdata = {
> + &dra7xx_l4_per3__usb_otg_ss4,
> + NULL,
> +};
> +
> +static struct omap_hwmod_ocp_if *dra72x_hwmod_ocp_ifs[] __initdata = {
> + NULL,
> +};
> +
> int __init dra7xx_hwmod_init(void)
> {
> + int ret;
> +
> omap_hwmod_init();
> - return omap_hwmod_register_links(dra7xx_hwmod_ocp_ifs);
> + ret = omap_hwmod_register_links(dra7xx_hwmod_ocp_ifs);
if (ret)
goto out;
> +
> + if (!ret && soc_is_dra74x())
no need of !ret
> + return omap_hwmod_register_links(dra74x_hwmod_ocp_ifs);
ret = omap_hwmod_register_links(dra74x_hwmod_ocp_ifs);
> + else if (!ret && soc_is_dra72x())
no need of else and !ret
> + return omap_hwmod_register_links(dra72x_hwmod_ocp_ifs);
ret = omap_hwmod_register_links(dra72x_hwmod_ocp_ifs);
> +
out:
> + return ret;
> }
>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 2/2] arm: add early_ioremap support
From: Tomasz Figa @ 2014-07-22 16:48 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1404898792-4201-3-git-send-email-leif.lindholm@linaro.org>
Hi Leif,
On 09.07.2014 11:39, Leif Lindholm wrote:
> From: Mark Salter <msalter@redhat.com>
>
> This patch uses the generic early_ioremap code to implement
> early_ioremap for ARM. The ARM-specific bits come mostly from
> an earlier patch from Leif Lindholm <leif.lindholm@linaro.org>
> here:
>
> https://lkml.org/lkml/2013/10/3/279
[snip]
> diff --git a/arch/arm/mm/early_ioremap.c b/arch/arm/mm/early_ioremap.c
> new file mode 100644
> index 0000000..1013109
> --- /dev/null
> +++ b/arch/arm/mm/early_ioremap.c
> @@ -0,0 +1,86 @@
> +/*
> + * early_ioremap() support for ARM
> + *
> + * Based on existing support in arch/x86/mm/ioremap.c
> + *
> + * Restrictions: currently only functional before paging_init()
Uhm, that's bad... This would explain why my earlycon code generates a
fault as soon as something prints after paging_init(). I'd say this
feature would be much more useful if mappings were carried over
paging_init(), so that mapped devices are available later as well.
I'll see if I can code this on top of your patch, but unfortunately it
might end up with -ENOTIME.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH] pinctrl: dra: dt-bindings: Fix pull enable/disable
From: Felipe Balbi @ 2014-07-22 16:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406043594-14181-1-git-send-email-nm@ti.com>
On Tue, Jul 22, 2014 at 10:39:54AM -0500, Nishanth Menon wrote:
> The DRA74/72 control module pins have a weak pull up and pull down.
> This is configured by bit offset 17. if BIT(17) is 1, a pull up is
> selected, else a pull down is selected.
>
> However, this pull resisstor is applied based on BIT(16) -
> PULLUDENABLE - if BIT(18) is *0*, then pull as defined in BIT(17) is
> applied, else no weak pulls are applied. We defined this in reverse.
>
> Reference: Table 18-5 (Description of the pad configuration register
> bits) in Technical Reference Manual Revision (DRA74x revision Q:
> SPRUHI2Q Revised June 2014 and DRA72x revision F: SPRUHP2F - Revised
> June 2014)
>
> Fixes: 6e58b8f1daaf1a ("ARM: dts: DRA7: Add the dts files for dra7 SoC and dra7-evm board")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
Tested on an upcoming board.
Tested-by: Felipe Balbi <balbi@ti.com>
Acked-by: Felipe Balbi <balbi@ti.com>
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140722/fe4fb421/attachment.sig>
^ permalink raw reply
* [PATCH 1/2] ARM: DRA7: Add support for soc_is_dra74x() and soc_is_dra72x() varients
From: Nishanth Menon @ 2014-07-22 16:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405499774-19770-2-git-send-email-lokeshvutla@ti.com>
On 07/16/2014 03:36 AM, Lokesh Vutla wrote:
> From: Rajendra Nayak <rnayak@ti.com>
>
> Use the corresponding compatibles to identify the devices.
>
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> Signed-off-by: Lokesh Vutla <lokeshvutla@ti.com>
> ---
> arch/arm/mach-omap2/soc.h | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/soc.h b/arch/arm/mach-omap2/soc.h
> index 01ca808..5e1be94 100644
> --- a/arch/arm/mach-omap2/soc.h
> +++ b/arch/arm/mach-omap2/soc.h
> @@ -245,6 +245,8 @@ IS_AM_SUBCLASS(437x, 0x437)
> #define soc_is_omap54xx() 0
> #define soc_is_omap543x() 0
> #define soc_is_dra7xx() 0
> +#define soc_is_dra74x() 0
> +#define soc_is_dra72x() 0
>
> #if defined(MULTI_OMAP2)
> # if defined(CONFIG_ARCH_OMAP2)
> @@ -393,7 +395,12 @@ IS_OMAP_TYPE(3430, 0x3430)
>
> #if defined(CONFIG_SOC_DRA7XX)
> #undef soc_is_dra7xx
> +#undef soc_is_dra74x
> +#undef soc_is_dra72x
> #define soc_is_dra7xx() (of_machine_is_compatible("ti,dra7"))
> +#define soc_is_dra74x() (of_machine_is_compatible("ti,dra74"))
> +#define soc_is_dra72x() (of_machine_is_compatible("ti,dra72"))
> +
> #endif
>
> /* Various silicon revisions for omap2 */
>
Acked-by: Nishanth Menon <nm@ti.com>
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 0/2] ARM: DRA7: hwmod: Add dra74x and dra72x specific ocp interface lists
From: Nishanth Menon @ 2014-07-22 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405499774-19770-1-git-send-email-lokeshvutla@ti.com>
On 07/16/2014 03:36 AM, Lokesh Vutla wrote:
> This series add seperate ocp interface lists that are specific to dra74x
> and dra72x, and moving USB OTG SS4 to dra74x only since its not present
> in dra72x. Without this USB OTG SS4 hwmod gives an abort on dra72x.
>
> Adding support for soc_is_dra74x() and soc_is_dra72x() in order to differentiate
> between dra74x and dra72x and pass the respective ocp interface lists.
>
> Verified on dra74x evm and dra72x evm using 3.16-rc5 based mainline kernel.
>
> Before:
> dra74x : http://paste.ubuntu.com/7802364/
> dra72x : http://paste.ubuntu.com/7802334/ (Kernel panic)
>
> After-
> dra74x : http://paste.ubuntu.com/7802340/
> dra72x : http://paste.ubuntu.com/7802338/ (booted)
>
> Rajendra Nayak (2):
> ARM: DRA7: Add support for soc_is_dra74x() and soc_is_dra72x()
> varients
> ARM: DRA7: hwmod: Add dra74x and dra72x specific ocp interface lists
>
> arch/arm/mach-omap2/omap_hwmod.c | 3 +++
> arch/arm/mach-omap2/omap_hwmod_7xx_data.c | 22 ++++++++++++++++++++--
> arch/arm/mach-omap2/soc.h | 7 +++++++
> 3 files changed, 30 insertions(+), 2 deletions(-)
>
Tested-by: Nishanth Menon <nm@ti.com>
BUT, I suggest a follow up series to do exactly the same (moving stuff
that are not common from dra7.dtsi to dra72x.dtsi and 74x.dtsi) as
well to ensure that dts indicates exactly the same information (only
the applicable IPs are present in dts).
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH 5/8] of: Add Tegra124 EMC bindings
From: Stephen Warren @ 2014-07-22 16:45 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAL1qeaHtGQxCO3cGdeCRUYuk6mxei6z1B63-iZdBECEbFqGhHw@mail.gmail.com>
On 07/21/2014 04:52 PM, Andrew Bresticker wrote:
> On Mon, Jul 21, 2014 at 2:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 07/11/2014 10:43 AM, Andrew Bresticker wrote:
>>> On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote:
>>>> Add binding documentation for the nvidia,tegra124-emc device tree
>>>> node.
>>>
>>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>>>
>>>> +Required properties :
>>>> +- compatible : "nvidia,tegra124-emc".
>>>> +- reg : Should contain 1 or 2 entries:
>>>> + - EMC register set
>>>> + - MC register set : Required only if no node with
>>>> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set
>>>> + is first read from the MC node. If it doesn't exist, it is read
>>>> + from this property.
>>>> +- timings : Should contain 1 entry for each supported clock rate.
>>>> + Entries should be named "timing at n" where n is a 0-based increasing
>>>> + number. The timings must be listed in rate-ascending order.
>>>
>>> There are upcoming boards which support multiple DRAM configurations
>>> and require a separate set of timings for each configuration. Could
>>> we instead have multiple sets of timings with the proper one selected
>>> at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0?
>>> Something like:
>>>
>>> emc {
>>> emc-table at 0 {
>>> nvidia,ram-code = <0>;
>>> timing at 0 {
>>> ...
>>> };
>>> ...
>>> };
>>
>> Until recently, there was a binding for Tegra20 EMC in mainline. We
>> should make sure the Tegra124 driver (or rather binding...) is at least
>> as feature-ful as that was.
>>
>> Furthermore, I thought that with some boards, there were more RAM
>> options that there were available RAM code strap bits. I assume that in
>> mainline, we'll simply have different base DT files for the different
>> sets of configurations? Or, will we want to add another level to the DT
>> to represent different sets of RAM code values? I'm not sure what data
>> source the bootloader uses to determine which set of RAM configuration
>> to use when there aren't enough entries in the BCT for the boot ROM to
>> do this automatically, and whether we have any way to get that value
>> into the kernel, so it could use it for the extra DT level lookup?
>
> For the ChromeOS boards at least we are neither limited by the number
> of strapping bits (4) nor the number of BCT entries supported by the
> boot ROM (since coreboot does not rely on the boot ROM for SDRAM
> initialization), so having a single set of SDRAM configurations for
> each board, indexed by APBDEV_PMC_STRAPPING_OPT_A_0[7:4] works just
> fine.
Does the bootloader adjust the DT that's passed to the kernel so that
only the relevant single set of EMC timings is contained in the DT?
On a system where the boot ROM initializes RAM, and where the HW design
might have multiple SDRAM configuration, here's what usually happens:
a) The BCT contains N sets of SDRAM configurations.
b) The boot ROM reads the SDRAM strapping bits, and uses them to pick
the correct SDRAM configuration from the N sets in the BCT.
c) The kernel DT has N sets of SDRAM configurations.
d) The kernel reads the SDRAM strapping bits, and uses them to pick the
correct SDRAM configuration from the N sets in the DT.
On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is
too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d)
won't work. I assume the kernel DT therefore must be adjusted to only
contain the single SDRAM configuration that is relevant for the current HW?
(isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index
and 2 bits for boot flash index, so max N is quite small?)
^ permalink raw reply
* [PATCH 2/2] ARM: multi_v7_defconfig: Enable MiPHY365x - ST's Generic (SATA & PCIe) PHY
From: Maxime Coquelin @ 2014-07-22 16:40 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722161732.GB31092@quad.lixom.net>
On 07/22/2014 06:17 PM, Olof Johansson wrote:
> On Tue, Jul 22, 2014 at 06:15:16PM +0200, Maxime Coquelin wrote:
>> Hi Olof,
>>
>> On 07/22/2014 06:06 PM, Olof Johansson wrote:
>>> On Tue, Jul 22, 2014 at 10:39:45AM +0100, Lee Jones wrote:
>>>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>>>
>>> Appled both. In the future, feel free to batch them up in one commit,
>>> no need to keep them separate since they're related.
>>
>> I added both to the pull request for defconfig I sent this afternoon.
>> Not an issue?
>
> Yes, that's an issue.
>
> If you comment on a patch you apply, then others will know that you have picked
> it up.
Oops... I usually reply to notify when I pick a patch, but I missed for
this one.
Sorry for that.
Maxime
>
> I've dropped the versions I applied now so that we don't get duplicate commits
> when I merge your branch.
>
>
> -Olof
>
^ permalink raw reply
* [PATCH 1/2] arm: use generic fixmap.h
From: Tomasz Figa @ 2014-07-22 16:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1404898792-4201-2-git-send-email-leif.lindholm@linaro.org>
Hi Leif,
On 09.07.2014 11:39, Leif Lindholm wrote:
> From: Mark Salter <msalter@redhat.com>
>
> ARM is different from other architectures in that fixmap pages are
> indexed with a positive offset from FIXADDR_START. Other architectures
> index with a negative offset from FIXADDR_TOP. In order to use the
> generic fixmap.h definitions, this patch redefines FIXADDR_TOP to be
> inclusive of the useable range. That is, FIXADDR_TOP is the virtual
> address of the topmost fixed page. The newly defined FIXADDR_END is
> the first virtual address past the fixed mappings.
>
> The patch also introduces local helper macros in highmem.c to reverse
> the iteration order of fixmap pages.
>
> Signed-off-by: Mark Salter <msalter@redhat.com>
> [Rebased to 3.16-rc4, reverse kmap fixmap traversal]
> Signed-off-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> arch/arm/include/asm/fixmap.h | 45 ++++++++++++++++++++++++++---------------
> arch/arm/mm/highmem.c | 13 +++++++-----
> arch/arm/mm/init.c | 2 +-
> 3 files changed, 38 insertions(+), 22 deletions(-)
>
I've tried to use this series to enable earlycon without hardcoded
static mappings, but apparently something is not right yet. Please see
below.
[snip]
> -extern void __this_fixmap_does_not_exist(void);
> +#define FIXMAP_PAGE_NORMAL (L_PTE_MT_WRITEBACK | L_PTE_YOUNG | L_PTE_PRESENT)
> +#define FIXMAP_PAGE_IO (L_PTE_MT_DEV_NONSHARED | L_PTE_YOUNG | L_PTE_PRESENT)
This set of flags gives a read-only mapping on the machine I'm testing
on (Exynos4412, Cortex A9MPcore). If I use the same set of flags as used
in arch/arm/mm/mmu.c for MT_DEVICE_NONSHARED then the mapping works fine.
Best regards,
Tomasz
^ permalink raw reply
* [PATCH 4/6] net/macb: add RX checksum offload feature
From: Florian Fainelli @ 2014-07-22 16:39 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CE9109.9040407@atmel.com>
2014-07-22 9:27 GMT-07:00 Cyrille Pitchen <cyrille.pitchen@atmel.com>:
> Le 18/07/2014 17:36, Eric Dumazet a ?crit :
>> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>>> ---
>>> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>>> drivers/net/ethernet/cadence/macb.h | 2 ++
>>> 2 files changed, 30 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>>> index 9bdcd1b..6acd6e2 100644
>>> --- a/drivers/net/ethernet/cadence/macb.c
>>> +++ b/drivers/net/ethernet/cadence/macb.c
>>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>>
>>> skb->protocol = eth_type_trans(skb, bp->dev);
>>> skb_checksum_none_assert(skb);
>>> + if (bp->dev->features & NETIF_F_RXCSUM)
>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>>
>>
>>
>> Really ?
>>
>> If this is what you meant, this deserves a big and fat comment.
>>
>>
>>
> Hi Eric,
>
> Isn't it the proper way to do? According to Cadence documentation about RX checksum offload:
> "If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded."
>
> If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function.
This is not the same thing as what the e1000 driver is doing, the
e1000 driver is checking a per-packet status mask which tells it
whether a given packet had its protocol level checksum computed by the
HW.
I think there might be some slight confusion here between the Ethernet
frame Frame Control Checksum which is at the Ethernet level, and will
(with most adapters not supporting RX_FCS_ERR) cause the Ethernet
controller to drop frames at the MAC level, and the higher level
protocol checksums such as the TCP or UDP checksum.
>
> Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register.
I don't think the Cadence GEM Ethernet controller is any different
than other hardware out there, there must be a per-packet status bit
which tells whether the TCP/UDP or other protocol checksum was
successfully validated by the hardware. One of the reason for that, is
that depending on the type of traffic your receive (e.g: ARP), there
might be no protocol-level checksum at all in the packet, and the
hardware should know about that.
>
> Did I make a mistake? Is it the kind of comment you expect to be added?
>
> Regards,
>
> Cyrille
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Florian
^ permalink raw reply
* [PATCH 0/2] pinctrl: sunxi: misc improvements for gpio
From: Linus Walleij @ 2014-07-22 16:33 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405358677-23657-1-git-send-email-wens@csie.org>
I'm waiting for Maxime to look at these patches before merging.
On Mon, Jul 14, 2014 at 7:24 PM, Chen-Yu Tsai <wens@csie.org> wrote:
> These 2 patches are fixes / improvements to the gpio side of the
> sunxi pinctrl driver. They are based on pinctrl/devel (3a19805).
> The patches change the same lines of code, so they are sent together.
>
> Patch 1 adds locking gpio lines when used as external interrupts.
> Similar patches were done by Linus and other maintainers for various
> platforms.
Nice.
> A somewhat related issue is that the sunxi pinctrl driver does not
> block users from requesting an already muxed pin as a gpio line.
> Maybe we should do some locking there as well? Are there any kernel
> interfaces for this? Or do we need to do it in the driver specifically
> for our hardware?
>
> (I had the unfortunate experience of poking GPIOs listed in the fex
> files, not noticing they were used by the uart console.)
The pin control core explicitly allows concurrent muxing to some
certain function and GPIO usage (see Documentation/pinctrl.txt).
This is because some HW may allow you to e.g. "spy" on pins
using it's GPIO registers, or use it from the GPIO subsystem
at the same time some other way.
If this does not apply to your hardware, you can further
restrictions in the driver, for example pinctrl_request_gpio()
can return something negative from the pin control part
of the driver, like -EBUSY.
> AFAIK pinctrl pin numbers are device specific, so I'm wondering if we
> should also number them in terms of offsets, rather than absolute pin
> numbers. It's more of an asthetic change though. Any thoughts?
Usually I say these pin numbers should try to match what is
in the data sheet so it is easy to understand and debug. Sometimes
pins are numbered with letters and stuff so they rather have names
than numbers, then some artificial numbering is applied, whatever
is helpful.
I'm not directly familiar with the sunxi numbering system though...
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH v2] bus: ARM CCN PMU driver
From: Olof Johansson @ 2014-07-22 16:30 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1406046564.25343.74.camel@hornet>
On Tue, Jul 22, 2014 at 9:29 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> On Tue, 2014-07-22 at 09:08 -0700, Olof Johansson wrote:
>> On Tue, Jul 22, 2014 at 3:11 AM, Pawel Moll <pawel.moll@arm.com> wrote:
>> > Hi Arnd, Kevin, Olof,
>> >
>> > On Fri, 2014-07-11 at 16:06 +0100, Pawel Moll wrote:
>> >> Driver providing perf backend for ARM Cache Coherent Network
>> >> interconnect. Supports counting all hardware events and crosspoint
>> >> watchpoints.
>> >>
>> >> Currently works with CCN-504 only, although there should be
>> >> no changes required for CCN-508 (just impossible to test it now).
>> >>
>> >> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
>> >
>> > Would you consider taking this through arm-soc tree? There isn't
>> > anything in MAINTAINERS, but I vaguely remember Arnd proposing this
>> > directory in the first place...
>>
>> Yeah, drivers/bus lacks an owner and we've normally been the ones
>> merging them. Please send a fresh version of the patch over.
>
> I've just rebased in on top of v3.16-rc6 and there's no change
> whatsoever (which is a good thing, I guess), so I can send v3, but the
> "changes since v2" will say "nothing" :-)
[PATCH RESEND v2] is the normal way to do it, but sure you can post a v3 too. ;)
-Olof
^ permalink raw reply
* [PATCH v2] bus: ARM CCN PMU driver
From: Pawel Moll @ 2014-07-22 16:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMijrD6SxAyjvQptmAB8swqcvXNkiVS_nRCtUVtX2vYUFQ@mail.gmail.com>
On Tue, 2014-07-22 at 09:08 -0700, Olof Johansson wrote:
> On Tue, Jul 22, 2014 at 3:11 AM, Pawel Moll <pawel.moll@arm.com> wrote:
> > Hi Arnd, Kevin, Olof,
> >
> > On Fri, 2014-07-11 at 16:06 +0100, Pawel Moll wrote:
> >> Driver providing perf backend for ARM Cache Coherent Network
> >> interconnect. Supports counting all hardware events and crosspoint
> >> watchpoints.
> >>
> >> Currently works with CCN-504 only, although there should be
> >> no changes required for CCN-508 (just impossible to test it now).
> >>
> >> Signed-off-by: Pawel Moll <pawel.moll@arm.com>
> >
> > Would you consider taking this through arm-soc tree? There isn't
> > anything in MAINTAINERS, but I vaguely remember Arnd proposing this
> > directory in the first place...
>
> Yeah, drivers/bus lacks an owner and we've normally been the ones
> merging them. Please send a fresh version of the patch over.
I've just rebased in on top of v3.16-rc6 and there's no change
whatsoever (which is a good thing, I guess), so I can send v3, but the
"changes since v2" will say "nothing" :-)
Thanks!
Pawel
^ permalink raw reply
* [PATCH 4/6] net/macb: add RX checksum offload feature
From: Cyrille Pitchen @ 2014-07-22 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405697784.10255.99.camel@edumazet-glaptop2.roam.corp.google.com>
Le 18/07/2014 17:36, Eric Dumazet a ?crit :
> On Fri, 2014-07-18 at 16:21 +0200, Cyrille Pitchen wrote:
>> Signed-off-by: Cyrille Pitchen <cyrille.pitchen@atmel.com>
>> ---
>> drivers/net/ethernet/cadence/macb.c | 29 ++++++++++++++++++++++++++++-
>> drivers/net/ethernet/cadence/macb.h | 2 ++
>> 2 files changed, 30 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
>> index 9bdcd1b..6acd6e2 100644
>> --- a/drivers/net/ethernet/cadence/macb.c
>> +++ b/drivers/net/ethernet/cadence/macb.c
>> @@ -766,6 +766,8 @@ static int gem_rx(struct macb *bp, int budget)
>>
>> skb->protocol = eth_type_trans(skb, bp->dev);
>> skb_checksum_none_assert(skb);
>> + if (bp->dev->features & NETIF_F_RXCSUM)
>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>
>
>
> Really ?
>
> If this is what you meant, this deserves a big and fat comment.
>
>
>
Hi Eric,
Isn't it the proper way to do? According to Cadence documentation about RX checksum offload:
"If any of the checksums (IP, TCP or UDP) are verified incorrect by the GEM, the packet is discarded."
If I understand, when RX checksum offload is enabled setting bit 24 of the Network Configuration Register, the driver only receives RX frames with correct checksum. Then it tells the kernel that there's no need to verify the checksum again in software. This is done setting skb->ip_summed to CHECKSUM_UNNECESSARY. Intel e1000 driver does the same in e1000_rx_checksum() function.
Also bit 24 of the Network Configuration Register is updated by macb_open() and macb_set_features() so it always matches the state of NETIF_F_RXCSUM flag in dev->features, once the network interface is up. That's why I'd rather read from dev->features than read from register.
Did I make a mistake? Is it the kind of comment you expect to be added?
Regards,
Cyrille
^ permalink raw reply
* [GIT PULL 2/3] ARM: tegra: move fuse code out of arch/arm
From: Stephen Warren @ 2014-07-22 16:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722102749.GB2219@arm.com>
On 07/22/2014 04:27 AM, Catalin Marinas wrote:
...
> ...It's also aimed at
> pushing back on hardware people who think they can mess up, for example,
> a GIC implementation because the rest is just software and you can
> always add #ifdefs.
I'm very concerned about this statement.
Yes, it would be nice if all HW was sanely designed, but it's not
always. The time to push back on bad HW design is during IP licensing
agreements or HW design reviews, not upstreaming.
Any pushback during SW upstreaming simply serves to make it difficult
for the SW people doing the upstreaming. It has zero impact on the
current HW design (it's already shipped). It probably has zero impact on
the next N revisions of the HW (they're already baked). It's possible it
has zero-to-minimal impact on any future HW (end-user product
requirements are what matter most).
^ permalink raw reply
* [GIT PULL v2] Mediatek basic support for v3.17
From: Matthias Brugger @ 2014-07-22 16:25 UTC (permalink / raw)
To: linux-arm-kernel
Patches 1-4 from v10 of the patch set have been merged by Daniel Lezcano [0].
This
----------------------------------------------------------------
The following changes since commit 7171511eaec5bf23fb06078f59784a3a0626b38f:
Linux 3.16-rc1 (2014-06-15 17:45:28 -1000)
are available in the git repository at:
https://github.com/mbgg/linux-mediatek.git tags/v3.17-next-mediatek-support
for you to fetch changes up to afaedea451149ab8f08c37ce688c8021ed347811:
arm: mediatek: add dts for Aquaris5 mobile phone (2014-07-22 17:52:38 +0200)
----------------------------------------------------------------
This patch set adds basic support for the Mediatek Cortex-A7 SoCs.
Support is quite basic, as the only component working up to now are the
timers.
----------------------------------------------------------------
Matthias Brugger (3):
arm: add basic support for Mediatek MT6589 boards
dt-bindings: add documentation for Mediatek SoC
arm: mediatek: add dts for Aquaris5 mobile phone
Documentation/devicetree/bindings/arm/mediatek.txt | 8 ++
arch/arm/Kconfig | 2 +
arch/arm/Makefile | 1 +
arch/arm/boot/dts/mt6589-aquaris5.dts | 25 ++++++
arch/arm/boot/dts/mt6589.dtsi | 94 ++++++++++++++++++++
arch/arm/mach-mediatek/Kconfig | 6 ++
arch/arm/mach-mediatek/Makefile | 1 +
arch/arm/mach-mediatek/mediatek.c | 27 ++++++
8 files changed, 164 insertions(+)
create mode 100644 Documentation/devicetree/bindings/arm/mediatek.txt
create mode 100644 arch/arm/boot/dts/mt6589-aquaris5.dts
create mode 100644 arch/arm/boot/dts/mt6589.dtsi
create mode 100644 arch/arm/mach-mediatek/Kconfig
create mode 100644 arch/arm/mach-mediatek/Makefile
create mode 100644 arch/arm/mach-mediatek/mediatek.c
^ permalink raw reply
* [GIT PULL 2/3] ARM: tegra: move fuse code out of arch/arm
From: Stephen Warren @ 2014-07-22 16:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722112651.GC2219@arm.com>
On 07/22/2014 05:26 AM, Catalin Marinas wrote:
> On Mon, Jul 21, 2014 at 05:38:25PM +0100, Stephen Warren wrote:
>> On 07/21/2014 09:54 AM, Catalin Marinas wrote:
>>> On arm64, I really want to get away from any SoC specific early
>>> initcall. One of the main reason is for things like SCU, interconnects,
>>> system cache configurations (even certain clocks) to be enabled in
>>> firmware before Linux starts (it's an education process but that's a way
>>> for example to prevent people sending patches to enable SoC coherency
>>> because they haven't thought about it before upstreaming).
>>>
>>> It would be nice to be able to initialise SoC stuff at device_initcall()
>>> level (and even keep such code as modules in initramfs) but one of the
>>> problems we have is dependency on clocks (and the clock model which
>>> doesn't follow the device/driver model). The of_platform_populate() is
>>> called at arch_initcall_sync (after arch_initcall to still allow some
>>> SoC code, if needed, to run at arch_initcall).
>>
>> The main thing I want to avoid is a ton of separate drivers that all
>> rely on each-other getting resolved by deferred probe. While that might
>> work out, it seems pointless to make the kernel try and probe a bunch of
>> stuff just to have it fail and get repeated, when we know exactly which
>> order everything should get initialized in.
>
> So of_platform_populate() is called at arch_initcall_sync() level on
> arm64. This allows at least two levels of probing separation before
> (e.g. drivers registered as arch_initcall) and after (device_initcall).
> If you register a driver earlier than arch_initcall_sync (see for
> example vexpress_osc_init), it will get probed when the platform devices
> are populated. Any later device_initcalls will get probed when the
> corresponding drivers are registered. If you need ordering between
> device_initcalls, I would recommend deferred probing.
>
> The tricky part is if you need more drivers to be initialised at
> arch_initcall_sync() in a specific order.
I believe relying on initcall ordering, even in the case where there are
enough initcall levels to achieve a particular SoC's needs, is
completely and utterly the wrong way to go.
Or at least, that's what the mantra has been for arch/arm for the last
few years. There's been plenty of active work to get away from
initcalls. Why should arch/arm64 be any different? Some consistency of
maintainer viewpoints would be nice.
(Note that I'm talking here about lots of separate initcalls that only
work in the right order due to manually assigning them initcall levels
that happen to work in the right order. Having a single initcall
function that explicitly calls all other initialization functions in the
right order is entirely different)
Disadvantages of using initcalls are:
* Not enough levels, so it's not generic/scalable.
* If you need to re-order things, you need to change the initcall level
of a bunch of stuff before/after it. This isn't scalable.
* It's hard to track them all down. Admittedly grep works, but that's a
lot more painful than just reading a function that calls other functions
explicitly.
* Each initcall has to determine if it's running on the correct HW or
not, rather than doing it once in a single top-level initcall, or in a
hook that only gets called at the right time.
...
>> Another issue is that we have SoCs which only differ in the CPU.
>
> Do you mean ARMv7 vs ARMv8 CPUs?
Yes.
^ permalink raw reply
* [PATCH 2/2] ARM: multi_v7_defconfig: Enable MiPHY365x - ST's Generic (SATA & PCIe) PHY
From: Olof Johansson @ 2014-07-22 16:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <53CE8E14.9020206@st.com>
On Tue, Jul 22, 2014 at 06:15:16PM +0200, Maxime Coquelin wrote:
> Hi Olof,
>
> On 07/22/2014 06:06 PM, Olof Johansson wrote:
> >On Tue, Jul 22, 2014 at 10:39:45AM +0100, Lee Jones wrote:
> >>Signed-off-by: Lee Jones <lee.jones@linaro.org>
> >
> >Appled both. In the future, feel free to batch them up in one commit,
> >no need to keep them separate since they're related.
>
> I added both to the pull request for defconfig I sent this afternoon.
> Not an issue?
Yes, that's an issue.
If you comment on a patch you apply, then others will know that you have picked
it up.
I've dropped the versions I applied now so that we don't get duplicate commits
when I merge your branch.
-Olof
^ permalink raw reply
* [STLinux Kernel] [PATCH v3 10/10] MAINTAINERS: Add sdhci-st file to ARCH/STI architecture
From: Maxime Coquelin @ 2014-07-22 16:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAOesGMhEnwGJR+SgM6XJhW9WT5LAo_-Hxd+kfQHBz7thu+QFJA@mail.gmail.com>
On 07/22/2014 06:00 PM, Olof Johansson wrote:
> On Tue, Jul 22, 2014 at 5:13 AM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
>> Hi Olof, Arnd, Kevin,
>>
>> I am preparing two tags for v3.17 regarding STi platform.
>> One for DT, and one for defconfigs.
>>
>> Except this one, I have no patches for ARM-SoC.
>> Can you take this patch directly?
>>
>> Or should I send a pull request with this one only?
>
> We can apply directly. I'll do that in a moment here.
Thanks!
Maxime
>
>
> -Olof
>
^ permalink raw reply
* [PATCH 2/2] ARM: multi_v7_defconfig: Enable MiPHY365x - ST's Generic (SATA & PCIe) PHY
From: Maxime Coquelin @ 2014-07-22 16:15 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722160630.GA31092@quad.lixom.net>
Hi Olof,
On 07/22/2014 06:06 PM, Olof Johansson wrote:
> On Tue, Jul 22, 2014 at 10:39:45AM +0100, Lee Jones wrote:
>> Signed-off-by: Lee Jones <lee.jones@linaro.org>
>
> Appled both. In the future, feel free to batch them up in one commit,
> no need to keep them separate since they're related.
I added both to the pull request for defconfig I sent this afternoon.
Not an issue?
Thanks,
Maxime
>
>
> -Olof
>
^ permalink raw reply
* [PATCH 1/3] ARM: exynos: remove unused <mach/memory.h>
From: Olof Johansson @ 2014-07-22 16:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722075138.GB28538@pengutronix.de>
On Tue, Jul 22, 2014 at 12:51 AM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> Hello,
>
> who takes care of this series?
>
> In fact they are all orthogonal to each other.
>
> The first patch
>
> ARM: exynos: remove unused <mach/memory.h>
>
> has been reviewed and tested by Tomasz Figa and Sachin Kamat. Can the
> Samsung people pick it up? Or armsoc?
Kukjin should apply this one.
> Patch 2 (v2!)
>
> ARM: remove remaining definitions of PLAT_PHYS_OFFSET from <mach/memory.h>
>
> touches several arch/arm/mach-*/include/mach/memory.h and
> arch/arm/Kconfig. armsoc? Russell?
This can go through either, but Russell has already reviewed it once
so send it to his patch tracker.
> Patch 3 fixes a warning regarding nommu and touches the Kconfig entry
> for Integrator and Renesas (non-multiplatform). armsoc?
Don't know without seeing the patch. What's the patch subject so I can find it?
-Olof
^ permalink raw reply
* [PATCH 2/3] ARM: smp_scu: enable SCU standby support
From: Catalin Marinas @ 2014-07-22 16:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20140722020911.GO8537@dragon>
On Tue, Jul 22, 2014 at 03:09:12AM +0100, Shawn Guo wrote:
> On Mon, Jul 21, 2014 at 11:26:38AM +0100, Catalin Marinas wrote:
> > The usual question - could the firmware enable this bit before Linux
> > starts?
>
> It could, I guess. Actually, on i.MX we're setting this bit in platform
> code right now. But I think setting this bit makes sense for most of
> the platforms, so it can reasonably be done in SCU core function. Isn't
> it the point of having core/common function after all?
Only that it wouldn't be consistent. If a platform (like some OMAPs)
boots in non-secure mode, the SCU would be already enabled the firmware
and your patch would not have any effect (that's one reason on arm64 I
try to get some consistency between various platforms and not rely on
the kernel which may or may not be able to enable certain features; but
it's late to enforced this on arm32).
> > We already do a read/modify/write sequence here and are only
> > supposed to write the enable bit as the rest are implementation defined.
>
> Isn't standby bit implemented by all A9 SCU except a couple of very
> early revisions (per Will)?
And we don't know the behaviour of setting this bit on such A9 early
revisions. So we can try to (1) find out if there are any in the field,
(2) read the RTL to see if anything happens or (3) add a check in Linux
for such revisions. I think (3) should be the case but you need to figure
out which revisions these are.
--
Catalin
^ permalink raw reply
* [PATCH 5/8] pinctrl: add driver for MB86S7x
From: Linus Walleij @ 2014-07-22 16:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1405233067-4725-1-git-send-email-mollie.wu@linaro.org>
On Sun, Jul 13, 2014 at 8:31 AM, Mollie Wu <mollie.wu@linaro.org> wrote:
> The mb86s70 and mb86s73 Fujitsu SoCs differ in that the latter provide a pinmux.
> GPIOs are supported on top of Pinctrl api.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Tetsuya Takinishi <t.takinishi@jp.fujitsu.com>
> Signed-off-by: Mollie Wu <mollie.wu@linaro.org>
This doesn't seem to be just about muxing but a full featured pin control
driver including pin config for electrical portions?
> +++ b/Documentation/devicetree/bindings/gpio/fujitsu,mb86s7x-gpio.txt
This looks OK.
> diff --git a/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt
> new file mode 100644
> index 0000000..ce2011b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/fujitsu,mb86s7x-pinctrl.txt
> @@ -0,0 +1,30 @@
> +Fujitsu MB86S7x Pin Controller
> +------------------------------
> +
> +Required properties:
> +- compatible: Should be "fujitsu,mb86s70-pinctrl" or "fujitsu,mb86s73-pinctrl"
> +- reg: Should contain the register physical address and length for the
> + pin controller.
> +- #gpio-range-cells: Should be 3
> +
> +Optional subnode-properties:
> +- mb86s7x,function: String specifying the function name.
> +- mb86s7x,drvst: Drive strength needed for pins to operate in that mode.
> + 0 (Hi-z), 2mA, 4mA, 6mA or 8mA
> +- mb86s7x,pullup: Should be 0 for Pull-Down or non-zero for Pull-Up
For all of these please switch the driver to using generic pin configuration.
We have stopped letting drivers use custom props for these things, even
functions are nowadays standardized to be just the function = "foo";
strings.
Consult
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
for the bindings,
Since the driver selects GENERIC_PINCONF, utilize the core
support for parsing DT info etc for these settings in
drivers/pinctrl/pinconf-generic.c and look how other drivers
exploit these helpers.
> +++ b/drivers/pinctrl/pinctrl-mb86s7x.c
(...)
> +#define PDR(x) (0x0 + x)
> +#define DDR(x) (0x10 + x)
> +#define PFR(x) (0x20 + x)
> +
> +#define CDRV_PDG(x) (0x0 + ((x) / 16) * 4)
> +#define FORCEZ_PDG(x) (0x100 + ((x) / 16) * 4)
> +#define PULL_PDG(x) (0x200 + ((x) / 16) * 4)
> +#define PIN_OFF(x) (((x) % 16) * 2)
I think the 0x0, 0x10, 0x20, 0x100, 0x200 etc offsets should
be named #defines telling us what kind of registers they will be
hitting.
> +#define PMUX_MB86S70 0
> +#define PMUX_MB86S73 1
More something like MB86S70_ID, MB86S73_ID or something are
better names for this?
> +/* Virtual pin numbers start so that reg offset calculations get simple */
> +enum {
> + PINS_VIRT = 128,
> + PINS_PCIE0 = PINS_VIRT,
> + PINS_HOLE1,
> + PINS_USB3H0,
> + PINS_HOLE2,
> + PINS_USB2H0,
> + PINS_USB2D0,
> + PINS_SDH30,
> + PINS_HOLE3,
> + PINS_EMMC0,
> + PINS_HSSPI0,
> + PINS_GMACD0,
> + PINS_GMACM0,
> + PINS_I2S0,
> + PINS_UART0,
> + PINS_OTHER0,
> + PINS_JTAG0,
> + PINS_PCIE1,
> + PINS_HOLE4,
> + PINS_USB3H1,
> + MB86S70_PMUX_PINS,
> +};
> +
> +struct mb86s70_gpio_chip {
> + spinlock_t lock; /* for goio regs */
> + struct clk *clk;
> + void __iomem *base;
> + struct gpio_chip gc;
> +};
> +
> +static int mb86s70_gpio_request(struct gpio_chip *gc, unsigned offset)
> +{
> + int ret = pinctrl_request_gpio(gc->base + offset);
> +
> + if (!ret) {
> + struct mb86s70_gpio_chip *gchip = container_of(gc,
> + struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + u32 val;
> +
> + spin_lock_irqsave(&gchip->lock, flags);
> + val = readl(gchip->base + PFR(offset / 8 * 4));
> + val &= ~(1 << (offset % 8)); /* as gpio-port */
> + writel(val, gchip->base + PFR(offset / 8 * 4));
When PFR is used like this I don't see how those macros are
really helpful. It's just very hard to read:
readl(base + PFR(offset / 8 * 4));
(Whis is already scary unless you know expression evaluation order by heart...)
Expands to
readl(base + 0x20 + (offset / 8 * 4));
Is it possible to just get rid of the PFR macro and create an inline
function like this:
static inline unsigned mb86s70_pfr(unsigned offset)
{
return 0x20 + (offset / 8 * 4);
}
And the above becomes:
#include <linux/bitops.h>
val = readl(gchip->base + mb86s70_pfr(offset));
val &= ~BIT(offset % 8);
writel(val, gchip->base + mb86s70_pfr(offset));
This way it's less cluttered IMO. But it's not a strong preference.
> +static int mb86s70_gpio_direction_output(struct gpio_chip *gc,
> + unsigned offset, int value)
> +{
> + struct mb86s70_gpio_chip *gchip =
> + container_of(gc, struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + unsigned char val;
> +
> + spin_lock_irqsave(&gchip->lock, flags);
> +
> + val = readl(gchip->base + PDR(offset / 8 * 4));
> + if (value)
> + val |= (1 << (offset % 8));
> + else
> + val &= ~(1 << (offset % 8));
In this and other places I'd just
val |= BIT(offset % 8); (etc)
> + writel(val, gchip->base + PDR(offset / 8 * 4));
> +
> + val = readl(gchip->base + DDR(offset / 8 * 4));
> + val |= (1 << (offset % 8));
> + writel(val, gchip->base + DDR(offset / 8 * 4));
> +
> + spin_unlock_irqrestore(&gchip->lock, flags);
> +
> + return 0;
> +}
Don't you want to use pinctrl_gpio_direction_output()
and pinctrl_gpio_direction_input() in these?
(Well maybe not.)
> +static int mb86s70_gpio_get(struct gpio_chip *gc, unsigned offset)
> +{
> + struct mb86s70_gpio_chip *gchip =
> + container_of(gc, struct mb86s70_gpio_chip, gc);
> + unsigned char val;
> +
> + val = readl(gchip->base + PDR(offset / 8 * 4));
> + val &= (1 << (offset % 8));
> + return val ? 1 : 0;
I usually just do this:
#include <linux/bitops.h>
return !!(readl(...) & BIT(offset % 8));
This applied in a few places simplifies the code I think.
> +static int mb86s70_gpio_probe(struct platform_device *pdev)
> +{
> + struct mb86s70_gpio_chip *gchip;
> + struct resource *res;
> + int ret;
> +
> + gchip = devm_kzalloc(&pdev->dev, sizeof(*gchip), GFP_KERNEL);
> + if (gchip == NULL)
> + return -ENOMEM;
Just
if (!gchip)
> +static int func_count, grp_count;
> +static const struct mb86s70_pmx_grp *mb86s7x_pgrps;
> +static const struct mb86s70_pmx_function *mb86s7x_pmx_funcs;
No static locals please. Add these to struct mb86s70_pmux_chip or
similar state container as they seem to be dynamic. Use
pinctrl_dev_get_drvdata()
to get from struct pinctrl_dev *pctl to the state container.
> +static int mb86s70_pctrl_get_groups_count(struct pinctrl_dev *pctl)
> +{
> + return grp_count;
> +}
> +
> +static const char *
> +mb86s70_pctrl_get_group_name(struct pinctrl_dev *pctl, unsigned selector)
> +{
> + return mb86s7x_pgrps[selector].name;
> +}
>
> +static int
> +mb86s70_pctrl_get_group_pins(struct pinctrl_dev *pctl, unsigned selector,
> + const unsigned **pins, unsigned *num_pins)
> +{
> + *pins = mb86s7x_pgrps[selector].pins;
> + *num_pins = mb86s7x_pgrps[selector].num_pins;
> +
> + return 0;
> +}
So these should use that method with pinctrl_dev_get_drvdata().
> +static int
> +mb86s70_pctrl_dt_node_to_map(struct pinctrl_dev *pctl,
> + struct device_node *node,
> + struct pinctrl_map **map, unsigned *num_maps)
> +{
Rewrite this to use pinconf_generic_dt_node_to_map()
and friends. Read other drivers doing this, for example pinctrl-msm.c
> + ret = of_property_read_string(node, "mb86s7x,function", &function);
> + if (ret) {
> + dev_err(pchip->dev,
> + "missing mb86s7x,function property in node %s\n",
> + node->name);
> + return -EINVAL;
> + }
For generic function parsing use just the name "function" rather
than "mb86s7x,function".
> +static const char * const pcie0_groups[] = {"pcie0_grp"};
> +static const char * const usb3h0_groups[] = {"usb3h0_grp"};
> +static const char * const usb2h0_groups[] = {"usb2h0_grp"};
> +static const char * const usb2d0_groups[] = {"usb2d0_grp"};
> +static const char * const sdh30_groups[] = {"sdh30_grp"};
> +static const char * const emmc0_groups[] = {"emmc0_grp"};
> +static const char * const hsspi0_groups[] = {"hsspi0_grp"};
> +static const char * const gmacd0_groups[] = {"gmacd0_grp"};
> +static const char * const gmacm0_groups[] = {"gmacm0_grp"};
> +static const char * const i2s0_groups[] = {"i2s0_grp"};
> +static const char * const other0_groups[] = {"other0_grp"};
> +static const char * const jtag0_groups[] = {"jtag0_grp"};
> +static const char * const pcie1_groups[] = {"pcie1_grp"};
> +static const char * const usb3h1_groups[] = {"usb3h1_grp"};
> +static const char * const extint16_groups[] = {"extint16_grp"};
> +static const char * const extint5_groups[] = {"extint5_grp"};
> +static const char * const tsif0_groups[] = {"tsif0_grp"};
> +static const char * const tsif1_groups[] = {"tsif1_grp"};
> +static const char * const cfg_groups[] = {"cfg_grp"};
> +static const char * const uart0_groups[] = {"uart0_grp"};
> +static const char * const uart1_groups[] = {"uart1_grp"};
> +static const char * const uart2_groups[] = {"uart2_grp"};
> +static const char * const pl244_groups[] = {"pl244_grp"};
> +static const char * const trace_groups[] = {"trace_grp"};
> +static const char * const memcs_groups[] = {"memcs_grp"};
> +static const char * const cap_groups[] = {"cap_grp"};
> +static const char * const smt_groups[] = {"smt_grp"};
> +
> +static const struct mb86s70_pmx_function mb86s73_pmx_funcs[] = {
> + {
> + .prog_val = 0x1,
> + .name = "extint5",
> + .groups = extint5_groups,
> + .num_groups = ARRAY_SIZE(extint5_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "pcie0",
> + .groups = pcie0_groups,
> + .num_groups = ARRAY_SIZE(pcie0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb3h0",
> + .groups = usb3h0_groups,
> + .num_groups = ARRAY_SIZE(usb3h0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb2h0",
> + .groups = usb2h0_groups,
> + .num_groups = ARRAY_SIZE(usb2h0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb2d0",
> + .groups = usb2d0_groups,
> + .num_groups = ARRAY_SIZE(usb2d0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "sdh30",
> + .groups = sdh30_groups,
> + .num_groups = ARRAY_SIZE(sdh30_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "emmc0",
> + .groups = emmc0_groups,
> + .num_groups = ARRAY_SIZE(emmc0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "hsspi0",
> + .groups = hsspi0_groups,
> + .num_groups = ARRAY_SIZE(hsspi0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "gmacd0",
> + .groups = gmacd0_groups,
> + .num_groups = ARRAY_SIZE(gmacd0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "gmacm0",
> + .groups = gmacm0_groups,
> + .num_groups = ARRAY_SIZE(gmacm0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "i2s0",
> + .groups = i2s0_groups,
> + .num_groups = ARRAY_SIZE(i2s0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "other0",
> + .groups = other0_groups,
> + .num_groups = ARRAY_SIZE(other0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "jtag0",
> + .groups = jtag0_groups,
> + .num_groups = ARRAY_SIZE(jtag0_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "pcie1",
> + .groups = pcie1_groups,
> + .num_groups = ARRAY_SIZE(pcie1_groups),
> + },
> + {
> + .prog_val = 0x0,
> + .name = "usb3h1",
> + .groups = usb3h1_groups,
> + .num_groups = ARRAY_SIZE(usb3h1_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "cfg",
> + .groups = cfg_groups,
> + .num_groups = ARRAY_SIZE(cfg_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart0",
> + .groups = uart0_groups,
> + .num_groups = ARRAY_SIZE(uart0_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart1",
> + .groups = uart1_groups,
> + .num_groups = ARRAY_SIZE(uart1_groups),
> + },
> + {
> + .prog_val = 0x1,
> + .name = "uart2",
> + .groups = uart2_groups,
> + .num_groups = ARRAY_SIZE(uart2_groups),
> + },
> + {
> + .prog_val = 0x2,
> + .name = "trace",
> + .groups = trace_groups,
> + .num_groups = ARRAY_SIZE(trace_groups),
> + },
> + {
> + .prog_val = 0x2,
> + .name = "pl244",
> + .groups = pl244_groups,
> + .num_groups = ARRAY_SIZE(pl244_groups),
> + },
> + {
> + .prog_val = 0x3,
> + .name = "smt",
> + .groups = smt_groups,
> + .num_groups = ARRAY_SIZE(smt_groups),
> + },
> + {
> + .prog_val = 0x3,
> + .name = "memcs",
> + .groups = memcs_groups,
> + .num_groups = ARRAY_SIZE(memcs_groups),
> + },
> +};
Open-coding the function tables like this is certainly OK, but most
drivers will use macros to compress the tables. But I'm happy with
this.
> +static int
> +mb86s70_pmx_get_functions_count(struct pinctrl_dev *pctl)
> +{
> + return func_count;
> +}
> +
> +static const char *
> +mb86s70_pmx_get_function_name(struct pinctrl_dev *pctl, unsigned selector)
> +{
> + return mb86s7x_pmx_funcs[selector].name;
> +}
> +
> +static int
> +mb86s70_pmx_get_function_groups(struct pinctrl_dev *pctl,
> + unsigned selector, const char * const **groups,
> + unsigned * const num_groups)
> +{
> + *groups = mb86s7x_pmx_funcs[selector].groups;
> + *num_groups = mb86s7x_pmx_funcs[selector].num_groups;
> +
> + return 0;
> +}
Again get these things from the state container.
> +static int
> +mb86s70_pmx_enable(struct pinctrl_dev *pctl,
> + unsigned func_selector, unsigned group_selector)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pins;
> + int i, j;
> +
> + if (group_selector >= grp_count) {
> + pr_err("%s:%d\n", __func__, __LINE__);
> + return -EINVAL;
> + }
> +
> + j = mb86s7x_pgrps[group_selector].num_pins;
> + pins = mb86s7x_pgrps[group_selector].pins;
> +
> + spin_lock_irqsave(&pchip->lock, flags);
> +
> + /* Busy if any pin in the same 'bunch' is taken */
> + for (i = 0; i < j; i++) {
> + u32 val;
> + int p = pins[i] / 4 * 4;
> +
> + if (pins[i] >= PINS_VIRT) /* Not for virtual pins */
> + continue;
> +
> + val = readl(pchip->base + p);
> + /* skip if no change needed */
> + if (val == mb86s7x_pmx_funcs[func_selector].prog_val)
> + continue;
> +
> + if (pchip->pin_busy[p]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p);
> + goto busy_exit;
> + }
> +
> + if (pchip->pin_busy[p + 1]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+1);
> + goto busy_exit;
> + }
I don't see why you are doing this and keeping track of
pins as "busy" or not. One thing the pin control core does
is to make sure pins do not collide in different use cases,
it seems like you're re-implementing this check again.
> + if (p == 64)
> + continue;
This if-clause seems dubious. At least add a comment as
to what is happening here and why.
> + if (pchip->pin_busy[p + 2]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+2);
> + goto busy_exit;
> + }
> +
> + if (pchip->pin_busy[p + 3]) {
> + pr_err("%s:%d:%d %d busy\n",
> + __func__, __LINE__, pins[i], p+3);
> + goto busy_exit;
> + }
I don't understand this either.
Why all this fuzz around the four pins from p ... p+3?
> + continue;
> +busy_exit:
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_err("%s:%d Take a look!\n", __func__, __LINE__);
> + return -EBUSY;
> + }
You don't have to have the busy_exit: inside the for-loop right?
Just push it below the return 0; in the end of this function
so you can get rid of the dangling continue;
> + pr_debug("Going to enable %s on pins -",
> + mb86s7x_pmx_funcs[func_selector].name);
> + for (i = 0; i < j; i++) {
> + int p = pins[i];
> +
> + pr_debug(" %d", p);
> + pchip->pin_busy[p] = true;
So I'm questioning this....
> + if (p < PINS_VIRT) /* Not for virtual pins */
We need an explanation somewhere about what "virtual pins"
means in this driver, I have never seen that before.
> + writel(mb86s7x_pmx_funcs[func_selector].prog_val,
> + pchip->base + p / 4 * 4);
This may need some static inline like described above to simplify
the code and make it more readable, but I see what is going on.
> + }
> +
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_debug("\n");
> +
> + return 0;
> +}
> +
> +static void
> +mb86s70_pmx_disable(struct pinctrl_dev *pctl,
> + unsigned func_selector, unsigned group_selector)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pins;
> + int i, j;
> +
> + if (group_selector >= grp_count) {
> + pr_err("%s:%d\n", __func__, __LINE__);
> + return;
> + }
> +
> + j = mb86s7x_pgrps[group_selector].num_pins;
> + pins = mb86s7x_pgrps[group_selector].pins;
> +
> + pr_debug("Going to disable %s on pins -",
> + mb86s7x_pmx_funcs[func_selector].name);
> + spin_lock_irqsave(&pchip->lock, flags);
> + for (i = 0; i < j; i++) {
> + pr_debug(" %d", pins[i]);
> + pchip->pin_busy[pins[i]] = false;
> + }
> + spin_unlock_irqrestore(&pchip->lock, flags);
> + pr_debug("\n");
> +}
Remove this function. The .disable member is gone from
struct pinmux_ops, as it was ambiguous, see commit
2243a87d90b42eb38bc281957df3e57c712b5e56
"pinctrl: avoid duplicated calling enable_pinmux_setting for a pin"
> +static int
> +mb86s70_pmx_gpio_set_direction(struct pinctrl_dev *pctl,
> + struct pinctrl_gpio_range *range,
> + unsigned pin, bool input)
> +{
> + struct gpio_chip *gc = range->gc;
> + struct mb86s70_gpio_chip *gchip = container_of(gc,
> + struct mb86s70_gpio_chip, gc);
> + unsigned long flags;
> + u32 off, bit, val;
> +
> + if (pin >= 64)
> + return -EINVAL;
This is a bit strange again...
> + spin_lock_irqsave(&gchip->lock, flags);
> + bit = (pin - gc->base) % 8;
> + off = (pin - gc->base) / 8 * 4;
> + val = readl(gchip->base + DDR(off));
> + if (input)
> + val &= ~(1 << bit);
> + else
> + val |= (1 << bit);
> + writel(val, gchip->base + DDR(off));
> + spin_unlock_irqrestore(&gchip->lock, flags);
> +
> + return 0;
> +}
So .set_direction is implemented but not called from the
gpiochip functions as pointed out above.
> +static int
> +mb86s70_pin_config_group_get(struct pinctrl_dev *pctl, unsigned group,
> + unsigned long *config)
> +{
> + return -EINVAL;
> +}
Don't implement stubs for this, just leave the vtable entry
unassigned, the core will return an error.
However it doesn't seem so hard to implement this
actually: the set code just below does exactly this, retrieves
the right bits and alters them, so why is not get implemented?
> +static int
> +mb86s70_pin_config_group_set(struct pinctrl_dev *pctl, unsigned group,
> + unsigned long *configs, unsigned num_configs)
> +{
> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
> + unsigned long flags;
> + const unsigned *pin;
> + int i, j, p;
> + u32 val, ds;
> +
> + p = mb86s7x_pgrps[group].num_pins;
> + pin = mb86s7x_pgrps[group].pins;
> +
> + spin_lock_irqsave(&pchip->lock, flags);
> +
> + for (i = 0; i < num_configs; i++) {
> + switch (pinconf_to_config_param(configs[i])) {
> + case PIN_CONFIG_DRIVE_STRENGTH:
> + /* Drive Strength should be 2, 4, 6 or 8 mA */
> + ds = pinconf_to_config_argument(configs[i]);
> + ds = (ds - 2) / 2;
I like this use of SI units, so use the generic bindings too!
(...)
> + pchip->desc.name = dev_name(&pdev->dev);
> + pchip->desc.npins = MB86S70_PMUX_PINS;
> + pchip->desc.pins = pchip->pins;
> + pchip->desc.pctlops = &mb86s70_pctrl_ops;
> + pchip->desc.pmxops = &mb86s70_pmx_ops;
> + pchip->desc.owner = THIS_MODULE;
> + if (type == PMUX_MB86S73) {
> + pchip->desc.confops = &mb86s70_conf_ops;
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + pchip->conf_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(pchip->conf_base))
> + return PTR_ERR(pchip->conf_base);
> + mb86s7x_pmx_funcs = mb86s73_pmx_funcs;
> + func_count = ARRAY_SIZE(mb86s73_pmx_funcs);
> + mb86s7x_pgrps = mb86s73_pgrps;
> + grp_count = ARRAY_SIZE(mb86s73_pgrps);
> + } else {
> + mb86s7x_pmx_funcs = mb86s70_pmx_funcs;
> + func_count = ARRAY_SIZE(mb86s70_pmx_funcs);
> + mb86s7x_pgrps = mb86s70_pgrps;
> + grp_count = ARRAY_SIZE(mb86s70_pgrps);
> + }
As mentioned don't use static locals for these things, add these
to the state container.
> +static const struct of_device_id mb86s70_gpio_dt_ids[] = {
> + { .compatible = "fujitsu,mb86s7x-gpio" },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, mb86s70_gpio_dt_ids);
"fujitsu" does not seem to exist in
Documentation/devicetree/bindings/vendor-prefixes.txt
Do you want to add it?
> +static struct platform_driver mb86s70_gpio_driver = {
> + .probe = mb86s70_gpio_probe,
> + .driver = {
> + .name = "mb86s70-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = mb86s70_gpio_dt_ids,
> + },
> +};
> +
> +static struct platform_driver mb86s70_pinmux_driver = {
> + .probe = mb86s70_pinmux_probe,
> + .driver = {
> + .name = "mb86s70-pinmux",
> + .owner = THIS_MODULE,
> + .of_match_table = mb86s70_pinmux_dt_ids,
> + },
> +};
> +
> +static int __init mb86s70_pins_init(void)
> +{
> + int ret;
> +
> + ret = platform_driver_register(&mb86s70_pinmux_driver);
> + if (ret)
> + return ret;
> +
> + return platform_driver_register(&mb86s70_gpio_driver);
> +}
> +subsys_initcall(mb86s70_pins_init);
It's not really necessary to split this up in two drivers in one
file, it is possible to just use one driver: mb86s70_pinmux_driver,
have just one .probe() function, and register *both* the gpiochip
*and* pin control driver from that probe().
(Well pin control first the gpiochip I guess, but you get the
idea.)
This driver needs some work, but I like the start, please keep at it.
Yours,
Linus Walleij
^ 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