diff mbox series

[v2,2/3] spi: Add HiSilicon v3xx SPI NOR flash controller driver

Message ID 1575900490-74467-3-git-send-email-john.garry@huawei.com
State New, archived
Headers show
Series HiSilicon v3xx SFC driver | expand

Commit Message

John Garry Dec. 9, 2019, 2:08 p.m. UTC
Add the driver for the HiSilicon v3xx SPI NOR flash controller, commonly
found in hi16xx chipsets.

This is a different controller than that in drivers/mtd/spi-nor/hisi-sfc.c;
indeed, the naming for that driver is poor, since it is really known as
FMC, and can support other memory technologies.

The driver module name is "hisi-sfc-v3xx", as recommended by HW designer,
being an attempt to provide a distinct name - v3xx being the unique
controller versioning.

Only ACPI firmware is supported.

DMA is not supported, and we just use polling mode for operation
completion notification.

The driver uses the SPI MEM OPs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/spi/Kconfig             |   9 +
 drivers/spi/Makefile            |   1 +
 drivers/spi/spi-hisi-sfc-v3xx.c | 284 ++++++++++++++++++++++++++++++++
 3 files changed, 294 insertions(+)
 create mode 100644 drivers/spi/spi-hisi-sfc-v3xx.c

Comments

John Garry Jan. 9, 2020, 3:54 p.m. UTC | #1
On 09/12/2019 14:08, John Garry wrote:
> +	if (ret)
> +		return ret;
> +
> +	if (op->data.dir == SPI_MEM_DATA_IN)
> +		hisi_sfc_v3xx_read_databuf(host, op->data.buf.in, len);
> +
> +	return 0;
> +}
> +
> +static int hisi_sfc_v3xx_exec_op(struct spi_mem *mem,
> +				 const struct spi_mem_op *op)
> +{
> +	struct hisi_sfc_v3xx_host *host;
> +	struct spi_device *spi = mem->spi;
> +	u8 chip_select = spi->chip_select;
> +
> +	host = spi_controller_get_devdata(spi->master);
> +
> +	return hisi_sfc_v3xx_generic_exec_op(host, op, chip_select);
> +}
> +
> +static const struct spi_controller_mem_ops hisi_sfc_v3xx_mem_ops = {
> +	.adjust_op_size = hisi_sfc_v3xx_adjust_op_size,
> +	.exec_op = hisi_sfc_v3xx_exec_op,
> +};
> +
> +static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hisi_sfc_v3xx_host *host;
> +	struct spi_controller *ctlr;
> +	u32 version;
> +	int ret;
> +
> +	ctlr = spi_alloc_master(&pdev->dev, sizeof(*host));
> +	if (!ctlr)
> +		return -ENOMEM;
> +


Hi Mark,

> +	ctlr->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
> +			  SPI_TX_DUAL | SPI_TX_QUAD;

I have an issue with dual/quad support. I naively thought that setting 
these bits would give me the highest protocol available.

However, now I notice that spi_device.mode needs to be set for supported 
protocols for the slave - I'm using the generic spi mem ops to check if 
protocols are supported based on this value.

 From checking acpi_spi_add_resource() or anywhere else, I cannot see 
how SPI_RX_DUAL or the others are set for spi_device.mode. What am I 
missing? Are these just not supported yet for ACPI? Or should the 
spi-nor code not be relying on this since we should be able to get this 
info from the SPI NOR part?

Cheers,
John

> +
> +	host = spi_controller_get_devdata(ctlr);
> +	host->dev = dev;
> +
> +	platform_set_drvdata(pdev, host);
> +
> +	host->regbase = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(host->regbase)) {
> +		ret = PTR_ERR(host->regbase);
> +		goto err_put_master;
Mark Brown Jan. 9, 2020, 9:28 p.m. UTC | #2
On Thu, Jan 09, 2020 at 03:54:00PM +0000, John Garry wrote:

> From checking acpi_spi_add_resource() or anywhere else, I cannot see how
> SPI_RX_DUAL or the others are set for spi_device.mode. What am I missing?
> Are these just not supported yet for ACPI? Or should the spi-nor code not be
> relying on this since we should be able to get this info from the SPI NOR
> part?

I'm not aware of any work on integrating this sort of stuff into ACPI
platforms so I think it's just not yet supported in ACPI.  I'm not
really sure what would be idiomatic for ACPI, figuring it out from what
the part supports might well be idiomatic there though I don't know how
common it is for people not to wire up all the data lines even if both
controller and device support wider transfers.  I've got a horrible
feeling that the idiomatic thing is a combination of that and a bunch of
per-device quirks.  There may be a spec I'm not aware of though I'd be a
bit surprised.
John Garry Jan. 10, 2020, 11:55 a.m. UTC | #3
On 09/01/2020 21:28, Mark Brown wrote:
> On Thu, Jan 09, 2020 at 03:54:00PM +0000, John Garry wrote:
> 
>>  From checking acpi_spi_add_resource() or anywhere else, I cannot see how
>> SPI_RX_DUAL or the others are set for spi_device.mode. What am I missing?
>> Are these just not supported yet for ACPI? Or should the spi-nor code not be
>> relying on this since we should be able to get this info from the SPI NOR
>> part?
> 

Hi Mark,

> I'm not aware of any work on integrating this sort of stuff into ACPI
> platforms so I think it's just not yet supported in ACPI.  I'm not
> really sure what would be idiomatic for ACPI, figuring it out from what
> the part supports might well be idiomatic there though I don't know how
> common it is for people not to wire up all the data lines even if both
> controller and device support wider transfers. 

OK, so I guess that is why we require the width property from the FW and 
can't blindly rely on SFDP.

  I've got a horrible
> feeling that the idiomatic thing is a combination of that and a bunch of
> per-device quirks.  There may be a spec I'm not aware of though I'd be a
> bit surprised.
> 

I'm not sure on that. I don't see anything in the ACPI spec.

I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the 
defacto method to describe the SPI NOR-compat part for ACPI - that's 
what I'm using. We could add properties there, but that seems improper.

I'll continue to look....

Thanks,
John
Mark Brown Jan. 10, 2020, 2:07 p.m. UTC | #4
On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote:

> I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the
> defacto method to describe the SPI NOR-compat part for ACPI - that's what
> I'm using. We could add properties there, but that seems improper.

OK, so that's just reusing the DT binding in which case everything
that's valid for the DT binding should also be valid for ACPI - I
thought that actually worked automatically without you having to do
anything in the code but ICBW.
John Garry Jan. 10, 2020, 2:58 p.m. UTC | #5
On 10/01/2020 14:07, Mark Brown wrote:

+ Mika, Andy

> On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: >
Hi Mark,

>> I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the
>> defacto method to describe the SPI NOR-compat part for ACPI - that's what
>> I'm using. We could add properties there, but that seems improper.
> 
> OK, so that's just reusing the DT binding in which case everything
> that's valid for the DT binding should also be valid for ACPI - I
> thought that actually worked automatically without you having to do
> anything in the code but ICBW.

I thought that it would be improper as we could be mixing ACPI methods 
to describe the serial bus (SPI Serial Bus Connection Resource 
Descriptor) and also DT properties which could conflict, like CS active 
high.

However I do see extra properties than "compatible" being added in DSD 
for PRP0001:
https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part)

And if we were to do this, I think that we would need to add some 
device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI 
FW parsing for ACPI path - I couldn't see that.

Thanks,
John
Mark Brown Jan. 10, 2020, 3:12 p.m. UTC | #6
On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote:
> On 10/01/2020 14:07, Mark Brown wrote:
> > On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: >

> > OK, so that's just reusing the DT binding in which case everything
> > that's valid for the DT binding should also be valid for ACPI - I
> > thought that actually worked automatically without you having to do
> > anything in the code but ICBW.

> I thought that it would be improper as we could be mixing ACPI methods to
> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and
> also DT properties which could conflict, like CS active high.

Yes, that's one of the issues with importing bits of DT into ACPI
unfortunately - you will get conflicts, it's not clear it's a good idea
to be using PRP0001 for SPI stuff given that there's bus level bindings
for both ACPI and SPI and they don't line up exactly.

> However I do see extra properties than "compatible" being added in DSD for
> PRP0001:
> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part)

> And if we were to do this, I think that we would need to add some
> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW
> parsing for ACPI path - I couldn't see that.

You'd need parsing code, yes.
John Garry Jan. 10, 2020, 4:09 p.m. UTC | #7
On 10/01/2020 15:12, Mark Brown wrote:
> On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote:
>> On 10/01/2020 14:07, Mark Brown wrote:
>>> On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: >
> 

Hi Mark,

>>> OK, so that's just reusing the DT binding in which case everything
>>> that's valid for the DT binding should also be valid for ACPI - I
>>> thought that actually worked automatically without you having to do
>>> anything in the code but ICBW.
> 
>> I thought that it would be improper as we could be mixing ACPI methods to
>> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and
>> also DT properties which could conflict, like CS active high.
> 
> Yes, that's one of the issues with importing bits of DT into ACPI
> unfortunately - you will get conflicts, it's not clear it's a good idea
> to be using PRP0001 for SPI stuff given that there's bus level bindings
> for both ACPI and SPI and they don't line up exactly.

Yeah, I'm not entirely comfortable with this yet.

> 
>> However I do see extra properties than "compatible" being added in DSD for
>> PRP0001:
>> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part)
> 
>> And if we were to do this, I think that we would need to add some
>> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW
>> parsing for ACPI path - I couldn't see that.
> 
> You'd need parsing code, yes.
> 

I'll continue to check the options.

Thanks,
john
Andy Shevchenko Jan. 10, 2020, 7:31 p.m. UTC | #8
On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote:
> On 10/01/2020 14:07, Mark Brown wrote:
> > On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: >

...

> > > I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the
> > > defacto method to describe the SPI NOR-compat part for ACPI - that's what
> > > I'm using. We could add properties there, but that seems improper.
> > 
> > OK, so that's just reusing the DT binding in which case everything
> > that's valid for the DT binding should also be valid for ACPI - I
> > thought that actually worked automatically without you having to do
> > anything in the code but ICBW.
> 
> I thought that it would be improper as we could be mixing ACPI methods to
> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and
> also DT properties which could conflict, like CS active high.
> 
> However I do see extra properties than "compatible" being added in DSD for
> PRP0001:
> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part)

PRP method is only for vendors to *test* the hardware in ACPI environment.
The proper method is to allocate correct ACPI ID.

Properties (_DSD in ACPI) may be used in the same way as for DT if we have no
other means in ACPI specification for them.

> And if we were to do this, I think that we would need to add some
> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW
> parsing for ACPI path - I couldn't see that.

It's okay as long as you have ACPI ID.

P.S. Most of the sensor drivers were updated in order to support ACPI PRP
method due to DIY hobbyist on IoT sector and embedded devices. This should not
be an official way how we support hardware on ACPI-based platforms.
John Garry Jan. 13, 2020, 10:09 a.m. UTC | #9
On 10/01/2020 19:31, Andy Shevchenko wrote:
> On Fri, Jan 10, 2020 at 02:58:54PM +0000, John Garry wrote:
>> On 10/01/2020 14:07, Mark Brown wrote:
>>> On Fri, Jan 10, 2020 at 11:55:37AM +0000, John Garry wrote: >
> 
> ...
> 
>>>> I will note that PRP0001+"jedec,spi-nor" compatible DSD seems to be the
>>>> defacto method to describe the SPI NOR-compat part for ACPI - that's what
>>>> I'm using. We could add properties there, but that seems improper.
>>>
>>> OK, so that's just reusing the DT binding in which case everything
>>> that's valid for the DT binding should also be valid for ACPI - I
>>> thought that actually worked automatically without you having to do
>>> anything in the code but ICBW.
>>
>> I thought that it would be improper as we could be mixing ACPI methods to
>> describe the serial bus (SPI Serial Bus Connection Resource Descriptor) and
>> also DT properties which could conflict, like CS active high.
>>
>> However I do see extra properties than "compatible" being added in DSD for
>> PRP0001:
>> https://patchwork.ozlabs.org/patch/662813/ (see EEPROM part)
> 

Hi Andy,

> PRP method is only for vendors to *test* the hardware in ACPI environment.
> The proper method is to allocate correct ACPI ID.

Yes, that would seem the proper thing to do. So the SPI NOR driver is 
based on micron m25p80 and compatible string is "jedec,spi-nor", so I 
don't know who should or would do this registration.

> 
> Properties (_DSD in ACPI) may be used in the same way as for DT if we have no
> other means in ACPI specification for them.
> 
>> And if we were to do this, I think that we would need to add some
>> device_property_read_u32("spi-rx-bus-width", ...), etc calls in the SPI FW
>> parsing for ACPI path - I couldn't see that.
> 
> It's okay as long as you have ACPI ID.

Well there is none AFAIK.

> 
> P.S. Most of the sensor drivers were updated in order to support ACPI PRP
> method due to DIY hobbyist on IoT sector and embedded devices. This should not
> be an official way how we support hardware on ACPI-based platforms.

Yeah, so we could do this. But, as I mentioned already, this could mean 
that we conflicting properties. For this the kernel driver prob should 
only pay attention to properties which ACPI cannot describe.

Even better would be to update the ACPI spec, especially for something 
general like SPI bus width

BTW, Do any of these sensors you mention have any ACPI standardization?

Thanks,
John
Mark Brown Jan. 13, 2020, 11:42 a.m. UTC | #10
On Mon, Jan 13, 2020 at 10:09:27AM +0000, John Garry wrote:
> On 10/01/2020 19:31, Andy Shevchenko wrote:

> > PRP method is only for vendors to *test* the hardware in ACPI environment.
> > The proper method is to allocate correct ACPI ID.

> Yes, that would seem the proper thing to do. So the SPI NOR driver is based
> on micron m25p80 and compatible string is "jedec,spi-nor", so I don't know
> who should or would do this registration.

The idiomatic approach appears to be for individual board vendors
to allocate IDs, you do end up with multiple IDs from multiple
vendors for the same thing.

> BTW, Do any of these sensors you mention have any ACPI standardization?

In general there's not really much standardizaiton for devices,
the bindings that do exist aren't really centrally documented and
the Windows standard is just to have the basic device
registration in the firmware and do all properties based on
quirking based on DMI information.
John Garry Jan. 13, 2020, 1:01 p.m. UTC | #11
On 13/01/2020 11:42, Mark Brown wrote:
> On Mon, Jan 13, 2020 at 10:09:27AM +0000, John Garry wrote:
>> On 10/01/2020 19:31, Andy Shevchenko wrote:
> 
>>> PRP method is only for vendors to *test* the hardware in ACPI environment.
>>> The proper method is to allocate correct ACPI ID.
> 
>> Yes, that would seem the proper thing to do. So the SPI NOR driver is based
>> on micron m25p80 and compatible string is "jedec,spi-nor", so I don't know
>> who should or would do this registration.
> 

Hi Mark,

> The idiomatic approach appears to be for individual board vendors
> to allocate IDs, you do end up with multiple IDs from multiple
> vendors for the same thing.

So we see sort of approach a lot when vendors integrate 3rd party IP 
into a SoC and then assign some vendor specific ID for that.

But I am not sure how appropriate that same approach would be for some 
3rd party memory part which we're simply wiring up on our board. Maybe 
it is.

> 
>> BTW, Do any of these sensors you mention have any ACPI standardization?
> 
> In general there's not really much standardizaiton for devices,
> the bindings that do exist aren't really centrally documented and
> the Windows standard is just to have the basic device
> registration in the firmware and do all properties based on
> quirking based on DMI information.
> 

OK, so there is always DMI. I hoped to avoid this sort of thing in the 
linux driver :)

Cheers,
John
Mark Brown Jan. 13, 2020, 2:06 p.m. UTC | #12
On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
> On 13/01/2020 11:42, Mark Brown wrote:

> > The idiomatic approach appears to be for individual board vendors
> > to allocate IDs, you do end up with multiple IDs from multiple
> > vendors for the same thing.

> But I am not sure how appropriate that same approach would be for some 3rd
> party memory part which we're simply wiring up on our board. Maybe it is.

It seems to be quite common for Intel reference designs to assign
Intel IDs to non-Intel parts on the board (which is where I
became aware of this practice).  

> > In general there's not really much standardizaiton for devices,
> > the bindings that do exist aren't really centrally documented and
> > the Windows standard is just to have the basic device
> > registration in the firmware and do all properties based on
> > quirking based on DMI information.

> OK, so there is always DMI. I hoped to avoid this sort of thing in the linux
> driver :)

Yes, there are some merits to an approach like that.
Andy Shevchenko Jan. 13, 2020, 2:17 p.m. UTC | #13
On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
> > On 13/01/2020 11:42, Mark Brown wrote:
>
> > > The idiomatic approach appears to be for individual board vendors
> > > to allocate IDs, you do end up with multiple IDs from multiple
> > > vendors for the same thing.
>
> > But I am not sure how appropriate that same approach would be for some 3rd
> > party memory part which we're simply wiring up on our board. Maybe it is.
>
> It seems to be quite common for Intel reference designs to assign
> Intel IDs to non-Intel parts on the board (which is where I
> became aware of this practice).

Basically vendor of component in question is responsible for ID, but
it seems they simple don't care.
Mark Brown Jan. 13, 2020, 2:27 p.m. UTC | #14
On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
> > > On 13/01/2020 11:42, Mark Brown wrote:

> > > > The idiomatic approach appears to be for individual board vendors
> > > > to allocate IDs, you do end up with multiple IDs from multiple
> > > > vendors for the same thing.

> > > But I am not sure how appropriate that same approach would be for some 3rd
> > > party memory part which we're simply wiring up on our board. Maybe it is.

> > It seems to be quite common for Intel reference designs to assign
> > Intel IDs to non-Intel parts on the board (which is where I
> > became aware of this practice).

> Basically vendor of component in question is responsible for ID, but
> it seems they simple don't care.

AFAICT a lot of the time it seems to be that whoever is writing
the software ends up assigning an ID, that may not be the silicon
vendor.
Andy Shevchenko Jan. 13, 2020, 2:34 p.m. UTC | #15
On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote:
> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote:
> > On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
> > > > On 13/01/2020 11:42, Mark Brown wrote:
> 
> > > > > The idiomatic approach appears to be for individual board vendors
> > > > > to allocate IDs, you do end up with multiple IDs from multiple
> > > > > vendors for the same thing.
> 
> > > > But I am not sure how appropriate that same approach would be for some 3rd
> > > > party memory part which we're simply wiring up on our board. Maybe it is.
> 
> > > It seems to be quite common for Intel reference designs to assign
> > > Intel IDs to non-Intel parts on the board (which is where I
> > > became aware of this practice).
> 
> > Basically vendor of component in question is responsible for ID, but
> > it seems they simple don't care.
> 
> AFAICT a lot of the time it seems to be that whoever is writing
> the software ends up assigning an ID, that may not be the silicon
> vendor.

...which is effectively abusing the ACPI ID allocation procedure.

(And yes, Intel itself did it in the past — see badly created ACPI IDs
 in the drivers)
John Garry Jan. 31, 2020, 10:08 a.m. UTC | #16
On 13/01/2020 14:34, Andy Shevchenko wrote:
> On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote:
>> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote:
>>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
>>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
>>>>> On 13/01/2020 11:42, Mark Brown wrote:
>>
>>>>>> The idiomatic approach appears to be for individual board vendors
>>>>>> to allocate IDs, you do end up with multiple IDs from multiple
>>>>>> vendors for the same thing.
>>
>>>>> But I am not sure how appropriate that same approach would be for some 3rd
>>>>> party memory part which we're simply wiring up on our board. Maybe it is.
>>
>>>> It seems to be quite common for Intel reference designs to assign
>>>> Intel IDs to non-Intel parts on the board (which is where I
>>>> became aware of this practice).
>>
>>> Basically vendor of component in question is responsible for ID, but
>>> it seems they simple don't care.
>>
>> AFAICT a lot of the time it seems to be that whoever is writing
>> the software ends up assigning an ID, that may not be the silicon
>> vendor.
> 
> ...which is effectively abusing the ACPI ID allocation procedure.
> 
> (And yes, Intel itself did it in the past — see badly created ACPI IDs
>   in the drivers)
> 

Hi Mark,

About this topic of ACPI having no method to describe device buswidth in 
the resource descriptor, it may be an idea for me to raise a Tianocore 
feature request @ https://bugzilla.tianocore.org/

There seems to be an avenue there for raising new features for the spec. 
I (or my org) can't participate in AWSG.

I would have no concrete proposal for spec update for now, though. 
Hopefully others with more expertise could contribute.

In the meantime, I have an RFC for using DMI to quirk support for this 
on the driver - I can share when ready.

Thanks,
John
Andy Shevchenko Jan. 31, 2020, 11:39 a.m. UTC | #17
On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote:
>
> On 13/01/2020 14:34, Andy Shevchenko wrote:
> > On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote:
> >> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote:
> >>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
> >>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
> >>>>> On 13/01/2020 11:42, Mark Brown wrote:
> >>
> >>>>>> The idiomatic approach appears to be for individual board vendors
> >>>>>> to allocate IDs, you do end up with multiple IDs from multiple
> >>>>>> vendors for the same thing.
> >>
> >>>>> But I am not sure how appropriate that same approach would be for some 3rd
> >>>>> party memory part which we're simply wiring up on our board. Maybe it is.
> >>
> >>>> It seems to be quite common for Intel reference designs to assign
> >>>> Intel IDs to non-Intel parts on the board (which is where I
> >>>> became aware of this practice).
> >>
> >>> Basically vendor of component in question is responsible for ID, but
> >>> it seems they simple don't care.
> >>
> >> AFAICT a lot of the time it seems to be that whoever is writing
> >> the software ends up assigning an ID, that may not be the silicon
> >> vendor.
> >
> > ...which is effectively abusing the ACPI ID allocation procedure.
> >
> > (And yes, Intel itself did it in the past — see badly created ACPI IDs
> >   in the drivers)
> >
>
> Hi Mark,
>
> About this topic of ACPI having no method to describe device buswidth in
> the resource descriptor, it may be an idea for me to raise a Tianocore
> feature request @ https://bugzilla.tianocore.org/
>

The 19.6.126 describes the SPI resource, in particular:

---8<---8<---
DataBitLength is the size, in bits, of the smallest transfer unit for
this connection. _LEN is automatically
created to refer to this portion of the resource descriptor.
---8<---8<---

Is it what you are looking for? (As far as I know most of the
firmwares simple abuse this field among others)

> There seems to be an avenue there for raising new features for the spec.
> I (or my org) can't participate in AWSG.

But have you read 19.6.126?

> I would have no concrete proposal for spec update for now, though.
> Hopefully others with more expertise could contribute.
John Garry Jan. 31, 2020, 12:03 p.m. UTC | #18
On 31/01/2020 11:39, Andy Shevchenko wrote:
> On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote:
>>
>> On 13/01/2020 14:34, Andy Shevchenko wrote:
>>> On Mon, Jan 13, 2020 at 02:27:54PM +0000, Mark Brown wrote:
>>>> On Mon, Jan 13, 2020 at 04:17:32PM +0200, Andy Shevchenko wrote:
>>>>> On Mon, Jan 13, 2020 at 4:07 PM Mark Brown <broonie@kernel.org> wrote:
>>>>>> On Mon, Jan 13, 2020 at 01:01:06PM +0000, John Garry wrote:
>>>>>>> On 13/01/2020 11:42, Mark Brown wrote:
>>>>
>>>>>>>> The idiomatic approach appears to be for individual board vendors
>>>>>>>> to allocate IDs, you do end up with multiple IDs from multiple
>>>>>>>> vendors for the same thing.
>>>>
>>>>>>> But I am not sure how appropriate that same approach would be for some 3rd
>>>>>>> party memory part which we're simply wiring up on our board. Maybe it is.
>>>>
>>>>>> It seems to be quite common for Intel reference designs to assign
>>>>>> Intel IDs to non-Intel parts on the board (which is where I
>>>>>> became aware of this practice).
>>>>
>>>>> Basically vendor of component in question is responsible for ID, but
>>>>> it seems they simple don't care.
>>>>
>>>> AFAICT a lot of the time it seems to be that whoever is writing
>>>> the software ends up assigning an ID, that may not be the silicon
>>>> vendor.
>>>
>>> ...which is effectively abusing the ACPI ID allocation procedure.
>>>
>>> (And yes, Intel itself did it in the past — see badly created ACPI IDs
>>>    in the drivers)
>>>
>>
>> Hi Mark,
>>

Hi Andy,

>> About this topic of ACPI having no method to describe device buswidth in
>> the resource descriptor, it may be an idea for me to raise a Tianocore
>> feature request @ https://bugzilla.tianocore.org/
>>
> 
> The 19.6.126 describes the SPI resource, in particular:
> 
> ---8<---8<---
> DataBitLength is the size, in bits, of the smallest transfer unit for
> this connection. _LEN is automatically
> created to refer to this portion of the resource descriptor.
> ---8<---8<---
> 
> Is it what you are looking for? (As far as I know most of the
> firmwares simple abuse this field among others)

I didn't think so - I thought that there was a distinction between width 
and length in SPI terms.

So how do you find that most firmwares abuse this field? AFAICS, linux 
kernel doesn't interpret this field at all.

> 
>> There seems to be an avenue there for raising new features for the spec.
>> I (or my org) can't participate in AWSG.
> 
> But have you read 19.6.126?
> 

Maybe some clarification at least could be achieved :)

Cheers,
John
Andy Shevchenko Jan. 31, 2020, 3:46 p.m. UTC | #19
On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote:
> On 31/01/2020 11:39, Andy Shevchenko wrote:
> > On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote:
> >> On 13/01/2020 14:34, Andy Shevchenko wrote:

...

> >> About this topic of ACPI having no method to describe device buswidth in
> >> the resource descriptor, it may be an idea for me to raise a Tianocore
> >> feature request @ https://bugzilla.tianocore.org/
> >>
> >
> > The 19.6.126 describes the SPI resource, in particular:
> >
> > ---8<---8<---
> > DataBitLength is the size, in bits, of the smallest transfer unit for
> > this connection. _LEN is automatically
> > created to refer to this portion of the resource descriptor.
> > ---8<---8<---
> >
> > Is it what you are looking for? (As far as I know most of the
> > firmwares simple abuse this field among others)
>
> I didn't think so - I thought that there was a distinction between width
> and length in SPI terms.

My interpretation of this field is a data width of the slave.
Basically what we have as transfer->size inside SPI in the Linux
kernel.

> So how do you find that most firmwares abuse this field? AFAICS, linux
> kernel doesn't interpret this field at all.

From all tables I have this is the result of appearance (some of the
tables are like 10x times present in my data base, but nevertheless)

    140 SpiSerialBusV2(0x0000,PolarityHigh,FourWireMode,0x08,
   411 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08,
     1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08,
    36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10,
    35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18,
    35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20,
     1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10,
     8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08,
     1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10,

So, it seems I stand corrected, the field is in right use, although
cases like 0x10 and 0x20 should be carefully checked.

We may teach kernel to get something meaningful out of it.
John Garry Jan. 31, 2020, 4:26 p.m. UTC | #20
On 31/01/2020 15:46, Andy Shevchenko wrote:
> On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote:
>> On 31/01/2020 11:39, Andy Shevchenko wrote:
>>> On Fri, Jan 31, 2020 at 12:08 PM John Garry <john.garry@huawei.com> wrote:
>>>> On 13/01/2020 14:34, Andy Shevchenko wrote:
> 
> ...
> 
>>>> About this topic of ACPI having no method to describe device buswidth in
>>>> the resource descriptor, it may be an idea for me to raise a Tianocore
>>>> feature request @ https://bugzilla.tianocore.org/
>>>>
>>>
>>> The 19.6.126 describes the SPI resource, in particular:
>>>
>>> ---8<---8<---
>>> DataBitLength is the size, in bits, of the smallest transfer unit for
>>> this connection. _LEN is automatically
>>> created to refer to this portion of the resource descriptor.
>>> ---8<---8<---
>>>

Hi Andy,

>>> Is it what you are looking for? (As far as I know most of the
>>> firmwares simple abuse this field among others)
>>
>> I didn't think so - I thought that there was a distinction between width
>> and length in SPI terms.
> 
> My interpretation of this field is a data width of the slave.
> Basically what we have as transfer->size inside SPI in the Linux
> kernel.
> 
>> So how do you find that most firmwares abuse this field? AFAICS, linux
>> kernel doesn't interpret this field at all.
> 
>>From all tables I have this is the result of appearance (some of the
> tables are like 10x times present in my data base, but nevertheless)
> 
>      140 SpiSerialBusV2(0x0000,PolarityHigh,FourWireMode,0x08,
>     411 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08,
>       1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08,
>      36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10,
>      35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18,
>      35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20,
>       1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10,
>       8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08,
>       1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10,
> 
> So, it seems I stand corrected, the field is in right use, although
> cases like 0x10 and 0x20 should be carefully checked.
> 
> We may teach kernel to get something meaningful out of it.
> 

It seems that someone already had a go at that:
https://lore.kernel.org/lkml/20170317212143.bogj6efzyvvf24yd@sirena.org.uk/

Thanks,
John
Mark Brown Feb. 1, 2020, 11:32 a.m. UTC | #21
On Fri, Jan 31, 2020 at 05:46:39PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 31, 2020 at 2:03 PM John Garry <john.garry@huawei.com> wrote:
> > On 31/01/2020 11:39, Andy Shevchenko wrote:

> > > DataBitLength is the size, in bits, of the smallest transfer unit for
> > > this connection. _LEN is automatically
> > > created to refer to this portion of the resource descriptor.

> > > Is it what you are looking for? (As far as I know most of the
> > > firmwares simple abuse this field among others)

> > I didn't think so - I thought that there was a distinction between width
> > and length in SPI terms.

> My interpretation of this field is a data width of the slave.
> Basically what we have as transfer->size inside SPI in the Linux
> kernel.

This discussion is about the number of data lines, SPI_TX_QUAD
and friends.

>      1 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x08,
>     36 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x10,
>     35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x18,
>     35 SpiSerialBusV2(0x0000,PolarityLow,FourWireMode,0x20,
>      1 SpiSerialBusV2(0x0000,PolarityLow,ThreeWireMode,0x10,
>      8 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x08,
>      1 SpiSerialBusV2(0x0001,PolarityLow,FourWireMode,0x10,

> So, it seems I stand corrected, the field is in right use, although
> cases like 0x10 and 0x20 should be carefully checked.

Those look like they're mainly controlling SPI_3WIRE so it does
look like a reasonable fit, yes.
Mark Brown Feb. 1, 2020, 11:34 a.m. UTC | #22
On Fri, Jan 31, 2020 at 04:26:46PM +0000, John Garry wrote:
> On 31/01/2020 15:46, Andy Shevchenko wrote:

> > So, it seems I stand corrected, the field is in right use, although
> > cases like 0x10 and 0x20 should be carefully checked.

> > We may teach kernel to get something meaningful out of it.

> It seems that someone already had a go at that:
> https://lore.kernel.org/lkml/20170317212143.bogj6efzyvvf24yd@sirena.org.uk/

This is a discussion about supporting DT bindings for
bits-per-word which is a different thing again, that's the size
of a data word which is not connected with the physical wiring.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.
I do frequently catch up on my mail on flights or while otherwise
travelling so this is even more pressing for me than just being about
making things a bit easier to read.
diff mbox series

Patch

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 870f7797b56b..d6ed0c355954 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -281,6 +281,15 @@  config SPI_FSL_QUADSPI
 	  This controller does not support generic SPI messages. It only
 	  supports the high-level SPI memory interface.
 
+config SPI_HISI_SFC_V3XX
+	tristate "HiSilicon SPI-NOR Flash Controller for Hi16XX chipsets"
+	depends on (ARM64 && ACPI) || COMPILE_TEST
+	depends on HAS_IOMEM
+	select CONFIG_MTD_SPI_NOR
+	help
+	  This enables support for HiSilicon v3xx SPI-NOR flash controller
+	  found in hi16xx chipsets.
+
 config SPI_NXP_FLEXSPI
 	tristate "NXP Flex SPI controller"
 	depends on ARCH_LAYERSCAPE || HAS_IOMEM
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index bb49c9e6d0a0..9b65ec5afc5e 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -48,6 +48,7 @@  obj-$(CONFIG_SPI_FSL_LPSPI)		+= spi-fsl-lpspi.o
 obj-$(CONFIG_SPI_FSL_QUADSPI)		+= spi-fsl-qspi.o
 obj-$(CONFIG_SPI_FSL_SPI)		+= spi-fsl-spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi-gpio.o
+obj-$(CONFIG_SPI_HISI_SFC_V3XX)		+= spi-hisi-sfc-v3xx.o
 obj-$(CONFIG_SPI_IMG_SPFI)		+= spi-img-spfi.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
diff --git a/drivers/spi/spi-hisi-sfc-v3xx.c b/drivers/spi/spi-hisi-sfc-v3xx.c
new file mode 100644
index 000000000000..4cf8fc80a7b7
--- /dev/null
+++ b/drivers/spi/spi-hisi-sfc-v3xx.c
@@ -0,0 +1,284 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// HiSilicon SPI NOR V3XX Flash Controller Driver for hi16xx chipsets
+//
+// Copyright (c) 2019 HiSilicon Technologies Co., Ltd.
+// Author: John Garry <john.garry@huawei.com>
+
+#include <linux/acpi.h>
+#include <linux/bitops.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
+
+#define HISI_SFC_V3XX_VERSION (0x1f8)
+
+#define HISI_SFC_V3XX_CMD_CFG (0x300)
+#define HISI_SFC_V3XX_CMD_CFG_DATA_CNT_OFF 9
+#define HISI_SFC_V3XX_CMD_CFG_RW_MSK BIT(8)
+#define HISI_SFC_V3XX_CMD_CFG_DATA_EN_MSK BIT(7)
+#define HISI_SFC_V3XX_CMD_CFG_DUMMY_CNT_OFF 4
+#define HISI_SFC_V3XX_CMD_CFG_ADDR_EN_MSK BIT(3)
+#define HISI_SFC_V3XX_CMD_CFG_CS_SEL_OFF 1
+#define HISI_SFC_V3XX_CMD_CFG_START_MSK BIT(0)
+#define HISI_SFC_V3XX_CMD_INS (0x308)
+#define HISI_SFC_V3XX_CMD_ADDR (0x30c)
+#define HISI_SFC_V3XX_CMD_DATABUF0 (0x400)
+
+struct hisi_sfc_v3xx_host {
+	struct device *dev;
+	void __iomem *regbase;
+	int max_cmd_dword;
+};
+
+#define HISI_SFC_V3XX_WAIT_TIMEOUT_US		1000000
+#define HISI_SFC_V3XX_WAIT_POLL_INTERVAL_US	10
+
+static int hisi_sfc_v3xx_wait_cmd_idle(struct hisi_sfc_v3xx_host *host)
+{
+	u32 reg;
+
+	return readl_poll_timeout(host->regbase + HISI_SFC_V3XX_CMD_CFG, reg,
+				  !(reg & HISI_SFC_V3XX_CMD_CFG_START_MSK),
+				  HISI_SFC_V3XX_WAIT_POLL_INTERVAL_US,
+				  HISI_SFC_V3XX_WAIT_TIMEOUT_US);
+}
+
+static int hisi_sfc_v3xx_adjust_op_size(struct spi_mem *mem,
+					struct spi_mem_op *op)
+{
+	struct spi_device *spi = mem->spi;
+	struct hisi_sfc_v3xx_host *host;
+	uintptr_t addr = (uintptr_t)op->data.buf.in;
+	int max_byte_count;
+
+	host = spi_controller_get_devdata(spi->master);
+
+	max_byte_count = host->max_cmd_dword * 4;
+
+	if (!IS_ALIGNED(addr, 4) && op->data.nbytes >= 4)
+		op->data.nbytes = 4 - (addr % 4);
+	else if (op->data.nbytes > max_byte_count)
+		op->data.nbytes = max_byte_count;
+
+	return 0;
+}
+
+/*
+ * memcpy_{to,from}io doesn't gurantee 32b accesses - which we require for the
+ * DATABUF registers -so use __io{read,write}32_copy when possible. For
+ * trailing bytes, copy them byte-by-byte from the DATABUF register, as we
+ * can't clobber outside the source/dest buffer.
+ *
+ * For efficient data read/write, we try to put any start 32b unaligned data
+ * into a separate transaction in hisi_sfc_v3xx_adjust_op_size().
+ */
+static void hisi_sfc_v3xx_read_databuf(struct hisi_sfc_v3xx_host *host,
+				       u8 *to, unsigned int len)
+{
+	void __iomem *from;
+	int i;
+
+	from = host->regbase + HISI_SFC_V3XX_CMD_DATABUF0;
+
+	if (IS_ALIGNED((uintptr_t)to, 4)) {
+		int words = len / 4;
+
+		__ioread32_copy(to, from, words);
+
+		len -= words * 4;
+		if (len) {
+			u32 val;
+
+			to += words * 4;
+			from += words * 4;
+
+			val = __raw_readl(from);
+
+			for (i = 0; i < len; i++, val >>= 8, to++)
+				*to = (u8)val;
+		}
+	} else {
+		for (i = 0; i < DIV_ROUND_UP(len, 4); i++, from += 4) {
+			u32 val = __raw_readl(from);
+			int j;
+
+			for (j = 0; j < 4 && (j + (i * 4) < len);
+			     to++, val >>= 8, j++)
+				*to = (u8)val;
+		}
+	}
+}
+
+static void hisi_sfc_v3xx_write_databuf(struct hisi_sfc_v3xx_host *host,
+					const u8 *from, unsigned int len)
+{
+	void __iomem *to;
+	int i;
+
+	to = host->regbase + HISI_SFC_V3XX_CMD_DATABUF0;
+
+	if (IS_ALIGNED((uintptr_t)from, 4)) {
+		int words = len / 4;
+
+		__iowrite32_copy(to, from, words);
+
+		len -= words * 4;
+		if (len) {
+			u32 val = 0;
+
+			to += words * 4;
+			from += words * 4;
+
+			for (i = 0; i < len; i++, from++)
+				val |= *from << i * 8;
+			__raw_writel(val, to);
+		}
+
+	} else {
+		for (i = 0; i < DIV_ROUND_UP(len, 4); i++, to += 4) {
+			u32 val = 0;
+			int j;
+
+			for (j = 0; j < 4 && (j + (i * 4) < len);
+			     from++, j++)
+				val |= *from << j * 8;
+			__raw_writel(val, to);
+		}
+	}
+}
+
+static int hisi_sfc_v3xx_generic_exec_op(struct hisi_sfc_v3xx_host *host,
+					 const struct spi_mem_op *op,
+					 u8 chip_select)
+{
+	int ret, len = op->data.nbytes;
+	u32 config = 0;
+
+	if (op->addr.nbytes)
+		config |= HISI_SFC_V3XX_CMD_CFG_ADDR_EN_MSK;
+
+	if (op->data.dir != SPI_MEM_NO_DATA) {
+		config |= (len - 1) << HISI_SFC_V3XX_CMD_CFG_DATA_CNT_OFF;
+		config |= HISI_SFC_V3XX_CMD_CFG_DATA_EN_MSK;
+	}
+
+	if (op->data.dir == SPI_MEM_DATA_OUT)
+		hisi_sfc_v3xx_write_databuf(host, op->data.buf.out, len);
+	else if (op->data.dir == SPI_MEM_DATA_IN)
+		config |= HISI_SFC_V3XX_CMD_CFG_RW_MSK;
+
+	config |= op->dummy.nbytes << HISI_SFC_V3XX_CMD_CFG_DUMMY_CNT_OFF |
+		  chip_select << HISI_SFC_V3XX_CMD_CFG_CS_SEL_OFF |
+		  HISI_SFC_V3XX_CMD_CFG_START_MSK;
+
+	writel(op->addr.val, host->regbase + HISI_SFC_V3XX_CMD_ADDR);
+	writel(op->cmd.opcode, host->regbase + HISI_SFC_V3XX_CMD_INS);
+
+	writel(config, host->regbase + HISI_SFC_V3XX_CMD_CFG);
+
+	ret = hisi_sfc_v3xx_wait_cmd_idle(host);
+	if (ret)
+		return ret;
+
+	if (op->data.dir == SPI_MEM_DATA_IN)
+		hisi_sfc_v3xx_read_databuf(host, op->data.buf.in, len);
+
+	return 0;
+}
+
+static int hisi_sfc_v3xx_exec_op(struct spi_mem *mem,
+				 const struct spi_mem_op *op)
+{
+	struct hisi_sfc_v3xx_host *host;
+	struct spi_device *spi = mem->spi;
+	u8 chip_select = spi->chip_select;
+
+	host = spi_controller_get_devdata(spi->master);
+
+	return hisi_sfc_v3xx_generic_exec_op(host, op, chip_select);
+}
+
+static const struct spi_controller_mem_ops hisi_sfc_v3xx_mem_ops = {
+	.adjust_op_size = hisi_sfc_v3xx_adjust_op_size,
+	.exec_op = hisi_sfc_v3xx_exec_op,
+};
+
+static int hisi_sfc_v3xx_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct hisi_sfc_v3xx_host *host;
+	struct spi_controller *ctlr;
+	u32 version;
+	int ret;
+
+	ctlr = spi_alloc_master(&pdev->dev, sizeof(*host));
+	if (!ctlr)
+		return -ENOMEM;
+
+	ctlr->mode_bits = SPI_RX_DUAL | SPI_RX_QUAD |
+			  SPI_TX_DUAL | SPI_TX_QUAD;
+
+	host = spi_controller_get_devdata(ctlr);
+	host->dev = dev;
+
+	platform_set_drvdata(pdev, host);
+
+	host->regbase = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(host->regbase)) {
+		ret = PTR_ERR(host->regbase);
+		goto err_put_master;
+	}
+
+	ctlr->bus_num = -1;
+	ctlr->num_chipselect = 1;
+	ctlr->mem_ops = &hisi_sfc_v3xx_mem_ops;
+
+	version = readl(host->regbase + HISI_SFC_V3XX_VERSION);
+
+	switch (version) {
+	case 0x351:
+		host->max_cmd_dword = 64;
+		break;
+	default:
+		host->max_cmd_dword = 16;
+		break;
+	}
+
+	ret = devm_spi_register_controller(dev, ctlr);
+	if (ret)
+		goto err_put_master;
+
+	dev_info(&pdev->dev, "hw version 0x%x\n", version);
+
+	return 0;
+
+err_put_master:
+	spi_master_put(ctlr);
+	return ret;
+}
+
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id hisi_sfc_v3xx_acpi_ids[] = {
+	{"HISI0341", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, hisi_sfc_v3xx_acpi_ids);
+#endif
+
+static struct platform_driver hisi_sfc_v3xx_spi_driver = {
+	.driver = {
+		.name	= "hisi-sfc-v3xx",
+		.acpi_match_table = ACPI_PTR(hisi_sfc_v3xx_acpi_ids),
+	},
+	.probe	= hisi_sfc_v3xx_probe,
+};
+
+module_platform_driver(hisi_sfc_v3xx_spi_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("John Garry <john.garry@huawei.com>");
+MODULE_DESCRIPTION("HiSilicon SPI NOR V3XX Flash Controller Driver for hi16xx chipsets");