Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200721141742.996350-1-jiaxun.yang@flygoat.com> <20200721141742.996350-2-jiaxun.yang@flygoat.com>
In-Reply-To: <20200721141742.996350-2-jiaxun.yang@flygoat.com>
From:   Rob Herring <robh+dt@kernel.org>
Date:   Wed, 22 Jul 2020 09:55:21 -0600
Message-ID: <CAL_JsqLFZc8ChR=Sj3N-G-b0tq_4F0-HSQPt5+WmkcDZrxUznA@mail.gmail.com>
Subject: Re: [PATCH v2 1/5] of_address: Add bus type match for pci ranges parser
To:     Jiaxun Yang <jiaxun.yang@flygoat.com>
Cc:     "open list:MIPS" <linux-mips@vger.kernel.org>,
        Thomas Bogendoerfer <tsbogend@alpha.franken.de>,
        Huacai Chen <chenhc@lemote.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Paul Burton <paulburton@kernel.org>,
        Nathan Chancellor <natechancellor@gmail.com>,
        Nick Desaulniers <ndesaulniers@google.com>,
        devicetree@vger.kernel.org,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 21, 2020 at 8:18 AM Jiaxun Yang <jiaxun.yang@flygoat.com> wrote:
>
> So the parser can be used to parse range property of ISA bus.
>
> As they're all using PCI-like method of range property, there is no need
> start a new parser.
>
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>
> --
> v2: Drop useless check, fix some na for bus_addr
>         add define of of_range_parser_init according to
>         Rob's suggestion.
> ---
>  drivers/of/address.c       | 27 +++++++++++++++------------
>  include/linux/of_address.h |  5 +++++
>  2 files changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 8eea3f6e29a4..7406636cea87 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -698,9 +698,10 @@ static int parser_init(struct of_pci_range_parser *parser,
>
>         parser->node = node;
>         parser->pna = of_n_addr_cells(node);
> -       parser->na = of_bus_n_addr_cells(node);
> -       parser->ns = of_bus_n_size_cells(node);
>         parser->dma = !strcmp(name, "dma-ranges");
> +       parser->bus = of_match_bus(node);
> +
> +       parser->bus->count_cells(parser->node, &parser->na, &parser->ns);
>
>         parser->range = of_get_property(node, name, &rlen);
>         if (parser->range == NULL)
> @@ -732,6 +733,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>         int na = parser->na;
>         int ns = parser->ns;
>         int np = parser->pna + na + ns;
> +       int busflag_na = 0;
>
>         if (!range)
>                 return NULL;
> @@ -739,12 +741,14 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>         if (!parser->range || parser->range + np > parser->end)
>                 return NULL;
>
> -       if (parser->na == 3)
> -               range->flags = of_bus_pci_get_flags(parser->range);
> -       else
> -               range->flags = 0;
> +       range->flags = parser->bus->get_flags(parser->range);
> +
> +       /* PCI and ISA have a extra cell for resource flags */
> +       if (strcmp(parser->bus->name, "pci") ||
> +           strcmp(parser->bus->name, "isa"))
> +               busflag_na = 1;

This should be abstracted out. Probably the easiest is to add a
'has_flags' boolean to the of_bus struct.

>
> -       range->pci_addr = of_read_number(parser->range, na);
> +       range->bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
>
>         if (parser->dma)
>                 range->cpu_addr = of_translate_dma_address(parser->node,
> @@ -759,11 +763,10 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>         /* Now consume following elements while they are contiguous */
>         while (parser->range + np <= parser->end) {
>                 u32 flags = 0;
> -               u64 pci_addr, cpu_addr, size;
> +               u64 bus_addr, cpu_addr, size;
>
> -               if (parser->na == 3)
> -                       flags = of_bus_pci_get_flags(parser->range);
> -               pci_addr = of_read_number(parser->range, na);
> +               flags = parser->bus->get_flags(parser->range);
> +               bus_addr = of_read_number(parser->range + busflag_na, na - busflag_na);
>                 if (parser->dma)
>                         cpu_addr = of_translate_dma_address(parser->node,
>                                         parser->range + na);
> @@ -774,7 +777,7 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser,
>
>                 if (flags != range->flags)
>                         break;
> -               if (pci_addr != range->pci_addr + range->size ||
> +               if (bus_addr != range->bus_addr + range->size ||
>                     cpu_addr != range->cpu_addr + range->size)
>                         break;
>
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 763022ed3456..3e8d6489cbf1 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -6,8 +6,11 @@
>  #include <linux/of.h>
>  #include <linux/io.h>
>
> +struct of_bus;
> +
>  struct of_pci_range_parser {
>         struct device_node *node;
> +       struct of_bus *bus;
>         const __be32 *range;
>         const __be32 *end;
>         int na;
> @@ -53,6 +56,7 @@ extern const __be32 *of_get_address(struct device_node *dev, int index,
>
>  extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
>                         struct device_node *node);
> +#define of_range_parser_init of_pci_range_parser_init
>  extern int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser,
>                         struct device_node *node);
>  extern struct of_pci_range *of_pci_range_parser_one(
> @@ -83,6 +87,7 @@ static inline int of_pci_range_parser_init(struct of_pci_range_parser *parser,
>  {
>         return -ENOSYS;
>  }
> +#define of_range_parser_init of_pci_range_parser_init

No need for 2 defines. Move this outside of the ifdef like the others.