Re: [PATCH V3] clk: bcm: Add driver for Northstar ILP clock

[RafaÅ MiÅecki]

On 11 August 2016 at 16:25, Jon Mason <jon.mason@xxxxxxxxxxxx> wrote:
> On Thu, Aug 11, 2016 at 4:49 AM, RafaÅ MiÅecki <zajec5@xxxxxxxxx> wrote:
>> On 10 August 2016 at 20:21, Jon Mason <jon.mason@xxxxxxxxxxxx> wrote:
>>> On Wed, Aug 10, 2016 at 1:44 PM, Ray Jui <ray.jui@xxxxxxxxxxxx> wrote:
>>>> On 8/10/2016 10:28 AM, RafaÅ MiÅecki wrote:
>>>>>
>>>>> On 10 August 2016 at 19:22, Jon Mason <jon.mason@xxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> On Wed, Aug 10, 2016 at 8:05 AM, RafaÅ MiÅecki <zajec5@xxxxxxxxx> wrote:
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>>>>>>> b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>>>>>>> new file mode 100644
>>>>>>> index 0000000..a18c73f
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/clock/brcm,ns-ilp.txt
>>>>>>> @@ -0,0 +1,40 @@
>>>>>>> +Broadcom Northstar ILP clock
>>>>>>> +============================
>>>>>>> +
>>>>>>> +This binding uses the common clock binding:
>>>>>>> +    Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>>>> +
>>>>>>> +This binding is used for ILP clock (sometimes referred as "slow clock")
>>>>>>> +on Broadcom Northstar devices using Corex-A7 CPU.
>>>>>>> +
>>>>>>> +This clock is part of PMU (Power Management Unit), a Broadcom's device
>>>>>>> +handing power-related aspects. Please note PMU contains more
>>>>>>> subdevices,
>>>>>>> +ILP is only one of them.
>>>>>>> +
>>>>>>> +ILP's rate has to be calculated on runtime and it depends on ALP clock
>>>>>>> +which has to be referenced.
>>>>>>> +
>>>>>>> +Required properties:
>>>>>>> +- compatible: "brcm,ns-ilp"
>>>>>>> +- reg: iomem address range of PMU (Power Management Unit)
>>>>>>> +- reg-names: "pmu", the only needed & supported reg right now
>>>>>>> +- clocks: has to reference an ALP clock
>>>>>>> +- #clock-cells: should be <0>
>>>>>>> +
>>>>>>> +Example:
>>>>>>> +
>>>>>>> +pmu@18012000 {
>>>>>>> +       compatible = "simple-bus";
>>>>>>> +       ranges = <0x00000000 0x18012000 0x00001000>;
>>>>>>
>>>>>>
>>>>>> I don't see a corresponding DT entry in this patch, but 18012000 is
>>>>>> the PCI block.  So, I am concerned this will collide if used there.
>>>>>>
>>>>>> I looked at the NS register reference guide, and I cannot find the
>>>>>> registers you are trying to reference.  Is this supposed to be
>>>>>> referencing the LCPLL clock registers in DMU?  If so, there is already
>>>>>> a driver in there for this (see drivers/clk/bcm/clk-nsp.c).
>>>>>
>>>>>
>>>>> This patch is for BCM53573 family, not BCM4708 family you are looking at.
>>>>>
>>>>> Found chip with id 53573, rev 0x02 and package 0x01
>>>>> Core 0 found: ChipCommon (manuf 0x4BF, id 0x800, rev 0x36, class 0x0)
>>>>> Core 1 found: IEEE 802.11 (manuf 0x4BF, id 0x812, rev 0x38, class 0x0)
>>>>> Core 2 found: PCIe Gen 2 (manuf 0x4BF, id 0x501, rev 0x05, class 0x0)
>>>>> Core 3 found: ARM CA7 (manuf 0x4BF, id 0x847, rev 0x00, class 0x0)
>>>>> Core 4 found: USB 2.0 Host (manuf 0x4BF, id 0x819, rev 0x05, class 0x0)
>>>>> Core 5 found: GBit MAC (manuf 0x4BF, id 0x82D, rev 0x08, class 0x0)
>>>>> Core 6 found: I2S (manuf 0x4BF, id 0x834, rev 0x06, class 0x0)
>>>>> Core 7 found: CNDS DDR2/3 memory controller (manuf 0x4BF, id 0x846,
>>>>> rev 0x00, class 0x0)
>>>>> Core 8 found: NAND flash controller (manuf 0x4BF, id 0x509, rev 0x01,
>>>>> class 0x0)
>>>>> Core 9 found: IEEE 802.11 (manuf 0x4BF, id 0x812, rev 0x38, class 0x0)
>>>>> Core 10 found: GBit MAC (manuf 0x4BF, id 0x82D, rev 0x08, class 0x0)
>>>>> Core 11 found: I2S (manuf 0x4BF, id 0x834, rev 0x06, class 0x0)
>>>>> Core 12 found: GCI (manuf 0x4BF, id 0x840, rev 0x08, class 0x0)
>>>>> Core 13 found: PMU (manuf 0x4BF, id 0x827, rev 0x1C, class 0x0)
>>>>>
>>>>
>>>> Out of curiosity, I searched the datasheet and found this is a wireless
>>>> router SoC done by the WLAN team. It happens to share some peripherals with
>>>> other iProc based SoCs.
>>>>
>>>> I cannot find a code name for this SoC from our internal documents. I guess
>>>> that name "Northstar" used here has confused both Jon and me.
>>>
>>> Ray is right.  I just spoke to one of the people here with knowledge
>>> of the HW, and this is not related at all to the 4708/9/5301X.  It MAY
>>> have some of the same peripherals, but the core is different (Cortex
>>> A7 instead of A9).
>>>
>>> I think we are best off to change the name and turn this into a
>>> separate device tree, driver base, etc.  I wasn't able to get a code
>>> name, so perhaps simply call it "BCM53573".
>>
>> Yes, I said clearly it uses Corex-A7 in the commit message and
>> Documentation entry.
>>
>> Florian already shared his doubts about BCM53573 belonging to the
>> Northstar, but I found out [1] that your (Broadcom's) SDK treats it as
>> Northstar device:
>>
>> Asus RT-AC1200G+
>>
>> # cat /proc/cpuinfo
>> Processor : ARMv7 Processor rev 5 (v7l)
>> BogoMIPS : 1795.68
>> Features : swp half thumb fastmult edsp
>> CPU implementer : 0x41
>> CPU architecture: 7
>> CPU variant : 0x0
>> CPU part : 0xc07
>> CPU revision : 5
>>
>> Hardware : Northstar Prototype
>> Revision : 0000
>> Serial : 0000000000000000
>>
>>
>> It seems Broadcom's WLAN team claims it's a Northstar and you claim
>> it's not. Could you discuss this internally and let us know, please?
>
> I discussed it with a member of the WLAN team prior to my previous
> email, which is why I made the previous statement.  It is a completely
> different SoC than the 5301x family.  It appears that someone in
> Marketing decided to reuse the name without discussing it with anyone
> in WLAN SW.  It is a completely different family of products than the
> iProc Northstar.  If we want to use the Northstar name, then perhaps
> we could call it something like "WLAN Northstar".  Either way, it
> needs to have a different mach-bcm entry.

Sorry, it wasn't clear to me initially, thanks for clarification. I'll
resend using different prefix.

-- 
RafaÅ