linux-kernel - Re: [PATCH 1/4] bus: uniphier-system-bus: add UniPhier System Bus Controller driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAK7LNATJENz_n5839RHrxOVi8QG+iyFV-sySKX=Xt=zsJtSKOQ@mail.gmail.com>
Date:	Thu, 26 Nov 2015 11:27:54 +0900
From:	Masahiro Yamada <yamada.masahiro@...ionext.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	devicetree@...r.kernel.org, Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>, arm@...nel.org,
	Kumar Gala <galak@...eaurora.org>,
	linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 1/4] bus: uniphier-system-bus: add UniPhier System Bus
 Controller driver

Hi Mark,

2015-11-25 2:38 GMT+09:00 Mark Rutland <mark.rutland@....com>:
> Hi,
>
>> >> +UniPhier System Bus Controller
>> >> +------------------------------
>> >> +
>> >> +The UniPhier System Bus Controller is a hardware block with registers that
>> >> +controls the System Bus accessing; how each bank is mapped onto the parent bus,
>> >> +various timing parameters of the bus access, etc.
>> >> +
>> >> +Required properties for System Bus Controller:
>> >> +- compatible: should be "socionext,uniphier-system-bus-controller".
>> >> +- reg: offsets and lengths of the register sets for the device.  It should
>> >> +  contain 2 regions: base & control register, misc register, in this order.
>> >
>> > The example also has a system-bus phandle.
>>
>> Actually, I was wondering which is better to describe the relation between
>> the bus and the controller,  phandle or compatible string..
>
> To describe relationships between nodes, use phandles.
>
> Compatible strings alone cannot define relationships -- you cannot
> encode how multiple instances are related.
>
>> > Is the "misc register" part of the bus controller, or is it a shared
>> > system controller?
>>
>> It is a part of the bus controller, but used for another purpose.
>> (i.e. partly this is syscon.  I know this is strange, but it is
>> what the hardware developers designed.)
>
> Ok. What else is going to need to use this in future?
>
>> > Assuming that the controller and bus are 1-1 related, make this a single
>> > node. e.g.
>> >
>> > system-bus {
>> >         compatible = "socionext,uniphier-system-bus";
>> >         reg = <0x58c00000 0x400>, <0x59800000 0x2000>;
>> >         #address-cells = <2>;
>> >         #size-cells = <1>;
>> >         ranges = <1 0x00000000 0x42000000 0x02000000>,
>> >                  <5 0x00000000 0x48000000 0x01000000>;
>> >
>> >         ...
>> >         child nodes here
>> >         ...
>> >
>> > };
>>
>> Hmm, make sense.  But, I prefer to reflect the hardware structure.
>>
>> The range of System Bus is <0x40000000 0x10000000>.
>>
>> The register of the System Bus Controller is
>> <0x58c00000 0x400>  (and <0x59800000 0x2000>)
>>
>>
>> The bus and its controller is different.
>
> So? We always describe the programming interface (i.e. the slave
> interface of the device that responds to the CPU).
>
> There's no need for separate nodes. It only makes the driver more
> complicated.
>
>> >> +static int uniphier_sbc_probe(struct platform_device *pdev)
>> >> +{
>> >> +     struct device *dev = &pdev->dev;
>> >> +     struct uniphier_sbc_priv *priv;
>> >> +     struct resource *regs;
>> >> +     struct device_node *bus_np;
>> >> +     int child_addrc, addrc, sizec, bank;
>> >> +     u64 child_addr, addr, size;
>> >> +     const __be32 *ranges;
>> >> +     int rlen, rone, ret;
>> >> +
>> >> +     bus_np = of_find_compatible_node(NULL, NULL,
>> >> +                                      "socionext,uniphier-system-bus");
>> >
>> > This is broken if you ever have multiple instances.
>> >
>> > Either use a single node, or if there is a more complex relationship
>> > between busses and their controllers, describe that explicitly with
>> > phandles.
>>
>>
>> Probably, I will stick to phandle in v2.
>
> I would prefer a single node unless there's some other complication
> regarding the relationship of the controller and the bus itself.


OK, i will try the single node way.


-- 
Best Regards
Masahiro Yamada
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/