Message-ID: <20230516122931.il4ai7fyxdo5gsff@bogus>
Date:   Tue, 16 May 2023 13:29:31 +0100
From:   Sudeep Holla <sudeep.holla@....com>
To:     "lihuisong (C)" <lihuisong@...wei.com>
Cc:     Arnd Bergmann <arnd@...db.de>,
        Bjorn Andersson <andersson@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sudeep Holla <sudeep.holla@....com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Shawn Guo <shawnguo@...nel.org>, linux-kernel@...r.kernel.org,
        soc@...nel.org, wanghuiqiang@...wei.com, tanxiaofei@...wei.com,
        liuyonglong@...wei.com, huangdaode@...wei.com,
        linux-acpi@...r.kernel.org, Len Brown <lenb@...nel.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        devicetree@...r.kernel.org, Rob Herring <robh+dt@...nel.org>,
        Frank Rowand <frowand.list@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Subject: Re: [PATCH] soc: hisilicon: Support HCCS driver on Kunpeng SoC

On Tue, May 16, 2023 at 03:35:54PM +0800, lihuisong (C) wrote:
>
> 在 2023/5/15 21:08, Sudeep Holla 写道:
> > On Thu, May 04, 2023 at 09:16:16PM +0800, lihuisong (C) wrote:
> > > I'm tring to use CRS with GAS to report PCC channel ID and get other
> > > informations driver need by address.
> > OK you had pcc-chan-id pcc-type and device-flags in the DSD style bindings
> > to begin with. I haven't understood device-flags here so can't comment on
> > that.
>
> We want to use the 'device-flags' to report some information by bit.

Please give more details, until then NACK for the idea.

> Currently, this driver requests PCC channel and use type2 to communicate
> with firmware.

OKAY...

> But, if some platform support type3 and PCC Operation Region, driver can
> choice this method to communicate with firmware.
> So firmware and driver have to use this flag to make compatibility.
>

I would rather add such things to the spec if it is any sort of limitation
with the current specification.

> >
> > > I found a way to obtain the generic register information according to
> > > "Referencing the PCC address space" in ACPI spec.
> > > And driver also get the PCC generic register information successfully.
> > >
> > Can you elaborate ? I assume by that you must be able to get pcc-chan-id
>
> Yes，driver can get pcc-chan-id by below register.
>
> Register (PCC, RegisterBitWidth, RegisterBitOffset, RegisterAddress, AccessSize)
>

Good to know.

> > right ? You must not need pcc-type as the pcc mailbox driver must handle
> > the type for you. If not, we may need to fix or add any missing support.
> Yes, PCC driver doesn't support it currently. And aother patch [1] we've
> been talking about does it.
> If it is applied to kernel, we can drop this pcc-type here.
>
> [1] https://patchwork.kernel.org/project/linux-acpi/patch/20230423110335.2679-2-lihuisong@huawei.com/

OK then we are good, no need for pcc-type then ?

> >
> > > But I don't know how to set and use the address in PCC register.
> > It must be same as what you would have specified in you new bindings
> > under "pcc-chan-id". I am confused as you say you were able to get the
> > PCC generic register information successfully but you still claim you
> > don't know how to set or use the address.
> My confusion about this address is mentioned below.

OK

> > > Where should this address come from?
> > > It seems that ACPI spec is not very detailed about this.
> > > Do you have any suggestions?
> > >
> > I am afraid, I don't have any as I am failing to understand the exact issue
> > you are facing.
> >
> > Let me try to ask the question explicity here:
> >
> > If you are just referring to just the <RegisterAddress,> in
> >
> > Register (PCC, RegisterBitWidth, RegisterBitOffset, RegisterAddress, AccessSize)
> Yeah, this is what I'm using.
> >
> > then,
> >
> > RegisterAddress is usually the offset in the comms address associated with
> Communication subspace in share memory of PCC subspace?
> > the PCC subspace ID specified in AccessSize. Yes the use of AccessSize for
> > the PCC subspace ID is bit confusing though.
> >
> > You can either list all the registers with _CRS individually or the driver
> List all the registers as following way?
> Name (_CRS, ResourceTemplate ()  // _CRS: Current Resource Settings
> {
>     QWordMemory (ResourceProducer, PosDecode, MinFixed, MaxFixed,
> NonCacheable, ReadWrite,
>         0x0000000000000000, // Granularity
>         0x0000000098190000, // Range Minimum
>         0x000000009819FFFF, // Range Maximum
>         0x0000000000000000, // Translation Offset
>         0x0000000000010000, // Length
>         ,, , AddressRangeMemory, TypeStatic)
> })

Not sure if you can use QWordMemory here TBH.

> > can just use the PCC subspace ID in AccessSize and keep RegisterAddress = 0
> > but access individual offset based on its own knowledge. I haven't seen the
> Following words come from ACPI spec.
> -->
> As an example, the following resource template refers to the feld occupying
> bits 8 through 15 at address 0x30 in PCC
> subspace 9:
> ResourceTemplate()
> {
> Register (
>     PCC, //AddressSpaceKeyword
>     8, //RegisterBitWidth
>     8, //RegisterBitOffset
>    .pcc 0x30, //RegisterAddress
>     9 //AccessSize (subspace ID)
>     )
> }
>
> If the width of the address is 32bit, set RegisterAddress to 0,
> RegisterBitOffset to 0 and set RegisterBitWidth to 64 here.
> Driver can access to the ((void __iomem *)pcc_comm_addr + 0x8 + 0) and
> ((void __iomem *)pcc_comm_addr + 0x8 + 4) address，right?
> (This virtual address = pcc mapped address + header size + offset within PCC
> subspace.)

Yes that's my understanding. I remember seeing the driver is just fetching
pcc-chan-id using DSD style key-value pair, which means you don't need
any other info other than the PCC subspace/channel ID, just have address
as 0.

Also I see the driver uses type for just rejecting the type 3 PCCT. The
question is will the driver probe and run on a platform with type 3 PCCT ?
If so what is the problem running on such a platform. I see it is useless
check in the driver and can be dropped. Also the comment above enum
HCCS_DEV_FLAGS_INTR_B is confusing and  so is the way flags is used.

> > full driver yet but I assuming that's how you would have used if you went with
> > your DSD pcc-chan-id proposal.
> >
> > > On the other hand, we think that System Memory space + method can also
> > > achieve above goal. What do you think of that?
> > Again I don't understand what you mean by that.
> Sorry, here is what I want to say.
> -->
> OperationRegion (CCS0, SystemMemory, 0x00000002081000CC, 0x04)
> Field (CCS0, DWordAcc, NoLock, Preserve)
> {
>     HAU1,   32
> }
> OperationRegion (CCS1, SystemMemory, 0x0000000201070410, 0x04)
> Field (CCS1, DWordAcc, NoLock, Preserve)
> {
>     HCGE,   32
> }
> Method (_DSM, 2, Serialized)  // _DSM: Device-Specific Method
> {
>     If ((Arg0 == ToUUID ("b06b81ab-0134-4a45-9b0c-483447b95fa7")))
>     {
>         If ((Arg1 == One))
>         {
>             Return (HAU1)
>         }
>
>         Return (HCGE)
>     }
> }
>
> Driver can call _DSM method to get some information, such as pcc_chan_id and
> device_flags.

Big fat NACK for _DSM for the above purpose, please stop abusing _DSM or _DSD
for such information which can be obtained with the existing _CRS.

--
Regards,
Sudeep

Powered by blists - more mailing lists